fix(server): reject path traversal in run/comparison IDs and drop wildcard CORS - #61
Open
rajarshidattapy wants to merge 1 commit into
Open
Conversation
|
Again. Please STOP sending me these changes. I tried to unsubscribe and remove from your repository and I can't. WTF!
Email is ***@***.***
Cheers,
Michael Craige
Get Outlook for iOS<https://aka.ms/o0ukef>
…________________________________
From: Rajarshi Datta ***@***.***>
Sent: Thursday, 13 August 2026 12:35:40
To: supermemoryai/memorybench ***@***.***>
Cc: Subscribed ***@***.***>
Subject: [supermemoryai/memorybench] fix(server): reject path traversal in run/comparison IDs and drop wildcard CORS (PR #61)
Fixes #60<#60> — arbitrary directory deletion via DELETE /api/runs/:runId, reachable from any website.
The bug
Route patterns like /api/runs/([^/]+) match before decodeURIComponent runs. A URL
pathname keeps %2F encoded, so the segment matches, and only afterwards becomes a real
separator:
DELETE /api/runs/..%2F..%2Fvictim
-> regex captures "..%2F..%2Fvictim"
-> decodeURIComponent -> "../../victim"
-> join("./data/runs", "../../victim") -> "victim"
-> rmSync("victim", { recursive: true })
Access-Control-Allow-Origin: * plus no auth and an unconditional OPTIONS handler made
this drivable cross-origin: any page a user visited while serve was running could delete
directories on their machine, or start runs that spend provider/judge credits.
The fix
1. Validate IDs at the chokepoint, not per route. CheckpointManager.getRunPath() and
BatchManager.getComparePath() are the funnels every path in those classes flows through, so
one guard in each covers load/exists/create/delete/copyCheckpoint and the routes
that join() onto getRunPath()/getResultsDir() themselves — including the arbitrary
report.json read in GET /api/runs/:runId/report and GET /api/leaderboard. Rejected IDs
throw UnsafeIdError, which the server maps to 400 rather than 500.
The accepted charset is /^[A-Za-z0-9][A-Za-z0-9._-]*$/ with .. rejected outright. Every
generated ID fits it (run-20260101-120000, provider-benchmark-20260101-ab12,
compare-20260101-120000, <compareId>-<provider>).
The leading-alphanumeric requirement matters more than it looks: "." passes a plain
[A-Za-z0-9._-]+ charset and contains no .., but join("./data/runs", ".") resolves to the
base directory itself — rmSync would have taken every run with it. The test caught this.
2. Replace the wildcard CORS with a loopback allowlist. The UI's port is chosen by Next.js
at startup, so any loopback origin is reflected and nothing else is. State-changing methods
from a disallowed origin are refused with 403, so the side effect never happens even if a
client ignores the missing ACAO. Requests with no Origin at all (CLI, curl) are unaffected.
Added Vary: Origin since the response now depends on it.
3. Same class of bug next door: getProviderCode/getProviderPrompts joined
checkpoint.provider into a path. The checkpoint is written before createProvider()
validates the name, so a bogus provider survives on disk and reaches the join. Now resolved
against the existing getAvailableProviders() registry.
Verification
* src/utils/paths.test.ts — 11 traversal vectors rejected (../../secrets, the pre-decode
..%2F..%2F form, .., ., backslash separators, absolute paths), the generated ID formats
accepted, and the joined-path containment property asserted directly rather than via the regex.
* Ran the decoded attack against a real CheckpointManager over a sandbox tree: delete and
load both reject, the victim files survive, data/runs itself survives, and a legitimate
run still round-trips create -> load -> delete.
* Origin allowlist checked against spoofing attempts (localhost.evil.com,
127.0.0.1.evil.com, localhost:3000.evil.com, null) plus the real UI origins.
* bun test green; prettier --check clean on all touched files.
Notes for the reviewer
* checkpoint.ts was already unformatted at HEAD, so I left the rest of that file alone rather
than bury a security fix under a whole-file reformat. My added lines are prettier-clean.
* Unrelated bug noticed while reading BatchManager.delete and deliberately not fixed here:
it rmSyncs the comparison directory and only then calls loadManifest, which reads
manifest.json from inside the directory it just removed. The manifest is therefore always
null and member runs are never actually deleted. Worth its own issue.
________________________________
You can view, comment on, or merge this pull request online at:
#61
Commit Summary
* f9cbe32<f9cbe32> fix(paths): implement safe ID validation to prevent path traversal vulnerabilities
File Changes
(6 files<https://github.com/supermemoryai/memorybench/pull/61/files>)
* M src/orchestrator/batch.ts<https://github.com/supermemoryai/memorybench/pull/61/files#diff-57b85215634b367161ff3b08c31fcd3b176b7ca0061a677c66caf3a37aa3867d> (7)
* M src/orchestrator/checkpoint.ts<https://github.com/supermemoryai/memorybench/pull/61/files#diff-5e26802aae4c76366d50f46fbf834a27283350ab0dc051f9c943e501f88504b9> (6)
* M src/server/index.ts<https://github.com/supermemoryai/memorybench/pull/61/files#diff-e4e7a68d82cdfba4e7dab9a32a8f55f3806ffa0cf33f1b8ab64b42941cf68888> (43)
* M src/server/routes/leaderboard.ts<https://github.com/supermemoryai/memorybench/pull/61/files#diff-7ad4235504535cf83635471a9f4d2df8e9ebd255d8115af7956b72d5962fd823> (16)
* A src/utils/paths.test.ts<https://github.com/supermemoryai/memorybench/pull/61/files#diff-0ba722b19f3c3cf2adff512c22cafa49e157efb4afe514fc38f5ac47fc8ce269> (48)
* A src/utils/paths.ts<https://github.com/supermemoryai/memorybench/pull/61/files#diff-f49f54e461a935d3cde70cb710c9052af5ac65a23e555322c09d8ef8429c9224> (31)
Patch Links:
* https://github.com/supermemoryai/memorybench/pull/61.patch
* https://github.com/supermemoryai/memorybench/pull/61.diff
—
Reply to this email directly, view it on GitHub<#61?email_source=notifications&email_token=ADAAQRL6Z4M64QCW6LZB3DL5JXU5ZA5CNFSNUABEM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UF42DENZTGA2DMMRTGGTHEZLBONXW5KTTOVRHGY3SNFRGKZFFMV3GK3TUVRTG633UMVZF6Y3MNFRWW>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ADAAQRL2OE2DRDGTYNHGRXT5JXU5ZAVCNFSNUABGKJSXA33TNF2G64TZHMYTANJYGM2DKMZXGM5US43TOVSTWNJRGQZTMMZXGA2DTILWAI>.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS<https://github.com/notifications/mobile/ios/ADAAQRI6ZVD2CLIY35WV3Z35JXU5ZA5CNFSNUABEM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UF42DENZTGA2DMMRTGGTHEZLBONXW5KTTOVRHGY3SNFRGKZFFMV3GK3TUVJTG633UMVZF62LPOM> and Android<https://github.com/notifications/mobile/android/ADAAQRLACWQZFN7DWM64ZML5JXU5ZA5CNFSNUABEM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UF42DENZTGA2DMMRTGGTHEZLBONXW5KTTOVRHGY3SNFRGKZFFMV3GK3TUVZTG633UMVZF6YLOMRZG62LE>. Download it today!
You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #60 — arbitrary directory deletion via
DELETE /api/runs/:runId, reachable from any website.The bug
Route patterns like
/api/runs/([^/]+)match beforedecodeURIComponentruns. A URLpathname keeps
%2Fencoded, so the segment matches, and only afterwards becomes a realseparator:
Access-Control-Allow-Origin: *plus no auth and an unconditionalOPTIONShandler madethis drivable cross-origin: any page a user visited while
servewas running could deletedirectories on their machine, or start runs that spend provider/judge credits.
The fix
1. Validate IDs at the chokepoint, not per route.
CheckpointManager.getRunPath()andBatchManager.getComparePath()are the funnels every path in those classes flows through, soone guard in each covers
load/exists/create/delete/copyCheckpointand the routesthat
join()ontogetRunPath()/getResultsDir()themselves — including the arbitraryreport.jsonread inGET /api/runs/:runId/reportandGET /api/leaderboard. Rejected IDsthrow
UnsafeIdError, which the server maps to 400 rather than 500.The accepted charset is
/^[A-Za-z0-9][A-Za-z0-9._-]*$/with..rejected outright. Everygenerated ID fits it (
run-20260101-120000,provider-benchmark-20260101-ab12,compare-20260101-120000,<compareId>-<provider>).The leading-alphanumeric requirement matters more than it looks:
"."passes a plain[A-Za-z0-9._-]+charset and contains no.., butjoin("./data/runs", ".")resolves to thebase directory itself —
rmSyncwould have taken every run with it. The test caught this.2. Replace the wildcard CORS with a loopback allowlist. The UI's port is chosen by Next.js
at startup, so any loopback origin is reflected and nothing else is. State-changing methods
from a disallowed origin are refused with 403, so the side effect never happens even if a
client ignores the missing
ACAO. Requests with noOriginat all (CLI, curl) are unaffected.Added
Vary: Originsince the response now depends on it.3. Same class of bug next door:
getProviderCode/getProviderPromptsjoinedcheckpoint.providerinto a path. The checkpoint is written beforecreateProvider()validates the name, so a bogus provider survives on disk and reaches the join. Now resolved
against the existing
getAvailableProviders()registry.Verification
src/utils/paths.test.ts— 11 traversal vectors rejected (../../secrets, the pre-decode..%2F..%2Fform,..,., backslash separators, absolute paths), the generated ID formatsaccepted, and the joined-path containment property asserted directly rather than via the regex.
CheckpointManagerover a sandbox tree:deleteandloadboth reject, the victim files survive,data/runsitself survives, and a legitimaterun still round-trips create -> load -> delete.
localhost.evil.com,127.0.0.1.evil.com,localhost:3000.evil.com,null) plus the real UI origins.bun testgreen;prettier --checkclean on all touched files.Notes for the reviewer
checkpoint.tswas already unformatted at HEAD, so I left the rest of that file alone ratherthan bury a security fix under a whole-file reformat. My added lines are prettier-clean.
BatchManager.deleteand deliberately not fixed here:it
rmSyncs the comparison directory and only then callsloadManifest, which readsmanifest.jsonfrom inside the directory it just removed. The manifest is therefore alwaysnulland member runs are never actually deleted. Worth its own issue.