diff --git a/docs/releases/unreleased.md b/docs/releases/unreleased.md index c29cde0dd..7618ebb68 100644 --- a/docs/releases/unreleased.md +++ b/docs/releases/unreleased.md @@ -42,3 +42,7 @@ When a change has user-facing documentation, include a canonical tasknotes.dev l - (#2147) Added context-menu actions for recording task completion today, on the scheduled date, on the due date, or on a chosen date. The actions can be grouped in a submenu from Appearance settings. See [Completing Tasks](https://tasknotes.dev/features/task-management/#completing-tasks). - Rescheduling a recurring task can reactivate affected completed or skipped instances after confirmation. See [Recurring Tasks](https://tasknotes.dev/features/recurring-tasks/). - Thanks to @renatomen for the contribution. + +## Security + +- Require an API token before starting local API/MCP listeners, and validate one-use OAuth callbacks on an OS-assigned loopback port without reflecting callback text into HTML. diff --git a/src/api/httpTypes.ts b/src/api/httpTypes.ts index 9774ab23b..b631ac8d6 100644 --- a/src/api/httpTypes.ts +++ b/src/api/httpTypes.ts @@ -16,6 +16,7 @@ export interface HTTPResponseLike { } export interface HTTPServerLike { + address?(): { port: number } | string | null; listening?: boolean; listen(port: number, callback?: () => void): void; listen(port: number, hostname: string, callback?: () => void): void; diff --git a/src/services/HTTPAPIService.ts b/src/services/HTTPAPIService.ts index 7d8252072..d0f565638 100644 --- a/src/services/HTTPAPIService.ts +++ b/src/services/HTTPAPIService.ts @@ -188,9 +188,9 @@ export class HTTPAPIService implements IWebhookNotifier { private authenticate(req: HTTPRequestLike): boolean { const authToken = this.plugin.settings.apiAuthToken; - // Skip auth if no token is configured + // A missing credential never opens an unauthenticated listener. if (!authToken) { - return true; + return false; } const authHeader = req.headers.authorization; @@ -292,6 +292,17 @@ export class HTTPAPIService implements IWebhookNotifier { } async start(): Promise { + if (!Platform.isDesktop || !Platform.isDesktopApp) + throw new Error("The HTTP API is only available in the desktop app."); + if (!this.plugin.settings.apiAuthToken) { + this.plugin.settings.apiAuthToken = btoa( + String.fromCharCode(...crypto.getRandomValues(new Uint8Array(32))) + ) + .replace(/\+/g, "-") + .replace(/\//g, "_") + .replace(/=+$/g, ""); + await this.plugin.saveSettings(); + } return new Promise((resolve, reject) => { if (!Platform.isDesktop || !Platform.isDesktopApp) { reject(new Error("The HTTP API is only available in the desktop app.")); diff --git a/src/services/OAuthService.ts b/src/services/OAuthService.ts index 9fed0b255..478aace5c 100644 --- a/src/services/OAuthService.ts +++ b/src/services/OAuthService.ts @@ -157,12 +157,7 @@ export class OAuthService { const codeChallenge = await this.generateCodeChallenge(codeVerifier); const state = this.generateState(); - // Find available port - const port = await this.findAvailablePort( - OAUTH_CONSTANTS.CALLBACK_PORT_START, - OAUTH_CONSTANTS.CALLBACK_PORT_END - ); - await this.startCallbackServer(port); + const port = await this.startCallbackServer(0); // Update redirect URI for this session const originalRedirectUri = config.redirectUri; @@ -185,11 +180,11 @@ export class OAuthService { `Opening browser for ${provider} authorization...` ); - // Open browser to authorization URL + // Register the resolver before the browser can return a fast callback. + const callback = this.waitForCallback(state, 300000); + void callback.catch(() => undefined); await this.openAuthorizationUrl(authUrl); - - // Wait for callback with timeout - const code = await this.waitForCallback(state, 300000); // 5 minute timeout + const code = await callback; // Exchange code for tokens const tokens = await this.exchangeCodeForTokens(config, code, codeVerifier); @@ -202,6 +197,10 @@ export class OAuthService { `Successfully connected to ${provider} Calendar!` ); } finally { + // Clear a pending resolver even if opening the browser failed. + const pending = this.pendingOAuthState.get(state); + this.pendingOAuthState.delete(state); + pending?.reject(new Error("OAuth flow ended before authorization")); // Restore original redirect URI config.redirectUri = originalRedirectUri; } @@ -231,43 +230,19 @@ export class OAuthService { return; } } catch (error) { - tasknotesLogger.warn("Failed to open OAuth URL in system browser; falling back to window.open.", { - category: "provider", - operation: "oauth-open-external", - error, - }); + tasknotesLogger.warn( + "Failed to open OAuth URL in system browser; falling back to window.open.", + { + category: "provider", + operation: "oauth-open-external", + error, + } + ); } window.open(authUrl, "_blank"); } - /** - * Finds an available port in the given range - */ - private async findAvailablePort(startPort: number, endPort: number): Promise { - const http = ensureHttpModule(); - - for (let port = startPort; port <= endPort; port++) { - try { - await new Promise((resolve, reject) => { - const server = http.createServer(); - server.once("error", reject); - server.once("listening", () => { - server.close(); - resolve(); - }); - server.listen(port, "127.0.0.1"); - }); - return port; - } catch { - // Port in use, try next one - continue; - } - } - - throw new Error(`No available ports found between ${startPort} and ${endPort}`); - } - /** * Generates a random code verifier for PKCE */ @@ -330,10 +305,10 @@ export class OAuthService { /** * Starts a temporary HTTP server to receive the OAuth callback */ - private async startCallbackServer(port: number): Promise { + private async startCallbackServer(port: number): Promise { return new Promise((resolve, reject) => { if (this.callbackServer) { - resolve(); // Already running + reject(new Error("An OAuth callback is already pending")); return; } @@ -363,7 +338,12 @@ export class OAuthService { }); this.callbackServer.listen(port, "127.0.0.1", () => { - resolve(); + const address = this.callbackServer?.address?.(); + if (!address || typeof address === "string") { + reject(new Error("OAuth listener did not expose its bound port")); + return; + } + resolve(address.port); }); }); } @@ -389,70 +369,48 @@ export class OAuthService { * Handles incoming HTTP requests to the callback server */ private handleCallback(req: HTTPRequestLike, res: HTTPResponseLike): void { - const hostHeader = req.headers.host; - const host = Array.isArray(hostHeader) ? hostHeader[0] : (hostHeader ?? "localhost"); - const url = new URL(req.url || "", `http://${host}`); - const code = url.searchParams.get("code"); - const state = url.searchParams.get("state"); - const error = url.searchParams.get("error"); - - // Send response to browser - res.writeHead(200, { "Content-Type": "text/html" }); - - if (error) { - res.end(` - - - OAuth Error - -

Authorization Failed

-

Error: ${error}

-

You can close this window.

- - - `); - - const pending = state ? this.pendingOAuthState.get(state) : null; - if (pending && state) { - pending.reject(new Error(`OAuth error: ${error}`)); - this.pendingOAuthState.delete(state); - } + const headers = { + "Content-Type": "text/html; charset=utf-8", + "Content-Security-Policy": + "default-src 'none'; frame-ancestors 'none'; base-uri 'none'", + "X-Content-Type-Options": "nosniff", + "Cache-Control": "no-store", + }; + let url: URL; + try { + url = new URL(req.url || "/", "http://127.0.0.1"); + } catch { + res.writeHead(400, headers); + res.end("Invalid callback"); return; } - - if (!code || !state) { - res.end(` - - - OAuth Error - -

Invalid Callback

-

Missing required parameters.

-

You can close this window.

- - - `); + const state = url.searchParams.get("state"); + const pending = state ? this.pendingOAuthState.get(state) : undefined; + const code = url.searchParams.get("code"); + const error = url.searchParams.get("error"); + if ( + req.method !== "GET" || + url.pathname !== "/" || + !state || + !pending || + (!code && !error) + ) { + res.writeHead(400, headers); + res.end( + "Invalid callback

No pending authorization matches this callback.

" + ); return; } - - res.end(` - - - OAuth Success - -

Authorization Successful!

-

You can close this window and return to Obsidian.

- - - - `); - - // Resolve the pending promise - const pending = this.pendingOAuthState.get(state); - if (pending) { - pending.resolve(code); - this.pendingOAuthState.delete(state); - } + // Consume before responding: a replay cannot resolve the pending flow. + this.pendingOAuthState.delete(state); + res.writeHead(error ? 400 : 200, headers); + res.end( + error + ? "Authorization failed

Authorization was not completed. Return to Obsidian.

" + : "Authorization complete

You can close this window and return to Obsidian.

" + ); + if (error) pending.reject(new Error("OAuth authorization failed")); + else pending.resolve(code as string); } /** diff --git a/tests/services/OAuthService.callback.test.ts b/tests/services/OAuthService.callback.test.ts new file mode 100644 index 000000000..e6825e735 --- /dev/null +++ b/tests/services/OAuthService.callback.test.ts @@ -0,0 +1,35 @@ +import { OAuthService } from "../../src/services/OAuthService"; +import { OAuthSecretStore } from "../../src/services/OAuthSecretStore"; + +describe("OAuth callback validation", () => { + it("validates and consumes OAuth state before writing a static response", () => { + const store = new OAuthSecretStore({ getSecret: () => null, setSecret: () => {} }); + const service: any = new OAuthService({} as any, store); + const response: any = { writeHead: jest.fn(), end: jest.fn() }; + service.handleCallback( + { method: "GET", url: "/?error=%3Cscript%3E&state=unknown", headers: {} }, + response + ); + expect(response.writeHead.mock.calls[0][0]).toBe(400); + expect(response.end.mock.calls[0][0]).not.toContain("