security(context): harden Markdown import against TOCTOU symlink races - #932
Conversation
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
PR Summary by QodoHarden Markdown import reads against TOCTOU symlink races
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1. TOCTOU possible without O_NOFOLLOW
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| try: | ||
| fd = os.open(str(file_path), flags) | ||
| except OSError as exc: |
There was a problem hiding this comment.
1. Toctou possible without o_nofollow 📎 Requirement gap ⛨ Security
On platforms where os.O_NOFOLLOW is unavailable, _read_markdown_file_content() uses a check-then-open sequence (Path.is_symlink() followed by os.open()) that can be bypassed by swapping in a symlink/reparse point after validation, causing the importer to follow the link and read an unintended target file. The later os.fstat() + stat.S_ISREG() only confirms the opened object is a regular file and cannot detect that symlink traversal occurred, silently weakening the intended cross-platform Markdown import security guarantees.
Agent Prompt
## Issue description
`AgentMemory._read_markdown_file_content()` has a TOCTOU symlink/reparse-point substitution risk on platforms without `os.O_NOFOLLOW`: an attacker can replace a validated path with a symlink between the `Path.is_symlink()` check and `os.open()`, causing the importer to follow the link and read an unintended file, while the current `fstat()` + `S_ISREG` check only verifies the opened object is a regular file and does not detect symlink traversal.
## Issue Context
Compliance requirements for #856 require that platforms lacking `O_NOFOLLOW` use a supported mechanism to prevent or reliably detect substitution between validation and open/read, and that behavior not silently weaken across platforms. To close the TOCTOU window without `O_NOFOLLOW`, validate the opened FD against a no-follow stat of the pathname (e.g., compare `fstat(fd)` to `lstat(path)` and reject if the path is a symlink or if the opened file does not match the pathname’s lstat).
## Fix Focus Areas
- semantica/context/agent_memory.py[1911-1941]
- docs/reference/context.md[589-633]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| for file_path in path.iterdir() | ||
| if file_path.is_file() | ||
| and not file_path.is_symlink() | ||
| and file_path.suffix.lower() in self._MARKDOWN_EXTENSIONS |
There was a problem hiding this comment.
3. Symlink entries silently ignored 🐞 Bug ≡ Correctness
When importing a directory, _read_markdown_path() filters out symlink entries (`and not file_path.is_symlink()`), so symlinked Markdown files are skipped instead of being rejected with the same actionable error as single-file imports. This can cause partial imports without any user-visible signal that some Markdown files were refused.
Agent Prompt
### Issue description
Directory imports currently *skip* symlinked Markdown files instead of rejecting them, which can lead to silent partial imports.
### Issue Context
The single-file path is rejected with a clear `ValueError` message. Directory import should behave consistently by detecting symlinked Markdown entries during enumeration and raising a `ValueError` (or, if you truly want to skip, emitting an explicit warning/diagnostic).
### Fix Focus Areas
- semantica/context/agent_memory.py[1950-1969]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
@lakshanmuruganandam can you fix the qodo reviews before we review it? |
Sameer6305
left a comment
Sameer6305
left a comment
There was a problem hiding this comment.
Thanks @lakshanmuruganandam for the security hardening work on this PR. I went through the implementation in detail, including the full Markdown import flow, the affected code paths, platform-specific behavior, and the regression coverage.
Review summary
The core implementation in this PR is strong:
O_NOFOLLOWis used where supported, providing kernel-level protection against symlink substitution duringos.open().fstat()+S_ISREGverifies that the opened descriptor refers to a regular file.- Existing direct-file and directory symlink protections are preserved.
- The file-descriptor ownership/cleanup path was reviewed.
- POSIX behavior and the Windows fallback were specifically examined.
Findings and fixes
During the review, we identified a few gaps that were worth addressing:
-
The Windows fallback does not have
O_NOFOLLOW, leaving a platform-specific TOCTOU limitation. We verified that eliminating this completely would require platform-specific Windows APIs outside the project's current scope. We therefore documented the limitation explicitly rather than implying a stronger guarantee than the implementation provides. -
The original security regression test exercised the private helper and could fail on Windows without the required symlink privilege handling. This was fixed with the appropriate Windows privilege skip.
-
Added public-API coverage to verify that
import_data(..., format="markdown")actually rejects symlink paths. -
Added directory-import coverage to verify that symlinked Markdown entries are not imported.
-
Added coverage for the
fstat()/S_ISREGdefense against non-regular files.
These changes were made in commit 7d2431d.
The targeted Markdown/context test suite was then verified with 46 passed, 4 skipped, 0 failures; the skips are the expected Windows symlink-privilege cases.
I also reviewed the final diff and cleaned up the temporary review/probe artifacts. No unrelated regressions or additional security issues were found.
Final verdict
Approved from my side. ✅
@KaifAhmad1, the PR is ready for merge from my side after your final review.
Documents the (semantica-agi#932, closes semantica-agi#856) fix in the Unreleased/Fixed section.
KaifAhmad1
left a comment
KaifAhmad1
left a comment
There was a problem hiding this comment.
Reviewed the diff directly (not just the bot summaries) and ran the test suite locally against this branch: 46 passed, 4 skipped (skips are the expected Windows symlink-privilege cases).
The core fix is solid:
os.open()withO_NOFOLLOWcloses the TOCTOU window atomically on POSIX (ELOOP → clearValueError).os.fstat()+stat.S_ISREG()rejects non-regular files even after a successful open.- Directory imports now exclude symlinked entries, consistent with the single-file path.
Two non-blocking items, already acknowledged in-thread:
- Windows has no
O_NOFOLLOW, so theis_symlink()pre-check is the only defense there — documented inline rather than overclaiming, matches the scope of #856. - Directory import silently skips symlinked entries rather than raising like the single-file path does (Qodo finding #3) — a UX inconsistency, not a security gap, since the content is never read either way.
Thanks @lakshanmuruganandam for the hardening work, and @Sameer6305 for the thorough follow-up review and test coverage. Approving.
Closes #856
Summary of Changes
AgentMemory._read_markdown_pathfile reading into a dedicated helper_read_markdown_file_content.os.openwithos.O_NOFOLLOW(on platforms supporting it) and verified file descriptor regular file mode withos.fstat(stat.S_ISREG).test_markdown_import_file_open_security_rejects_symlinkintests/context/test_agent_memory_markdown.py.