Revise PR #320: fail closed when ?reader= disagrees with the verified token, and restore the archive limit the mention feed lost

[jaylfc]

CARD TITLE (intent, not commit subject): Revise PR #320: fail closed when ?reader= disagrees with the verified token, and restore the archive limit the mention feed lost

Autonomous build of board card tsk-wlq4ui.

Files:
changelog.d/tsk-wlq4ui-mention-fixes.md | 17 +
taosmd/api.py | 4 +
taosmd/http_server.py | 59 +++-
taosmd/mentions.py | 92 +++++
taosmd/remote.py | 20 ++
taosmd/service.py | 154 ++++++++-
tests/test_a2a_mentions.py | 584 ++++++++++++++++++++++++++++++++
8 files changed, 935 insertions(+), 2 deletions(-)

[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: 21 minutes

Limit details: You’ve used all 1 included review currently available under your plan.

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: 294daece-cf7b-4a08-9c4c-596d65e1be59

📥 Commits

Reviewing files that changed from the base of the PR and between c0d8b8b and 241601b.

📒 Files selected for processing (7)

• changelog.d/tsk-6icvd4-mentions-feed.md
• taosmd/api.py
• taosmd/http_server.py
• taosmd/mentions.py
• taosmd/remote.py
• taosmd/service.py
• tests/test_a2a_mentions.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: _handle_a2a_mentions does not fail closed when ?reader= disagrees with the verified token sub

An authenticated user can supply an arbitrary ?reader= query parameter to read another user's mentions. The code uses qp_reader or token_sub, which picks the caller-supplied value when present. This is an identity-spoofing / authorization bypass.

---

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

[kilo-code-bot]

WARNING: _handle_a2a_mentions catches _ra.AuthError but not _ra.HumanAuthError

If authorize raises HumanAuthError, it will bubble up to the generic except Exception handler and return HTTP 500 instead of 403. This is inconsistent with _handle_a2a_send, which handles HumanAuthError explicitly.

---

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

[kilo-code-bot]

SUGGESTION: Dead code: root_threads is computed but never used

root_threads is built on line 940 but never referenced later in the function. The per-message thread_root field is populated from the separate thread_roots dict instead.

---

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
CRITICAL	1
WARNING	1
SUGGESTION	1

Issue Details (click to expand)

CRITICAL

File	Line	Issue
taosmd/http_server.py	1699	_handle_a2a_mentions does not fail closed when ?reader= disagrees with verified token sub

WARNING

File	Line	Issue
taosmd/http_server.py	1695	_handle_a2a_mentions catches _ra.AuthError but not _ra.HumanAuthError

SUGGESTION

File	Line	Issue
taosmd/service.py	940	Dead code: root_threads is computed but never used

Files Reviewed (7 files)

• taosmd/http_server.py - 2 issues
• taosmd/service.py - 1 issue
• taosmd/mentions.py
• taosmd/remote.py
• taosmd/api.py
• tests/test_a2a_mentions.py
• changelog.d/tsk-6icvd4-mentions-feed.md

Fix these issues in Kilo Cloud

---

_{Reviewed by step-3.7-flash · Input: 89.7K · Output: 25K · Cached: 1.5M}

[jaylfc]

BLOCKED. The carry-forward is done correctly and completely, which was STEP 0 of the card and is the part that is easy to get wrong. But neither blocker was fixed. _handle_a2a_mentions is byte-identical to the branch this revises, and the archive query is unchanged, so the PR's title describes work that is not in it.

Revision card is tsk-mxjvnb, filed and read-back verified before this comment.

What went right, and it is not nothing

• git rev-list --merges c0d8b8b..origin/exec/tsk-wlq4ui is 0, so the executor's merge-commit bounce was avoided and the --squash instruction was followed.
• The carry-forward is real and complete: 7 files, +906/-2, taosmd/mentions.py, the RemoteClient reader plumbing, and all 25 tests from tests/test_a2a_mentions.py.
• Suite on a trial merge with c0d8b8bb: 1521 passed, 12 skipped. Four gates clean, no conflict markers.

Everything on the card's "carry ALL of it forward unchanged" list is present. Keep all of it again.

BLOCKER 1 - ?reader= still overrides the verified token

taosmd/http_server.py:1699, unchanged:

qp_reader = (qs.get("reader") or [None])[0]
reader = qp_reader or token_sub

_handle_a2a_mentions is string-equal between origin/exec/tsk-6icvd4 and origin/exec/tsk-wlq4ui, same length, same line numbers. There is no comparison between qp_reader and token_sub anywhere in the function, so nothing can fail closed.

Measured on the merged tree using this PR's own authed_server fixture:

CONTROL  carol token, no ?reader=      -> 200 ['note for @carol']
ARM      carol token, ?reader=bob      -> 200 ['secret for @bob']    <- required: 403
CONTROL  carol token, ?reader=carol    -> 200 ['note for @carol']

Both controls behave, so the middle row is a reading rather than a broken probe. The required shape is quoted verbatim from the card: verified sub is authoritative; a ?reader= that does not normalise to it fails closed with 403; a matching one is served so the client's honest round-trip keeps working; the standalone path stays free. Fallback plus fail-closed on mismatch, never the reuse arm.

One thing to fix alongside it: authorize(token, raw_sub) takes raw_sub from an unverified decode of the same token. The signature check still runs, so this is not the hole, but it makes the anti-impersonation comparison inside authorize_sender vacuous by construction, because it compares the token's subject against itself.

BLOCKER 2 - the archive limit is still missing

taosmd/service.py:922, unchanged:

all_rows = await archive.query(event_type=EVENT_A2A)

Still the only bus read in service.py without an explicit limit; the siblings at 620, 699, 733 and 818 all carry limit=100_000. Measured by posting one mention and pushing unrelated traffic in behind it:

bus size   10   40   49   51   60   120
visible     1    1    1    0    0     0     <- cliff exactly at 50

200 with an empty list and no error. The live build channel is past 3100 messages.

The suite count is the third piece of evidence

1521 is the 1496 baseline plus exactly the 25 carried-forward tests, so no test was added for either blocker. The card required both by name. Grepping the test file for reader= finds only the standalone no-verifier case, where honouring the parameter is correct.

The generalisable half

The mechanics this card warned about were all handled: squash not merge, no merge commits, the full file set brought across. What did not happen is the fix. A carry-forward that lands cleanly produces a large, healthy-looking diff (+906) and a rising suite count, and both of those read as progress on a revision. Neither is evidence that a blocker moved. The check that separates them is small and specific: diff the two named lines against the branch being revised, and run the two arms the previous review recorded. Both came back identical here, and both take seconds.