Reject percent-encoded parent traversal in credential broker paths - #154
Closed
yassine-ceo wants to merge 1 commit into
Closed
Reject percent-encoded parent traversal in credential broker paths#154yassine-ceo wants to merge 1 commit into
yassine-ceo wants to merge 1 commit into
Conversation
rajpratham1
approved these changes
Aug 3, 2026
rajpratham1
left a comment
rajpratham1
left a comment
There was a problem hiding this comment.
The implementation addresses a legitimate path traversal concern, applies the validation consistently, and includes targeted regression tests that verify both rejection behavior and the absence of unintended upstream requests. I don't see any blocking issues in the proposed changes.
16francej
added
code-pr
pull Bot
pushed a commit
to danielabelski/qm
that referenced
this pull request
Aug 4, 2026
The broker's path allowlist checked literal "../" but not its percent-encoded forms, so an allowed prefix could be escaped with %2e%2e%2f (single- or double-encoded). Decode before checking, reject any path whose decoded form contains a parent traversal, and cover the git smart-HTTP route with the same guard. Regression tests exercise both routes with plain, single-, and double-encoded traversals. Reported in yc-software#154 (closes the yc-software#125 git-broker finding). Co-authored-by: yassine-ceo <yassine-ceo@users.noreply.github.com>
Collaborator
|
Fixed on our side, going out shortly with you as co-author on the commit 🙏 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the Git Broker finding from #125.
Problem
The credential broker and the git HTTP broker gate upstream paths through
brokerPathAllowed, which matches the raw pathname againstallowedPathPrefixes. Percent-encoded parent segments (..%2f,%2e%2e%2f) survive URL parsing with the encoded bytes intact, so a path like/acme/repo.git/..%2fadminstill starts with the allowed prefix while upstream servers decode it to/admin— a path traversal that bypasses the allowlist.Fix
brokerPathAllowednow rejects any pathname containing a..segment once percent-decoded (hasParentSegment), fail-closed on malformed escapes. This sits in the shared gate bothbrokerCredentialCallandbrokerGitHttpflow through, so both entry points are covered by one check.Tests
test/credential-broker.test.ts: encoded traversals (..%2f,%2e%2e%2f,%2e%2e%2fvariants) refused withpath_not_allowed, zero upstream calls.test/git-http-broker.test.ts: encoded traversals in the git broker path refused withpath_not_allowed, no upstream git fetch.Verified:
npm run typecheck,npm run lint(oxlint), and both affected test suites pass.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.