fix(dashboard): match non-directory public auth-gate entries exactly - #78966
Open
Gearhead1175 wants to merge 1 commit into
Open
Gearhead1175 wants to merge 1 commit into
Gearhead1175 wants to merge 1 commit into
Conversation
`_path_is_public` tested `path == prefix or path.startswith(prefix)`
against every `_GATE_PUBLIC_PREFIXES` entry. The `startswith` arm makes
the `==` arm redundant and turns each non-directory entry into a
wildcard: `/login` also exempts `/loginX`, `/favicon.ico` also exempts
`/favicon.icox`, `/api/auth/providers` also exempts
`/api/auth/providersX`. Any request path that merely starts with such an
entry bypasses the OAuth gate.
Directory entries were never affected — the trailing slash on `/assets/`
already stops it matching `/assetsleak/`, as the list's own comment
notes. The bug is confined to entries without one.
The data already encodes the distinction, so honour it in the matcher:
prefix semantics for `/`-terminated entries, exact match for everything
else. This mirrors how `PUBLIC_API_PATHS` is handled a few lines up,
where the docstring calls out the identical hazard ("adding
`/api/status` doesn't accidentally expose
`/api/status/secret-extension`").
No route currently registered under a public entry's prefix carries
sensitive data, so this is hardening rather than a live bypass — but the
allowlist is the boundary, and it should mean what it says.
Adds regression coverage in three directions: exact entries stay public,
`/`-terminated entries keep matching children, and prefixed siblings
stay gated. The five sibling cases fail on the unfixed matcher.
Contributor
|
looks mergeable The patch constrains non-directory OAuth auth-gate allowlist entries to exact paths while preserving directory-prefix behavior. Focused regression coverage passed and no source-backed security findings remain. Review setup: I reviewed a run-owned local rebase or patch replay against current GitHub Security evidence:
Not checked:
Signed: GPT-5.6-luna-max in Codex |
This branch has not been deployed
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
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.
What does this PR do?
_path_is_publicinhermes_cli/dashboard_auth/middleware.py:77testspath == prefix or path.startswith(prefix)against every_GATE_PUBLIC_PREFIXESentry. Thestartswitharm makes the==arm redundant and turns each non-directory entry into a wildcard:/login/loginX/favicon.ico/favicon.icox/api/auth/providers/api/auth/providersX/auth/logout/auth/logout-allAny request path that merely starts with a public entry bypasses the OAuth gate.
Directory entries were never affected — the trailing slash on
/assets/already stops it matching/assetsleak/, as the list's own comment at line 46 notes. The bug is confined to entries without one.The data already encodes the distinction, so this honours it in the matcher: prefix semantics for
/-terminated entries, exact match for everything else. That mirrors howPUBLIC_API_PATHSis handled a few lines up, where the docstring calls out the identical hazard ("adding/api/statusdoesn't accidentally expose/api/status/secret-extension").Impact is hardening, not a live bypass. Comparing the registered route table against the public entries, no route currently sits under one of those prefixes, so nothing sensitive is reachable on
maintoday. This matters when someone later adds a route — or a static file underweb_dist— whose name extends a public entry; the allowlist is the boundary and it should mean what it says.Filing publicly rather than via GHSA per SECURITY.md §3.2 — no §3.1 outcome is reachable, so this is hardening, not a vulnerability report.
Related Issue
No existing issue. Searched open/closed issues and PRs for
_path_is_public,GATE_PUBLIC_PREFIXES,auth gate prefix,startswith auth bypass— closest hits are #61305, #35547, #27932, which touch this file but not this matcher.Type of Change
Changes Made
hermes_cli/dashboard_auth/middleware.py—_path_is_publicmatches/-terminated entries by prefix, all others exactly; docstring updatedtests/hermes_cli/test_dashboard_auth_middleware.py— 23 parametrised regression cases in three directionsHow to Test
Reproduce on
main:With this branch both return
False. The five sibling cases fail on the unfixed matcher and pass with it.Checklist
Code
scripts/run_tests.shand all tests passDocumentation & Housekeeping
cli-config.yaml.example— N/A (no config keys)CONTRIBUTING.md/AGENTS.md— N/A (no architecture change)Screenshots / Logs
Full suite via
scripts/run_tests.sh -q:File-level diff of the failure sets: 22 files fail on
main, 24 on this branch. The two extra —tests/test_tui_gateway_server.pyandtests/tools/test_transcription_tools.py— pass 563/563 when run in isolation on this branch, and both failed on wall-clock timing under 32-way parallelism (reader.join(timeout=0.5)→TimeoutExpired; the tui file took 74.1s loaded vs 16.1s isolated). Nothing in the baseline set passed here in the other direction.No dashboard/auth/web_server file appears in either failure set, and
test_dashboard_auth_middleware.pypassed (37✓) in the full run. The pre-existing failures are provider routing, media generation, sandbox backends, and doctor — all needing external services this environment lacks. Raw totals jitter between runs because of the timing-sensitive tests above, so the file-level diff is the meaningful comparison rather than the counts.--EnA