fix(request): decode Query and ContentFields values exactly once - #570
Open
freitasjca wants to merge 3 commits into
Open
freitasjca wants to merge 3 commits into
freitasjca wants to merge 3 commits into
Conversation
InitializeQuery and InitializeContentFields now create THorseCoreParam with ADecodeValues=False, as InitializeParams already does. Every store path already holds DECODED values: InitializeQuery runs DecodeParam on each key and value, WebBroker ContentFields are decoded by ExtractHTTPFields, and non-WebBroker providers store decoded values. But GetItem/TryGetValue decoded AGAIN on every read and wrote the result back: ?v=100%25 -> EConvertError "Error decoding URL style (%XX) encoded string" ?v=50%25off -> EConvertError "Invalid URL encoded character (%of)" ?v=a%2B%2541 -> silently "a A" instead of "a+%41" Field(...).AsString read the stored value and was always correct. This affects the default Indy provider too. User-reported as an intermittent HTTP 500. Validated on Windows/Delphi with a three-way read test (Field, [v], [v] again): 131 passed / 8 failed before, 139/139 after, on top of 3.3.5.
Tests.Horse.Request.DecodeOnce (unit, no network): stores already-decoded values in THorseRequest.Query / ContentFields / Params and asserts that Field, the first indexed read, a repeated indexed read and TryGetValue all return them unchanged. Cases: '100%', '50%off', 'a+%41', plus a no-percent control. 'a+%41' fails without the fix on FPC too, where HTTPDecode may not raise. Tests.Integration.QueryDecode (port 9126): GET ?v=100%25 / 50%25off / a%2B%2541 and a form-urlencoded PUT v=100%25 through the default provider; the handler reads each value twice and then via Field. Validated on Windows/Delphi: 10 new failures on 3.3.5 (EConvertError "Error decoding URL style (%XX)", "Invalid URL encoded character", silent "a A", HTTP 500s), all 13 new tests pass with the fix. The remaining suite failures are identical with and without it.
freitasjca
added a commit
to freitasjca/horse-provider-mormot
that referenced
this pull request
Sep 14, 2026
… pairs FIX-DECODE-ONCE-1 companion to HashLoad/horse#570. Query and application/x-www-form-urlencoded keys and values are now URL-decoded when stored, like every other Horse provider. The decoder is tolerant: malformed %xx is kept, + is a space, and invalid UTF-8 falls back to the raw text, because it runs outside any handler's try/except. Also fixes form-urlencoded parsing: the WHOLE "name=value" text was stored as the key with an empty value, so Req.ContentFields['name'] always returned ''. Tests 41-45 (same as horse-provider-crosssocket), plus the HARNESS-CB-1 test client fix (SetEvent in finally, callback exceptions reported as status -1). Validated on Windows/Delphi: 15 new failures before the fix, 124/124 after, with the #570 Horse fix. Against an unpatched Horse, Req.Query[...] still double-decodes, but Field() and form fields are now correct.
freitasjca
added a commit
to freitasjca/horse-provider-ics
that referenced
this pull request
Sep 14, 2026
FIX-DECODE-ONCE-1 companion to HashLoad/horse#570 (Horse.Provider.ICS.Request): query and form-urlencoded keys and values are decoded once at store time with a tolerant decoder. Form parsing stored the whole "name=value" as the key, so Req.ContentFields['name'] was always ''. FIX-ICS-UTF8-BODY (Horse.Provider.ICS): ICS's AnswerString encodes the body with TEncoding.Default, which is ANSI on Windows, so non-ASCII response characters went out as ANSI under charset=utf-8. All three call sites now use AnswerBodyTB(TEncoding.UTF8.GetBytes(Body)), public since ICS 9.2 and on the same AnswerStream path. POSIX was unaffected. Tests: 41-45 added. HARNESS-CB-1 test client fix (SetEvent in finally, callback exceptions reported as status -1). Test 15 now sends Content-Length: 0: TCrossHttpClient omits the header for an empty body, and ICS rejects a POST without it with its own HTML 400 before the provider runs (curl: 10/10), which left the test's body check unreachable. Validated on Windows/Delphi: 117/117 with the #570 Horse fix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Req.Query[...]andReq.ContentFields[...]decode a value that was already URL-decoded, and do it again on every read. With the default Indy provider:Req.Query['v']today?v=100%25EConvertError: Error decoding URL style (%XX) encoded string at position 4→ HTTP 500100%?v=50%25offEConvertError: Invalid URL encoded character (%of) at position 3→ HTTP 50050%off?v=a%2B%2541a A(no error, wrong value)a+%41It was reported to us as an intermittent HTTP 500. It's intermittent because
DecodeParamskips values without a%, so it only fails when the data contains one.Cause
Every path that fills these collections already stores decoded values:
InitializeQuerycallsDecodeParamon each key and value before storing them.ContentFieldsare decoded byExtractHTTPFields.Since 3.3.0,
THorseCoreParam.GetItem/TryGetValuealso runDecodeParamon read, then write the result back, so each repeated read decodes again.Field(...).AsStringreads the dictionary directly and always returned the right value, so two accessors disagree on the same request.Fix
InitializeQueryandInitializeContentFieldsnow createTHorseCoreParamwithADecodeValues = False, whichInitializeParamsalready does. Two lines insrc/Horse.Request.pas, no other behaviour change.Tests
tests/src/tests/Tests.Horse.Request.DecodeOnce.pas(unit): stores decoded values inQuery,ContentFieldsandParamsand checks thatField,[v], a repeated[v]andTryGetValuereturn them unchanged.tests/src/tests/Tests.Integration.QueryDecode.pas(integration, port 9126): the table above through the default provider, plus a form-urlencodedPUT.Console.dpr.Validated on Windows / Delphi 12 (Indy):
Not run on FPC. The change is the same constructor call
InitializeParamsalready makes.