Skip to content

Commit 083c414

Browse files
os-zhuangclaude
andauthored
fix(runtime): kill polynomial-redos trailing-slash regex in notifications domain (CodeQL high from #3507) (#3510)
The extraction PR moved the legacy `.replace(/\/+$/, '')` verbatim into domains/notifications.ts, which made it "changed code" and surfaced a js/polynomial-redos CodeQL alert that the line had latently carried since ADR-0030. Fix is the same split+filter treatment the security domain already uses for the identical pattern (its comment even cites the rule). Side effect: redundant slashes collapse ('//read//' → 'read'), consistent with the security domain; locked by a new test. Verified: seam suite 19 tests, runtime 624 green, DTS build green. Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 8f124a7 commit 083c414

3 files changed

Lines changed: 26 additions & 1 deletion

File tree

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
"@objectstack/runtime": patch
3+
---
4+
5+
fix(runtime): replace the polynomial-redos trailing-slash regex in the notifications domain with split+filter (CodeQL high, surfaced by #3507)
6+
7+
The legacy `path.replace(/\/+$/, '')` in the notifications handler had
8+
carried a polynomial-backtracking regex over request-controlled input since
9+
ADR-0030; the domain extraction (#3507) made the line "changed code" and
10+
CodeQL flagged it. Same split+filter treatment the security domain already
11+
uses for the identical pattern. Redundant slashes in the sub-path now
12+
collapse (`//read//``read`), matching the security domain's semantics.

packages/runtime/src/domain-handler-registry.test.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,14 @@ describe('HttpDispatcher extracted domains (PR-2)', () => {
187187
expect(notification.listInbox).toHaveBeenCalledWith('u1', expect.objectContaining({ limit: 5 }));
188188
});
189189

190+
it('/notifications tolerates redundant slashes in the sub-path (split+filter, CodeQL redos fix)', async () => {
191+
const notification = { listInbox: vi.fn(), markRead: vi.fn().mockResolvedValue({ updated: 1 }), markAllRead: vi.fn() };
192+
const context: any = { executionContext: { userId: 'u1' } };
193+
const result = await makeDispatcher({ notification }).handleNotification('//read//', 'POST', { ids: ['n1'] }, {}, context);
194+
expect(result.response?.status).toBe(200);
195+
expect(notification.markRead).toHaveBeenCalledWith('u1', ['n1']);
196+
});
197+
190198
it('/security responds 503 when no security service is wired (legacy in-handler semantics)', async () => {
191199
const result = await makeDispatcher().dispatch('GET', '/security/suggested-bindings', undefined, {}, {} as any);
192200
expect(result.handled).toBe(true);

packages/runtime/src/domains/notifications.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,12 @@ export async function handleNotificationRequest(
4949
}
5050

5151
const m = method.toUpperCase();
52-
const subPath = path.replace(/^\/+/, '').replace(/\/+$/, '');
52+
// split+filter drops leading/trailing/duplicate slashes without a regex
53+
// over request-controlled input (CodeQL js/polynomial-redos) — same
54+
// treatment the security domain got for the identical latent pattern.
55+
// Surfaced when the extraction (#3507) made this line "changed code":
56+
// the legacy `.replace(/\/+$/, '')` had carried the trap since ADR-0030.
57+
const subPath = path.split('/').filter(Boolean).join('/');
5358

5459
// GET /notifications — list the user's inbox joined with read-state.
5560
if (subPath === '' && m === 'GET') {

0 commit comments

Comments
 (0)