Uh oh!
There was an error while loading. Please reload this page.
Forward-port #1920: tighten world-local ID validation - #2097
Merged
Conversation
Forward-port of code review fixes from #1920 (backport of #1829): - Reject dots inside entity IDs in `assertSafeEntityId`. Internal IDs (ULIDs, step_N, etc.) never contain dots, but `stripTag()` / `getObjectCreatedAt()` strip a trailing `.[tag]` suffix from filenames, so a request-supplied runId like `wrun_123.foo` would be silently mangled during listing/pagination. - Reject empty `correlationId` on events that include one. The event schemas only require `z.string()`, so without this check a step_created / hook_created / wait_created request with `correlationId: ''` would silently be written under a malformed composite key like `${runId}-`. The streamer regression tests from #1920 are not forward-ported because main's streamer surface differs (renamed methods, separate test coverage) from stable's v4 shape.
Contributor
🦋 Changeset detectedLatest commit: 20257be The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 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
📊 Benchmark Results
workflow with no steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) workflow with 1 step💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) workflow with 10 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) workflow with 25 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) workflow with 50 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Express Promise.all with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) Promise.all with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) Promise.all with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) Promise.race with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) Promise.race with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) Promise.race with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Express workflow with 10 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Express workflow with 25 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Express workflow with 50 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) workflow with 10 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Express workflow with 25 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) workflow with 50 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) Stream Benchmarks(includes TTFB metrics)workflow with stream💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) stream pipeline with 5 transform steps (1MB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) 10 parallel streams (1MB each)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Express fan-out fan-in 10 streams (1MB each)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) SummaryFastest Framework by WorldWinner determined by most benchmark wins
Fastest World by FrameworkWinner determined by most benchmark wins
Column Definitions
Worlds:
❌ Some benchmark jobs failed:
Check the workflow run for details.
Check the workflow run for details. |
Contributor
🧪 E2E Test Results❌ Some tests failed Summary
❌ Failed Tests▲ Vercel Production (1 failed)nitro (1 failed):
📋 Other (1 failed)e2e-vercel-prod-tanstack-start (1 failed):
Details by Category❌ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
❌ 📋 Other
❌ Some E2E test jobs failed:
Check the workflow run for details. |
Contributor
There was a problem hiding this comment.
Pull request overview
Forward-ports the follow-up hardening from #1920 onto main for @workflow/world-local ID validation, closing edge cases around dotted IDs (tag-stripping ambiguity) and empty correlationId values (malformed composite keys).
Changes:
- Disallow
.inassertSafeEntityId(in addition to existing traversal-related restrictions). - Validate
correlationIdeven when it is an empty string so it fails fast instead of writing under${runId}-. - Add/adjust tests covering empty
correlationIdfor key event types and dotted-ID rejection, plus a changeset for the patch release.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/world-local/src/storage/events-storage.ts | Tightens correlationId validation to reject empty string inputs before filesystem key composition. |
| packages/world-local/src/storage.test.ts | Adds regression tests ensuring empty correlationId is rejected for step/hook/wait creation events. |
| packages/world-local/src/fs.ts | Updates assertSafeEntityId to reject dots and refreshes the error message/docs accordingly. |
| packages/world-local/src/fs.test.ts | Moves dotted IDs into the unsafe set and documents why dots are rejected. |
| .changeset/world-local-tighten-id-validation.md | Records the patch-level behavioral change for consumers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
karthikscale3
approved these changes
May 22, 2026
Uh oh!
There was an error while loading. Please reload this page.
Contributor
No backport to This commit is itself a forward-port from To override, re-run the Backport to stable workflow manually via |
This was referenced May 22, 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 freeto 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
Forward-port of the code review fixes applied to #1920 (the
stablebackport of #1829), somainandstablestay in sync on this hardening.The original path-traversal fix (#1829) is already on
main. This PR forward-ports the two follow-up fixes from #1920 that apply equally tomain:Reject
.inside entity IDs inassertSafeEntityId. Internal IDs (ULIDs,step_N, etc.) never contain dots, butstripTag()/getObjectCreatedAt()strip a trailing.[tag]suffix from filenames. A request-suppliedrunIdlikewrun_123.foowould be silently mangled towrun_123during listing/pagination, breaking lookups for tagged file handling. The previously-"safe"wrun_ABC.vitest-0example infs.test.tsmoved to the unsafe set, since that shape is only ever constructed internally bytaggedPath— never passed in as a request ID.Reject empty
correlationIdon events that include one. The event schemas only requirez.string(), so without this check astep_created/hook_created/wait_createdrequest withcorrelationId: ''would silently be written under a malformed${runId}-composite key. Dropped thedata.correlationId.length > 0guard so empty values now throwUnsafeEntityIdErrorup-front.The third fix from #1920 — streamer regression tests for
writeToStream/closeStream/listStreamsByRunId/getStreamChunks— is not forward-ported becausemain's streamer surface differs fromstable's v4 shape (renamed methods, separate test coverage).Test plan
All 351 tests pass, including the new regression tests for empty
correlationId(step_created,hook_created,wait_created) and the additional dotted-ID cases infs.test.ts.