Revise PR #242: bearer token still crosses origins on redirect, plus a missing route

[jaylfc]

CARD TITLE (intent, not commit subject): Revise PR #242: bearer token still crosses origins on redirect, plus a missing route

Autonomous build of board card tsk-revvkw.

Files:
taosmd/config.py | 47 +++++++
taosmd/http_server.py | 16 +++
taosmd/ref_fetch.py | 154 +++++++++++++++++++++++
taosmd/remote.py | 8 ++
taosmd/service.py | 62 ++++++++-
tests/test_http_server.py | 35 ++++++
tests/test_ref_fetch.py | 314 ++++++++++++++++++++++++++++++++++++++++++++++
7 files changed, 635 insertions(+), 1 deletion(-)

[qodo-code-review]

ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing

[coderabbitai]

Warning

Review limit reached

@jaylfc, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 22 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: fb260942-83a2-423b-b04c-5c81e57b5c9f

📥 Commits

Reviewing files that changed from the base of the PR and between 8b20e09 and edfe704.

📒 Files selected for processing (7)

• taosmd/config.py
• taosmd/http_server.py
• taosmd/ref_fetch.py
• taosmd/remote.py
• taosmd/service.py
• tests/test_http_server.py
• tests/test_ref_fetch.py

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[gitar-bot]

Important

You are using the Gitar free plan. Upgrade to unlock code review, CI analysis, auto-apply, custom automations, and more.

_Gitar

[kilo-code-bot]

WARNING: Double-encoded path traversal possible in _reject_dot_segments

urllib.parse.unquote only performs single-level decoding, so a path like %252e%252e (double-encoded ..) bypasses the dot-segment check. An attacker could craft a ref URI with double-encoded dots to traverse directories if the downstream server decodes paths more than once.

---

Reply with @kilocode-bot fix it to have Kilo Code address this issue.

[kilo-code-bot]

WARNING: Missing _apply_token_binding on /refs/fetch

Unlike other data endpoints (/ingest, /search), this handler does not call _apply_token_binding(agent, project). Registry-token verification and active-grant checks are skipped, so a bearer-token holder without an active grant can fetch files.

---

Reply with @kilocode-bot fix it to have Kilo Code address this issue.

[kilo-code-bot]

WARNING: Domain exceptions from fetch_by_ref return 500 instead of proper HTTP status codes

service.fetch_by_ref raises NotFoundError (should be 404), UnauthorizedError (should be 401/403), and HashMismatchError (should be 400), but _dispatch only catches _BadRequest, ValueError, and Exception. These domain errors fall through to the generic 500 handler.

---

Reply with @kilocode-bot fix it to have Kilo Code address this issue.

[kilo-code-bot]

Code Review Summary

Status: 3 Issues Found | Recommendation: Address before merge

Overview

Severity	Count
WARNING	3

Issue Details (click to expand)

WARNING

File	Line	Issue
taosmd/ref_fetch.py	38	Double-encoded path traversal possible in _reject_dot_segments
taosmd/http_server.py	1110	Missing _apply_token_binding on /refs/fetch
taosmd/http_server.py	1118	Domain exceptions from fetch_by_ref return 500 instead of proper HTTP status codes

Files Reviewed (4 files)

• taosmd/config.py
• taosmd/http_server.py - 2 issues
• taosmd/ref_fetch.py - 1 issue
• taosmd/service.py

Fix these issues in Kilo Cloud

---

_{Reviewed by step-3.7-flash · Input: 84.5K · Output: 31.2K · Cached: 1.4M}

[jaylfc]

All three blockers on #242 are fixed and I proved each by running. One required change: the test guarding the token does not actually guard it.

Reviewed on head edfe704. 88 tests pass across test_ref_fetch.py and test_http_server.py.

Blocker 1, the bearer token crossing origins on redirect: FIXED, proven both directions

Same two-server setup that closed #238: a controller that 302s to an attacker origin, and an
attacker that records what arrives.

#242 pattern (plain urlopen):
  outcome: fetch returned normally
  attacker received: 'Bearer SECRET-REGISTRY-TOKEN-DO-NOT-LEAK'
  VERDICT: *** TOKEN LEAKED ***

#264 pattern (_NoRedirect opener):
  outcome: HTTPError 302 (redirect not followed)
  attacker received: None
  VERDICT: no token reached the attacker origin

