fix(e2b): correct query parsing for ListSandboxes filters - #821
fix(e2b): correct query parsing for ListSandboxes filters#821AnshulPatil2005 wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #821 +/- ##
==========================================
+ Coverage 83.08% 83.10% +0.01%
==========================================
Files 259 259
Lines 22555 22552 -3
==========================================
+ Hits 18739 18741 +2
+ Misses 3093 3089 -4
+ Partials 723 722 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
d35dea5 to
845e812
Compare
r.URL.Query() has already unescaped the query, so the extra url.QueryUnescape on metadata decoded it twice. A value carrying an encoded percent sign arrived as "%" and was then read as the start of a new escape, failing the request with 400 invalid URL escape. The loop also read only values[0], so a repeated parameter silently lost everything after the first: state=running&state=paused filtered on running alone, and a second metadata parameter was dropped. An invalid state reported the whole parameter rather than the token that was rejected. Drop the second unescape, read every value, and name the offending token. Fixes openkruise#804 Signed-off-by: Anshul <anshulpatil1022@gmail.com>
845e812 to
afb4dc4
Compare
2 participants
I. Describe what this PR does
Three fixes in
parseListSandboxesRequest, all from #804.r.URL.Query()has already unescaped the query, so the extraurl.QueryUnescapeon metadata decoded it a second time. Dropped.The loop read only
values[0], so a repeated parameter lost everything after the first.stateandmetadatanow read every value.statestill accepts the comma separated form as well.An invalid state reported the whole parameter instead of the token that failed, so
state=running,foosaidnot: 'running,foo'. It now namesfoo.The
defaultbranch is left alone. It maps an unknown parameter to a single metadata key, and a map holds one value per key either way, so pickingvalues[0]over the last one is arbitrary rather than wrong.II. Does this pull request fix one issue?
fixes #804
III. Describe how to verify it
go test ./pkg/servers/e2b/ -run TestParseListSandboxesRequest -count=1Four of the seven cases fail on master and pass here. The other three cover behaviour that should not change: the comma separated state form, encoded separators still splitting into pairs, and forbidden metadata keys still being rejected.
IV. Special notes for reviews
One correction to the issue. It gives
?metadata=progress=100%as the repro for the 400, but a bare%never reaches this function:url.ParseQueryfails on the invalid escape andr.URL.Query()drops the parameter, so metadata is silently empty and no error is returned.The 400 comes from the opposite case, a correctly encoded value.
?metadata=note=50%25offarrives here already decoded tonote=50%off, and the second unescape then reads%ofas an escape sequence and fails. So the bug is real but it hits clients that encode properly rather than clients that do not, which seemed worth getting right in the test. That case ispercent sign in a metadata value survives.TestParseCreateSandboxRequest,TestCreateSandboxandTestCloneSandboxfail for me on a clean master too, so they look unrelated to this.