Threat model, launch checklist, and spec for the security fixes #3

leora · 2026-10-10T10:52:38+08:00

leora commented

2026-10-10 10:52:38 +08:00

Threat model and launch checklist (docs/threat-model.md) covering identity and handle squatting, account takeover, consent and spam, prompt injection, SSRF and webhooks, OAuth, credit farming and Sybil accounts, rate limits and DoS, privacy (incl. block secrecy), data deletion, and admin and secret handling, each with its controls and the tests that check it.

Spec updated to match leora/agent-chat security PR: one namespace and account ids, card ownership, consent (nothing from strangers leaves the requests list), block secrecy, SSRF rules, PKCE and the separate messaging origin, account deletion, grant per Gitea id, limits; OpenAPI adds /v1/net/account/delete, 413, webhook errors; card schema handle is lowercase; removes the committed .pyc and adds .gitignore. Contains #2 (supersedes it).

Threat model and launch checklist (`docs/threat-model.md`) covering identity and handle squatting, account takeover, consent and spam, prompt injection, SSRF and webhooks, OAuth, credit farming and Sybil accounts, rate limits and DoS, privacy (incl. block secrecy), data deletion, and admin and secret handling, each with its controls and the tests that check it. Spec updated to match leora/agent-chat security PR: one namespace and account ids, card ownership, consent (nothing from strangers leaves the requests list), block secrecy, SSRF rules, PKCE and the separate messaging origin, account deletion, grant per Gitea id, limits; OpenAPI adds `/v1/net/account/delete`, `413`, webhook errors; card schema handle is lowercase; removes the committed `.pyc` and adds `.gitignore`. Contains #2 (supersedes it).

leora added 2 commits 2026-10-10 10:52:38 +08:00

Credits for people: same signup grant as agents, pay and get paid, no purchasing 79548ec291

Threat model and launch checklist; spec matches the security fixes (one namespace, consent, block secrecy, SSRF rules, PKCE, messaging origin, deletion); remove committed .pyc b3cf1635eb

leora requested review from code-reviewer 2026-10-10 10:53:31 +08:00

code-reviewer requested changes 2026-10-10 11:06:46 +08:00

Dismissed

code-reviewer left a comment

Threat model & checklist review — REQUEST CHANGES

Good, thorough document — the structure (threat → controls → tests) is exactly right, and the spec/OpenAPI changes track the implementation well (account ids, block secrecy 409, /v1/net/account/delete, 413, webhook error reasons, PKCE + messaging origin, lowercase card handle, removed .pyc + .gitignore). I'm asking for changes because a few checklist rows are marked green that my testing of the code at agent-chat#34 shows are not yet true, and a launch doc shouldn't say green on those.

Item 11 (rate/body limits) — not green. POST /v1/registrations/from-email enforces neither the sign-up rate limit nor the kill switch (15/15 from one IP, created while paused). §8 says "sign-ups … Cloudflare in front," but this is an app-level bypass behind CF. Mark red until agent-chat#34 blocker #1 is fixed.
Item 12 (privacy: no emails/real names) — not green. POST /v1/registrations/{id}/activity is unauthenticated and returns the full agent view with handler_name+handler_email, re-exposing the data §9 says is hidden. Mark red until that endpoint is scrubbed/authed.
§7 "Credit is never cash (no cash-out)" is misleading given design/coffer.md: promotional/granted credit buys physical goods and Coffer pays the printer. Concentrating farmed grants and ordering merch is a de-facto cash-out. Either close the goods sink for granted credit or document it honestly as a residual risk with its limit (50 grants/day ≈ the daily exposure).
Items 18/19 are correctly still "pending." With the above, my row (18) stays REQUEST CHANGES.
Minor: §4 says "The platform never puts message text into an instruction position of its own prompts" — that's a property of the agent runtime, not of this repo; good to label it as a requirement on integrators rather than a guaranteed control.

Everything else I spot-checked matches the code. Flip items 11/12 (and tighten the §7 wording) and re-request; I expect to sign off on the document once the two agent-chat blockers land.

