security(padid): reject ueberdb delimiter ':' in pad ids (copyPad/movePad injection) - #8073
Conversation
…ePad injection) GHSA-wg58-mhwv-35pq. copyPad / movePad / copyPadWithoutHistory take their destination via the `destinationID` API field, which — unlike padID / padName — is never run through sanitizePadId. Because isValidPadId only forbade `$`, a destinationID like `victim:revs:0` survived into the engine: the embedded `:` (the ueberdb key-namespace delimiter) let it address another pad's internal `pad:<id>:revs:<n>` records. That both bypassed the force=false "destination already exists" guard (which checks only the top-level `atext`) and clobbered the victim pad's revision history. - isValidPadId now rejects `:` (never legal in a pad id; it's the DB key delimiter). The name portion excludes `$` and `:`. - Pad.copy() and Pad.copyPadWithoutHistory() validate destinationID via isValidPadId before any db write; movePad routes through copy(), so the check runs before the source is removed. Adds isValidPadId unit cases and an integration test proving copyPad / copyPadWithoutHistory reject a `:`-bearing destinationID with force=false and leave the victim's rev-0 changeset untouched, while a normal copy still works. Reported privately (finder credited on the advisory). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code Review by Qodo
1.
|
PR Summary by QodoReject ':' in pad IDs to block copyPad/movePad destinationID injection
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
There was a problem hiding this comment.
Pull request overview
This PR addresses a pad ID injection/cross-pad corruption vector by disallowing ueberdb’s key-namespace delimiter (:) in pad IDs and by validating destinationID early in the copyPad/movePad/copyPadWithoutHistory code paths before any database writes.
Changes:
- Update
PadManager.isValidPadId()to reject:(and add unit coverage for the new rule). - Add
destinationIDvalidation toPad.copy()andPad.copyPadWithoutHistory()to prevent delimiter-based DB key targeting. - Add an integration regression test that ensures
copyPad/copyPadWithoutHistoryreject:-bearing destination IDs and do not clobber another pad’s revision records.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/node/db/PadManager.ts | Reject : in isValidPadId() to prevent ueberdb key-namespace delimiter abuse. |
| src/node/db/Pad.ts | Validate destinationID via isValidPadId() before performing any copy-related DB writes. |
| src/tests/backend/specs/PadManager.ts | Add unit tests ensuring :-containing pad IDs are rejected (including group-pad form). |
| src/tests/backend/specs/copyPadColonInjection.ts | Add integration regression coverage for the destinationID : injection/corruption scenario. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| exports.isValidPadId = (padId: string) => | ||
| /^(g.[a-zA-Z0-9]{16}\$)?[^$]{1,50}$/.test(padId) && !dotSegmentPadId.test(padId); | ||
| /^(g.[a-zA-Z0-9]{16}\$)?[^$:]{1,50}$/.test(padId) && !dotSegmentPadId.test(padId); |
| // `:` is the ueberdb key-namespace delimiter (records are stored under | ||
| // `pad:<id>`, `pad:<id>:revs:<n>`, `pad:<id>:chat:<n>`). A pad id containing a | ||
| // `:` can therefore address another pad's internal sub-records, so it is never | ||
| // valid — the name portion excludes `$` (group-pad separator) and `:`. | ||
| // (GHSA-wg58-mhwv-35pq: copyPad/movePad destinationID injection.) | ||
| exports.isValidPadId = (padId: string) => | ||
| /^(g.[a-zA-Z0-9]{16}\$)?[^$]{1,50}$/.test(padId) && !dotSegmentPadId.test(padId); | ||
| /^(g.[a-zA-Z0-9]{16}\$)?[^$:]{1,50}$/.test(padId) && !dotSegmentPadId.test(padId); |
Code Review by Qodo
1.
|
…s still redirect Addresses Qodo review on #8073. Rejecting ":" in isValidPadId made padurlsanitize's validate-first ordering return 404 for a browser visiting a legacy `/p/<id with ":">` URL, instead of redirecting it to the sanitized `_` form. Reorder padurlsanitize to sanitize FIRST (sanitizePadId maps whitespace and ":" to "_"), then validate the sanitized id. `/p/foo:bar` now redirects to `/p/foo_bar` as before; an id that stays invalid after sanitizing (e.g. containing "$") is still forbidden. This restores the pre-fix redirect behavior while keeping ":" out of stored pad ids. Adds a regression test for the ":" redirect, the "$" 404, and a clean id. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks @qodo-free-for-open-source-projects — correct, rejecting |
Thanks for the update. Sanitizing before validation preserves the legacy redirect behavior while still preventing |
Thanks for the update. The sanitize-before-validation ordering preserves legacy |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/node/db/PadManager.ts:208
- The updated isValidPadId regex still allows ':' in the group-pad prefix because
g.uses.(any character) instead of a literal dot. That means pad IDs likeg:AAAAAAAAAAAAAAAA$padwould pass validation even though they contain the ueberdb delimiter, undermining the intent of rejecting ':' everywhere and also loosening the documented groupID format (g.<16 chars>).
exports.isValidPadId = (padId: string) =>
/^(g.[a-zA-Z0-9]{16}\$)?[^$:]{1,50}$/.test(padId) && !dotSegmentPadId.test(padId);
| // Reject URLs ending in `/` outright. | ||
| if (/\/$/.test(req.url)) { | ||
| res.status(404).send('Such a padname is forbidden'); | ||
| return; | ||
| } | ||
|
|
||
| // Sanitize FIRST, then validate the sanitized result. sanitizePadId maps | ||
| // legacy characters (whitespace and the ueberdb delimiter `:`) to `_`, so | ||
| // a URL like `/p/foo:bar` still redirects to `/p/foo_bar` even though `:` | ||
| // is not itself a valid pad id (GHSA-wg58-mhwv-35pq). An id that stays | ||
| // invalid after sanitizing (e.g. one containing `$`) is forbidden. | ||
| const sanitizedPadId = await padManager.sanitizePadId(padId); | ||
|
|
||
| if (!padManager.isValidPadId(sanitizedPadId)) { | ||
| res.status(404).send('Such a padname is forbidden'); | ||
| return; | ||
| } |
Rejecting ":" in isValidPadId also gates padManager.getPad(), so pads whose id contains a ":" — legal before GHSA-wg58-mhwv-35pq, which is why padIdTransforms maps ":" at all — became unopenable, not just uncreatable: sanitizePadId returns such an id unchanged once the pad exists, and getPad then threw. Refuse an invalid id only when no pad with that exact id exists (in getPad and in the pad-URL param). The injection primitive stays closed: doesPadExist() requires a top-level `atext`, which the `pad:<id>:revs:<n>` sub-records an injected destinationID addresses do not have. Copy destinations keep the strict check, so no new ":" id can be created. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Pushed a follow-up (bc3687e) before merging. Rejecting So both gates now refuse an invalid id only when no pad with that exact id exists. The injection primitive stays closed: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/node/db/PadManager.ts:216
- The
isValidPadIdregex usesg.in the group-pad prefix, but.in a regex matches any character. That would treat IDs likegX<16 chars>$nameas valid group pads, which is likely unintended and loosens pad ID validation. Escaping the dot makes the prefix strictlyg..
exports.isValidPadId = (padId: string) =>
/^(g.[a-zA-Z0-9]{16}\$)?[^$:]{1,50}$/.test(padId) && !dotSegmentPadId.test(padId);
| // flush the source pad | ||
| this.saveToDatabase(); | ||
|
|
Fixes GHSA-wg58-mhwv-35pq (triage). Verified still present on
develop.copyPad/movePad/copyPadWithoutHistorytake their destination via thedestinationIDAPI field, which — unlikepadID/padName— is never run throughsanitizePadId. BecauseisValidPadIdonly forbade$, a destinationID likevictim:revs:0survived into the engine. The embedded:(the ueberdb key-namespace delimiter,pad:<id>:revs:<n>) let it address another pad's internal records, which:force=falseoverwrite guard —doesPadExistchecks the top-levelatext, but a:revs:record stores atext undermeta, so it read as "doesn't exist"; andRequires the global API key / OAuth-admin (PR:H), so low severity, but it's a clean cross-pad corruption primitive.
Fix
isValidPadIdnow rejects:(never legal in a pad id — it's the DB key delimiter). Name portion excludes both$and:.Pad.copy()andPad.copyPadWithoutHistory()validatedestinationIDviaisValidPadIdbefore any db write.movePadroutes throughcopy(), so the check runs before the source is removed (a bad destination can't delete the source either).Tests
isValidPadIdunit cases for:rejection (incl. group-pad form).copyPad/copyPadWithoutHistoryreject a:-bearing destinationID withforce=falseand leave the victim's rev-0 changeset untouched; a normal copy still works.tsc --noEmitclean.🤖 Generated with Claude Code