Skip to content

fix: eliminate TOCTOU race in yamlio.load_yaml() - #3909

Open
Quratulain-bilal wants to merge 2 commits into
github:mainfrom
Quratulain-bilal:fix/yamlio-load-toctou
Open

fix: eliminate TOCTOU race in yamlio.load_yaml()#3909
Quratulain-bilal wants to merge 2 commits into
github:mainfrom
Quratulain-bilal:fix/yamlio-load-toctou

Conversation

@Quratulain-bilal

Copy link
Copy Markdown
Contributor

Problem

yamlio.load_yaml() checks exists() then calls read_text(). The file can be deleted between the two calls, causing a raw FileNotFoundError instead of the clear BundlerError.

Fix

Remove the exists() pre-check and catch FileNotFoundError from read_text().

Testing

  • Verified BundlerError is raised when file is missing

Remove exists() pre-check and catch FileNotFoundError from read_text()
to provide a clear BundlerError even under race conditions.

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

Eliminates the TOCTOU race when loading YAML files.

Changes:

  • Converts FileNotFoundError into BundlerError.
  • Removes the vulnerable existence pre-check.
  • Missing a targeted regression test.
Show a summary per file
File Description
src/specify_cli/bundler/lib/yamlio.py Handles deletion during file reads.

Review details

Tip

Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread src/specify_cli/bundler/lib/yamlio.py
Add regression test for the TOCTOU fix in load_yaml(). The mocked
Path is observable as present (exists() returns True) but read_text()
raises FileNotFoundError, proving the exists() removal eliminates
the race window.

Co-authored-by: GitHub Copilot (model: mimo-v2.5-free, supervised)
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.

3 participants