Skip to content

fix: cache.save_cached race + _clone_repo argument-injection hardening#589

Closed
Theprofitplatform wants to merge 2 commits into
Graphify-Labs:v5from
Theprofitplatform:fix/cache-race-and-clone-hardening
Closed

fix: cache.save_cached race + _clone_repo argument-injection hardening#589
Theprofitplatform wants to merge 2 commits into
Graphify-Labs:v5from
Theprofitplatform:fix/cache-race-and-clone-hardening

Conversation

@Theprofitplatform

Copy link
Copy Markdown

Two small, independent hardening fixes — each in its own commit with a regression test.

1. cache.save_cached: per-writer unique tempfile

save_cached used entry.with_suffix(\".tmp\") for the staging path, which is a single shared filename per hash. Concurrent writers for the same hash (parallel extractors, retries) raced on that file — one writer's partial bytes could land in another's atomic `os.replace`, leaving a truncated/mixed cache entry on disk.

Switched to `tempfile.mkstemp(dir=target_dir, prefix=f"{h}.", suffix=".tmp")` so every writer gets its own staging file. The final `os.replace` is still atomic; only the final rename contends, and last-writer-wins is fine because two writers with the same hash are writing the same content.

Test: `test_save_cached_concurrent_writers_no_collision` fans out 8 threads writing the same hash, asserts the final entry is parseable, no orphan `.tmp` files remain, and no writer raised.

2. _clone_repo: argument-injection hardening

Two changes to the git invocation:

  • Reject `branch` names starting with `-`. git can treat refspecs starting with `-` as options in some positions, so a value like `--upload-pack=evilcmd` passed via `--branch` could be reinterpreted by git for the next argument. Now fails fast with a clear error.
  • Add a `--` separator before positional URL/dest arguments. The `git_url` is currently regex-locked to github.com URLs so immediate exposure is small, but the separator makes it impossible for future changes to the URL-building code to leak an option through this surface. Same guard added to the `git pull origin ` path.

Test: `tests/test_clone.py`

  • `test_clone_rejects_branch_starting_with_dash` — `SystemExit` raised before any `subprocess.run`
  • `test_clone_argv_uses_double_dash_separator` — constructed argv contains a literal `--` before positional URL/dest

Out of scope (filing separately)

These came up in the same review pass but need design discussion, not a 5-fix PR:

  • DNS rebinding TOCTOU in `security.validate_url` / `safe_fetch` (resolves twice, attacker DNS can return public IP first and metadata IP second).
  • `yt-dlp` SSRF bypass in `transcribe.download_audio` — the URL never goes through `validate_url`.

I'll open these as separate issues so they can be discussed independently.

Test run

```
$ pytest tests/test_cache.py tests/test_clone.py
12 passed (1 pre-existing failure in test_clear_cache that exists on v5 HEAD without my changes, due to cache_dir now returning .../cache/{kind}/)
```

Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com

Abhishek Maharjan added 2 commits April 28, 2026 21:31
Concurrent save_cached calls for the same hash used to share a single
`entry.with_suffix(".tmp")` path. Two writers would race on the same temp
file — one's bytes could land in the other's atomic os.replace, leaving a
truncated or mixed-write cache entry. tempfile.mkstemp gives each writer
its own filename in the same directory, so os.replace remains atomic and
the only contention is the final rename (which is benign — last writer
wins, same content given the same hash).

Adds a regression test that fans out 8 threads writing the same hash and
asserts the final entry parses, no orphan .tmp files remain, and no
writer raised.
Two changes to the git invocation in _clone_repo:

1. Reject branch names starting with "-". git treats refspecs starting
   with `-` as options in some positions, so a value like
   `--upload-pack=evilcmd` passed as `branch` could be reinterpreted by
   git as an option for the next argument. Fail fast with a clear error.

2. Add a `--` separator before positional URL/dest arguments. Today the
   git_url is regex-locked to github.com URLs so the immediate exposure
   is small, but the `--` makes it impossible for any future change to
   the URL-building code to leak an option through this surface. Same
   guard added to the `git pull origin <branch>` path.

Adds tests/test_clone.py covering:
  * branch starting with "-" causes SystemExit before any subprocess.run
  * the constructed argv contains a literal "--" separator before
    positional URL/dest pairs
@Qodo-Free-For-OSS

Copy link
Copy Markdown

Hi, save_cached() creates a raw file descriptor via tempfile.mkstemp() and then wraps it with os.fdopen(), but if os.fdopen() raises before owning the fd, the descriptor is leaked and the temp file may become undeletable (notably on Windows). This is a production-path resource leak that can accumulate in long-running runs or leave orphan .tmp files.

Severity: action required | Category: reliability

How to fix: Ensure fd closed on failure

Agent prompt to fix - you can give this to your LLM of choice:

Issue description

graphify/cache.py::save_cached uses tempfile.mkstemp() + os.fdopen(). If os.fdopen() raises before taking ownership of the fd, the fd leaks and the temp file may be left behind / undeletable.

Issue Context

mkstemp() returns an already-open OS fd. It must be closed even when fdopen() fails.

Fix Focus Areas

  • graphify/cache.py[102-123]

Suggested fix

Refactor to a pattern that guarantees closure:

  • Option A (preferred): use tempfile.NamedTemporaryFile(mode="w", encoding="utf-8", dir=..., prefix=..., suffix=..., delete=False) and write via the file object; capture tmp = Path(f.name); close occurs via the context manager.
  • Option B: wrap os.fdopen(fd, ...) in an inner try/except and os.close(fd) if fdopen fails, then re-raise.

Keep the existing cleanup logic (tmp.unlink(missing_ok=True) on exceptions) and the atomic os.replace behavior.


Found by Qodo. Free code review for open-source maintainers.

safishamsi added a commit that referenced this pull request May 1, 2026
…ne comments, query boost, cache race, markdownify, content hash

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@safishamsi

Copy link
Copy Markdown
Collaborator

The requested changes have been implemented in 3fdae8f. Cache writer now uses tempfile.mkstemp for a unique tmp file per writer eliminating the race condition. Branch names starting with - are rejected and -- separator added before positional git args. Shipped in v0.6.2.

@safishamsi safishamsi closed this May 1, 2026
matzls pushed a commit to matzls/graphify that referenced this pull request May 10, 2026
…bs#638 Graphify-Labs#589 Graphify-Labs#586 Graphify-Labs#593: kimi thinking, manifest, inline comments, query boost, cache race, markdownify, content hash

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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