Skip to content

revert PR #3009 changes to just disabling path escaping by default in static methods/middleware#3016

Open
aldas wants to merge 6 commits into
labstack:masterfrom
aldas:StaticDirectoryHandler_Router_path_variable_escaping
Open

revert PR #3009 changes to just disabling path escaping by default in static methods/middleware#3016
aldas wants to merge 6 commits into
labstack:masterfrom
aldas:StaticDirectoryHandler_Router_path_variable_escaping

Conversation

@aldas

@aldas aldas commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

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.

…sible Unauthorized static file disclosure

Fix StaticDirectoryHandler and Router Inconsistent Escaping leading to possible Unauthorized static file disclosure
@codecov

codecov Bot commented Jun 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.29%. Comparing base (01b45be) to head (c2a5ef6).
⚠️ Report is 19 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@vishr vishr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: true static case [C1]; (b) encoded dot-dot (%2E%2E) at the echo.StaticDirectoryHandler level (middleware has it, the echo path doesn't); (c) a positive opt-in test proving /hello%20world.txt is actually served when the flag is on.
  • fs.Stat error collapses to bare ErrNotFound, discarding EACCES/I-O faults with no wrap or log (pre-existing) — worth errors.Is(err, fs.ErrNotExist) while we're here.
  • Docs/grammar: "forbit" → "forbid" and a broken run-on sentence in the StaticDirectoryHandler note; document the empty-RawPath cmp.Or fallback so it isn't "simplified" away later; the two EnablePathUnescaping* doc copies diverge (the middleware one omits the UseEscapedPathForMatching guidance).

✅ 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.

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.

2 participants