i3X: stop history reads leaking other devices, and validate GET history time bounds - #709
Merged
Merged
Conversation
Contributor
Author
Review: i3X history could be read for any device, and the GET route did not validate its time boundsBranch: What changed (functional level)Two defects on the i3X read path. Any caller could read any device's history. Since ConfigDB v13 the device object UUID, the originMap The fallback is deleted. The GET history route did not validate its time bounds. WalkthroughHistory reads
Why empty rather than 404
Time bound validation
How to test
Test results
Decisions / open questions
Files of noteFix:
Tests:
Context, unmodified:
|
buildFluxQuery had a fallback for elementIds with no MetricMeta which interpolated the caller's raw elementId into the topLevelInstance tag filter. Since ConfigDB v13 a device's object UUID is also its originMap Instance_UUID and its InfluxDB topLevelInstance tag, so any caller could pass any device UUID and read that device's whole history. queryHistory only guards compositions, and an elementId absent from the object tree has no object at all, so it fell straight through to the fallback. The Influx connection uses the admin token, so nothing downstream stopped it. It was also the only place a caller-supplied string reached the Flux query unvalidated. buildFluxQuery now returns null when there is no MetricMeta and queryHistory returns an empty result set for that case, matching what it already does for composition objects. A leaf with no meta is indistinguishable from an unknown element, so an empty result also avoids leaking whether an element exists.
The GET route took startTime/endTime straight from the query string and passed them into the Flux range() with no validation, while the bulk POST route already required RFC 3339. Apply the same check, with the same message, so the two routes behave identically.
AlexGodbehere
force-pushed
the
ago/i3x-history-hardening
branch
from
August 27, 2026 08:28
9b4687b to
deb79f8
Compare
AlexGodbehere
added a commit
that referenced
this pull request
Aug 27, 2026
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:
```ts
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:
```js
/* 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 `clientId`s 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two defects in the
acs-i3xhistory path. Both are on the read side of the i3X API and both let a caller reach further into InfluxDB than intended.Defect 1: any caller could read any device's history
History.buildFluxQueryhad a fallback branch. When the requestedelementIdhad noMetricMetain the object tree, instead of refusing it built a query filtered on the caller's rawelementId:queryHistoryonly bailed out early when the object was a composition. AnelementIdthat is not in the object tree at all has no object, so the composition guard did not fire and the request went straight into the fallback.That matters because since ConfigDB v13 the device's object UUID, the originMap
Instance_UUIDand the InfluxDBtopLevelInstancetag are all the same value. SoGET /v1/objects/<any-device-uuid>/history?startTime=...&endTime=...returned that device's entire history for the window, for every metric it publishes, regardless of whether the caller had any business with that device. The InfluxDB connection uses the admin token (INFLUX_TOKENfrom theinfluxdb-auth/admin-tokensecret, seedeploy/templates/i3x/i3x.yaml), so nothing further down the stack narrowed it.It was also the one place a caller-supplied string reached the Flux query text without ever being checked.
The fallback is gone.
buildFluxQuerynow requiresMetricMetaand returnsnullwhen there is none.Why an empty result rather than a 404
queryHistoryalready returns[]for composition objects, on the grounds that history over a composition would be a meaningless mix of child metrics. The no-meta case is the same shape of answer: there is no single metric series to return.More importantly, from inside
buildFluxQuerya leaf that genuinely has no meta is indistinguishable from anelementIdthat does not exist at all. If the no-meta case 404'd while a real-but-empty leaf returned[], the response would become an existence oracle over the object tree - a caller could enumerate which UUIDs are real by watching the status code. Returning[]for both keeps the two indistinguishable and keeps the route consistent with the composition behaviour that is already there.A caller who wants to know whether an element exists still has
GET /objects/:elementId, which 404s properly and is the right place for that question.Defect 2: the GET history route did not validate its time bounds
get_object_historytookstartTime/endTimefromreq.queryand passed them intoqueryHistory, where they land inside the Fluxrange(start: ..., stop: ...). It checked only that they were present.The bulk
POST /objects/historyroute in the same file already did this properly withvalidator.isRFC3339. The same check is now on the GET route, with the same error message and the samebadRequestcall, so the two routes reject the same inputs the same way.How to test
cd acs-i3x && npm install && npm test- 13 suites, 348 tests.GET /v1/objects/<device-uuid>/history?startTime=2026-01-01T00:00:00Z&endTime=2026-02-01T00:00:00Z. Before this change you got that device's full history back; now you get{"elementId": "...", "values": []}.topLevelInstance+pathquery is unchanged.GET /v1/objects/<leaf>/history?startTime=-30d)&endTime=now(). You get a 400 withstartTime and endTime must be an RFC 3339 timestamp.POST /v1/objects/historywith the same bad bounds. Same 400, same message.Tests added
In
test/history.test.ts:buildFluxQueryreturnsnullfor an elementId with noMetricMeta, and specifically for a device-shaped UUID.queryHistoryreturns[]and issues no Influx query at all for an unknown elementId -collectRowsis asserted not to have been called, so the query never even reaches the client.In
test/api-v1.test.ts:startTimeand a non-RFC3339endTimewith 400, andqueryHistoryis asserted not to have been called.2026-04-01T00:00:00+01:00), and passes them through unmodified.Test results
Note on flakiness, which predates this branch:
test/e2e.test.tsfails intermittently, roughly one run in four, on a different assertion each time (Content-Type headers,Cross-endpoint consistency,Response envelope compliance). The failures are always a response body arriving empty on one of severalsupertestrequests fired in parallel, on endpoints unrelated to history. I reproduced it on the unmodified base commit as well, so it is a pre-existing test-harness flake rather than anything introduced here. It is not in this PR's scope to fix but somebody should.Noticed, not fixed
getCurrentValuecorrectly requiresMetricMeta, so the same hole never existed on the value path. Worth knowing when reviewing.[VALUE]console.loglines inapi-v1.tslog element ids and values on every request. Out of scope here; I understand logging hygiene is being handled separately.acs-i3x/docs/2026-04-01-acs-i3x.mdstill documents the oldbuildFluxQuerysignature including the fallback. Left alone to avoid colliding with the docs work in flight.Release notes
GET /v1/objects/:elementId/historynow validatesstartTimeandendTime. Both must be RFC 3339 timestamps, matching the existing behaviour ofPOST /v1/objects/history. Malformed bounds now return 400 instead of being passed into the underlying query.