Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: check validity of trace- and span-id in context update from header #1046

Merged
merged 14 commits into from
Oct 7, 2024

Conversation

JoshuaMoelans
Copy link
Member

@JoshuaMoelans JoshuaMoelans commented Oct 2, 2024

Analysis of #1012 showed a potential way for users to send invalid trace and span id values. This can happen when a customer uses sentry_transaction_context_update_from_header(...) which reads a string that looks like traceid-spanid(-sampled)?. The only formatting that currently gets checked is whether there is a first -.

Although these are never used in an unsafe context within our code, it still makes sense to reject the invalid formats on our end rather than relying on Relay to reject them.

The formats we expect are the following (from our docs):

  • trace_id = 32 hex characters
  • span_id = 16 hex characters

ToDo

  • Make sure we agree on this approach
  • Add a warning mechanism when an invalid input is received
  • Update the relevant docs page

Copy link

github-actions bot commented Oct 2, 2024

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 271b06f

Copy link

codecov bot commented Oct 2, 2024

Codecov Report

Attention: Patch coverage is 96.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 81.78%. Comparing base (dbb9580) to head (271b06f).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1046      +/-   ##
==========================================
- Coverage   83.33%   81.78%   -1.56%     
==========================================
  Files          53       53              
  Lines        6338     6363      +25     
  Branches     1198     1207       +9     
==========================================
- Hits         5282     5204      -78     
- Misses       1042     1046       +4     
- Partials       14      113      +99     

@JoshuaMoelans JoshuaMoelans marked this pull request as ready for review October 4, 2024 09:29
@JoshuaMoelans
Copy link
Member Author

ToDo: update the docs (think it is this page) with a valid header.

CHANGELOG.md Show resolved Hide resolved
src/sentry_tracing.c Outdated Show resolved Hide resolved
src/sentry_tracing.c Outdated Show resolved Hide resolved
src/sentry_tracing.c Outdated Show resolved Hide resolved
src/sentry_tracing.c Show resolved Hide resolved
@supervacuus
Copy link
Collaborator

I went ahead with the merge and created a docs issue as a follow-up to your todo item: getsentry/sentry-docs#11504

@supervacuus supervacuus merged commit 65bfb62 into master Oct 7, 2024
23 checks passed
@supervacuus supervacuus deleted the test/invalid_traceid_tests_Joshua branch October 7, 2024 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants