fix(session-ingest): refuse unauthorized organization_id claims; hide never-ingested placeholders - #4780
Conversation
…meState Add the persisted per-session cost column to the getWithRuntimeState output schema and return projection, thread it through FetchedSessionData as an optional field, and populate it in the mobile fetchSession mapping. The mobile session detail screen will use it to render the same canonical cost the list reads from the Postgres column. Web fetchSession behavior is unchanged (the new field is optional).
The list rendered the persisted column at 2 decimals with a </bin/zsh.01 floor while the detail rendered a client-side message sum at 4 decimals, plus a hand-rolled toFixed(4) duplicate in the header fallback, and spoken labels that dropped sub-cent costs entirely. The same session read as up to four different costs, and an empty-snapshot session showed its cost in the list but nothing on the detail screen. Derive every surface from one canonical microdollar total: the detail combines the persisted column and the live message sum via selectSessionCostInputs (max of two lower bounds, cost being monotonic), all surfaces render through formatSessionTotalCost, and all spoken labels through formatSpokenCost on the same threshold. The context sheet splits its total (combined microdollars) from its breakdown input (live USD sum) so no phantom subagent residual appears, and omits the Total cost row instead of rendering $0.0000 when no cost has been reported.
The best-effort total_cost_microdollars write sat after the unguarded O11Y ingestSessionMetrics call, so an analytics binding failure skipped the cost persist entirely and the column stayed NULL. Reorder so the cost write runs first; the O11Y call stays unguarded and the metricsEmitted dedup is unchanged, preserving the existing alarm retry semantics.
Standalone worker-side query mirroring the session-access predicates in cloud-agent-session-access.ts: a direct organization_memberships row for (kilo_user_id, organization_id) and a non-soft-deleted organization. Parent-organization inheritance and kilocode_users.is_admin are deliberately excluded, matching every existing worker-side check.
…ingest applyMetadataChanges persisted cli_sessions_v2.organization_id verbatim from the client-supplied kilo_meta.orgId with no membership check, so any caller could re-tenant their own session into any existing organization, and a nonexistent organization aborted the whole metadata batch on the FK while the client still got HTTP 200. Gate the write on hasOrganizationAccess inside the same transaction; on refusal drop only organization_id, persist the rest of the batch, and warn. Access-cache invalidation and changedNonStatus now key on an actually-applied org write so a refusal is not reported as a scope change.
…ist and search POST /api/session creates bare placeholder rows before any user turn; if the CLI dies before its first debounced flush the row stays permanently title/status/cost NULL with created_on_platform 'unknown' and renders as Untitled session / UNKNOWN in the Agents list. Exclude rows where all four list columns are unwritten; any row with a title, a status, a known platform, or a persisted cost (including 0) still appears.
Regression coverage for the guarantee that the Postgres total_cost_microdollars persist completes before the unguarded O11Y.ingestSessionMetrics RPC: drives alarm() with O11Y rejecting and asserts the update chain (connection, value, session/user filter) was awaited first. Marker records at await time via a thenable, so a build-now-await-later refactor also fails.
|
(bot) E2E verification complete on head 43558b0 (iOS simulator + local stack, port offset 1600): Item 1 — org authorization (service-level, against session-ingest):
Item 2 — placeholder filter (on-device):
Criterion 8a adversarial row (content committed to DO, no metadata projection, cost NULL): hidden from list/search — the documented accepted residual; the row is not deleted (export API still returns its content). All fixture rows (13) deleted; simulator released with its original name restored. |
…ix/session-ingest-org-authz
…ix/session-ingest-org-authz
|
(bot) Standin review of head Full-diff review (fresh reviewer): No valid actionable findings. Non-blocking observations: the documented DO ingest-meta residual after a refused org write; the helper's unit tests assert at the mock-chain level (predicates mirror Security review of the final diff: No findings. Both outcomes are now also recorded in the PR body. Local validation on this exact head:
CI note (corrected): on head |
|
(bot) Standin review of head Result: No findings. Verification performed (all against the full diff
Residual testing risks:
|
Batch J — PR #4777 follow-ups: ingest org authorization + placeholder sessions
Stacked on
fix/mobile-cost-and-exit(#4777). GitHub retargets this PR tomainautomaticallywhen #4777 merges. Two follow-ups from #4777's out-of-scope list, both reproduced on the
unmodified baseline before fixing.
Item 1 —
session-ingestpersistedorganization_idwith no membership check (security)POST /api/session/:id/ingestpersistedcli_sessions_v2.organization_idverbatim from theclient-supplied
kilo_meta.orgId(validated only asuuid().optional()), with no check that thecaller belongs to the claimed organization.
Measured blast radius (reproduced on the baseline; narrower than "steal a session into your
organization"):
read, export, and further ingest all return
session_not_found404, because session readsrequire membership once
organization_idis set. A data-availability loss — the most concreteuser harm.
organization_idspace with no membership relationship — adata-integrity violation. Two org-scoped reads filter on
organization_idwithoutkilo_user_id(shareForWebhookTrigger's org branch,feature-adoption), but neither isexploitable for a cross-tenant read today: the first still requires webhook-trigger access to
the org, the second additionally requires
cloud_agent_session_id IS NOT NULL, which ingestcannot set.
still gets 404 — every session read also requires
kilo_user_id = caller.A second bug closed by the same guard: a claimed
orgIdthat is a valid uuid but no existingorganization raised the FK inside the metadata transaction, so the entire metadata batch was
lost — title, platform, status, git fields — while the client was told
HTTP 200 {"success":true}. With the check in front of the write, an unauthorized or nonexistentorganization is refused as a field and everything else in the batch persists.
Fix. New
hasOrganizationAccessin@kilocode/worker-utils(directorganization_membershipsrow for the pair +organizations.deleted_at IS NULL; mirrors theexisting worker-side session-access predicates as a standalone query; no parent-org inheritance,
no admin bypass — uniform with every worker-side check), called inside
applyMetadataChanges—the single funnel for all three ingest callers (direct, queue, partial flush) — on the same
transaction as the UPDATE.
Decision: refuse the field, don't reject the request. Content is committed to the DO and R2
before metadata is projected, so a 403 would report failure for data already stored; the queue
path has no client to reject; and rejecting would freeze title/status for legitimate users whose
membership was revoked mid-session or whose org was soft-deleted. Refusing loses nothing: the
session stays exactly where it legitimately is. Refusals are
console.warn'd (ids only).Access-cache invalidation and the non-status-change signal now key on an actually-applied org
write, so a refusal is not misreported as a scope change. An ingest with no
orgIdperforms zeroadditional queries.
Tests: new
applyMetadataChangessuite (member persists + cache invalidated; non-memberrefused while title/git/status persist; refusal is not a scope change — no cache drop, no phantom
session.updated; refusal + parentId still emits; soft-deleted refused; no-orgId identical withzero membership queries; explicit null still clears; nonexistent org no longer loses the batch)
plus helper unit tests (member / non-member / soft-deleted member).
Item 2 — stray
Untitled session/UNKNOWNrows in the Agents list (data hygiene)POST /api/sessioncreates a bare placeholder row before any user turn; the CLI fires it eagerlyon local session creation and metadata arrives later through a ~1s-debounced queue. If the CLI
dies inside that window the row is permanently
title NULL,status NULL,created_on_platform 'unknown',total_cost_microdollars NULL— and renders in the list asUntitled session/UNKNOWN. Reproduced on the baseline (kill-in-debounce), with a healthycontrol session writing all four columns.
Deleting or not creating the row is not available (mobile-initiated
create_sessiondepends onthe cloud row existing so the session can attach). The fix is at the shared query boundary:
cliSessionsV2.listandcliSessionsV2.searchnow exclude rows where all four list columnsare unwritten.
What the predicate proves, precisely: all four unwritten means no metadata projection ever
succeeded and no metrics emission ever persisted a cost. It does not prove the row has no
content — content lives in the DO/R2 and commits independently of the projection, and
total_cost_microdollarsis written only by the alarm-driven metrics emission (best-effort), notper flush: presence proves a metrics emission happened, absence proves nothing.
Accepted residual, as an invariant: any row whose four list columns are all unwritten is
hidden, regardless of whether the DO holds content. No Postgres-visible signal can distinguish
that from a bare placeholder on a paginated list query. In practice the exposure window is the
~1s startup debounce (a single successful projection reveals the row), and item 1 removes the
largest durable cause — a bogus
orgIdin CLI config previously aborted every projection on theFK.
Interactions:
'unknown'is not inKNOWN_PLATFORMS, so these rows previously fell into theotherplatform filter bucket; that bucket no longer lists placeholders (intended).recentRepositoriesis unchanged (already requiresgit_url IS NOT NULL). No mobile change: thelive/pinned tray already neutralises
'unknown'toLIVEfor freshly created sessions.Tests: placeholder hidden from
list; titled-but-'unknown'shown; status-but-no-titleshown; cost-only (including
total_cost_microdollars = 0) shown; pagination stable withplaceholders interleaved;
searchby exact session id hides the placeholder.Existing-rows audit (local only — production was NOT queried)
Local result: 0 rows. This workflow has no production database access — production was not
queried, and no data migration is included. Note the FK's
ON DELETE SET NULLmeans rowspointing at since-deleted organizations were already nulled and are invisible to this query.
Post-merge manual action: run the audit against production and decide whether any rows need
repair.
Follow-ups reported, deliberately not fixed here
session-ingest-rpc/cloud-agent-next session-prepareorganization write:createSessionForCloudAgentwritesorganization_idfrom a zod-string().optional()fieldwith no membership check, and
session-prepare(unlikesession-start) performs nomembership assertion. Verified not client-reachable with an attacker-chosen organization:
prepareSessionisinternalApiProtectedProcedure(INTERNAL_API_SECRET + customer token), andboth entrances establish the organization server-side — the org router injects the
membership-checked id, and the personal router's schema strips any client-supplied org field.
Defense-in-depth candidate behind two server boundaries; not this PR.
orgIdandemits only on change, so after a refusal the DO believes the org is set while Postgres does
not; re-sending the same
orgIdwon't re-emit (desirable for the attack case; a user who latergenuinely joins the org stays personal until the CLI sends a different value). Clearing one DO
ingest-meta key needs a new DO method + RPC surface — out of proportion for a security fix.
create_sessionrollback leaves an orphan cloud row (mobile-initiatedsession with no prompt rolls back locally only on attach failure). One source of placeholder
rows; item 2 hides them rather than deleting them.
Security review
A dedicated security review of the final diff ran (gating for item 1). Scope: the new
hasOrganizationAccesshelper, theapplyMetadataChangesguard and its side-effectre-keying, the list/search predicate, every caller path that reaches them (direct ingest,
queue consumer, partial flush), the o11y metrics path, and the FK behavior for
refused/nonexistent claims. Outcome: no findings. A fresh full-diff review (standin for
the automated reviewer, which posts no summary on PRs whose base is not
main) alsoreturned no valid actionable findings; its non-blocking observations are the DO
ingest-meta residual and the RPC-path follow-up already documented above, plus a note that
the helper's unit tests mock at the chain level (the predicates mirror
cloud-agent-session-access.tson inspection).Verification
@kilocode/worker-utils: 326 tests, typecheck, lint — green.services/session-ingest: 604 tests (3 skipped), typecheck, lint — green on the finalhead.
pnpm test:integrationfails at import time identically on the clean baseline(
pgCJS/ESM shim under vitest-pool-workers; 2 files, 0 tests) — pre-existing toolchainissue, not introduced here; the DO integration sources are unmodified and no CI job runs
this suite.
apps/web: typecheck, lint, Jestcli-sessions-v2-router74/74 — green.pnpm format,pnpm typecheck,pnpm lint,git diff --check— clean.fix/mobile-cost-and-exit) onlytrufflehogandthe
Kilo Code Reviewcheck trigger —ci.yml,kilo-app-ci.ymlandcodeql.ymlrun onpull_requestevents targetingmainonly. The full suite runs automatically when GitHubretargets this PR to
mainafter fix(mobile): one canonical session cost across list and detail #4777 merges; the local equivalents above are green onthe exact final head.
UNKNOWN/Untitled sessionplaceholders in the Agents list for an account that has them inPostgres; real CLI session and mobile-created session still listed). Results posted in a
follow-up comment.