fix: encoded value be decoded issue #8701
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes issues related to decoding of percent-encoded query parameter values by introducing a skipDecode option and updating the encoding logic to better preserve special characters.
- Added a skipDecode option to deconstructQueryStringToParams to avoid unintended decoding.
- Modified flexibleEncodeComponent usage in buildQueryParameter to keep comma characters intact.
- Corrected typos in comments and adjusted test cases for improved validation.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/insomnia/src/utils/url/querystring.ts | Introduces skipDecode option and improves encoding/decoding logic |
| packages/insomnia/src/utils/url/querystring.test.ts | Adds new tests covering encoded values and edge-case token handling |
Comments suppressed due to low confidence (3)
packages/insomnia/src/utils/url/querystring.test.ts:146
- The test case for 'ENC' failing suggests that the handling of reserved encoded tokens may need further refinement. Consider updating flexibleEncodeComponent to either treat 'ENC' as a reserved prefix or add documentation on how such cases should be handled.
it.fails('build query param with __ENC__ returns incorrect result', () => {
packages/insomnia/src/utils/url/querystring.test.ts:151
- The failing test for 'RAW' indicates that inputs containing 'RAW' may not be processed as expected. It is recommended to review the token reversion logic in flexibleEncodeComponent to clarify whether these substrings are reserved and handle them accordingly.
it.fails('build query param with __RAW__ returns incorrect result', () => {
packages/insomnia/src/utils/url/querystring.test.ts:156
- The test expecting an error when processing a 'RAW' prefixed value ('__RAW__AA') may indicate an inconsistency in error handling. Consider either updating the implementation to throw an error for reserved tokens or adjusting the test expectations to reflect the intended behavior.
it.fails('build query param with __RAW__AA throws error', () => {
| expect(str).toBe('foo=1%2C'); | ||
| }); | ||
|
|
||
| it.fails('build query param with __ENC__ returns incorrect result', () => { |
There was a problem hiding this comment.
There are some risks associated with the current solution.
| @@ -245,10 +247,7 @@ export const smartEncodeUrl = (url: string, encode?: boolean, options?: IQuerySt | |||
| * @param str string to encode | |||
| * @param ignore characters to ignore | |||
| */ | |||
| export const flexibleEncodeComponent = (str = '', ignore = '') => { | |||
| // Sometimes spaces screw things up because of url.parse | |||
| str = str.replace(/%20/g, ' '); | |||
There was a problem hiding this comment.
This line was from 9 years before, I guess the original proposal is to avoid %20 to be double-encoded when decodeURI or decodeURIComponent throws errors. I think it's safe to remove from the current code since it's using __ENC__ to avoid double-encoding now.
| } | ||
| // Skip decode if already encoded, to avoid some decoded characters cannot be encoded again. | ||
| // E.g. %4E -> N, encodeURIComponent(decodeURIComponent('%4E')) is N which is not expected. | ||
| const qsParams = deconstructQueryStringToParams(parsedUrl.query, true, { strictNullHandling, skipDecode: true }); |
There was a problem hiding this comment.
buildQueryStringFromParams will encode the name and value, and already ensure the string is not double-encoded. It's unnecessary to call flexibleEncodeComponent here.
And decoding the string will cause some strings cannot be encoded back since decodeURI and encodeURI are not exact opposites
Background
When users pass percent-encoded strings in the query parameters, the encoded characters will be decoded. E.g. %4E.
Root Cause
smartEncodeUrlcallsdeconstructQueryStringToParamsbeforebuildQueryStringFromParams,deconstructQueryStringToParamsalways decodes the params, the original proposal I guess is to keep all the strings to be only encoded once. AlthoughencodeURIComponentanddecodeURIComponentare often thought of as complementary, they are not exact opposites. This is becausedecodeURIComponentwill decode all percent-encoded sequences, whereasencodeURIComponentonly encodes necessary characters.,in the string, it'll be decoded to%2C, which seems it cause issues before, so%2Cwill always be replaced with,. But if there is%2Cin the original input, we should consider it as%2Cinstead of,.Changes
skipDecodetodeconstructQueryStringToParamsignoreparam instead of replace all%2Cto,Closes #975