Skip to content

fix!: contain store YAML file references to the store file's directory - #737

Merged
SoulPancake merged 13 commits into
mainfrom
fix/store-yaml-path-traversal
Aug 11, 2026
Merged

fix!: contain store YAML file references to the store file's directory#737
SoulPancake merged 13 commits into
mainfrom
fix/store-yaml-path-traversal

Conversation

@SoulPancake

@SoulPancake SoulPancake commented Aug 5, 2026

Copy link
Copy Markdown
Member

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.

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.
Copilot AI lite review requested due to automatic review settings August 5, 2026 07:15
@SoulPancake
SoulPancake requested a review from a team as a code owner August 5, 2026 07:15
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a06aea86-097b-419e-9493-9e7437b6b9ac

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes address containment, traversal rejection, regular-file validation, security tests, and opt-in external references required by issue #736.
Out of Scope Changes check ✅ Passed The changes remain within scope and support the issue through implementation, CLI options, documentation, and tests.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the primary change: containing store YAML file references within the store file's directory.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/store-yaml-path-traversal

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-test tuple_file via os.OpenRoot(...).Stat(...) to block traversal/absolute paths/symlink escapes, with --allow-external-files to bypass containment.
  • Introduce internal/safefile to 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.

Comment thread cmd/store/import.go
Comment thread internal/storetest/read-from-input.go Outdated
Comment thread cmd/model/test.go Outdated
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Caution

Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted.

Error details
putComment timed out

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 2558b43 and eb311dd.

📒 Files selected for processing (11)
  • README.md
  • cmd/model/test.go
  • cmd/store/import.go
  • internal/safefile/safefile.go
  • internal/storetest/read-from-input.go
  • internal/storetest/security_test.go
  • internal/storetest/storedata.go
  • internal/storetest/storedata_test.go
  • tests/fixtures/traversal/traversal-store.fga.yaml
  • tests/import-tests-cases.yaml
  • tests/model-test-cases.yaml

Comment thread internal/storetest/security_test.go Outdated
Comment thread internal/storetest/storedata.go Outdated
Comment thread internal/storetest/storedata.go Outdated
Comment thread internal/storetest/storedata.go
Comment thread internal/storetest/security_test.go Outdated

@Siddhant-K-code Siddhant-K-code left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address the three inline findings before merge:

  • os.Root validation is discarded before the actual read, allowing a symlink/.. resolution mismatch.
  • Nested files referenced by .fga.mod bypass 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.
Copilot AI review requested due to automatic review settings August 5, 2026 08:06
@SoulPancake

Copy link
Copy Markdown
Member Author

All three fixed, thanks.

The root.Stat / filepath.Join divergence was the real one — I reproduced the full exfiltration with base/link -> sub/dir and base/target.json -> ../outside-secret.json: the stat resolves to base/sub/target.json and passes while the read followed base/target.json out of the tree. resolveFile no longer returns a path; reads go through the same os.Root handle that validated them, re-statting the descriptor rather than the name. Regression test added.

On fga.mod, the .. vector doesn't reproduce language basically language.TransformModFile rejects it with invalid contents item ../outside.fga before any read. What did get through is a plain-named contents entry whose file is a symlink out of the tree, so I've contained those reads too: LoadModel records the base (modular models defer reading module files until parse time) and each entry resolves through os.Root. Naming an fga.mod directly is unchanged.

FIFO test moved to security_unix_test.go behind //go:build unixGOOS=windows go vet failed before, passes now.

@Siddhant-K-code

@Siddhant-K-code

Siddhant-K-code commented Aug 5, 2026

Copy link
Copy Markdown
Member

@SoulPancake - Lints are failing now, can you please fix that too?

Unexported methods must follow the exported ones on the type.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:lll on 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/filePath for modular models are built with path.Dir/path.Join, but modFile/containBase can now contain OS-specific separators (from filepath.Join in store YAML resolution). On Windows, a backslash-separated modFile makes path.Dir(modFile) return ".", breaking module resolution and potentially causing containment to misbehave. Use filepath.Dir/filepath.Join and normalize contents entries with filepath.FromSlash.
			return err
		}

		return nil

