From 4e2a6b8dcfa6e2894fe53e796aeedd344f873955 Mon Sep 17 00:00:00 2001 From: Michael Santoso Date: Fri, 11 Sep 2026 09:12:12 +0700 Subject: [PATCH 1/3] fix(gateway): stop forwarding client-asserted x-user-* identity headers The permission middleware that actually guards the platform reads its subject and roles straight off the request: `@openfoundry/permissions` takes `x-user-id` and `x-user-roles` as given, and `resolvePermissionsForRoles("ADMIN")` then grants everything. The gateway proxy forwarded those headers verbatim, stripping only hop-by-hop entries and `Host`, so the caller supplied its own authorization. Reproduced end to end against the real gateway and svc-ontology with ENFORCE_PERMISSIONS=true, the value deploy/.env.production and deploy/docker-compose.yml both set: no token, no headers -> 403 no token, X-User-Roles: ADMIN -> 201 Created same, reading it back -> 200, data returned no token, X-User-Roles: VIEWER (control) -> 403, so the check is live valid Bearer token, no role headers -> 403 A complete inversion: the authenticated user denied, the anonymous header-spoofer admitted. The gateway is the public edge, so it now drops every `x-user-*` header arriving from a client. This only removes an inbound trust; it grants no one access and does not populate the headers either. Measured against every registered route of the five guarded services, driving each one through the real gateway and recording what requirePermission actually decided (112 of the 118 call sites reachable with synthetic URLs): start.sh config, ordinary client 112 ALLOW -> 112 ALLOW start.sh config, spoofed ADMIN 112 ALLOW -> 112 ALLOW ENFORCE_PERMISSIONS=true, ordinary 112 DENY -> 112 DENY ENFORCE_PERMISSIONS=true, spoofed 112 ALLOW -> 112 DENY No route stops working; one cell changes, and it is the attacker's. Under start.sh the HTTP status distribution is identical before and after. The enforcing-mode denial for ordinary clients already existed, because `request.claims` is never set: authPlugin is registered unencapsulated in server.ts without fastify-plugin, so its onRequest hook applies to no route. Seed and sync scripts are unaffected - they call the service ports directly rather than through the proxy, verified by running seed-healthcare.sh against the patched stack. The console sends only `Authorization: Bearer`. Service tests inject headers straight into the service, not through the gateway. `x-uploaded-by` is deliberately left alone: it is attribution rather than an authorization decision, and nothing else supplies it. This closes the escalation only on the gateway path. Reaching a service port directly still bypasses it, which is why those ports must not be public. --- AGENTS.md | 18 +++++++++ services/svc-gateway/src/proxy.test.ts | 52 ++++++++++++++++++++++++++ services/svc-gateway/src/proxy.ts | 31 ++++++++++++++- 3 files changed, 99 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 9478dd1..d3bcdda 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -55,6 +55,8 @@ own default include globs rather than the root config's `**/tests/**` + `**/src/ Integration tests are `pnpm run test:integration` (config in `tests/vitest.config.ts`). Most files start their own in-process server; `api-endpoints` and `sdk-compat` need a real gateway on `localhost:8080` via `bash start.sh`, which needs Docker Postgres. +`api-endpoints` additionally needs *seeded* data: it reads the first existing ontology rather than creating one, so against an empty store its whole first test yields nothing and 20 tests cascade into "expected undefined to be defined". +Seed with any `scripts/demo/seed-*.sh` before blaming a change. With data seeded it is 36/37, the one failure being Actions apply returning 404 where the test expects 400; that failure predates this note. Suites that start their own server pass `new OntologyStore(null)` / `new ObjectStore(null)` so they stay off the shared file store; a store built with the default path loads `/tmp/openfoundry-data/`, @@ -101,6 +103,22 @@ Without `DATABASE_URL` every service uses its in-memory or `/tmp/openfoundry-dat Dev login is `admin` / `admin123` (see `DEV_USERS` in `services/svc-multipass/src/routes/auth.ts`). +## Identity and permissions + +Two different `requirePermission` exports coexist and only one is reachable. +`services/svc-gateway/src/middleware/rbac.ts` has zero importers repo-wide; the one that actually guards 118 routes is `@openfoundry/permissions`, whose header-based variant reads `x-user-id` / `x-user-roles`. +Those headers are a trusted-hop input, never a client input: the gateway proxy drops every `x-user-*` header so a caller cannot assert its own roles. Do not re-add them to the forwarded set, and do not introduce a second identity header without a hop that sets it from a verified token. +Seed and sync scripts talk to the service ports directly, not through the gateway, so that strip does not reach them - which is also why a service port must never be publicly reachable. + +`request.claims` is never populated: `authPlugin` is registered unencapsulated at `services/svc-gateway/src/server.ts:153` without `fastify-plugin`, so its `onRequest` hook applies to nothing. +Until that is fixed, no token-based permission check enforces anything. `claims.sub` is also a username, not a RID (`svc-multipass/src/routes/auth.ts:122`), and issued tokens carry no `roles` claim at all (`packages/auth-tokens/src/token-creator.ts`). + +Neither model is Foundry's, so do not converge on either as-is. +Foundry grants *roles on a resource* - `POST /v2/filesystem/resources/{resourceRid}/roles/add`, inherited by the container's contents, with principals that may be groups - while roles themselves are platform-defined and read-only over the API (`GET /v2/admin/roles/{roleId}` and `getBatch` only, no create or delete). +Against that, `@openfoundry/permissions` roles are global (an EDITOR may write every ontology in the deployment) and `rbac.ts` is a bare `(subject, resource, permission)` ACL with no role, container, group or owner. +`createPgStoreHook` in `packages/permissions/src/middleware.ts` is the one piece already shaped right: admin bypass, then resource grant, then role fallback. +Tenant isolation is a separate, *mandatory* layer in Foundry (Organizations) and is `org_rid` + RLS here - no permission check delivers it, and none should be described as doing so. + ## Front-end to service sharp edges The console must not send `Content-Type: application/json` on a request with no body: Fastify rejects it with `FST_ERR_CTP_EMPTY_JSON_BODY`, which the gateway reports as a 500. diff --git a/services/svc-gateway/src/proxy.test.ts b/services/svc-gateway/src/proxy.test.ts index 39c6ca8..52a2981 100644 --- a/services/svc-gateway/src/proxy.test.ts +++ b/services/svc-gateway/src/proxy.test.ts @@ -206,6 +206,58 @@ describe("Gateway proxy routing", () => { expect(headers["x-forwarded-for"]).toBeDefined(); }); + it("does not forward client-asserted x-user-* headers to upstream", async () => { + const mockResponse = new Response("{}", { + status: 200, + headers: { "content-type": "application/json" }, + }); + vi.mocked(globalThis.fetch).mockResolvedValueOnce(mockResponse); + + const app = await createServer({ config: TEST_CONFIG }); + await app.inject({ + method: "GET", + url: "/api/v2/ontologies", + headers: { + // Downstream, requirePermission("ontology:read") would honour these. + "x-user-id": "mallory", + "x-user-roles": "ADMIN", + }, + }); + + const [, fetchOptions] = vi.mocked(globalThis.fetch).mock.calls[0]; + const headers = (fetchOptions as RequestInit).headers as Record; + expect(headers["x-user-id"]).toBeUndefined(); + expect(headers["x-user-roles"]).toBeUndefined(); + }); + + it("strips x-user-* regardless of header casing, and keeps other headers", async () => { + const mockResponse = new Response("{}", { + status: 200, + headers: { "content-type": "application/json" }, + }); + vi.mocked(globalThis.fetch).mockResolvedValueOnce(mockResponse); + + const app = await createServer({ config: TEST_CONFIG }); + await app.inject({ + method: "GET", + url: "/api/v2/ontologies", + headers: { + "X-User-Roles": "ADMIN", + "X-User-Groups": "platform-admins", + authorization: "Bearer token-123", + "x-uploaded-by": "alice", + }, + }); + + const [, fetchOptions] = vi.mocked(globalThis.fetch).mock.calls[0]; + const headers = (fetchOptions as RequestInit).headers as Record; + const names = Object.keys(headers).map((k) => k.toLowerCase()); + expect(names.some((n) => n.startsWith("x-user-"))).toBe(false); + // Everything the caller is entitled to assert still goes through. + expect(headers["authorization"]).toBe("Bearer token-123"); + expect(headers["x-uploaded-by"]).toBe("alice"); + }); + it("strips upstream hop-by-hop headers from response", async () => { const mockResponse = new Response("{}", { status: 200, diff --git a/services/svc-gateway/src/proxy.ts b/services/svc-gateway/src/proxy.ts index 2672c15..6f1513d 100644 --- a/services/svc-gateway/src/proxy.ts +++ b/services/svc-gateway/src/proxy.ts @@ -15,6 +15,29 @@ const HOP_BY_HOP_HEADERS = new Set([ "upgrade", ]); +// --------------------------------------------------------------------------- +// Client-asserted identity headers +// --------------------------------------------------------------------------- + +/** + * Headers by which a caller asserts *who it is* rather than *what it wants*. + * + * Downstream services treat these as trusted: the permission middleware in + * `@openfoundry/permissions` reads `x-user-roles` and grants whatever those + * roles imply. They are therefore only ever safe when a trusted hop sets them + * from a verified token, and never safe to forward from the public edge. + * + * The gateway is that edge, so it drops every `x-user-*` header arriving from + * a client. Nothing legitimate sends them today: the console authenticates + * with `Authorization: Bearer`, and the seed and sync scripts talk to the + * service ports directly rather than through this proxy. + * + * This only removes an inbound trust. It does not grant anyone access, and it + * does not populate the headers either - see the note in `middleware/auth.ts` + * about `request.claims` never being set. + */ +const CLIENT_ASSERTED_IDENTITY_PREFIX = "x-user-"; + // --------------------------------------------------------------------------- // Proxy // --------------------------------------------------------------------------- @@ -26,6 +49,7 @@ const HOP_BY_HOP_HEADERS = new Set([ * Behaviour: * - Preserves method, path, query string, headers, and body. * - Strips hop-by-hop headers from both the outgoing and incoming directions. + * - Strips client-asserted `x-user-*` identity headers from the outgoing direction. * - Injects `X-Forwarded-For` and `X-Request-Id` headers. * - Returns 502 Bad Gateway when the target is unreachable. */ @@ -40,8 +64,11 @@ export async function proxyRequest( // Build outgoing headers, stripping hop-by-hop entries. const outgoingHeaders: Record = {}; for (const [key, value] of Object.entries(request.headers)) { - if (HOP_BY_HOP_HEADERS.has(key.toLowerCase())) continue; - if (key.toLowerCase() === "host") continue; // let fetch set the correct Host + const lowerKey = key.toLowerCase(); + if (HOP_BY_HOP_HEADERS.has(lowerKey)) continue; + if (lowerKey === "host") continue; // let fetch set the correct Host + // Never forward a caller's own claim about its identity or roles. + if (lowerKey.startsWith(CLIENT_ASSERTED_IDENTITY_PREFIX)) continue; if (value !== undefined) { outgoingHeaders[key] = Array.isArray(value) ? value.join(", ") : value; } From 41a2041bebc7e906a5e07a826e9f1886530acce0 Mon Sep 17 00:00:00 2001 From: Michael Santoso Date: Fri, 11 Sep 2026 09:19:03 +0700 Subject: [PATCH 2/3] no-mistakes(review): document known consequences of x-user-* header strip --- AGENTS.md | 2 ++ services/svc-gateway/src/proxy.ts | 8 ++++++++ 2 files changed, 10 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index d3bcdda..63e63b4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -109,6 +109,8 @@ Two different `requirePermission` exports coexist and only one is reachable. `services/svc-gateway/src/middleware/rbac.ts` has zero importers repo-wide; the one that actually guards 118 routes is `@openfoundry/permissions`, whose header-based variant reads `x-user-id` / `x-user-roles`. Those headers are a trusted-hop input, never a client input: the gateway proxy drops every `x-user-*` header so a caller cannot assert its own roles. Do not re-add them to the forwarded set, and do not introduce a second identity header without a hop that sets it from a verified token. Seed and sync scripts talk to the service ports directly, not through the gateway, so that strip does not reach them - which is also why a service port must never be publicly reachable. +Role enforcement is still absent after that strip: what it removed was a role claim any caller could forge, not working enforcement, and nothing yet sets those headers from a verified token. +Until a trusted hop does, `ENFORCE_PERMISSIONS=true` makes every gateway-proxied route answer 403, because the permission hook sees neither `x-user-id` nor `request.claims`. `request.claims` is never populated: `authPlugin` is registered unencapsulated at `services/svc-gateway/src/server.ts:153` without `fastify-plugin`, so its `onRequest` hook applies to nothing. Until that is fixed, no token-based permission check enforces anything. `claims.sub` is also a username, not a RID (`svc-multipass/src/routes/auth.ts:122`), and issued tokens carry no `roles` claim at all (`packages/auth-tokens/src/token-creator.ts`). diff --git a/services/svc-gateway/src/proxy.ts b/services/svc-gateway/src/proxy.ts index 6f1513d..0fd51ed 100644 --- a/services/svc-gateway/src/proxy.ts +++ b/services/svc-gateway/src/proxy.ts @@ -35,6 +35,14 @@ const HOP_BY_HOP_HEADERS = new Set([ * This only removes an inbound trust. It does not grant anyone access, and it * does not populate the headers either - see the note in `middleware/auth.ts` * about `request.claims` never being set. + * + * Known consequence: role-based enforcement does NOT work after this change. + * What was removed is fake enforcement - a client could assert its own roles - + * and real enforcement waits on the gateway task that validates JWTs and sets + * these headers from validated claims. Until that trusted hop exists, running + * with `ENFORCE_PERMISSIONS=true` makes every gateway-proxied route answer 403, + * because the permission hook sees neither `x-user-id` nor `request.claims`. + * That is known and accepted, not an oversight. */ const CLIENT_ASSERTED_IDENTITY_PREFIX = "x-user-"; From a474bb909bace1990ce30d8a12a7439a1d7d67fc Mon Sep 17 00:00:00 2001 From: Michael Santoso Date: Fri, 11 Sep 2026 09:26:26 +0700 Subject: [PATCH 3/3] no-mistakes(document): fix stale identity-header doc pointers and deploy config notes --- deploy/.env.production | 3 +++ deploy/docker-compose.yml | 2 ++ services/svc-gateway/src/proxy.ts | 4 ++-- 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/deploy/.env.production b/deploy/.env.production index c84324d..7b3a714 100644 --- a/deploy/.env.production +++ b/deploy/.env.production @@ -53,4 +53,7 @@ TRACING_ENABLED=false TRACING_ENDPOINT= # --- Permissions -------------------------------------------------------------- +# The gateway strips client-asserted `x-user-*` headers and nothing yet sets them +# from a verified token, so `true` makes guarded gateway-proxied routes answer 403. +# See the identity-header note in services/svc-gateway/src/proxy.ts. ENFORCE_PERMISSIONS=true diff --git a/deploy/docker-compose.yml b/deploy/docker-compose.yml index f649b19..7dacab3 100644 --- a/deploy/docker-compose.yml +++ b/deploy/docker-compose.yml @@ -30,6 +30,8 @@ x-service-env: &service-env LOG_LEVEL: ${LOG_LEVEL:-warn} LOG_FORMAT: json NODE_ENV: production + # Guarded gateway-proxied routes answer 403 while this is "true": see the + # identity-header note in services/svc-gateway/src/proxy.ts. ENFORCE_PERMISSIONS: "true" S3_ENDPOINT: http://minio:9000 S3_ACCESS_KEY: ${MINIO_ROOT_USER} diff --git a/services/svc-gateway/src/proxy.ts b/services/svc-gateway/src/proxy.ts index 0fd51ed..2153b22 100644 --- a/services/svc-gateway/src/proxy.ts +++ b/services/svc-gateway/src/proxy.ts @@ -33,8 +33,8 @@ const HOP_BY_HOP_HEADERS = new Set([ * service ports directly rather than through this proxy. * * This only removes an inbound trust. It does not grant anyone access, and it - * does not populate the headers either - see the note in `middleware/auth.ts` - * about `request.claims` never being set. + * does not populate the headers either; `request.claims` is never set either, + * because `authPlugin` is registered unencapsulated in `server.ts`. * * Known consequence: role-based enforcement does NOT work after this change. * What was removed is fake enforcement - a client could assert its own roles -