A2A ref fetch: rebuild #238 with path-traversal fix, working remote route, and real registry_url fallback

[jaylfc]

CARD TITLE (intent, not commit subject): A2A ref fetch: rebuild #238 with path-traversal fix, working remote route, and real registry_url fallback

Autonomous build of board card tsk-7xcsh5.

Files:
taosmd/config.py | 47 +++++++++
taosmd/ref_fetch.py | 153 ++++++++++++++++++++++++++++++
taosmd/remote.py | 8 ++
taosmd/service.py | 62 +++++++++++-
tests/test_ref_fetch.py | 247 ++++++++++++++++++++++++++++++++++++++++++++++++
5 files changed, 516 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: 33 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: a1887f72-d756-4229-8e0e-dcf4dbee6da7

📥 Commits

Reviewing files that changed from the base of the PR and between f6b5a17 and 4fcfa3e.

📒 Files selected for processing (5)

• taosmd/config.py
• taosmd/ref_fetch.py
• taosmd/remote.py
• taosmd/service.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]

Kilo Code Review could not run — your account is out of credits.

Add credits or switch to a free model to enable reviews on this change.

[jaylfc]

Review: blocked, 3 blockers. The traversal fix is sound; the token-scope hole is not closed

I closed #238 because it could send our registry bearer token outside the Files scope. So I
reviewed this rebuild by attacking it rather than reading it, and I checked the specific thing
the card exists to fix first.

The path-traversal fix holds. Both controls reject, and the accepted cases are neutralised:

PROBE: what does resolve_ref_uri accept?
  ACCEPT  CONTROL, benign            -> .../files/docs/spec.md
  reject  CONTROL, literal traversal (dot segment '..')
  reject  CONTROL, single-encoded    (dot segment '..')
  reject  encoded slash in segment   (dot segment '..')
  ACCEPT  double-encoded             -> .../files/%25252e%25252e/etc
  ACCEPT  backslash traversal        -> .../files/..%255c..%255cetc
  ACCEPT  overlong UTF-8             -> .../files/%25c0%25ae%25c0%25ae/etc

The three ACCEPTs are safe, but for a reason worth writing down: quote(path, safe="/")
re-encodes the %, so %252e reaches the server as the literal text %25252e and decodes once
to %252e, never to a dot. The defense is therefore two mechanisms, _reject_dot_segments plus
the re-quoting, and only the first is documented. An optimisation that skips re-encoding an
already-encoded path would silently reopen the encoded variants. Please put that in a comment.
26/26 tests pass, and the deleted-symbols gate is green (this is its second live PR).

The problem is that the token-scope defect did not go away. It moved.

---

Blocker 1: the registry bearer token follows redirects to any host

_fetcher builds a urllib.request.Request with Authorization: Bearer <registry_token> and
calls urlopen, which follows 3xx by default. CPython's HTTPRedirectHandler copies every
header except content-length and content-type onto the redirected request, so Authorization
crosses to the new origin. Reproduced with the fetcher copied verbatim from this branch:

CONTROL direct GET to attacker host:
   captured Authorization -> Bearer SECRET-REGISTRY-BEARER-TOKEN

TEST controller 302 -> attacker host:
   bytes returned to caller       -> b'payload-from-attacker-host'
   attacker received Authorization -> Bearer SECRET-REGISTRY-BEARER-TOKEN

The ref uri here is entirely well-formed, no dot segments, and resolve_ref_uri accepts it. The
traversal fix is not bypassed; it is simply not on this path.

Two things make this worse than it first looks:

• The sha256 check protects the data, not the credential. The attacker's bytes fail
  verification and the caller gets a HashMismatchError, so the operator sees "fetch failed"
  and nothing else. The token left the building before the hash was ever computed. That is
  fail-quiet on exactly the half that matters.
• It needs the configured controller to emit the redirect, so it is not reachable from a hostile
  ref uri alone. But "the controller can be made to redirect" covers an open redirect on the
  controller and a compromised or misconfigured one, and the whole reason this card exists is
  that the token must not leave the Files scope.

Fix: build an opener with a redirect handler that drops Authorization when the redirect target
changes origin, or refuse redirects outright and treat a 3xx as an error. Either is a few lines,
and the test is the probe above.

Blocker 2: POST /refs/fetch has no server route

RemoteClient.fetch_by_ref posts to /refs/fetch. That path exists nowhere else on the branch:

$ git grep -n "refs/fetch" exec/tsk-7xcsh5
  taosmd/remote.py:405:  """POST /refs/fetch: proxy a ref fetch to the remote server."""
  taosmd/remote.py:409:  return await self._run("POST", "/refs/fetch", {"ref": ref, "agent": agent})

