Skip to content

fix(file-explorer): accept proxy signatures for paths that are not requote-stable - #4649

Closed
isaaclam678 wants to merge 1 commit into
kirodotdev:mainfrom
isaaclam678:fix/file-explorer-requote-auth
Closed

fix(file-explorer): accept proxy signatures for paths that are not requote-stable#4649
isaaclam678 wants to merge 1 commit into
kirodotdev:mainfrom
isaaclam678:fix/file-explorer-requote-auth

Conversation

@isaaclam678

@isaaclam678 isaaclam678 commented Aug 20, 2026

Copy link
Copy Markdown

Problem / Motivation

The Files app shows a silently empty tree for any folder whose path contains a space — e.g. ~/My Documents or ~/internal work. The backend rejects every request for such paths with a 401 the UI swallows, so the app just looks broken with no error shown.

Why it matters

Space-containing folder names are everywhere on real machines (macOS especially), so affected users experience the app as "shows no files at all" for exactly the folders they care about — with no clue why. The failure is invisible: no error surfaces in the UI or the browser console beyond a swallowed 401.

What changed (motivation → approach → change)

Observed symptom: folder listings for space paths return 401 while sibling paths without spaces work.

Root cause: the gateway signs the DECODED request target (aiohttp's match_info / request.query_string), then sends a plain-str URL that yarl re-encodes — so the wire bytes differ from the signed form whenever the decoded text does not survive a requote round-trip. A space is the everyday case: signed as ' ', sent as '+'. The file-explorer backend verified only the raw wire bytes, so verification failed for every such request.

Change: _authorized_or_health verifies the raw wire target first (unchanged behavior, zero risk to currently-working paths), then falls back to the decoded form — the same contract every aiohttp-based app backend in this repo already uses; the file-explorer backend was the one deviating. The handler also gains an optional body parameter so future non-GET routes can sign over their payload.

Tests

New test/test_file_explorer_proxy_auth.py:

  • test_decoded_target_with_space_accepted — the regression: fails against the pre-fix handler and passes with this change (verified by running it against upstream/main's server.py: 5 failed → 5 passed).
  • test_raw_wire_target_accepted — pins that the historical raw-bytes form still verifies.
  • test_wrong_signature_rejected — wrong key never passes either form.
  • test_body_participates_in_the_signature — a signature over one body must not verify over different bytes.
  • test_health_stays_unauthenticated — the gateway's unsigned liveness probe keeps working.

Full existing file-explorer suite passes (75 tests). isort / flake8 / mypy clean on the touched files.

Manual verification

Reproduced and verified end-to-end against a locally booted backend with signed requests: pre-fix, a listing of a real space path returns 401; post-fix it returns the folder's entries. Also verified in a production install where the same symptom was originally diagnosed.

Related Issues

Discovered while preparing #4648; this fix is standalone and #4650 stacks on it.

Checklist

  • Single commit with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable) — N/A, behavior fix with no doc surface
  • No secrets, credentials, or internal references in the diff

…quote-stable

The gateway signs the decoded request target, but its HTTP client
re-encodes the URL on send, so the wire bytes differ from the signed
form whenever the decoded text does not survive a requote round-trip.
A space is the everyday case: signed as ' ' but sent as '+', so every
folder or file whose path contains a space failed HMAC verification
and the Files UI showed a silently empty tree.

Verify the raw wire target first (unchanged behavior), then fall back
to the decoded form — the same contract every aiohttp-based app
backend already uses. The handler also grows an optional body
parameter so future non-GET routes can sign over their payload.

The regression test fails against the pre-fix handler and passes with
this change; raw-form compatibility, wrong-key rejection, body
tampering, and the unauthenticated health probe are pinned alongside.
@isaaclam678
isaaclam678 requested a review from a team as a code owner August 20, 2026 07:23
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention labels Aug 20, 2026
@iamwhatever

Copy link
Copy Markdown
Collaborator

👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated.

Missing sections:

  • ## Problem / Motivation
  • ## Why it matters
  • ## What changed
  • ## Tests

Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle.

2 similar comments
@bolichen97

Copy link
Copy Markdown
Collaborator

👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated.

Missing sections:

  • ## Problem / Motivation
  • ## Why it matters
  • ## What changed
  • ## Tests

Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle.

@iamwhatever

Copy link
Copy Markdown
Collaborator

👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated.

Missing sections:

  • ## Problem / Motivation
  • ## Why it matters
  • ## What changed
  • ## Tests

Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 20, 2026
@isaaclam678

Copy link
Copy Markdown
Author

Withdrawing this PR — superseded before it was needed. While iterating on CI I found #4377 (merged 18 Aug) already fixed this bug class at the gateway: it now signs the raw wire form of the request-target (yarl.URL(..., encoded=True).raw_path_qs), and the new test_app_proxy_target_ratchet.py enforces that backends never reconstruct the signed target from decoded request parts — the opposite direction of this PR's decoded-form fallback, which the ratchet correctly rejects on current main.

The symptom this PR chased (silent 401 → empty tree for space paths) was real on the released build but is already resolved on main. The one still-useful piece — _authorized_or_health verifying POST bodies — moved into #4650, which now stands alone. Thanks!

@github-actions github-actions Bot removed the readiness: action required A blocking check or review needs attention label Aug 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants