10000 Parse authorization header by estringana · Pull Request #3279 · DataDog/dd-trace-php · GitHub
[go: up one dir, main page]

Skip to content

Parse authorization header #3279

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

Merged
merged 7 commits into from
Jun 26, 2025
Merged

Conversation

estringana
Copy link
Contributor

Description

Parse authorization header

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

@codecov-commenter
Copy link
codecov-commenter commented May 29, 2025

Codecov Report

Attention: Patch coverage is 18.18182% with 9 lines in your changes missing coverage. Please review.

Project coverage is 73.23%. Comparing base (7954d35) to head (0bbf0a0).

Files with missing lines Patch % Lines
appsec/src/extension/commands/request_init.c 0.00% 4 Missing and 1 partial ⚠️
appsec/src/extension/tags.c 33.33% 3 Missing and 1 partial ⚠️

❌ Your patch status has failed because the patch coverage (18.18%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

❗ There is a different number of reports uploaded between BASE (7954d35) and HEAD (0bbf0a0). Click for more details.

HEAD has 13 uploads less than BASE
Flag BASE (7954d35) HEAD (0bbf0a0)
tracer-php 24 11
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3279      +/-   ##
============================================
- Coverage     79.23%   73.23%   -6.01%     
  Complexity     2969     2969              
============================================
  Files           119      146      +27     
  Lines         11685    16145    +4460     
  Branches          0     1114    +1114     
============================================
+ Hits           9259    11823    +2564     
- Misses         2426     3743    +1317     
- Partials          0      579     +579     
Flag Coverage Δ
appsec-extension 68.58% <18.18%> (?)
tracer-php 75.00% <ø> (-4.24%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
appsec/src/extension/tags.c 80.28% <33.33%> (ø)
appsec/src/extension/commands/request_init.c 86.20% <0.00%> (ø)

... and 32 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7954d35...0bbf0a0. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pr-commenter
Copy link
pr-commenter bot commented May 29, 2025

Benchmarks [ appsec ]

Benchmark execution time: 2025-06-26 12:13:13

Comparing candidate commit 6c31010 in PR branch estringana/parse-digest-header with baseline commit 6c39fcd in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 12 metrics, 0 unstable metrics.

@estringana estringana force-pushed the estringana/parse-digest-header branch from ff8a41e to 0bbf0a0 Compare May 30, 2025 13:33
Copy link
Contributor
@Anilm3 Anilm3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scanner test only requires the header schema no the header itself as a tag, this needs to be passed to libddwaf as part of the server.request.headers.no_cookies address.

We should certainly not be sending this in the trace.

@estringana estringana force-pushed the estringana/parse-digest-header branch 2 times, most recently from ec06df7 to d2e7536 Compare June 17, 2025 10:58
Copy link
Contributor
@Anilm3 Anilm3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM although I'd add a bit more in the description as this only covers digest authorization scheme but not others such as basic or bearer

@Anilm3 Anilm3 marked this pull request as ready for review June 18, 2025 09:44
@Anilm3 Anilm3 requested a review from a team as a code owner June 18, 2025 09:44
@estringana estringana force-pushed the estringana/parse-digest-header branch from 32bf10a to e8671b6 Compare June 25, 2025 08:45
@estringana estringana force-pushed the estringana/parse-digest-header branch from e8671b6 to 69aada0 Compare June 26, 2025 10:15
@estringana estringana merged commit 46fc08a into master Jun 26, 2025
1816 of 1882 checks passed
@estringana estringana deleted the estringana/parse-digest-header branch June 26, 2025 13:20
@github-actions github-actions bot added this to the 1.11.0 milestone Jun 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0