Skip to content

rewrite: fix wrong index check in trimPathPrefix#7812

Merged
mholt merged 2 commits into
caddyserver:masterfrom
alhudz:rewrite-trimpathprefix-index
Jun 12, 2026
Merged

rewrite: fix wrong index check in trimPathPrefix#7812
mholt merged 2 commits into
caddyserver:masterfrom
alhudz:rewrite-trimpathprefix-index

Conversation

@alhudz

@alhudz alhudz commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Repro: with uri strip_prefix /aaaaaa, a request for /%61%61 (which decodes to /aa) comes back rewritten to /aa instead of being left untouched. /aa is not under /aaaaaa, so nothing should be stripped.
Cause: trimPathPrefix walks the escaped path and the prefix in lock-step, then decides it matched with iPath >= len(prefix). iPath indexes the escaped request path while len(prefix) is the prefix length, so a short percent-encoded path whose byte length reaches len(prefix) exits the loop with the prefix only partly consumed yet still passes that check and is treated as a match. strip_path_suffix reaches the same code through the reversed call.
Fix: compare iPrefix against len(prefix) so a match requires the whole prefix to have been consumed.

Assistance Disclosure

I used an LLM to help analyse the loop and sanity-check the repro; I wrote and verified the fix and the regression test locally.

@CLAassistant

CLAassistant commented Jun 9, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@alhudz

alhudz commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

done!

@mholt

mholt commented Jun 10, 2026

Copy link
Copy Markdown
Member

Thanks; however the PR template is missing, can you please restore the assistance disclosure?

Also, the CLA still seems to not be signed -- maybe make sure your commits are with an email on your GH account.

@mholt mholt added the needs info 📭 Requires more information label Jun 10, 2026
@alhudz alhudz force-pushed the rewrite-trimpathprefix-index branch from 9f38ea4 to 21874f7 Compare June 11, 2026 05:23
@alhudz

alhudz commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

Restored the disclosure section in the PR body, sorry about dropping it.

The CLA failure was down to a typo in my commit email (al.hudz.k@ instead of alhudz.k@), so the commit wasn't linked to my account. Amended the author email and force-pushed; the branch is rebased on current master in the process.

@mholt mholt enabled auto-merge (squash) June 12, 2026 00:46
@mholt

mholt commented Jun 12, 2026

Copy link
Copy Markdown
Member

Alright, thank you!

@mholt mholt removed the needs info 📭 Requires more information label Jun 12, 2026
@mholt mholt added this to the v2.11.5 milestone Jun 12, 2026
@mholt mholt merged commit 52dc670 into caddyserver:master Jun 12, 2026
27 checks passed
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.

3 participants