The vulnerable half is the control: it shows the probe is capable of catching a leak, so the
clean result on this branch means something.

Blocker 2, POST /refs/fetch had no server route: FIXED

http_server.py is now in the diff and dispatches POST /refs/fetch to _handle_refs_fetch,
which validates ref and agent before calling the service.

Blocker 3, guard and fetch resolving different base URLs: FIXED

There is now exactly one resolution site. service.fetch_by_ref no longer computes files_url
itself; it passes data_dir through and ref_fetch.fetch_by_ref calls _get_files_url(data_dir)
once. They cannot diverge because there is nothing left to diverge. Measured:

files_url only in the data_dir config -> https://only-in-datadir.example
registry_url fallback, no files_url   -> https://registry-only.example
CONTROL neither configured            -> RAISED "files_url is not configured"

The control matters here: on #242 the guard passed with a data_dir-only value while the fetch
raised, so "it resolved" was not by itself evidence.

The traversal fix from #242 still holds

taos://slug/files/ok.txt                     -> resolves (control)
taos://slug/files/../../etc/passwd           -> REJECTED
taos://slug/files/%2e%2e/%2e%2e/etc/passwd   -> REJECTED
taos://slug/files/a/./../../etc/passwd       -> REJECTED
taos://slug/files/%252e%252e/etc/passwd      -> resolves to %25252e%25252e (neutralised)

Last row is the same undocumented-but-load-bearing mechanism I flagged on #242: quote(path, safe="/") re-encodes the %, so a double-encoded segment becomes a literal filename rather
than traversal. It is still worth a comment saying so, because a future change to the quoting
would silently remove a defence nobody knows is there.

REQUIRED before merge: test_fetcher_uses_no_redirect_opener passes while the token leaks

The fix is right; the test does not defend it. It monkeypatches build_opener away entirely, so
no HTTP happens, and then asserts only:

assert issubclass(build_opener_calls[0][0], urllib.request.HTTPRedirectHandler)

That asserts the shape of the fix, not the property. I ran it against a handler that inherits
the default behaviour:

class _NoRedirect(urllib.request.HTTPRedirectHandler):
    pass   # inherits the default: still follows redirects

PR's assertion (issubclass of HTTPRedirectHandler): PASSES
attacker received: 'Bearer SECRET-REGISTRY-TOKEN-DO-NOT-LEAK'
VERDICT: *** TOKEN LEAKED while the PR's test passes ***

So the one test standing between us and a repeat of #238 goes green on the vulnerable version.
Given this token is the reason #238 was closed and rebuilt, that guard needs to be behavioural.

Suggested replacement, which is what I ran above: stand up two http.server instances on
ephemeral ports, have the first 302 to the second, point the real fetcher at the first, and
assert the second received no Authorization header. It needs no network and no new dependency.
Keep the existing structural assertion as well if you like, but it cannot be the only one.

One smaller note

A redirecting controller now surfaces as a bare urllib.error.HTTPError (302). The except
chain handles 401/403/404 and re-raises the rest, so it escapes the RefFetchError family and
the HTTP route turns it into a 500. Since refusing a redirect is now a deliberate security
decision rather than a transport failure, it deserves its own message, e.g. raising
RefFetchError("refusing to follow redirect from <origin> to <origin>"). That also makes the
behaviour discoverable to whoever misconfigures a controller.

[jaylfc]

Merging as-is on Jay's call, with the required test change carded as a follow-up rather than blocking the fix.

What is landing: the _NoRedirect opener. Verified correct in both directions - the redirect no longer carries Authorization to a second origin, and the control (same fetcher, redirect handler restored) still leaks. That is the defect that closed #238 and it is genuinely fixed here.

What is NOT landing, and why it is carded: test_fetcher_uses_no_redirect_opener monkeypatches build_opener away and then asserts only issubclass(..., HTTPRedirectHandler). I measured that a do-nothing subclass satisfies that assertion while the token still leaves the origin. So the test passes on code that has the bug, and CI does not defend the guard this PR exists to install.

The honest statement of what merges here: the fix is proven by my probe, not by the suite. A future refactor of ref_fetch.py can reintroduce the leak and stay green. Follow-up card replaces the test with one that runs a real redirect against a local server and asserts the second origin received no Authorization header - the assertion has to be about the header on the wire, not about the class of an opener.