Skip to content

Commit 1b87aab

Browse files
committed
fix(storage): parse resolver URLs structurally instead of with a backtracking regex
CodeQL js/polynomial-redos: the resolver-URL pattern was anchored only at the tail, so the engine retried from every start position. The values it scans come straight out of tenant records, so a value repeating the matched prefix turns that into a polynomial blowup on attacker-influenced data. Replaced with slicing and splitting — linear in the URL's length whatever it contains, and clearer about what it actually accepts. Covered by a timing regression on an adversarial value plus a table of accepted shapes and near-misses. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SHpGw3GBA9aFpfwVArRWfd
1 parent 8bbaaa3 commit 1b87aab

2 files changed

Lines changed: 76 additions & 5 deletions

File tree

packages/services/service-storage/src/backfill-file-references.test.ts

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -231,6 +231,54 @@ describe('backfillFileReferences (ADR-0104 D3 wave 2)', () => {
231231
expect(report.scannedObjects).toEqual(['product']);
232232
});
233233

234+
/**
235+
* The values scanned here come straight out of tenant records, so URL
236+
* matching must not be able to backtrack. A pattern anchored only at the tail
237+
* retries from every start position, which a value repeating the matched
238+
* prefix turns into a polynomial blowup (CodeQL js/polynomial-redos).
239+
*/
240+
it('matches resolver URLs in linear time on an adversarial value', async () => {
241+
// Many repetitions of the matched prefix, ending in a segment that is not
242+
// an id token — the shape that makes a tail-anchored pattern retry from
243+
// every start position.
244+
const hostile = '/storage/files/'.repeat(6000) + '!';
245+
const engine = fakeEngine({ product: [{ id: 'p1', image: hostile }], sys_file: [] });
246+
247+
const started = process.hrtime.bigint();
248+
const report = await run(engine, {});
249+
const elapsedMs = Number(process.hrtime.bigint() - started) / 1e6;
250+
251+
// Not a reference (the trailing segment is not an id token) — and reaching
252+
// that verdict must not depend on how many times the prefix repeats.
253+
expect(report.converted).toBe(0);
254+
expect(elapsedMs).toBeLessThan(1000);
255+
});
256+
257+
it('accepts the resolver URL shapes and rejects near-misses', async () => {
258+
const engine = fakeEngine({
259+
product: [
260+
{ id: 'p1', image: 'https://app.example.com/api/v1/storage/files/aaa' },
261+
{ id: 'p2', image: '/api/v1/storage/files/bbb/url' },
262+
{ id: 'p3', image: '/api/v1/storage/files/ccc?v=2' },
263+
{ id: 'p4', image: '/api/v1/storage/files/ddd/' },
264+
{ id: 'p5', image: 'https://cdn.example.com/files/eee' }, // no /storage
265+
{ id: 'p6', image: '/api/v1/storage/files/' }, // no id
266+
],
267+
sys_file: [],
268+
});
269+
270+
await run(engine, { apply: true });
271+
272+
expect(engine.tables.product.map((r) => r.image)).toEqual([
273+
'aaa',
274+
'bbb',
275+
'ccc',
276+
'ddd',
277+
'https://cdn.example.com/files/eee',
278+
'/api/v1/storage/files/',
279+
]);
280+
});
281+
234282
it('pages through large objects and flags truncation at the bound', async () => {
235283
const many = Array.from({ length: 500 }, (_, i) => ({
236284
id: `p${i}`,

packages/services/service-storage/src/backfill-file-references.ts

Lines changed: 28 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -41,8 +41,6 @@ import type { IStorageService } from '@objectstack/spec/contracts';
4141
const SYSTEM_CTX = { isSystem: true } as const;
4242
const SCAN_PAGE_SIZE = 200;
4343

44-
/** `…/storage/files/<id>` (optionally with a trailing `/url` or query). */
45-
const LOCAL_FILE_URL_RE = /\/storage\/files\/([A-Za-z0-9_-]{1,64})(?:\/url)?(?:[?#].*)?$/;
4644
/** `data:<mime>;base64,<payload>` — the only inline form carrying real bytes. */
4745
const DATA_URI_RE = /^data:([^;,]*)(;base64)?,(.*)$/s;
4846

@@ -116,6 +114,32 @@ function truncate(v: string, n = 96): string {
116114
return v.length <= n ? v : `${v.slice(0, n)}…`;
117115
}
118116

117+
/**
118+
* The `sys_file` id a URL names, if it points at this platform's own resolver
119+
* (`…/storage/files/<id>`, optionally `/url`-suffixed or query-decorated).
120+
*
121+
* Parsed structurally rather than with a pattern. The obvious regex for this is
122+
* unanchored at the head, so a hostile value repeating `/storage/files/` makes
123+
* the engine retry from every position — polynomial backtracking on data that,
124+
* here, comes straight out of tenant records. Slicing and splitting is linear
125+
* in the URL's length no matter what it contains.
126+
*/
127+
function localFileIdFrom(url: string): string | null {
128+
let path = url;
129+
for (const sep of ['?', '#']) {
130+
const at = path.indexOf(sep);
131+
if (at >= 0) path = path.slice(0, at);
132+
}
133+
if (path.endsWith('/url')) path = path.slice(0, -'/url'.length);
134+
while (path.endsWith('/')) path = path.slice(0, -1);
135+
136+
const parts = path.split('/');
137+
if (parts.length < 3) return null;
138+
if (parts[parts.length - 3] !== 'storage' || parts[parts.length - 2] !== 'files') return null;
139+
const id = parts[parts.length - 1];
140+
return isFileIdToken(id) ? id : null;
141+
}
142+
119143
/** The URL a legacy value points at, if any (inline blob or bare string). */
120144
function urlOf(value: unknown): string | null {
121145
if (typeof value === 'string') return value;
@@ -258,9 +282,8 @@ export async function backfillFileReferences(
258282
continue;
259283
}
260284

261-
const local = LOCAL_FILE_URL_RE.exec(url);
262-
if (local) {
263-
const fileId = local[1];
285+
const fileId = localFileIdFrom(url);
286+
if (fileId) {
264287
actions.push({
265288
...record_,
266289
kind: 'resolved_local_url',

0 commit comments

Comments
 (0)