Copilot AI review requested due to automatic review settings August 5, 2026 08:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:lll comment, 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.Join only treat / as a separator. When modFile is 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 the fga.mod directory. This regression is more likely now that callers build modFile with filepath.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.
Copilot AI review requested due to automatic review settings August 5, 2026 08:29
@SoulPancake

Copy link
Copy Markdown
Member Author

Quickly evaluating if there is a performance/allocation regression with these

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:lll comment; it looks like gofmt wasn'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.Join on modFile, which is an OS filesystem path. On Windows, backslashes aren't treated as separators by path, so directory := path.Dir(modFile) can resolve to . and break containment and module resolution. Normalize modFile to slashes before path.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.Open without 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 unix is not a standard GOOS constraint, so this test file will be excluded from normal go test runs unless a custom -tags unix is provided. Use an actual OS expression (explicit list of supported Unix-like OSes) so the FIFO coverage runs by default where syscall.Mkfifo exists.
//go:build unix

Comment thread internal/safefile/safefile.go
Comment thread internal/storetest/read-from-input.go Outdated
Copilot AI review requested due to automatic review settings August 6, 2026 15:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  • checkRegularPath and os.Open have 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 by file.Stat rather 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 .., ReadContained can read a different file than this lexical filepath.Join names. ReadFromContents then discards the bytes for fga.mod and stores this returned path, which readModelFromModFGA reopens later. For example, link -> sub/dir and model_file: link/../model.fga.mod reads base/sub/model.fga.mod here but later parses base/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 containBase is empty, direct CLI modular models also use ReadExternal, which adds the new regular-file restriction to the top-level fga.mod and all its entries. This changes direct --format modular inputs such as FIFOs/process-substitution descriptors, despite the PR's nested-only scope and statement that directly named fga.mod files 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

Comment thread internal/safefile/safefile.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.mod files, despite the PR scope stating that only nested references receive the regular-file check and that naming an fga.mod directly is unaffected. ReadModelFromModFGA now reaches this branch with an empty base for model write, model validate, model transform, and store create, so paths such as pipes/process substitutions that previously used os.ReadFile are 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 readRef directly and the integration escape fixture uses only model_file; they do not verify containment is actually enforced at the three tuple-loading call sites (tuple_file, tuple_files[], and per-test tuple_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)

Comment thread internal/storetest/storedata.go
Comment thread cmd/store/import.go
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
Siddhant-K-code requested a balanced review from Copilot August 10, 2026 14:20

@Siddhant-K-code Siddhant-K-code left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall seems fine now. Do you think, we should add a callout regarding breaking change in CHANGELOG?

@SoulPancake

Copy link
Copy Markdown
Member Author

@Siddhant-K-code
Yes, We will do so.
Let me change the PR title.
We will do a minor bump too with this and edit the release-please generated changelog with a proper note about this.

@SoulPancake SoulPancake changed the title fix: contain store YAML file references to the store file's directory fix!: contain store YAML file references to the store file's directory Aug 10, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  • ReadFromContents replaces the already-read contents with modelPath for an fga.mod. In contained mode that path is the lexically cleaned filepath.Join result, not necessarily the path resolved by os.Root; for link/../model.fga.mod, this validates sub/model.fga.mod but later reopens base/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 unix build 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 unix is 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 containBase is used for direct command-line modular models as well as uncontained nested models, and this branch now routes both through ReadExternal, which rejects non-regular inputs. Thus directly named fga.mod files or module entries backed by a FIFO/pipe no longer work, contrary to the PR's stated scope that direct fga.mod input 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

Comment thread internal/safefile/nonblock_unix.go
Comment thread internal/safefile/nonblock_other.go
Comment thread tests/model-test-cases.yaml

@Siddhant-K-code Siddhant-K-code left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Just make sure to update changelog regarding breaking change

@SoulPancake
SoulPancake added this pull request to the merge queue Aug 10, 2026
@SoulPancake
SoulPancake removed this pull request from the merge queue due to a manual request Aug 10, 2026
@SoulPancake
SoulPancake added this pull request to the merge queue Aug 11, 2026
Merged via the queue into main with commit cef729e Aug 11, 2026
27 checks passed
@SoulPancake
SoulPancake deleted the fix/store-yaml-path-traversal branch August 11, 2026 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Store YAML nested file references are not contained to the store file's directory

3 participants