fix: allow scoped @collective/name in workflow and definition names#888
fix: allow scoped @collective/name in workflow and definition names#888
Conversation
The path traversal validation in WorkflowSchema and DefinitionSchema rejected any name containing '/', which blocked extension workflows using scoped names like @john/pod-inventory. The '/' in scoped @collective/name patterns is a namespace separator, not a path traversal vector. Updated the inline Zod refinement in both schemas to allow '/' only when the name matches the established SCOPED_NAME_PATTERN (^@[a-z0-9_-]+\/[a-z0-9_-]+(\/[a-z0-9_-]+)*$). Unscoped names with '/' are still rejected. The data_metadata.ts validation is intentionally unchanged — data artifact names are internal and should not use scoped patterns. Fixes #887
8bc16bf to
480e265
Compare
There was a problem hiding this comment.
Code Review
Blocking Issues
None.
Suggestions
- Extract shared regex constant: The scoped name regex
^@[a-z0-9_-]+\/[a-z0-9_-]+(\/[a-z0-9_-]+)*$is now duplicated in 5 places (extension_manifest.ts,pull.ts,yank.ts,workflow.ts,definition.ts). Consider extracting it to a shared domain value object or constant (e.g., insrc/domain/extensions/scoped_name.ts) to keep the pattern in one canonical location. Not blocking since the regex is correct and consistent across all sites.
Notes
- Regex matches the existing
SCOPED_NAME_PATTERNfromextension_manifest.tsexactly — good consistency. - Validation logic is sound:
..,\, and\0are rejected first, then/is only allowed when the full name matches the scoped pattern. Path traversal vectors like@scope/../etcare caught by the..check before reaching the/branch. - Tests cover acceptance (
@john/pod-inventory,@swamp/aws/ec2) and rejection (@/,@scope/,@UPPER/case) — good coverage of edge cases. data_metadata.tsintentionally unchanged — correct bounded context boundary.- DDD: Changes are appropriately scoped to aggregate root schema validation in the domain layer.
- No libswamp import boundary violations.
- No security concerns — the change strictly tightens the validation by only allowing
/in a well-defined scoped pattern.
LGTM ✓
There was a problem hiding this comment.
Adversarial Review
Critical / High
None.
Medium
- Inline regex duplication —
workflow.ts:48anddefinition.ts:158both inline the regex/^@[a-z0-9_-]+\/[a-z0-9_-]+(\/[a-z0-9_-]+)*$/rather than importingSCOPED_NAME_PATTERNfromextension_manifest.ts(where it's already defined at line 26, and also duplicated inpull.ts:37andyank.ts:27). If the pattern evolves (e.g., allowing.in segments), these two new sites could drift. Not a bug today, but a maintenance risk. Consider extracting to a shared constant.
Low
-
Missing test for
..+/combo — The existing../../../tmp/eviltest covers..in unscoped names, but there's no test for the combined vector@foo/../bar(scoped-looking name with path traversal). The code correctly rejects it becauseincludes("..")fires before the regex check, but an explicit test would document this defense. Purely a test coverage observation. -
Unscoped
@names pass validation — A name like@single(no slash) passes the validation because it contains no/,..,\, or\0. This isn't a security issue (no path traversal), but it means the name field accepts partial scoped patterns. This is likely fine since format enforcement belongs elsewhere.
Verdict
PASS — The fix is correct and well-scoped. The .. check fires before the / check, blocking all path traversal vectors through scoped names. The regex exactly matches the canonical SCOPED_NAME_PATTERN. Edge cases (empty segments, uppercase, dots, null bytes in scoped names) are all properly rejected. Tests cover the key acceptance and rejection cases.
Summary
Extension workflows using scoped names like
@john/pod-inventorywere rejectedat load time with a path traversal error because the
WorkflowSchemaandDefinitionSchemaname validation unconditionally rejected any name containing/. The/in scoped@collective/namepatterns is a namespace separator, nota path traversal vector.
Root cause: The Zod refinement on the
namefield in bothWorkflowSchema(
src/domain/workflows/workflow.ts:41-44) andDefinitionSchema(
src/domain/definitions/definition.ts:151-154) used a flat blocklist:This blocked all
/characters, including legitimate ones in scoped extensionnames like
@john/pod-inventoryor@swamp/aws/ec2.Fix: Updated the inline validation in both schemas to allow
/only whenthe full name matches the established
SCOPED_NAME_PATTERN(
^@[a-z0-9_-]+\/[a-z0-9_-]+(\/[a-z0-9_-]+)*$), which is the same regex usedin
extension_manifest.ts. Unscoped names containing/(e.g.a/b,/etc/passwd) are still rejected. Thedata_metadata.tsvalidation isintentionally unchanged — data artifact names are internal and should not use
scoped patterns.
Changes:
src/domain/workflows/workflow.ts— Updated name validation refinementsrc/domain/definitions/definition.ts— Updated name validation refinementsrc/domain/workflows/workflow_test.ts— Added tests for scoped nameacceptance and malformed scoped name rejection
src/domain/definitions/definition_test.ts— Same test additionsVerified end-to-end: compiled binary,
swamp repo init,swamp extension pull @john/k8s,swamp workflow search pod— all 13 workflows load withoutwarnings.
Test Plan
..', unscoped/,\, null bytes)@collective/nameaccepted (@john/pod-inventory,@swamp/aws/ec2)@/,@scope/,@UPPER/case)deno check,deno lint,deno fmtall clean@john/k8s, all 13 workflows load correctlyFixes #887
🤖 Generated with Claude Code