Skip to content

fix(broker): traversal is a path SEGMENT of .., not the substring anywhere - #2231

Merged
rubenvdlinde merged 1 commit into
developmentfrom
fix/broker-traversal-guard-segments
Jul 31, 2026
Merged

fix(broker): traversal is a path SEGMENT of .., not the substring anywhere#2231
rubenvdlinde merged 1 commit into
developmentfrom
fix/broker-traversal-guard-segments

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

What was wrong

CredentialBrokerService::normalisePath() denied any path containing the substring ... That rejected legitimate paths whose segments merely contain dots — most importantly GitHub's diff endpoint:

/repos/{owner}/{repo}/compare/{base}...{head}

So every commit comparison through the broker was refused as traversal.

Why it matters more than it looks

hydra-flows-first-port task 2.5 makes "diff the produced tree against the base before moving the ref" a mandatory rail on the commit-by-API path. The reason is specific: base_tree overwrites rather than merges, so a tree built against a base that has since moved silently reverts every file the caller did not send, and the resulting commit looks clean. That has happened for real (two files lost an import and a template gate during the nc-vue migration, caught only by running the full suite).

The rail could not be built at all over the brokered path while this guard stood.

Found by running the chain end to end: blob 201, tree 201, commit 201, then compare 403.

The fix

Check for a segment that IS .., which is what traversal actually is. A segment like ..b, a..b or ... is a literal name and never walks anywhere.

Security property unchanged, pinned both ways:

path before after
/repos/Conduction/../../etc/passwd denied denied
/repos/Conduction/%2e%2e/%2e%2e/etc/passwd denied denied
/repos/Conduction/openregister/compare/A...B denied allowed
/repos/Conduction/some..name denied allowed

The encoded case is pinned separately on purpose: the guard decodes once before checking, so an encoded traversal segment is still caught — a segment check running before the decode would be trivially bypassable, and that is the mistake this test exists to prevent. Double-encoding is unaffected: %252e%252e single-decodes to %2e%2e, which is not .., exactly as before.

Verification

15,553 unit tests green (4 new); phpcs (full tree, CI settings) and phpstan clean.

…nywhere

`normalisePath()` denied any path containing the substring `..`. That rejected
legitimate paths whose segments merely contain dots — most importantly GitHub's
diff endpoint, `/repos/{o}/{r}/compare/{base}...{head}`, so EVERY commit
comparison was refused as traversal.

That is not a cosmetic refusal. `hydra-flows-first-port` task 2.5 makes "diff
the produced tree against the base before moving the ref" a MANDATORY rail on
the commit-by-API path — precisely because `base_tree` overwrites rather than
merges, so a tree built against a base that has moved silently reverts every
file the caller did not send while producing a clean-looking commit. The rail
could not be built at all over the brokered path while this guard stood.

Found by running the chain: blob 201, tree 201, commit 201, then compare 403.

The check now looks for a segment that IS `..`, which is what traversal
actually is. The security property is unchanged and pinned both ways:

- `/repos/Conduction/../../etc/passwd`        still denied
- `/repos/Conduction/%2e%2e/%2e%2e/etc/passwd` still denied (the guard decodes
  once BEFORE checking, so an encoded traversal segment is caught — pinned
  separately because a segment check running before the decode would be
  trivially bypassable)
- `/repos/Conduction/openregister/compare/A...B`  now allowed
- `/repos/Conduction/some..name`                  now allowed

Double-encoding is unaffected: `%252e%252e` single-decodes to `%2e%2e`, which
is not `..`, exactly as before.

15,553 unit tests green (4 new); phpcs, phpstan clean.
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 41d0a6b

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
composer ✅ 174/174
npm ✅ 616/616
PHPUnit
Newman
Playwright ⏭️

Quality workflow — 2026-07-31 14:28 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
rubenvdlinde merged commit 1aa0404 into development Jul 31, 2026
27 checks passed
@rubenvdlinde
rubenvdlinde deleted the fix/broker-traversal-guard-segments branch July 31, 2026 14:34
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.

1 participant