Repository navigation
fix(http): decode HTML entities in filenames in one pass - #1651
Merged
monkeyWie merged 2 commits intoNov 12, 2026
Merged
Conversation
unescapeHTMLEntities applied its replacements while ranging over a map, so the order changed from run to run. An escaped ampersand such as "a&lt;b.txt" came out as "a<b.txt" or, when & was replaced first, as the double-decoded "a<b.txt". Replace all entities in a single pass with strings.NewReplacer and cover the escaped-ampersand cases in TestUnescapeHTMLEntities, running each case repeatedly so an order-dependent implementation fails.
Member
|
Nice catch on the map-iteration nondeterminism — the 200x loop in the tests plus the fail/pass CI links on your fork make this a textbook bug fix. Verified locally: all 7 subtests pass with the fix and the two new cases reliably fail against the old implementation. Merging, thanks @ChandraChaan! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1651 +/- ##
==========================================
- Coverage 73.55% 73.53% -0.02%
==========================================
Files 106 106
Lines 16894 16884 -10
==========================================
- Hits 12426 12416 -10
Misses 3168 3168
Partials 1300 1300 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
monkeyWie
approved these changes
Nov 12, 2026
monkeyWie
left a comment
monkeyWie
left a comment
Member
There was a problem hiding this comment.
Verified locally — all subtests pass, and the new cases reliably fail against the old implementation.LGTM
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
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.
unescapeHTMLEntitiesininternal/protocol/http/helper.goapplies its replacements while ranging over a map. Go randomizes map iteration order and the replacements are chained, so the result depends on which entity is replaced first.For a filename that was HTML-escaped once, such as
a<b.txtsent asa&lt;b.txtin aContent-Dispositionheader, the result is eithera<b.txt(correct) ora<b.txt(when&is replaced before<, the text is decoded twice). It varies from run to run. The same applies to&quot;,&gt;,&#39;and&apos;.This replaces the map loop with a single
strings.NewReplacer, which scans left to right once, so every entity is decoded exactly once and the result is deterministic.TestUnescapeHTMLEntitiesgets two cases for the escaped-ampersand inputs, and each case now runs 200 times, so an order-dependent implementation fails reliably.Verified on my fork (
testworkflow, "Test check" job runninggo test):unescapeHTMLEntities() = "a<b.txt", want "a<b.txt"and... "a\"b.txt", want "a"b.txt", thenFAIL github.com/GopeedLab/gopeed/internal/protocol/http)ok github.com/GopeedLab/gopeed/internal/protocol/http 170.251s)On that fix run,
Test webview integration (macOS)was cancelled before it ran, so I haven't seen it pass; every other job succeeded. The first attempt of the test-only run stalled on the apt install step, so I cancelled it and re-ran it; the link above is the re-run.