revert PR #3009 changes to just disabling path escaping by default in static methods/middleware#3016
Conversation
…sible Unauthorized static file disclosure Fix StaticDirectoryHandler and Router Inconsistent Escaping leading to possible Unauthorized static file disclosure
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3016 +/- ##
==========================================
+ Coverage 93.17% 93.29% +0.11%
==========================================
Files 43 43
Lines 4501 4621 +120
==========================================
+ Hits 4194 4311 +117
- Misses 189 190 +1
- Partials 118 120 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
vishr
left a comment
There was a problem hiding this comment.
Review: default is solid, but two non-default paths still disclose protected files
Reviewed this branch (builds, go vet, and go test -race ./... all green) with a focus on the two advisories. The default config is verified to close both GHSA-vfp3-v2gw-7wfq and GHSA-3pmx-cf9f-34xr — nice and simple. Findings below are about non-default modes, a now-dead field, and docs. One is a blocker.
🔴 Critical
C1 — UseEscapedPathForMatching=true still leaks the protected file, and the new doc comment recommends that mode.
With RouterConfig{UseEscapedPathForMatching: true}, GET /public%2F..%2Fadmin%2Fsecret.txt returns 200 + the protected file. The router decodes the path itself, so StaticDirectoryHandler receives the wildcard already as public/../admin/secret.txt; path.Clean collapses it to admin/secret.txt and serves it past the /admin/* guard. Disabling handler-side unescaping can't help here — the decode happened upstream in the router.
This is GHSA-3pmx variant 2, and the new note in StaticDirectoryHandler ("Enabling RouterConfig.UseEscapedPathForMatching makes path escaping in static files and router consistent") actively steers users toward the exploitable config. There is currently no test exercising UseEscapedPathForMatching with static files.
Suggested fix: reject any .. path segment in the resolved wildcard (the pathutil.HasDotDotSegment approach in #3015). It closes this variant regardless of who did the decoding, and is a single fs.ValidPath-style invariant rather than an encoding denylist. Alternatively, if we decide this mode is "documented, won't-fix," the docs should stop recommending it.
🟠 Important
I2 — opt-in mode silently reopens the GHSA-vfp3 encoded-slash bypass.
Since the encoded-separator guard was removed, EnablePathUnescaping*=true turns GET /admin%2Fprivate.txt from a deliberate, logged 404 into a 200 file-serve with no signal. It's documented as a footgun, but at request level it's a silent ACL cross. Cheap mitigation: keep the %2F/%5C rejection even in the unescape branch — no legitimate filename contains an encoded separator, so it costs nothing and preserves vfp3 protection for opt-in users.
I1 — middleware.StaticConfig.DisablePathUnescaping is now a silent no-op.
The deprecated field is read nowhere (the handler keys only on EnablePathUnescaping). On upgrade, a middleware user on the old default silently loses unescaping, and one who set DisablePathUnescaping: true has it ignored. Either honor it or change the comment to state plainly that the field is now ignored.
I3 — cmp.Or(c.Request().URL.RawPath, …) directory redirect feeds raw client-encoded input past a byte-literal sanitizeURI.
sanitizeURI only checks a literal leading ///\\; it's not percent-aware, so an encoded authority such as /%2F%2Fevil.example/dir on a directory request could land in the Location header. Lower confidence (browser normalization may save most cases), but it's exposure the decoded Path form didn't have. Consider redirecting with decoded Path, or making sanitizeURI percent-aware.
🟡 Suggestions
- Tests: add (a) a
UseEscapedPathForMatching: truestatic case [C1]; (b) encoded dot-dot (%2E%2E) at theecho.StaticDirectoryHandlerlevel (middleware has it, the echo path doesn't); (c) a positive opt-in test proving/hello%20world.txtis actually served when the flag is on. fs.Staterror collapses to bareErrNotFound, discardingEACCES/I-O faults with no wrap or log (pre-existing) — wortherrors.Is(err, fs.ErrNotExist)while we're here.- Docs/grammar: "forbit" → "forbid" and a broken run-on sentence in the
StaticDirectoryHandlernote; document the empty-RawPathcmp.Orfallback so it isn't "simplified" away later; the twoEnablePathUnescaping*doc copies diverge (the middleware one omits theUseEscapedPathForMatchingguidance).
✅ Strengths
Default closes both advisories. pathutil removal is clean (no dangling refs). path.Clean/backslash protection retained. New regression tests use strong NotContains("TOP-SECRET")/"private file" assertions that fail if the vuln is present. Good encoding-variant breadth on the default path.
Net: this is the right base — simpler and more maintainable than the denylist. I'd like C1 resolved (the .. guard) and I1/I2 (one doc line + one cheap guard) before release; the rest are follow-ups. #3015 carries the .. guard ready to graft on; I'll close it once that lands here.
Aggregated from a multi-agent review (code / tests / comments / error-handling); the C1 and UseEscapedPathForMatching leaks were each reproduced with a scratch test.
Revert PR #3009 changes to just disabling path escaping by default in static methods/middleware
#3009 is a little bit brute force hack to solve the problem from LLM. Claude proposed checking and fixing path used is not a maintainable solutions and there could be so many clever ways how bad actors try to manipulate the path, and the root cause is that the Router and code serving Static files are conceptionally using path differently - so more maintainable solution is to make these 2 acting consitently.
Note: disabling path escaping in static methods and static middleware is a breaking change.