From d1ce461c00f3fef019bd8d36d04e53d18610105c Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 7 Nov 2026 15:02:56 -0500 Subject: [PATCH 01/18] fix(oauth): forbid framing and set security headers on every response Both Express apps now mount one middleware ahead of everything else. It sets X-Frame-Options: DENY, Content-Security-Policy: frame-ancestors 'none', X-Content-Type-Options: nosniff and Referrer-Policy: same-origin, and both apps stop sending X-Powered-By. Co-Authored-By: Claude Opus 5.5 --- .../integration/server-integration.test.ts | 28 ++++++++++ .../server-oauth-integration.test.ts | 52 ++++++++++++++++--- src/__tests__/integration/test-harness.ts | 25 +++++++++ src/utils/__tests__/security-headers.test.ts | 44 ++++++++++++++++ src/utils/security-headers.ts | 19 +++++++ src/vault-mcp/server.ts | 4 ++ .../setup/__tests__/setup-server.test.ts | 27 +++++++++- src/vault-mcp/setup/setup-server.ts | 4 ++ 8 files changed, 194 insertions(+), 9 deletions(-) create mode 100644 src/utils/__tests__/security-headers.test.ts create mode 100644 src/utils/security-headers.ts diff --git a/src/__tests__/integration/server-integration.test.ts b/src/__tests__/integration/server-integration.test.ts index fc6317a98..a81e9c9d9 100644 --- a/src/__tests__/integration/server-integration.test.ts +++ b/src/__tests__/integration/server-integration.test.ts @@ -27,6 +27,8 @@ import { mcpInitStatus, callTool, textContent, + readSecurityHeaders, + EXPECTED_SECURITY_HEADERS, } from "./test-harness.js" import type { ToolResult } from "./test-harness.js" @@ -1410,6 +1412,32 @@ describe("default config", () => { }) expect(response.status).toBe(404) + // Express's 404 page replaces Content-Security-Policy with a policy of + // its own, so X-Frame-Options alone forbids framing it. + expect(response.headers.get("x-frame-options")).toBe("DENY") + expect(response.headers.get("x-powered-by")).toBeNull() + }) + }) + + describe("security headers", () => { + it.each([ + { label: "/healthz", path: "/healthz", init: {}, expectedStatus: 200 }, + { + label: "an unauthenticated /mcp request", + path: "/mcp", + init: { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ jsonrpc: "2.0", method: "initialize", id: 1, params: {} }), + }, + expectedStatus: 401, + }, + { label: "/setup", path: "/setup", init: {}, expectedStatus: 200 }, + ])("sets them on $label, without X-Powered-By", async ({ path, init, expectedStatus }) => { + const response = await fetch(`http://127.0.0.1:${port}${path}`, init) + + expect(response.status).toBe(expectedStatus) + expect(readSecurityHeaders(response)).toEqual(EXPECTED_SECURITY_HEADERS) }) }) diff --git a/src/__tests__/integration/server-oauth-integration.test.ts b/src/__tests__/integration/server-oauth-integration.test.ts index 36753ad40..93719267e 100644 --- a/src/__tests__/integration/server-oauth-integration.test.ts +++ b/src/__tests__/integration/server-oauth-integration.test.ts @@ -8,7 +8,12 @@ import { join } from "node:path" import Database from "better-sqlite3" import { DateTime } from "luxon" import { signJwt } from "../../jwt.js" -import { startServer, freePort } from "./test-harness.js" +import { + startServer, + freePort, + readSecurityHeaders, + EXPECTED_SECURITY_HEADERS, +} from "./test-harness.js" const REDIRECT_URI = "http://127.0.0.1/callback" const TOKEN_A = "integration-token-before-rotation" @@ -74,18 +79,15 @@ const registerClient = async (port: number): Promise => { return registered } -/** Consent page → approve with the auth token → PKCE code exchange. */ -const authorize = async ({ +const buildAuthorizeUrl = ({ port, client, - authToken, + challenge, }: { port: number client: RegisteredClient - authToken: string -}): Promise => { - const verifier = base64Url(randomBytes(32)) - const challenge = base64Url(createHash("sha256").update(verifier).digest()) + challenge: string +}): URL => { const authorizeUrl = new URL(`http://127.0.0.1:${port}/authorize`) authorizeUrl.search = new URLSearchParams({ response_type: "code", @@ -95,6 +97,22 @@ const authorize = async ({ code_challenge_method: "S256", scope: "vault", }).toString() + return authorizeUrl +} + +/** Consent page → approve with the auth token → PKCE code exchange. */ +const authorize = async ({ + port, + client, + authToken, +}: { + port: number + client: RegisteredClient + authToken: string +}): Promise => { + const verifier = base64Url(randomBytes(32)) + const challenge = base64Url(createHash("sha256").update(verifier).digest()) + const authorizeUrl = buildAuthorizeUrl({ port, client, challenge }) const consentHtml = await (await fetch(authorizeUrl)).text() const requestId = /name="request_id"\s+value="([^"]+)"/.exec(consentHtml)?.[1] @@ -165,6 +183,24 @@ const refresh = ({ }) } +// ── Consent page response headers ────────────────────────────── + +describe("consent page response headers", () => { + it("serves the consent page with the security headers and without X-Powered-By", async () => { + const port = await freePort() + const server = await startServer(port, { MCP_AUTH_TOKEN: TOKEN_A }) + onTestFinished(() => server.cleanup()) + const client = await registerClient(port) + const challenge = base64Url(createHash("sha256").update("header-check").digest()) + + const response = await fetch(buildAuthorizeUrl({ port, client, challenge })) + + expect(response.status).toBe(200) + expect(await response.text()).toContain("

Authorize access

