Sponsored Content
Skip to content

refactor: Improve Forwarded header field parsing - #7126

Merged
bthomee merged 1 commit into
developfrom
lmaisons/refactor-for-header
May 13, 2026
Merged

refactor: Improve Forwarded header field parsing#7126
bthomee merged 1 commit into
developfrom
lmaisons/refactor-for-header

Conversation

@lmaisons

@lmaisons lmaisons commented May 12, 2026

Copy link
Copy Markdown
Contributor

Improve the Forwarded header field parsing in forwardedFor():

  • Compute the correct remaining length using pointer arithmetic consistent with the rest of the function
  • Add a directive-boundary predicate so for= is only matched at the start of the value or after a field delimiter per RFC 7239

@xrplf-ai-reviewer xrplf-ai-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No issues.

Review by Claude Opus 4.6 · Prompt: V15

@bthomee bthomee added this to the 3.1.3 (develop) milestone May 12, 2026
@codecov

codecov Bot commented May 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.47368% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.1%. Comparing base (aa55392) to head (e11cc5d).
⚠️ Report is 12 commits behind head on develop.

Files with missing lines Patch % Lines
src/xrpld/rpc/detail/Role.cpp 89.5% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #7126   +/-   ##
=======================================
  Coverage     82.1%   82.1%           
=======================================
  Files         1010    1010           
  Lines        76064   76070    +6     
  Branches      7375    7370    -5     
=======================================
+ Hits         62470   62481   +11     
+ Misses       13594   13589    -5     
Files with missing lines Coverage Δ
src/xrpld/rpc/detail/Role.cpp 91.4% <89.5%> (-0.4%) ⬇️

... and 3 files with indirect coverage changes

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@lmaisons
lmaisons force-pushed the lmaisons/refactor-for-header branch from 4b5cf46 to 17f5112 Compare May 12, 2026 17:10

@xrplf-ai-reviewer xrplf-ai-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No issues.

Review by Claude Opus 4.6 · Prompt: V15

@lmaisons
lmaisons force-pushed the lmaisons/refactor-for-header branch from 17f5112 to 803376b Compare May 12, 2026 17:32

@xrplf-ai-reviewer xrplf-ai-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No issues.

Review by Claude Opus 4.6 · Prompt: V15

@lmaisons
lmaisons force-pushed the lmaisons/refactor-for-header branch from 803376b to e11cc5d Compare May 12, 2026 17:34

@xrplf-ai-reviewer xrplf-ai-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No issues.

Review by Claude Opus 4.6 · Prompt: V15

@pratikmankawde pratikmankawde left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can add tokenizing per RFC 7239 grammar instead of patching the std::search loop:

  • Split on , boundaries (forwarded-element list)
  • Trim white-spaces
  • Split each element on ; (parameter list)
  • Match directive name case-insensitively

The current approach (search + boundary-check + retry loop) works for the known cases, but it's fragile — any future directive containing the substring "for=" would require another boundary fix. A proper tokenizer eliminates this class of bugs entirely and is only marginally more code. Why do the same fix twice across two branches without addressing the structural weakness?

@a1q123456 a1q123456 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

reapproving

@godexsoft godexsoft left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd recommend to address @pratikmankawde comment in a follow up PR 👍

@bthomee

bthomee commented May 13, 2026

Copy link
Copy Markdown
Collaborator

@lmaisons is this ready to merge?

@bthomee bthomee added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label May 13, 2026
@bthomee
bthomee added this pull request to the merge queue May 13, 2026
Merged via the queue into develop with commit aa5e4ff May 13, 2026
3 checks passed
@bthomee
bthomee deleted the lmaisons/refactor-for-header branch May 13, 2026 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants