Skip to content

i3x: bind subscription ownership to the authenticated principal - #711

Open
AlexGodbehere wants to merge 2 commits into
mainfrom
ago/i3x-subscription-ownership
Open

i3x: bind subscription ownership to the authenticated principal#711
AlexGodbehere wants to merge 2 commits into
mainfrom
ago/i3x-subscription-ownership

Conversation

@AlexGodbehere

Copy link
Copy Markdown
Contributor

Binds i3X subscription ownership to the authenticated Factory+ principal instead of the client-supplied clientId.

This is a behavioural change to existing endpoints. The subscription routes can no longer return 403, and a subscription belonging to another principal now returns 404 where it previously returned 403. See Compatibility below.

The problem

SubscriptionManager.getAndVerify enforced ownership like this:

const sub = this.subscriptions.get(subscriptionId);
if (!sub) { /* 404 */ }
if (sub.clientId !== clientId) { /* 403 */ }

Both subscriptionId and clientId arrive from the client — clientId in the request body, subscriptionId in the body or query. The check compares one client-supplied string against another, so it proves nothing.

The takeover scenario in plain terms. Alice creates a subscription and registers half a dozen production assets against it. Her clientId and subscriptionId are visible in her browser's network tab, in any ingress or proxy log that captures request bodies, and in anything that mirrors HTTP traffic. Mallory — who needs credentials good enough to authenticate to the i3X service at all, and no right whatsoever to Alice's data — sends {"clientId": "<Alice's clientId>", "subscriptionId": "<Alice's subscriptionId>"} to:

  • /v1/subscriptions/sync, and reads every value Alice has queued;
  • /v1/subscriptions/stream, and attaches to her live SSE feed;
  • /v1/subscriptions/delete, and destroys it;
  • /v1/subscriptions/register, and adds elements to it, or /unregister and silently blinds her monitor.

Nothing in acs-i3x read req.auth at all before this change (grep -rn "req.auth" acs-i3x/lib returned nothing), even though the shared FplusHttpAuth middleware in lib/js-service-api/lib/auth.js sets it on every route except the public /v1/info.

There was a second, smaller problem in the same three lines. An unknown id returned 404; a known id owned by someone else returned 403. That pair is an existence oracle: any authenticated caller could walk ids and learn which subscriptions are live, without ever being able to read one.

The fix

  • Subscription gains an owner field, set at creation time from req.auth. It is never supplied by the client and never appears on the wire.
  • getAndVerify compares owner against the caller's principal. clientId is no longer consulted for access control.
  • clientId is still stored on the subscription and still echoed in responses, because it is part of the i3X spec shape and clients send it. It just stops being the thing that protects anything.
  • The route handlers thread req.auth into the SubscriptionManager through a small subscription_owner(req) helper in api-v1.ts. All seven call sites were changed: create, list, delete, register, unregister, sync, stream.

404, not 403, for a foreign subscription

A subscription owned by another principal is now reported exactly as one that does not exist: status 404, message Subscription <id> not found. The message no longer names the owner and no longer says "does not belong to". A caller cannot tell "wrong owner" from "no such id", which closes the oracle.

There is precedent for this in the codebase, and it was deliberate there too. acs-directory/lib/api_v1.js, around lines 292-295:

/* We unhelpfully return 404 here to prevent unauthorised clients
 * from discovering which alerts exist. */

Same reasoning, same shape.

What owner is when req.auth is null

subscription_owner returns req.auth as-is, which is undefined only if a request somehow reaches a subscription route without passing FplusHttpAuth. That should be impossible: bin/api.ts marks only /v1/info public, and /v1/info never touches subscriptions. Rather than rely on that, getAndVerify treats a falsy owner as never matching — it fails closed to 404 instead of matching a subscription that happened to be stored with a falsy owner. list() does the same. This is defence in depth on a path that should not exist, and it deliberately introduces no new error status.

Compatibility

Both known consumers were checked by reading their code.

acs-admin — transparent, no change needed.

  • acs-admin/src/store/useMonitorStore.js generates a fresh random clientId per store instance (acs-admin-<uuid>) and holds subscriptionId in the same store, set only from the createSubscription call it just made. The store is not persisted, so it never carries a subscriptionId across a page load and never tries to reattach to a subscription it did not create.
  • Every request goes through client.Fetch.fetch, or a service token resolved by the same service-client for SSE (useI3xClient.js, useI3xSSE.js), so the principal is the logged-in user. One session equals one principal equals one clientId, and ownership-by-principal is a strict superset of what the clientId check allowed.
  • Two tabs, one login: each tab gets its own store, so its own clientId and its own subscriptionId. They never touch each other's. Covered by the test "lets one principal use two different clientIds".
  • Reconnect: _startStream only runs when this.subscriptionId is already set from this session, so there is no reattach-with-a-new-clientId path to break. On reload the store starts empty and creates a fresh subscription; the orphan expires on the server TTL.
  • acs-admin does not branch on HTTP status anywhere in the i3X path. request() throws a plain Error on any non-ok response, and the store's cleanup paths .catch(() => {}). The 403-to-404 change is invisible to it.
  • One widening worth naming: if the same principal ever did learn another of its own subscription ids, it could now reach it, where the clientId mismatch would previously have 403'd. Same human, same credential, so not a privilege boundary — but it is a real change and a reviewer should decide they are happy with it.

External Rust consumer (the nostromo bridge, separate private repo) — this change is what it needs.

  • It sends a single constant clientId of nostromo-bridge with a single credential. One principal, one clientId, so its own subscriptions always match on owner.
  • Its reconnect logic maps only HTTP 404 to "subscription is gone, create a fresh one"; anything else propagates and it backs off and retries. A 403 would make it spin rather than self-heal.
  • Verified that nothing in the subscription path can return 403: grep -rn "403" acs-i3x/lib/ acs-i3x/bin/ now returns nothing at all, anywhere in the service. The only status the subscription routes produce for a missing-or-foreign subscription is 404, with the plain "not found" message the bridge already handles. TTL expiry, deletion by the owner, and a foreign id are now indistinguishable to it, which is exactly what its state machine assumes.

How to test

  1. cd acs-i3x && npm install && npm test — 13 suites, 349 tests, all passing.
  2. Manual takeover check against a running stack, with two principals A and B that can both reach i3X:
    1. As A: POST /v1/subscriptions with {"clientId": "alice-client", "displayName": "test"}. Note the returned subscriptionId.
    2. As A: POST /v1/subscriptions/register with that subscriptionId and a real elementId, then POST /v1/subscriptions/sync. You get values.
    3. As B, sending A's exact clientId and subscriptionId: POST /v1/subscriptions/sync. Expect HTTP 404, Subscription <id> not found. Before this change it returned A's data.
    4. As B: POST /v1/subscriptions/sync with a random UUID as subscriptionId. Expect a response identical in shape and status to step 3, differing only in the id.
    5. As B: POST /v1/subscriptions/delete with A's subscriptionId. Expect a per-id 404 inside the envelope, then confirm as A that the subscription still works.
    6. Repeat step 3 against /v1/subscriptions/stream and /v1/subscriptions/register. All 404, none 403.
  3. acs-admin smoke test: open the monitor dialog, subscribe to a few elements, confirm live values stream in. Open a second tab, subscribe to different elements, confirm both stream independently. Close and reopen.

Tests added

In test/subscriptions.test.ts, a new ownership block:

  • the owner cannot be spoofed — an attacker holding both the subscriptionId and the clientId gets 404 from all eight manager entry points, and the subscription is intact afterwards;
  • a foreign subscription and an unknown id produce the same status and the same message form, and the message leaks neither the owner nor the fact of ownership;
  • no subscription operation returns 403, including with an empty and an undefined owner;
  • two principals sharing one clientId stay separated;
  • one principal using two clientIds works, which is the acs-admin two-tab case;
  • the full single-principal lifecycle: create, register, value change, sync, stream, delete.

The existing "throws 403 for wrong clientId" cases were retitled and now assert 404.

In test/api-v1.test.ts and test/e2e.test.ts, the test apps install a stand-in for FplusHttpAuth that sets req.auth, and the route tests assert the handlers pass the principal rather than the body clientId to the manager. e2e.test.ts deliberately uses a principal string that differs from the clientId the requests carry, so a regression that reverted to the body value would fail rather than coincidentally pass.

Test results

Test Suites: 13 passed, 13 total
Tests:       349 passed, 349 total

Four consecutive full runs, all clean. Nothing unrelated is failing on this branch.

Known pre-existing flake, for whoever reviews: test/e2e.test.ts intermittently fails, roughly one run in four, with a response body arriving empty on one of several supertest requests fired in parallel. It reproduces on unmodified main and was hit by the sibling work on #709. I did not see it in four runs here, and it is unrelated to this change — if CI trips it, it is not this PR.

Noticed, not fixed

  • acs-i3x/docs/to-improve.md row D7 records the old intent, "clientId mismatch should be 403", as Resolved. That row is now historically inaccurate. Left alone because docs hygiene in this service is being handled separately.
  • acs-i3x/docs/2026-04-01-acs-i3x.md line 526 lists APIError(403) → 403 with error envelope in the error mapping. Still true generically, but no subscription route emits 403 any more.
  • SubscriptionManager logs with bare console.log rather than a bound debug logger, and those lines include element ids and values. Out of scope; logging hygiene is in flight elsewhere.
  • SubscriptionManager.stream throws a plain Error with no .status when a subscription already has an active stream, so the envelope will surface it as a 500. Arguably should be a 409. Left alone.

Release notes

i3X subscriptions are now owned by the authenticated Factory+ principal rather than by the clientId in the request body. Previously, any caller who could authenticate to the i3X service and who learned another client's subscriptionId and clientId — both of which travel in plain request bodies — could read that subscription's queued values, attach to its live SSE stream, register or unregister elements on it, or delete it outright. Ownership is now checked against the verified principal, which a client cannot choose.

clientId is unchanged on the wire. Clients still send it, it is still stored and echoed back, and nothing needs to change in what any client sends.

One behavioural change to be aware of: requesting a subscription that belongs to a different principal now returns 404 Not Found where it previously returned 403 Forbidden. This is deliberate. It makes a foreign subscription indistinguishable from one that does not exist, so the response cannot be used to discover which subscription ids are live, and it matches what acs-directory already does for alerts. Any client that treats 404 as "my subscription is gone, create a new one" will self-heal correctly; a client that specifically watched for 403 on these routes will no longer see it.

Subscription ownership was enforced by comparing the clientId in the
request body against the clientId stored on the subscription. Both
sides came from the client, so anyone who learned a subscriptionId
could claim the matching clientId and read the queued values, attach
to the SSE stream, or delete the subscription.

Subscriptions now record the authenticated principal (req.auth, set by
FplusHttpAuth) as their owner, and every access is checked against it.
clientId is still stored and echoed because it is part of the i3X wire
shape, but it no longer protects anything.

A subscription owned by another principal now reports 404, identically
to one that does not exist, so the pair cannot be used to probe which
subscription ids are live. acs-directory does the same for alerts, for
the same reason. Nothing in the subscription routes returns 403 any
more.
Adds an ownership block to the SubscriptionManager tests covering
clientId spoofing, foreign-subscription 404 parity with unknown ids,
the absence of any 403, two principals sharing a clientId, one
principal using two clientIds, and the full single-principal
lifecycle.

The route tests now install a stand-in for FplusHttpAuth so req.auth
is set, and assert that handlers pass the principal rather than the
body clientId to the SubscriptionManager.
@AlexGodbehere

Copy link
Copy Markdown
Contributor Author

Review: i3X subscription ownership is now the authenticated principal, not a client-supplied string

Branch: ago/i3x-subscription-ownership
Worktree: .claude/worktrees/agent-a7a9a277188fb2675
PR: #711

What changed (functional level)

SubscriptionManager decided who owned a subscription by comparing the clientId in the request body against the clientId stored on the subscription. Both came from the client, so the check proved nothing. Anyone who could authenticate to i3X at all and who learned a subscriptionId — from a browser network tab, an ingress log, or any traffic mirror — could send the matching clientId and read the subscription's queued values via /sync, attach to its live SSE stream, register or unregister elements on it, or delete it.

Subscriptions now carry an owner, taken from req.auth at creation time. req.auth is set by the shared FplusHttpAuth middleware and cannot be chosen by the caller. Every access is checked against it. clientId is still stored and echoed because it is part of the i3X wire shape — no client changes what it sends — but it no longer protects anything.

Worked example. Alice creates a subscription as principal alice@REALM with clientId: "acs-admin-9f3c", gets back subscriptionId: "7b2e…", registers a CNC spindle-load metric. Mallory, authenticated as mallory@REALM, posts {"clientId": "acs-admin-9f3c", "subscriptionId": "7b2e…"} to /v1/subscriptions/sync. Before: 200 with Alice's queued values. After: 404, Subscription 7b2e… not found — byte-identical to what Mallory gets for a subscriptionId he invented.

Walkthrough

Ownership

  • Subscription gains owner, set from req.auth. Never client-supplied, never on the wire.
  • getAndVerify checks owner; clientId is out of the access-control path entirely.
  • list() filters on owner too, so bulk listing cannot be used as a side channel.
  • All seven route call sites in api-v1.ts (create, list, delete, register, unregister, sync, stream) thread the principal through a subscription_owner(req) helper.

