fix(security): the external-datasource federation HTTP family requires an authenticated caller (#9686)

.changeset/external-datasource-federation-auth-floor.md

@@ -0,0 +1,60 @@
+---
+"@objectstack/rest": patch
+---
+
+fix(security): the external-datasource federation HTTP family requires an authenticated caller, on every route (#9686)
+
+<!-- adr-0087: not-required (no-migration-prescription) The change is an
+authentication floor on five mounted HTTP routes plus the composition edge that
+feeds it the caller's identity. No authorable metadata key is added, renamed,
+retired or tombstoned, and no stored shape changes, so there is no conversion to
+register. The behavioural change is that `/api/v1/datasources/:name/external/*`
+now answers `401 UNAUTHENTICATED` to a caller with no resolvable identity, where
+it previously served every route — including the two that write. -->
+
+`registerExternalDatasourceRoutes` mounts the five federation routes
+(`GET .../external/tables`, `POST .../external/tables/:remote/draft`,
+`POST .../external/tables/:remote/import`, `POST .../external/refresh-catalog`,
+`POST .../external/validate`) straight onto `IHttpServer`, so they pass through
+none of the seams that produce the platform's 401s: `RestServer.enforceAuth` is
+a private method invoked inside that server's own handlers — not middleware a
+direct mount is routed through — and the dispatcher domains' floor runs inside
+the dispatcher. Being composed by `RestServer` was not itself a guard.
+
+**The missing piece was an edge in the composition, not a line in a handler.**
+`mountAndRecordDirectRoutes` resolves the `RestServer`'s execution-context
+resolver and handed it to ONE of the two registrars it mounts:
+`registerPackageRoutes` got the identity and applied the shared anonymous floor,
+`registerExternalDatasourceRoutes` got nothing and checked nothing. The resolver
+now reaches both, and the federation registrar applies the same floor:
+
+- the **decision** is `shouldDenyAnonymous` (`@objectstack/core`), the one
+ function every HTTP seam on the platform shares — `isSystem` is not settable
+ from the wire and a CORS `OPTIONS` preflight passes, both by its construction;
+- the **identity** is the `RestServer`'s own resolver, which admits every
+ credential kind the platform admits — a better-auth session *and* a
+ `sys_api_key`. This family is SDK-expressed (`datasources.external.*` on
+ `ObjectStackClient`), so a floor that read only a session would have refused
+ callers the rest of the surface accepts;
+- it **fails closed**: anything that throws, and anything resolving to no
+ identity, is refused. No configuration, posture or absent service opens it;
+- the check runs **before** the service lookup, so an anonymous caller cannot
+ learn from a `503` which services a deployment has wired — and, on the two
+ routes that change state, the refusal provably precedes the write;
+- the 401 is written through this surface's shared `sendError`, so the status,
+ code and message are the platform's while the envelope stays this family's.
+
+**A pinned equivalence is restored, not merely an exposure closed.**
+`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`. #4249 gave those two spellings one failure contract and
+#7955 one request shape. After the datasource-admin family grew its own floor
+(#9391), one operation answered 401 at one spelling and served anonymously at
+the other. `remote-tables-twin.equivalence.test.ts` now compares the two on the
+admission axis as well, so a guard added to one spelling and not the other fails
+whichever side it is added to.
+
+Authentication and nothing more: whether these routes should further require a
+capability is the separately-ruled question #9593 asks of the admin family, and
+is deliberately not folded in here.

packages/rest/src/direct-mount-base-follows-apipath.test.ts

@@ -238,7 +238,15 @@ describe('#6306 — with `apiPath` set, the direct-mount routes follow it', () =
 
  const ext = resolveRoute(table, 'GET', `${discovery.routes.datasources}/pg_main/external/tables`);
  expect(ext, 'the advertised datasources base must be the base of the mounted family').toBeDefined();
- expect((await drive(ext!)).statusCode).toBe(200);
+ // [#9686] The federation family now carries the same anonymous floor as the
+ // package route above, wired from the same composition and the same
+ // resolver — so this boot, which has no auth service in its ctx, answers
+ // 401 here for the same reason it does two lines up. 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. The
+ // gate itself is pinned in `external-datasource-routes-auth-guard.test.ts`.
+ expect((await drive(ext!)).statusCode).toBe(401);
  });
 });
 

packages/rest/src/direct-mount-composition.ts

@@ -61,9 +61,18 @@ export interface DirectMountComposition {
  /** The `protocol` slice the package routes read registry packages through. */
  protocol?: PackageRoutesOptions['protocol'];
  /**
- * [#7033 / #7023] Resolves the caller's execution context for the package
- * routes' authorization gate — the `RestServer`'s own resolver, so the
- * capability check reads the same identity the rest of the surface does.
+ * [#7033 / #7023] Resolves the caller's execution context for the direct-
+ * mount gates — the `RestServer`'s own resolver, so the checks read the
+ * same identity the rest of the surface does.
+ *
+ * [#9686] Handed to BOTH registrars this step mounts. It used to reach only
+ * `registerPackageRoutes`, and that asymmetry was the whole of the
+ * federation family's exposure: one registrar got the identity and applied
+ * the shared anonymous floor while the other got nothing and checked
+ * nothing — including on the two routes that write. There is no reading of
+ * this composition under which one direct mount needs the caller's identity
+ * and its neighbour does not, so the resolver is passed to every registrar
+ * mounted here, and a registrar added later inherits the same wiring.
  */
  resolveExecutionContext?: PackageRoutesOptions['resolveExecutionContext'];
  /** ADR-0006 project scoping — mirrors the package routes under the scoped base. */
@@ -134,7 +143,11 @@ export function mountAndRecordDirectRoutes(composition: DirectMountComposition):
  // `@objectstack/datasource-admin` package, which registers its own.
  try {
  recorder.recordDirectMountedRoutes(
- registerExternalDatasourceRoutes(server, ctx, versionedBase),
+ // [#9686] The resolver is the SAME one the package registrar above
+ // receives — this family's anonymous floor reads the identity the
+ // rest of the surface reads, and a deployment that wires no
+ // resolver refuses rather than serves (the registrar fails closed).
+ registerExternalDatasourceRoutes(server, ctx, versionedBase, { resolveExecutionContext }),
  );
  ctx.logger.info('Datasource federation routes registered');
  } catch (e: any) {

packages/rest/src/external-datasource-envelope.conformance.test.ts

@@ -44,7 +44,28 @@ interface Captured {
  body: any;
 }
 
-function mount(svc: unknown) {
+/**
+ * [#9686] The family requires an authenticated caller, so every case in this
+ * file — whose subject is the ENVELOPE of the success / 400 / 503 arms — mounts
+ * with a resolver standing in for a credentialed one. Without it each case
+ * would read the 401 body instead of the arm it names, and this file would
+ * silently stop measuring what it exists to measure.
+ *
+ * The guard itself is pinned in `external-datasource-routes-auth-guard.test.ts`;
+ * the 401's own envelope is the last case below, which is this file's business.
+ */
+const CREDENTIALED = async () => ({ userId: 'u_env_conformance' });
+
+/**
+ * A resolver that RESOLVES, and resolves to no identity — the anonymous case as
+ * the production resolver expresses it. Spelled as its own constant because
+ * passing `undefined` for the parameter below would take the default above:
+ * "no argument" and "no identity" are different facts, and only one of them is
+ * what the 401 case means to drive.
+ */
+const ANONYMOUS = async () => undefined;
+
+function mount(svc: unknown, resolveExecutionContext: any = CREDENTIALED) {
  const routes = new Map<string, RouteHandler>();
  const server = {
  get: (p: string, h: RouteHandler) => { routes.set(`GET:${p}`, h); },
@@ -57,7 +78,7 @@ function mount(svc: unknown) {
  close: async () => {},
  } as unknown as IHttpServer;
  const ctx = { getService: vi.fn().mockReturnValue(svc) } as any;
- registerExternalDatasourceRoutes(server, ctx, '/api/v1');
+ registerExternalDatasourceRoutes(server, ctx, '/api/v1', { resolveExecutionContext });
  return routes;
 }
 
@@ -301,3 +322,20 @@ describe('external-datasource envelope (#3843) — error bodies', () => {
  }
  });
 });
+
+describe('[#9686] the anonymous refusal is written in the same declared envelope', () => {
+ it('an unauthenticated caller gets 401 { success: false, error: { code } }, not a hand-written body', async () => {
+ // The guard added a body to this surface, and a new body is exactly where
+ // an envelope drifts. Same assertions the arms above make, on the arm the
+ // authentication floor produces.
+ const routes = mount({ listRemoteTables: async () => [{ name: 'customers' }] }, ANONYMOUS);
+ const { status, body } = await drive(routes, 'GET', `${EXT}/tables`);
+
+ expect(status).toBe(401);
+ expect(BaseResponseSchema.safeParse(body).success, `body is not a BaseResponse: ${JSON.stringify(body)}`).toBe(true);
+ expect(envelopeViolations(body), `not the declared envelope: ${JSON.stringify(body)}`).toEqual([]);
+ expect(body.success).toBe(false);
+ expect(body.error?.code).toBe('UNAUTHENTICATED');
+ expect(body.data).toBeUndefined();
+ });
+});