🐛 protect encoded dots in keys during decoding when decodeDotInKeys is false#36
🐛 protect encoded dots in keys during decoding when decodeDotInKeys is false#36
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughGuards percent-decoding of keys in query-string parsing so encoded dots (%2E/%2e) aren't treated as separators when dot-splitting is enabled but decodeDotInKeys is disabled; expands dot-decoding to recognise both %2E and %2e and enables dot-notation when either allowDots or decodeDotInKeys is set. No public API changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Input (query string)
participant Parser as _parseQueryStringValues
participant Guard as KeyGuard (escape %2E/%2e → %252E/%252e)
participant Decoder as Percent Decoder
participant Splitter as _splitKeyIntoSegments
participant Builder as _parseObject / Result Map
Client->>Parser: part ("a.b=value" / "a%2E b=value" / "bareKey")
alt key is bare
Parser->>Guard: keyInput = entire part
else contains '='
Parser->>Guard: keyInput = part.slice(0,pos)
end
alt allowDots && !decodeDotInKeys && keyInput contains "%2"
Guard->>Decoder: guarded keyInput (escaped %2E/%2e)
else
Guard->>Decoder: keyInput
end
Decoder-->>Parser: decodedKey
Parser->>Splitter: split using (allowDots || decodeDotInKeys)
Splitter-->>Builder: segments (dots/brackets)
Parser->>Builder: assign value into result map
Builder-->>Client: final params map
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #36 +/- ##
==========================================
- Coverage 95.33% 95.24% -0.10%
==========================================
Files 14 14
Lines 815 820 +5
==========================================
+ Hits 777 781 +4
- Misses 38 39 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 (2)
lib/src/extensions/decode.dart (2)
150-158: Correctly preserves encoded dots in bare keys; consider extracting a small helper to deduplicateThe guarding logic is sound and matches the stated intent: by double-encoding
%2E/%2ein the key before decoding, the dot never appears as a literal and won’t be dot-split. This is exactly what we want whenallowDots && !decodeDotInKeys.To reduce duplication and make the intent reusable, consider extracting the guard into a tiny helper and calling it from both key paths.
Apply this diff within this block:
- String keyInput = part; - if (options.allowDots && - !options.decodeDotInKeys && - keyInput.contains('%2')) { - keyInput = - keyInput.replaceAll('%2E', '%252E').replaceAll('%2e', '%252e'); - } - key = options.decoder(keyInput, charset: charset); + final String keyInput = _guardEncodedDotsInKey(part, options); + key = options.decoder(keyInput, charset: charset);And add this helper (inside the same extension is fine):
static String _guardEncodedDotsInKey(String s, DecodeOptions options) { if (options.allowDots && !options.decodeDotInKeys && s.contains('%2')) { // Case-insensitive replace: %2E / %2e → %252E (so a single decode yields %2E) return s.replaceAll(RegExp(r'%2e', caseSensitive: false), '%252E'); } return s; }Also, tiny nit: the top-of-file implementation note still claims “No behavioural changes are introduced here; comments only.” That line is now outdated; consider amending it.
161-170: Same protection forkey=pairs; unify via the helper for consistencyThis mirrors the bare-key case and is correct: values remain unaffected, only the key slice is guarded. For consistency and readability, reuse the same helper here.
Apply this diff within this block:
- String keyInput = part.slice(0, pos); - if (options.allowDots && - !options.decodeDotInKeys && - keyInput.contains('%2')) { - keyInput = - keyInput.replaceAll('%2E', '%252E').replaceAll('%2e', '%252e'); - } - key = options.decoder(keyInput, charset: charset); + final String keyInput = _guardEncodedDotsInKey(part.slice(0, pos), options); + key = options.decoder(keyInput, charset: charset);If a custom
options.decoderis ever supplied that repeatedly decodes until no percent-escapes remain (nonstandard), this guard would be bypassed. The default single-pass behaviour is expected; if you support custom decoders, it’s worth documenting that they should only perform a single decode pass.Would you like me to add unit tests for:
- bare key:
a%2EbwithallowDots=true,decodeDotInKeys=false→ key remainsa%2Eb- key=value:
a%2Eb=cwith the same options- control cases where
decodeDotInKeys=true(encoded dot should participate in dot splitting)- mixed-case
%2E/%2e?
📜 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 (1)
lib/src/extensions/decode.dart(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
lib/src/extensions/decode.dart (3)
150-159: Good, targeted protection of encoded dots before decoding; consider deduping into a helper.The approach of re-encoding
%2E/%2eto%252E/%252ewhenallowDotsis true anddecodeDotInKeysis false correctly prevents premature dot-splitting. The same logic appears again below for thekey=valuepath — factoring it into a small helper will reduce duplication and keep the behaviour consistent.Apply this diff locally to reuse a helper:
- // Protect %2E/%2e in keys so dot-splitting doesn’t see them unless decodeDotInKeys is true. - String keyInput = part; - if (options.allowDots && - !options.decodeDotInKeys && - keyInput.contains('%2')) { - keyInput = - keyInput.replaceAll('%2E', '%252E').replaceAll('%2e', '%252e'); - } - key = options.decoder(keyInput, charset: charset); + // Protect %2E/%2e in keys so dot-splitting doesn’t see them unless decodeDotInKeys is true. + final String keyInput = _protectEncodedDotsInKey(part, options); + key = options.decoder(keyInput, charset: charset);And add this helper inside the same extension (for example, near the bottom of the file):
static String _protectEncodedDotsInKey(String input, DecodeOptions options) { if (options.allowDots && !options.decodeDotInKeys && input.contains('%2')) { // Re-encode '%' of '%2E' -> '%25', so the decoder yields '%2E' (not '.'). return input.replaceAll('%2E', '%252E').replaceAll('%2e', '%252e'); } return input; }Also consider adding tests that cover:
- allowDots=true, decodeDotInKeys=false with keys:
a%2Eb,a%2eb(should not split on '.').- allowDots=true, decodeDotInKeys=true with the same keys (should decode to dots and then split).
- Keys that already contain
%252Eto ensure no double protection occurs.
161-170: Mirror the same protection via a shared helper to avoid drift between branches.This duplicates the protection logic from the bare-key path. Using a shared helper keeps behaviour identical and easier to maintain.
Apply this diff:
- // Protect %2E/%2e in the key slice only; values decode normally. - String keyInput = part.slice(0, pos); - if (options.allowDots && - !options.decodeDotInKeys && - keyInput.contains('%2')) { - keyInput = - keyInput.replaceAll('%2E', '%252E').replaceAll('%2e', '%252e'); - } - key = options.decoder(keyInput, charset: charset); + // Protect %2E/%2e in the key slice only; values decode normally. + final String keyInput = _protectEncodedDotsInKey(part.slice(0, pos), options); + key = options.decoder(keyInput, charset: charset);If you adopt the helper, ensure the corresponding tests also cover
key=valuepairs, not just bare keys (e.g.,a%2Eb=1).
319-319: Potential behaviour change: dot-notation now enabled whendecodeDotInKeysis true — confirm intent.Using
allowDots: options.allowDots || options.decodeDotInKeysmeans that plain dots (e.g.,a.b) will now be treated as separators even whenallowDotsis false, as long asdecodeDotInKeysis true. This broadens the scope of dot-splitting and may break existing callers that wanted only percent-encoded dots to be treated specially, but not plain dots.
If this is intentional, please:
- Document that
decodeDotInKeysalso enables dot-notation splitting.- Add tests that assert:
- allowDots=false, decodeDotInKeys=true:
a.b=1→{'a': {'b': '1'}}.- allowDots=false, decodeDotInKeys=true:
a%2Eb=1→{'a': {'b': '1'}}.- allowDots=false, decodeDotInKeys=false:
a.b=1remains a single keya.b.If this is not desired, consider reverting to the previous behaviour:
- allowDots: options.allowDots || options.decodeDotInKeys, + allowDots: options.allowDots,
📜 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 (1)
lib/src/extensions/decode.dart(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (1)
lib/src/extensions/decode.dart (1)
275-277: Case-insensitive%2Edecoding is a nice improvement.Decoding both
%2Eand%2ewhendecodeDotInKeysis enabled removes a subtle edge case and aligns expectations.
…f decodeDotInKeys option
1 participant
This pull request updates the decoding logic in the
extension _$Decode on QSwithinlib/src/extensions/decode.dartto improve how percent-encoded dots (%2E/%2e) are handled in query string keys, particularly when dot splitting is enabled but decoding of dots in keys is not desired. The main change protects encoded dots from being prematurely interpreted as key separators, ensuring more accurate parsing.Key improvements to query string key decoding:
%2E/%2ein keys and replace them with%252E/%252ewhenallowDotsis true anddecodeDotInKeysis false, preventing accidental dot splitting during decoding.=sign), ensuring consistent behavior in all parsing scenarios.