fix: reject path traversal in definition names#480
Merged
Conversation
Defense in depth: add Zod validation to DefinitionSchema that rejects '..', '/', '\', and null bytes in definition names. While definition names are not currently used to construct file paths (UUIDs are used instead), this closes a validation gap that could become a security issue if naming conventions change in the future. The validation mirrors the existing pattern already present in DataMetadataSchema (src/domain/data/data_metadata.ts). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR correctly adds defense-in-depth validation to reject path traversal characters in definition names, mirroring the existing pattern in DataMetadataSchema.
No blocking issues found.
Code Quality ✅
- TypeScript strict mode compliant - proper Zod
.refine()usage - Named exports used consistently
- No
anytypes - Code style follows CLAUDE.md guidelines
Domain-Driven Design ✅
The validation is correctly placed in the DefinitionSchema which is used by the Definition.create() factory method. This enforces invariants at construction time - the right approach for an Entity. The Definition class properly encapsulates its state with a private constructor and controlled mutation methods.
Test Coverage ✅
Four new test cases comprehensively cover:
- Forward slashes (
/etc/passwd,foo/bar) - Backslashes (
foo\bar) - Path traversal sequences (
../../../etc/passwd,foo..bar) - Null bytes (
foo\0bar)
Tests live next to source file as required and use @std/assert.
Security ✅
This is a proper security fix - the validation prevents path traversal characters that could become problematic if naming conventions change in the future. The implementation mirrors the established pattern in DataMetadataSchema (lines 110-114 in src/domain/data/data_metadata.ts).
Minor Suggestions (non-blocking)
- Consider edge case documentation: The
..check rejects names likefoo..barwhich may be legitimate non-traversal names. This is intentional defense-in-depth and matchesDataMetadataSchema, but a brief comment explaining the intentionally restrictive approach could help future maintainers.
LGTM! Clean implementation that follows established patterns and adds good test coverage.
visnetodev
pushed a commit
to visneto-aitest/swamp
that referenced
this pull request
May 30, 2026
…wamp-club#483) (systeminit#1476) ## Summary Adds `swamp doctor secrets`, a read-only diagnostic that reports model definitions whose `sensitive: true` global arguments hold a **cleartext literal** value instead of a `vault.get(...)` expression, with value-free vault remediation guidance, and a non-zero exit when any are found (CI-gateable). Follow-up to the at-rest write guard (swamp-club#480 / systeminit#1469). That guard only protects **new** writes at the `YamlDefinitionRepository.save()` chokepoint — it leaves two gaps a write-time guard cannot close: 1. **Legacy definitions** authored before the guard still hold the secret in cleartext until re-saved. 2. **Datastore sync / migration** copies definition YAML byte-for-byte, so a cleartext literal authored elsewhere lands on disk without passing through `save()`. This PR is the **detection** half: it scans what is already on disk and points the user at the fix. ## What it does - **Reuses the existing `findLiteralSensitiveGlobalArgs` domain rule** (from systeminit#480), so what the scan surfaces is exactly what a re-save would now refuse — no parallel detection logic. - Scans both the source-of-truth `models/` tree (where datastore-synced definitions land after a pull) **and** `.swamp/auto-definitions`. - **Read-only and value-free:** never calls `save()`, never echoes the secret value in any output mode or the JSON payload. - **Best-effort residual:** definitions whose extension type cannot be resolved are reported as advisory "could not be assessed" warnings, not silently skipped. - Both `log` and `--json` output modes (json shape: `{ overallStatus, scanned, findings, unresolved }`). - **Scoped to model definitions.** Workflow-definition global args and a sync-time guard are intentionally out of scope. ## Files - `src/domain/models/sensitive_field_extractor.ts` — `buildSensitiveArgRemediations` (value-free, structured sibling of the guard's message builder) - `src/libswamp/models/doctor_secrets.ts` — the scan service + `createDoctorSecretsDeps` - `src/presentation/renderers/doctor_secrets.ts` — log + json renderers - `src/cli/commands/doctor_secrets.ts` + `doctor.ts` + `mod.ts` — command wiring/exports - `design/doctor-secrets.md` — design doc ## Test Plan - Unit tests: domain remediation builder (incl. value-free assertions), service (leak / clean / vault-expr / unresolved-type / multi-def), renderer (log + json, pass/fail, value-free), CLI wiring. - Integration test (`integration/doctor_secrets_test.ts`): real `YamlDefinitionRepository` + real registry, planting a cleartext leak on disk in **both** the `models/` and `.swamp/auto-definitions` trees. - End-to-end through the **compiled binary** in a scratch repo: clean scan (exit 0) and a planted-leak scan (exit 1) in both `log` and `--json`, confirming the secret never appears in output. - `deno check`, `deno lint`, `deno fmt`, full `deno run test` (6491 passed) all green. ## Follow-ups - UAT: systeminit/swamp-uat#243 - Docs: swamp-club Lab systeminit#490 (document the command in the manual reference `doctor.md`) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
1 participant
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
Fixes: #477
Defense in depth:
swamp model createcurrently accepts absolute paths and path traversal sequences (e.g.,/etc/passwd,../../../foo) as model names without error. While definition names are not currently used to construct file paths (UUIDs are used instead), this is a validation gap that could become a security issue if naming conventions change in the future.This PR adds Zod validation to
DefinitionSchemathat rejects..,/,\, and null bytes in definition names, mirroring the existing pattern already present inDataMetadataSchema.Changes
src/domain/definitions/definition.ts— Added.refine()validation to thenamefield inDefinitionSchemathat rejects path traversal characterssrc/domain/definitions/definition_test.ts— Added 4 test cases covering forward slashes, backslashes,..path traversal, and null bytesTest plan
deno check— type checking passesdeno lint— linting passesdeno fmt— formatting passesdeno run test src/domain/definitions/definition_test.ts— all 40 tests pass (4 new)deno run test— full test suite passes (2078 tests)🤖 Generated with Claude Code