fix!: contain store YAML file references to the store file's directory - #737
Conversation
Nested file references in store YAML (model_file, tuple_file, tuple_files and per-test tuple_file) were resolved with path.Join(basePath, value) and no containment check, so a ".." prefix escaped the directory holding the store file and fga store import / fga model test would read it. References are now resolved through os.Root, which rejects traversal, absolute paths and symlinks pointing outside the base directory. Targets must also be regular files: reading a FIFO with no writer blocks forever, and an endless device such as /dev/zero grows the read buffer until the process is OOM-killed. Stat is metadata-only, so both are rejected before any read happens. BREAKING CHANGE: references that resolve outside the store file's directory are rejected by default. Pass --allow-external-files to fga store import or fga model test to opt back in.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR hardens store YAML file handling in fga store import and fga model test by preventing nested file references from escaping the store file’s directory and by refusing to read non-regular files that could hang or OOM the process. It adds an opt-out flag (--allow-external-files) for workflows that intentionally reference files outside the store/test file’s directory.
Changes:
- Resolve
model_file,tuple_file,tuple_files[], and per-testtuple_fileviaos.OpenRoot(...).Stat(...)to block traversal/absolute paths/symlink escapes, with--allow-external-filesto bypass containment. - Introduce
internal/safefileto reject non-regular files before any read occurs, and apply it to the top-level store YAML as well. - Update docs and tests/fixtures to assert the new default behavior and the flag override.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/model-test-cases.yaml | Adjusts integration expectations: relative-path fixtures now fail by default and succeed with --allow-external-files, adds traversal failure case. |
| tests/import-tests-cases.yaml | Updates import integration expectations for the new default containment behavior and the opt-out flag. |
| tests/fixtures/traversal/traversal-store.fga.yaml | Adds a traversal-attempt fixture used by integration tests. |
| README.md | Documents the new --allow-external-files flag for both store import and model test. |
| internal/storetest/storedata.go | Adds resolveFile (os.Root-based containment + regular-file enforcement) and wires it into all nested reference loads. |
| internal/storetest/storedata_test.go | Updates unit tests to pass the new allowExternalFiles parameter to LoadTuples. |
| internal/storetest/security_test.go | Adds focused security tests for containment and non-regular file rejection. |
| internal/storetest/read-from-input.go | Extends ReadFromFile signature to accept allowExternalFiles and checks the top-level YAML is a regular file. |
| internal/safefile/safefile.go | New helper for rejecting non-regular files (FIFO/device/etc.) before reading. |
| cmd/store/import.go | Adds --allow-external-files flag and passes it through to store YAML parsing. |
| cmd/model/test.go | Adds --allow-external-files flag and passes it through when loading each test YAML file. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/storetest/security_test.go`:
- Around line 3-7: Move TestResolveFileRejectsNonRegular and its
syscall.Mkfifo-dependent imports into a separate Unix-only test file using the
appropriate build constraint, while retaining all other tests in
security_test.go and removing any now-unused imports there.
In `@internal/storetest/storedata.go`:
- Around line 113-119: Update the allowExternal path resolution to use ref
directly when filepath.IsAbs(ref) is true, while retaining
filepath.Join(basePath, ref) for relative references; continue validating the
resolved path with safefile.CheckRegular and add coverage for an absolute
external reference when allowExternal is enabled.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3e061714-81dc-4328-af5a-5ad35fbd8d20
📒 Files selected for processing (11)
README.mdcmd/model/test.gocmd/store/import.gointernal/safefile/safefile.gointernal/storetest/read-from-input.gointernal/storetest/security_test.gointernal/storetest/storedata.gointernal/storetest/storedata_test.gotests/fixtures/traversal/traversal-store.fga.yamltests/import-tests-cases.yamltests/model-test-cases.yaml
Siddhant-K-code
left a comment
There was a problem hiding this comment.
Please address the three inline findings before merge:
os.Rootvalidation is discarded before the actual read, allowing a symlink/..resolution mismatch.- Nested files referenced by
.fga.modbypass containment. - The FIFO test does not compile on Windows.
Address review feedback on the containment fix. The check and the read used different path resolution. resolveFile validated a reference with root.Stat but returned filepath.Join, and those disagree across a symlink: with base/link -> sub/dir, os.Root resolves link/../target.json to base/sub/target.json while filepath.Join collapses it to base/target.json. Making that a symlink out of the tree meant the stat passed while the read escaped, so containment could be bypassed. Reads now go through the same os.Root handle that validated them, and the returned descriptor is re-statted rather than the name, so the file that is read is the file that was checked. A modular model referenced from a store file read its fga.mod contents entries separately, outside containment. A literal ".." entry is already rejected by language.TransformModFile, but a plain-named entry whose file is a symlink out of the tree was followed. Those reads are now contained too: LoadModel records the base, which has to travel with the store data because a modular model defers reading its module files until parse time. Naming an fga.mod directly on the command line is unaffected. Move the FIFO test behind //go:build unix. syscall.Mkfifo does not exist on Windows, so a runtime GOOS skip still broke the build there. Also correct a stale comment about /dev/zero hanging rather than exhausting memory, and fix //nolint:lll placement and spacing.
|
All three fixed, thanks. The On FIFO test moved to |
|
@SoulPancake - Lints are failing now, can you please fix that too? |
Unexported methods must follow the exported ones on the type.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (2)
cmd/store/import.go:355
- There is a large run of whitespace before
//nolint:lllon this flag line (looks like it missed gofmt). This makes the file noisy in diffs and doesn’t match typical Go formatting.
importCmd.Flags().Int("max-parallel-requests", tuple.MaxParallelRequests, "Max number of requests to issue to the server in parallel.") //nolint:lll
internal/authorizationmodel/model.go:273
directory/filePathfor modular models are built withpath.Dir/path.Join, butmodFile/containBasecan now contain OS-specific separators (fromfilepath.Joinin store YAML resolution). On Windows, a backslash-separatedmodFilemakespath.Dir(modFile)return ".", breaking module resolution and potentially causing containment to misbehave. Usefilepath.Dir/filepath.Joinand normalizecontentsentries withfilepath.FromSlash.
return err
}
return nil
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (2)
cmd/store/import.go:355
- This line has a large run of spaces before the
//nolint:lllcomment, which suggests gofmt wasn't run (and will likely be reverted by gofmt/linters). Please reformat to the standard gofmt layout.
importCmd.Flags().Int("max-parallel-requests", tuple.MaxParallelRequests, "Max number of requests to issue to the server in parallel.") //nolint:lll
internal/authorizationmodel/model.go:383
- On Windows,
path.Dir/path.Joinonly treat/as a separator. WhenmodFileis an OS-native path (e.g.C:\...\fga.mod),path.Dir(modFile)becomes.and module files will be resolved relative to the process working directory instead of thefga.moddirectory. This regression is more likely now that callers buildmodFilewithfilepath.Join.
Use filepath.Dir and filepath.Join (optionally normalizing contents entries with filepath.FromSlash) so modular models work with Windows-style paths and containment computations (filepath.Rel) stay consistent.
directory := path.Dir(modFile)
for _, fileName := range parsedModFile.Contents.Value {
filePath := path.Join(directory, fileName.Value)
Two regressions from the containment change, neither intended. The top-level store file was rejected unless it was a regular file, which broke reading it from a pipe: fga model test --tests <(...) and fga store import --file <(...) both worked before and started failing with "not a regular file". That check was never needed there — the traversal issue is about the files a store YAML references, not the file the user named on the command line, and --allow-external-files did not help since the check applied in both modes. Nested references are still guarded, so a planted store file cannot point a reference at /dev/zero or a FIFO. Reads also used io.ReadAll, which starts small and repeatedly doubles, allocating roughly twice the file size: 11.0MB for a 5MB file against os.ReadFile's 5.25MB, and 118MB for 50MB. The size from the stat that already ran is now used to pre-size the buffer, growing only if the file turned out larger, which brings allocation back to parity. CheckRegular is now only used within the package, so it is unexported.
|
Quickly evaluating if there is a performance/allocation regression with these |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
cmd/store/import.go:355
- This line has a large run of spaces before the
//nolint:lllcomment; it looks likegofmtwasn't applied and may cause formatting/lint noise. Please reformat to standard spacing.
importCmd.Flags().Int("max-parallel-requests", tuple.MaxParallelRequests, "Max number of requests to issue to the server in parallel.") //nolint:lll
internal/authorizationmodel/model.go:383
- The modular model loader uses
path.Dir/path.JoinonmodFile, which is an OS filesystem path. On Windows, backslashes aren't treated as separators bypath, sodirectory := path.Dir(modFile)can resolve to.and break containment and module resolution. NormalizemodFileto slashes beforepath.Dir, and convert the joined module file path back to an OS path before reading.
directory := path.Dir(modFile)
for _, fileName := range parsedModFile.Contents.Value {
filePath := path.Join(directory, fileName.Value)
internal/storetest/read-from-input.go:35
- The doc comment says referenced files “must be regular files”, and the PR description also claims the same guard applies to the top-level store YAML. However, this function still opens the store file directly via
os.Openwithout a pre-check, so a FIFO store file could still block the process before YAML decoding starts. Consider applying the same regular-file guard to the store file itself to match the documented behavior.
// Files referenced from within the store YAML (model_file, tuple_file,
// tuple_files, and per-test tuple_file) are, by default, contained to the
// directory holding the store file and must be regular files. Set
// allowExternalFiles to true to permit references that resolve outside that
// directory (e.g. via "..") for trusted workflows.
internal/storetest/security_unix_test.go:1
//go:build unixis not a standard GOOS constraint, so this test file will be excluded from normalgo testruns unless a custom-tags unixis provided. Use an actual OS expression (explicit list of supported Unix-like OSes) so the FIFO coverage runs by default wheresyscall.Mkfifoexists.
//go:build unix
…nto fix/store-yaml-path-traversal
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (1)
internal/storetest/storedata.go:285
- The error message here is a bit awkward/redundant ("global tuple %s file") and doesn’t quote the filename, which can be confusing when paths contain spaces. Consider switching to "global tuple file %q" (and reusing the same phrasing for both error returns in this function).
if err != nil {
return fmt.Errorf("failed to process global tuple %s file due to %w", file, err)
}
tuples, err := tuplefile.ParseTuples(resolved, contents)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
internal/safefile/safefile.go:154
checkRegularPathandos.Openhave the same check/open race as the contained path: replacing a checked regular file with a FIFO can make this call block before the descriptor check runs. External-file mode still promises to reject non-regular targets, so use a nonblocking open followed byfile.Statrather than relying on the pathname pre-check.
file, err := os.Open(name)
internal/storetest/storedata.go:147
- For a modular reference containing a symlink followed by
..,ReadContainedcan read a different file than this lexicalfilepath.Joinnames.ReadFromContentsthen discards the bytes forfga.modand stores this returned path, whichreadModelFromModFGAreopens later. For example,link -> sub/dirandmodel_file: link/../model.fga.modreadsbase/sub/model.fga.modhere but later parsesbase/model.fga.mod(or fails if it is absent). Preserve the already-opened modular contents and resolve its entries through the same root-relative lookup rather than returning a cleaned path for reopening; add this modular variant to the symlink/..regression test.
return joined, data, nil
internal/authorizationmodel/model.go:44
- When
containBaseis empty, direct CLI modular models also useReadExternal, which adds the new regular-file restriction to the top-levelfga.modand all its entries. This changes direct--format modularinputs such as FIFOs/process-substitution descriptors, despite the PR's nested-only scope and statement that directly namedfga.modfiles are unaffected. Keep the direct top-level mod-file read on its prior path, and apply safe nested reads only to references originating from a store file (or document and test the broader breaking change).
if containBase == "" {
return safefile.ReadExternal(filePath) //nolint:wrapcheck
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
internal/authorizationmodel/model.go:45
- This also changes directly supplied
fga.modfiles, despite the PR scope stating that only nested references receive the regular-file check and that naming anfga.moddirectly is unaffected.ReadModelFromModFGAnow reaches this branch with an empty base formodel write,model validate,model transform, andstore create, so paths such as pipes/process substitutions that previously usedos.ReadFileare rejected as non-regular. Keep the original top-level read behavior, or explicitly include and document this additional breaking change.
func readModelFile(filePath string, containBase string) ([]byte, error) {
if containBase == "" {
return safefile.ReadExternal(filePath) //nolint:wrapcheck
}
internal/storetest/storedata.go:277
- The security tests exercise
readRefdirectly and the integration escape fixture uses onlymodel_file; they do not verify containment is actually enforced at the three tuple-loading call sites (tuple_file,tuple_files[], and per-testtuple_file). Add traversal/absolute-reference cases for each field, including the opt-in path, so a missed or regressed wiring change cannot leave one of the reported entry points exposed.
resolved, contents, err := readRef(basePath, file, allowExternalFiles)
Importing a store without --store-id parsed the model through CreateStoreWithModel, which read a modular model's module files with no contain base, so an fga.mod contents entry whose file was a symlink pointing outside the store directory was still followed. The update path already passed ModelContainBase. CreateStoreWithModelContained carries the base through the create path; CreateStoreWithModel keeps its signature and stays uncontained for direct store create, where the user names the model file themselves. The regression test drives importStore down the create path with a module file symlinked out of the tree and asserts the import fails before any model is written.
… read The metadata check and the open in ReadContained and ReadExternal are not atomic: a writable target could be swapped from a regular file to a FIFO after the check, and a blocking read-only open would then hang before the descriptor re-check in readOpened could reject it. Opening with O_NONBLOCK closes that window - the open returns immediately regardless of the file type, a no-op for regular files, and the descriptor is still re-checked before its contents are read. The flag is zero on platforms without Unix FIFO open semantics. The test opens a real FIFO through an os.Root handle with the same flags and asserts it is rejected on mode without blocking.
Siddhant-K-code
left a comment
There was a problem hiding this comment.
Overall seems fine now. Do you think, we should add a callout regarding breaking change in CHANGELOG?
|
@Siddhant-K-code |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.
Suppressed comments (4)
internal/storetest/storedata.go:180
ReadFromContentsreplaces the already-read contents withmodelPathfor anfga.mod. In contained mode that path is the lexically cleanedfilepath.Joinresult, not necessarily the path resolved byos.Root; forlink/../model.fga.mod, this validatessub/model.fga.modbut later reopensbase/model.fga.mod, loading a different model and resolving modules from the wrong directory. Preserve the already-read mod contents and root-relative resolution through modular parsing instead of reopening the lexical path.
authorizationmodel.ReadFromContents(
modelPath,
contents,
&inputModel,
&format,
internal/safefile/safefile_unix_test.go:1
- This test is excluded from normal Unix builds because Go does not implicitly define a
unixbuild tag. As a result, CI never runs the test intended to catch a blocking FIFO open—and currently compiles the zero-valued fallback instead. Use the same explicit GOOS constraint as the nonblocking implementation.
//go:build unix
internal/storetest/security_unix_test.go:1
- This FIFO coverage is never compiled by the normal Unix test run because
unixis not an implicit Go build tag. Replace it with the same explicit GOOS constraint used by the nonblocking implementation so the regular-file rejection is actually exercised on supported Unix platforms.
//go:build unix
internal/authorizationmodel/model.go:44
- An empty
containBaseis used for direct command-line modular models as well as uncontained nested models, and this branch now routes both throughReadExternal, which rejects non-regular inputs. Thus directly namedfga.modfiles or module entries backed by a FIFO/pipe no longer work, contrary to the PR's stated scope that directfga.modinput is unaffected. Separate the direct-input read policy from the nested-reference policy so opted-out nested references retain regular-file checks without changing direct input behavior.
if containBase == "" {
return safefile.ReadExternal(filePath) //nolint:wrapcheck
Siddhant-K-code
left a comment
There was a problem hiding this comment.
LGTM.
Just make sure to update changelog regarding breaking change
Closes #736
Problem
The four nested file fields in store YAML — model_file, tuple_file, tuple_files[] and per-test tuple_file — were resolved with path.Join(basePath, value) and no containment check. path.Join runs path.Clean, which collapses .., so path.Join("/tmp/base", "../secret.txt") yields /tmp/secret.txt and fga store import / fga model test would read it. StoreData.Validate() never inspected these fields.
There was also no guard on the type of file a reference pointed at, which is the availability half of the report. A FIFO with no writer blocks os.ReadFile forever, hanging the process. An endless device such as /dev/zero reads a large amount before failing — worth noting the report described this as an unbounded read leading to OOM, but in practice it terminates once YAML parsing hits a control character, so it wastes time rather than exhausting memory. Both are reachable through any of the four fields.
Fix
References are read through an os.Root handle on the base directory instead of being resolved with path.Join, which rejects .. traversal, absolute paths, and symlinks pointing outside the tree.
The read goes through the same handle that validated the reference, rather than validating a reference and then resolving it to a path for someone else to open. That distinction matters: a lexical path and a root-relative lookup do not always agree. Given a symlink base/link -> sub/dir, os.Root resolves link/../target.json to base/sub/target.json while filepath.Join collapses it to base/target.json. Making the latter a symlink out of the tree means a check against the former passes while the read escapes — so validating one path and reading another leaves the containment bypassable. safefile.ReadContained therefore opens and reads through the handle, and re-stats the returned descriptor rather than the name, so the file that is read is the file that was checked. A filepath.Rel check, being purely lexical, would not have covered this at all.
A referenced target must also be a regular file. A metadata-only Stat runs first, since opening a FIFO would block before any check could reject it, and the opened descriptor is re-checked before its contents are read. Between containment and the file-type check, both DoS shapes are refusedrefused for nested references: an in-tree FIFO is rejected on mode, and /dev/zero (or a symlink to it) is rejected as an escape.
The file named on the command line is deliberately not subject to the file-type check. The reported issue is about the files a store YAML references, not the file the user passed, and checking it broke reading a store file from a pipe — fga model test --tests <(...) and fga store import --file <(...) both work on main.
A modular model referenced from a store file reads its fga.mod contents entries at parse time, separately from the store file's own references, so those reads are contained too — LoadModel records the base and it travels with the store data to be enforced there. (A literal .. entry is already rejected by language.TransformModFile; the case this closes is a plain-named entry whose file is a symlink out of the tree.) Naming an fga.mod directly on the command line is unaffected. Because fga.mod contents entries are slash-separated while modFile is an OS path, entries are joined with filepath.Dir/filepath.Join after filepath.FromSlash, so a backslash-separated .fga.mod path on Windows resolves its module files next to the mod file rather than against the working directory.
Errors are joined per-file, so one bad reference reports and the rest still load.
Scope: nested references only
Containment and the regular-file check apply to the four nested fields, not to the store file the user names on the command line. That file is still opened with os.Open and no file-type check, so fga model test --tests /dev/zero or a FIFO passed as --tests is not refused by this PR.
That is deliberate, and it is the reason the file-type check is not applied there: the reported issue is about the files a store YAML references, not the file the user passed, and checking the top-level file broke reading a store file from a pipe — fga model test --tests <(...) and fga store import --file <(...) both work on main and still work here. Anyone who can pass --tests already chooses the process's arguments, so a self-inflicted hang is a different threat model from a store file that reads paths its author never intended.
Breaking change
References that resolve outside the store file's directory are now rejected by default. A store file like this stops working:
tests/fixtures/relative-path/relative-path-store.fga.yaml
name: Relative Path Store
model_file: ../basic-model.fga
tuple_file: ../basic-tuples.json
This pattern is legitimate and in use — sharing one model or tuple file across several store files in sibling directories. Both commands take --allow-external-files to opt back in:
fga store import --file ./store.fga.yaml --allow-external-files
fga model test --tests ./store.fga.yaml --allow-external-files
With the flag set, containment is skipped but the regular-file check still applies.
With the flag set, containment is skipped but the regular-file check still applies. An absolute reference is then used as-is rather than joined onto the store file's directory, so model_file: /shared/models/basic-model.fga resolves to that path.
The commit carries a BREAKING CHANGE: footer, so this releases as a minor bump.
Tests
internal/storetest/security_test.go — containment: reference inside base resolves, .. traversal blocked, absolute path blocked, traversal permitted when external files are allowed, an absolute reference resolved as-is when external files are allowed, and the symlink/.. divergence above cannot escape.
internal/storetest/security_unix_test.go — a real mkfifo FIFO is rejected in both modes. Unix-only build tag, since syscall.Mkfifo does not exist on Windows and a runtime GOOS skip still breaks the build there.
tests/model-test-cases.yaml / tests/import-tests-cases.yaml — the existing relative-path cases now assert failure by default and success with --allow-external-files; a new tests/fixtures/traversal/traversal-store.fga.yaml covers an escape attempt.
go build ./... and go test ./... pass.
Performance
Reads pre-size their buffer from the Stat that already ran, rather than using io.ReadAll, which starts small and repeatedly doubles. Allocation is at parity with os.ReadFile (5.25MB for a 5MB file; io.ReadAll used 11.0MB). The remaining cost is the extra OpenRoot/Stat syscalls, roughly 120µs per referenced file, once per file at load time.
fga tuple write and fga tuple delete read tuple files through the original path and are unchanged.
Notes
Absolute paths were already harmless with path.Join — path.Join("/base", "/etc/passwd") returns /base/etc/passwd, since an absolute second argument is appended rather than honoured. os.Root rejects them explicitly now regardless.
os.Root requires Go 1.24; go.mod is on 1.25.7.
Summary by CodeRabbit
New Features
Added --allow-external-files to store import and model test.
External referenced files remain disabled by default and require explicit opt-in.
Bug Fixes
Blocked path traversal, external references, unsafe symlinks, and non-regular files during file loading.
Blocked path traversal, external references, unsafe symlinks, and non-regular files for files referenced from within a store file.
Added clear errors when referenced files are inaccessible or unsafe.
Documentation
Documented the new option, default behavior, and associated trust warning.