[backport] fix(world-local): prevent path traversal via request-supplied IDs (#1829)#1920
Open
TooTallNate wants to merge 1 commit intostablefrom
Open
[backport] fix(world-local): prevent path traversal via request-supplied IDs (#1829)#1920TooTallNate wants to merge 1 commit intostablefrom
TooTallNate wants to merge 1 commit intostablefrom
Conversation
) * fix(world-local): prevent path traversal via request-supplied IDs 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. * address review feedback - 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. --------- Co-authored-by: JJ Kasper <jj@jjsweb.site>
🦋 Changeset detectedLatest commit: 02dd50a The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 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
🧪 E2E Test Results❌ Some tests failed Summary
❌ Failed Tests💻 Local Development (328 failed)express-stable (82 failed):
fastify-stable (82 failed):
hono-stable (82 failed):
nitro-stable (82 failed):
🌍 Community Worlds (83 failed)mongodb (11 failed):
redis (7 failed):
turso (65 failed):
Details by Category✅ ▲ Vercel Production
❌ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
❌ 🌍 Community Worlds
✅ 📋 Other
❌ Some E2E test jobs failed:
Check the workflow run for details. |
Contributor
There was a problem hiding this comment.
Pull request overview
Backport of the world-local path-traversal fix from main to stable, adding centralized ID validation and applying it across filesystem-backed storage paths so request-supplied identifiers cannot escape the local workflow data directory.
Changes:
- Added
assertSafeEntityId,UnsafeEntityIdError, andresolveWithinBaseinfs.tsto validate IDs and enforce path containment. - Threaded ID validation through runs, steps, events, hooks, legacy handling, and streamer storage paths.
- Added regression tests for filesystem helpers and storage-layer traversal cases, plus a changeset for the patch release.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
packages/world-local/src/streamer.ts |
Validates stream names/run IDs before building stream metadata and chunk paths. |
packages/world-local/src/storage/steps-storage.ts |
Validates stepId and optional caller-supplied runId in step lookups/listing. |
packages/world-local/src/storage/runs-storage.ts |
Validates runId before reading run JSON. |
packages/world-local/src/storage/legacy.ts |
Adds local runId validation and switches legacy writes to contained path resolution. |
packages/world-local/src/storage/hooks-storage.ts |
Validates hookId before reading hook JSON. |
packages/world-local/src/storage/events-storage.ts |
Adds request-ID validation and containment checks for event/lock-path handling. |
packages/world-local/src/storage.test.ts |
Adds storage-level regression tests for traversal attempts. |
packages/world-local/src/fs.ts |
Introduces centralized ID/path safety helpers and applies them in shared FS utilities. |
packages/world-local/src/fs.test.ts |
Adds unit tests for ID validation, error behavior, and base-dir containment. |
.changeset/world-local-path-traversal.md |
Declares the patch release note for @workflow/world-local. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+59
to
+65
| export function assertSafeEntityId(kind: string, value: string): void { | ||
| if ( | ||
| value.length === 0 || | ||
| value.startsWith('.') || | ||
| value.includes('/') || | ||
| value.includes('\\') || | ||
| value.includes('\0') |
Comment on lines
+91
to
+95
| if ( | ||
| 'correlationId' in data && | ||
| typeof data.correlationId === 'string' && | ||
| data.correlationId.length > 0 | ||
| ) { |
Comment on lines
+123
to
+124
| assertSafeEntityId('runId', runId); | ||
| assertSafeEntityId('streamName', streamName); |
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
Backport of #1829 (security fix) to the
stablebranch.Request-supplied identifiers (runId, eventId, stepId, hookId, correlationId, stream names, and tags) flowed directly into
path.join()calls in@workflow/world-local, allowing a client to send values like../../../packageand cause the backend to read or write files outside the workflow data directory.This adds a centralized validator (
assertSafeEntityId) that rejects IDs which are empty, start with., or contain path separators or NUL bytes, and applies it at every storage-layer entry point that composes IDs into filesystem paths.Backport details
The cherry-pick applied cleanly except for two files where stable's pre-v5 shape diverged from main:
packages/world-local/src/storage/steps-storage.ts— Stable'ssteps.get()accepts an optionalrunIdand looks it up vialistJSONFileswhen missing; main's signature requiresrunId. Resolution: assertstepIdalways, and only assertrunIdwhen it was supplied (the disk lookup only ever produces internal-derived runIds, so it can't be attacker-controlled).packages/world-local/src/streamer.ts— Stable still has the v4 streamer interface (separatecloseStream/listStreamsByRunId/getStreamChunksmethods rather than main's renamed equivalents). The auto-merge produced a stray duplicate block that conflicted with stable's existinggetStreamChunkschunk-walking loop. Resolution: kept stable's existing loop; threaded theassertSafeEntityId('runId', runId)call into stable'slistStreamsByRunId. The other three call sites (listChunkFilesForStream,registerStreamForRun, plus the import) auto-merged correctly.All 339 tests in
packages/world-localpass, including the new path-traversal test cases (60 infs.test.tscoveringassertSafeEntityId,UnsafeEntityIdError, error truncation, etc.). Workspacepnpm typecheckis clean.Test plan