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

[aldas]

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.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.29%. Comparing base (01b45be) to head (b58df7c).
⚠️ 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]

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

[aldas]

Suggested fix: reject any .. path segment in the resolved wildcard (the pathutil.HasDotDotSegment approach in #3015).

and

no legitimate filename contains an encoded separator, so it costs nothing and preserves vfp3 protection for opt-in users

So far we have allowed .. in paths. For cases ases then there is no "guard route" to block some subfolder from being served with static handler rejecting request makes people life just harded and is a breaking change.

I think we must make sure that escpaped .. and slashes can not be used to move out of served folder, up in directory structure, but moving within served folder is OK.

In my oppinion this example is fundamentally broken approach and these guards must be added to e.GET("/admin/*" as middlewares.

Request to /assets/../admin%2fprivate.txt escaped or unescaped will never match to guard route. And resolves to /admin/private.txt after path.Clean() is done.

// 0.
// given folder structure:
// private.txt
// public/
// public/index.html
// public/text.txt
// public/admin/private.txt

// 1. share `public/` folder contents from the server root. This folder actually contains subfolder `admin` which
// contents we want to forbid from downloading
e.Static("/", "public")

// 2. naively assume that everything under /admin folder is now forbidden
e.GET("/admin/*", func(c *Context) error {
	return ErrForbidden
})

---

I have fixed I1 and I3,
and added comments+tests for I2 some of the configuration combinations being insecure

[aldas]

@vishr please, rerun your checks. Claude at my side seems to be OK with current state.

[aldas]

@vishr I propose this PR as v5.2.1 release with documenting that Static file handling has breaking change - no default unescaping is done and there are (potentially unsafe) knobs to reenable it.

[vishr]

Approving — re-verified on b58df7c: builds, go vet, and go test -race ./... green; I1 (deprecated field documented as ignored) and I3 (redirect reverted to decoded Path) are resolved; the default config closes the high-severity encoded %2E%2E variant; and escaping above the served root returns 404 in every mode.

Conceding C1. You're right and I was wrong to flag the .. reject as a blocker. I verified that /sub/../index.html is legitimate within-root navigation (200), so rejecting all .. segments — what #3015 did — would break valid use while only affecting within-root lateral access. Since fs.FS already prevents leaving the served root, the residual cases (UseEscapedPathForMatching, and literal .. in the default router) are within-root access to a subtree you chose to serve — the broken-sibling-guard composition you described, correctly handled by documentation. I've closed #3015 as superseded.

Two non-blocking asks before release:

1. Advisory + CHANGELOG wording that states exactly what changed: encoded path-traversal that bypassed route guards is fixed by the new no-unescape default; serving a folder exposes its whole subtree, so guard sensitive subfolders with middleware on the static mount, not a sibling route. (So the GHSA doesn't read as "all variants blocked.")
2. Tiny grammar nit in the StaticDirectoryHandler note: "forbit" → "forbid".

And a follow-up worth its own issue (not this PR): the root cause is router-vs-handler disagreeing on normalization. Cleaning/redirecting .. request paths before routing (like net/http's ServeMux) would make /public/../admin/secret.txt redirect to /admin/secret.txt and actually hit the guard route — closing the pattern without any .. denylist. Bigger behavioral change, separate discussion.

5.2.1 with a documented breaking-change note is reasonable for a security patch. 👍

[aldas]

@vishr , I will do v5.2.1 release in 2 hours - it is ok?

[aldas]

v5.2.1 is relased.

but I forgot to pump the version in version.go. anyway, this is a minor thing and as there are couple other things (#2996 , #2949 ) to release we can bump the string in next release.

These last 2 days have been eventfull to be honest :)

@vishr, LLM is a double edged blade. It can do precise, small, changes extremely good, but falls apart when broader conceptual awareness is needed. And the creep of complexity into codebase is huge as LLM is trying to do almost whatever to please the requester with the solution on the hand and not considering that in broader picture the solution is not suitable.

[vishr]

@aldas Thanks for getting v5.2.1 out — eventful couple of days! The version.go string can ride along with #2996 / #2949 next release.

Totally agree it's a double-edged blade, and the complexity-creep point is spot on — needs a careful hand on the wheel. But used carefully, with a human owning the broader picture, it's a huge help.

On a lighter note, check out the new site: https://echo.labstack.com