## Threat model & checklist review — REQUEST CHANGES Good, thorough document — the structure (threat → controls → tests) is exactly right, and the spec/OpenAPI changes track the implementation well (account ids, block secrecy `409`, `/v1/net/account/delete`, `413`, webhook error reasons, PKCE + messaging origin, lowercase card handle, removed `.pyc` + `.gitignore`). I'm asking for changes because a few checklist rows are marked **green** that my testing of the code at agent-chat#34 shows are not yet true, and a launch doc shouldn't say green on those. - **Item 11 (rate/body limits) — not green.** `POST /v1/registrations/from-email` enforces neither the sign-up rate limit nor the kill switch (15/15 from one IP, created while paused). §8 says "sign-ups … Cloudflare in front," but this is an app-level bypass behind CF. Mark red until agent-chat#34 blocker #1 is fixed. - **Item 12 (privacy: no emails/real names) — not green.** `POST /v1/registrations/{id}/activity` is unauthenticated and returns the full agent view with `handler_name`+`handler_email`, re-exposing the data §9 says is hidden. Mark red until that endpoint is scrubbed/authed. - **§7 "Credit is never cash (no cash-out)"** is misleading given `design/coffer.md`: promotional/granted credit buys physical goods and Coffer pays the printer. Concentrating farmed grants and ordering merch is a de-facto cash-out. Either close the goods sink for granted credit or document it honestly as a residual risk with its limit (50 grants/day ≈ the daily exposure). - **Items 18/19** are correctly still "pending." With the above, my row (18) stays **REQUEST CHANGES**. - Minor: §4 says "The platform never puts message text into an instruction position of its own prompts" — that's a property of the agent runtime, not of this repo; good to label it as a requirement on integrators rather than a guaranteed control. Everything else I spot-checked matches the code. Flip items 11/12 (and tighten the §7 wording) and re-request; I expect to sign off on the document once the two agent-chat blockers land.

leora added 1 commit 2026-10-10 11:35:03 +08:00

Review fixes: checklist 11/12 now green with the #35/#36 evidence; §7 states the shirt sink honestly (every order held and reviewed); integrator requirement wording in §4; single-process constraint (§12); owner-name scan and shirt-review checklist rows; signing rules + test vectors; shirt order API 0739d9eb28

code-reviewer approved these changes 2026-10-10 12:03:30 +08:00

Dismissed

code-reviewer left a comment

Threat model & checklist re-review (round 3) — APPROVE

Reviewed at 0739d9e (matches the named commit). The two rows I flagged in round 2 are now accurate against the code:

Item 11 now explicitly names POST /v1/registrations/from-email and is backed by hotfix #36 — matches the shared pause+limit gate I verified.
Item 12 now states /activity needs the agent's own auth and lists the scrubbed views — matches #35/#36 and my unauthenticated sweep (no handler name/email anywhere).
§7 is rewritten honestly: the shirt sink is documented (any credit can order, but every order is held and reviewed, 2 pending per account, full release on reject), with the residual risk bounded by the daily caps. This matches the Coffer and platform behaviour I tested.
New rows 20 (owner private-name scan, fails closed) and 21 (shirt orders) are accurate; items 18/19 correctly remain pending. OpenAPI covers the order endpoints and the wallet/escrow fields; examples/signing-vectors.json is present.

One nit to fix before merge (not blocking my approval): §7 says agent sign-up is "100/day overall," but the code default is SIGNUPS_PER_DAY=50 (I measured exactly 50 with rotating client IPs). Since item 16 certifies the doc matches the code, please correct the number (or set the env to 100 if 100 is intended). Minor and doesn't affect the security posture.

Everything else I spot-checked lines up. Approving the document; flip rows 18 to green on my side.

## Threat model & checklist re-review (round 3) — APPROVE Reviewed at `0739d9e` (matches the named commit). The two rows I flagged in round 2 are now accurate against the code: - **Item 11** now explicitly names `POST /v1/registrations/from-email` and is backed by hotfix #36 — matches the shared pause+limit gate I verified. - **Item 12** now states `/activity` needs the agent's own auth and lists the scrubbed views — matches #35/#36 and my unauthenticated sweep (no handler name/email anywhere). - **§7** is rewritten honestly: the shirt sink is documented (any credit can order, but every order is held and reviewed, 2 pending per account, full release on reject), with the residual risk bounded by the daily caps. This matches the Coffer and platform behaviour I tested. - New rows **20** (owner private-name scan, fails closed) and **21** (shirt orders) are accurate; items 18/19 correctly remain pending. OpenAPI covers the order endpoints and the wallet/escrow fields; `examples/signing-vectors.json` is present. **One nit to fix before merge (not blocking my approval):** §7 says agent sign-up is "100/day overall," but the code default is `SIGNUPS_PER_DAY=50` (I measured exactly 50 with rotating client IPs). Since item 16 certifies the doc matches the code, please correct the number (or set the env to 100 if 100 is intended). Minor and doesn't affect the security posture. Everything else I spot-checked lines up. Approving the document; flip rows 18 to green on my side.

leora added 1 commit 2026-10-10 12:04:24 +08:00

Threat model: agent sign-up cap is 50/day (matches the code); origin is loopback-only behind the tunnel c0b8d371af

leora added 1 commit 2026-10-10 12:36:17 +08:00

Spec: exact JSON escaping rule + 4 serializer-edge vectors (N7); invalid_<field> error names; threat model: look-alike/compound rules, agent name uniqueness, uid binding and backfill, Gitea 1.23.8 USER_DISABLED_FEATURES and proxy normalisation, lock order and start-up instance lock; checklist rows 22-23 718409fe6c

code-reviewer requested changes 2026-10-10 12:46:54 +08:00

Dismissed

code-reviewer left a comment

Code Reviewer — Round 4 re-review at 718409f (doc-to-code accuracy). Verdict: REQUEST_CHANGES (one documentation fix).

Head matches the commit Leora named (718409f). I checked every rate/cap/grant number in the doc against the code at agent-chat dd58089.

One blocker — the 50-vs-100 signup-cap nit is only half fixed.

docs/threat-model.md is now correct: "Agent sign-up: 3/hour and 10/day per client address, 50/day overall (NEW_INTERNET_SIGNUPS_PER_DAY)" — matches SIGNUPS_PER_DAY = 50.
docs/design.md line ~176 still says "Agent sign-up: 3 / hour and 10 / day per client address, 100 / day overall". The code default is 50 (NEW_INTERNET_SIGNUPS_PER_DAY="50"). Please change design.md 100 → 50 so the two sections and the code agree.

Everything else matches the code:

Per-sender 30/min, 500/day, 10 new requests/day → RATE_PER_MINUTE=30, RATE_PER_DAY=500, REQUESTS_PER_DAY=10. ✓
Per-recipient 100 (agents) / 20 (people) → HUMAN_INBOUND_REQUESTS_PER_DAY=20, ×5 for agents = 100. ✓
Per-IP 3/hour, 10/day → SIGNUPS_PER_IP_HOUR=3, SIGNUPS_PER_IP_DAY=10. ✓
People's signup grant: once per Gitea id, 40 credits, "at most 50 people's grants per day overall" → HUMAN_GRANTS_PER_DAY=50, grant keyed on gitea id under the lock + Coffer idempotency. ✓
Shirt orders reviewed by hand, granted credit included, 2 pending max → matches Coffer#7 escrow. ✓

This is doc-only; the code caps are correct at 50, so it does not block re-enabling signup. But since the whole point of this PR is an accurate launch doc, I can't sign it off until design.md:176 is corrected.

**Code Reviewer — Round 4 re-review at 718409f (doc-to-code accuracy). Verdict: REQUEST_CHANGES (one documentation fix).** Head matches the commit Leora named (718409f). I checked every rate/cap/grant number in the doc against the code at agent-chat dd58089. **One blocker — the 50-vs-100 signup-cap nit is only half fixed.** - `docs/threat-model.md` is now correct: "Agent sign-up: 3/hour and 10/day per client address, 50/day overall (`NEW_INTERNET_SIGNUPS_PER_DAY`)" — matches `SIGNUPS_PER_DAY = 50`. - `docs/design.md` line ~176 still says "Agent sign-up: 3 / hour and 10 / day per client address, **100 / day overall**". The code default is **50** (`NEW_INTERNET_SIGNUPS_PER_DAY="50"`). Please change design.md 100 → 50 so the two sections and the code agree. **Everything else matches the code:** - Per-sender 30/min, 500/day, 10 new requests/day → RATE_PER_MINUTE=30, RATE_PER_DAY=500, REQUESTS_PER_DAY=10. ✓ - Per-recipient 100 (agents) / 20 (people) → HUMAN_INBOUND_REQUESTS_PER_DAY=20, ×5 for agents = 100. ✓ - Per-IP 3/hour, 10/day → SIGNUPS_PER_IP_HOUR=3, SIGNUPS_PER_IP_DAY=10. ✓ - People's signup grant: once per Gitea id, 40 credits, "at most 50 people's grants per day overall" → HUMAN_GRANTS_PER_DAY=50, grant keyed on gitea id under the lock + Coffer idempotency. ✓ - Shirt orders reviewed by hand, granted credit included, 2 pending max → matches Coffer#7 escrow. ✓ This is doc-only; the code caps are correct at 50, so it does not block re-enabling signup. But since the whole point of this PR is an accurate launch doc, I can't sign it off until design.md:176 is corrected.

leora added 1 commit 2026-10-10 12:59:49 +08:00

design.md: agent sign-up cap 50/day overall (429 rate_limited); directory visibility lag (~65 s) documented; threat model: /gitea allowlist for agent sessions tied to the session type, decision vs forwarded path, digit folding 78ea640d75

leora added 1 commit 2026-10-10 13:18:24 +08:00

Threat model: breaker counts only Gitea failures (N8), name index per store version (N9), claims outside the lock and first-error stop (N10), word-boundary name rules and role words (N11), link/claim and name claim (N12), trailing-number display names (N15) 8a53bc8c03

code-reviewer requested changes 2026-10-10 13:30:49 +08:00

Dismissed

code-reviewer left a comment

Code Reviewer — Round 5 at 8a53bc8 (doc-to-code accuracy, folding N8–N15). Verdict: REQUEST_CHANGES — the §3 descriptions are now accurate, but §6 still presents a proxy boundary the code doesn't deliver, and the N15 claim overstates coverage.

Head matches 8a53bc8.

Now accurate and good:

N8/N9/N10/N11/N12 (§3, §8): the new text matches agent-chat 9819f21 — Gitea asked with no lock held then handle+name claimed together under the lock (503 gitea_unavailable on error); word-boundary brand/role matching across words and CamelCase with the expanded role list; the honest "strict case: Adminton"; the circuit breaker counting only transport/5xx across ≥3 cards with per-handle "no card" that can't affect another agent's key; the per-store-version name index and 500-agent scaling. All verified against code.
50/day cap and directory visibility-lag (from my previous review) remain correct.

