mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-18 21:16:47 +00:00
fix(server): accept timeoutMs as a fetch deadline and close the null init gap
The outbound-timeout selectors flagged safeFetch(url, undefined,
{ timeoutMs }) and safeFetch(url, { headers }, { timeoutMs }), although
timeoutMs is the deadline over the whole exchange that server/CLAUDE.md
tells callers to pass. At the same time an init spelled null or void 0
passed unseen, so a guarded call could still go without any timeout.
A call whose third argument is an object literal with a timeoutMs key is
now exempt, and undefined, null and void 0 (also behind an as cast) all
count as no init. The cast forms pin the inner node type, because esquery
compares String(value) and a missing name would otherwise read as
'undefined' and flag every cast init. The selector test covers both
sides.
This commit is contained in:
+1
-1
@@ -48,7 +48,7 @@ Nest owns everything. Every domain is a DI module under `src/nest/<domain>/` (`c
|
||||
- **Transactions are not optional.** Multi-statement writes go in `await this.uow.transactional(async () => …)` (`UnitOfWork` from `nest/database`, provided by the global `OrmModule`); never hand-roll `BEGIN`/`COMMIT` and never call `db.transaction()` (the Phase 1 async sweep removes it). Every DB-touching method is `async`; `@typescript-eslint/no-floating-promises` and `no-misused-promises` are errors, so an unawaited call or a `Promise` in a condition fails lint. Bind values with `?`; identifiers only via a literal allow-list. In `src/db/repositories/` and `src/db/seeders/`, ESLint refuses raw SQL in any spelling: `raw()`, the `sql` tag, and SQL text handed to `execute`/`run`/`all`/`get`. Build the statement with the query builder; `MaintenanceRepository`, `DemoRepository` and the six existing seeders are the named exceptions in `eslint.config.mjs`.
|
||||
- **Migrations are append-only.** A migration is identified by its class name in `mikro_orm_migrations`, and files sort by the timestamp in that name — never rename or delete one that has shipped, and never edit its body; add a new one. No env interpolation in SQL. (The old positional `schema_version` array and the hand-kept `db/migrations.ts` test builder are both gone; the numbered migrations are the only schema history, for tests too.) A new foreign key comes with an index that leads with its column(s), or deleting the parent scans the child table; `tests/unit/db/foreign-key-indexes.test.ts` fails on one without.
|
||||
- **Every write endpoint validates** through the Zod pipe with a schema from `@trek/shared` — no `Record<string, unknown>` + ad-hoc checks. Also verify every referenced id exists and belongs to the same trip.
|
||||
- **Every outbound `fetch`** gets a timeout, a response-size cap and boundary validation — no `as`-casting provider responses, no silent `catch → null`. User-influenced URLs go through the SSRF guard. Never fall back to another user's API key. ESLint requires an init with a `signal` on `fetch`, `safeFetch`, `safeFetchFollow` and `safeFetchAdminConfigured` alike (`safeFetchLlm` takes `LLM_TIMEOUT_MS` inside). Behind that, the guarded wrappers (`utils/ssrfGuard.ts`) cap every body at `DEFAULT_MAX_RESPONSE_BYTES` (64 MB) and, for a call without a signal, wait `DEFAULT_RESPONSE_TIMEOUT_MS` (30 s) for the headers. Pass a tighter `maxBytes` when you buffer a small answer, `timeoutMs` for a deadline over the whole exchange, and `maxBytes: null` only where the body is streamed on or counted by the caller (`pipeAsset`, the doc-sync transfers, the Ollama pull).
|
||||
- **Every outbound `fetch`** gets a timeout, a response-size cap and boundary validation — no `as`-casting provider responses, no silent `catch → null`. User-influenced URLs go through the SSRF guard. Never fall back to another user's API key. ESLint requires an init with a `signal` on `fetch`, `safeFetch`, `safeFetchFollow` and `safeFetchAdminConfigured` alike, or a literal `timeoutMs` in the options object of `safeFetch`/`safeFetchFollow`; an init spelled `undefined`, `null` or `void 0` counts as none (`safeFetchLlm` takes `LLM_TIMEOUT_MS` inside). Behind that, the guarded wrappers (`utils/ssrfGuard.ts`) cap every body at `DEFAULT_MAX_RESPONSE_BYTES` (64 MB) and, for a call without a signal, wait `DEFAULT_RESPONSE_TIMEOUT_MS` (30 s) for the headers. Pass a tighter `maxBytes` when you buffer a small answer, `timeoutMs` for a deadline over the whole exchange, and `maxBytes: null` only where the body is streamed on or counted by the caller (`pipeAsset`, the doc-sync transfers, the Ollama pull).
|
||||
- **Fail closed.** Unrecognized flag values mean OFF; sandbox/permission opt-outs refuse in production; never pick a less-safe path based on the *absence* of a file. GET handlers must not write.
|
||||
- **Don't fork canonical paths.** Auth verification is `verifyJwtAndLoadUser` (with the `password_version` gate) — reuse it.
|
||||
- **Money**: integer cents; largest-remainder splits (`BudgetService.splitEqualShares` is the reference); FX frozen at entry; explicit flags, never sentinel values; custom splits must reconcile to zero before persisting.
|
||||
|
||||
@@ -46,6 +46,27 @@ const RAW_SQL_SELECTORS = [
|
||||
},
|
||||
];
|
||||
|
||||
// A guarded call whose options object (third argument) sets `timeoutMs` has a
|
||||
// deadline over the whole exchange, so it does not need a signal in the init.
|
||||
// esquery only resolves `:nth-child` inside `:has` when each step is its own
|
||||
// nested `:has`, hence the shape.
|
||||
const FETCH_CALL = `CallExpression[callee.name=${FETCH_CALLEE}]:not(:has(> ObjectExpression.arguments:nth-child(3):has(> Property[key.name='timeoutMs'])))`;
|
||||
|
||||
// An argument that stands for "no init": undefined, null or void 0, also behind an `as` cast.
|
||||
const NO_INIT = [
|
||||
"Identifier[name='undefined']",
|
||||
"Literal[raw='null']",
|
||||
"UnaryExpression[operator='void']",
|
||||
// esquery compares String(value), so a missing `name` would read as 'undefined':
|
||||
// the cast's inner node type is pinned first.
|
||||
"TSAsExpression[expression.type='Identifier'][expression.name='undefined']",
|
||||
"TSAsExpression[expression.type='Literal'][expression.raw='null']",
|
||||
"TSAsExpression[expression.type='UnaryExpression'][expression.operator='void']",
|
||||
].join(', ');
|
||||
|
||||
const FETCH_TIMEOUT_MESSAGE =
|
||||
'Outbound fetch needs a timeout: pass { signal: AbortSignal.timeout(ms) }, or { timeoutMs } in the options of safeFetch/safeFetchFollow.';
|
||||
|
||||
const FETCH_SELECTORS = [
|
||||
{
|
||||
// Every outbound fetch needs a timeout (server/CLAUDE.md): a provider
|
||||
@@ -55,17 +76,17 @@ const FETCH_SELECTORS = [
|
||||
// wrappers in utils/ssrfGuard.ts are held to the same rule: they hand
|
||||
// the platform a spread init, so the rule cannot see through them, and
|
||||
// their own fallback only bounds the wait for the headers.
|
||||
selector: `CallExpression[callee.name=${FETCH_CALLEE}][arguments.length=1]`,
|
||||
message: 'Outbound fetch needs a timeout: pass { signal: AbortSignal.timeout(ms) }.',
|
||||
selector: `${FETCH_CALL}[arguments.length=1]`,
|
||||
message: FETCH_TIMEOUT_MESSAGE,
|
||||
},
|
||||
{
|
||||
selector: `CallExpression[callee.name=${FETCH_CALLEE}] > ObjectExpression.arguments:nth-child(2):not(:has(Property[key.name='signal'])):not(:has(SpreadElement))`,
|
||||
message: 'Outbound fetch needs a timeout: pass { signal: AbortSignal.timeout(ms) }.',
|
||||
selector: `${FETCH_CALL} > ObjectExpression.arguments:nth-child(2):not(:has(Property[key.name='signal'])):not(:has(SpreadElement))`,
|
||||
message: FETCH_TIMEOUT_MESSAGE,
|
||||
},
|
||||
{
|
||||
// `safeFetch(url, undefined, options)` is the same call without an init.
|
||||
selector: `CallExpression[callee.name=${FETCH_CALLEE}] > Identifier.arguments:nth-child(2)[name='undefined']`,
|
||||
message: 'Outbound fetch needs a timeout: pass { signal: AbortSignal.timeout(ms) }.',
|
||||
selector: `${FETCH_CALL} > :matches(${NO_INIT}).arguments:nth-child(2)`,
|
||||
message: FETCH_TIMEOUT_MESSAGE,
|
||||
},
|
||||
];
|
||||
|
||||
|
||||
@@ -51,7 +51,8 @@ declare const init: object;
|
||||
declare const signal: AbortSignal;
|
||||
`;
|
||||
|
||||
const TIMEOUT = 'Outbound fetch needs a timeout: pass { signal: AbortSignal.timeout(ms) }.';
|
||||
const TIMEOUT =
|
||||
'Outbound fetch needs a timeout: pass { signal: AbortSignal.timeout(ms) }, or { timeoutMs } in the options of safeFetch/safeFetchFollow.';
|
||||
|
||||
describe('outbound fetch timeout selectors', () => {
|
||||
let options: unknown[];
|
||||
@@ -67,6 +68,14 @@ describe('outbound fetch timeout selectors', () => {
|
||||
['safeFetchFollow(url, undefined, { bypassInternalIpAllowed: true })'],
|
||||
["safeFetchFollow(url, { headers: { a: 'b' } })"],
|
||||
['safeFetchAdminConfigured(url, {})'],
|
||||
// Other spellings of "no init" must not slip past the undefined case.
|
||||
['safeFetch(url, void 0, { rejectUnauthorized: false })'],
|
||||
['safeFetch(url, null, { rejectUnauthorized: false })'],
|
||||
['safeFetch(url, null as never, { maxBytes: 1024 })'],
|
||||
['safeFetch(url, undefined as never, {})'],
|
||||
// A timeoutMs nested in the init is not the options deadline.
|
||||
['safeFetch(url, { headers: { timeoutMs: 5 } })'],
|
||||
['safeFetch(url, undefined, { maxBytes: 1024 }, { timeoutMs: 5000 })'],
|
||||
])('flags %s', (call) => {
|
||||
expect(messages(options, `${PRELUDE}\nvoid ${call};`)).toEqual([TIMEOUT]);
|
||||
});
|
||||
@@ -81,6 +90,14 @@ describe('outbound fetch timeout selectors', () => {
|
||||
['safeFetchFollow(url, { signal }, { bypassInternalIpAllowed: true })'],
|
||||
// safeFetchLlm takes its deadline from LLM_TIMEOUT_MS inside the wrapper.
|
||||
["safeFetchLlm(url, { method: 'POST' })"],
|
||||
// timeoutMs in the options is a deadline over the whole exchange.
|
||||
['safeFetch(url, undefined, { timeoutMs: 5000 })'],
|
||||
['safeFetch(url, void 0, { timeoutMs: 5000, maxBytes: 1024 })'],
|
||||
['safeFetch(url, { headers: {} }, { timeoutMs: 5000 })'],
|
||||
['safeFetchFollow(url, undefined, { timeoutMs: 5000 })'],
|
||||
// A cast init is still an init: the "no init" casts must not match it.
|
||||
['fetch(url, { ...init, signal } as object)'],
|
||||
['safeFetch(url, init as object, { maxBytes: 1024 })'],
|
||||
])('accepts %s', (call) => {
|
||||
expect(messages(options, `${PRELUDE}\nvoid ${call};`)).toEqual([]);
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user