fix(mempalace): validate Kind/Tenant/Key against path traversal#5
Merged
Conversation
Write/Read/List all join caller-supplied strings into the on-disk
path under WingDir(tenant)/kind/<key>.json. The pre-change "safe
key check" comment in Read was aspirational — there was no actual
check.
In practice Write is only called from `advise_cmd` with hardcoded
kind="decision" or "bom" and an already-validated tenant, so this
gate is preventative. But the Bridge is a public package-level API
and the test surface proved an attacker who could call it directly
(or a future caller that forgets to pre-validate) could escape the
wing trivially:
Drawer{Kind: "..", Tenant: "acme", Body: ...}
→ wrote to tenants/acme/mempalace-wing/../<sha>.json
Drawer{Kind: "decision", Tenant: "acme", Key: "../escape", ...}
→ wrote to tenants/acme/mempalace-wing/decision/../escape.json
Now:
- Kind must match `^[a-z][a-z0-9-]{0,31}$`
- Tenant must match the same DNS-label grammar tenant.New() enforces
- Key (when supplied explicitly) must be 64 lowercase hex chars —
the content-addressed shape Write computes internally
All three components are checked on every public entry point
(Write, Read, List) and the violation returns wrapped ErrInvalidInput
so callers can distinguish input rejection from FS-side failure.
The List check also lets us drop the slightly suspicious legacy
`.json`-suffix-strip logic from "this might be safe" to "we know
the key was already in our grammar".
Coverage: mempalace 90.5% (target 87), global PASS.
tzone85
added a commit
that referenced
this pull request
Jun 11, 2026
Adds an Unreleased section to CHANGELOG.md covering the five sequential PRs that landed today: - #2 chore(security): bump x/sys for GO-2026-5024 - #3 fix(api): validate tenant id at route boundary - #4 fix(ingest): reject option-shaped --base-ref - #5 fix(mempalace): validate Kind/Tenant/Key against path traversal - #6 chore(supply-chain): pin govulncheck + base images by digest Updates README Status to mention the hardening pass and replaces a stale `§9` design-spec cross-reference with the section's actual name (per the project's doc-style rule against the § symbol). No code changes, no behaviour changes — pure documentation refresh for the audit story.
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
Write,Read,Listnow validate every caller-supplied path component (Kind, Tenant, Key) against a strict regex before touching the filesystem. Violations return wrappedErrInvalidInput.// safe key checkcomment inReadwas aspirational — there was no actual check. Test proved an attacker who reached the package-level API (or a future caller that forgot to pre-validate) could escape the wing trivially withKind: ".."or an explicitKey: "../escape".In practice the only current caller is
advise_cmdwith hardcodedkind="decision"/"bom"and an already-validated tenant, so this gate is preventative rather than reactive. But mempalace is a public package-level API — the new caller in 6 months won't know about the implicit pre-validation contract.Grammar:
^[a-z][a-z0-9-]{0,31}$tenant.New()Writecomputes internally)TDD
RED:
TestWrite_RejectsTraversalShapedInputs+TestRead_RejectsTraversalShapedInputs— 15 sub-tests across.., slash, uppercase, non-hex. All 15 failed before the fix — many actually wrote files outside the wing dir.GREEN: added regex validators + checks at every public entry point. All 15 sub-tests pass.
Test plan
go test -race ./...— all packages greenbash scripts/cover_check.sh— PASS, mempalace 90.5% (target 87)golangci-lint run— 0 issues