") + expect(readSecurityHeaders(response)).toEqual(EXPECTED_SECURITY_HEADERS) + }) +}) + // ── Expired access token challenge ───────────────────────────── describe("expired access token challenge", () => { diff --git a/src/__tests__/integration/test-harness.ts b/src/__tests__/integration/test-harness.ts index f622211e0..8db1c067d 100644 --- a/src/__tests__/integration/test-harness.ts +++ b/src/__tests__/integration/test-harness.ts @@ -382,3 +382,28 @@ export const mcpInitStatus = async (port: number, authHeader?: string): Promise< }) return response.status } + +type SecurityHeaders = { + frameOptions: string | null + contentSecurityPolicy: string | null + contentTypeOptions: string | null + referrerPolicy: string | null + poweredBy: string | null +} + +/** The headers both apps add to every response, plus X-Powered-By, which both turn off. */ +export const readSecurityHeaders = (response: Response): SecurityHeaders => ({ + frameOptions: response.headers.get("x-frame-options"), + contentSecurityPolicy: response.headers.get("content-security-policy"), + contentTypeOptions: response.headers.get("x-content-type-options"), + referrerPolicy: response.headers.get("referrer-policy"), + poweredBy: response.headers.get("x-powered-by"), +}) + +export const EXPECTED_SECURITY_HEADERS: SecurityHeaders = { + frameOptions: "DENY", + contentSecurityPolicy: "frame-ancestors 'none'", + contentTypeOptions: "nosniff", + referrerPolicy: "same-origin", + poweredBy: null, +} diff --git a/src/utils/__tests__/security-headers.test.ts b/src/utils/__tests__/security-headers.test.ts new file mode 100644 index 000000000..af8a23dfb --- /dev/null +++ b/src/utils/__tests__/security-headers.test.ts @@ -0,0 +1,44 @@ +import { describe, it, expect, onTestFinished } from "vitest" +import type { Server } from "node:http" +import express from "express" +import { securityHeaders } from "../security-headers.js" + +const startApp = async (): Promise => { + const app = express() + app.use(securityHeaders) + app.get("/page", (_req, res) => { + res.send("page body") + }) + const server = await new Promise((resolve) => { + const listening = app.listen(0, "127.0.0.1", () => resolve(listening)) + }) + onTestFinished(() => new Promise((resolve) => server.close(() => resolve()))) + const serverAddress = server.address() + + if (!serverAddress || typeof serverAddress === "string") { + throw new Error("expected a TCP address from a listening server") + } + return `http://127.0.0.1:${serverAddress.port}` +} + +describe("securityHeaders", () => { + it("sets the framing, sniffing and referrer headers, then hands the request on", async () => { + const baseUrl = await startApp() + + const response = await fetch(`${baseUrl}/page`) + + expect({ + frameOptions: response.headers.get("x-frame-options"), + contentSecurityPolicy: response.headers.get("content-security-policy"), + contentTypeOptions: response.headers.get("x-content-type-options"), + referrerPolicy: response.headers.get("referrer-policy"), + }).toEqual({ + frameOptions: "DENY", + contentSecurityPolicy: "frame-ancestors 'none'", + contentTypeOptions: "nosniff", + referrerPolicy: "same-origin", + }) + expect(response.status).toBe(200) + await expect(response.text()).resolves.toBe("page body") + }) +}) diff --git a/src/utils/security-headers.ts b/src/utils/security-headers.ts new file mode 100644 index 000000000..373bb9a77 --- /dev/null +++ b/src/utils/security-headers.ts @@ -0,0 +1,19 @@ +import type { NextFunction, Request, Response } from "express" + +const SECURITY_HEADERS = { + // Browsers that predate the frame-ancestors directive read only this header. + "X-Frame-Options": "DENY", + // frame-ancestors alone: a fuller policy would block the pages' inline script and styles. + "Content-Security-Policy": "frame-ancestors 'none'", + "X-Content-Type-Options": "nosniff", + // Sends no referrer to other sites. no-referrer would also turn a page's own + // form posts into `Origin: null`. + "Referrer-Policy": "same-origin", +} + +/** Express middleware that stops other sites from framing a response and from + * receiving its address as a referrer, and turns off content-type sniffing. */ +export const securityHeaders = (_req: Request, res: Response, next: NextFunction): void => { + res.set(SECURITY_HEADERS) + next() +} diff --git a/src/vault-mcp/server.ts b/src/vault-mcp/server.ts index e11cb2e8d..4dcadf50a 100644 --- a/src/vault-mcp/server.ts +++ b/src/vault-mcp/server.ts @@ -23,6 +23,7 @@ import { logger } from "../logger.js" import { extractClientIp, headerAsString } from "../auth.js" import { describeError } from "../utils/describe-error.js" import { urlHasCredentials } from "../utils/url-has-credentials.js" +import { securityHeaders } from "../utils/security-headers.js" import env from "env-var" /** Error middleware — logs the failure with request context, answers 500. @@ -219,6 +220,9 @@ const startServer = async (): Promise => { }) const app = express() + // First, so every response carries the headers, errors and redirects included. + app.use(securityHeaders) + app.disable("x-powered-by") // Proxy trust is deployment-explicit: TRUST_PROXY_HOPS grants one // X-Forwarded-For hop per proxy the deployment controls. With the // default 0, req.ip — the OAuth rate limiter's fallback bucket key — diff --git a/src/vault-mcp/setup/__tests__/setup-server.test.ts b/src/vault-mcp/setup/__tests__/setup-server.test.ts index 5de47f8c9..b50368de4 100644 --- a/src/vault-mcp/setup/__tests__/setup-server.test.ts +++ b/src/vault-mcp/setup/__tests__/setup-server.test.ts @@ -5,7 +5,12 @@ import { tmpdir } from "node:os" import { join } from "node:path" import { fileURLToPath } from "node:url" import { describe, expect, it, onTestFinished, vi } from "vitest" -import { freePort, stopChild } from "../../../__tests__/integration/test-harness.js" +import { + EXPECTED_SECURITY_HEADERS, + freePort, + readSecurityHeaders, + stopChild, +} from "../../../__tests__/integration/test-harness.js" import { startFakeObsidianApi } from "./fake-obsidian-api.js" // The entry point starts through tsx; the 15 s start timeout inside the tests @@ -290,6 +295,26 @@ describe("setup-server entry point", () => { }) }) + it("sets the security headers on the sign-in page and the browser redirect, without X-Powered-By", async () => { + const server = await spawnSetupServer({ + HOME: tmpdir(), + MCP_AUTH_TOKEN: AUTH_TOKEN, + PUBLIC_URL: "https://vault.example.com", + }) + await waitForStart(server) + + const signInPage = await fetch(`http://127.0.0.1:${server.port}/setup`) + const browserRedirect = await fetch(`http://127.0.0.1:${server.port}/anything`, { + headers: { Accept: "text/html" }, + redirect: "manual", + }) + + expect(signInPage.status).toBe(200) + expect(readSecurityHeaders(signInPage)).toEqual(EXPECTED_SECURITY_HEADERS) + expect(browserRedirect.status).toBe(302) + expect(readSecurityHeaders(browserRedirect)).toEqual(EXPECTED_SECURITY_HEADERS) + }) + it("shows the saved-login notice when SETUP_REASON is login-failed", async () => { const server = await spawnSetupServer({ HOME: tmpdir(), diff --git a/src/vault-mcp/setup/setup-server.ts b/src/vault-mcp/setup/setup-server.ts index f9faaeb3e..da17ddc8f 100644 --- a/src/vault-mcp/setup/setup-server.ts +++ b/src/vault-mcp/setup/setup-server.ts @@ -11,6 +11,7 @@ import env from "env-var" import { loadConfig } from "../config.js" import { logger } from "../../logger.js" import { describeError } from "../../utils/describe-error.js" +import { securityHeaders } from "../../utils/security-headers.js" import { createSetupRoutes } from "./setup-routes.js" import type { HostingPlatform } from "./setup-page.js" @@ -53,6 +54,9 @@ const startSetupServer = (): void => { const setupUrl = publicUrl ? new URL("/setup", publicUrl).href : "/setup" const app = express() + // First, so every response carries the headers, errors and redirects included. + app.use(securityHeaders) + app.disable("x-powered-by") app.set("trust proxy", config.trustProxyHops) // Healthy on purpose: the platform's health check must pass for the From 14d9ea175bd81b1b237eb6adbb8fb35744821bb2 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 7 Nov 2026 15:04:43 -0500 Subject: [PATCH 02/18] fix(oauth): stop browsers caching consent decisions and setup pages Every /oauth/decide response, including the form parser's errors, and every setup-mode response now sends Cache-Control: no-store, as does the full server's /setup page. Co-Authored-By: Claude Opus 5.5 --- .../integration/server-integration.test.ts | 7 +++++ .../oauth/__tests__/oauth-routes.test.ts | 30 +++++++++++++++++++ src/vault-mcp/oauth/oauth-routes.ts | 8 +++++ src/vault-mcp/server.ts | 7 ++++- .../setup/__tests__/setup-server.test.ts | 20 +++++++++++++ src/vault-mcp/setup/setup-server.ts | 5 ++++ 6 files changed, 76 insertions(+), 1 deletion(-) diff --git a/src/__tests__/integration/server-integration.test.ts b/src/__tests__/integration/server-integration.test.ts index a81e9c9d9..c08c020c2 100644 --- a/src/__tests__/integration/server-integration.test.ts +++ b/src/__tests__/integration/server-integration.test.ts @@ -1405,6 +1405,13 @@ describe("default config", () => { ) }) + it("marks the already-configured page as not cacheable", async () => { + const response = await fetch(`http://127.0.0.1:${port}/setup`) + + expect(response.status).toBe(200) + expect(response.headers.get("cache-control")).toBe("no-store") + }) + it("does not redirect a browser GET on the root once configured — that behavior belongs to setup mode only", async () => { const response = await fetch(`http://127.0.0.1:${port}/`, { headers: { accept: "text/html,application/xhtml+xml" }, diff --git a/src/vault-mcp/oauth/__tests__/oauth-routes.test.ts b/src/vault-mcp/oauth/__tests__/oauth-routes.test.ts index 5d311b904..7d77b0f3a 100644 --- a/src/vault-mcp/oauth/__tests__/oauth-routes.test.ts +++ b/src/vault-mcp/oauth/__tests__/oauth-routes.test.ts @@ -171,6 +171,7 @@ describe("OAuth consent token submission", () => { const { requestId } = await startPendingRequest() const response = await submitToken(requestId, token) expect(response.status).toBe(302) + expect(response.headers.get("cache-control")).toBe("no-store") const locationHeader = response.headers.get("location") if (!locationHeader) throw new Error("expected Location header on 302") @@ -208,6 +209,7 @@ describe("OAuth consent token submission", () => { const response = await submitToken(requestId, token) expect(response.status).toBe(200) expect(response.headers.get("location")).toBeNull() + expect(response.headers.get("cache-control")).toBe("no-store") expect(readConsentPage(await response.text())).toEqual({ error: "Invalid token. Please try again.", values: ["Test Client", clientId], @@ -272,6 +274,7 @@ describe("OAuth consent body validation", () => { body: "request_id=a&request_id=b&token=t&action=approve", }) expect(response.status).toBe(400) + expect(response.headers.get("cache-control")).toBe("no-store") const text = await response.text() expect(text).toBe("Invalid form submission.") }) @@ -283,9 +286,36 @@ describe("OAuth consent body validation", () => { body: "request_id=a", }) expect(response.status).toBe(400) + expect(response.headers.get("cache-control")).toBe("no-store") const text = await response.text() expect(text).toBe("Invalid form submission.") }) + + it("returns 400 for a request ID with no pending request", async () => { + const response = await fetch(`${baseUrl}/oauth/decide`, { + method: "POST", + headers: { "content-type": "application/x-www-form-urlencoded" }, + body: "request_id=nonexistent-id&token=t&action=approve", + }) + + expect(response.status).toBe(400) + expect(response.headers.get("cache-control")).toBe("no-store") + await expect(response.text()).resolves.toBe("Authorization request expired or invalid.") + }) + + it("marks the form parser's 413 for an oversized form as not cacheable", async () => { + // Over express.urlencoded's default 100 KB body limit + const oversizedToken = "t".repeat(200 * 1024) + + const response = await fetch(`${baseUrl}/oauth/decide`, { + method: "POST", + headers: { "content-type": "application/x-www-form-urlencoded" }, + body: `request_id=a&token=${oversizedToken}&action=approve`, + }) + + expect(response.status).toBe(413) + expect(response.headers.get("cache-control")).toBe("no-store") + }) }) describe("OAuth consent audit logging", () => { diff --git a/src/vault-mcp/oauth/oauth-routes.ts b/src/vault-mcp/oauth/oauth-routes.ts index 8808ce8f1..d9d1f273f 100644 --- a/src/vault-mcp/oauth/oauth-routes.ts +++ b/src/vault-mcp/oauth/oauth-routes.ts @@ -133,6 +133,14 @@ export const createOAuthRoutes = ({ }), ) + // No consent decision response may be cached: the re-rendered page holds the + // request ID, and the redirect holds the authorization code. Mounted ahead of + // the form parser so its errors, such as a 413 for an oversized form, carry it too. + router.use("/oauth/decide", (_req: Request, res: Response, next: NextFunction) => { + res.set("Cache-Control", "no-store") + next() + }) + // Consent form submission (unauthenticated — part of authorize flow) router.post( "/oauth/decide", diff --git a/src/vault-mcp/server.ts b/src/vault-mcp/server.ts index 4dcadf50a..9c3a40d0a 100644 --- a/src/vault-mcp/server.ts +++ b/src/vault-mcp/server.ts @@ -240,7 +240,12 @@ const startServer = async (): Promise => { // token (setup/setup-server.ts); once configured, the same address says so // instead of 404ing on a link from that flow or a guide. app.get("/setup", (_req: Request, res: Response) => { - res.type("html").send(renderSetupPage({ kind: "configured" })) + // Not cached, so the address shows setup mode's page again if the + // container returns to setup mode. + res + .set("Cache-Control", "no-store") + .type("html") + .send(renderSetupPage({ kind: "configured" })) }) app.use( diff --git a/src/vault-mcp/setup/__tests__/setup-server.test.ts b/src/vault-mcp/setup/__tests__/setup-server.test.ts index b50368de4..fa9bd1bb6 100644 --- a/src/vault-mcp/setup/__tests__/setup-server.test.ts +++ b/src/vault-mcp/setup/__tests__/setup-server.test.ts @@ -315,6 +315,26 @@ describe("setup-server entry point", () => { expect(readSecurityHeaders(browserRedirect)).toEqual(EXPECTED_SECURITY_HEADERS) }) + it("marks the sign-in page and the browser redirect as not cacheable", async () => { + const server = await spawnSetupServer({ + HOME: tmpdir(), + MCP_AUTH_TOKEN: AUTH_TOKEN, + PUBLIC_URL: "https://vault.example.com", + }) + await waitForStart(server) + + const signInPage = await fetch(`http://127.0.0.1:${server.port}/setup`) + const browserRedirect = await fetch(`http://127.0.0.1:${server.port}/anything`, { + headers: { Accept: "text/html" }, + redirect: "manual", + }) + + expect(signInPage.status).toBe(200) + expect(signInPage.headers.get("cache-control")).toBe("no-store") + expect(browserRedirect.status).toBe(302) + expect(browserRedirect.headers.get("cache-control")).toBe("no-store") + }) + it("shows the saved-login notice when SETUP_REASON is login-failed", async () => { const server = await spawnSetupServer({ HOME: tmpdir(), diff --git a/src/vault-mcp/setup/setup-server.ts b/src/vault-mcp/setup/setup-server.ts index da17ddc8f..ee9767933 100644 --- a/src/vault-mcp/setup/setup-server.ts +++ b/src/vault-mcp/setup/setup-server.ts @@ -57,6 +57,11 @@ const startSetupServer = (): void => { // First, so every response carries the headers, errors and redirects included. app.use(securityHeaders) app.disable("x-powered-by") + // Setup mode serves sign-in forms and their results, so no response may be cached. + app.use((_req: Request, res: Response, next: NextFunction) => { + res.set("Cache-Control", "no-store") + next() + }) app.set("trust proxy", config.trustProxyHops) // Healthy on purpose: the platform's health check must pass for the From e52949d4ea1bc4f5270fb7a7fbf35af151708a69 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 7 Nov 2026 15:08:28 -0500 Subject: [PATCH 03/18] fix(oauth): show where approval sends access and grant every sign-in the vault scope The consent page now shows the destination of Approve (the redirect address's scheme and host, or the address as written when it has no host) and a fixed description of the grant, in place of the scope list. New sign-ins always get the vault scope; refresh grants keep their stored scope. Co-Authored-By: Claude Opus 5.5 --- .../server-oauth-integration.test.ts | 47 +++++++-- .../oauth/__tests__/consent-page.test.ts | 95 ++++++++++++++++++- .../oauth/__tests__/oauth-provider.test.ts | 40 +++++--- .../oauth/__tests__/oauth-routes.test.ts | 18 ++-- src/vault-mcp/oauth/consent-page.ts | 35 ++++--- src/vault-mcp/oauth/oauth-provider.ts | 21 ++-- src/vault-mcp/oauth/oauth-routes.ts | 2 +- 7 files changed, 205 insertions(+), 53 deletions(-) diff --git a/src/__tests__/integration/server-oauth-integration.test.ts b/src/__tests__/integration/server-oauth-integration.test.ts index 93719267e..75521f868 100644 --- a/src/__tests__/integration/server-oauth-integration.test.ts +++ b/src/__tests__/integration/server-oauth-integration.test.ts @@ -7,7 +7,7 @@ import { tmpdir } from "node:os" import { join } from "node:path" import Database from "better-sqlite3" import { DateTime } from "luxon" -import { signJwt } from "../../jwt.js" +import { signJwt, verifyJwt } from "../../jwt.js" import { startServer, freePort, @@ -23,13 +23,17 @@ const base64Url = (buffer: Buffer): string => buffer.toString("base64").replace(/\+/g, "-").replace(/\//g, "_").replace(/=+$/, "") type RegisteredClient = { client_id: string; client_secret: string } -type IssuedTokens = { access_token: string; refresh_token: string } +type IssuedTokens = { access_token: string; refresh_token: string; scope: string } const isRegisteredClient = (value: unknown): value is RegisteredClient => typeof value === "object" && value !== null && "client_id" in value && "client_secret" in value const isIssuedTokens = (value: unknown): value is IssuedTokens => - typeof value === "object" && value !== null && "access_token" in value && "refresh_token" in value + typeof value === "object" && + value !== null && + "access_token" in value && + "refresh_token" in value && + "scope" in value /** Initialize over /mcp with the transport's required Accept header, so * a valid bearer is distinguishable (200) from a rejected one (401). */ @@ -83,10 +87,12 @@ const buildAuthorizeUrl = ({ port, client, challenge, + scope, }: { port: number client: RegisteredClient challenge: string + scope: string }): URL => { const authorizeUrl = new URL(`http://127.0.0.1:${port}/authorize`) authorizeUrl.search = new URLSearchParams({ @@ -95,7 +101,7 @@ const buildAuthorizeUrl = ({ redirect_uri: REDIRECT_URI, code_challenge: challenge, code_challenge_method: "S256", - scope: "vault", + scope, }).toString() return authorizeUrl } @@ -105,14 +111,16 @@ const authorize = async ({ port, client, authToken, + scope = "vault", }: { port: number client: RegisteredClient authToken: string + scope?: string }): Promise => { const verifier = base64Url(randomBytes(32)) const challenge = base64Url(createHash("sha256").update(verifier).digest()) - const authorizeUrl = buildAuthorizeUrl({ port, client, challenge }) + const authorizeUrl = buildAuthorizeUrl({ port, client, challenge, scope }) const consentHtml = await (await fetch(authorizeUrl)).text() const requestId = /name="request_id"\s+value="([^"]+)"/.exec(consentHtml)?.[1] @@ -193,7 +201,7 @@ describe("consent page response headers", () => { const client = await registerClient(port) const challenge = base64Url(createHash("sha256").update("header-check").digest()) - const response = await fetch(buildAuthorizeUrl({ port, client, challenge })) + const response = await fetch(buildAuthorizeUrl({ port, client, challenge, scope: "vault" })) expect(response.status).toBe(200) expect(await response.text()).toContain("

Authorize access

") @@ -453,6 +461,33 @@ describe("refresh-token reuse detection", () => { }, 60_000) }) +// ── Granted scope ───────────────────────────────────────────── + +describe("granted scope", () => { + it("grants vault to a sign-in that asks for another scope, and keeps it on refresh", async () => { + const port = await freePort() + const server = await startServer(port, { MCP_AUTH_TOKEN: TOKEN_A }) + onTestFinished(() => server.cleanup()) + const client = await registerClient(port) + + const issued = await authorize({ port, client, authToken: TOKEN_A, scope: "admin-zq9" }) + const accessTokenPayload = verifyJwt({ + token: issued.access_token, + secret: TOKEN_A, + expectedIssuer: `http://127.0.0.1:${port}/`, + expectedAudience: `http://127.0.0.1:${port}/mcp`, + }) + const refreshed = await refresh({ port, client, refreshToken: issued.refresh_token }) + const refreshedTokens: unknown = await refreshed.json() + + if (!isIssuedTokens(refreshedTokens)) throw new Error("malformed refresh") + expect(issued.scope).toBe("vault") + expect(accessTokenPayload?.scope).toBe("vault") + expect(refreshed.status).toBe(200) + expect(refreshedTokens.scope).toBe("vault") + }, 60_000) +}) + // ── Scope-widening refresh ──────────────────────────────────── describe("scope-widening refresh", () => { diff --git a/src/vault-mcp/oauth/__tests__/consent-page.test.ts b/src/vault-mcp/oauth/__tests__/consent-page.test.ts index be5ccb0f7..cce3a7583 100644 --- a/src/vault-mcp/oauth/__tests__/consent-page.test.ts +++ b/src/vault-mcp/oauth/__tests__/consent-page.test.ts @@ -4,10 +4,103 @@ import { renderConsentPage } from "../consent-page.js" const baseParams = { clientName: "Test Client", clientId: "client-123", - scopes: ["vault"], + redirectUri: "https://claude.ai/api/mcp/auth_callback", requestId: "req-abc", } +const GRANT_TEXT = "Everything this server's tools can do in your vault" + +/** Matches a field's label div followed by its value div, capturing both texts. */ +const FIELD_ROW_PATTERN = /
([^<]*)<\/div>\s*
([^<]*)<\/div>/g + +/** Every label and value row on the page, in page order. */ +const readFieldRows = (html: string) => { + return [...html.matchAll(FIELD_ROW_PATTERN)].map((match) => ({ + label: match[1], + value: match[2], + })) +} + +const expectedRows = (destination: string): Array<{ label: string; value: string }> => [ + { label: "Application", value: "Test Client" }, + { label: "Client ID", value: "client-123" }, + { label: "Sends access to", value: destination }, + { label: "Access", value: GRANT_TEXT }, +] + +describe("consent page rows", () => { + it.each([ + { + label: "a web address as its origin", + redirectUri: "https://claude.ai/api/mcp/auth_callback", + destination: "https://claude.ai", + }, + { + label: "a loopback address with its port", + redirectUri: "http://127.0.0.1:19876/callback", + destination: "http://127.0.0.1:19876", + }, + { + label: "an IPv6 loopback address with its port", + redirectUri: "http://[::1]:9000/cb", + destination: "http://[::1]:9000", + }, + { + label: "an app scheme with a host as scheme and host", + redirectUri: "claude://claude.ai/cb", + destination: "claude://claude.ai", + }, + { + label: "an app link with no host as written", + redirectUri: "claude.ai:callback", + destination: "claude.ai:callback", + }, + { + label: "a native app callback with no host as written", + redirectUri: "com.example.app:/oauth/callback", + destination: "com.example.app:/oauth/callback", + }, + { + label: "the host after the userinfo, not the userinfo", + redirectUri: "https://claude.ai@evil.example/cb", + destination: "https://evil.example", + }, + { + label: "a host with a lookalike letter as punycode", + redirectUri: "https://clаude.ai/cb", + destination: "https://xn--clude-5ve.ai", + }, + { + label: "a lookalike subdomain in full", + redirectUri: "https://claude.ai.evil.example/cb", + destination: "https://claude.ai.evil.example", + }, + { + label: "a host holding & escaped", + redirectUri: "https://a&b.example/cb", + destination: "https://a&b.example", + }, + { + label: "an unparseable address as written, escaped", + redirectUri: "not a url ", + destination: "not a url <b>", + }, + ])("shows $label", ({ redirectUri, destination }) => { + const html = renderConsentPage({ ...baseParams, redirectUri }) + + expect(readFieldRows(html)).toEqual(expectedRows(destination)) + }) + + it("shows a 300-character host whole", () => { + const longHost = `${"a".repeat(292)}.example` + + const html = renderConsentPage({ ...baseParams, redirectUri: `https://${longHost}/cb` }) + + expect(longHost).toHaveLength(300) + expect(readFieldRows(html)).toEqual(expectedRows(`https://${longHost}`)) + }) +}) + describe("consent page reveal toggle", () => { it("renders the token field masked by default", () => { const html = renderConsentPage(baseParams) diff --git a/src/vault-mcp/oauth/__tests__/oauth-provider.test.ts b/src/vault-mcp/oauth/__tests__/oauth-provider.test.ts index 449bd710c..7db0bf665 100644 --- a/src/vault-mcp/oauth/__tests__/oauth-provider.test.ts +++ b/src/vault-mcp/oauth/__tests__/oauth-provider.test.ts @@ -1851,6 +1851,9 @@ describe("OAuth resource parameter (RFC 8707)", () => { }) describe("OAuth default scope", () => { + const GRANT_ROW_VALUE = + '
Everything this server\'s tools can do in your vault
' + /** Starts an authorization flow with the given scopes and returns the * rendered consent HTML plus the request id it carries. */ const startAuthFlowWithScopes = async ( @@ -1882,8 +1885,7 @@ describe("OAuth default scope", () => { const started = logs.find((log) => log.message === "oauth_authorize_started") expect(started?.data.scopes).toEqual(["vault"]) - expect(consentHtml).toContain("
  • vault
  • ") - expect(consentHtml).not.toContain("No specific scopes requested") + expect(consentHtml).toContain(GRANT_ROW_VALUE) const code = oauth.approveRequest(requestId, testLogger) const issued = await oauth.provider.exchangeAuthorizationCode(client, code) @@ -1904,8 +1906,7 @@ describe("OAuth default scope", () => { const started = logs.find((log) => log.message === "oauth_authorize_started") expect(started?.data.scopes).toEqual(["vault"]) - expect(consentHtml).toContain("
  • vault
  • ") - expect(consentHtml).not.toContain("
  • ") + expect(consentHtml).toContain(GRANT_ROW_VALUE) const code = oauth.approveRequest(requestId, testLogger) const issued = await oauth.provider.exchangeAuthorizationCode(client, code) @@ -1919,36 +1920,47 @@ describe("OAuth default scope", () => { const started = logs.find((log) => log.message === "oauth_authorize_started") expect(started?.data.scopes).toEqual(["vault"]) - expect(consentHtml).toContain("
  • vault
  • ") + expect(consentHtml).toContain(GRANT_ROW_VALUE) const code = oauth.approveRequest(requestId, testLogger) const issued = await oauth.provider.exchangeAuthorizationCode(client, code) expect(issued.scope).toBe("vault") }) - it("does not widen a narrower explicit scope to vault", async () => { - const { oauth, testLogger, client } = await setupAuditTest() + it("grants vault when the request names another scope", async () => { + const { logs, oauth, testLogger, client } = await setupAuditTest() - const { consentHtml, requestId } = await startAuthFlowWithScopes(oauth, client, ["read"]) + const { requestId } = await startAuthFlowWithScopes(oauth, client, ["read"]) - expect(consentHtml).toContain("
  • read
  • ") - expect(consentHtml).not.toContain("
  • vault
  • ") + const started = logs.find((log) => log.message === "oauth_authorize_started") + expect(started?.data.scopes).toEqual(["vault"]) const code = oauth.approveRequest(requestId, testLogger) const issued = await oauth.provider.exchangeAuthorizationCode(client, code) - expect(issued.scope).toBe("read") + expect(issued.scope).toBe("vault") + const verified = await oauth.provider.verifyAccessToken(issued.access_token) + expect(verified.scopes).toEqual(["vault"]) + }) + + it("leaves the scope a request names off the consent page", async () => { + const { oauth, client } = await setupAuditTest() + + const { consentHtml } = await startAuthFlowWithScopes(oauth, client, ["admin-zq9"]) + + expect(consentHtml).toContain('
    https://example.com
    ') + expect(consentHtml).toContain(GRANT_ROW_VALUE) + expect(consentHtml).not.toContain("admin-zq9") }) it("grants vault when the scopes key is absent from the request", async () => { const { logs, testLogger, oauth, client } = await setupAuditTest() - // When the SDK receives no `scope` parameter at all, params.scopes is - // undefined — the ?? [] nullish guard in authorize handles this. + // When the SDK receives no `scope` parameter at all, params.scopes is undefined. const { consentHtml, requestId } = await startAuthFlowWithScopes(oauth, client, undefined) const started = logs.find((log) => log.message === "oauth_authorize_started") expect(started?.data.scopes).toEqual(["vault"]) - expect(consentHtml).toContain("
  • vault
  • ") + expect(consentHtml).toContain(GRANT_ROW_VALUE) const code = oauth.approveRequest(requestId, testLogger) const issued = await oauth.provider.exchangeAuthorizationCode(client, code) diff --git a/src/vault-mcp/oauth/__tests__/oauth-routes.test.ts b/src/vault-mcp/oauth/__tests__/oauth-routes.test.ts index 7d77b0f3a..9239aa1e1 100644 --- a/src/vault-mcp/oauth/__tests__/oauth-routes.test.ts +++ b/src/vault-mcp/oauth/__tests__/oauth-routes.test.ts @@ -46,6 +46,9 @@ const recordingLogger = (sink: LogCall[]): Logger => { const AUTH_TOKEN = "local-dev-token" const TEST_URLS = { serverUrl: new URL("http://localhost:8000") } const REDIRECT_URI = "http://localhost:9999/callback" +// What the consent page shows for REDIRECT_URI and for the grant +const REDIRECT_DESTINATION = "http://localhost:9999" +const GRANT_TEXT = "Everything this server's tools can do in your vault" // A test request's peer address is loopback, in a form that varies by // platform (127.0.0.1, ::1, or IPv4-mapped). const LOOPBACK_ADDRESSES = ["127.0.0.1", "::1", "::ffff:127.0.0.1"] @@ -197,7 +200,6 @@ describe("OAuth consent token submission", () => { return { error: /
    ([^<]*)<\/div>/.exec(html)?.[1], values: [...html.matchAll(/
    ([^<]*)<\/div>/g)].map((match) => match[1]), - scopes: [...html.matchAll(/
  • ([^<]*)<\/li>/g)].map((match) => match[1]), requestId: REQUEST_ID_PATTERN.exec(html)?.[1], } } @@ -212,24 +214,24 @@ describe("OAuth consent token submission", () => { expect(response.headers.get("cache-control")).toBe("no-store") expect(readConsentPage(await response.text())).toEqual({ error: "Invalid token. Please try again.", - values: ["Test Client", clientId], - scopes: ["vault"], + values: ["Test Client", clientId, REDIRECT_DESTINATION, GRANT_TEXT], requestId, }) }, ) - it("re-renders the consent page with the default scope after a wrong token when the request named none", async () => { - const { requestId, clientId } = await startPendingRequest([]) + it("re-renders the consent page without the scope the request named after a wrong token", async () => { + const { requestId, clientId } = await startPendingRequest(["admin-zq9"]) const response = await submitToken(requestId, "not-the-token") + const html = await response.text() - expect(readConsentPage(await response.text())).toEqual({ + expect(readConsentPage(html)).toEqual({ error: "Invalid token. Please try again.", - values: ["Test Client", clientId], - scopes: ["vault"], + values: ["Test Client", clientId, REDIRECT_DESTINATION, GRANT_TEXT], requestId, }) + expect(html).not.toContain("admin-zq9") }) }) diff --git a/src/vault-mcp/oauth/consent-page.ts b/src/vault-mcp/oauth/consent-page.ts index c3ab40dfb..d4d20a018 100644 --- a/src/vault-mcp/oauth/consent-page.ts +++ b/src/vault-mcp/oauth/consent-page.ts @@ -5,22 +5,34 @@ import { escapeHtml } from "../../utils/escape-html.js" type ConsentPageParams = { clientName: string clientId: string - scopes: string[] + redirectUri: string requestId: string error?: string } +// A new sign-in always gets the vault scope, and no tool checks a scope, so +// approval grants whatever the tools allow. +const GRANT_DESCRIPTION = "Everything this server's tools can do in your vault" + +/** Where Approve sends access, as the page shows it: + * - an address with a host: its scheme and host, port included, so only a + * web address starts with `https://`; + * - an address with no host, such as the app link `claude.ai:callback`, or + * one URL.parse cannot read: the address as written. */ +const redirectUriToDestination = (redirectUri: string): string => { + const redirectUrl = URL.parse(redirectUri) + + if (!redirectUrl?.host) return redirectUri + return `${redirectUrl.protocol}//${redirectUrl.host}` +} + export const renderConsentPage = ({ clientName, clientId, - scopes, + redirectUri, requestId, error, }: ConsentPageParams): string => { - const scopeList = scopes.length - ? scopes.map((scope) => `
  • ${escapeHtml(scope)}
  • `).join("") - : "
  • No specific scopes requested
  • " - return ` @@ -35,9 +47,6 @@ export const renderConsentPage = ({ .field{margin-bottom:1rem} .label{font-size:.75rem;color:#a1a1aa;text-transform:uppercase;letter-spacing:.05em;margin-bottom:.25rem} .value{font-size:.9rem;color:#e4e4e7;word-break:break-all} - ul{list-style:none;padding:0} - ul li{padding:.25rem 0;font-size:.9rem} - ul li::before{content:"\\2022";color:#6366f1;margin-right:.5rem} .token-input{display:flex;gap:.5rem;margin-top:.25rem} .token-input input{flex:1;min-width:0;padding:.6rem .75rem;background:#0f1117;border:1px solid #2e2e38;border-radius:6px;color:#fafafa;font-size:.9rem;font-family:ui-monospace,SFMono-Regular,Menlo,monospace} .token-input input:focus{outline:none;border-color:#6366f1} @@ -65,8 +74,12 @@ export const renderConsentPage = ({
    ${escapeHtml(clientId)}
    -
    Requested scopes
    -
      ${scopeList}
    +
    Sends access to
    +
    ${escapeHtml(redirectUriToDestination(redirectUri))}
    +
    +
    +
    Access
    +
    ${escapeHtml(GRANT_DESCRIPTION)}
    diff --git a/src/vault-mcp/oauth/oauth-provider.ts b/src/vault-mcp/oauth/oauth-provider.ts index c7c3f7a04..eb9a3f7d7 100644 --- a/src/vault-mcp/oauth/oauth-provider.ts +++ b/src/vault-mcp/oauth/oauth-provider.ts @@ -57,12 +57,12 @@ const MAX_PENDING_REQUEST_PARAMS_BYTES = 8192 // Every rejected code gets this one message, so a code issued to another // client looks the same as an unknown one. const INVALID_AUTH_CODE_MESSAGE = "Authorization code expired or invalid" -// The scope granted to a request that names none (see authorize); also what -// the static token carries (see verifyAccessToken) and what the metadata -// advertises as scopes_supported. +// The scope every new sign-in is granted (see authorize); also what the static +// token carries (see verifyAccessToken) and what the metadata advertises as +// scopes_supported. export const DEFAULT_SCOPE = "vault" as const -/** `authorize` fills in the default scope before storing a request, so scopes is always set. */ +/** `authorize` sets the granted scope before storing a request, so scopes is always set. */ type GrantedAuthorizationParams = AuthorizationParams & { scopes: string[] } type PendingAuthRequest = { @@ -589,10 +589,9 @@ export const createOAuthProvider = ({ return store }, - /** A request that names no scope is granted DEFAULT_SCOPE; named scopes - * are granted as requested. The default is applied here, before the - * request is stored, so the consent page, the code exchange, and every - * later refresh read the same value. */ + /** Every request is granted DEFAULT_SCOPE, whatever scope it names. The + * grant is set here, before the request is stored, so the code exchange + * and every later refresh read the same value. */ async authorize( client: OAuthClientInformationFull, params: AuthorizationParams, @@ -606,9 +605,7 @@ export const createOAuthProvider = ({ // redirect the client with error=invalid_target. assertResourceIsThisServer(params.resource, client.client_id) const requestId = randomUUID() - // The SDK splits `scope=` into [""], so blank entries count as none. - const requestedScopes = (params.scopes ?? []).filter((scope) => scope !== "") - const scopes = requestedScopes.length > 0 ? requestedScopes : [DEFAULT_SCOPE] + const scopes = [DEFAULT_SCOPE] const grantedParams = { ...params, scopes } const paramsBytes = Buffer.byteLength(JSON.stringify(grantedParams)) @@ -636,7 +633,7 @@ export const createOAuthProvider = ({ renderConsentPage({ clientName: client.client_name ?? client.client_id, clientId: client.client_id, - scopes, + redirectUri: params.redirectUri, requestId, }), ) diff --git a/src/vault-mcp/oauth/oauth-routes.ts b/src/vault-mcp/oauth/oauth-routes.ts index d9d1f273f..374594b83 100644 --- a/src/vault-mcp/oauth/oauth-routes.ts +++ b/src/vault-mcp/oauth/oauth-routes.ts @@ -202,7 +202,7 @@ export const createOAuthRoutes = ({ renderConsentPage({ clientName: pending.client.client_name ?? pending.client.client_id, clientId, - scopes: pending.params.scopes, + redirectUri: pending.params.redirectUri, requestId: request_id, error: "Invalid token. Please try again.", }), From 63f79e04bed33c8bc2cadf737298891711c1f90f Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 7 Nov 2026 15:16:38 -0500 Subject: [PATCH 04/18] docs: describe the consent page's destination row, the granted scope and the response headers Co-Authored-By: Claude Opus 5.5 --- ARCHITECTURE.md | 10 ++++++---- SECURITY.md | 10 ++++++++-- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 600f6f82b..646f39d83 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -785,10 +785,12 @@ cryptographic or binding checks. The binding claims is fine. A mismatch is answered with `invalid_target` before any code or refresh token is consumed; clients that send no `resource` are accepted. -`vault` is the only scope the server advertises: a client that requests it -gets it, and a client that requests no scope is granted it at authorization -time, so the consent page, the access token, and every refresh carry the same -value as the static token. +`vault` is the only scope the server advertises. Every new sign-in is granted +it at authorization time, whatever scope the client requests, so the access +token and every refresh carry the same value as the static token. A refresh +grant stored with another scope keeps that scope. The consent page shows where +Approve sends access (the redirect address's scheme and host, or the whole +address when it has no host) and a fixed description of the grant. **Token storage:** what each credential is and where it lives. diff --git a/SECURITY.md b/SECURITY.md index 957f7124f..477b4caf6 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -136,8 +136,14 @@ mechanism-level detail. untrusted content could reach an LLM context - **XSS:** `escapeHtml()` on the OAuth consent page and the setup pages escapes `&`, `<`, `>`, `"` in every interpolated value (client name, - client ID, scopes, error messages, request IDs, the Obsidian account - name, vault names, API error text) + client ID, the address approval sends access to, error messages, request + IDs, the Obsidian account name, vault names, API error text) +- **Framing and response headers:** every response from both servers carries + `X-Frame-Options: DENY` and `Content-Security-Policy: frame-ancestors 'none'` + (Express's own 404 page replaces the latter), so no other site can frame the + consent or setup pages. Responses also carry `X-Content-Type-Options: nosniff` + and `Referrer-Policy: same-origin`, never `X-Powered-By`. Consent decisions + and setup-mode responses send `Cache-Control: no-store` ### Data corruption prevention From b7cbbbb96191ffd63f763f1bd58241648946c158 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 7 Nov 2026 15:16:55 -0500 Subject: [PATCH 05/18] test: use neutral example hosts in the consent page destination vectors Co-Authored-By: Claude Opus 5.5 --- src/vault-mcp/oauth/__tests__/consent-page.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/vault-mcp/oauth/__tests__/consent-page.test.ts b/src/vault-mcp/oauth/__tests__/consent-page.test.ts index cce3a7583..50321d66d 100644 --- a/src/vault-mcp/oauth/__tests__/consent-page.test.ts +++ b/src/vault-mcp/oauth/__tests__/consent-page.test.ts @@ -62,8 +62,8 @@ describe("consent page rows", () => { }, { label: "the host after the userinfo, not the userinfo", - redirectUri: "https://claude.ai@evil.example/cb", - destination: "https://evil.example", + redirectUri: "https://claude.ai@other.example/cb", + destination: "https://other.example", }, { label: "a host with a lookalike letter as punycode", @@ -72,8 +72,8 @@ describe("consent page rows", () => { }, { label: "a lookalike subdomain in full", - redirectUri: "https://claude.ai.evil.example/cb", - destination: "https://claude.ai.evil.example", + redirectUri: "https://claude.ai.other.example/cb", + destination: "https://claude.ai.other.example", }, { label: "a host holding & escaped", From 57aec8485f8fde24766e38dc5836bf74b6fc6eff Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 7 Nov 2026 15:22:10 -0500 Subject: [PATCH 06/18] docs: give the response headers their own SECURITY.md section Co-Authored-By: Claude Opus 5.5 --- SECURITY.md | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index 477b4caf6..9ad36da64 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -138,12 +138,20 @@ mechanism-level detail. escapes `&`, `<`, `>`, `"` in every interpolated value (client name, client ID, the address approval sends access to, error messages, request IDs, the Obsidian account name, vault names, API error text) -- **Framing and response headers:** every response from both servers carries - `X-Frame-Options: DENY` and `Content-Security-Policy: frame-ancestors 'none'` - (Express's own 404 page replaces the latter), so no other site can frame the - consent or setup pages. Responses also carry `X-Content-Type-Options: nosniff` - and `Referrer-Policy: same-origin`, never `X-Powered-By`. Consent decisions - and setup-mode responses send `Cache-Control: no-store` + +### Response headers + +Both servers set these headers: + +- **Framing** — every response carries `X-Frame-Options: DENY` and + `Content-Security-Policy: frame-ancestors 'none'`, so no other site can + frame the consent or setup pages. Express's own 404 page replaces the + content policy; `X-Frame-Options` still applies there +- **Content type and referrer** — every response carries + `X-Content-Type-Options: nosniff` and `Referrer-Policy: same-origin`, and + none carries `X-Powered-By` +- **Caching** — consent decisions and every setup-mode response send + `Cache-Control: no-store` ### Data corruption prevention From 49ce00a71fda2ab951f7893e743e74963dc33c3e Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 7 Nov 2026 15:22:32 -0500 Subject: [PATCH 07/18] docs: name the server and setup mode in the response headers section Co-Authored-By: Claude Opus 5.5 --- SECURITY.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/SECURITY.md b/SECURITY.md index 9ad36da64..1a9ca326b 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -141,7 +141,7 @@ mechanism-level detail. ### Response headers -Both servers set these headers: +The server, and the `:remote` image's setup mode, set these headers: - **Framing** — every response carries `X-Frame-Options: DENY` and `Content-Security-Policy: frame-ancestors 'none'`, so no other site can From fdb9e03e6074f4a67ee604a24dc2551b329c60a0 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 7 Nov 2026 15:31:58 -0500 Subject: [PATCH 08/18] fix(review): describe the consent decision's parser-error response accurately MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The server's error handler answers a form parser error with a 500, so the comment no longer names a 413. Ship-Check: pr-review · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- src/vault-mcp/oauth/oauth-routes.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/vault-mcp/oauth/oauth-routes.ts b/src/vault-mcp/oauth/oauth-routes.ts index 374594b83..8575ebd3b 100644 --- a/src/vault-mcp/oauth/oauth-routes.ts +++ b/src/vault-mcp/oauth/oauth-routes.ts @@ -135,7 +135,8 @@ export const createOAuthRoutes = ({ // No consent decision response may be cached: the re-rendered page holds the // request ID, and the redirect holds the authorization code. Mounted ahead of - // the form parser so its errors, such as a 413 for an oversized form, carry it too. + // the form parser so the response to a parser error, such as an oversized + // form, carries it too. router.use("/oauth/decide", (_req: Request, res: Response, next: NextFunction) => { res.set("Cache-Control", "no-store") next() From 731b86d61a11b1525ad13200890b91da94ff929c Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 7 Nov 2026 15:44:34 -0500 Subject: [PATCH 09/18] style: clarify the response-header, consent-page and setup-server comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Scope the "mounted first" comments in both apps to what mounting first does, and name the consent and setup pages in the content-policy comment. - Say why the consent page always shows the scheme, why it does not use URL.origin, and that the SDK passes only addresses URL.parse can read. - State that `vault` is the server's only scope, and that any consent action but Approve denies. - Point setup-server at the scripts that set SETUP_REASON and derive PUBLIC_URL, and say why it keeps its own error middleware. - SECURITY.md: name the redirect address plainly, say which 404 page sets its own content policy, and list every page that sends no-store. - ARCHITECTURE.md: say where a refresh grant with another scope comes from. Ship-Check: code-quality · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- ARCHITECTURE.md | 3 ++- SECURITY.md | 13 +++++++------ src/utils/security-headers.ts | 3 ++- src/vault-mcp/oauth/consent-page.ts | 11 +++++++---- src/vault-mcp/oauth/oauth-provider.ts | 6 +++--- src/vault-mcp/oauth/oauth-routes.ts | 2 ++ src/vault-mcp/server.ts | 3 ++- src/vault-mcp/setup/setup-server.ts | 10 +++++++--- 8 files changed, 32 insertions(+), 19 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 646f39d83..0547bb4fd 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -788,7 +788,8 @@ cryptographic or binding checks. The binding claims `vault` is the only scope the server advertises. Every new sign-in is granted it at authorization time, whatever scope the client requests, so the access token and every refresh carry the same value as the static token. A refresh -grant stored with another scope keeps that scope. The consent page shows where +grant that an earlier version stored with another scope keeps that scope. The +consent page shows where Approve sends access (the redirect address's scheme and host, or the whole address when it has no host) and a fixed description of the grant. diff --git a/SECURITY.md b/SECURITY.md index 1a9ca326b..9eb9764d1 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -136,8 +136,8 @@ mechanism-level detail. untrusted content could reach an LLM context - **XSS:** `escapeHtml()` on the OAuth consent page and the setup pages escapes `&`, `<`, `>`, `"` in every interpolated value (client name, - client ID, the address approval sends access to, error messages, request - IDs, the Obsidian account name, vault names, API error text) + client ID, the redirect address, error messages, request IDs, the + Obsidian account name, vault names, API error text) ### Response headers @@ -145,13 +145,14 @@ The server, and the `:remote` image's setup mode, set these headers: - **Framing** — every response carries `X-Frame-Options: DENY` and `Content-Security-Policy: frame-ancestors 'none'`, so no other site can - frame the consent or setup pages. Express's own 404 page replaces the - content policy; `X-Frame-Options` still applies there + frame the consent or setup pages. The server's 404 for an unknown address + is Express's built-in page, which replaces that content policy with its + own; `X-Frame-Options` still applies there - **Content type and referrer** — every response carries `X-Content-Type-Options: nosniff` and `Referrer-Policy: same-origin`, and none carries `X-Powered-By` -- **Caching** — consent decisions and every setup-mode response send - `Cache-Control: no-store` +- **Caching** — the consent page, its decisions, the server's `/setup` page + and every setup-mode response send `Cache-Control: no-store` ### Data corruption prevention diff --git a/src/utils/security-headers.ts b/src/utils/security-headers.ts index 373bb9a77..7312cc6e7 100644 --- a/src/utils/security-headers.ts +++ b/src/utils/security-headers.ts @@ -3,7 +3,8 @@ import type { NextFunction, Request, Response } from "express" const SECURITY_HEADERS = { // Browsers that predate the frame-ancestors directive read only this header. "X-Frame-Options": "DENY", - // frame-ancestors alone: a fuller policy would block the pages' inline script and styles. + // The policy names only frame-ancestors, because a fuller one would block the + // consent and setup pages' inline script and styles. "Content-Security-Policy": "frame-ancestors 'none'", "X-Content-Type-Options": "nosniff", // Sends no referrer to other sites. no-referrer would also turn a page's own diff --git a/src/vault-mcp/oauth/consent-page.ts b/src/vault-mcp/oauth/consent-page.ts index d4d20a018..bc92b0fe5 100644 --- a/src/vault-mcp/oauth/consent-page.ts +++ b/src/vault-mcp/oauth/consent-page.ts @@ -15,14 +15,17 @@ type ConsentPageParams = { const GRANT_DESCRIPTION = "Everything this server's tools can do in your vault" /** Where Approve sends access, as the page shows it: - * - an address with a host: its scheme and host, port included, so only a - * web address starts with `https://`; - * - an address with no host, such as the app link `claude.ai:callback`, or - * one URL.parse cannot read: the address as written. */ + * - an address with a host: its scheme and host, port included. The scheme + * always shows, so an app link never reads as a website. + * - an address with no host, such as the app link `claude.ai:callback`: the + * address as written. + * The SDK passes only addresses URL.parse can read; one it cannot would also + * show as written. */ const redirectUriToDestination = (redirectUri: string): string => { const redirectUrl = URL.parse(redirectUri) if (!redirectUrl?.host) return redirectUri + // Not URL.origin, which is "null" for an app scheme such as claude://. return `${redirectUrl.protocol}//${redirectUrl.host}` } diff --git a/src/vault-mcp/oauth/oauth-provider.ts b/src/vault-mcp/oauth/oauth-provider.ts index eb9a3f7d7..494784822 100644 --- a/src/vault-mcp/oauth/oauth-provider.ts +++ b/src/vault-mcp/oauth/oauth-provider.ts @@ -57,9 +57,9 @@ const MAX_PENDING_REQUEST_PARAMS_BYTES = 8192 // Every rejected code gets this one message, so a code issued to another // client looks the same as an unknown one. const INVALID_AUTH_CODE_MESSAGE = "Authorization code expired or invalid" -// The scope every new sign-in is granted (see authorize); also what the static -// token carries (see verifyAccessToken) and what the metadata advertises as -// scopes_supported. +// The server has only this scope. Every new sign-in is granted it (see +// authorize), the static token carries it (see verifyAccessToken), and the +// metadata advertises it as scopes_supported. export const DEFAULT_SCOPE = "vault" as const /** `authorize` sets the granted scope before storing a request, so scopes is always set. */ diff --git a/src/vault-mcp/oauth/oauth-routes.ts b/src/vault-mcp/oauth/oauth-routes.ts index 8575ebd3b..17fa57b64 100644 --- a/src/vault-mcp/oauth/oauth-routes.ts +++ b/src/vault-mcp/oauth/oauth-routes.ts @@ -178,6 +178,8 @@ export const createOAuthRoutes = ({ clientId, }) + // Any action but an explicit Approve denies, so an unexpected form + // value can never grant access. if (action !== "approve") { consentLogger.info("oauth_consent_denied_by_user") deletePendingRequest(request_id) diff --git a/src/vault-mcp/server.ts b/src/vault-mcp/server.ts index 9c3a40d0a..3dc872a6e 100644 --- a/src/vault-mcp/server.ts +++ b/src/vault-mcp/server.ts @@ -220,7 +220,8 @@ const startServer = async (): Promise => { }) const app = express() - // First, so every response carries the headers, errors and redirects included. + // First, so a response that later middleware ends early, such as an error + // or a redirect, carries the headers too. app.use(securityHeaders) app.disable("x-powered-by") // Proxy trust is deployment-explicit: TRUST_PROXY_HOPS grants one diff --git a/src/vault-mcp/setup/setup-server.ts b/src/vault-mcp/setup/setup-server.ts index ee9767933..103f82517 100644 --- a/src/vault-mcp/setup/setup-server.ts +++ b/src/vault-mcp/setup/setup-server.ts @@ -17,8 +17,8 @@ import type { HostingPlatform } from "./setup-page.js" /** Which container hosting platform the page is served from, read from the * variable each platform sets on its services. Checked in the same order - * as print-derived-env's PUBLIC_URL derivation, so the page never names a - * different platform than the one the URL came from. */ + * as the PUBLIC_URL derivation in rootfs/etc/s6-overlay/scripts/print-derived-env, + * so the page never names a different platform than the one the URL came from. */ const detectHostingPlatform = (): HostingPlatform | undefined => { if (env.get("RENDER_EXTERNAL_URL").asString()?.trim()) return "render" if (env.get("RAILWAY_PUBLIC_DOMAIN").asString()?.trim()) return "railway" @@ -48,13 +48,15 @@ const startSetupServer = (): void => { .asUrlString() const vaultName = env.get("VAULT_NAME").default("").asString().trim() const vaultPassword = env.get("VAULT_PASSWORD").asString() || undefined + // init-obsidian-login sets this value when Obsidian rejects the saved login. const savedLoginRejected = env.get("SETUP_REASON").default("").asString() === "login-failed" const hostingPlatform = detectHostingPlatform() const setupUrl = publicUrl ? new URL("/setup", publicUrl).href : "/setup" const app = express() - // First, so every response carries the headers, errors and redirects included. + // First, so a response that later middleware ends early, such as an error + // or a redirect, carries the headers too. app.use(securityHeaders) app.disable("x-powered-by") // Setup mode serves sign-in forms and their results, so no response may be cached. @@ -104,6 +106,8 @@ const startSetupServer = (): void => { res.status(503).json({ error: "setup required", setup_url: setupUrl }) }) + // Not server.ts's createErrorMiddleware, because importing server.ts would + // load the index, OAuth and MCP modules that setup mode never runs. app.use((err: Error, req: Request, res: Response, _next: NextFunction) => { logger.error("unhandled_error", { method: req.method, From 2ad0bd6d5abf8aaf854550e1cdba61cef2d2ce11 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 7 Nov 2026 15:47:41 -0500 Subject: [PATCH 10/18] refactor(oauth): let the consent page own its name fallback, and name the reveal toggle's variables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The page shows the client ID when a client has no name; both render paths passed the same fallback before. The Show/Hide toggle's inline script on the consent and setup pages uses descriptive names. The Referrer-Policy comment says what a null Origin would hide. Ship-Check: triage · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- src/utils/security-headers.ts | 4 ++-- src/vault-mcp/oauth/__tests__/consent-page.test.ts | 11 ++++++++++- src/vault-mcp/oauth/consent-page.ts | 7 ++++--- src/vault-mcp/oauth/oauth-provider.ts | 2 +- src/vault-mcp/oauth/oauth-routes.ts | 2 +- src/vault-mcp/setup/setup-page.ts | 2 +- 6 files changed, 19 insertions(+), 9 deletions(-) diff --git a/src/utils/security-headers.ts b/src/utils/security-headers.ts index 7312cc6e7..f2be28888 100644 --- a/src/utils/security-headers.ts +++ b/src/utils/security-headers.ts @@ -7,8 +7,8 @@ const SECURITY_HEADERS = { // consent and setup pages' inline script and styles. "Content-Security-Policy": "frame-ancestors 'none'", "X-Content-Type-Options": "nosniff", - // Sends no referrer to other sites. no-referrer would also turn a page's own - // form posts into `Origin: null`. + // Sends no referrer to other sites. no-referrer would also make the consent + // form's post arrive with `Origin: null`, hiding which site sent it. "Referrer-Policy": "same-origin", } diff --git a/src/vault-mcp/oauth/__tests__/consent-page.test.ts b/src/vault-mcp/oauth/__tests__/consent-page.test.ts index 50321d66d..67a24a157 100644 --- a/src/vault-mcp/oauth/__tests__/consent-page.test.ts +++ b/src/vault-mcp/oauth/__tests__/consent-page.test.ts @@ -99,6 +99,15 @@ describe("consent page rows", () => { expect(longHost).toHaveLength(300) expect(readFieldRows(html)).toEqual(expectedRows(`https://${longHost}`)) }) + + it("shows the client ID as the application when the client has no name", () => { + const html = renderConsentPage({ ...baseParams, clientName: undefined }) + + expect(readFieldRows(html)).toEqual([ + { label: "Application", value: "client-123" }, + ...expectedRows("https://claude.ai").slice(1), + ]) + }) }) describe("consent page reveal toggle", () => { @@ -120,7 +129,7 @@ describe("consent page reveal toggle", () => { // The toggle must target the input by its id and flip it to a visible // type — otherwise "Show" silently does nothing. expect(onclick).toBe( - "var t=document.getElementById('token');var s=t.type==='password';t.type=s?'text':'password';this.textContent=s?'Hide':'Show'", + "var tokenInput=document.getElementById('token');var hidden=tokenInput.type==='password';tokenInput.type=hidden?'text':'password';this.textContent=hidden?'Hide':'Show'", ) }) }) diff --git a/src/vault-mcp/oauth/consent-page.ts b/src/vault-mcp/oauth/consent-page.ts index bc92b0fe5..36213a377 100644 --- a/src/vault-mcp/oauth/consent-page.ts +++ b/src/vault-mcp/oauth/consent-page.ts @@ -3,7 +3,8 @@ import { escapeHtml } from "../../utils/escape-html.js" type ConsentPageParams = { - clientName: string + /** The client's registered name; the page shows its ID when it has none. */ + clientName: string | undefined clientId: string redirectUri: string requestId: string @@ -70,7 +71,7 @@ export const renderConsentPage = ({ ${error ? `
    ${escapeHtml(error)}
    ` : ""}
    Application
    -
    ${escapeHtml(clientName)}
    +
    ${escapeHtml(clientName ?? clientId)}
    Client ID
    @@ -90,7 +91,7 @@ export const renderConsentPage = ({
    Server token
    - +
    diff --git a/src/vault-mcp/oauth/oauth-provider.ts b/src/vault-mcp/oauth/oauth-provider.ts index 494784822..8abe9adad 100644 --- a/src/vault-mcp/oauth/oauth-provider.ts +++ b/src/vault-mcp/oauth/oauth-provider.ts @@ -631,7 +631,7 @@ export const createOAuthProvider = ({ }) res.type("html").send( renderConsentPage({ - clientName: client.client_name ?? client.client_id, + clientName: client.client_name, clientId: client.client_id, redirectUri: params.redirectUri, requestId, diff --git a/src/vault-mcp/oauth/oauth-routes.ts b/src/vault-mcp/oauth/oauth-routes.ts index 17fa57b64..aad77cdb6 100644 --- a/src/vault-mcp/oauth/oauth-routes.ts +++ b/src/vault-mcp/oauth/oauth-routes.ts @@ -203,7 +203,7 @@ export const createOAuthRoutes = ({ consentLogger.warn("oauth_consent_bad_token") res.type("html").send( renderConsentPage({ - clientName: pending.client.client_name ?? pending.client.client_id, + clientName: pending.client.client_name, clientId, redirectUri: pending.params.redirectUri, requestId: request_id, diff --git a/src/vault-mcp/setup/setup-page.ts b/src/vault-mcp/setup/setup-page.ts index b408eba5c..75f270ffd 100644 --- a/src/vault-mcp/setup/setup-page.ts +++ b/src/vault-mcp/setup/setup-page.ts @@ -114,7 +114,7 @@ const tokenField = (settingsLocationPhrase: string): string => `
    MCP token
    - +
    The MCP_AUTH_TOKEN value from ${settingsLocationPhrase} — it proves this is your server.
    ` From 85acbcc3f8f05f5a7e427a2cb70b0fee16807b51 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 7 Nov 2026 15:55:42 -0500 Subject: [PATCH 11/18] test: name each caching and header test for the behaviour it checks, and pin the setup page's reveal toggle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The consent decision's no-store checks move out of the approve, reject and 400 tests into their own cases, so a missing header fails tests named for caching. - The 404 page's framing check moves from the root-redirect test into the security headers block. - "keeps the scope a request names" becomes "grants vault when the request names vault", matching the fixed grant. - The setup page's Show/Hide handler gets the pin the consent page's has. Ship-Check: test-audit · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- .../integration/server-integration.test.ts | 18 ++++++-- .../oauth/__tests__/oauth-provider.test.ts | 2 +- .../oauth/__tests__/oauth-routes.test.ts | 44 ++++++++++++++----- .../setup/__tests__/setup-page.test.ts | 11 +++++ 4 files changed, 59 insertions(+), 16 deletions(-) diff --git a/src/__tests__/integration/server-integration.test.ts b/src/__tests__/integration/server-integration.test.ts index c08c020c2..a46b9e302 100644 --- a/src/__tests__/integration/server-integration.test.ts +++ b/src/__tests__/integration/server-integration.test.ts @@ -1419,14 +1419,24 @@ describe("default config", () => { }) expect(response.status).toBe(404) - // Express's 404 page replaces Content-Security-Policy with a policy of - // its own, so X-Frame-Options alone forbids framing it. - expect(response.headers.get("x-frame-options")).toBe("DENY") - expect(response.headers.get("x-powered-by")).toBeNull() }) }) describe("security headers", () => { + it("forbids framing Express's 404 page, without X-Powered-By", async () => { + const response = await fetch(`http://127.0.0.1:${port}/`, { + headers: { accept: "text/html,application/xhtml+xml" }, + }) + + expect(response.status).toBe(404) + // Express's 404 page replaces Content-Security-Policy with a policy of + // its own, so X-Frame-Options alone forbids framing it. + expect({ + frameOptions: response.headers.get("x-frame-options"), + poweredBy: response.headers.get("x-powered-by"), + }).toEqual({ frameOptions: "DENY", poweredBy: null }) + }) + it.each([ { label: "/healthz", path: "/healthz", init: {}, expectedStatus: 200 }, { diff --git a/src/vault-mcp/oauth/__tests__/oauth-provider.test.ts b/src/vault-mcp/oauth/__tests__/oauth-provider.test.ts index 7db0bf665..77c3939cd 100644 --- a/src/vault-mcp/oauth/__tests__/oauth-provider.test.ts +++ b/src/vault-mcp/oauth/__tests__/oauth-provider.test.ts @@ -1913,7 +1913,7 @@ describe("OAuth default scope", () => { expect(issued.scope).toBe("vault") }) - it("keeps the scope a request names", async () => { + it("grants vault when the request names vault", async () => { const { logs, testLogger, oauth, client } = await setupAuditTest() const { consentHtml, requestId } = await startAuthFlowWithScopes(oauth, client, ["vault"]) diff --git a/src/vault-mcp/oauth/__tests__/oauth-routes.test.ts b/src/vault-mcp/oauth/__tests__/oauth-routes.test.ts index 9239aa1e1..2a4029325 100644 --- a/src/vault-mcp/oauth/__tests__/oauth-routes.test.ts +++ b/src/vault-mcp/oauth/__tests__/oauth-routes.test.ts @@ -174,7 +174,6 @@ describe("OAuth consent token submission", () => { const { requestId } = await startPendingRequest() const response = await submitToken(requestId, token) expect(response.status).toBe(302) - expect(response.headers.get("cache-control")).toBe("no-store") const locationHeader = response.headers.get("location") if (!locationHeader) throw new Error("expected Location header on 302") @@ -211,7 +210,6 @@ describe("OAuth consent token submission", () => { const response = await submitToken(requestId, token) expect(response.status).toBe(200) expect(response.headers.get("location")).toBeNull() - expect(response.headers.get("cache-control")).toBe("no-store") expect(readConsentPage(await response.text())).toEqual({ error: "Invalid token. Please try again.", values: ["Test Client", clientId, REDIRECT_DESTINATION, GRANT_TEXT], @@ -233,6 +231,18 @@ describe("OAuth consent token submission", () => { }) expect(html).not.toContain("admin-zq9") }) + + it.each([ + { label: "approval redirect", token: AUTH_TOKEN, expectedStatus: 302 }, + { label: "wrong-token re-render", token: "not-the-token", expectedStatus: 200 }, + ])("marks the $label as not cacheable", async ({ token, expectedStatus }) => { + const { requestId } = await startPendingRequest() + + const response = await submitToken(requestId, token) + + expect(response.status).toBe(expectedStatus) + expect(response.headers.get("cache-control")).toBe("no-store") + }) }) describe("OAuth consent body validation", () => { @@ -276,7 +286,6 @@ describe("OAuth consent body validation", () => { body: "request_id=a&request_id=b&token=t&action=approve", }) expect(response.status).toBe(400) - expect(response.headers.get("cache-control")).toBe("no-store") const text = await response.text() expect(text).toBe("Invalid form submission.") }) @@ -288,7 +297,6 @@ describe("OAuth consent body validation", () => { body: "request_id=a", }) expect(response.status).toBe(400) - expect(response.headers.get("cache-control")).toBe("no-store") const text = await response.text() expect(text).toBe("Invalid form submission.") }) @@ -301,21 +309,35 @@ describe("OAuth consent body validation", () => { }) expect(response.status).toBe(400) - expect(response.headers.get("cache-control")).toBe("no-store") await expect(response.text()).resolves.toBe("Authorization request expired or invalid.") }) - it("marks the form parser's 413 for an oversized form as not cacheable", async () => { - // Over express.urlencoded's default 100 KB body limit - const oversizedToken = "t".repeat(200 * 1024) - + it.each([ + { + label: "400 for duplicate form fields", + body: "request_id=a&request_id=b&token=t&action=approve", + expectedStatus: 400, + }, + { label: "400 for missing fields", body: "request_id=a", expectedStatus: 400 }, + { + label: "400 for a request ID with no pending request", + body: "request_id=nonexistent-id&token=t&action=approve", + expectedStatus: 400, + }, + { + // The body exceeds express.urlencoded's default 100 KB limit. + label: "form parser's 413 for an oversized form", + body: `request_id=a&token=${"t".repeat(200 * 1024)}&action=approve`, + expectedStatus: 413, + }, + ])("marks the $label as not cacheable", async ({ body, expectedStatus }) => { const response = await fetch(`${baseUrl}/oauth/decide`, { method: "POST", headers: { "content-type": "application/x-www-form-urlencoded" }, - body: `request_id=a&token=${oversizedToken}&action=approve`, + body, }) - expect(response.status).toBe(413) + expect(response.status).toBe(expectedStatus) expect(response.headers.get("cache-control")).toBe("no-store") }) }) diff --git a/src/vault-mcp/setup/__tests__/setup-page.test.ts b/src/vault-mcp/setup/__tests__/setup-page.test.ts index 1ffa3ac4a..64b39af3e 100644 --- a/src/vault-mcp/setup/__tests__/setup-page.test.ts +++ b/src/vault-mcp/setup/__tests__/setup-page.test.ts @@ -7,6 +7,9 @@ const SIGN_IN = { insecureTransport: false, } +/** Matches the reveal button, capturing its onclick handler. */ +const REVEAL_ONCLICK_PATTERN = /class="reveal"[^>]*onclick="([^"]+)"/ + describe("renderSetupPage — sign-in", () => { it("asks for the MCP token (masked) and the Obsidian email and password", () => { const html = renderSetupPage(SIGN_IN) @@ -19,6 +22,14 @@ describe("renderSetupPage — sign-in", () => { expect(html).not.toContain("not using HTTPS") }) + it("wires the reveal button to show and hide the token input", () => { + const html = renderSetupPage(SIGN_IN) + + expect(REVEAL_ONCLICK_PATTERN.exec(html)?.[1]).toBe( + "var tokenInput=document.getElementById('token');var hidden=tokenInput.type==='password';tokenInput.type=hidden?'text':'password';this.textContent=hidden?'Hide':'Show'", + ) + }) + it("explains a rejected saved login above the form", () => { const html = renderSetupPage({ ...SIGN_IN, savedLoginRejected: true }) From 5e7e8dd51d0c502f2fe4fb5a5c184f4b859345b8 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 7 Nov 2026 15:56:05 -0500 Subject: [PATCH 12/18] test: read the consent page's reveal toggle without non-null assertions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ship-Check: test-audit · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- src/vault-mcp/oauth/__tests__/consent-page.test.ts | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/vault-mcp/oauth/__tests__/consent-page.test.ts b/src/vault-mcp/oauth/__tests__/consent-page.test.ts index 67a24a157..0f4535c4f 100644 --- a/src/vault-mcp/oauth/__tests__/consent-page.test.ts +++ b/src/vault-mcp/oauth/__tests__/consent-page.test.ts @@ -13,6 +13,12 @@ const GRANT_TEXT = "Everything this server's tools can do in your vault" /** Matches a field's label div followed by its value div, capturing both texts. */ const FIELD_ROW_PATTERN = /
    ([^<]*)<\/div>\s*
    ([^<]*)<\/div>/g +/** Matches the masked token input, capturing its id. */ +const TOKEN_INPUT_ID_PATTERN = /]*onclick="([^"]+)"/ + /** Every label and value row on the page, in page order. */ const readFieldRows = (html: string) => { return [...html.matchAll(FIELD_ROW_PATTERN)].map((match) => ({ @@ -119,12 +125,8 @@ describe("consent page reveal toggle", () => { it("wires the reveal button to the token input's id", () => { const html = renderConsentPage(baseParams) - const tokenIdMatch = /]*onclick="([^"]+)"/.exec(html) - expect(tokenIdMatch).not.toBeNull() - expect(onclickMatch).not.toBeNull() - const tokenId = tokenIdMatch![1] - const onclick = onclickMatch![1] + const tokenId = TOKEN_INPUT_ID_PATTERN.exec(html)?.[1] + const onclick = REVEAL_ONCLICK_PATTERN.exec(html)?.[1] expect(tokenId).toBe("token") // The toggle must target the input by its id and flip it to a visible // type — otherwise "Show" silently does nothing. From 3c08e20b49c620864b80a96162d5550c753e0a52 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 7 Nov 2026 16:11:58 -0500 Subject: [PATCH 13/18] fix(docs): say that Express's 404 page answers any request no route handles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The page replaces the content policy for a known path called with an unanswered method too, such as GET /oauth/decide, not only for an unknown path. Ship-Check: bug-check · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- SECURITY.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index 9eb9764d1..f54ec4e0a 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -145,9 +145,9 @@ The server, and the `:remote` image's setup mode, set these headers: - **Framing** — every response carries `X-Frame-Options: DENY` and `Content-Security-Policy: frame-ancestors 'none'`, so no other site can - frame the consent or setup pages. The server's 404 for an unknown address - is Express's built-in page, which replaces that content policy with its - own; `X-Frame-Options` still applies there + frame the consent or setup pages. The server's 404 for a request no route + answers is Express's built-in page, which replaces that content policy + with its own; `X-Frame-Options` still applies there - **Content type and referrer** — every response carries `X-Content-Type-Options: nosniff` and `Referrer-Policy: same-origin`, and none carries `X-Powered-By` From db36121cea4a62f0aaa0fc039a7173138876204f Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 7 Nov 2026 16:13:10 -0500 Subject: [PATCH 14/18] fix(oauth): show the client ID when a client's registered name is empty MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ship-Check: triage · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- src/vault-mcp/oauth/__tests__/consent-page.test.ts | 7 +++++-- src/vault-mcp/oauth/consent-page.ts | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/vault-mcp/oauth/__tests__/consent-page.test.ts b/src/vault-mcp/oauth/__tests__/consent-page.test.ts index 0f4535c4f..b94f47438 100644 --- a/src/vault-mcp/oauth/__tests__/consent-page.test.ts +++ b/src/vault-mcp/oauth/__tests__/consent-page.test.ts @@ -106,8 +106,11 @@ describe("consent page rows", () => { expect(readFieldRows(html)).toEqual(expectedRows(`https://${longHost}`)) }) - it("shows the client ID as the application when the client has no name", () => { - const html = renderConsentPage({ ...baseParams, clientName: undefined }) + it.each([ + { label: "no name", clientName: undefined }, + { label: "an empty name", clientName: "" }, + ])("shows the client ID as the application when the client has $label", ({ clientName }) => { + const html = renderConsentPage({ ...baseParams, clientName }) expect(readFieldRows(html)).toEqual([ { label: "Application", value: "client-123" }, diff --git a/src/vault-mcp/oauth/consent-page.ts b/src/vault-mcp/oauth/consent-page.ts index 36213a377..11c031676 100644 --- a/src/vault-mcp/oauth/consent-page.ts +++ b/src/vault-mcp/oauth/consent-page.ts @@ -71,7 +71,7 @@ export const renderConsentPage = ({ ${error ? `
    ${escapeHtml(error)}
    ` : ""}
    Application
    -
    ${escapeHtml(clientName ?? clientId)}
    +
    ${escapeHtml(clientName || clientId)}
    Client ID
    From 1394d1577df9ac2ffc418be5b4796c5ab56fb118 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 7 Nov 2026 16:49:19 -0500 Subject: [PATCH 15/18] refactor(oauth): name the only scope VAULT_SCOPE, and reflow the scope paragraph MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every new sign-in now gets this scope whatever it asks for, so the DEFAULT_SCOPE name no longer described it. Ship-Check: triage · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- ARCHITECTURE.md | 6 +++--- src/vault-mcp/oauth/oauth-provider.ts | 8 ++++---- src/vault-mcp/oauth/oauth-routes.ts | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 0547bb4fd..a6ca00903 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -789,9 +789,9 @@ cryptographic or binding checks. The binding claims it at authorization time, whatever scope the client requests, so the access token and every refresh carry the same value as the static token. A refresh grant that an earlier version stored with another scope keeps that scope. The -consent page shows where -Approve sends access (the redirect address's scheme and host, or the whole -address when it has no host) and a fixed description of the grant. +consent page shows where Approve sends access (the redirect address's scheme +and host, or the whole address when it has no host) and a fixed description of +the grant. **Token storage:** what each credential is and where it lives. diff --git a/src/vault-mcp/oauth/oauth-provider.ts b/src/vault-mcp/oauth/oauth-provider.ts index 8abe9adad..c76a7e51c 100644 --- a/src/vault-mcp/oauth/oauth-provider.ts +++ b/src/vault-mcp/oauth/oauth-provider.ts @@ -60,7 +60,7 @@ const INVALID_AUTH_CODE_MESSAGE = "Authorization code expired or invalid" // The server has only this scope. Every new sign-in is granted it (see // authorize), the static token carries it (see verifyAccessToken), and the // metadata advertises it as scopes_supported. -export const DEFAULT_SCOPE = "vault" as const +export const VAULT_SCOPE = "vault" as const /** `authorize` sets the granted scope before storing a request, so scopes is always set. */ type GrantedAuthorizationParams = AuthorizationParams & { scopes: string[] } @@ -589,7 +589,7 @@ export const createOAuthProvider = ({ return store }, - /** Every request is granted DEFAULT_SCOPE, whatever scope it names. The + /** Every request is granted VAULT_SCOPE, whatever scope it names. The * grant is set here, before the request is stored, so the code exchange * and every later refresh read the same value. */ async authorize( @@ -605,7 +605,7 @@ export const createOAuthProvider = ({ // redirect the client with error=invalid_target. assertResourceIsThisServer(params.resource, client.client_id) const requestId = randomUUID() - const scopes = [DEFAULT_SCOPE] + const scopes = [VAULT_SCOPE] const grantedParams = { ...params, scopes } const paramsBytes = Buffer.byteLength(JSON.stringify(grantedParams)) @@ -807,7 +807,7 @@ export const createOAuthProvider = ({ return { token, clientId: "static", - scopes: [DEFAULT_SCOPE], + scopes: [VAULT_SCOPE], expiresAt: DateTime.now().plus({ years: 10 }).toUnixInteger(), } } diff --git a/src/vault-mcp/oauth/oauth-routes.ts b/src/vault-mcp/oauth/oauth-routes.ts index aad77cdb6..02796f2db 100644 --- a/src/vault-mcp/oauth/oauth-routes.ts +++ b/src/vault-mcp/oauth/oauth-routes.ts @@ -8,7 +8,7 @@ import { metadataHandler } from "@modelcontextprotocol/sdk/server/auth/handlers/ import type { OAuthProtectedResourceMetadata } from "@modelcontextprotocol/sdk/shared/auth.js" import { extractClientIp, safeEqual, tokenBindingForServer } from "../../auth.js" import { renderConsentPage } from "./consent-page.js" -import { DEFAULT_SCOPE, type OAuthProvider } from "./oauth-provider.js" +import { VAULT_SCOPE, type OAuthProvider } from "./oauth-provider.js" import type { Logger } from "../../logger.js" type OAuthRoutesOptions = { @@ -77,7 +77,7 @@ export const createOAuthRoutes = ({ }, } - const scopesSupported = [DEFAULT_SCOPE] + const scopesSupported = [VAULT_SCOPE] // The same derivation the provider mints tokens from, so metadata and // tokens can't disagree. From 2e5198bcf6faceb5061e9f8388348b0ad31a7b22 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 7 Nov 2026 16:58:21 -0500 Subject: [PATCH 16/18] test: pin no-store on the consent page, and say which layer answers an oversized form MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The consent page's no-store comes from the MCP SDK's authorize handler; the integration test now asserts it, so the SECURITY.md claim stays true if the SDK changes. The oversized-form case notes that only the bare router answers 413. The vault-scope test group drops its old default-scope title. Ship-Check: pr-monitor · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- src/__tests__/integration/server-oauth-integration.test.ts | 3 ++- src/vault-mcp/oauth/__tests__/oauth-provider.test.ts | 2 +- src/vault-mcp/oauth/__tests__/oauth-routes.test.ts | 4 +++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/__tests__/integration/server-oauth-integration.test.ts b/src/__tests__/integration/server-oauth-integration.test.ts index 75521f868..fa45f3b0e 100644 --- a/src/__tests__/integration/server-oauth-integration.test.ts +++ b/src/__tests__/integration/server-oauth-integration.test.ts @@ -194,7 +194,7 @@ const refresh = ({ // ── Consent page response headers ────────────────────────────── describe("consent page response headers", () => { - it("serves the consent page with the security headers and without X-Powered-By", async () => { + it("serves the consent page with the security headers, not cacheable and without X-Powered-By", async () => { const port = await freePort() const server = await startServer(port, { MCP_AUTH_TOKEN: TOKEN_A }) onTestFinished(() => server.cleanup()) @@ -206,6 +206,7 @@ describe("consent page response headers", () => { expect(response.status).toBe(200) expect(await response.text()).toContain("

    Authorize access

    ") expect(readSecurityHeaders(response)).toEqual(EXPECTED_SECURITY_HEADERS) + expect(response.headers.get("cache-control")).toBe("no-store") }) }) diff --git a/src/vault-mcp/oauth/__tests__/oauth-provider.test.ts b/src/vault-mcp/oauth/__tests__/oauth-provider.test.ts index 77c3939cd..f12988ed4 100644 --- a/src/vault-mcp/oauth/__tests__/oauth-provider.test.ts +++ b/src/vault-mcp/oauth/__tests__/oauth-provider.test.ts @@ -1850,7 +1850,7 @@ describe("OAuth resource parameter (RFC 8707)", () => { }) }) -describe("OAuth default scope", () => { +describe("OAuth vault scope", () => { const GRANT_ROW_VALUE = '
    Everything this server\'s tools can do in your vault
    ' diff --git a/src/vault-mcp/oauth/__tests__/oauth-routes.test.ts b/src/vault-mcp/oauth/__tests__/oauth-routes.test.ts index 2a4029325..6f486d05f 100644 --- a/src/vault-mcp/oauth/__tests__/oauth-routes.test.ts +++ b/src/vault-mcp/oauth/__tests__/oauth-routes.test.ts @@ -325,7 +325,9 @@ describe("OAuth consent body validation", () => { expectedStatus: 400, }, { - // The body exceeds express.urlencoded's default 100 KB limit. + // The body exceeds express.urlencoded's default 100 KB limit. This router + // alone answers the parser's 413; the full server's error middleware + // turns the same error into a 500, and no-store holds either way. label: "form parser's 413 for an oversized form", body: `request_id=a&token=${"t".repeat(200 * 1024)}&action=approve`, expectedStatus: 413, From a1bf47d64ebfed9c9a206e2d761e3175d324ec7a Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 7 Nov 2026 17:05:36 -0500 Subject: [PATCH 17/18] style: say an empty client name also shows the client ID, and give the consent page its own paragraph MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ship-Check: code-quality · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- ARCHITECTURE.md | 9 +++++---- src/vault-mcp/oauth/consent-page.ts | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index a6ca00903..9e6895fc9 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -788,10 +788,11 @@ cryptographic or binding checks. The binding claims `vault` is the only scope the server advertises. Every new sign-in is granted it at authorization time, whatever scope the client requests, so the access token and every refresh carry the same value as the static token. A refresh -grant that an earlier version stored with another scope keeps that scope. The -consent page shows where Approve sends access (the redirect address's scheme -and host, or the whole address when it has no host) and a fixed description of -the grant. +grant that an earlier version stored with another scope keeps that scope. + +The consent page shows where Approve sends access (the redirect address's +scheme and host, or the whole address when it has no host) and a fixed +description of the grant. **Token storage:** what each credential is and where it lives. diff --git a/src/vault-mcp/oauth/consent-page.ts b/src/vault-mcp/oauth/consent-page.ts index 11c031676..a19f27537 100644 --- a/src/vault-mcp/oauth/consent-page.ts +++ b/src/vault-mcp/oauth/consent-page.ts @@ -3,7 +3,7 @@ import { escapeHtml } from "../../utils/escape-html.js" type ConsentPageParams = { - /** The client's registered name; the page shows its ID when it has none. */ + /** The client's registered name; the page shows its ID when the name is missing or empty. */ clientName: string | undefined clientId: string redirectUri: string From 4bd03918c0c4b0aa3aebed9c9801b69bde47ee93 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 7 Nov 2026 17:10:56 -0500 Subject: [PATCH 18/18] test: pin the full server's 500 for an oversized consent form as not cacheable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The router-only test answers an oversized form with the parser's 413. On the full server the error middleware answers the same error with a 500, and the response must still carry no-store. Ship-Check: test-audit · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- .../server-oauth-integration.test.ts | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/__tests__/integration/server-oauth-integration.test.ts b/src/__tests__/integration/server-oauth-integration.test.ts index fa45f3b0e..a46b6bdb1 100644 --- a/src/__tests__/integration/server-oauth-integration.test.ts +++ b/src/__tests__/integration/server-oauth-integration.test.ts @@ -208,6 +208,30 @@ describe("consent page response headers", () => { expect(readSecurityHeaders(response)).toEqual(EXPECTED_SECURITY_HEADERS) expect(response.headers.get("cache-control")).toBe("no-store") }) + + it("answers an oversized consent form with a 500 that is not cacheable", async () => { + const port = await freePort() + const server = await startServer(port, { MCP_AUTH_TOKEN: TOKEN_A }) + onTestFinished(() => server.cleanup()) + + // The body exceeds express.urlencoded's default 100 KB limit, and the + // server's error middleware answers the parser's error. + const response = await fetch(`http://127.0.0.1:${port}/oauth/decide`, { + method: "POST", + headers: { "content-type": "application/x-www-form-urlencoded" }, + body: `request_id=a&token=${"t".repeat(200 * 1024)}&action=approve`, + }) + + expect({ + status: response.status, + cacheControl: response.headers.get("cache-control"), + body: await response.json(), + }).toEqual({ + status: 500, + cacheControl: "no-store", + body: { error: "internal server error" }, + }) + }) }) // ── Expired access token challenge ─────────────────────────────