Address review: typed e2e assertions, hoisted issuer canonicalizer

The e2e probe-failure assertions now narrow with Predicate.isTagged and assert the typed message instead of stringifying a cast. canonicalIssuer is hoisted to module scope with the issuer-vs-resource canonicalization difference documented in one place, and the DCR resource canonicalization records why legacy raw-string rows need no backfill.

Rhys Sullivan committed 3 months ago

Commit 4dff9e51 parent b6868e53 files · +26 −14Browse files

Unified · Split

3 files

e2e/scenarios/oauth-dcr-compliance.test.ts +10 −7
packages/core/sdk/src/oauth-discovery.ts +11 −7
packages/core/sdk/src/oauth-service.ts +5 −0

e2e/scenarios/oauth-dcr-compliance.test.ts+10−7

 import { createServer } from "node:net";
 import { expect } from "@effect/vitest";
-import { Effect } from "effect";
+import { Effect, Predicate } from "effect";
 import { composePluginApi } from "@executor-js/api/server";
 import { createEmulator, type Emulator } from "@executor-js/emulate";
 import { mcpHttpPlugin } from "@executor-js/plugin-mcp/api";
+import type { OAuthProbeError } from "@executor-js/sdk";
 import {
   AuthTemplateSlug,
   ConnectionName,
       const failure = yield* Effect.flip(
         client.oauth.probe({ payload: { url: `${emulator.url}/mcp` } }),
       );
-      expect(String((failure as { readonly _tag?: string })._tag ?? failure)).toContain(
+      expect(Predicate.isTagged("OAuthProbeError")(failure)).toBe(true);
-        "OAuthProbeError",
+      const probeError = failure as OAuthProbeError;
-      );
+      expect(probeError.message).toContain("issuer");
+      expect(probeError.message).toContain("https://evil.example.com");
     }),
   ),
 );
       const failure = yield* Effect.flip(
         client.oauth.probe({ payload: { url: `${emulator.url}/mcp` } }),
       );
-      expect(String((failure as { readonly _tag?: string })._tag ?? failure)).toContain(
+      expect(Predicate.isTagged("OAuthProbeError")(failure)).toBe(true);
-        "OAuthProbeError",
+      const probeError = failure as OAuthProbeError;
-      );
+      expect(probeError.message).toContain("resource");
+      expect(probeError.message).toContain("https://other.example.com/mcp");
     }),
   ),
 );

packages/core/sdk/src/oauth-discovery.ts+11−7

       }),
   });
+/** RFC 8414 issuer identity: scheme, host, port, and path after URL
+ *  normalization; a trailing slash alone does not create a different issuer.
+ *  Deliberately NOT `canonicalResourceUrl`: an issuer never carries a query,
+ *  and its trailing slash is insignificant (RFC 8414 §2), whereas a resource
+ *  identifier's query and trailing slash are part of the identity (RFC 8707). */
+const canonicalIssuer = (value: string): string => {
+  const url = new URL(value);
+  const path = url.pathname.replace(/\/+$/, "");
+  return `${url.protocol.toLowerCase()}//${url.host.toLowerCase()}${path}`;
+};
 const validateAuthorizationServerMetadata = (
   metadata: OAuthAuthorizationServerMetadata,
   expectedIssuer: string,
 ): Effect.Effect<void, OAuthDiscoveryError> =>
   Effect.gen(function* () {
     yield* validateEndpointUrl(metadata.issuer, "issuer", policy);
-    const canonicalIssuer = (value: string): string => {
-      const url = new URL(value);
-      const path = url.pathname.replace(/\/+$/, "");
-      return `${url.protocol.toLowerCase()}//${url.host.toLowerCase()}${path}`;
-    };
-    // RFC 8414 issuer identity compares scheme, host, port, and path after URL
-    // normalization; a trailing slash alone does not create a different issuer.
     if (canonicalIssuer(metadata.issuer) !== canonicalIssuer(expectedIssuer)) {
       return yield* new OAuthDiscoveryError({
         message:

packages/core/sdk/src/oauth-service.ts+5−0

     Effect.gen(function* () {
       const issuer = canonicalDcrIssuer(input.issuer, input.registrationEndpoint);
       const requestedResource = input.resource ?? null;
+      // Canonicalized ONCE here; reuse-matching, persistence, and every later
+      // authorize/token request all see this exact value. Rows persisted
+      // before canonicalization may hold a raw string that no longer matches
+      // (case, trailing slash) — those clients simply re-register, which is
+      // safe and self-healing, so no backfill migration is warranted.
       const resource =
         requestedResource === null
           ? null