fix findings from the casa security self-check

packages/api/src/agent-side/tool-params.ts

@@ -24,7 +24,8 @@ export function parseToolParams(tool: CollectionTool, params: unknown): Result<R
 	const known = Object.keys(properties);
 	const values: Record<string, string> = {};
 	for (const [key, value] of Object.entries(given)) {
-		const property = properties[key];
+		// Its own properties alone: `constructor` or `toString` is no param, though every object has one.
+		const property = Object.hasOwn(properties, key) ? properties[key] : undefined;
 		if (!property) {
 			const takes = known.length > 0 ? `; it takes ${known.join(', ')}` : '; it takes none';
 			return Err(ApiErr.validationError(`${tool.name} takes no ${key}${takes}`));

packages/api/src/agent-side/tools.test.ts

@@ -292,8 +292,9 @@ describe('runAgentTool', () => {
 		const outsideEnum = await runAgentTool(caller, 'mail-1', 'emails_list', {folder: 'Spam'});
 		const missingId = await runAgentTool(caller, 'mail-1', 'emails_get', {});
 		const notText = await runAgentTool(caller, 'mail-1', 'emails_get', {id: 7});
+		const inherited = await runAgentTool(caller, 'mail-1', 'emails_get', {id: 'inbox-1', constructor: 'x'});
 
-		for (const result of [unknown, outsideEnum, missingId, notText]) {
+		for (const result of [unknown, outsideEnum, missingId, notText, inherited]) {
 			expect(result.unwrapErr().kind).toBe('validation_error');
 		}
 		expect(connector.list).not.toHaveBeenCalled();

packages/api/src/connections/sql/sql-statement.test.ts

@@ -21,17 +21,26 @@ describe('checkStatement', () => {
 		expect(checkStatement('; delete from users').unwrapErr()).toMatchObject({kind: 'validation_error'});
 	});
 
-	test('refuses a second statement after a semicolon, wherever it hides', () => {
-		expect(checkStatement('select 1; delete from users').unwrapErr()).toMatchObject({kind: 'validation_error', message: 'One statement at a time'});
-		expect(checkStatement('select 1;; select 2').unwrapErr()).toMatchObject({kind: 'validation_error', message: 'One statement at a time'});
-		expect(checkStatement("select 'a'; drop table users").unwrapErr()).toMatchObject({kind: 'validation_error', message: 'One statement at a time'});
-		expect(checkStatement("select 'a'--;\n; drop table users").unwrapErr()).toMatchObject({kind: 'validation_error', message: 'One statement at a time'});
-	});
-
-	test('lets a semicolon in a string, an identifier or a comment through', () => {
-		expect(checkStatement("select 'a;b', 'it''s; fine', 'back\\'; slash' from users").isOk()).toBe(true);
-		expect(checkStatement('select "a;b" from `t;x`').isOk()).toBe(true);
-		expect(checkStatement('select 1 -- a; comment\nfrom users').isOk()).toBe(true);
-		expect(checkStatement('select 1 /* a; comment */ from users').isOk()).toBe(true);
+	test('refuses a semicolon anywhere but at the end, wherever it hides', () => {
+		const statements = [
+			'select 1; delete from users',
+			'select 1;; select 2',
+			"select 'a'; drop table users",
+			"select 'a'--;\n; drop table users",
+			// Postgres: a dollar-quoted string, and a backslash that escapes nothing, close where a
+			// reading of quotes that doesn't know them wouldn't.
+			"SELECT $a$'$a$; COMMIT; DELETE FROM users; --'",
+			"SELECT 'x\\'; COMMIT; DELETE FROM users; --'",
+			// MySQL: a `#` comment hides the quote that would otherwise open a string.
+			"SELECT 1 # '\n; COMMIT; DROP TABLE users; -- '",
+			// Inside a string, an identifier or a comment, too.
+			"select 'a;b' from users",
+			'select "a;b" from users',
+			'select 1 -- a; comment\nfrom users',
+			'select 1 /* a; comment */ from users',
+		];
+		for (const sql of statements) {
+			expect(checkStatement(sql).unwrapErr(), sql).toMatchObject({kind: 'validation_error', message: 'One statement at a time, with no semicolon inside it'});
+		}
 	});
 });

packages/api/src/connections/sql/sql-statement.ts

@@ -5,73 +5,26 @@ import {ApiErr, type ApiError} from '@proxy/utils';
 /**
  * What an agent's query may be: one statement that only reads. A read-only transaction stops
  * writes once it runs, but not everything: MySQL lets a DDL statement commit it and go ahead, and
- * a second statement after a semicolon would run too. So the statement is checked first: one
- * alone, starting with a word that only reads.
+ * a second statement after a semicolon would run too, a COMMIT first ending the transaction. So
+ * the statement is checked first: one alone, with no semicolon but at its end, starting with a
+ * word that only reads. A semicolon in a string or a comment is refused too: telling one apart
+ * means reading quotes and comments as each database does (Postgres's `$tag$` strings and its
+ * backslashes, which escape nothing; MySQL's `#` comments), and getting that wrong runs a write.
  */
 
 const READS = ['select', 'with', 'show', 'explain', 'describe', 'desc', 'table', 'values'];
 
 export type Statement = {sql: string; keyword: string};
 
-// Where the statement ends and another would begin: a semicolon outside quotes and comments.
-function hasSecondStatement(sql: string): boolean {
-	let i = 0;
-	while (i < sql.length) {
-		const char = sql[i];
-		const next = sql[i + 1];
-		if (char === '-' && next === '-') {
-			const end = sql.indexOf('\n', i);
-			i = end === -1 ? sql.length : end + 1;
-			continue;
-		}
-		if (char === '/' && next === '*') {
-			const end = sql.indexOf('*/', i + 2);
-			i = end === -1 ? sql.length : end + 2;
-			continue;
-		}
-		if (char === "'" || char === '"' || char === '`') {
-			i = closingQuote(sql, i, char);
-			continue;
-		}
-		if (char === ';') {
-			return true;
-		}
-		i += 1;
-	}
-	return false;
-}
-
-// Past the quote that closes the one at `start`: a doubled quote, or one after a backslash, is
-// part of the text.
-function closingQuote(sql: string, start: number, quote: string): number {
-	let i = start + 1;
-	while (i < sql.length) {
-		const char = sql[i];
-		if (char === '\\') {
-			i += 2;
-			continue;
-		}
-		if (char === quote && sql[i + 1] === quote) {
-			i += 2;
-			continue;
-		}
-		if (char === quote) {
-			return i + 1;
-		}
-		i += 1;
-	}
-	return sql.length;
-}
-
 /** The statement a query runs, when it is one statement that reads; the agent is told what it may be otherwise. */
 export function checkStatement(text: string): Result<Statement, ApiError> {
 	// A trailing semicolon ends the statement rather than starting another.
 	const sql = text.trim().replace(/[\s;]+$/, '');
 	if (sql === '') {
 		return Err(ApiErr.validationError('Write the SQL to run'));
 	}
-	if (hasSecondStatement(sql)) {
-		return Err(ApiErr.validationError('One statement at a time'));
+	if (sql.includes(';')) {
+		return Err(ApiErr.validationError('One statement at a time, with no semicolon inside it'));
 	}
 	// Past any comments to the first word.
 	const keyword = sql

packages/api/src/oauth/oauth.test.ts

@@ -354,6 +354,13 @@ describe('handleTokenRoute', () => {
 		expect(missing.body).toMatchObject({error: 'invalid_client'});
 	});
 
+	test('a Basic header that does not decode is an unknown client, not a failure', async () => {
+		const res = await token({grant_type: 'refresh_token', refresh_token: 'refresh-1'}, {authorization: `Basic ${Buffer.from('client-1:%E0%A4%A').toString('base64')}`});
+
+		expect(res.statusCode).toBe(401);
+		expect(res.body).toMatchObject({error: 'invalid_client'});
+	});
+
 	test('any other grant type is unsupported', async () => {
 		const res = await token({grant_type: 'password', client_id: 'client-1'});
 

packages/api/src/oauth/token.ts

@@ -1,5 +1,5 @@
 import type {Request, Response} from 'express';
-import {Err, type Result} from 'ts-results-es';
+import {Err, Result} from 'ts-results-es';
 
 import {type ApiError, Do} from '@proxy/utils';
 import {createOAuthGrant, getOAuthClient, getOAuthGrantByRefreshToken, rotateOAuthGrant, takeOAuthCode, type OAuthClientRow, type OAuthTokens} from '@proxy/db/oauth';
@@ -31,7 +31,8 @@ function field(body: Record<string, unknown>, name: string): string | null {
 	return typeof value === 'string' && value !== '' ? value : null;
 }
 
-// HTTP Basic, as `client_secret_basic` sends the client's id and secret, each form-encoded.
+// HTTP Basic, as `client_secret_basic` sends the client's id and secret, each form-encoded. One
+// that doesn't decode is no credentials, so the client is refused rather than the request failing.
 function basicCredentials(header: string | undefined): {id: string; secret: string} | null {
 	if (!header?.startsWith('Basic ')) {
 		return null;
@@ -41,7 +42,7 @@ function basicCredentials(header: string | undefined): {id: string; secret: stri
 	if (colon < 0) {
 		return null;
 	}
-	return {id: decodeURIComponent(decoded.slice(0, colon)), secret: decodeURIComponent(decoded.slice(colon + 1))};
+	return Result.wrap(() => ({id: decodeURIComponent(decoded.slice(0, colon)), secret: decodeURIComponent(decoded.slice(colon + 1))})).unwrapOr(null);
 }
 
 function authenticateClient(req: Request, body: Record<string, unknown>): Promise<Result<OAuthClientRow, OAuthError>> {

packages/api/src/server/create-http-app.ts

@@ -7,6 +7,7 @@ import express from 'express';
 import {log} from '../observability/log';
 import {env} from '../utils/env';
 import {createApiRouter} from './create-api-router';
+import {setSecurityHeaders} from './security-headers';
 import {serveWeb} from './serve-web';
 
 // The web app's build sits at a fixed place beside this package, packages/web/dist, in the repo
@@ -20,6 +21,8 @@ export function createHttpApp({webDist = defaultWebDist}: {webDist?: string} = {
 	// Behind a TLS-terminating proxy, trust X-Forwarded-* so `req.secure` and `req.hostname`
 	// describe the original request.
 	app.set('trust proxy', true);
+	app.disable('x-powered-by');
+	app.use(setSecurityHeaders);
 
 	app.get('/health', (_req, res) => {
 		res.json({ok: true});

packages/api/src/server/security-headers.test.ts

@@ -0,0 +1,50 @@
+import {afterAll, beforeEach, describe, expect, mock, test} from 'bun:test';
+import type {Server} from 'node:http';
+import type {AddressInfo} from 'node:net';
+
+const mockEnv: {NODE_ENV: string; APP_URL: string} = {NODE_ENV: 'test', APP_URL: 'https://app.example.com'};
+
+mock.module('../utils/env', () => ({env: mockEnv}));
+
+const servers: Server[] = [];
+
+// The whole app without a web app build, as the api alone is served in development.
+async function start(): Promise<string> {
+	const {createHttpApp} = await import('./create-http-app');
+	const server = createHttpApp({webDist: '/nonexistent'}).listen(0);
+	servers.push(server);
+	return `http://localhost:${(server.address() as AddressInfo).port}`;
+}
+
+beforeEach(() => {
+	mockEnv.APP_URL = 'https://app.example.com';
+});
+
+afterAll(() => {
+	for (const server of servers) {
+		server.close();
+	}
+});
+
+describe('security headers', () => {
+	test("on the api's answers, its errors and its 404s, without saying it runs Express", async () => {
+		const base = await start();
+		for (const path of ['/health', '/auth/methods', '/api/me', '/nope']) {
+			const res = await fetch(`${base}${path}`);
+			expect(res.headers.get('x-content-type-options')).toBe('nosniff');
+			expect(res.headers.get('referrer-policy')).toBe('strict-origin-when-cross-origin');
+			expect(res.headers.get('x-frame-options')).toBe('DENY');
+			expect(res.headers.get('content-security-policy')).toBe("frame-ancestors 'none'");
+			expect(res.headers.get('strict-transport-security')).toBe('max-age=31536000');
+			expect(res.headers.get('x-powered-by')).toBeNull();
+		}
+	});
+
+	test('no HSTS where Personal Agent Proxy is served over plain HTTP', async () => {
+		mockEnv.APP_URL = 'http://localhost';
+		const base = await start();
+		const res = await fetch(`${base}/health`);
+		expect(res.headers.get('strict-transport-security')).toBeNull();
+		expect(res.headers.get('x-content-type-options')).toBe('nosniff');
+	});
+});

packages/api/src/server/security-headers.ts

@@ -0,0 +1,23 @@
+import type {NextFunction, Request, Response} from 'express';
+
+import {useSecureCookies} from '../auth/session';
+
+// A year, what scanners expect.
+const hstsMaxAgeSeconds = 365 * 24 * 60 * 60;
+
+/**
+ * On every response, the api's and the web app's alike: no guessing a file's type from its
+ * contents, no full URLs sent to other sites, and no framing by any site, the web app's own pages
+ * included. Served over HTTPS, browsers are also told to use nothing else from then on; whatever
+ * terminates TLS in front, Render or Caddy, adds no such header itself.
+ */
+export function setSecurityHeaders(_req: Request, res: Response, next: NextFunction): void {
+	res.setHeader('X-Content-Type-Options', 'nosniff');
+	res.setHeader('Referrer-Policy', 'strict-origin-when-cross-origin');
+	res.setHeader('X-Frame-Options', 'DENY');
+	res.setHeader('Content-Security-Policy', "frame-ancestors 'none'");
+	if (useSecureCookies()) {
+		res.setHeader('Strict-Transport-Security', `max-age=${hstsMaxAgeSeconds}`);
+	}
+	next();
+}

packages/api/src/server/serve-web.test.ts

@@ -43,7 +43,7 @@ describe('the web app on the api origin', () => {
 			expect(res.headers.get('cache-control')).toBe('no-cache');
 			expect(res.headers.get('x-content-type-options')).toBe('nosniff');
 			expect(res.headers.get('referrer-policy')).toBe('strict-origin-when-cross-origin');
-			expect(res.headers.get('x-frame-options')).toBe('SAMEORIGIN');
+			expect(res.headers.get('x-frame-options')).toBe('DENY');
 			expect(await res.text()).toContain('<title>Personal Agent Proxy</title>');
 		}
 	});
@@ -54,7 +54,7 @@ describe('the web app on the api origin', () => {
 		const asset = await fetch(`${base}/assets/index-abc.js`);
 		expect(asset.status).toBe(200);
 		expect(asset.headers.get('cache-control')).toContain('immutable');
-		expect(asset.headers.get('x-frame-options')).toBe('SAMEORIGIN');
+		expect(asset.headers.get('x-frame-options')).toBe('DENY');
 	});
 
 	test('a missing asset is a 404, not the page', async () => {

packages/api/src/server/serve-web.ts

@@ -10,14 +10,6 @@ function isApiPath(path: string): boolean {
 	return apiPaths.some((apiPath) => path === apiPath || path.startsWith(`${apiPath}/`));
 }
 
-// On everything the web app is served with: no guessing a file's type from its contents, no full
-// URLs sent to other sites, and no framing by them.
-function setSecurityHeaders(res: express.Response): void {
-	res.setHeader('X-Content-Type-Options', 'nosniff');
-	res.setHeader('Referrer-Policy', 'strict-origin-when-cross-origin');
-	res.setHeader('X-Frame-Options', 'SAMEORIGIN');
-}
-
 /**
  * Serves the web app's build from `dir`, so one server is the whole of Personal Agent Proxy on one origin: the
  * files as they are, and `index.html` for every other path.
@@ -28,16 +20,15 @@ export function serveWeb(dir: string): express.Router {
 
 	// Vite names assets by their content, so a name never changes what it serves. A missing one is
 	// a 404, not the app's page.
-	router.use('/assets', express.static(join(root, 'assets'), {immutable: true, maxAge: '1y', fallthrough: false, setHeaders: setSecurityHeaders}));
-	router.use(express.static(root, {index: false, setHeaders: setSecurityHeaders}));
+	router.use('/assets', express.static(join(root, 'assets'), {immutable: true, maxAge: '1y', fallthrough: false}));
+	router.use(express.static(root, {index: false}));
 
 	router.get('*', (req, res, next) => {
 		if (isApiPath(req.path)) {
 			next();
 			return;
 		}
 
-		setSecurityHeaders(res);
 		// Always revalidated, so a new release reaches the browser on its next load.
 		res.setHeader('Cache-Control', 'no-cache');
 		// Without a callback, Express passes a failure to the error handler itself, leaving out

packages/api/src/utils/secret-crypto.test.ts

@@ -42,6 +42,16 @@ describe('secret crypto', () => {
 		expect(decryptSecret(forged).unwrapErr().kind).toBe('internal_error');
 	});
 
+	test('fails with a shortened tag, though it is the start of the right one', async () => {
+		const {encryptSecret, decryptSecret} = await import('./secret-crypto');
+		const [version, iv, tag, ciphertext] = encryptSecret('secret').unwrap().split('.');
+		const shortTag = Buffer.from(tag ?? '', 'base64url')
+			.subarray(0, 4)
+			.toString('base64url');
+
+		expect(decryptSecret([version, iv, shortTag, ciphertext].join('.')).unwrapErr().kind).toBe('internal_error');
+	});
+
 	test('fails without a key', async () => {
 		mockEnv.ENCRYPTION_KEY = undefined;
 		const {encryptSecret} = await import('./secret-crypto');

packages/api/src/utils/secret-crypto.ts

@@ -9,6 +9,9 @@ import {env} from './env';
 // a new key or cipher later without touching what is already stored.
 const VERSION = 'v1';
 const ALGORITHM = 'aes-256-gcm';
+// The whole tag, always: told nothing, decryption would take a shortened one, which is far easier
+// to forge.
+const AUTH_TAG_LENGTH = 16;
 
 function encryptionKey(): Result<Buffer, ApiError> {
 	if (!env.ENCRYPTION_KEY) {
@@ -20,7 +23,7 @@ function encryptionKey(): Result<Buffer, ApiError> {
 export function encryptSecret(plaintext: string): Result<string, ApiError> {
 	return encryptionKey().map((key) => {
 		const iv = randomBytes(12);
-		const cipher = createCipheriv(ALGORITHM, key, iv);
+		const cipher = createCipheriv(ALGORITHM, key, iv, {authTagLength: AUTH_TAG_LENGTH});
 		const ciphertext = Buffer.concat([cipher.update(plaintext, 'utf8'), cipher.final()]);
 		const parts = [iv, cipher.getAuthTag(), ciphertext].map((part) => part.toString('base64url'));
 		return [VERSION, ...parts].join('.');
@@ -36,7 +39,7 @@ export function decryptSecret(stored: string): Result<string, ApiError> {
 
 	return encryptionKey().andThen((key) =>
 		Result.wrap(() => {
-			const decipher = createDecipheriv(ALGORITHM, key, Buffer.from(iv, 'base64url'));
+			const decipher = createDecipheriv(ALGORITHM, key, Buffer.from(iv, 'base64url'), {authTagLength: AUTH_TAG_LENGTH});
 			decipher.setAuthTag(Buffer.from(tag, 'base64url'));
 			return Buffer.concat([decipher.update(Buffer.from(ciphertext, 'base64url')), decipher.final()]).toString('utf8');
 		}).mapErr((cause) => ApiErr.internalError(cause)),