feat: Typed forwarded header #101
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe changes introduce comprehensive support for the HTTP "Forwarded" header, including its parsing, typed representation, serialization, and integration into the headers management system. Extensions and accessors for "Forwarded" are added, and thorough unit tests are implemented. Additionally, a workflow step is updated to conditionally run only for non-forked pull requests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Headers
participant ForwardedHeader
Client->>Headers: Access Headers.forwarded
Headers->>ForwardedHeader: Parse/serialize Forwarded header value(s)
ForwardedHeader-->>Headers: Structured ForwardedHeader object
Headers-->>Client: Return typed ForwardedHeader (get/set)
sequenceDiagram
participant GitHub Actions
participant Workflow Step
GitHub Actions->>Workflow Step: Evaluate condition (PR from fork?)
alt Not from fork
Workflow Step->>Workflow Step: Run "Push Benchmark Data"
else From fork
Workflow Step-->>GitHub Actions: Skip step
end
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
nielsenko
force-pushed
the
forwarded-header
branch
from
52998ab to
e842519
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #101 +/- ##
==========================================
+ Coverage 82.51% 82.73% +0.22%
==========================================
Files 81 82 +1
Lines 2522 2659 +137
Branches 1408 1466 +58
==========================================
+ Hits 2081 2200 +119
- Misses 441 459 +18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
lib/src/headers/typed/typed_headers.dart (1)
26-26: Keep exports alphabetically sorted for quick scanningNit, but keeping the list strictly alphabetical (or otherwise consistently grouped) makes locating a header module painless.
forwarded_header.dartshould be moved so it sits betweenexpect_header.dartandfrom_header.dart.lib/src/headers/standard_headers_extensions.dart (2)
4-4: Redundant direct import
typed/typed_headers.dartalready re-exportsforwarded_header.dart, so this extra import is unnecessary and leaks an internal path.-import 'typed/headers/forwarded_header.dart';Removing it avoids duplicate imports and keeps layering clear.
90-91: New getters/setters added out of alphabetical blockThe new
forwardedaccessor is appended to the end of each list, breaking the otherwise alphabetic ordering that accelerates visual lookup.Moving the getter/setter so they appear after
crossOriginOpenerPolicywould restore order and consistency.Also applies to: 201-202, 289-290
lib/src/headers/headers.dart (1)
7-8: Potentially avoid direct import to prevent future cyclesIf feasible, import
typed/typed_headers.dartinstead of the leaf file – it already exposesForwardedHeader, and doing so keeps this file independent of the internal folder layout.Only take this if it does not introduce circular dependencies (it shouldn’t in current graph, but worth a quick check).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/benchmark.yaml(1 hunks)lib/src/headers/headers.dart(4 hunks)lib/src/headers/standard_headers_extensions.dart(4 hunks)lib/src/headers/typed/headers/forwarded_header.dart(1 hunks)lib/src/headers/typed/typed_headers.dart(1 hunks)test/headers/typed_headers/forwarded_header_test.dart(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
lib/src/headers/typed/headers/forwarded_header.dart
[warning] 107-107: lib/src/headers/typed/headers/forwarded_header.dart#L107
Added line #L107 was not covered by tests
[warning] 110-110: lib/src/headers/typed/headers/forwarded_header.dart#L110
Added line #L110 was not covered by tests
[warning] 143-147: lib/src/headers/typed/headers/forwarded_header.dart#L143-L147
Added lines #L143 - L147 were not covered by tests
[warning] 149-149: lib/src/headers/typed/headers/forwarded_header.dart#L149
Added line #L149 was not covered by tests
[warning] 152-159: lib/src/headers/typed/headers/forwarded_header.dart#L152-L159
Added lines #L152 - L159 were not covered by tests
[warning] 166-166: lib/src/headers/typed/headers/forwarded_header.dart#L166
Added line #L166 was not covered by tests
[warning] 180-180: lib/src/headers/typed/headers/forwarded_header.dart#L180
Added line #L180 was not covered by tests
[warning] 296-296: lib/src/headers/typed/headers/forwarded_header.dart#L296
Added line #L296 was not covered by tests
[warning] 298-298: lib/src/headers/typed/headers/forwarded_header.dart#L298
Added line #L298 was not covered by tests
[warning] 301-301: lib/src/headers/typed/headers/forwarded_header.dart#L301
Added line #L301 was not covered by tests
[warning] 304-304: lib/src/headers/typed/headers/forwarded_header.dart#L304
Added line #L304 was not covered by tests
lib/src/headers/standard_headers_extensions.dart
[warning] 202-202: lib/src/headers/standard_headers_extensions.dart#L202
Added line #L202 was not covered by tests
🔇 Additional comments (3)
.github/workflows/benchmark.yaml (1)
49-51:if:guard can fail on non-PR events
github.event.pull_requestis only defined forpull_requestevents.
When the workflow is triggered bypush,schedule, orworkflow_dispatch, this expression evaluates tonull == false, which still runs the step and the subsequentgit pushwill fail on forks.Consider an explicit event gate:
-if: github.event.pull_request.head.repo.fork == false +if: > + github.event_name != 'pull_request' || + github.event.pull_request.head.repo.fork == falselib/src/headers/headers.dart (1)
249-251: Accessor and constant look goodThe accessor, inclusion in
_requestOnly, and constant definition are consistent with the style used for other headers. Nice addition.Also applies to: 307-308, 367-368
test/headers/typed_headers/forwarded_header_test.dart (1)
1-425: Excellent, exhaustive test coverageThe test matrix is thorough and clearly documents current limitations via
skip. Great work!
nielsenko
force-pushed
the
forwarded-header
branch
from
2f7c67e to
b3c6fb2
Compare
SandPod
left a comment
SandPod
left a comment
There was a problem hiding this comment.
A couple of questions regarding the quoiting support.
Do we already have support for the X-Forwarded-For header?
nielsenko
force-pushed
the
forwarded-header
branch
from
ef2e5e9 to
50f0264
Compare
No, not as a typed header. @coderabbitai make an issue for this. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@nielsenko I'll create an issue for adding support for the 🏁 Script executed: Length of output: 2096 Let me fix the shell script issue and create the GitHub issue properly: X-Forwarded-For: client, proxy1, proxy2 🏁 Script executed: Length of output: 150 Let me fix the heredoc issue and create the GitHub issue: 🏁 Script executed: Length of output: 1291 Perfect! I've successfully created GitHub issue #104 for adding support for the
You can view the issue at: #104 |
2 participants
Description
This PR introduces support for the
ForwardedHTTP header (RFC 7239).It includes:
ForwardedHeader,ForwardedElement,ForwardedIdentifier) for parsing and representing the header.ForwardedHeaderimplementation.Related Issues
Pre-Launch Checklist
Please ensure that your PR meets the following requirements before submitting:
///), ensuring consistency with existing project documentation.Breaking Changes
Additional Notes
This is to support client indentification in serverpod (supersedes #98)
Summary by CodeRabbit
New Features
Tests
Chores