feat: Add operator== and hashCode on all header classes #146
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 📝 WalkthroughWalkthroughAdds value-based equality/hashCode across many typed headers, tightens constructors with non-empty assertions, updates wildcard handling to use empty lists, introduces new parse factories, restructures ETag condition headers, enhances Content-Disposition extended parsing, migrates RequestMethod to enum with name-based encoding, adjusts codecs/middleware/tests, and updates X-Forwarded-For exports/imports. Changes
Sequence Diagram(s)Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests
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. Comment |
nielsenko
force-pushed
the
fix-header-equality
branch
2 times, most recently
from
27bf1f5 to
aa2177b
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #146 +/- ##
==========================================
+ Coverage 84.20% 92.43% +8.22%
==========================================
Files 83 83
Lines 2716 3119 +403
Branches 1480 1592 +112
==========================================
+ Hits 2287 2883 +596
+ Misses 429 236 -193 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
nielsenko
force-pushed
the
fix-header-equality
branch
6 times, most recently
from
c83c3eb to
e76dd5a
Compare
nielsenko
changed the title
nielsenko
force-pushed
the
fix-header-equality
branch
from
c55ffe1 to
6f48243
Compare
nielsenko
changed the title
nielsenko
force-pushed
the
fix-header-equality
branch
from
6f48243 to
52fafb3
Compare
|
@CodeRabbit 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: 32
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (26)
lib/src/headers/typed/headers/permission_policy_header.dart (1)
35-44: Validate directive name is non-empty during parsing
'=(self)'would currently yield an empty directive name and later encode to an invalid=(). Fail fast with a clear error.Apply this diff:
final directiveParts = part.split('='); final name = directiveParts.first.trim(); + if (name.isEmpty) { + throw const FormatException('Directive name cannot be empty'); + } final values = directiveParts.length > 1 ? directiveParts[1] .replaceAll('(', '') .replaceAll(')', '') .split(' ') .map((final s) => s.trim()) .where((final s) => s.isNotEmpty)lib/src/headers/typed/headers/forwarded_header.dart (1)
259-292: Fix: avoid emitting an empty header value['']If all elements produce zero pairs,
elementsAsStringsstays empty andjoin(', ')yields''. Return[]instead.Iterable<String> toStrings() { if (elements.isEmpty) return []; @@ } - return [elementsAsStrings.join(', ')]; + if (elementsAsStrings.isEmpty) return []; + return [elementsAsStrings.join(', ')]; }lib/src/headers/typed/headers/expect_header.dart (1)
22-24: Docstring-contract mismatch: code throws on non-100-continue, but docs say it returns a custom instanceCurrent behavior is to throw
FormatException('Invalid value'). Update the docs to match the behavior (or implement a custom-instance path if intended).Doc fix:
- /// Parses a [value] and returns the corresponding [ExpectHeader] instance. - /// If the value does not match any predefined types, it returns a custom instance. + /// Parses a [value] and returns the corresponding [ExpectHeader] instance. + /// Only the `100-continue` directive is supported; any other value results + /// in a [FormatException].Also applies to: 33-34
lib/src/headers/typed/headers/sec_fetch_dest_header.dart (1)
66-68: Docstring-contract mismatch: code throws on unknown values, but docs say it returns a custom instance
parsethrowsFormatException('Invalid value')for unknown destinations. Align the docs with actual behavior (or support custom values if intended).Doc fix:
- /// Parses a [value] and returns the corresponding [SecFetchDestHeader] instance. - /// If the value does not match any predefined types, it returns a custom instance. + /// Parses a [value] and returns the corresponding [SecFetchDestHeader] instance. + /// Only known destination tokens are supported; any other value results + /// in a [FormatException].Also applies to: 121-123
lib/src/headers/typed/headers/cross_origin_embedder_policy_header.dart (1)
28-31: Doc mismatch: parse() does not return a “custom instance”.The implementation throws FormatException for unknown values. Update the comment to reflect behavior.
Suggested replacement:
/// Parses a [value] and returns the corresponding constant instance. /// Throws [FormatException] if the value is not one of: 'unsafe-none', /// 'require-corp', or 'credentialless'.lib/src/headers/typed/headers/sec_fetch_site_header.dart (1)
29-31: Doc mismatch: parse() doesn’t create a custom instance.For unknown values the code throws FormatException. Update comment to match behavior.
Suggested replacement:
/// Parses [value] and returns the corresponding constant instance. /// Throws [FormatException] if [value] is not one of: /// 'same-origin', 'same-site', 'cross-site', or 'none'.lib/src/headers/typed/headers/sec_fetch_mode_header.dart (1)
32-34: Doc mismatch: parse() comment contradicts implementation.It throws on unknown values; it doesn’t return a custom instance. Update the comment accordingly.
Suggested replacement:
/// Parses [value] and returns the corresponding constant instance. /// Throws [FormatException] if [value] is not one of: /// 'cors', 'no-cors', 'same-origin', 'navigate', 'nested-navigate', 'websocket'.lib/src/headers/typed/headers/etag_header.dart (1)
58-61: Bug: weak-prefix detection ignores leading/trailing whitespace.
parse()checksvalue.startsWith('W/')instead of the trimmed input, causing mis-parsing for values like ' W/"abc"'. Trim first, then detect and slice.Apply:
- final isWeak = value.startsWith(_weakPrefix); - final tagValue = isWeak ? value.substring(2).trim() : value.trim(); - return ETagHeader(value: tagValue.replaceAll(_quote, ''), isWeak: isWeak); + final trimmed = value.trim(); + final isWeak = trimmed.startsWith(_weakPrefix); + final tagValue = isWeak ? trimmed.substring(2).trim() : trimmed; + return ETagHeader(value: tagValue.replaceAll(_quote, ''), isWeak: isWeak);lib/src/headers/typed/headers/if_range_header.dart (1)
23-29: Enforce XOR: exactly one of date or etag must be set.
Current check only rejects both-null; both-set is still allowed and encodes as date silently. Tighten constructor per spec intent and PR objective #145.IfRangeHeader({ this.lastModified, this.etag, }) { - if (lastModified == null && etag == null) { - throw const FormatException('Either date or etag must be provided'); - } + final bothNull = lastModified == null && etag == null; + final bothSet = lastModified != null && etag != null; + if (bothNull || bothSet) { + throw const FormatException('Provide exactly one of date or etag'); + } }lib/src/headers/typed/headers/cross_origin_resource_policy_header.dart (1)
27-44: Doc/behavior mismatch in parse().
Comment promises “returns a custom instance” for unknown values, but code throwsFormatException. Align the doc or implementation.- /// Parses a [value] and returns the corresponding [CrossOriginResourcePolicyHeader] instance. - /// If the value does not match any predefined types, it returns a custom instance. + /// Parses [value] and returns the corresponding predefined instance. + /// Throws [FormatException] if the value is not one of the supported tokens.lib/src/headers/typed/headers/cross_origin_opener_policy_header.dart (1)
29-46: Doc/behavior mismatch in parse().
Comment says unknown values return a custom instance; implementation throws. Please align.- /// Parses a [value] and returns the corresponding [CrossOriginOpenerPolicyHeader] instance. - /// If the value does not match any predefined types, it returns a custom instance. + /// Parses [value] and returns the corresponding predefined instance. + /// Throws [FormatException] for unsupported tokens.lib/src/headers/typed/headers/strict_transport_security_header.dart (1)
23-29: ValidatemaxAgein constructor (negative not allowed).
Prevent invalid instances created via the public constructor, consistent with #145.StrictTransportSecurityHeader({ required this.maxAge, this.includeSubDomains = false, this.preload = false, - }); + }) { + if (maxAge < 0) { + throw const FormatException('maxAge must be >= 0'); + } + }lib/src/headers/typed/headers/content_range_header.dart (1)
24-35: Disallow half-null ranges (start or end null alone) — currently encodes invalid strings likebytes 10-null/*.
Constructor allowsstart != nullXORend != null, while_encode()assumes either both null or both set. Enforce the invariant at construction.Apply this diff:
ContentRangeHeader({ this.unit = 'bytes', this.start, this.end, this.size, }) { + // start and end must be provided together or both be null + if ((start == null) != (end == null)) { + throw const FormatException( + 'Start and end must both be provided, or both be null', + ); + } if (start != null && end != null && start! > end!) { throw const FormatException('Invalid range'); } }Also applies to: 73-79
lib/src/headers/typed/headers/connection_header.dart (1)
97-113: Parse should be case-insensitive for tokens.
keep-alive,close,upgrade,downgradeare case-insensitive HTTP tokens. Convert to lower-case in the switch.Apply:
- switch (trimmed) { + switch (trimmed.toLowerCase()) { case _keepAlive: return keepAlive; case _close: return close; case _upgrade: return upgrade; case _downgrade: return downgrade; default: throw const FormatException('Invalid value'); }Also update the doc that mentions “returns a custom instance” to reflect that it throws.
lib/src/headers/typed/headers/accept_header.dart (1)
81-92: Bug: q-value parsing grabs the wrong segment and misses parameters beyond index 1.
parts[1].splitTrimAndFilterUnique('q=').firstOrNullyields '' for "q=0.8" and ignores q-values not in position 1.Use a robust scan:
- double? quality; - if (parts.length > 1) { - final qualityParts = - parts[1].splitTrimAndFilterUnique(separator: 'q=').firstOrNull; - if (qualityParts != null) { - final value = double.tryParse(qualityParts); - if (value == null || value < 0 || value > 1) { - throw const FormatException('Invalid quality value'); - } - quality = value; - } - } + double? quality; + if (parts.length > 1) { + final qParam = parts + .skip(1) + .map((p) => p.trim()) + .firstWhereOrNull((p) => p.toLowerCase().startsWith('q=')); + if (qParam != null) { + final qStr = qParam.substring(qParam.indexOf('=') + 1).trim(); + final q = double.tryParse(qStr); + if (q == null || q < 0 || q > 1) { + throw const FormatException('Invalid quality value'); + } + quality = q; + } + }lib/src/headers/typed/headers/cookie_header.dart (1)
82-92: Bug: parsing fails for values containing '=' (common with base64/padded cookies).
Splitting on '=' into exactly two parts rejects valid cookies likeid=abc=.Parse on the first '=' only:
- factory Cookie.parse(final String value) { - final splitValue = value.split('='); - if (splitValue.length != 2) { - throw const FormatException('Invalid cookie format'); - } - return Cookie( - name: splitValue.first.trim(), - value: splitValue.last.trim(), - ); - } + factory Cookie.parse(final String input) { + final sep = input.indexOf('='); + if (sep <= 0 || sep == input.length - 1) { + throw const FormatException('Invalid cookie format'); + } + final name = input.substring(0, sep).trim(); + final val = input.substring(sep + 1).trim(); + return Cookie(name: name, value: val); + }lib/src/headers/typed/headers/authorization_header.dart (4)
35-41: Prefix check bug: don’t trim the scheme constant.
prefix.trim()turns'Bearer 'into'Bearer', sostartsWithmatches invalid inputs (e.g.,BearerXYZ), then delegates to a parser that will throw. Check against the exactprefix.- if (value.startsWith(BearerAuthorizationHeader.prefix.trim())) { + if (value.startsWith(BearerAuthorizationHeader.prefix)) { return BearerAuthorizationHeader.parse(value); - } else if (value.startsWith(BasicAuthorizationHeader.prefix.trim())) { + } else if (value.startsWith(BasicAuthorizationHeader.prefix)) { return BasicAuthorizationHeader.parse(value); - } else if (value.startsWith(DigestAuthorizationHeader.prefix.trim())) { + } else if (value.startsWith(DigestAuthorizationHeader.prefix)) { return DigestAuthorizationHeader.parse(value); }
185-188: Avoid leaking Basic credentials in logs.
toString()exposes the password. Mask it.- String toString() => - 'BasicAuthorizationHeader(username: $username, password: $password)'; + String toString() => + 'BasicAuthorizationHeader(username: $username, password: ****)';
326-342: Digest headerValue formatting is invalid.Joining with
', 'produces"Digest, username=..."(comma after scheme). It must be"Digest username=...".- String get headerValue { - return [ - 'Digest', - '$_username="$username"', - '$_realm="$realm"', - '$_nonce="$nonce"', - '$_uri="$uri"', - '$_response="$response"', - if (algorithm != null) '$_algorithm="$algorithm"', - if (qop != null) '$_qop="$qop"', - if (nc != null) '$_nc="$nc"', - if (cnonce != null) '$_cnonce="$cnonce"', - if (opaque != null) '$_opaque="$opaque"' - ].join(', '); - } + String get headerValue { + final params = <String>[ + '$_username="$username"', + '$_realm="$realm"', + '$_nonce="$nonce"', + '$_uri="$uri"', + '$_response="$response"', + if (algorithm != null) '$_algorithm="$algorithm"', + if (qop != null) '$_qop="$qop"', + if (nc != null) '$_nc="$nc"', + if (cnonce != null) '$_cnonce="$cnonce"', + if (opaque != null) '$_opaque="$opaque"', + ]; + return '$prefix${params.join(', ')}'; + }
274-324: Digest.parse doesn’t verify the “Digest ” prefix.Align with Bearer/Basic by checking the scheme and trimming it off before parsing params.
- factory DigestAuthorizationHeader.parse(final String value) { - if (value.isEmpty) { + factory DigestAuthorizationHeader.parse(final String value) { + if (value.isEmpty) { throw const FormatException('Digest token cannot be empty.'); } - - final Map<String, String> params = {}; + if (!value.startsWith(prefix)) { + throw const FormatException('Invalid digest prefix'); + } + final raw = value.substring(prefix.length).trim(); + final Map<String, String> params = {}; - final regex = RegExp(r'(\w+)="([^"]*)"'); - for (final match in regex.allMatches(value)) { + final regex = RegExp(r'(\w+)="([^"]*)"'); + for (final match in regex.allMatches(raw)) { params[match.group(1)!] = match.group(2)!; }lib/src/headers/typed/headers/content_encoding_header.dart (1)
88-96: Doc-code mismatch: unknown encodings.Comment says “returns a custom instance,” but code throws on unknown values. This may break roundtrips (#144). Either permit custom encodings or update docs/tests to enforce a closed set.
factory ContentEncoding.parse(final String name) { @@ - switch (trimmed) { + switch (trimmed) { @@ - default: - throw const FormatException('Invalid value'); + default: + // Allow custom/extension encodings to preserve roundtrip fidelity. + return ContentEncoding._(trimmed); } }Also applies to: 110-112
lib/src/headers/typed/headers/content_disposition_header.dart (2)
27-30: Make parameters list immutable to uphold equality/hash invariantsStoring a mutable
parameterslist risks hash/equality violations if mutated after construction. Copy defensively and wrap inList.unmodifiable; this requires droppingconston the constructor.- const ContentDispositionHeader({ - required this.type, - this.parameters = const [], - }); + ContentDispositionHeader({ + required this.type, + List<ContentDispositionParameter> parameters = const [], + }) : parameters = List.unmodifiable(parameters);
113-121: Don’t split on '=' without a limit; breaks quoted values containing '='
part.split('=')fails for values likefilename="a=b.txt". Parse using the first=only and strip surrounding quotes safely.- final keyValue = part.split('=').map((final e) => e.trim()).toList(); - - if (keyValue.length != 2) { - throw const FormatException('Invalid parameter format'); - } - - final name = keyValue[0]; - var value = keyValue[1].replaceAll('"', ''); - final bool isExtended = name.endsWith('*'); + final eq = part.indexOf('='); + if (eq <= 0 || eq == part.length - 1) { + throw const FormatException('Invalid parameter format'); + } + final rawName = part.substring(0, eq).trim(); + var value = part.substring(eq + 1).trim(); + if (value.length >= 2 && value.startsWith('"') && value.endsWith('"')) { + value = value.substring(1, value.length - 1); + } + final bool isExtended = rawName.endsWith('*'); + final name = isExtended ? rawName.substring(0, rawName.length - 1) : rawName;lib/src/headers/typed/headers/authentication_header.dart (1)
46-62: Parsing drops non key="value" tokens when mixed with quoted pairsIf
paramsStringcontains both quoted pairs and bare tokens (e.g.,token="abc", xyz), the regex only captures the pairs and silently ignoresxyz, causing lossy roundtrips.
- Either reject mixed forms or parse both forms explicitly. Example approach: split by commas (outside quotes), then for each segment parse
key="value"or a bare token.I can provide a safe parser if you confirm desired behavior for mixed forms.
lib/src/headers/typed/headers/accept_encoding_header.dart (1)
47-62: Robustly parseqwith optional whitespaceCurrent parsing uses
split(';q='), which fails for common forms likegzip; q=0.5(with spaces). Parse by semicolons and then find theq=param ignoring whitespace.- final encodings = splitValues.map((final value) { - final encodingParts = value.split(';q='); - final encoding = encodingParts[0].trim().toLowerCase(); - if (encoding.isEmpty) { - throw const FormatException('Invalid encoding'); - } - double? quality; - if (encodingParts.length > 1) { - final value = double.tryParse(encodingParts[1].trim()); - if (value == null || value < 0 || value > 1) { - throw const FormatException('Invalid quality value'); - } - quality = value; - } - return EncodingQuality(encoding, quality); - }).toList(); + final encodings = splitValues.map((final value) { + final parts = value.split(';'); + final encoding = parts.first.trim().toLowerCase(); + if (encoding.isEmpty) { + throw const FormatException('Invalid encoding'); + } + double? quality; + if (parts.length > 1) { + final qPart = parts + .skip(1) + .map((p) => p.trim()) + .firstWhere((p) => p.toLowerCase().startsWith('q='), orElse: () => ''); + if (qPart.isNotEmpty) { + final qStr = qPart.split('=').last.trim(); + final parsed = double.tryParse(qStr); + if (parsed == null || parsed < 0 || parsed > 1) { + throw const FormatException('Invalid quality value'); + } + quality = parsed; + } + } + return EncodingQuality(encoding, quality); + }).toList();lib/src/headers/typed/headers/etag_condition_header.dart (1)
8-28: Prevent invalid/mutable state: make.etagsnon-empty and immutableEmpty non-wildcard ETag lists create invalid headers and mutable lists can break equality/hash. Enforce non-empty and freeze the list in the base ctor (dropping
constis acceptable per PR goals #145).- /// Creates an [ETagConditionHeader] with specific ETags. - const ETagConditionHeader.etags(this.etags) : isWildcard = false; + /// Creates an [ETagConditionHeader] with specific ETags. + ETagConditionHeader.etags(final List<ETagHeader> etags) + : assert(etags.isNotEmpty), + etags = List.unmodifiable(etags), + isWildcard = false;And adjust subclass constructors:
- const IfMatchHeader.etags(super.etags) : super.etags(); + IfMatchHeader.etags(super.etags) : super.etags(); - const IfNoneMatchHeader.etags(super.etags) : super.etags(); + IfNoneMatchHeader.etags(super.etags) : super.etags();
🧹 Nitpick comments (65)
lib/src/headers/typed/headers/permission_policy_header.dart (1)
65-75: Equality/hash look correct; small consistency nitDeep list equality is correct. If you want to align with the PR’s stated pattern, consider
Object.hashAll(directives)for hashing (order-sensitive as today) — not required.Optional diff:
- int get hashCode => - const ListEquality<PermissionsPolicyDirective>().hash(directives); + int get hashCode => Object.hashAll(directives);lib/src/headers/typed/headers/te_header.dart (2)
56-63: Order sensitivity: verify desired semantics; consider unordered equality.If TE values are a set (order-insensitive), switch to
UnorderedIterableEquality; otherwise keep as-is.Optional change:
- other is TEHeader && - const ListEquality<TeQuality>().equals(encodings, other.encodings); + other is TEHeader && + const UnorderedIterableEquality<TeQuality>() + .equals(encodings, other.encodings);- int get hashCode => const ListEquality<TeQuality>().hash(encodings); + int get hashCode => + const UnorderedIterableEquality<TeQuality>().hash(encodings);
31-45: Parsing;qis brittle (extra params, spacing, order, case).Splitting on
';q='will fail for inputs likegzip;level=9;q=0.5orq = 0.7. Consider tokenizing on;and scanning params (case-insensitive key), ignoring unknown params, and rejectingtrailers;q=....Sketch:
TeQuality _parseOne(String raw) { final parts = raw.split(';'); final enc = parts.first.trim(); double? q; for (final p in parts.skip(1)) { final kv = p.split('='); if (kv.length == 2 && kv[0].trim().toLowerCase() == 'q') { final parsed = double.tryParse(kv[1].trim()); if (parsed == null || parsed < 0 || parsed > 1) { throw const FormatException('Invalid quality value'); } q = parsed; } } return TeQuality(enc, q); }Add tests for:
gzip;level=9;q=0.5qwith spaces/case (Q = 0.7)trailers;q=0.5→ invalid- List order equality per chosen semantics.
lib/src/headers/typed/headers/forwarded_header.dart (3)
254-254: Add an informative error messageEmpty message hinders debugging and test expectations.
- if (allElements.isEmpty) throw const FormatException(''); + if (allElements.isEmpty) { + throw const FormatException('No valid forwarded-element found'); + }
109-111: Typo in toString(): class name mismatchUse
ForwardedIdentifierto match the class.- String toString() => 'ForwardedNode(identifier: $identifier, port: $port)'; + String toString() => + 'ForwardedIdentifier(identifier: $identifier, port: $port)';
205-213: Parsing isn’t quote-aware for;and=Known TODO, but calling out:
split(';')andsplit('=')will break on quoted values containing those chars. Consider a small state machine or regex that respects quoted-strings, or reuse a parser shared across headers to close #102.Also applies to: 215-221
test/static/range_edge_cases_test.dart (1)
257-262: Keep header construction style and casing consistent with the rest of the fileHeaders are case-insensitive, but earlier tests use
Headers.fromMapand lowercase'range'. For consistency and clarity, mirror that style here.Apply:
- final headers = Headers.build((final mh) => mh['Range'] = ['bytes=']); - await expectLater( - makeRequest(handler, '/test_file.txt', headers: headers), - throwsA(isA<InvalidHeaderException>()), - ); + final headers = Headers.fromMap({'range': ['bytes=']}); + expect( + () async => await makeRequest(handler, '/test_file.txt', headers: headers), + throwsA(isA<InvalidHeaderException>()), + );lib/src/headers/typed/headers/cross_origin_embedder_policy_header.dart (1)
61-64: Nit: Align toString label with field name.Prefer "policy" over "value" for clarity and consistency.
Apply:
- String toString() { - return 'CrossOriginEmbedderPolicyHeader(value: $policy)'; - } + String toString() { + return 'CrossOriginEmbedderPolicyHeader(policy: $policy)'; + }lib/src/headers/typed/headers/retry_after_header.dart (1)
69-71: Nit: Ensure UTC when formatting the date.Guard against a non-UTC DateTime passed by callers; formatHttpDate expects GMT.
Apply:
- return formatHttpDate(date!); + return formatHttpDate(date!.toUtc());lib/src/headers/typed/headers/sec_fetch_site_header.dart (1)
64-67: Nit: Align toString label with field name.Use "site" instead of "value" for clarity.
Apply:
- String toString() { - return 'SecFetchSiteHeader(value: $site)'; - } + String toString() { + return 'SecFetchSiteHeader(site: $site)'; + }lib/src/headers/typed/headers/sec_fetch_mode_header.dart (1)
71-74: Nit: Align toString label with field name.Use "mode" instead of "value" for clarity.
Apply:
- String toString() { - return 'SecFetchModeHeader(value: $mode)'; - } + String toString() { + return 'SecFetchModeHeader(mode: $mode)'; + }lib/src/headers/typed/headers/etag_header.dart (1)
28-47: Doc clarity: function may throw.
isValidETag()throws on empty input; the doc says it “Returns true if…”. Briefly note it throws on empty to avoid surprises.lib/src/headers/typed/headers/if_range_header.dart (1)
40-43: Optional: consider rejecting weak ETags for If-Range.
HTTP If-Range is intended to use strong validators; decide whether to rejectW/-prefixed tags at parse-time.lib/src/headers/typed/headers/cross_origin_resource_policy_header.dart (1)
30-44: Optional: case-insensitive parsing.
Header tokens are commonly lower-case; allowing mixed case can improve robustness.- final trimmed = value.trim(); + final trimmed = value.trim().toLowerCase();lib/src/headers/typed/headers/cross_origin_opener_policy_header.dart (1)
31-46: Optional: case-insensitive parsing.
Permit mixed-case tokens for resilience.- final trimmed = value.trim(); + final trimmed = value.trim().toLowerCase();lib/src/headers/typed/headers/strict_transport_security_header.dart (2)
39-67: Optional: directive parsing robustness.
- Treat directive names case-insensitively.
- Consider rejecting duplicate max-age directives explicitly (currently de-duped by extension, but being explicit aids future readers).
- for (final directive in splitValues) { + for (final raw in splitValues) { + final directive = raw.toLowerCase(); if (directive.startsWith(_maxAgePrefix)) { maxAge = int.tryParse(directive.substring(_maxAgePrefix.length));
72-77: Optional: stable directive ordering.
Current encoding is fine; keepingmax-agefirst then flags matches common practice. No change needed.lib/src/headers/typed/headers/transfer_encoding_header.dart (1)
111-132: Doc mismatch and stricter value set.The doc says “returns a custom instance,” but the code throws. Update the comment or implement the “custom” path.
Apply:
- /// Parses a [name] and returns the corresponding [TransferEncoding] instance. - /// If the name does not match any predefined encodings, it returns a custom instance. + /// Parses a [name] and returns the corresponding [TransferEncoding] instance. + /// Throws [FormatException] if the name is not one of the predefined encodings.lib/src/headers/typed/headers/clear_site_data_header.dart (1)
124-131: Type equality/hash are fine. Minor doc/typo fixes elsewhere.Equality on
valueandhashCodeonvalue.hashCodeare correct.Also fix the stray “P” in the constructor comment (Line 89) and align the factory doc with behavior (it throws on invalid).
lib/src/headers/typed/headers/from_header.dart (1)
45-53: Equality/hash are fine; consider order-insensitive compare if semantics allow.If multiple addresses are possible and order is irrelevant, use
UnorderedIterableEquality. Otherwise, current implementation is acceptable.Possible tweak:
- bool operator ==(final Object other) => + bool operator ==(final Object other) => identical(this, other) || - other is FromHeader && - const IterableEquality<String>().equals(emails, other.emails); + other is FromHeader && + const UnorderedIterableEquality<String>().equals(emails, other.emails); - int get hashCode => const IterableEquality<String>().hash(emails); + int get hashCode => const UnorderedIterableEquality<String>().hash(emails);lib/src/headers/typed/headers/accept_header.dart (1)
107-117: LGTM: value-based equality/hash for MediaRange.Minor: avoid accidental shadowing by not naming locals
valuein parse() if you touch this again.lib/src/headers/typed/headers/upgrade_header.dart (1)
65-66: Preserve lexical protocol version (optional).
Storing version asdoubleloses formatting (e.g., encodes2.0as2). If textual fidelity matters, keepversionasStringvalidated by a regex and compare as string.Also applies to: 106-108
lib/src/headers/typed/headers/cookie_header.dart (1)
42-44: Optional: convenience lookup ignoring case.
You de-duplicate by lowercased names, but getCookie() is case-sensitive. Consider addinggetCookieIgnoreCase(String name).lib/src/headers/typed/headers/content_language_header.dart (1)
45-52: Is order semantically significant for Content-Language?
If not, prefer set-based equality/hash; otherwise keep as-is. Please confirm intended semantics.Possible change:
- const IterableEquality<String>().equals(languages, other.languages); + const SetEquality<String>() + .equals(languages.toSet(), other.languages.toSet());and
- int get hashCode => const IterableEquality<String>().hash(languages); + int get hashCode => const SetEquality<String>().hash(languages.toSet());lib/src/headers/typed/headers/access_control_expose_headers_header.dart (3)
60-69: Use case-insensitive deep equality for header names.Header field names are case-insensitive; comparing 'ETag' vs 'etag' should be equal. Switch to a case-insensitive equality and hash.
- bool operator ==(final Object other) => + bool operator ==(final Object other) => identical(this, other) || other is AccessControlExposeHeadersHeader && isWildcard == other.isWildcard && - const IterableEquality<String>().equals(headers, other.headers); + const IterableEquality<String>(CaseInsensitiveEquality()) + .equals(headers, other.headers); @override - int get hashCode => - Object.hash(isWildcard, const IterableEquality<String>().hash(headers)); + int get hashCode => Object.hash( + isWildcard, + const IterableEquality<String>(CaseInsensitiveEquality()).hash(headers));
60-69: Option: treat header names as an unordered set.Order is not semantically meaningful for this header. If you want true value semantics, consider order-insensitive equality/hash.
-const IterableEquality<String>(CaseInsensitiveEquality()) +const UnorderedIterableEquality<String>(CaseInsensitiveEquality())Note: only adopt if encoding preserves a canonical order or tests always build in the same order.
71-74: Polish toString for wildcard.Currently toString() prints headers: [] for wildcard, which is slightly confusing. Consider returning '*' when isWildcard is true.
- String toString() => - 'AccessControlExposeHeadersHeader(headers: $headers, isWildcard: $isWildcard)'; + String toString() => isWildcard + ? 'AccessControlExposeHeadersHeader(*)' + : 'AccessControlExposeHeadersHeader(headers: $headers)';test/headers/header_test.dart (5)
70-79: Use a shared constant for the fixed UTC date.Minor readability/duplication: define a top-level const (or final) fixedDate and reuse across tests.
- var headers = - Headers.build((final mh) => mh.date = DateTime.utc(2025, 9, 23)); + const fixedDate = DateTime.utc(2025, 9, 23); + var headers = Headers.build((final mh) => mh.date = fixedDate);
474-505: Strengthen comparisons for list-valued headers.Currently list types skip equality/hash checks. Consider using DeepCollectionEquality (possibly unordered) to validate list contents without changing production code.
- if (header1 is! List) { + if (header1 is! List) { expect(header1, equals(header2)); expect(header1.hashCode, equals(header2.hashCode), reason: 'hashCode for: $header1'); - } + } else { + // import 'package:collection/collection.dart'; + expect( + const DeepCollectionEquality().equals(header1, header2), + isTrue, + ); + }
507-571: Add wildcard variant for Access-Control-Expose-Headers.Symmetry with AllowHeaders tests; also exercises the new wildcard semantics.
( Headers.accessControlExposeHeaders, (final h) => h.accessControlExposeHeaders = AccessControlExposeHeadersHeader.headers(headers: ['foo']) ), + ( + Headers.accessControlExposeHeaders, + (final h) => + h.accessControlExposeHeaders = + const AccessControlExposeHeadersHeader.wildcard() + ),
518-521: Verify token used in Accept-Encoding test.'jpeg' is not a content-coding (typical tokens: gzip, br, deflate, identity). If validation exists, this may fail or lower test credibility. Recommend using 'gzip' in this spot.
- encodings: [EncodingQuality('jpeg', 0.5)]) + encodings: [EncodingQuality('gzip', 0.5)])
682-684: Confirm empty ETag validity.An empty opaque-tag is generally invalid per HTTP specs. If the implementation treats it as valid, fine; otherwise switch to a minimal valid value (e.g., ETagHeader(value: 'W/"0"' or '"0"') to ensure test intent isn’t masking a bug.
- (Headers.etag, (final h) => h.etag = const ETagHeader(value: '')), + (Headers.etag, (final h) => h.etag = const ETagHeader(value: '"0"')),lib/src/headers/typed/headers/content_security_policy_header.dart (3)
57-66: Equality may need to be order-insensitive.CSP directive order typically isn’t semantically significant. If you want logically equivalent headers to compare equal regardless of directive order, use UnorderedIterableEquality.
- other is ContentSecurityPolicyHeader && - const ListEquality<ContentSecurityPolicyDirective>() - .equals(directives, other.directives); + other is ContentSecurityPolicyHeader && + const UnorderedIterableEquality<ContentSecurityPolicyDirective>() + .equals(directives, other.directives); @@ - int get hashCode => - const ListEquality<ContentSecurityPolicyDirective>().hash(directives); + int get hashCode => + const UnorderedIterableEquality<ContentSecurityPolicyDirective>() + .hash(directives);
90-93: Avoid trailing space when a directive has no values.Current encode emits “sandbox ”. Minor but avoidable.
- String _encode() => '$name ${values.join(' ')}'; + String _encode() => values.isEmpty ? name : '$name ${values.join(' ')}';
75-88: Seal the directive type to keep equality/hash sound.
ContentSecurityPolicyDirectiveisn’t final; subclasses could break symmetry. Make itfinal(like the header) and consider freezingvaluesas an unmodifiable list on construction.- class ContentSecurityPolicyDirective { + final class ContentSecurityPolicyDirective { @@ - ContentSecurityPolicyDirective({ - required this.name, - required this.values, - }); + ContentSecurityPolicyDirective({ + required this.name, + required Iterable<String> values, + }) : values = List.unmodifiable(values);lib/src/headers/typed/headers/range_header.dart (2)
81-90: Confirm intended equality semantics (order).If multiple ranges’ order shouldn’t affect equality, prefer
UnorderedIterableEquality<Range>; otherwise current order-sensitive comparison is fine.- const ListEquality<Range>().equals(ranges, other.ranges); + const UnorderedIterableEquality<Range>().equals(ranges, other.ranges); @@ - int get hashCode => - Object.hash(unit, const ListEquality<Range>().hash(ranges)); + int get hashCode => + Object.hash(unit, const UnorderedIterableEquality<Range>().hash(ranges));
106-116: Add basic numeric validation for ranges.Guard against negative values and inverted ranges (
start > end) to prevent generating invalid headers.Range({ this.start, this.end, }) { if (start == null && end == null) { throw const FormatException( 'At least one of start or end must be specified'); } + if ((start != null && start < 0) || (end != null && end < 0)) { + throw const FormatException('Range values must be non-negative'); + } + if (start != null && end != null && start > end) { + throw const FormatException('Range start cannot be greater than end'); + } }lib/src/headers/typed/headers/authorization_header.dart (3)
146-166: Basic.parse: be explicit about malformeduser:pass.The current try/catch masks RangeError, which is fine, but a simple guard avoids exceptions on happy path.
try { final decoded = utf8.decode(base64Decode(base64Part)); final split = decoded.indexOf(':'); + if (split <= 0) { + throw const FormatException('Invalid basic token format'); + } return BasicAuthorizationHeader( username: decoded.substring(0, split), password: decoded.substring(split + 1), ); } catch (e) { throw const FormatException('Invalid basic token format'); }
129-137: Empty password policy change — confirm intent.RFC 7617 allows empty passwords. Rejecting them may be breaking. If intentional, note it in docs; otherwise permit empty passwords.
373-387: Consider masking sensitive digest fields in toString().At minimum, hide
responseandcnonce; optionally shortennonceandopaque.lib/src/headers/typed/headers/content_disposition_header.dart (4)
143-149: Normalize name handling for extended paramsUsing
replaceAll('*', '')can over-strip if a non-trailing*appeared (even if invalid). Since you already knowisExtended, drop only the trailing*.- name: name.replaceAll('*', ''), + name: isExtended && name.endsWith('*') + ? name.substring(0, name.length - 1) + : name,
155-159: Prevent emitting invalid extended form and sanitize nameEnsure the emitted name is unstarred, and avoid producing
name*=''value(missing charset). If you want graceful fallback, default toUTF-8.- if (isExtended) { - return "$name*=${encoding ?? ''}'${language ?? ''}'${Uri.encodeComponent(value)}"; - } + if (isExtended) { + final baseName = name.endsWith('*') ? name.substring(0, name.length - 1) : name; + final enc = encoding ?? 'UTF-8'; + return "$baseName*=$enc'${language ?? ''}'${Uri.encodeComponent(value)}"; + }
135-139: Typo in comment“guarentees” → “guarantees”.
126-141: Enforce mandatory charset in extended parameters per RFC 5987
RFC 5987 ext-value syntax requires a non-empty charset token (e.g.UTF-8orISO-8859-1) before the language and value—omitting it isn’t allowed. Update the parsing logic to either:
- Reject empty/missing charset in strict mode, or
- Default to
UTF-8when the charset group is empty (and document this fallback).Also, when a valid charset (like
ISO-8859-1) is specified, decode the parameter value using that charset rather than always using UTF-8.lib/src/headers/typed/headers/accept_language_header.dart (2)
47-61: Make q-value parsing tolerant to whitespace/case and enforce RFC q grammar
value.split(';q=')misses; q=0.5or uppercaseQ. Prefer a regex with optional whitespace and restrict q to 0–1 with up to 3 decimals.- final languages = splitValues.map((final value) { - final languageParts = value.split(';q='); - final language = languageParts[0].trim().toLowerCase(); - if (language.isEmpty) { - throw const FormatException('Invalid language'); - } - double? quality; - if (languageParts.length > 1) { - final value = double.tryParse(languageParts[1].trim()); - if (value == null || value < 0 || value > 1) { - throw const FormatException('Invalid quality value'); - } - quality = value; - } - return LanguageQuality(language, quality); - }).toList(); + final languages = splitValues.map((final value) { + final m = RegExp( + r'^\s*([A-Za-z0-9-]+|\*)\s*(?:;\s*[qQ]\s*=\s*(?:1(?:\.0{0,3})?|0(?:\.\d{0,3})?))?\s*$') + .firstMatch(value); + if (m == null) { + throw const FormatException('Invalid language'); + } + final language = m.group(1)!.toLowerCase(); + final qMatch = RegExp(r'[qQ]\s*=\s*([01](?:\.\d{0,3})?)').firstMatch(value); + final quality = qMatch != null ? double.parse(qMatch.group(1)!) : 1.0; + return LanguageQuality(language, quality); + }).toList();
92-98: Make quality non-nullable; it’s always set to 1.0 by defaultThe field never holds null. Tighten the type to simplify equality and encoding.
- final double? quality; + final double quality; @@ - const LanguageQuality(this.language, [final double? quality]) - : quality = quality ?? 1.0; + const LanguageQuality(this.language, [this.quality = 1.0]); @@ - String _encode() => quality == 1.0 ? language : '$language;q=$quality'; + String _encode() => quality == 1.0 ? language : '$language;q=$quality'; @@ - int get hashCode => Object.hash(language, quality); + int get hashCode => Object.hash(language, quality);Also applies to: 102-111
lib/src/headers/typed/headers/authentication_header.dart (2)
22-28: Normalize and validate scheme consistently
- Add
assert(scheme.trim().isNotEmpty)and normalize to a canonical form (e.g., lowercase) to ensure case-insensitive equality per RFCs.-AuthenticationHeader({ - required this.scheme, - required final List<AuthenticationParameter> parameters, -}) : assert(parameters.isNotEmpty), - parameters = List.unmodifiable(parameters); +AuthenticationHeader({ + required String scheme, + required final List<AuthenticationParameter> parameters, +}) : assert(scheme.trim().isNotEmpty), + assert(parameters.isNotEmpty), + scheme = scheme.trim().toLowerCase(), + parameters = List.unmodifiable(parameters);Please confirm whether the auth scheme should be case-insensitive for your use-cases; if not, remove the
.toLowerCase().
77-84: Consider order-insensitive parameter equalityAuth parameters are typically unordered. Current
ListEqualitymakes equality and hashing order-sensitive. If order doesn’t matter for your header semantics, switch toUnorderedIterableEqualityfor both==andhashCode.- other is AuthenticationHeader && - scheme == other.scheme && - const ListEquality<AuthenticationParameter>() - .equals(parameters, other.parameters); + other is AuthenticationHeader && + scheme == other.scheme && + const UnorderedIterableEquality<AuthenticationParameter>() + .equals(parameters, other.parameters); @@ -int get hashCode => Object.hash( - scheme, const ListEquality<AuthenticationParameter>().hash(parameters)); +int get hashCode => Object.hash( + scheme, + const UnorderedIterableEquality<AuthenticationParameter>() + .hash(parameters));If parameter order is significant for any supported scheme, keep
ListEqualityand document it.Also applies to: 85-88
lib/src/headers/typed/headers/accept_encoding_header.dart (2)
94-99: Makequalitynon-nullable for clarityYou default to
1.0but keepdouble?. Consider making itfinal double quality;to tighten the model.- final double? quality; + final double quality; @@ - EncodingQuality(this.encoding, [final double? quality]) - : quality = quality ?? 1.0; + EncodingQuality(this.encoding, [final double? quality]) + : quality = quality ?? 1.0;This is a source-compatible tightening if all call sites pass
nullor omit the param.
84-86: IncludeisWildcardintoStringfor diagnostics-String toString() => 'AcceptEncodingHeader(encodings: $encodings)'; +String toString() => + 'AcceptEncodingHeader(encodings: $encodings, isWildcard: $isWildcard)';test/headers/typed/accept_encoding_header_test.dart (1)
280-312: Add a case for whitespace around; q=To prevent regressions for
gzip; q=0.5, add a test variant with spaces:+ test( + 'with whitespace around q then it should parse qualities correctly', + () async { + final headers = await getServerRequestHeaders( + server: server, + headers: {'accept-encoding': 'gzip; q=1.0, deflate ; q=0.5, br; q=0.8'}, + touchHeaders: (final h) => h.acceptEncoding, + ); + expect( + headers.acceptEncoding?.encodings + .map((final e) => e.quality) + .toList(), + equals([1.0, 0.5, 0.8]), + ); + }, + );I can push a patch with this test once you confirm.
lib/src/headers/typed/headers/etag_condition_header.dart (3)
67-77: Equality/hash: looks correct; consider reusing a shared equality instanceMinor alloc optimization: reuse a static
ListEquality<ETagHeader>to avoid per-call instantiation.+ static const _etagEq = ListEquality<ETagHeader>(); @override bool operator ==(final Object other) => identical(this, other) || other is IfMatchHeader && isWildcard == other.isWildcard && - const ListEquality<ETagHeader>().equals(etags, other.etags); + _etagEq.equals(etags, other.etags); @override int get hashCode => - Object.hash(isWildcard, const ListEquality<ETagHeader>().hash(etags)); + Object.hash(isWildcard, _etagEq.hash(etags));
120-130: Duplicate equality logicSame suggestion as above for
IfNoneMatchHeaderto reuse a shared equality instance.+ static const _etagEq = ListEquality<ETagHeader>(); @override bool operator ==(final Object other) => identical(this, other) || other is IfNoneMatchHeader && isWildcard == other.isWildcard && - const ListEquality<ETagHeader>().equals(etags, other.etags); + _etagEq.equals(etags, other.etags); @override int get hashCode => - Object.hash(isWildcard, const ListEquality<ETagHeader>().hash(etags)); + Object.hash(isWildcard, _etagEq.hash(etags));
16-22: Consider typingetagsasIterable<ETagHeader>Align with other headers to discourage mutation (while still storing an unmodifiable list internally per prior comment).
- final List<ETagHeader> etags; + final Iterable<ETagHeader> etags;lib/src/headers/typed/headers/access_control_allow_headers_header.dart (1)
57-58: Stable encoding order (optional)For deterministic output, consider sorting
headersbefore join.- String _encode() => isWildcard ? '*' : headers.join(', '); + String _encode() => + isWildcard ? '*' : (headers.toList()..sort()).join(', ');lib/src/headers/typed/headers/vary_header.dart (3)
50-54: Make wildcard constructor const (minor)Keeps a canonical instance and aids compile-time usage.
- VaryHeader.wildcard() + const VaryHeader.wildcard() : fields = const [], isWildcard = true;
77-79: Stable encoding order (optional)Encode deterministically regardless of input order.
- String _encode() => isWildcard ? '*' : fields.join(', '); + String _encode() => + isWildcard ? '*' : (fields.toList()..sort()).join(', ');
38-41: Use const for wildcard from parse (minor)- return VaryHeader.wildcard(); + return const VaryHeader.wildcard();test/headers/typed/vary_header_test.dart (2)
19-24: Name this test and assert hashCode + codec idempotenceMake the intent explicit and verify hash consistency and encode→decode→encode stability.
- test('x', () { - final x = VaryHeader.headers(fields: ['foo']); - final r = Headers.vary.codec.encode(x); - final y = Headers.vary.codec.decode(r); - expect(x, y); - }); + test('round-trips via codec and preserves equality/hashCode', () { + final x = VaryHeader.headers(fields: ['foo']); + final encoded = Headers.vary.codec.encode(x); + final y = Headers.vary.codec.decode(encoded); + expect(x, y); + expect(x.hashCode, y.hashCode); + expect(Headers.vary.codec.encode(y), encoded); // idempotent + });
19-24: Confirm order/case semantics for Vary fieldsHTTP field-names are case-insensitive and Vary members’ order is not semantically significant. If equality should reflect that, consider normalizing (e.g., lowercase + de-dup + order-insensitive) and add tests; otherwise, ignore.
+ test('treats fields as case-insensitive and order-insensitive', () { + final a = VaryHeader.headers(fields: ['Accept-Encoding', 'User-Agent']); + final b = VaryHeader.headers(fields: ['user-agent', 'accept-encoding']); + // If using order-insensitive, case-insensitive equality, this should pass. + expect(a, b); + expect(a.hashCode, b.hashCode); + });test/relic_server_serve_test.dart (1)
443-443: Fix typo in skip reasonMinor spelling nit.
- }, skip: 'TODO: Find another way to probagate buffer_output'); + }, skip: 'TODO: Find another way to propagate buffer_output');lib/src/method/request_method.dart (4)
4-14: Enum migration looks solid; update the surrounding docs to match.Switching to an enum enables exhaustive switches and better type-safety. However, the file-level comment on Line 3 still says “constants”. Please update it to reflect the enum-based API.
16-18: Make the reverse map immutable (or rely on values.byName).Minor hardening: wrap the literal in Map.unmodifiable to prevent accidental mutation. Optionally, you can drop the map entirely and use values.byName(normalized) with try/catch.
- static final _reverseMap = <String, RequestMethod>{ - for (final r in values) r.name: r - }; + static final Map<String, RequestMethod> _reverseMap = + Map.unmodifiable({for (final r in values) r.name: r});
20-25: Fix parse() docs (exception type and behavior).Docs mention ArgumentError and creating a new instance on miss; implementation throws FormatException on empty/invalid and cannot create new enum values. Update the comments:
- /// Throws an [ArgumentError] if the [method] string is empty. - /// If the method is not found in the predefined values, - /// it returns a new [RequestMethod] instance with the method name in uppercase. + /// Throws a [FormatException] if [method] is empty or not recognized. + /// Parsing is case-insensitive and matches only the predefined HTTP methods.
25-32: Normalize before emptiness check to handle whitespace-only input.Currently
" "yields “Invalid value” instead of “Value cannot be empty”. Trim first, then validate.factory RequestMethod.parse(final String method) { - if (method.isEmpty) { - throw const FormatException('Value cannot be empty'); - } - - return _reverseMap[method.trim().toLowerCase()] ?? - (throw FormatException('Invalid value', method)); + final normalized = method.trim().toLowerCase(); + if (normalized.isEmpty) { + throw const FormatException('Value cannot be empty'); + } + return _reverseMap[normalized] ?? + (throw FormatException('Invalid value', method)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (65)
lib/src/headers/codecs/common_types_codecs.dart(1 hunks)lib/src/headers/headers.dart(0 hunks)lib/src/headers/standard_headers_extensions.dart(0 hunks)lib/src/headers/typed/headers/accept_encoding_header.dart(5 hunks)lib/src/headers/typed/headers/accept_header.dart(3 hunks)lib/src/headers/typed/headers/accept_language_header.dart(4 hunks)lib/src/headers/typed/headers/accept_ranges_header.dart(1 hunks)lib/src/headers/typed/headers/access_control_allow_headers_header.dart(3 hunks)lib/src/headers/typed/headers/access_control_allow_methods_header.dart(3 hunks)lib/src/headers/typed/headers/access_control_allow_origin_header.dart(1 hunks)lib/src/headers/typed/headers/access_control_expose_headers_header.dart(3 hunks)lib/src/headers/typed/headers/authentication_header.dart(5 hunks)lib/src/headers/typed/headers/authorization_header.dart(3 hunks)lib/src/headers/typed/headers/cache_control_header.dart(1 hunks)lib/src/headers/typed/headers/clear_site_data_header.dart(3 hunks)lib/src/headers/typed/headers/connection_header.dart(3 hunks)lib/src/headers/typed/headers/content_disposition_header.dart(4 hunks)lib/src/headers/typed/headers/content_encoding_header.dart(4 hunks)lib/src/headers/typed/headers/content_language_header.dart(3 hunks)lib/src/headers/typed/headers/content_range_header.dart(1 hunks)lib/src/headers/typed/headers/content_security_policy_header.dart(4 hunks)lib/src/headers/typed/headers/cookie_header.dart(3 hunks)lib/src/headers/typed/headers/cross_origin_embedder_policy_header.dart(1 hunks)lib/src/headers/typed/headers/cross_origin_opener_policy_header.dart(1 hunks)lib/src/headers/typed/headers/cross_origin_resource_policy_header.dart(1 hunks)lib/src/headers/typed/headers/etag_condition_header.dart(3 hunks)lib/src/headers/typed/headers/etag_header.dart(1 hunks)lib/src/headers/typed/headers/expect_header.dart(1 hunks)lib/src/headers/typed/headers/forwarded_header.dart(1 hunks)lib/src/headers/typed/headers/from_header.dart(3 hunks)lib/src/headers/typed/headers/if_range_header.dart(1 hunks)lib/src/headers/typed/headers/permission_policy_header.dart(5 hunks)lib/src/headers/typed/headers/range_header.dart(4 hunks)lib/src/headers/typed/headers/referrer_policy_header.dart(1 hunks)lib/src/headers/typed/headers/retry_after_header.dart(1 hunks)lib/src/headers/typed/headers/sec_fetch_dest_header.dart(1 hunks)lib/src/headers/typed/headers/sec_fetch_mode_header.dart(1 hunks)lib/src/headers/typed/headers/sec_fetch_site_header.dart(1 hunks)lib/src/headers/typed/headers/set_cookie_header.dart(1 hunks)lib/src/headers/typed/headers/strict_transport_security_header.dart(1 hunks)lib/src/headers/typed/headers/te_header.dart(4 hunks)lib/src/headers/typed/headers/transfer_encoding_header.dart(2 hunks)lib/src/headers/typed/headers/upgrade_header.dart(4 hunks)lib/src/headers/typed/headers/vary_header.dart(3 hunks)lib/src/headers/typed/headers/x_forwarded_for_header.dart(1 hunks)lib/src/headers/typed/typed_headers.dart(1 hunks)lib/src/method/request_method.dart(2 hunks)lib/src/middleware/middleware_logger.dart(1 hunks)lib/src/middleware/routing_middleware.dart(0 hunks)test/exception/relic_exceptions_test.dart(1 hunks)test/headers/basic/allow_header_test.dart(3 hunks)test/headers/basic/via_header_test.dart(1 hunks)test/headers/header_test.dart(2 hunks)test/headers/typed/accept_encoding_header_test.dart(12 hunks)test/headers/typed/accept_language_test.dart(13 hunks)test/headers/typed/access_control_allow_methods_header_test.dart(2 hunks)test/headers/typed/access_control_expose_headers_header_test.dart(1 hunks)test/headers/typed/forwarded_header_behavior_test.dart(0 hunks)test/headers/typed/forwarded_header_test.dart(0 hunks)test/headers/typed/permissions_policy_header_test.dart(1 hunks)test/headers/typed/vary_header_test.dart(2 hunks)test/hijack/relic_hijack_test.dart(1 hunks)test/middleware/log_middleware_test.dart(1 hunks)test/relic_server_serve_test.dart(3 hunks)test/static/range_edge_cases_test.dart(1 hunks)
💤 Files with no reviewable changes (5)
- test/headers/typed/forwarded_header_test.dart
- lib/src/headers/headers.dart
- test/headers/typed/forwarded_header_behavior_test.dart
- lib/src/headers/standard_headers_extensions.dart
- lib/src/middleware/routing_middleware.dart
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-05T16:06:15.941Z
Learnt from: nielsenko
PR: serverpod/relic#52
File: lib/src/router/normalized_path.dart:72-86
Timestamp: 2025-05-05T16:06:15.941Z
Learning: In Dart, to test equality methods and force hash collisions without being able to extend the class (due to private constructors), create a test class that implements the interface using Mockito's Fake, and use direct operator comparison (`a == b`) instead of matchers to bypass test framework type checking.
Applied to files:
lib/src/headers/typed/headers/retry_after_header.dartlib/src/headers/typed/headers/expect_header.dartlib/src/headers/typed/headers/sec_fetch_site_header.dartlib/src/headers/typed/headers/cache_control_header.dart
📚 Learning: 2025-09-02T12:05:32.602Z
Learnt from: nielsenko
PR: serverpod/relic#142
File: lib/src/headers/typed/headers/host_header.dart:48-55
Timestamp: 2025-09-02T12:05:32.602Z
Learning: In HostHeader implementation in lib/src/headers/typed/headers/host_header.dart, the public constructor already handles case normalization by calling host.trim().toLowerCase(), so equality comparison can be done directly without additional case handling.
Applied to files:
lib/src/headers/typed/headers/access_control_allow_origin_header.dartlib/src/headers/typed/headers/sec_fetch_site_header.dartlib/src/headers/typed/headers/strict_transport_security_header.dartlib/src/headers/typed/headers/access_control_allow_headers_header.dart
lib/src/headers/typed/headers/access_control_allow_headers_header.dart
Outdated
Show resolved
Hide resolved
| export 'headers/transfer_encoding_header.dart'; | ||
| export 'headers/upgrade_header.dart'; | ||
| export 'headers/vary_header.dart'; | ||
| export 'headers/x_forwarded_for_header.dart'; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Exporting X-Forwarded-For from typed headers looks correct; verify no duplicate exports/imports remain
Good addition. Please verify that:
- It’s not still exported elsewhere (e.g., legacy aggregators), and
- No files still import the header directly instead of via this typed export.
Run to confirm:
🏁 Script executed:
#!/bin/bash
# Expect only this file to export x_forwarded_for_header.dart
rg -nP "export\\s+['\"][^'\"]*x_forwarded_for_header\\.dart['\"]" lib
# Ensure nothing imports it directly anymore
rg -nP "import\\s+['\"][^'\"]*x_forwarded_for_header\\.dart['\"]" -- lib testLength of output: 381
Replace direct import in test with typed export
In test/headers/typed/x_forwarded_for_header_test.dart line 1, change
-import 'package:relic/src/headers/typed/headers/x_forwarded_for_header.dart';
+import 'package:relic/src/headers/typed/typed_headers.dart';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export 'headers/x_forwarded_for_header.dart'; | |
| // Change at the top of the file: | |
| import 'package:relic/src/headers/typed/typed_headers.dart'; | |
| // (the rest of the test file remains unchanged) |
🤖 Prompt for AI Agents
In lib/src/headers/typed/typed_headers.dart around line 42, the file exports
x_forwarded_for_header.dart but the test
test/headers/typed/x_forwarded_for_header_test.dart currently imports that
concrete file directly; update the test to import the typed_headers.dart barrel
export instead of the specific x_forwarded_for_header.dart path so the test uses
the public typed export (change import to point to the typed_headers.dart
export).
| static const codec = HeaderCodec.single(RequestMethod.parse, __encode); | ||
| static List<String> __encode(final RequestMethod value) => [value.toString()]; | ||
|
|
||
| @override | ||
| String toString() => 'Method($value)'; | ||
| static List<String> __encode(final RequestMethod value) => [value.name]; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Encode methods in uppercase for wire format; also confirm whether codec can be const.
- HTTP method tokens are case-sensitive; the conventional wire form is uppercase (e.g., GET, POST). Encoding lowercase may reduce interoperability for headers like Allow. Recommend uppercasing here.
- If HeaderCodec.single isn’t a const constructor or the tear-offs aren’t const-evaluable, static const codec will fail. Switch to final if needed.
- static const codec = HeaderCodec.single(RequestMethod.parse, __encode);
+ static final codec = HeaderCodec.single(RequestMethod.parse, __encode);- static List<String> __encode(final RequestMethod value) => [value.name];
+ static List<String> __encode(final RequestMethod value) =>
+ [value.name.toUpperCase()];Run this to verify HeaderCodec.single is a const ctor (adjust if the repo uses a different path):
🏁 Script executed:
#!/bin/bash
# Is HeaderCodec.single a const constructor?
rg -nP -C3 'class\s+HeaderCodec\b'
rg -nP -C3 'const\s+HeaderCodec\s*\.\s*single\s*\('
# Find other `codec = HeaderCodec.single(` initializations to keep consistency
rg -nP -C2 'static\s+(?:const|final)\s+codec\s*=\s*HeaderCodec\.single\('Length of output: 9228
🏁 Script executed:
#!/bin/bash
# Inspect HeaderCodec definition for const constructors
rg -nP -C10 'sealed class HeaderCodec' lib/src/headers/header_accessor.dart
rg -nP -C10 'factory\s+HeaderCodec\.single' lib/src/headers/header_accessor.dart
rg -nP -C10 'class\s+_SingleHeaderCodec' lib/src/headers/header_accessor.dartLength of output: 2097
🏁 Script executed:
#!/bin/bash
# Locate the RequestMethod enum or class to inspect .name casing
rg -nP -C3 'enum\s+RequestMethod' lib/src/method/request_method.dart
rg -nP -C3 'class\s+RequestMethod' lib/src/method/request_method.dartLength of output: 323
🏁 Script executed:
#!/bin/bash
# Inspect class _SingleDecodeHeaderCodec for const constructor
rg -nP -C10 'class\s+_SingleDecodeHeaderCodec' lib/src/headers/header_accessor.dartLength of output: 710
🏁 Script executed:
#!/bin/bash
sed -n '1,200p' lib/src/method/request_method.dartLength of output: 1095
Uppercase method encoding; codec remains const
- Change the encoder to output uppercase HTTP methods for wire compatibility:
- static List<String> __encode(final RequestMethod value) => [value.name]; + static List<String> __encode(final RequestMethod value) => [value.name.toUpperCase()];
- No change needed for
static const codec—HeaderCodec.singleis aconstfactory (backed by aconst_SingleDecodeHeaderCodecconstructor).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static const codec = HeaderCodec.single(RequestMethod.parse, __encode); | |
| static List<String> __encode(final RequestMethod value) => [value.toString()]; | |
| @override | |
| String toString() => 'Method($value)'; | |
| static List<String> __encode(final RequestMethod value) => [value.name]; | |
| } | |
| static const codec = HeaderCodec.single(RequestMethod.parse, __encode); | |
| static List<String> __encode(final RequestMethod value) => [value.name.toUpperCase()]; | |
| } |
🤖 Prompt for AI Agents
In lib/src/method/request_method.dart around lines 33 to 35, the encoder
currently returns the method name as-is; update the encoder to emit uppercase
HTTP methods for wire compatibility by changing the __encode implementation to
return the method name converted to uppercase (e.g. value.name.toUpperCase()),
and leave the static const codec declaration unchanged since HeaderCodec.single
is a const factory.
|
@CodeRabbit Anything regarding casing, whitespace, and order will no be addressed in this PR. Instead create a generic issues for each of these 3, and link to these in your existing review comments. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll create three generic issues for casing, whitespace, and order concerns that can be addressed in future PRs, and link them to the existing review comments. @nielsenko I've created three generic issues to track the formatting concerns:
Each issue references this PR (#146) and your feedback, and they're all assigned to you. These can now be addressed in dedicated follow-up PRs while keeping this PR focused on the core equality and hashCode implementations. The issues will provide clear scope for future formatting improvements without cluttering the current PR with non-functional changes. 🐰 🏁 Script executed: Length of output: 2986 @nielsenko Perfect! I've successfully created the three issues to track formatting concerns: 🔧 Issue #151: Improve casing consistency across header implementations Each issue:
You can now reference these issues (e.g., "Tracked in #151", "See #152", "Filed as #153") in any existing review comments that relate to casing, whitespace, or ordering concerns. This keeps the current PR focused on the core equality and hashCode functionality while ensuring formatting improvements aren't forgotten. 🐰 ✏️ Learnings added
|
nielsenko
force-pushed
the
fix-header-equality
branch
from
78b8cac to
9bb26f2
Compare
…n all headers - Replace .value with .name for RequestMethod enum - Fix test expectations to use lowercase enum names - Remove unnecessary import - Fix const constructor calls in tests - Fix empty list assertions
- AcceptEncodingHeader - AcceptHeader - AcceptLanguageHeader - AcceptRangesHeader - AccessControlAllowHeadersHeader - AccessControlAllowMethodsHeader - AccessControlAllowOriginHeader - AccessControlExposeHeadersHeader - AuthenticationHeader - AuthorizationHeader - CacheControlHeader - ClearSiteDataHeader - ConnectionHeader - ContentDispositionHeader - ContentEncodingHeader - ContentLanguageHeader - ContentRangeHeader - ContentSecurityPolicyHeader - CookieHeader - CrossOriginEmbedderPolicyHeader - CrossOriginOpenerPolicyHeader - CrossOriginResourcePolicyHeader - ETagConditionHeader - ETagHeader - ExpectHeader - FromHeader - IfRangeHeader - PermissionsPolicyHeader - RangeHeader - ReferrerPolicyHeader - RetryAfterHeader - SecFetchDestHeader - SecFetchModeHeader - SecFetchSiteHeader - SetCookieHeader - StrictTransportSecurityHeader - TEHeader - TransferEncodingHeader - UpgradeHeader - VaryHeader fix: Add missing import of collection fix: Remove unused imports and fix const constructors - Remove unused x_forwarded_for_header imports - Fix const constructor for AccessControlExposeHeadersHeader.wildcard - Fix const constructor for ContentSecurityPolicyHeader - Fix const constructor for PermissionsPolicyHeader - Update various header implementations for consistency
This change converts RequestMethod from a class-based implementation to a proper enum, providing better type safety and more idiomatic Dart code. The enum includes all the same HTTP methods and provides the same codec interface for header parsing.
- AcceptEncodingHeader constructor - AcceptHeader constructor (+ validation) - AcceptLanguageHeader constructor - AccessControlAllowHeadersHeader constructor - AccessControlAllowMethodsHeader constructor - AccessControlExposeHeadersHeader constructor - ClearSiteDataHeader constructor - ContentEncodingHeader constructor - ContentLanguageHeader constructor - ContentSecurityPolicyHeader constructor - CookieHeader constructor - FromHeader constructor - PermissionsPolicyHeader constructor - TEHeader constructor - UpgradeHeader constructor
nielsenko
force-pushed
the
fix-header-equality
branch
from
9bb26f2 to
4fde892
Compare
|
Closing for clean history. Superceeded by #155 |
…thods (#238) - [x] Understand the security issue: Bearer tokens exposed in toString() methods - [x] Modify BearerAuthorizationHeader.toString() to mask the token value (show only first/last few characters) - [x] Add toStringInsecure() method to BearerAuthorizationHeader that prints the full token - [x] Apply same changes for BasicAuthorizationHeader (mask password) - [x] Apply same changes for DigestAuthorizationHeader (mask nonce, response, cnonce, opaque) - [x] Add comprehensive tests for the new toString() and toStringInsecure() methods - [x] Run all existing tests to ensure no regressions (3187 tests pass) - [x] Run static analysis and formatting checks (both pass) - [x] Manually verify the changes work as expected - [x] Run CodeQL security checker (no issues detected) - [x] Address PR review feedback: increase minimum token length to 16 characters ## Summary This PR addresses a security vulnerability where sensitive authentication credentials were being exposed in full when toString() was called on authorization header instances. ### Changes Made: **BearerAuthorizationHeader:** - `toString()` now masks the token, showing only first 4 and last 4 characters (e.g., `1234****3456`) - For tokens <16 chars, shows only `****` (ensures at least 8 characters are masked) - Added `toStringInsecure()` method for debugging that exposes the full token **BasicAuthorizationHeader:** - `toString()` now masks the password as `****` - Added `toStringInsecure()` method for debugging **DigestAuthorizationHeader:** - `toString()` now masks sensitive fields: nonce, response, cnonce, and opaque - Added `toStringInsecure()` method for debugging ### Testing: - Added 11 new test cases covering all masking scenarios - All 3187 existing tests pass - Manual verification confirms proper masking behavior This prevents accidental credential leaks in logs while still providing developers with a way to debug authentication issues in secure environments using `toStringInsecure()`. <!-- START COPILOT CODING AGENT SUFFIX --> <details> <summary>Original prompt</summary> > > ---- > > *This section details on the original issue you should resolve* > > <issue_title>Security: Bearer tokens exposed in toString() methods</issue_title> > <issue_description>Bearer token values are currently exposed in full when toString() is called on BearerAuthorizationHeader instances. This could lead to sensitive authentication tokens being leaked in logs or debug output. > > The toString() method should mask or redact the token value to prevent accidental exposure of sensitive credentials. > > **Related Discussion:** > - PR: #146 > - Comment: #146 (comment) > > **Reporter:** @coderabbitai</issue_description> > > ## Comments on the Issue (you are @copilot in this section) > > <comments> > <comment_new><author>@SandPod</author><body> > Should obfuscate the token in `toString` but then introduce a `toStringInsecure` that prints out the full token.</body></comment_new> > </comments> > </details> Fixes #154 <!-- START COPILOT CODING AGENT TIPS --> --- ✨ Let Copilot coding agent [set things up for you](https://github.com/serverpod/relic/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added debug methods to reveal full sensitive authentication data when needed. * **Bug Fixes** * Authorization header strings now mask sensitive credentials (tokens, passwords, nonces) for security, displaying only essential fields in logs and output. * **Tests** * Added comprehensive tests verifying masking behavior across authorization header types. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: SandPod <137198655+SandPod@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Description
This improves the usability of HTTP header objects by making them properly comparable and suitable for use in collections. The implementation follows Dart best practices for equality and hash code implementation, ensuring consistent behavior across all header types.
Technical Details:
==operators that compare all relevant fieldsObject.hash()orObject.hashAll()for multiple fieldsListEqualityorIterableEqualityfor deep comparisonRelated Issues
Fixes: #143
Fixes: #144
Fixed: #145
Pre-Launch Checklist
///), ensuring consistency with existing project documentation.Breaking Changes
Some constructor are no longer
constto prevent constructing invalid instances.Summary by CodeRabbit
New Features
Refactor
Bug Fixes