diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 600f6f82b..9e6895fc9 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -785,10 +785,14 @@ 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 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/SECURITY.md b/SECURITY.md index 957f7124f..f54ec4e0a 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -136,8 +136,23 @@ 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 redirect address, error messages, request IDs, the + Obsidian account name, vault names, API error text) + +### Response 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 + 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` +- **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/__tests__/integration/server-integration.test.ts b/src/__tests__/integration/server-integration.test.ts index fc6317a98..a46b9e302 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" @@ -1403,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" }, @@ -1413,6 +1422,42 @@ describe("default config", () => { }) }) + 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 }, + { + 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) + }) + }) + describe("OAuth rate limiting", () => { const register = (forwardedIp: string) => { return fetch(`http://127.0.0.1:${port}/register`, { diff --git a/src/__tests__/integration/server-oauth-integration.test.ts b/src/__tests__/integration/server-oauth-integration.test.ts index 36753ad40..a46b6bdb1 100644 --- a/src/__tests__/integration/server-oauth-integration.test.ts +++ b/src/__tests__/integration/server-oauth-integration.test.ts @@ -7,8 +7,13 @@ 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 { startServer, freePort } from "./test-harness.js" +import { signJwt, verifyJwt } from "../../jwt.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" @@ -18,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). */ @@ -74,18 +83,17 @@ 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, + scope, }: { port: number client: RegisteredClient - authToken: string -}): Promise => { - const verifier = base64Url(randomBytes(32)) - const challenge = base64Url(createHash("sha256").update(verifier).digest()) + challenge: string + scope: string +}): URL => { const authorizeUrl = new URL(`http://127.0.0.1:${port}/authorize`) authorizeUrl.search = new URLSearchParams({ response_type: "code", @@ -93,8 +101,26 @@ const authorize = async ({ redirect_uri: REDIRECT_URI, code_challenge: challenge, code_challenge_method: "S256", - scope: "vault", + scope, }).toString() + return authorizeUrl +} + +/** Consent page → approve with the auth token → PKCE code exchange. */ +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, scope }) const consentHtml = await (await fetch(authorizeUrl)).text() const requestId = /name="request_id"\s+value="([^"]+)"/.exec(consentHtml)?.[1] @@ -165,6 +191,49 @@ const refresh = ({ }) } +// ── Consent page response headers ────────────────────────────── + +describe("consent page response headers", () => { + 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()) + const client = await registerClient(port) + const challenge = base64Url(createHash("sha256").update("header-check").digest()) + + const response = await fetch(buildAuthorizeUrl({ port, client, challenge, scope: "vault" })) + + 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") + }) + + 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 ───────────────────────────── describe("expired access token challenge", () => { @@ -417,6 +486,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/__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..f2be28888 --- /dev/null +++ b/src/utils/security-headers.ts @@ -0,0 +1,20 @@ +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", + // 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 make the consent + // form's post arrive with `Origin: null`, hiding which site sent it. + "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/oauth/__tests__/consent-page.test.ts b/src/vault-mcp/oauth/__tests__/consent-page.test.ts index be5ccb0f7..b94f47438 100644 --- a/src/vault-mcp/oauth/__tests__/consent-page.test.ts +++ b/src/vault-mcp/oauth/__tests__/consent-page.test.ts @@ -4,10 +4,121 @@ 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 + +/** 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) => ({ + 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@other.example/cb", + destination: "https://other.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.other.example/cb", + destination: "https://claude.ai.other.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}`)) + }) + + 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" }, + ...expectedRows("https://claude.ai").slice(1), + ]) + }) +}) + describe("consent page reveal toggle", () => { it("renders the token field masked by default", () => { const html = renderConsentPage(baseParams) @@ -17,17 +128,13 @@ 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. 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/__tests__/oauth-provider.test.ts b/src/vault-mcp/oauth/__tests__/oauth-provider.test.ts index 449bd710c..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,10 @@ 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
' + /** 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,51 +1906,61 @@ 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) 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"]) 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 5d311b904..6f486d05f 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"] @@ -196,7 +199,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], } } @@ -210,24 +212,36 @@ describe("OAuth consent token submission", () => { expect(response.headers.get("location")).toBeNull() 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") + }) + + 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") }) }) @@ -286,6 +300,48 @@ describe("OAuth consent body validation", () => { 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) + await expect(response.text()).resolves.toBe("Authorization request expired or invalid.") + }) + + 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. 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, + }, + ])("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, + }) + + expect(response.status).toBe(expectedStatus) + expect(response.headers.get("cache-control")).toBe("no-store") + }) }) describe("OAuth consent audit logging", () => { diff --git a/src/vault-mcp/oauth/consent-page.ts b/src/vault-mcp/oauth/consent-page.ts index c3ab40dfb..a19f27537 100644 --- a/src/vault-mcp/oauth/consent-page.ts +++ b/src/vault-mcp/oauth/consent-page.ts @@ -3,24 +3,40 @@ import { escapeHtml } from "../../utils/escape-html.js" type ConsentPageParams = { - clientName: string + /** The client's registered name; the page shows its ID when the name is missing or empty. */ + clientName: string | undefined 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. 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}` +} + 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 +51,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} @@ -58,15 +71,19 @@ export const renderConsentPage = ({ ${error ? `
    ${escapeHtml(error)}
    ` : ""}
    Application
    -
    ${escapeHtml(clientName)}
    +
    ${escapeHtml(clientName || clientId)}
    Client ID
    ${escapeHtml(clientId)}
    -
    Requested scopes
    -
      ${scopeList}
    +
    Sends access to
    +
    ${escapeHtml(redirectUriToDestination(redirectUri))}
    +
    +
    +
    Access
    +
    ${escapeHtml(GRANT_DESCRIPTION)}
    @@ -74,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 c7c3f7a04..c76a7e51c 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. -export const DEFAULT_SCOPE = "vault" as const +// 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 VAULT_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 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( 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 = [VAULT_SCOPE] const grantedParams = { ...params, scopes } const paramsBytes = Buffer.byteLength(JSON.stringify(grantedParams)) @@ -634,9 +631,9 @@ export const createOAuthProvider = ({ }) res.type("html").send( renderConsentPage({ - clientName: client.client_name ?? client.client_id, + clientName: client.client_name, clientId: client.client_id, - scopes, + redirectUri: params.redirectUri, requestId, }), ) @@ -810,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 8808ce8f1..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. @@ -133,6 +133,15 @@ 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 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() + }) + // Consent form submission (unauthenticated — part of authorize flow) router.post( "/oauth/decide", @@ -169,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) @@ -192,9 +203,9 @@ 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, - scopes: pending.params.scopes, + redirectUri: pending.params.redirectUri, requestId: request_id, error: "Invalid token. Please try again.", }), diff --git a/src/vault-mcp/server.ts b/src/vault-mcp/server.ts index e11cb2e8d..3dc872a6e 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,10 @@ const startServer = async (): Promise => { }) const app = express() + // 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 // X-Forwarded-For hop per proxy the deployment controls. With the // default 0, req.ip — the OAuth rate limiter's fallback bucket key — @@ -236,7 +241,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-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 }) diff --git a/src/vault-mcp/setup/__tests__/setup-server.test.ts b/src/vault-mcp/setup/__tests__/setup-server.test.ts index 5de47f8c9..fa9bd1bb6 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,46 @@ 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("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-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.
    ` diff --git a/src/vault-mcp/setup/setup-server.ts b/src/vault-mcp/setup/setup-server.ts index f9faaeb3e..103f82517 100644 --- a/src/vault-mcp/setup/setup-server.ts +++ b/src/vault-mcp/setup/setup-server.ts @@ -11,13 +11,14 @@ 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" /** 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" @@ -47,12 +48,22 @@ 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 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. + 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 @@ -95,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,