fix(oauth): forbid framing of the consent and setup pages and show where approval sends access

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.
 

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
 

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`, {

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,27 +83,44 @@ const registerClient = async (port: number): Promise<RegisteredClient> => {
   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<IssuedTokens> => {
-  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",
     client_id: client.client_id,
     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<IssuedTokens> => {
+  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("<h1>Authorize access</h1>")
+    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", () => {

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,
+}

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<string> => {
+  const app = express()
+  app.use(securityHeaders)
+  app.get("/page", (_req, res) => {
+    res.send("page body")
+  })
+  const server = await new Promise<Server>((resolve) => {
+    const listening = app.listen(0, "127.0.0.1", () => resolve(listening))
+  })
+  onTestFinished(() => new Promise<void>((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")
+  })
+})

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()
+}