feat: X-Forwarded-for typed header #107
Conversation
📝 WalkthroughWalkthroughThe changes introduce a new typed header class for Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Headers
participant XForwardedForHeader
Client->>Headers: Set X-Forwarded-For header (raw value)
Headers->>XForwardedForHeader: Parse raw header value(s)
XForwardedForHeader->>XForwardedForHeader: Split, trim, validate addresses
XForwardedForHeader-->>Headers: Return typed header instance
Headers-->>Client: Provide access to parsed addresses list
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related issues
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #107 +/- ##
==========================================
+ Coverage 82.73% 82.77% +0.04%
==========================================
Files 82 83 +1
Lines 2659 2677 +18
Branches 1466 1475 +9
==========================================
+ Hits 2200 2216 +16
- Misses 459 461 +2 ☔ 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: 0
🧹 Nitpick comments (5)
lib/src/headers/extension/string_list_extensions.dart (2)
20-23: Consider documenting the new helper’s contract
splitTrimAndFilterUniquenow delegates tosplitAndTrim, but the public-facing doc comment still enumerates the old algorithm step-by-step (including uniqueness) while the helper itself is undocumented.
It would be clearer to:
- Move the uniqueness bullet-point from
splitAndTrim’s description tosplitTrimAndFilterUnique.- Add a short doc comment on
splitAndTrimexplaining that it does not de-duplicate.No functional change – purely about preventing reader confusion.
24-30: Minor perf nit – avoid a second iteration
LinkedHashSet.from(splitAndTrim(...))walks the iterable twice (once to create the list for the set and once to copy).
If you want to micro-optimise, build theLinkedHashSetdirectly while expanding:-LinkedHashSet<String>.from( - splitAndTrim(separator: separator, emptyCheck: emptyCheck)); +final result = LinkedHashSet<String>(); +for (final el in this) { + for (final part in el.split(separator)) { + final trimmed = part.trim(); + if (!emptyCheck || trimmed.isNotEmpty) result.add(trimmed); + } +} +return result;Completely optional; probably not measurable unless the header list is very large.
test/headers/header_test.dart (1)
154-181: The TODO comment is now stale
Headers.xForwardedForwas added to the “invalid raw value” exclusion list, but the preceding TODO still claims the exclusions are “not all correct”.
If the list has been audited as part of this PR, consider removing or updating the TODO to avoid confusion.lib/src/headers/typed/headers/x_forwarded_for_header.dart (2)
25-33: Validate IP literals (optional)At the moment any non-empty token is accepted as an “address”.
While spec-compliant (many servers are permissive), consider an optional check usingInternetAddress.tryParseand allowing the literal"unknown". This guards against garbage values early:final parsedAddresses = values.splitAndTrim(); if (parsedAddresses.isEmpty) { throw const FormatException('Value cannot be empty'); } -return XForwardedForHeader(parsedAddresses); +if (parsedAddresses.any((a) => + a != 'unknown' && InternetAddress.tryParse(a) == null)) { + throw const FormatException('Invalid IP literal'); +} +return XForwardedForHeader(parsedAddresses);Up to you whether the stricter behaviour is desirable.
40-42: Encoding – drop the extra space for round-trip fidelity
join(', ')emits a space after every comma, whereas many upstream proxies emit no space (1.2.3.4,5.6.7.8).
Parsing handles either, but round-tripping (decode → encode) changes the byte sequence. Usingjoin(',')retains the original compact form and avoids surprises in signature/macro validations.-String _encode() => addresses.join(', '); +String _encode() => addresses.join(',');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/src/headers/extension/string_list_extensions.dart(1 hunks)lib/src/headers/headers.dart(5 hunks)lib/src/headers/typed/headers/x_forwarded_for_header.dart(1 hunks)test/headers/header_test.dart(1 hunks)test/headers/typed/x_forwarded_for_header_test.dart(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
lib/src/headers/typed/headers/x_forwarded_for_header.dart
[warning] 54-54: lib/src/headers/typed/headers/x_forwarded_for_header.dart#L54
Added line #L54 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Run Unit Tests (macos-latest, stable, downgrade)
- GitHub Check: Run Unit Tests (macos-latest, 3.5.0, upgrade)
- GitHub Check: Run Unit Tests (macos-latest, beta, downgrade)
- GitHub Check: Run Unit Tests (macos-latest, beta, upgrade)
- GitHub Check: Run Unit Tests (macos-latest, stable, upgrade)
- GitHub Check: Run Unit Tests (macos-latest, 3.5.0, downgrade)
- GitHub Check: Run Unit Tests (windows-latest, 3.5.0, downgrade)
🔇 Additional comments (3)
lib/src/headers/headers.dart (1)
248-251: Accessor registered correctly – LGTM
Headers.xForwardedForis exposed with its codec and placed in the request-only set. Integration looks consistent with existing pattern.lib/src/headers/typed/headers/x_forwarded_for_header.dart (1)
54-54: Missing test coverage only ontoString()– acceptableCodec, equality and parsing are thoroughly tested. Lack of coverage on
toString()is not an issue.test/headers/typed/x_forwarded_for_header_test.dart (1)
130-140: Great edge-case coverageIncluding IPv6, obfuscated identifiers and ports demonstrates thoughtful testing beyond the happy path.
2 participants
Description
feat: Add typed header for X-Forwarded-For
Introduces
XForwardedForHeaderfor typed access to theX-Forwarded-ForHTTP header.Includes:
Headersclass and relevant test suites.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
To be used as fallback, when
Forwardedheader not set in serverpod.Summary by CodeRabbit
X-Forwarded-ForHTTP header, providing typed access and validation for lists of IP addresses.X-Forwarded-Forheader, including parsing, immutability, equality, and codec behavior.X-Forwarded-Forheader.