Skip to content

Make URL normalization idempotent - #48

Open
binggao1230 wants to merge 1 commit into
PuerkitoBio:masterfrom
binggao1230:fix-normalize-idempotence
Open

binggao1230 wants to merge 1 commit into
PuerkitoBio:masterfrom
binggao1230:fix-normalize-idempotence

Conversation

@binggao1230

Copy link
Copy Markdown

NormalizeURLString/NormalizeURL are not idempotent for the library's own recommended greedy flag sets: Normalize(Normalize(x)) != Normalize(x). Since purell exists to produce stable dedup/cache keys, idempotence is a defining property, and the value a second pass reaches is exactly the one already intended (all trailing slashes stripped) — so this is a convergence bug, not a behaviour change.

Two causes, both in purell.go:

  1. removeTrailingSlash stripped a single trailing slash, so a path ending in two or more slashes needed several passes to collapse: http://host// → http://host/ → http://host. This breaks idempotence with the standalone FlagRemoveTrailingSlash.
  2. NormalizeURL applied the flag pass once. Flags such as removeDirectoryIndex run before removeTrailingSlash, and %2F decodes to / on parse, so a trailing slash can hide a segment a later pass would remove: http://host/index.html/ (UnsafeGreedy) → http://host/index.html → http://host.

Minimal repro:

n1, _ := purell.NormalizeURLString("http://host//", purell.FlagRemoveTrailingSlash) // "http://host/"
n2, _ := purell.NormalizeURLString(n1, purell.FlagRemoveTrailingSlash)              // "http://host"  (!= n1)

Fix: removeTrailingSlash strips every trailing slash (a root / still collapses to ""), and NormalizeURL iterates the flag pass to a bounded fixed point.

The converged output equals the value repeated normalization already reached, so no existing expected value changes — the full existing suite passes unmodified. A property fuzz over ~325k random URLs per flag set goes from:

FlagsUsuallySafeGreedy   not-idempotent 13798 -> 0
FlagsUnsafeGreedy        not-idempotent   232 -> 0
FlagsSafe, *NonGreedy    already 0

TestNormalizeIsIdempotent covers the standalone flag, both greedy sets, and the directory-index / %2F sub-cases.

NormalizeURL ran the flag pass exactly once, so Normalize(Normalize(x))
could differ from Normalize(x) for the recommended greedy flag sets:

  - removeTrailingSlash stripped only one trailing slash, so a path ending
    in two or more slashes needed several passes to fully collapse; and
  - flags such as removeDirectoryIndex run before removeTrailingSlash, so a
    trailing slash (or a %2F decoded to one) could hide a segment that a
    later pass would then remove.

Strip every trailing slash in removeTrailingSlash (a root "/" still
collapses to ""), and iterate the flag pass in NormalizeURL to a bounded
fixed point. The converged value equals the one repeated normalization
already reached, so no existing behaviour changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant