Revise PR #232: receipt identity is forgeable, and a routed admin endpoint has no handler

[jaylfc]

CARD TITLE (intent, not commit subject): Revise PR #232: receipt identity is forgeable, and a routed admin endpoint has no handler

Autonomous build of board card tsk-qo6tpb.

REVIEW WARNING (automated): this card's text asks for tests, but the diff changes no test file. Either the acceptance criteria are unmet or the card needs correcting. Do not merge without resolving this.

Files:
taosmd/api.py | 4 ++
taosmd/http_server.py | 110 ++++++++++++++++++++++++++++++++++-
taosmd/migrations.py | 18 ++++++
taosmd/receipts.py | 158 ++++++++++++++++++++++++++++++++++++++++++++++++++
taosmd/service.py | 98 +++++++++++++++++++++++++++++++
5 files changed, 387 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: 36 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: b0b3cfe5-087e-4916-a571-c8a03288bc2b

📥 Commits

Reviewing files that changed from the base of the PR and between 6de8798 and fa3f980.

📒 Files selected for processing (5)

• taosmd/api.py
• taosmd/http_server.py
• taosmd/migrations.py
• taosmd/receipts.py
• taosmd/service.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]

CRITICAL: Missing _handle_admin_a2a_prune_receipts method definition

The dispatch at line 1089-1090 calls self._handle_admin_a2a_prune_receipts() for POST /a2a/admin/prune-receipts, but this method is never defined anywhere in the file. This will raise AttributeError at runtime whenever the admin endpoint is called.

---

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

[kilo-code-bot]

WARNING: JWT decoded without signature verification

_get_authenticated_agent_id uses jwt.decode(token, options={"verify_signature": False}) to extract the sub claim, but never actually verifies the token. The docstring claims "verified registry token" yet no verification occurs. Any client can forge a JWT with an arbitrary sub claim and impersonate any agent.

Additionally, because _check_token only accepts the server token (which is not a JWT), this method always returns None for authenticated requests, making the new PATCH /a2a/receipts endpoint always return 401 and preventing delivered receipts from ever being recorded in the SSE stream.

---

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

[kilo-code-bot]

Code Review Summary

Status: 2 Issues Found | Recommendation: Address before merge

Overview

Severity	Count
CRITICAL	1
WARNING	1

Issue Details (click to expand)

CRITICAL

File	Line	Issue
taosmd/http_server.py	1090	_handle_admin_a2a_prune_receipts is called but never defined, causing AttributeError at runtime

WARNING

File	Line	Issue
taosmd/http_server.py	747	_get_authenticated_agent_id decodes JWT without verifying signature, allowing identity forgery; also makes receipt endpoints non-functional because _check_token only accepts non-JWT server tokens

Files Reviewed (5 files)

• taosmd/api.py
• taosmd/http_server.py - 2 issues
• taosmd/migrations.py
• taosmd/receipts.py
• taosmd/service.py

Fix these issues in Kilo Cloud

---

_{Reviewed by step-3.7-flash · Input: 108.8K · Output: 22.5K · Cached: 1.6M}

[jaylfc]

Reviewed. Blocked: 3 blockers, all three are the original #232 defects unchanged. Everything below was run, with the positive control in the same command.

One thing is genuinely better than its predecessors and I want to lead with it, because it is the defect that broke #230 and #266: the new migration is registered correctly. _A2A_RECEIPTS is added to REGISTRY and a2a-receipts.db to DB_FILES, with a table_exists guard rather than a column check. That is the fix those two PRs got wrong, done right here.

BLOCKER 1 - Receipt identity is still forgeable, and the docstring now claims otherwise

_get_authenticated_agent_id (http_server.py:729) is unchanged in substance:

unverified = _jwt.decode(token, options={"verify_signature": False})
sub = unverified.get("sub", "") or ""
return sub if sub else None

Run against that exact code path, with a token signed by a key the server has never seen:

FORGED token signed with a key the server has never seen.
  _get_authenticated_agent_id returns: 'taOS-dev'

POSITIVE CONTROL - the real verifier, same forged token:
  decode_and_verify REJECTED the forgery: AuthError
  CONTROL legit token ACCEPTED, sub = taosmd-dev

CONTROL - token with no sub: None
CONTROL - garbage:           None

So any client can still write receipts as any agent, and the correct API rejects the very token this path accepts.

Two things make this worse than it was on #232. First, the correct call is in this same file, 126 lines below: _registry_verifier.authorize(token, claimed_identity) at line 873. The file now contains the right pattern and the wrong pattern, and the receipts path picked the wrong one again.

Second, the new docstring says:

"Return the agent identity from a verified registry token"

The code does not verify. A docstring asserting a security property the code lacks is worse than the untouched original - it is what a future reviewer will read instead of the four lines below it. If nothing else in this PR changes, that word must go.

BLOCKER 2 - POST /a2a/admin/prune-receipts still has no handler

Dispatched at line 1090 (self._handle_admin_a2a_prune_receipts()), never defined. Enumerated both sides so this is not a grep artefact:

DEFINED (6)                              CALLED (7)
_handle_admin_a2a_delete_channel          _handle_admin_a2a_delete_channel
_handle_admin_a2a_rename_channel          _handle_admin_a2a_prune_receipts   <-- no definition
_handle_admin_a2a_supersede_message       _handle_admin_a2a_rename_channel
_handle_admin_shelf_archive               _handle_admin_a2a_supersede_message
_handle_admin_shelf_create                _handle_admin_shelf_archive
_handle_admin_shelf_unarchive             _handle_admin_shelf_create
                                          _handle_admin_shelf_unarchive

Six of seven resolve; the one that does not is exactly the route this PR adds. The other six are the control - the grep finds definitions when they exist. The endpoint is documented at line 153 and will AttributeError on the first call. This is #232 defect 2, unchanged.

BLOCKER 3 - Five service wrappers forward to remote methods that do not exist

remote.py is not in this diff at all, yet service.py gained calls into it:

  present: a2a_channels            MISSING: a2a_get_receipt
  present: a2a_feed                MISSING: a2a_get_receipts
  present: a2a_members             MISSING: a2a_prune_receipts
  present: a2a_send                MISSING: a2a_record_delivered
                                   MISSING: a2a_record_seen

The four present ones are the control: pre-existing wrappers resolve, and every one of the five added by this PR is missing. So on any remote-configured install, all five receipt operations raise AttributeError. Local-only suites cannot see it - which is the whole reason tsk-bwgr26 was carded (a CI gate matching RemoteClient._run literals against real routes). This is now the fourth PR in a row shipping a client half with no server half (#232, #234, #242, this one).

DEFECT 4 - No tests at all

Zero changes under tests/. 387 added lines, a new database, a new admin route and a new identity path, with no coverage. That is #232 defect 4 unchanged, and it is why all five checks are green on a PR whose three blockers are each reproducible in a few lines.

Also

This branch conflicts with master as it stands.

What I recommend

Do not rebuild - receipts.py and the migration are sound and worth keeping. The fix list is short and mechanical:

1. Replace the unverified decode with _registry_verifier.authorize(...) (line 873 is the working example), and drop "verified" from the docstring until it is true.
2. Write _handle_admin_a2a_prune_receipts.
3. Add the five RemoteClient methods.
4. Tests, including one that presents a forged token and asserts no receipt is written - that assertion is the point of the card, so it must go red without the fix.

[jaylfc]

Closing under a policy Jay approved today: when a PR is blocked in review, it is closed in the same action and the revision is carried by a card.

The reason is mechanical, and I measured it before proposing it. A blocked PR in this repo is never revised in place. Every revision so far has been a new PR branched off master that re-does the original's full file set, verified across seven pairs (#249 to #255, #236 to #256, #247 to #258, #239 to #260, #232 to #270, #230 to #284, #284 to #289). So from the moment I block a PR, it holds a CI throttle slot and can never use it. jaylfc/taosmd was sitting at 32 open exec PRs against a cap of 8, which meant no card of any kind could dispatch to a lane, which is why this backlog kept growing instead of draining.

Nothing here is lost, and I checked each part rather than assuming it:

• The revision card tsk-otz3nn carries the blockers from my review, with a link back to the full text.
• This review stays readable. Closing a PR does not delete its comments.
• The branch exec/tsk-qo6tpb still exists. Closing a PR does not delete its branch. git fetch origin exec/tsk-qo6tpb recovers the work.
• The originating card tsk-qo6tpb is closed, so no lane re-dispatches it from master and rebuilds the same defects. That ordering matters: the card went first, then this PR.

Reopen if you disagree with the disposition. This is a throttle decision, not a judgement that the work was wrong.