Skip to content

finding: the external-datasource federation HTTP family mounts outside the enforceAuth seam with no guard of its own — read and write #9686

Closed

Description

@os-project-manager

Found while implementing #9391 (the datasource-admin authentication guard). Filed rather than fixed there: it is a different registrar in a different package, and #9391's scope was ruled to be exactly the datasource-admin family.

Disclosure posture — inherited deliberately

⛔ No reproduction here, in any PR body, or in any commit message. #9391 was revised on 2026-08-17 to remove its request/response table after the maintainer deleted the public reproduction; that decision travels with adjacent work. What follows is the cause, the source location, and the contract a fix would restore — which is what someone fixing it needs.

What was observed

registerExternalDatasourceRoutes (packages/rest/src/external-datasource-routes.ts) mounts five routes under /api/v1/datasources/:name/external/* and performs no authentication check. Read at source: the module imports nothing auth-related, and none of the five handlers consults an identity before dispatching.

The five, by disposition:

route kind
GET .../external/tables read — remote-table introspection
POST .../external/tables/:remote/draft read — introspect + type-map, no persistence
POST .../external/tables/:remote/import write — creates a live runtime-origin federated object
POST .../external/refresh-catalog write — refreshes the cached catalog snapshot
POST .../external/validate read — validates the federated objects

Cause — the same shape #9391 had, one package over

These routes are composed by mountAndRecordDirectRoutes (packages/rest/src/direct-mount-composition.ts), which mounts two registrars. That composition resolves the RestServer's own execution-context resolver and passes it to one of them:

  • registerPackageRoutes receives resolveExecutionContext and applies the shared anonymous floor (shouldDenyAnonymous over the resolved context, failing closed when the resolver is absent — packages/rest/src/package-routes.ts);
  • registerExternalDatasourceRoutes receives no resolver and contains no check.

Being composed by RestServer is not itself a guard: enforceAuth is a private method invoked inside RestServer's own handlers, not a middleware a direct mount passes through. That is the same reason the datasource-admin family needed its own line, and the same reason registerPackageRoutes grew one.

Why this is worth a card of its own, not a footnote on #9391

Two of these five routes are the declared twins of routes #9391 just guarded: GET .../external/tables and GET /api/v1/datasources/:name/remote-tables reach the same IExternalDatasourceService.listRemoteTables, and POST .../external/tables/:remote/draft and POST /api/v1/datasources/:name/object-draft reach the same generateObjectDraft. The equivalence is deliberate and pinned (packages/rest/src/remote-tables-twin.equivalence.test.ts; #4249 gave the two spellings one failure contract, #7955 one request shape). After #9391 lands, one operation is reachable at one spelling only with authentication and at the other without — so the family's own equivalence principle now argues for closing this, independently of the exposure.

Unlike the admin spelling, this family is SDK-expressed (datasources.external.* on ObjectStackClient, per the REST route ledger), so a fix should be checked against those callers presenting a credential.

What a fix would look like (a lead, not a decision)

Hand the federation registrar the same resolveExecutionContext the composition already resolves for its sibling, and apply the same shared anonymous floor — decision from shouldDenyAnonymous, identity from the resolver, envelope from this surface's sendError, fail-closed when unwired. Whether these routes should require a capability beyond authentication is a separate question of the kind #9593 answers for the admin family, and should not be folded in.

Not triaged here: the other direct-mount registrars vary by design (service-settings gates inside the service, service-storage gates uploads and parent-governed downloads, service-i18n serves locale bundles that may legitimately be pre-login). Each needs its own reading; this card is only about the federation family, where the read is unambiguous.

Backlinks: #9391 (the sibling family, guarded), #7744 (added this area's route ledgers without a guard), #9593 (the capability question for the admin family). Unassigned and unqueued — for the triage seat to level and route.

Activity

  1. os-project-manager commented on Aug 18, 2026

    @os-project-manager
    CollaboratorAuthor

    ⛔ Graded priority:p0 + pm:queue + domain:services + target:v17, claimed, and dispatching immediately

    PM seat os-project-manager, session session_01Y26DJEHSBhhAQ6wwfsHNza, branch claude/issue-9686-federation-routes-auth-guard. Filed by my own dev off #9391 twenty minutes ago; no prior claim.

    Why I am levelling this rather than waiting for triage

    ⚠️ I do not normally set grades — domain:* and finding grades have one producer, and a PM seat "correcting" a grade is the drift the single-producer rule exists to stop. I refused to relabel #9691 on exactly that ground an hour ago. Three things make this different, and I want them on the record rather than assumed:

    1. There is no grade to override. This card was filed unlevelled. Making the first call is not contradicting a producer's judgment.
    2. The class already has a maintainer-endorsed precedent. The datasource-admin HTTP family is reachable unauthenticated, read and write — mounts outside the enforceAuth seam #9391 — the same defect, one package over — was set priority:p0 by the wave PM "under the maintainer's instruction to decide this card's filing shape", on the reasoning that an unauthenticated write path on a published surface is release-blocking by any reading of the release-blocker test. This card is that card's twin. Applying a settled precedent is not inventing a grade.
    3. ⭐ The cost of waiting is measured, not hypothetical. The datasource-admin HTTP family is reachable unauthenticated, read and write — mounts outside the enforceAuth seam #9391 sat pm:queue/p0 for ~16.5 hours — ~10.5h of it named by the triage seat as its own two missed Routine shifts. POST .../external/tables/:remote/import creates a live runtime-origin federated object and is reachable with no identity. I am not putting that behind another Routine cycle to preserve a procedural nicety.

    Triage may re-level, exactly as #9391's filer invited. If this seat has the class wrong, say so and I will follow it.

    The second reason to close this, independent of the exposure

    ⭐ The equivalence argument in this card is the part that makes it un-shelvable, and it gets stronger the moment #9391's fix lands (PR #9695, flipped and armed at 15:43Z):

    • GET .../external/tables and GET /api/v1/datasources/:name/remote-tables reach the same listRemoteTables
    • POST .../external/tables/:remote/draft and POST /api/v1/datasources/:name/object-draft reach the same generateObjectDraft

    That equivalence is deliberate and pinned — #4249 gave the two spellings one failure contract, #7955 one request shape, and remote-tables-twin.equivalence.test.ts holds them to it. So once #9695 merges, one operation answers 401 at one spelling and serves anonymously at the other. The family's own stated principle then argues for closing this, with no reference to severity at all. Leaving it open leaves a pinned equivalence quietly false.

    ⚠️ And the difference from the admin family that a fix must respect: this one is SDK-expressed (datasources.external.* on ObjectStackClient), so the guard has to be checked against those callers presenting a credential — not only against an anonymous refusal.

    Scope fence, inherited

    ⛔ Authentication only. Whether these routes need a capability beyond authentication is the same question #9593 answers for the admin family; it is not folded in here. ⛔ And the disclosure posture travels: no reproduction in the PR body, in any commit message, or in any code comment.

    ⛔ Also out of scope: the other direct-mount registrars. service-settings gates inside the service, service-storage gates uploads and parent-governed downloads, service-i18n serves locale bundles that may legitimately be pre-login. Each needs its own reading. This card is the federation family, where the read is unambiguous.


    Generated by Claude Code

  2. os-project-manager commented on Aug 18, 2026

    @os-project-manager
    CollaboratorAuthor
    {
      "issue": 9686,
      "status": "done",
      "branch": "claude/issue-9686-federation-routes-auth-guard",
      "pr": "https://github.com/objectstack-ai/objectstack/pull/9783",
      "premise_still_valid": true,
      "summary": "Re-verified on main @ 6cb88d9f2 before editing: external-datasource-routes.ts imported nothing auth-related and none of the five handlers consulted an identity, while direct-mount-composition.ts passed resolveExecutionContext to registerPackageRoutes only. No guard had appeared and the composition was unrestructured, so the premise held exactly as filed. Took the card's own lead rather than the admin family's local resolution: the federation registrar now receives the SAME resolver the composition already resolves for its sibling, and applies the shared anonymous floor (shouldDenyAnonymous, ANONYMOUS_DENY_* semantics, this surface's sendError) as the first statement of every handler, before the service lookup. Measured reasons the threaded resolver beat resolving identity locally: it reaches resolveAuthzContext, so it admits both credential kinds (session AND sys_api_key) with nothing re-derived; it stays multi-tenant-correct (it resolves the request's environment and looks auth up in the right kernel, which a PluginContext-local lookup would not); and it adds no service lookup, so check:slot-lookup reports 'none new'. Fails closed on a sync throw, a rejected promise, or no identity. Scope held to authentication — no capability check (#9593's question), and the composition mounts exactly two registrars, so no sixth unguarded family exists there to file. One existing pin changed meaning and says so in a comment: direct-mount-base-follows-apipath.test.ts asserted 200 for an anonymous caller on a real plugin boot, two lines below the package route's 401 — that literal asymmetry now reads 401.",
      "tests": "All evidence at HEAD = df2961b61 (quoted from the runs at that head). REVERSE VERIFICATION, prediction recorded before running: ablate the two implementation files to origin/main via `git restore --source=origin/main` (tree-only, index untouched — porcelain showed ' M', not 'MM'), keep every test, expect the anonymous halves red and the entitled halves green, 7 tests over 4 files. Observed exactly that: 'Test Files 4 failed | 125 passed (129) / Tests 7 failed | 2098 passed (2105)' — so 2098 pre-existing tests stayed green under the unfixed state. The 7: three anonymous cases in the new pin, two new twin-admission cases, the 401-envelope case, the apipath discovery probe. The pin's two entitled cases PASSED under ablation, as predicted (an unguarded route serves a credentialed caller too — which is why a one-sided pin could not tell this fix from a broken feature). Restored with `git restore --source=HEAD`, porcelain clean (byte-identical), then FIXED TREE: `pnpm --filter @objectstack/rest test` → 'Test Files 129 passed (129) / Tests 2105 passed (2105)'; `pnpm --filter @objectstack/rest typecheck` → clean. The new pin boots createRestApiPlugin(...).start(ctx) — the production composition — and asserts status AND error.code UNAUTHENTICATED on all five routes, the service spies uncalled AND the 'external-datasource' lookup never performed, plus an entitled caller getting 200/200/201/200/200 on the same routes in the same run, by session and by sys_api_key. No test resolves the code under test from dist (rest's vitest aliases plugin-hono-server and service-datasource to source; deps built via `pnpm --filter '@objectstack/rest^...' build`, upstream direction). GATE UNION re-derived off `git merge-base` after the final commit (`node scripts/pm/dispatch-gates.mjs`, same 10 families, no additions) and re-run at df2961b61, all exit 0: check:nul-bytes, check:route-envelope, check:cross-package-test-inputs, check:dispatcher-error-vocabulary, check:changeset-gate-self-tests, check:objectui-changeset, check:query-options-erasure, check:engine-double-contract, check:where-matcher, check:type-check-coverage, check:type-check-debt, check-adr-0087-registration.mjs, check-changeset-no-major.mjs, check-empty-changeset.mjs, check-cross-package-test-inputs.mjs, docs-audit/check-affected-docs.mjs. Plus `pnpm check:slot-lookup` explicitly (#9721): 'ratchet holds: 107 unswept site(s) in 25 file(s), none new'. check:type-check-debt did NOT pass first time and is the round it saved: packages/rest excludes *.test.ts from its tsconfig, so its own typecheck never reads its tests, and the ratchet measured @objectstack/rest at 156 against a frozen 155 — the +1 was mine (ResolveAuthzInput.ql is a required member the twin fixture's resolver omitted). Fixed at the error, never the ledger; it now measures 155 and the gate reports 'surplus: none — every entry sits exactly at its measurement'. Running it required the full closure build (`turbo run build --filter='./packages/*' --filter='./packages/*/*'`, 70 tasks) — its refusal on an unbuilt closure is 'not measured', not 'not applicable'. Every heavy step ran under flock /tmp/os-heavy-verify.lock.",
      "open_questions": [],
      "out_of_scope_findings": []
    }

    Notes for the PM, outside the JSON

    • The killed agent's work was recoverable and is preserved. The stale worktree still held its uncommitted edits (5 modified files + 1 new test). Rather than discard them, they are committed on the local branch salvage/issue-9686-prior (98d63743d, local to this container, never pushed). This PR was re-derived from current main independently; nothing was carried over unverified.
    • No sixth unguarded family to file. mountAndRecordDirectRoutes mounts exactly two registrars, and both now receive the caller's identity. The registrars the card fenced off (service-settings, service-storage, service-i18n) mount elsewhere and were not read.
    • Disclosure posture held: no reproduction in the PR body, any commit message, or any code comment.

    Generated by Claude Code


    Generated by Claude Code

  3. os-project-manager commented on Aug 19, 2026

    @os-project-manager
    CollaboratorAuthor

    Landed and verified on main — PR #9783 merged as 185c7bd51. With #9391 (#9695) earlier today, both unauthenticated-write families are closed.

    probe control (pre-merge) observed
    shouldDenyAnonymous in external-datasource-routes.ts 0 3 ✅
    the registerExternalDatasourceRoutes( call site (server, ctx, versionedBase) — no resolver (server, ctx, versionedBase, { resolveExecutionContext }) ✅
    the anonymous discovery probe in direct-mount-base-follows-apipath.test.ts asserted 200 asserts 401 ✅

    ⚠️ A note on the middle row, because I got it wrong first. My armed probe was "grep -c resolveExecutionContext in direct-mount-composition.ts should be ≥2" — but the pre-change baseline was already 3, so that probe would have passed on unlanded code. A probe whose expected value is already satisfied before the change is not a probe. Re-derived against the measured control — line :137 called the federation registrar without the resolver while :117 called its sibling with it — and the discriminating check is the call site, not the file's total count.

    ⭐ The anti-control is the row that matters, and the fix to it is better than a flipped number

    The old line asserted 200 for an anonymous caller. The new one asserts 401, and carries this:

    [#9686] … Reading 200 here was the asymmetry #9686 closed: one composition, two registrars, one of them handed the caller's identity. The base-placement subject of this pin is unchanged — a routing miss still fails the toBeDefined() above.

    Two things right about that. It records what the line used to claim and why that was wrong, so nobody re-derives the old expectation from first principles. And it protects the pin's actual subject — this test exists to prove the advertised URL is mounted at the moved base, and the status code is incidental to that. A careless fix would have changed a number and left a reader unable to tell whether the pin still tests what its name says.

    That line is the durable finding from this card: the hole was not untested, it was written down as an expectation. A test asserting 200 looks exactly like every other passing test, and it supplies the "someone checked this" feeling that stops anyone looking again — which is why this outlived a route ledger that described the posture in prose ("mounted, working, and guarded by nothing").

    The equivalence is enforced now, not assumed

    remote-tables-twin.equivalence.test.ts compares the twins on the admission axis alongside the request-shape axis — no credential, a credential the deployment does not admit, and one it does. Since #9695 landed, one operation answered 401 at one spelling and served anonymously at the other; that is true again, and a guard added to one spelling and not the other now fails there, whichever side it is added to.

    What the fix did that the card only hinted at

    The card's lead was to hand the federation registrar the resolver the composition already had. The PR did that at the composition, so a registrar added later inherits the wiring — and its reasoning is the part to keep: there is no reading of that composition under which one direct mount needs the caller's identity and its neighbour does not.

    Three measured consequences of threading the existing resolver rather than resolving identity locally: it reaches resolveAuthzContext, so it admits both a session and a sys_api_key — mandatory here, since this family is SDK-expressed; it stays multi-tenant-correct, resolving the request's environment and looking auth up in the right kernel; and it adds no service lookup, so check:slot-lookup reported "none new". That last one is the gate that took #9695 red — avoided here by design rather than repaired after the fact.

    2098 pre-existing tests stayed green under the unfixed state.

    #9593 (whether these routes need a capability beyond authentication) remains the separate question, deliberately not folded in.


    Generated by Claude Code

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

Metadata

Metadata

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions