Skip to content

fix: eliminate TOCTOU races in catalog_fetch() - #3910

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

fix: eliminate TOCTOU races in catalog_fetch()#3910
Quratulain-bilal wants to merge 2 commits into
github:mainfrom
Quratulain-bilal:fix/catalog-fetch-toctou

Conversation

@Quratulain-bilal

Copy link
Copy Markdown
Contributor

Problem

catalog_fetch() checks exists() then calls read_text() for both file:// and bare path URLs. The file can be deleted between the two calls.

Fix

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

Testing

  • Verified BundlerError is raised when catalog file is missing

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 TOCTOU races when reading local catalogs.

Changes:

  • Removes exists() pre-checks.
  • Converts FileNotFoundError into BundlerError.
Show a summary per file
File Description
src/specify_cli/bundler/services/adapters.py Safely handles disappearing local catalog files.

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/services/adapters.py

@mnriem mnriem left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please address Copilot feedback

…ath URLs

Remove exists() pre-checks and catch FileNotFoundError from read_text()
to provide clear BundlerError messages even under race conditions.
…ath URLs

Remove exists() pre-checks and catch FileNotFoundError from read_text()
for both file:// and bare path catalog sources. Also catches
OSError/UnicodeError to preserve the decode-error wrapping contract.

Add regression test for the TOCTOU fix covering both file:// URLs and
bare paths: 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)
@Quratulain-bilal
Quratulain-bilal force-pushed the fix/catalog-fetch-toctou branch from 9c1819d to 09a8ed5 Compare August 10, 2026 21:24
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