History

Address review: exclude OAuth callback from platform reads, skip catalog sync on the platform view, discriminate the principal union

Rhys Sullivan committed 3 months ago

Commit de26d641 parent abbf44112 files · +107 −42Browse files

Unified · Split

12 files
  1. apps/cloud/src/api/protected-api-key-auth.node.test.ts +1 −0
  2. apps/cloud/src/api/protected-jwt-auth.node.test.ts +1 −0
  3. apps/cloud/src/auth/workos-auth-provider.ts +3 −0
  4. apps/host-cloudflare/src/auth/cloudflare-access.ts +2 −0
  5. apps/host-selfhost/src/auth/identity.ts +1 −0
  6. apps/host-selfhost/src/platform-credential.test.ts +17 −3
  7. apps/host-selfhost/src/testing/test-app.ts +2 −0
  8. apps/local/src/identity.ts +1 −0
  9. packages/core/api/src/server/execution-stack-middleware.ts +51 −26
  10. packages/core/api/src/server/execution-stack.ts +7 −6
  11. packages/core/api/src/server/identity.ts +14 −7
  12. packages/core/sdk/src/executor.ts +7 −0
apps/cloud/src/api/protected-api-key-auth.node.test.ts+1−0
@@ -89,6 +89,7 @@ describe("protected API key auth", () => { · 88 unmodified lines
8989 );
9090
9191 expect(identity).toEqual({
92+ kind: "member",
9293 accountId: "user_123",
9394 organizationId: "org_123",
9495 organizationName: "Org org_123",
apps/cloud/src/api/protected-jwt-auth.node.test.ts+1−0
@@ -110,6 +110,7 @@ describe("protected JWT (device-login) auth", () => { · 109 unmodified lines
110110 const identity = yield* run(request(token), config);
111111
112112 expect(identity).toEqual({
113+ kind: "member",
113114 accountId: "user_123",
114115 organizationId: "org_123",
115116 organizationName: "Org org_123",
apps/cloud/src/auth/workos-auth-provider.ts+3−0
@@ -145,6 +145,7 @@ const resolveJwtPrincipal = (token: string, jwt: JwtBearerConfig) => · 144 unmodified lines
145145 if (!org) return yield* new NoOrganization(NO_ORGANIZATION_IN_ACCESS_TOKEN);
146146
147147 return {
148+ kind: "member",
148149 accountId: verified.accountId,
149150 organizationId: org.id,
150151 organizationName: org.name,
@@ -243,6 +244,7 @@ export const resolveBearerAuth = ( · 92 unmodified lines
243244 if (!org) return yield* new NoOrganization(NO_ORGANIZATION_IN_API_KEY);
244245
245246 return {
247+ kind: "member",
246248 accountId: owner.accountId,
247249 organizationId: org.id,
248250 organizationName: org.name,
@@ -315,6 +317,7 @@ export const resolveSessionPrincipal = (request: Request) => · 66 unmodified lines
315317 const org = yield* authorizeOrganizationSelector(session.userId, selector);
316318 if (!org) return yield* new NoOrganization(NO_ORGANIZATION_IN_SESSION);
317319 return {
320+ kind: "member",
318321 accountId: session.userId,
319322 organizationId: org.id,
320323 organizationName: org.name,
apps/host-cloudflare/src/auth/cloudflare-access.ts+2−0
@@ -38,6 +38,7 @@ export const principalFromAccessClaims = ( · 37 unmodified lines
3838 const isAdmin = email.length > 0 && config.adminEmails.includes(email.toLowerCase());
3939
4040 return {
41+ kind: "member",
4142 accountId: sub || email || commonName,
4243 organizationId: config.organizationId,
4344 organizationName: config.organizationName,
@@ -68,6 +69,7 @@ export const makeAccessVerifier = (config: CloudflareConfig) => { · 24 unmodified lines
6869 // fixed admin. Only when explicitly enabled (and the instance is otherwise
6970 // unprotected). Mirrors the local app's single-user model.
7071 const devPrincipal: Principal = {
72+ kind: "member",
7173 accountId: "dev",
7274 organizationId: config.organizationId,
7375 organizationName: config.organizationName,
apps/host-selfhost/src/auth/identity.ts+1−0
@@ -67,6 +67,7 @@ export const betterAuthIdentityLayer: Layer.Layer<IdentityProvider, never, Bette · 66 unmodified lines
6767 // default to the seeded org rather than rejecting with NoOrganization.
6868 const resolvedOrganizationId = resolved.session.activeOrganizationId ?? organizationId;
6969 return {
70+ kind: "member" as const,
7071 accountId: resolved.user.id,
7172 organizationId: resolvedOrganizationId,
7273 organizationName,
apps/host-selfhost/src/platform-credential.test.ts+17−3
@@ -127,9 +127,8 @@ test("connection reads answer org-owned rows and never a member's personal ones" · 126 unmodified lines
127127 new Request("http://localhost/api/connections", { headers: platformHeaders }),
128128 );
129129 expect(res.status).toBe(200);
130− const raw = await res.text();
131− // oxlint-disable-next-line executor/no-json-parse -- boundary: test asserts on the raw wire body (for the token sweep) and its parsed form together
132− const body = JSON.parse(raw) as ReadonlyArray<{ readonly name: string }>;
130+ const body = (await res.json()) as ReadonlyArray<{ readonly name: string }>;
131+ const raw = JSON.stringify(body);
133132 expect(
134133 body.map((connection) => connection.name),
135134 "the org-owned connection is visible",
@@ -141,6 +140,21 @@ test("connection reads answer org-owned rows and never a member's personal ones" · 5 unmodified lines
141140 expect(raw, "no credential material either way").not.toContain("token");
142141 });
143142
143+test("the OAuth callback is refused despite being a GET", async () => {
144+ // The one core GET with side effects: completing it would burn an org-owned
145+ // in-flight authorization code in an outbound token exchange before the
146+ // storage policy could refuse the final write. The safe-request gate excludes
147+ // it by name; a platform credential landing here is a clear 403, not a
148+ // half-executed flow.
149+ const res = await handler(
150+ new Request("http://localhost/api/oauth/callback?code=x&state=y", {
151+ headers: platformHeaders,
152+ }),
153+ );
154+ expect(res.status, "the callback is not a safe read").toBe(403);
155+ expect(await res.text()).toContain("read-only");
156+});
157+
144158 test("every non-GET is refused before a handler runs", async () => {
145159 const post = await handler(
146160 new Request("http://localhost/api/connections", {
apps/host-selfhost/src/testing/test-app.ts+2−0
@@ -81,6 +81,7 @@ export const singleAdminIdentityLayer = ( · 80 unmodified lines
8181 IdentityProvider.of({
8282 authenticate: () =>
8383 Effect.succeed<Principal>({
84+ kind: "member",
8485 accountId: options.userId,
8586 organizationId: options.organizationId,
8687 organizationName: options.organizationName,
@@ -117,6 +118,7 @@ export const headerIdentityLayer: Layer.Layer<IdentityProvider> = Layer.succeed( · 30 unmodified lines
117118 const organizationId = request.headers.get("x-test-org");
118119 if (!userId || !organizationId) return Effect.fail(new Unauthorized());
119120 return Effect.succeed<Principal>({
121+ kind: "member",
120122 accountId: userId,
121123 organizationId,
122124 organizationName: `Org ${organizationId}`,
apps/local/src/identity.ts+1−0
@@ -31,6 +31,7 @@ import { safeEqual } from "./serve-shared"; · 30 unmodified lines
3131 * cwd-derived (in `app.ts`), independent of these ids.
3232 */
3333 export const LOCAL_PRINCIPAL: Principal = {
34+ kind: "member",
3435 accountId: "local",
3536 organizationId: "local",
3637 organizationName: "Local",
packages/core/api/src/server/execution-stack-middleware.ts+51−26
@@ -35,7 +35,7 @@ · 34 unmodified lines
3535 // ---------------------------------------------------------------------------
3636
3737 import { HttpRouter, HttpServerRequest, HttpServerResponse } from "effect/unstable/http";
38−import { Context, Effect, Layer } from "effect";
38+import { Context, Data, Effect, Layer } from "effect";
3939 import type * as Cause from "effect/Cause";
4040
4141 import type { AnyPlugin } from "@executor-js/sdk";
@@ -185,15 +185,16 @@ export const makeExecutionStackMiddleware = < · 143 unmodified lines
185185 const webRequest = yield* HttpServerRequest.toWeb(request);
186186 const resolved = yield* options.strategy.renderFailure(
187187 options.authenticate(webRequest).pipe(
188− // The PLATFORM branch's method gate lives here, before any
189− // handler: an org credential is read-only by construction, so a
190− // non-GET request is refused as a typed 403 rather than reaching
191− // a handler that would bind it to a subject or attempt a write
192− // (the storage policy would refuse the write anyway — this makes
193− // the refusal a clear response instead of a 500).
188+ // The PLATFORM branch's gate lives here, before any handler: an
189+ // org credential is read-only by construction, so anything that
190+ // is not a safe read is refused as a typed 403 rather than
191+ // reaching a handler that would bind it to a subject or attempt
192+ // a write (the storage policy would refuse the write anyway —
193+ // this makes the refusal a clear response instead of a 500).
194194 Effect.filterOrFail(
195− (principal): principal is ResolvedPrincipal =>
196− !isPlatformPrincipal(principal) || webRequest.method === "GET",
195+ (principal) =>
196+ !isPlatformPrincipal(principal) ||
197+ isPlatformSafeRequest(webRequest.method, new URL(webRequest.url).pathname),
197198 () =>
198199 new ReadOnlyCredential({
199200 code: "read_only_credential",
@@ -208,14 +209,14 @@ export const makeExecutionStackMiddleware = < · 8 unmodified lines
208209 if (isPlatformPrincipal(resolved)) {
209210 // Org credential: the subject-less, write-refusing platform stack.
210211 // No `touchSubject` runs, so the credential never mints a phantom
211− // user row in the very lists the admin plane serves.
212+ // user row in the very lists the admin plane serves. Neither
213+ // `RequestWebOrigin` nor `RequestOrgSlug` is provided: both feed
214+ // browser-handoff URL construction, which only interactive member
215+ // flows perform, and `makePlatformExecutor` reads neither.
212216 const { executor } = yield* makePlatformExecutionStack<TPlugins>(
213217 resolved.organizationId,
214218 ).pipe(
215219 Effect.provide(options.stackLayer),
216− Effect.provideService(RequestWebOrigin, {
217− origin: requestWebOriginFromRequest(webRequest),
218− }),
219220 Effect.withSpan("executor.stack.http.resolve_platform"),
220221 );
221222 return yield* httpEffect.pipe(
@@ -274,24 +275,48 @@ const isPrincipal = ( · 52 unmodified lines
274275 ): value is ResolvedPrincipal => !HttpServerResponse.isHttpServerResponse(value);
275276
276277 /**
277− * The engine the platform branch provides: every read answers "nothing here",
278− * honestly — the platform view owns no executions and pauses none. The
279− * execute/resume members exist only to satisfy the service shape; they sit
280− * behind the middleware's GET-only gate, so reaching one is a wiring bug and
281− * dies as a defect rather than failing with a typed error a client could
282− * mistake for a product answer.
278+ * Whether a request is a SAFE READ the platform branch may serve. The method
279+ * check (GET, plus HEAD — the router serves HEAD off the GET route) is
280+ * necessary but NOT sufficient: `GET /oauth/callback` is the one core GET with
281+ * side effects — completing it reads an org-owned oauth session, performs an
282+ * outbound authorization-code exchange with the org's client credentials, and
283+ * then attempts the connection write. A read-only credential must be refused
284+ * BEFORE that exchange burns the in-flight code, not after the storage policy
285+ * rejects the final write, so the callback path is excluded by name. It is a
286+ * browser-redirect surface — no legitimate machine-credential caller lands on
287+ * it with a Bearer header.
288+ */
289+const isPlatformSafeRequest = (method: string, pathname: string): boolean =>
290+ (method === "GET" || method === "HEAD") && !pathname.endsWith("/oauth/callback");
291+
292+/** Reaching an engine member the platform branch cannot honestly serve is a
293+ * wiring bug (the safe-request gate keeps those paths unreachable), so it
294+ * dies as a tagged defect rather than failing with a typed error a client
295+ * could mistake for a product answer. */
296+class PlatformEngineUnavailable extends Data.TaggedError("PlatformEngineUnavailable")<{
297+ readonly member: string;
298+}> {}
299+
300+/**
301+ * The engine the platform branch provides. The one member a safe read can
302+ * actually reach — `getPausedExecution`, via `GET /executions/:id` — answers
303+ * null (a 404), honestly: the platform view owns no executions. The paused
304+ * counters answer the same empty story for any future reader. Everything else
305+ * (execute, resume, the MCP tool description) exists to satisfy the service
306+ * shape, sits behind the middleware's safe-request gate, and dies if reached.
283307 */
284308 const readOnlyExecutionEngine: ExecutionEngine<Cause.YieldableError> = {
285− // oxlint-disable-next-line executor/no-effect-escape-hatch -- boundary: unreachable behind the middleware's GET-only gate; reaching it is a wiring bug, not a typed product outcome
286− execute: () => Effect.die("platform credential: execution engine unavailable"),
287− // oxlint-disable-next-line executor/no-effect-escape-hatch -- boundary: unreachable behind the middleware's GET-only gate; reaching it is a wiring bug, not a typed product outcome
288− executeWithPause: () => Effect.die("platform credential: execution engine unavailable"),
289− // oxlint-disable-next-line executor/no-effect-escape-hatch -- boundary: unreachable behind the middleware's GET-only gate; reaching it is a wiring bug, not a typed product outcome
290− resume: () => Effect.die("platform credential: execution engine unavailable"),
309+ // oxlint-disable-next-line executor/no-effect-escape-hatch -- boundary: unreachable behind the middleware's safe-request gate; reaching it is a wiring bug, not a typed product outcome
310+ execute: () => Effect.die(new PlatformEngineUnavailable({ member: "execute" })),
311+ // oxlint-disable-next-line executor/no-effect-escape-hatch -- boundary: unreachable behind the middleware's safe-request gate; reaching it is a wiring bug, not a typed product outcome
312+ executeWithPause: () => Effect.die(new PlatformEngineUnavailable({ member: "executeWithPause" })),
313+ // oxlint-disable-next-line executor/no-effect-escape-hatch -- boundary: unreachable behind the middleware's safe-request gate; reaching it is a wiring bug, not a typed product outcome
314+ resume: () => Effect.die(new PlatformEngineUnavailable({ member: "resume" })),
291315 getPausedExecution: () => Effect.succeed(null),
292316 pausedExecutionCount: () => Effect.succeed(0),
293317 hasPausedExecutions: () => Effect.succeed(false),
294− getDescription: Effect.succeed(""),
318+ // oxlint-disable-next-line executor/no-effect-escape-hatch -- boundary: only the MCP tool server reads this, and the MCP plane never serves a platform credential
319+ getDescription: Effect.die(new PlatformEngineUnavailable({ member: "getDescription" })),
295320 };
296321
297322 const LOOPBACK_HOSTNAMES = new Set(["localhost", "127.0.0.1", "::1", "[::1]"]);
packages/core/api/src/server/execution-stack.ts+7−6
@@ -147,12 +147,13 @@ export const makeExecutionStack = < · 146 unmodified lines
147147
148148 // ---------------------------------------------------------------------------
149149 // makePlatformExecutionStack — the org-credential sibling: a subject-less,
150−// write-refusing executor (`makePlatformExecutor`) and NO engine. An org key is
151−// an observer; the execution engine exists to run code as an acting member, so
152−// rather than building one that would fail on use, the middleware refuses
153−// non-GET requests up front and never provides `ExecutionEngineService` on this
154−// branch. Read-only handlers never read the engine, so the platform branch
155−// serves them unchanged.
150+// write-refusing executor (`makePlatformExecutor`) and no REAL engine. An org
151+// key is an observer; the execution engine exists to run code as an acting
152+// member, so no engine is built here. The middleware still provides
153+// `ExecutionEngineService` (handlers' service requirements demand one) — as a
154+// stub whose reachable reads answer "nothing here" and whose execute/resume
155+// members sit behind the middleware's safe-request gate (see
156+// `readOnlyExecutionEngine` in ./execution-stack-middleware.ts).
156157 // ---------------------------------------------------------------------------
157158
158159 export const makePlatformExecutionStack = <
packages/core/api/src/server/identity.ts+14−7
@@ -29,6 +29,11 @@ import { Context, Effect, Schema } from "effect"; · 28 unmodified lines
2929 * resolver already yielded it) AND `roles` (cloud supplies `[]`).
3030 */
3131 export interface Principal {
32+ /** Discriminant of {@link ResolvedPrincipal}: an acting member, as opposed
33+ * to the org-level `"platform"` credential. Required so every construction
34+ * site declares which arm it is, and the union matches on a literal tag
35+ * instead of probing for a property's presence. */
36+ readonly kind: "member";
3237 readonly accountId: string;
3338 readonly organizationId: string;
3439 readonly organizationName: string;
@@ -73,24 +78,26 @@ export interface PlatformPrincipal { · 38 unmodified lines
7378 export type ResolvedPrincipal = Principal | PlatformPrincipal;
7479
7580 export const isPlatformPrincipal = (value: ResolvedPrincipal): value is PlatformPrincipal =>
76− "kind" in value && value.kind === "platform";
81+ value.kind === "platform";
7782
7883 /**
7984 * The single `AuthContext` every executor-API handler reads. The roles-bearing
8085 * tag from self-host is the model; cloud now provides `roles: []` on it, which
8186 * is forward-compatible (cloud handlers never read roles today).
8287 *
83− * `accountId` is `null` for an org-level platform credential — there is no
84− * acting member behind it. Nullable rather than a sentinel so any handler that
85− * needs a member has to say so (and refuse), instead of silently acting as a
86− * user that does not exist.
88+ * `accountId` and `email` are `null` for an org-level platform credential —
89+ * there is no acting member behind it. Nullable rather than sentinel-valued so
90+ * any handler that needs a member has to say so (and refuse), instead of
91+ * silently acting as a user that does not exist. (`email: ""` on the member
92+ * api-key path is the pre-existing "member with no resolved email" value and
93+ * unrelated to this.)
8794 */
8895 export class AuthContext extends Context.Service<
8996 AuthContext,
9097 {
9198 readonly accountId: string | null;
9299 readonly organizationId: string;
93− readonly email: string;
100+ readonly email: string | null;
94101 readonly name: string | null;
95102 readonly avatarUrl: string | null;
96103 readonly roles: readonly string[];
@@ -111,7 +118,7 @@ export const authContextFromPrincipal = (principal: Principal): AuthContext["Ser · 14 unmodified lines
111118 export const authContextFromPlatform = (principal: PlatformPrincipal): AuthContext["Service"] => ({
112119 accountId: null,
113120 organizationId: principal.organizationId,
114− email: "",
121+ email: null,
115122 name: null,
116123 avatarUrl: null,
117124 roles: [],
packages/core/sdk/src/executor.ts+7−0
@@ -3585,6 +3585,13 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea · 3584 unmodified lines
35853585 // Best-effort: a failed rebuild leaves the stale-but-working catalog in
35863586 // place and retries on the next read.
35873587 const syncStaleConnectionTools = Effect.gen(function* () {
3588+ // The platform view can never persist a rebuilt catalog (writes are
3589+ // denied at the storage boundary), so attempting the sync would only
3590+ // fire upstream `resolveTools` calls whose results are thrown away —
3591+ // network side effects on a read-only credential. Skip it entirely:
3592+ // read-only-ness of the platform read path is a stated invariant here,
3593+ // not an accident of the best-effort catch below.
3594+ if (config.platformView === true) return;
35883595 const integrations = yield* core.findMany("integration", {});
35893596 if (integrations.length === 0) return;
35903597 const integrationBySlug = new Map(integrations.map((row) => [row.slug, row] as const));