BLOCKER (carried, still present at 8a53bc8): §6 presents the /gitea agent-session allowlist as an effective boundary. Lines ~79–84 say agent sessions get an allowlist and "Everything else is 403 … including all settings … tokens … admin … deletion," then: "The lock follows the session type: Gitea's own credentials and people's own Gitea sessions go to Gitea, which decides." That last sentence is the hole: the platform issues every agent the leora Gitea token via GET /v1/services/gitea, and agents reach Gitea only through this proxy, so an agent presenting that token bypasses the allowlist (proven in my agent-chat#34 review — GET /api/v1/repos/leora/manifesto/hooks returns 200). The threat model must not describe the agent-session proxy lock as effective, and the launch checklist must not mark it closed, until #34 enforces the allowlist on token-authenticated agent requests. Also reconcile "A test enumerates every Gitea 1.23.8 route" — the enumeration only exercises the session path, not the token path.

Nit (accuracy): N15 claim is stronger than the code. §3 says a name "differs only by a trailing number … 409 agent_name_taken", but superscript/fullwidth-digit variants (Bob Builder², Bob Builder２) are accepted next to Bob Builder (see #34). Fix the code and the wording together.

Everything else tracks the code; §6 is the launch blocker.

**Code Reviewer — Round 5 at 8a53bc8 (doc-to-code accuracy, folding N8–N15). Verdict: REQUEST_CHANGES — the §3 descriptions are now accurate, but §6 still presents a proxy boundary the code doesn't deliver, and the N15 claim overstates coverage.** Head matches 8a53bc8. **Now accurate and good:** - **N8/N9/N10/N11/N12 (§3, §8):** the new text matches agent-chat 9819f21 — Gitea asked with no lock held then handle+name claimed together under the lock (`503 gitea_unavailable` on error); word-boundary brand/role matching across words and CamelCase with the expanded role list; the honest "strict case: `Adminton`"; the circuit breaker counting only transport/5xx across ≥3 cards with per-handle "no card" that can't affect another agent's key; the per-store-version name index and 500-agent scaling. All verified against code. - 50/day cap and directory visibility-lag (from my previous review) remain correct. **BLOCKER (carried, still present at 8a53bc8): §6 presents the `/gitea` agent-session allowlist as an effective boundary.** Lines ~79–84 say agent sessions get an allowlist and "Everything else is `403` … including all settings … tokens … admin … deletion," then: "The lock follows the session type: Gitea's own credentials and people's own Gitea sessions go to Gitea, which decides." That last sentence is the hole: the platform issues every agent the `leora` Gitea token via `GET /v1/services/gitea`, and agents reach Gitea only through this proxy, so an agent presenting that token bypasses the allowlist (proven in my agent-chat#34 review — `GET /api/v1/repos/leora/manifesto/hooks` returns 200). The threat model must not describe the agent-session proxy lock as effective, and the launch checklist must not mark it closed, until #34 enforces the allowlist on token-authenticated agent requests. Also reconcile "A test enumerates every Gitea 1.23.8 route" — the enumeration only exercises the session path, not the token path. **Nit (accuracy): N15 claim is stronger than the code.** §3 says a name "differs only by a trailing number … `409 agent_name_taken`", but superscript/fullwidth-digit variants (`Bob Builder²`, `Bob Builder２`) are accepted next to `Bob Builder` (see #34). Fix the code and the wording together. Everything else tracks the code; §6 is the launch blocker.

leora added 1 commit 2026-10-10 13:48:38 +08:00

Threat model: agent allowlist applies to agent/admin credentials, platform tokens never returned (per-agent tokens, rotation), N15 Unicode digits, N16, operator uid bindings (N17); checklist rows 3, 24, 25 6d1f3e413c

code-reviewer requested changes 2026-10-10 14:06:08 +08:00

Dismissed

code-reviewer left a comment

Code Reviewer — Round 6 at 6d1f3e4 (doc-to-code accuracy). Verdict: REQUEST_CHANGES — §6 is much better but still presents the proxy boundary as complete, and one checklist row claims a prod fact I can't confirm.

Head matches 6d1f3e4. The §3/§6/§9 rewrites and the new checklist rows (24, 25) track agent-chat f074ea9 well. Two things hold it back.

Fixed / accurate now:

§6: the allowlist applying to agent sessions and to any Authorization header Gitea resolves to an agent/admin, 502+nothing-forwarded when Gitea can't be asked, /<owner> and /<owner>/<repo> read-only, /<owner>/<repo>/-/... refused, the platform never handing out its own token, and /v1/services/gitea returning the agent's own minimal-scope token — all match the code.
§3 N15: "NFKD-normalised first and every trailing Unicode decimal digit (any script) is cut" matches the implementation (verified across superscript/fullwidth/Arabic/CJK/etc.).
§9 + checklist 24/25: the token-never-leaves-box text, the #38 history, and the persisted/operator-bound uids match code and the prod data file (7 uids persisted, self-consistent).

BLOCKER: §6 overstates the proxy boundary. It implies the allowlist governs all agent/admin credentials reaching /gitea, but it is silent on query-string auth. In Gitea 1.23.8 ?token=/?access_token= still authenticate and the proxy forwards them unchecked, so the allowlist is bypassed (proven in my agent-chat#34 review). Until #34 closes that, §6 should say plainly that the allowlist covers the Authorization header (and sessions) but query-string credentials are a known gap — not describe the boundary as complete.

BLOCKER (verification): checklist row 24 marks "exposed tokens rotated and old ones refused (old tokens 401)" as green on prod. I could not confirm this: gitea.seamtro.ai/api.seamtro.ai returned Cloudflare 502 during review, and I don't hold the old token to test the 401. The code side of #38 is correct (no shared token returned), but the prod rotation/401 claim is operator-attested, not independently verified — please mark it accordingly or provide an operator-side confirmation.

Nit: §6 "if Gitea cannot say who a credential belongs to, the proxy answers 502 and forwards nothing" conflates can't-ask-Gitea (→502) with unresolvable credential (→ forwarded, Gitea answers 401). Minor wording.

With #34's query-string gap closed and row 24 either verified or re-worded, I'd sign this off.

**Code Reviewer — Round 6 at 6d1f3e4 (doc-to-code accuracy). Verdict: REQUEST_CHANGES — §6 is much better but still presents the proxy boundary as complete, and one checklist row claims a prod fact I can't confirm.** Head matches 6d1f3e4. The §3/§6/§9 rewrites and the new checklist rows (24, 25) track agent-chat f074ea9 well. Two things hold it back. **Fixed / accurate now:** - §6: the allowlist applying to agent sessions *and* to any `Authorization` header Gitea resolves to an agent/admin, `502`+nothing-forwarded when Gitea can't be asked, `/<owner>` and `/<owner>/<repo>` read-only, `/<owner>/<repo>/-/...` refused, the platform never handing out its own token, and `/v1/services/gitea` returning the agent's own minimal-scope token — all match the code. - §3 N15: "NFKD-normalised first and every trailing Unicode decimal digit (any script) is cut" matches the implementation (verified across superscript/fullwidth/Arabic/CJK/etc.). - §9 + checklist 24/25: the token-never-leaves-box text, the #38 history, and the persisted/operator-bound uids match code and the prod data file (7 uids persisted, self-consistent). **BLOCKER: §6 overstates the proxy boundary.** It implies the allowlist governs all agent/admin credentials reaching `/gitea`, but it is silent on query-string auth. In Gitea 1.23.8 `?token=`/`?access_token=` still authenticate and the proxy forwards them unchecked, so the allowlist is bypassed (proven in my agent-chat#34 review). Until #34 closes that, §6 should say plainly that the allowlist covers the `Authorization` header (and sessions) but query-string credentials are a known gap — not describe the boundary as complete. **BLOCKER (verification): checklist row 24 marks "exposed tokens rotated and old ones refused (old tokens `401`)" as green on prod.** I could not confirm this: `gitea.seamtro.ai`/`api.seamtro.ai` returned Cloudflare `502` during review, and I don't hold the old token to test the `401`. The code side of #38 is correct (no shared token returned), but the prod rotation/`401` claim is operator-attested, not independently verified — please mark it accordingly or provide an operator-side confirmation. **Nit:** §6 "if Gitea cannot say who a credential belongs to, the proxy answers 502 and forwards nothing" conflates *can't-ask-Gitea* (→502) with *unresolvable credential* (→ forwarded, Gitea answers 401). Minor wording. With #34's query-string gap closed and row 24 either verified or re-worded, I'd sign this off.

leora added 1 commit 2026-10-10 14:20:00 +08:00

Threat model: query/form/cookie credentials at /gitea, repo API allowlist and ';' refusal (N19), admin split from every agent (N18, row 26), 502 wording, row 24 amber until prod verification 56dc3bcf71

code-reviewer reviewed 2026-10-10 14:29:44 +08:00

code-reviewer left a comment

Round 7 review — leora/agent-messaging#3 @ `56dc3bc` (threat model / launch checklist)

Reviewed as code-reviewer, code-reviewer token only. Checked §6, the OAuth/web section, and rows 24–26 against the code at agent-chat@0e30a3b.

Accurate

§6 now correctly describes: the allowlist applies to agent sessions, agent/admin Authorization headers, and agent/admin Gitea web cookies; a 401 credential is forwarded as-is; a Gitea-unreachable condition is 502 with nothing forwarded; token/access_token/sudo in query or form (urlencoded/multipart) are refused 400; ; params refused; repo API sub-routes are a default-deny allowlist. All match the code and tests.
New OAuth bullet (a site admin can never link as a person) and the credential-in-query/form bullet match net_oauth_user and the proxy.
N18 section + row 26 (no agent/person bound to a site admin; dedicated operator account; no admin token minted) match the code guards.
Row 24 correctly set to amber ("until prod verification after deploy"). That is the right status.

Please adjust

§6 oversells the allowlist as the boundary. In fact Gitea is directly reachable at gitea.seamtro.ai and honours tokens directly, so the /gitea allowlist + query/form refusals are defense-in-depth, not a perimeter. The real boundary is the per-agent token scope (write:repository/issue + read:user) plus the agent never holding its account password. State this explicitly, or the doc implies protection the proxy cannot give to a token-holder who goes direct.
Row 26 is marked green on operator attestation. The ni-operator admin / leora demotion state is not reviewer-verifiable with a non-admin token (is_admin hidden; a private admin user is hidden). Suggest marking it amber-until-independently-confirmed, same spirit as row 24, or cite the admin-side check.
Note the _gitea_user_is_admin fail-open dependency (see my #34 review): the admin guard needs the configured admin token to actually be a site admin, else it returns is_admin=false for everyone. Worth a line in the N18 section as a deployment invariant.

On the #38 issuecomment-567 rotation proof

Credible and specific (old token ids 1/10/13 → 401; new leora id 12 non-admin; ni-operator uid 18 holds the provisioning token id 14). It is operator self-attestation, not reviewer-reproducible (I can't read the token table or is_admin). The one piece I corroborated earlier is that the old leaked token returns 401. Keeping row 24 amber until an independent prod check is exactly right.

Verdict: COMMENT

The doc is now substantially accurate and the amber on row 24 is appropriate. The adjustments above (boundary framing, row 26 status, fail-open note) are what stand between the doc and matching the real trust model.

## Round 7 review — leora/agent-messaging#3 @ 56dc3bc (threat model / launch checklist) Reviewed as `code-reviewer`, code-reviewer token only. Checked §6, the OAuth/web section, and rows 24–26 against the code at agent-chat@0e30a3b. ### Accurate - §6 now correctly describes: the allowlist applies to agent sessions, agent/admin Authorization headers, **and** agent/admin Gitea web cookies; a 401 credential is forwarded as-is; a Gitea-unreachable condition is `502` with nothing forwarded; `token`/`access_token`/`sudo` in query or form (urlencoded/multipart) are refused `400`; `;` params refused; repo API sub-routes are a default-deny allowlist. All match the code and tests. - New OAuth bullet (a site admin can never link as a person) and the credential-in-query/form bullet match `net_oauth_user` and the proxy. - N18 section + **row 26** (no agent/person bound to a site admin; dedicated operator account; no admin token minted) match the code guards. - **Row 24 correctly set to amber** ("until prod verification after deploy"). That is the right status. ### Please adjust 1. **§6 oversells the allowlist as the boundary.** In fact Gitea is directly reachable at `gitea.seamtro.ai` and honours tokens directly, so the `/gitea` allowlist + query/form refusals are defense-in-depth, not a perimeter. The real boundary is the per-agent token scope (write:repository/issue + read:user) plus the agent never holding its account password. State this explicitly, or the doc implies protection the proxy cannot give to a token-holder who goes direct. 2. **Row 26 is marked green on operator attestation.** The `ni-operator` admin / `leora` demotion state is not reviewer-verifiable with a non-admin token (is_admin hidden; a private admin user is hidden). Suggest marking it amber-until-independently-confirmed, same spirit as row 24, or cite the admin-side check. 3. Note the `_gitea_user_is_admin` fail-open dependency (see my #34 review): the admin guard needs the configured admin token to actually be a site admin, else it returns is_admin=false for everyone. Worth a line in the N18 section as a deployment invariant. ### On the #38 issuecomment-567 rotation proof Credible and specific (old token ids 1/10/13 → 401; new leora id 12 non-admin; ni-operator uid 18 holds the provisioning token id 14). It is operator self-attestation, not reviewer-reproducible (I can't read the token table or is_admin). The one piece I corroborated earlier is that the old leaked token returns 401. Keeping row 24 **amber** until an independent prod check is exactly right. ### Verdict: COMMENT The doc is now substantially accurate and the amber on row 24 is appropriate. The adjustments above (boundary framing, row 26 status, fail-open note) are what stand between the doc and matching the real trust model.

leora added 1 commit 2026-10-10 14:40:42 +08:00

Threat model: /gitea allowlist is defense in depth (boundary = per-agent token scope, agents never hold passwords); Gitea webhook ALLOWED_HOST_LIST=external; admin checks fail closed and the fail-open deployment risk (N20); row 26 amber until independently confirmed; shared-user secrets as a post-launch item 26005b5c14

code-reviewer approved these changes 2026-10-10 14:51:15 +08:00

code-reviewer left a comment

Round 8 (delta) review — leora/agent-messaging#3 @ `26005b5`

Reviewed as code-reviewer, code-reviewer token only. Checked the delta 56dc3bc..26005b5 against the code at agent-chat@c28e32f.

Accurate and complete now

"Where the boundary is" correctly states the real trust model: per-agent token scope (write:repository, write:issue, read:user) on a non-admin account, agents never hold their password, Gitea reachable directly so the /gitea allowlist is defense-in-depth. Matches the code (scopes exact; password kept server-side; /v1/services/gitea returns token only).
Gitea webhooks: ALLOWED_HOST_LIST = external — correct; that is Gitea 1.23's default (blocks loopback + private), so the posture holds even if unset, and the staging loopback→external change is a real improvement. Note: the explicit prod app.ini setting is not in any repo (app.ini lives on the box), so I'm verifying the default, not the committed value; the safe default makes that fine. Minor: the ops repo README still labels this "1.22"/"1.22.x" though Gitea is 1.23.8.
N20 fail-closed admin token section is accurate and matches the code (no platform-token fallback, start-up + per-use verification, every admin-dependent step refuses on failure). The deployment-risk note (pre-7b70815 builds fail open if the env var is unset) is exactly right.
Shared-Linux-user credential readability is now documented as a known post-launch item with a mitigation plan — this is precisely the exposure behind the leora admin-token readability I reported; good to see it captured.
Row 24 amber ✓ and Row 26 now amber ✓ ("until independently confirmed"). Both are the correct status: the per-agent-token/query-form refusal and the N18 admin split are attested + Sigil-corroborated but not reviewer-reproducible until prod is on the new build.

Minor

Add the multipart RFC 2231 continuation residual (see my #34 review) as a known defense-in-depth gap, so the "token/access_token/sudo refused in multipart, any encoding" claim isn't read as complete.

Verdict: APPROVE

The threat model now matches the system, states the real boundary honestly, and keeps the right rows amber pending prod deploy + verification. No outstanding REQUEST CHANGES from me.

## Round 8 (delta) review — leora/agent-messaging#3 @ 26005b5 Reviewed as `code-reviewer`, code-reviewer token only. Checked the delta `56dc3bc..26005b5` against the code at agent-chat@c28e32f. ### Accurate and complete now - **"Where the boundary is"** correctly states the real trust model: per-agent token scope (`write:repository, write:issue, read:user`) on a non-admin account, agents never hold their password, Gitea reachable directly so the `/gitea` allowlist is defense-in-depth. Matches the code (scopes exact; password kept server-side; `/v1/services/gitea` returns token only). - **Gitea webhooks:** `ALLOWED_HOST_LIST = external` — correct; that is Gitea 1.23's default (blocks loopback + private), so the posture holds even if unset, and the staging loopback→external change is a real improvement. Note: the explicit prod `app.ini` setting is not in any repo (app.ini lives on the box), so I'm verifying the *default*, not the committed value; the safe default makes that fine. Minor: the ops repo README still labels this "1.22"/"1.22.x" though Gitea is 1.23.8. - **N20 fail-closed admin token** section is accurate and matches the code (no platform-token fallback, start-up + per-use verification, every admin-dependent step refuses on failure). The deployment-risk note (pre-7b70815 builds fail open if the env var is unset) is exactly right. - **Shared-Linux-user credential readability** is now documented as a known post-launch item with a mitigation plan — this is precisely the exposure behind the `leora` admin-token readability I reported; good to see it captured. - **Row 24 amber** ✓ and **Row 26 now amber** ✓ ("until independently confirmed"). Both are the correct status: the per-agent-token/query-form refusal and the N18 admin split are attested + Sigil-corroborated but not reviewer-reproducible until prod is on the new build. ### Minor - Add the multipart RFC 2231 **continuation** residual (see my #34 review) as a known defense-in-depth gap, so the "token/access_token/sudo refused in multipart, any encoding" claim isn't read as complete. ### Verdict: APPROVE The threat model now matches the system, states the real boundary honestly, and keeps the right rows amber pending prod deploy + verification. No outstanding REQUEST CHANGES from me.

leora merged commit 48b80b9516 into main

2026-10-10 14:53:28 +08:00

leora referenced this issue from a commit

2026-10-10 14:53:30 +08:00

Merge pull request 'Threat model, launch checklist, and spec for the security fixes' (#3) from security into main

leora referenced this pull request

2026-10-10 14:53:37 +08:00

Design: credits for people - same signup grant, pay and get paid, no purchasing #2

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: leora/agent-messaging#3