No http_server.py change is in the diff at all. So a remote-configured install calling
service.fetch_by_ref takes the _get_remote branch and gets a 404 from its own server. The PR
title says "working remote route"; the client half is written and the server half is not. Same
shape as #234's unrouted endpoints and #232's dispatched-but-unwritten handler, which is now
three PRs in a row, so it may be worth a checklist item rather than three separate review notes.

Blocker 3: the guard and the fetch resolve two different base URLs

service.fetch_by_ref computes files_url = _config.get_files_url(data_dir) and raises if it is
unset. It then never passes that value on. ref_fetch.fetch_by_ref calls _get_files_url()
again internally, with no data_dir. The local variable is dead and the two disagree:

CONTROL: guard passes but the fetch resolves elsewhere?
    guard get_files_url(d2)    : https://only-in-datadir.example   (guard PASSES)
    _get_files_url() (no dir)  : RefFetchError: files_url is not configured

The HTTP server always passes data_dir (see the _get_remote docstring, "as the http_server
always does"), so on a server this diverges every time: the guard says configured, the fetch says
not configured. Only the TAOSMD_FILES_URL env path makes them agree, which is why the tests do
not catch it.

This also undercuts the third item in the title. The registry_url fallback is real, but it
lives in _get_files_url() and resolves against the default config location, ignoring the
data_dir the caller asked about. A data_dir install with files_url set can still fall through
to the default config's registry_url and fetch from the registry host, carrying the bearer
token there. Given the card is about token scope, that fallback deserves an explicit decision
rather than arriving as a side effect: pass the resolved files_url into
ref_fetch.fetch_by_ref as a parameter and delete the second resolution.

---

Smaller things

• agent is threaded through and never used. _fetcher(url, agent) ignores it; every fetch
  uses the one install-wide registry token. If the intent is per-agent authorisation, it is not
  implemented, and if it is not, the parameter invites the belief that it is.
• No response size limit. resp.read() is unbounded and the result is base64'd at 1.33x
  before returning, so a large or hostile file is two copies in memory. A max_bytes guard costs
  a line.
• The sha256 comparison is case-sensitive against a hex string supplied in the ref. An
  uppercase hash fails as a mismatch rather than matching. .lower() on both sides.
• resolve_ref_uri accepts ....//etc, which is safe on a server that does not normalise but is
  worth rejecting alongside the dot segments since it costs nothing.

What I would do with it

The rebuild does the thing it was carded for and does it correctly, which I want to say plainly
because the traversal fix is the part I was most sceptical of. Blocker 1 is the same defect
class that closed #238 reappearing on a path the fix does not cover, and I would not land it
with the token still crossing origins. Blockers 2 and 3 are both "written but not wired", and
both have a one-line reproduction above.

Revision goes back to the lane on tsk-7xcsh5. The test file is well built and each blocker above
converts directly into a failing test, so please add those first.

[jaylfc]

Revision card filed: tsk-revvkw.

This PR is not being abandoned and it stays open. The card's STEP 0 squash-merges this branch so the work carries forward.

The reason it needs a new card rather than a revision of this one's card is mechanical, and I checked it in source today rather than assuming the tooling had landed: next_card.py:32 excludes any card whose exec/* PR is open, so the original card is unclaimable while this PR exists, and executor.sh still sets BASE per repo (master for taosmd) with no per-card override, so a plain replacement card would rebuild from master and discard this branch entirely.

STEP 0 uses git merge --squash, not git merge, because executor.sh:584 bounces any card whose branch contains a merge commit. The acceptance line requires git rev-list --merges to be empty before push.

Findings are unchanged from my review above; the card summarises them and points back here as the authoritative detail.

[jaylfc]

Superseded and closed: #264 merged as ac8f714.

Same reasoning as #234, and the same hazard: #264 is a carry-forward revision of this PR (carry forward tsk-7xcsh5), so nothing auto-closed this one either. Leaving it open means a later merge silently reverts the redirect fix that just landed.

Verified on master after the merge rather than taken on trust:

• _NoRedirect at service.py:375-384 - redirect_request returns None and the opener is built with it, so the bearer token can no longer follow a 3xx to a second origin. Control: absent on pre-merge master 6de8798.
• POST /refs/fetch now has a real server route at http_server.py:1020 - that was blocker 2 here.

One known gap carried forward openly: #264's test_fetcher_uses_no_redirect_opener is vacuous (a do-nothing HTTPRedirectHandler subclass passes it while the token leaks). The fix is proven by probe, not by the suite. Carded as a follow-up rather than left implicit.

The review findings here stand as history. Branch untouched.