From a67cdac380486e45609067e5f765a9cac37dba96 Mon Sep 17 00:00:00 2001 From: Juan Castro Date: Thu, 1 Oct 2026 16:51:24 -0400 Subject: [PATCH] fix: bound the inputs on the grant and token routes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three permission routes sized their work by the request body rather than by the route, so one call could cost far more than its rate-limit slot implies. - `extra` and `meta` were only type-checked. They ride every row a grant writes (up to 16) and are re-serialised into the per-(user, app) cache on each miss, so they are now capped at 4 KiB. - `/auth/create-access-token` took a `permissions` array of any length: one permission check and one sequential INSERT each. It now uses the same 16-per-request cap the grant routes already had, and runs every entry through the validator those routes use. - Withdrawing an app's cross-app data grants looks them up by permission text, and no index on `user_to_app_permissions` led with `permission`. Indexed per engine, mirroring what mysql_mig_22 / postgres_mig_11 / sqlite 0067 did for `user_to_user_permissions`. `grant-dev-app` validated none of its input — not even the type of `extra` — so it now runs the same validator as its user-app sibling. Two paths could also carry a permission wider than the `varchar(255)` column it lands in. `grantUserAppPermission` already rejected that after rewriting; `grantDevAppPermission` now does the same, and `createAccessToken` checks it before the session row a failing INSERT would otherwise orphan. Caps are published in rate-limits-and-quotas.md. Every in-tree caller sends one permission and a small `extra`, so none of them change behaviour. --- .../database/SqliteDatabaseClient.test.ts | 2 +- .../clients/database/SqliteDatabaseClient.ts | 1 + .../migrations/mysql/mysql_mig_45.sql | 39 +++++ .../migrations/postgres/postgres_mig_34.sql | 24 +++ .../sqlite/0090_user-app-permission-index.sql | 26 ++++ .../controllers/auth/AuthController.test.ts | 140 ++++++++++++++++++ .../controllers/auth/AuthController.ts | 50 ++++++- src/backend/services/auth/AuthService.ts | 14 +- .../services/permission/PermissionService.ts | 6 + .../stores/permission/PermissionStore.test.ts | 17 +++ src/docs/src/rate-limits-and-quotas.md | 3 +- 11 files changed, 312 insertions(+), 10 deletions(-) create mode 100644 src/backend/clients/database/migrations/mysql/mysql_mig_45.sql create mode 100644 src/backend/clients/database/migrations/postgres/postgres_mig_34.sql create mode 100644 src/backend/clients/database/migrations/sqlite/0090_user-app-permission-index.sql diff --git a/src/backend/clients/database/SqliteDatabaseClient.test.ts b/src/backend/clients/database/SqliteDatabaseClient.test.ts index b682304cb..95493c67c 100644 --- a/src/backend/clients/database/SqliteDatabaseClient.test.ts +++ b/src/backend/clients/database/SqliteDatabaseClient.test.ts @@ -27,7 +27,7 @@ import { DatabaseClientFactory } from './index.js'; import { SqliteDatabaseClient } from './SqliteDatabaseClient.js'; /** Highest schema version the migration table can reach. */ -const CURRENT_SCHEMA_VERSION = 85; +const CURRENT_SCHEMA_VERSION = 86; /** * These suites migrate real files on disk. Idle they finish in well under a diff --git a/src/backend/clients/database/SqliteDatabaseClient.ts b/src/backend/clients/database/SqliteDatabaseClient.ts index 6dc459fc8..7cd22b5ba 100644 --- a/src/backend/clients/database/SqliteDatabaseClient.ts +++ b/src/backend/clients/database/SqliteDatabaseClient.ts @@ -119,6 +119,7 @@ const AVAILABLE_MIGRATIONS: [number, string[]][] = [ [82, ['0087_feedback-attachments.sql']], [83, ['0088_apps-index-url.sql']], [84, ['0089_team-require-2fa.sql']], + [85, ['0090_user-app-permission-index.sql']], ]; export class SqliteDatabaseClient extends AbstractDatabaseClient { diff --git a/src/backend/clients/database/migrations/mysql/mysql_mig_45.sql b/src/backend/clients/database/migrations/mysql/mysql_mig_45.sql new file mode 100644 index 000000000..495070e57 --- /dev/null +++ b/src/backend/clients/database/migrations/mysql/mysql_mig_45.sql @@ -0,0 +1,39 @@ +-- Copyright (C) 2024-present Puter Technologies Inc. +-- +-- This file is part of Puter. +-- +-- Puter is free software: you can redistribute it and/or modify +-- it under the terms of the GNU Affero General Public License as published +-- by the Free Software Foundation, either version 3 of the License, or +-- (at your option) any later version. +-- +-- This program is distributed in the hope that it will be useful, +-- but WITHOUT ANY WARRANTY; without even the implied warranty of +-- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +-- GNU Affero General Public License for more details. +-- +-- You should have received a copy of the GNU Affero General Public License +-- along with this program. If not, see . + +-- Mirrors SQLite migration 0090. Guarded like mysql_mig_22, because there is +-- no per-file applied-state tracking and a replay must do nothing. + +DROP PROCEDURE IF EXISTS _puter_add_user_app_permission_index; +DELIMITER // +CREATE PROCEDURE _puter_add_user_app_permission_index() +BEGIN + IF NOT EXISTS ( + SELECT 1 FROM INFORMATION_SCHEMA.STATISTICS + WHERE TABLE_SCHEMA = DATABASE() + AND TABLE_NAME = 'user_to_app_permissions' + AND INDEX_NAME = 'idx_user_to_app_permissions_permission' + ) THEN + ALTER TABLE `user_to_app_permissions` + ADD INDEX `idx_user_to_app_permissions_permission` (`permission`); + END IF; +END// +DELIMITER ; + +CALL _puter_add_user_app_permission_index(); + +DROP PROCEDURE IF EXISTS _puter_add_user_app_permission_index; diff --git a/src/backend/clients/database/migrations/postgres/postgres_mig_34.sql b/src/backend/clients/database/migrations/postgres/postgres_mig_34.sql new file mode 100644 index 000000000..e769090b6 --- /dev/null +++ b/src/backend/clients/database/migrations/postgres/postgres_mig_34.sql @@ -0,0 +1,24 @@ +-- Copyright (C) 2024-present Puter Technologies Inc. +-- +-- This file is part of Puter. +-- +-- Puter is free software: you can redistribute it and/or modify +-- it under the terms of the GNU Affero General Public License as published +-- by the Free Software Foundation, either version 3 of the License, or +-- (at your option) any later version. +-- +-- This program is distributed in the hope that it will be useful, +-- but WITHOUT ANY WARRANTY; without even the implied warranty of +-- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +-- GNU Affero General Public License for more details. +-- +-- You should have received a copy of the GNU Affero General Public License +-- along with this program. If not, see . + +-- Mirrors SQLite migration 0090, as postgres_mig_11 did for user_to_user. +-- text_pattern_ops is what makes the left-anchored LIKE a range scan under a +-- non-C collation -- the plain index dev_to_app already has does not. +CREATE INDEX IF NOT EXISTS idx_user_to_app_permissions_permission + ON user_to_app_permissions (permission text_pattern_ops); +CREATE INDEX IF NOT EXISTS idx_dev_to_app_permissions_permission_pattern + ON dev_to_app_permissions (permission text_pattern_ops); diff --git a/src/backend/clients/database/migrations/sqlite/0090_user-app-permission-index.sql b/src/backend/clients/database/migrations/sqlite/0090_user-app-permission-index.sql new file mode 100644 index 000000000..ea1136caf --- /dev/null +++ b/src/backend/clients/database/migrations/sqlite/0090_user-app-permission-index.sql @@ -0,0 +1,26 @@ +-- Copyright (C) 2024-present Puter Technologies Inc. +-- +-- This file is part of Puter. +-- +-- Puter is free software: you can redistribute it and/or modify +-- it under the terms of the GNU Affero General Public License as published +-- by the Free Software Foundation, either version 3 of the License, or +-- (at your option) any later version. +-- +-- This program is distributed in the hope that it will be useful, +-- but WITHOUT ANY WARRANTY; without even the implied warranty of +-- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +-- GNU Affero General Public License for more details. +-- +-- You should have received a copy of the GNU Affero General Public License +-- along with this program. If not, see . + +-- Withdrawing an app's cross-app data grants looks them up by permission text, +-- and no existing index on either table leads with `permission`. Same fix as +-- `idx_user_to_user_permissions_permission` in 0067. +CREATE INDEX IF NOT EXISTS `idx_user_to_app_permissions_permission` + ON `user_to_app_permissions` (`permission`); + +-- The sweep reads both tables; MySQL and Postgres already index this one. +CREATE INDEX IF NOT EXISTS `idx_dev_to_app_permissions_permission` + ON `dev_to_app_permissions` (`permission`); diff --git a/src/backend/controllers/auth/AuthController.test.ts b/src/backend/controllers/auth/AuthController.test.ts index b58c5380d..224140b45 100644 --- a/src/backend/controllers/auth/AuthController.test.ts +++ b/src/backend/controllers/auth/AuthController.test.ts @@ -2201,6 +2201,49 @@ describe('AuthController grant flows', () => { expect(res.body).toEqual({}); }); + it('grant-user-app: 400 on an `extra`/`meta` larger than the cap', async () => { + const appName = `tb-${uuidv4()}`; + const app = await server.stores.app.create( + { + name: appName, + title: 'TestBoundApp', + index_url: `https://${appName}.example.test/index.html`, + }, + { ownerUserId: issuer.id }, + ); + const permission = 'service:tb-app:ii:read'; + const oversized = { blob: 'z'.repeat(5000) }; + for (const body of [ + { app_uid: app.uid, permission, extra: oversized }, + { app_uid: app.uid, permission, meta: oversized }, + ]) { + await expect( + inCtx(issuerActor, () => + controller.handleGrantUserApp( + makeReq(body, { actor: issuerActor }), + makeRes(), + ), + ), + ).rejects.toMatchObject({ statusCode: 400 }); + } + + const res = makeRes(); + await inCtx(issuerActor, () => + controller.handleGrantUserApp( + makeReq( + { + app_uid: app.uid, + permission, + extra: { blob: 'z'.repeat(1000) }, + }, + { actor: issuerActor }, + ), + res, + ), + ); + expect(res.body).toEqual({}); + }); + it('grant-user-app: refuses a key-value delegation over a whole namespace', async () => { // The prompt for it would read "let this app show your app data to // whoever it picks", which describes no bounded capability — so the @@ -3515,6 +3558,75 @@ describe('AuthController.handleCreateAccessToken + handleRevokeAccessToken', () ).rejects.toMatchObject({ statusCode: 400 }); }); + it('rejects a permission wider than the column, leaving no session behind', async () => { + // Nothing rewrites these, so an over-wide one would reach the INSERT + // only after the session row was created and the JWT signed. + const before = (await server.clients.db.read( + "SELECT COUNT(*) AS n FROM sessions WHERE kind = 'access_token'", + [], + )) as Array<{ n: number }>; + await expect( + controller.handleCreateAccessToken( + makeReq( + { permissions: [`service:${'a'.repeat(300)}:ii:read`] }, + { actor }, + ), + makeRes(), + ), + ).rejects.toMatchObject({ statusCode: 400 }); + const after = (await server.clients.db.read( + "SELECT COUNT(*) AS n FROM sessions WHERE kind = 'access_token'", + [], + )) as Array<{ n: number }>; + expect(after[0].n).toBe(before[0].n); + }); + + it('rejects more permissions than one request may carry', async () => { + // Each entry costs a permission check and an INSERT. + const permissions = Array.from( + { length: 17 }, + (_, i) => `service:cap-${i}:ii:read`, + ); + await expect( + controller.handleCreateAccessToken( + makeReq({ permissions }, { actor }), + makeRes(), + ), + ).rejects.toMatchObject({ statusCode: 400 }); + }); + + it('still mints at the cap', async () => { + const permissions = Array.from( + { length: 16 }, + (_, i) => `service:atcap-${i}:ii:read`, + ); + const res = makeRes(); + await controller.handleCreateAccessToken( + makeReq({ permissions }, { actor }), + res, + ); + expect((res.body as { token: string }).token).toEqual( + expect.any(String), + ); + }); + + it.each([ + ['an oversized permission string', ['x'.repeat(4097)]], + [ + 'an oversized `extra`', + [['service:x:ii:read', { a: 'y'.repeat(5000) }]], + ], + ['a non-object `extra`', [['service:x:ii:read', 'nope']]], + ['an empty permission string', ['']], + ])('rejects %s with 400', async (_label, permissions) => { + await expect( + controller.handleCreateAccessToken( + makeReq({ permissions: permissions as never[] }, { actor }), + makeRes(), + ), + ).rejects.toMatchObject({ statusCode: 400 }); + }); + it('mints a verifiable access-token JWT for valid permissions', async () => { const res = makeRes(); await controller.handleCreateAccessToken( @@ -6779,6 +6891,34 @@ describe('AuthController dev-app permission flows', () => { ).rejects.toMatchObject({ statusCode: 400 }); }); + it.each([ + ['a non-object `extra`', { extra: 'nope' }], + ['an array `extra`', { extra: [1, 2] }], + ['an oversized `extra`', { extra: { blob: 'z'.repeat(5000) } }], + ['an oversized `meta`', { meta: { blob: 'z'.repeat(5000) } }], + ['an oversized `permission`', { permission: 'x'.repeat(4097) }], + // Survives the route cap but not the column it lands in. + [ + 'a `permission` wider than the column', + { permission: `service:${'a'.repeat(300)}:ii:read` }, + ], + ])('grant-dev-app: 400 on %s', async (_label, patch) => { + const { actor } = await makeUserAndActor(); + await expect( + controller.handleGrantDevApp( + makeReq( + { + app_uid: `app-${uuidv4()}`, + permission: 'fs:read', + ...patch, + }, + { actor }, + ), + makeRes(), + ), + ).rejects.toMatchObject({ statusCode: 400 }); + }); + it('revoke-dev-app: 400 on missing app_uid/origin/permission', async () => { const { actor } = await makeUserAndActor(); await expect( diff --git a/src/backend/controllers/auth/AuthController.ts b/src/backend/controllers/auth/AuthController.ts index 8304f6cbe..0676e0f1c 100644 --- a/src/backend/controllers/auth/AuthController.ts +++ b/src/backend/controllers/auth/AuthController.ts @@ -116,6 +116,8 @@ const FINGERPRINT_MAX_LENGTH = 128; // One consent prompt covers a handful of scopes at most. The cap keeps a // crafted request from turning a single grant call into a bulk write. const MAX_PERMISSIONS_PER_REQUEST = 16; +// Rides every row a request writes, and the per-(user, app) cache after that. +const GRANT_EXTRA_MAX_BYTES = 4096; const DISPATCH_ID_MAX_LENGTH = 128; // One name for the flag, so the write and the read cannot drift apart. const APP_AUTHENTICATED_FLAG = 'flag:app-is-authenticated'; @@ -3297,6 +3299,16 @@ export class AuthController extends PuterController { legacyCode: 'bad_request', }); } + if ( + Buffer.byteLength(JSON.stringify(value), 'utf8') > + GRANT_EXTRA_MAX_BYTES + ) { + throw new HttpError( + 400, + `\`${key}\` may not exceed ${GRANT_EXTRA_MAX_BYTES} bytes`, + { legacyCode: 'bad_request' }, + ); + } } } @@ -3864,6 +3876,13 @@ export class AuthController extends PuterController { async handleGrantDevApp(req: Request, res: Response): Promise { let { app_uid } = req.body ?? {}; const { origin, permission, extra, meta } = req.body ?? {}; + this.#validateAppPermissionParams({ + app_uid, + origin, + permission, + extra, + meta, + }); if (origin && !app_uid) { // Registered apps only, for the same reason the user-app handlers // insist on it: a synthesised `app-` is resolved @@ -4204,6 +4223,11 @@ export class AuthController extends PuterController { legacyCode: 'bad_request', }); } + if (permissions.length > MAX_PERMISSIONS_PER_REQUEST) { + throw new HttpError(400, 'Too many `permissions`', { + legacyCode: 'bad_request', + }); + } // Optional user-facing name for the manage-sessions UI. Trim and clamp // to the same 64-char limit the rename endpoint enforces. @@ -4237,13 +4261,25 @@ export class AuthController extends PuterController { // Normalize specs: string → [string], [string] → [string, {}], [string, extra] → as-is const normalized = permissions.map((spec) => { - if (typeof spec === 'string') return [spec]; - if (Array.isArray(spec)) return spec; - throw new HttpError( - 400, - 'Each permission must be a string or [string, extra?]', - { legacyCode: 'bad_request' }, - ); + const entry = + typeof spec === 'string' + ? [spec] + : Array.isArray(spec) + ? spec + : null; + if (!entry || typeof entry[0] !== 'string' || !entry[0]) { + throw new HttpError( + 400, + 'Each permission must be a string or [string, extra?]', + { legacyCode: 'bad_request' }, + ); + } + // Same caps the grant routes apply, before a session or row exists. + this.#validateAppPermissionParams({ + permission: entry[0], + extra: entry[1], + }); + return entry; }); const token = await this.services.auth.createAccessToken( diff --git a/src/backend/services/auth/AuthService.ts b/src/backend/services/auth/AuthService.ts index 5ed968fcc..dfc313215 100644 --- a/src/backend/services/auth/AuthService.ts +++ b/src/backend/services/auth/AuthService.ts @@ -36,7 +36,7 @@ import type { LayerInstances } from '../../types'; import { sessionCookieFlags } from '../../util/cookieFlags.js'; import { Span } from '../../util/span.js'; import type { puterServices } from '../index'; -import { FULL_API_ACCESS } from '../permission/consts'; +import { FULL_API_ACCESS, PERMISSION_MAX_LEN } from '../permission/consts'; import { PuterService } from '../types'; import type { AccessTokenPayload, @@ -1517,6 +1517,18 @@ export class AuthService extends PuterService { ); } + // Unrewritten: the column width is the limit, and no row exists yet. + for (const [permission] of permissions) { + if ( + typeof permission === 'string' && + permission.length > PERMISSION_MAX_LEN + ) { + throw new HttpError(400, 'Invalid `permission`', { + legacyCode: 'bad_request', + }); + } + } + // Full-API-access sentinel: a token that may do anything its issuing // user can do via the API (resolved against the issuer at check time — // see PermissionService.#scanAccessToken). Only a plain user actor may diff --git a/src/backend/services/permission/PermissionService.ts b/src/backend/services/permission/PermissionService.ts index 0dc709de3..797c50c34 100644 --- a/src/backend/services/permission/PermissionService.ts +++ b/src/backend/services/permission/PermissionService.ts @@ -1553,6 +1553,12 @@ export class PermissionService extends PuterService { ): Promise { permission = await this.rewritePermission(permission); this.assertGrantableFsPermission(permission); + // Post-rewrite, for the same reason as the user-app grant above. + if (permission.length > PERMISSION_MAX_LEN) { + throw new HttpError(400, 'Invalid `permission`', { + legacyCode: 'bad_request', + }); + } const app = await this.stores.app.resolveApp(appIdentifier); if (!app) throw new HttpError(404, `entity_not_found: app:${appIdentifier}`, { diff --git a/src/backend/stores/permission/PermissionStore.test.ts b/src/backend/stores/permission/PermissionStore.test.ts index 6093c3ae0..a718ec766 100644 --- a/src/backend/stores/permission/PermissionStore.test.ts +++ b/src/backend/stores/permission/PermissionStore.test.ts @@ -232,6 +232,23 @@ describe('PermissionStore', () => { // -- prefix deletion ----------------------------------------------- describe('deleteAppGrantsByPermissionPrefix', () => { + it.each(['user_to_app_permissions', 'dev_to_app_permissions'])( + 'has an index leading with `permission` on %s', + async (table) => { + // Every other index on these tables leads with another column. + const postgres = server.clients.db.engineName === 'postgres'; + const rows = (await server.clients.db.read( + postgres + ? 'SELECT indexname AS name FROM pg_indexes WHERE tablename = ?' + : "SELECT name FROM sqlite_master WHERE type = 'index' AND tbl_name = ?", + [table], + )) as Array<{ name: string }>; + expect(rows.map((r) => r.name)).toContain( + `idx_${table}_permission`, + ); + }, + ); + it('removes the exact permission and its subtree, across both tables', async () => { const user = await makeUser(); const app = await makeApp(user.id); diff --git a/src/docs/src/rate-limits-and-quotas.md b/src/docs/src/rate-limits-and-quotas.md index 97e673c0b..e52fcfc24 100644 --- a/src/docs/src/rate-limits-and-quotas.md +++ b/src/docs/src/rate-limits-and-quotas.md @@ -171,7 +171,8 @@ The Puter desktop makes PDF upload thumbnails locally within these budgets. Goin | Limit | Value | | ----- | ----- | | Grant / revoke calls | 60/min | -| Permissions per grant or revoke request | 16 | +| Permissions per grant, revoke, or access-token request | 16 | +| `extra` / `meta` on a grant or access-token permission | 4 KiB each | | Filesystem entries one `create` grant may create per request | 4 | | Path depth a `create` grant may create below the home directory | 16 components |