fix(world-local): prevent path traversal via request-supplied IDs#1829
Merged
TooTallNate merged 5 commits intomainfrom Apr 30, 2026
Merged
fix(world-local): prevent path traversal via request-supplied IDs#1829TooTallNate merged 5 commits intomainfrom
TooTallNate merged 5 commits intomainfrom
Conversation
Request-supplied identifiers (runId, eventId, stepId, hookId, correlationId, stream names, and tags) flowed directly into path.join() calls, allowing a client to send values like '../../../package' and cause the backend to read or write files outside the workflow data directory. Add a centralized validator (assertSafeEntityId) that rejects IDs which are empty, start with '.', or contain path separators or NUL bytes. Apply it at each storage-layer entry point that composes IDs into filesystem paths: fs.taggedPath / readJSONWithFallback / paginatedFileSystemQuery, the runs / steps / events / hooks storage methods, and the streamer.
🦋 Changeset detectedLatest commit: 8d3898a The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
Contributor
📊 Benchmark Results
workflow with no steps💻 Local Development
workflow with 1 step💻 Local Development
workflow with 10 sequential steps💻 Local Development
workflow with 25 sequential steps💻 Local Development
workflow with 50 sequential steps💻 Local Development
Promise.all with 10 concurrent steps💻 Local Development
Promise.all with 25 concurrent steps💻 Local Development
Promise.all with 50 concurrent steps💻 Local Development
Promise.race with 10 concurrent steps💻 Local Development
Promise.race with 25 concurrent steps💻 Local Development
Promise.race with 50 concurrent steps💻 Local Development
workflow with 10 sequential data payload steps (10KB)💻 Local Development
workflow with 25 sequential data payload steps (10KB)💻 Local Development
workflow with 50 sequential data payload steps (10KB)💻 Local Development
workflow with 10 concurrent data payload steps (10KB)💻 Local Development
workflow with 25 concurrent data payload steps (10KB)💻 Local Development
workflow with 50 concurrent data payload steps (10KB)💻 Local Development
Stream Benchmarks (includes TTFB metrics)workflow with stream💻 Local Development
stream pipeline with 5 transform steps (1MB)💻 Local Development
10 parallel streams (1MB each)💻 Local Development
fan-out fan-in 10 streams (1MB each)💻 Local Development
SummaryFastest Framework by WorldWinner determined by most benchmark wins
Fastest World by FrameworkWinner determined by most benchmark wins
Column Definitions
Worlds:
|
Contributor
🧪 E2E Test Results✅ All tests passed Summary
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
✅ 📋 Other
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens @workflow/world-local’s filesystem-backed storage against path traversal by validating request-supplied identifiers before they’re used in file paths, and adds regression tests to cover common traversal payloads.
Changes:
- Introduces
assertSafeEntityId(andUnsafeEntityIdError) and applies it across filesystem path composition helpers. - Adds ID validation at storage entry points (runs/steps/events/hooks) and streamer operations that use IDs in filenames.
- Expands unit + integration coverage to ensure traversal payloads are rejected; adds a changeset for the patch release.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/world-local/src/fs.ts | Adds centralized ID validation + applies it in taggedPath, readJSONWithFallback, and paginatedFileSystemQuery. |
| packages/world-local/src/streamer.ts | Validates runId and streamName before using them as filename prefixes / keys. |
| packages/world-local/src/storage/runs-storage.ts | Validates runId before reading run JSON from disk. |
| packages/world-local/src/storage/steps-storage.ts | Validates runId/stepId before reading steps and before listing by run prefix. |
| packages/world-local/src/storage/events-storage.ts | Validates runId/eventId and request correlationId before composing composite keys and paths. |
| packages/world-local/src/storage/hooks-storage.ts | Validates hookId before reading hook JSON from disk. |
| packages/world-local/src/fs.test.ts | Adds focused tests for assertSafeEntityId and for validation in taggedPath/readJSONWithFallback. |
| packages/world-local/src/storage.test.ts | Adds regression tests ensuring traversal payloads are rejected across storage APIs. |
| .changeset/world-local-path-traversal.md | Declares a patch release for the security fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pranaygp
reviewed
Apr 22, 2026
Contributor
There was a problem hiding this comment.
Solid, well-scoped fix. Validation is applied at every entry point I could trace, tests cover the exact payloads from the Latacora report, and all 328 @workflow/world-local tests pass locally. A few non-blocking suggestions inline — mostly around defense-in-depth and error-type consistency. Copilot already flagged the docstring inaccuracies so I won't duplicate those.
One additional note not worth a line comment: legacy.ts's handleLegacyEvent uses runId in path.join directly (lines 47 and 73 — unchanged by this PR). It's safe today because the function is only called from events.create after assertSafeEntityId('runId', runId) has already run, but the function is exported and nothing in its signature documents that invariant. A one-line assertSafeEntityId('runId', runId) at the top of handleLegacyEvent would make the guarantee local to the file.
pranaygp
reviewed
Apr 23, 2026
Contributor
pranaygp
left a comment
pranaygp
left a comment
There was a problem hiding this comment.
Two follow-ups not already covered in prior review rounds.
- UnsafeEntityIdError now extends WorkflowWorldError for consistency with other storage-layer errors and the platform error-to-HTTP mapping. - Add resolveWithinBase(basedir, ...segments) containment helper and apply it at every taggedPath / readJSONWithFallback / .locks path construction site in events-storage and legacy, so a forgotten assertSafeEntityId at a future call site can't silently regress. - Truncate attacker-controlled values in the error message. - Drop unused assertSafeEntityIds helper and the unreachable typeof check under the TS signature. - Fix docstrings on assertSafeEntityId / taggedPath JSDoc example / filePrefix validation comment to match what the code actually does. - handleLegacyEvent now re-asserts runId locally so the invariant is documented at the call site instead of implicitly inherited from events.create.
Member
Author
|
Thanks for the review. Pushed b33b922 addressing every comment:
Tests: 335 passing in |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
packages/world-local/src/streamer.ts:77
listChunkFilesForStream()validatesstreamName, buttagis also embedded into streamer chunk filenames elsewhere viatagSuffix(and is used here to build tagged extensions). Iftagcan ever be influenced outside trusted config, a value containing/,.., or\\could still lead to writes outside the intended chunks directory. Recommend validatingtagwith the same rules (e.g.,assertSafeEntityId('tag', tag)) at streamer creation time or before any chunk path is composed, and/or usingresolveWithinBase()when building chunk file paths for defense in depth.
// Name is used as a filename prefix below; validate it can't escape chunksDir.
assertSafeEntityId('streamName', name);
const listPromises: Promise<string[]>[] = [
listFilesByExtension(chunksDir, '.bin'),
listFilesByExtension(chunksDir, '.json'),
];
if (tag) {
listPromises.push(listFilesByExtension(chunksDir, `.${tag}.bin`));
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ): string { | ||
| const resolvedBase = path.resolve(basedir); | ||
| const joined = path.resolve(basedir, ...segments); | ||
| if (joined !== resolvedBase && !joined.startsWith(resolvedBase + path.sep)) { |
ijjk
approved these changes
Apr 29, 2026
VaguelySerious
approved these changes
Apr 30, 2026
This was referenced Apr 30, 2026
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.
Summary
runId,eventId,stepId,hookId,correlationId, stream names, and tags) flowed straight intopath.join()inside@workflow/world-local, so a client could send a payload like{"runId":"../../../package"}to/.well-known/workflow/v1/flowand read or write files outside the workflow data directory.assertSafeEntityIdhelper inpackages/world-local/src/fs.tsthat rejects IDs which are empty, start with., or contain/,\, or NUL bytes. This is permissive enough to accept every existing valid ID shape (ULIDs, composite keys likewrun_X-step_Y, base64url stream namespaces, tags likevitest-0) while blocking real traversal vectors.fs.taggedPath/readJSONWithFallback/paginatedFileSystemQuery, the runs / steps / events / hooks storage methods, and the streamer.Test plan
pnpm --filter @workflow/world-local test— 266 → 328 passing (62 new unit + integration tests covering the exact payloads from the report:../../../package,../runs/wrun_…, backslash variants, NUL bytes,.locks, etc.)pnpm --filter @workflow/core test— 591 passing