404 rather than 403

  • A subscription owned by someone else now returns 404 with the plain "not found" message, identical to an unknown id. The old 403 message named the owning client, which was itself a small leak.
  • Previously the 404/403 split was an existence oracle: any authenticated caller could probe which subscription ids were live.
  • Precedent, cited in the PR body: acs-directory/lib/api_v1.js around lines 292-295 does the same thing for alerts, with the comment "We unhelpfully return 404 here to prevent unauthorised clients from discovering which alerts exist."
  • grep -rn "403" acs-i3x/lib/ acs-i3x/bin/ now returns nothing anywhere in the service.

Unauthenticated requests

  • req.auth is undefined only if a request reaches a subscription route without passing FplusHttpAuth. bin/api.ts marks only /v1/info public and that route never touches subscriptions, so it should be unreachable.
  • Rather than rely on that, getAndVerify treats a falsy owner as never matching and fails closed to 404. No new error status, no new code path for callers to hit.

Tests

  • New ownership block in test/subscriptions.test.ts: spoofing the clientId fails across all eight manager entry points and leaves the subscription intact; foreign and unknown ids are indistinguishable in both status and message; no operation returns 403 (including empty and undefined owner); two principals sharing a clientId stay separate; one principal with two clientIds works; full create/register/sync/stream/delete lifecycle.
  • The route tests install a stand-in for FplusHttpAuth and assert the handlers pass the principal, not the body clientId. e2e.test.ts uses a principal string that differs from the request clientId, so a regression to the body value fails rather than coincidentally passing.

How to test

  1. cd acs-i3x && npm install && npm test — 13 suites, 349 tests.
  2. With two principals A and B that can both reach i3X:
    1. As A, POST /v1/subscriptions with {"clientId": "alice-client"}; note the subscriptionId.
    2. As A, register a real elementId and POST /v1/subscriptions/sync — values come back.
    3. As B, send A's exact clientId and subscriptionId to /v1/subscriptions/sync. Expect 404.
    4. As B, send a random UUID as subscriptionId. Expect the same status and message shape as step 3.
    5. As B, POST /v1/subscriptions/delete with A's id. Expect a per-id 404 in the envelope; confirm as A that the subscription still works.
    6. Repeat step 3 against /stream and /register. All 404, none 403.
  3. acs-admin: open the monitor dialog, subscribe to elements, confirm live values. Open a second tab, subscribe to different elements, confirm both stream independently.

Decisions / open questions

  • Signature change rather than an extra argument. Every SubscriptionManager method now takes owner where it took clientId; create takes (owner, clientId, displayName?). Both are strings, so the compiler will not catch a call site that passes the wrong one. There are no other callers in the tree (grep over lib/ and bin/ confirms), but if that bothers a reviewer, an opts object would make it typo-proof at the cost of a wider diff.
  • 404 for the foreign case is the load-bearing decision. It is a behavioural change to a live endpoint. It is also what makes the external Rust bridge self-heal: its reconnect logic maps only 404 to "recreate", and a 403 would have made it back off and spin.
  • Fail-closed on falsy owner is dead code by design. Deliberate rather than accidental; the alternative was to throw 401 from create, which would have been equally dead and would have added an error path.
  • acs-admin widening. One principal can now reach any of its own subscriptions regardless of which clientId created them. Same human, same credential, so not a privilege boundary, but it is a real change and worth a reviewer's nod.
  • Branch was recut from main at bd25a03e after the worktree turned out to be forked from ago/directory-session-retention. git diff --stat main...HEAD touches acs-i3x/ only.

Files of note

Implementation:

  • acs-i3x/lib/subscriptions.tsowner field, getAndVerify rewrite, 403 removed
  • acs-i3x/lib/api-v1.tssubscription_owner(req) helper, seven route call sites

Tests:

  • acs-i3x/test/subscriptions.test.ts — new ownership block, existing 403 cases retitled to 404
  • acs-i3x/test/api-v1.test.ts — auth stand-in, principal-not-clientId assertions, 403 case rewritten as 404
  • acs-i3x/test/e2e.test.ts — auth stand-in, principal deliberately distinct from clientId

Cited but not touched:

  • acs-directory/lib/api_v1.js — the 404-instead-of-403 precedent
  • lib/js-service-api/lib/auth.js — where req.auth comes from
  • acs-admin/src/store/useMonitorStore.js, src/composables/useI3xClient.js, useI3xSSE.js — compatibility check

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant