Harden config file loading: size cap, strict fields, symlink reject#131
Merged
Harden config file loading: size cap, strict fields, symlink reject#131
Conversation
Three gaps in the config loaders: 1. No size cap. A 2 GiB `.broodbox.yaml` in a malicious repo would OOM bbox before it could warn. `os.ReadFile` slurps the whole file unconditionally. 2. No strict-unknown-fields. A typo like `mcp.athz.profile: observe` (should be `authz`) was silently dropped, and the operator fell back to the permissive default. Security-critical typos failed open. 3. Workspace-local `.broodbox.yaml` followed symlinks. A malicious repo could point the workspace config file at any readable path on the host, confusing the trust boundary between workspace and global config. Add a shared `internal/infra/configfile` package with `ReadFile` and `DecodeStrict`. Three loaders use it: - internal/infra/config/loader.go: global Load() — size cap + strict. Symlinks allowed because the operator picks the path. - internal/infra/config/loader.go: LoadFromPath() — size cap + strict + symlink reject + non-regular-file reject. Workspace path is repo-controllable. - internal/infra/mcp/configloader.go: LoadMCPFileConfig — size cap + strict. --mcp-config is operator-supplied, no symlink reject. Size cap is 1 MiB (real configs are single-digit KiB; the limit exists only to bound DoS, not to be restrictive). Strict mode uses yaml.Decoder.KnownFields(true) so unknown keys produce an error naming the offending field and line number. Non-regular files (FIFOs, sockets, device nodes) are rejected before Open, so a malicious or accidental FIFO at `.broodbox.yaml` fails cleanly with "not a regular file" instead of hanging bbox on a blocking open. Document the new file rules in docs/USER_GUIDE.md so users hitting the symlink rejection know to copy the template rather than link it. Co-Authored-By: Claude Opus 4.7 (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
Closes a LOW security finding with three parts. The config loaders (global
~/.config/broodbox/config.yaml, workspace.broodbox.yaml,--mcp-configfile) previously:.broodbox.yamlwould OOM bbox before it could warn.mcp.athz.profile: observe(should beauthz) failed open tofull-access. Security-critical typos fell back to permissive defaults with no error..broodbox.yamlat any readable path on the host.Changes
New
internal/infra/configfilepackage withReadFile(1 MiB cap, optional symlink reject, non-regular-file reject) andDecodeStrict(yaml.Decoder.KnownFields(true)). Three loaders wire it in:Loader.Load(global)LoadFromPath(workspace)LoadMCPFileConfig(--mcp-config)Plus one UX-safety addition flagged by reviews: all three loaders reject non-regular files (FIFOs, sockets, device nodes). Without this, a FIFO planted at
.broodbox.yamlwould causeos.Opento block indefinitely waiting for a writer, hanging bbox with no error.Error-handling stays asymmetric (established in prior PR): global config errors are fatal (user owns the file); workspace errors are warn-and-skip (malicious repo cannot DOS the session).
Docs: extended
docs/USER_GUIDE.mdsection on per-workspace config with the new file rules so users hitting the symlink rejection know to copy templates instead of linking.Test plan
task fmt && task lint && task test— all greenmcp.athz.profile), oversized file (2 MiB), workspace symlink, FIFO at workspace path, MCP config typo/oversized.broodbox.yamltypo →Warning: failed to load local config ...: parsing config file ...: yaml: unmarshal errors: line N: field athz not found(bbox continues with global).broodbox.yaml→Warning: ...: config file ... is too large: 2097152 bytes (limit 1048576)(bbox continues).broodbox.yamlsymlink →Warning: ...: symlinks are not allowed for workspace-local config (points to "target.yaml")(bbox continues).broodbox.yaml→Warning: ...: not a regular file (mode=prw-r--r--)(bbox continues; does not hang)🤖 Generated with Claude Code