security(padid): reject ueberdb delimiter ':' in pad ids (copyPad/movePad injection)#8073
security(padid): reject ueberdb delimiter ':' in pad ids (copyPad/movePad injection)#8073JohnMcLear wants to merge 2 commits into
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.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…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; | ||
| } |
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