Repository navigation
Conversation
The object is opened through the handle-relative lookup with delete access and read-only sharing, so no writer can change the bytes between the authentication and the delete. Only then the POSIX delete disposition is set on that same handle. There is no FILE_FLAG_DELETE_ON_CLOSE and no fallback to the classic disposition; a read-only file is refused. The disposition is the only new unsafe call, with one fixed structure whose layout is pinned by a test. A counter-proof shows what write sharing would have let through. The cases that every platform shares now run on Windows too.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Reviewer's GuideAdds test-support-only Windows removal that authenticates through a handle-relative, read-share-only handle and deletes via POSIX disposition on that same handle, with comprehensive Windows proofs, neutral test coverage, explicit durability limitations, and documented FFI safety. Sequence diagram for authenticated Windows removalsequenceDiagram
participant Caller
participant PinnedDir
participant Windows
participant FileHandle
Caller->>PinnedDir: remove_file(name, authenticate, observer)
PinnedDir->>Windows: open_relative(name, FILE_GENERIC_READ | DELETE, FILE_SHARE_READ)
Windows-->>PinnedDir: FileHandle or Busy
PinnedDir->>FileHandle: authenticate(PinnedFile)
alt authentication refused
FileHandle-->>PinnedDir: false
PinnedDir-->>Caller: Refused
else authentication succeeds
PinnedDir->>Caller: observer(RemovalStage::Checked)
PinnedDir->>Windows: SetFileInformationByHandle(FileDispositionInfoEx)
Windows-->>PinnedDir: POSIX delete result
PinnedDir-->>Caller: Removed(NotConfirmed) or error
end
Flow diagram for Windows handle-relative removal guaranteesflowchart LR
A[Open child relative to pinned directory] --> B[Hold delete handle with read-only sharing]
B --> C[Authenticate bytes on same handle]
C -->|refused or error| D[No deletion]
C -->|accepted| E[Set POSIX delete disposition]
E --> F[Removed with NotConfirmed durability]
B -->|existing or later writer| G[Open fails Busy]
E -->|unsupported POSIX semantics| H[Unsupported]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
🤖 CodeAnt AI — Review Status
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
🏁 CodeAnt Quality Gate ResultsCommit: ✅ Overall Status: PASSEDQuality Gate Details
|
codeant-ai
Bot
added
the
size:L
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="crates/worldscript-secure-storage/src/pinned/windows/removal.rs" line_range="42" />
<code_context>
+ &dir.handle,
+ name,
+ FILE_GENERIC_READ | DELETE,
+ FILE_SHARE_READ,
+ )?;
+ let file = expect_kind(file, EntryKind::File)?;
</code_context>
<issue_to_address>
**Mapped writes bypass sharing**
When a process retains a writable memory-mapped view of the file while removal opens and authenticates it, a writable memory-mapped view can remain active after its creating file handle is closed, so the read-only share mode does not necessarily prevent writes to the opened file. The authentication can accept one byte state and `dispose` can then delete the same file after the mapped writer changes it, removing bytes that were never authenticated.
Use a Windows mechanism that also excludes or detects writes through existing writable mappings, or narrow the guarantee and ensure callers do not rely on stable authenticated bytes in that case.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and if the handle-relative lookup or authentication check is wrong, the POSIX delete disposition could remove a file or empty directory the caller did not intend to delete. Reverting stops future removals, but it cannot restore objects already deleted.
Blocking findings: crates/worldscript-secure-storage/src/pinned/windows/removal.rs:42
| &dir.handle, | ||
| name, | ||
| FILE_GENERIC_READ | DELETE, | ||
| FILE_SHARE_READ, |
There was a problem hiding this comment.
🟡 Medium · Mapped writes bypass sharing
When a process retains a writable memory-mapped view of the file while removal opens and authenticates it, a writable memory-mapped view can remain active after its creating file handle is closed, so the read-only share mode does not necessarily prevent writes to the opened file. The authentication can accept one byte state and dispose can then delete the same file after the mapped writer changes it, removing bytes that were never authenticated.
Use a Windows mechanism that also excludes or detects writes through existing writable mappings, or narrow the guarantee and ensure callers do not rely on stable authenticated bytes in that case.
Prompt for AI agents
In `crates/worldscript-secure-storage/src/pinned/windows/removal.rs` at line 42:
**Mapped writes bypass sharing**
When a process retains a writable memory-mapped view of the file while removal opens and authenticates it, a writable memory-mapped view can remain active after its creating file handle is closed, so the read-only share mode does not necessarily prevent writes to the opened file. The authentication can accept one byte state and `dispose` can then delete the same file after the mapped writer changes it, removing bytes that were never authenticated.
Use a Windows mechanism that also excludes or detects writes through existing writable mappings, or narrow the guarantee and ensure callers do not rely on stable authenticated bytes in that case.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bdf2011ff9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // A file system or Windows version without POSIX delete semantics: nothing is deleted. | ||
| Some(ERROR_INVALID_PARAMETER | ERROR_NOT_SUPPORTED) => Err(PinnedError::Unsupported), | ||
| _ => Err(io_error(error)), |
There was a problem hiding this comment.
When FileDispositionInfoEx is unavailable on Windows 10 1607 or an unsupported filesystem/driver, SetFileInformationByHandle can return ERROR_INVALID_FUNCTION; Microsoft's STL handles that result as an unsupported POSIX-delete capability. This match instead falls through to io_error, which becomes PinnedError::Io(Other), contradicting the adjacent contract and preventing callers from distinguishing a permanent unsupported platform from a transient I/O failure. Include ERROR_INVALID_FUNCTION (and preferably ERROR_CALL_NOT_IMPLEMENTED for equivalent implementations) in the Unsupported branch.
Useful? React with 👍 / 👎.
CodeAnt Nitpicks1 code suggestion1.
|
CodeAnt PR Risk: Medium Risk
Assessed commit: |
…445-gate4e-h1a2-windows-removal
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Quality Gate Profile: The Bare Minimum
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
|
Superseded by #1075, not merged. This PR was stacked on the history of the squash-merged #1069 and #1070, and its base branch no longer reflects The 2 findings open here are accounted for in the body of #1075 (table): the unmapped No history was rewritten and the branch is kept. Checks and reviews on this head did not carry over to #1075. |
…#1075) * feat(core): remove on Windows through the authenticated handle (#445) Fresh re-cut of #1071 on the main that carries the Unix hardening and the directory removal, not a retarget: that PR was stacked on the history of the squash-merged #1069 and #1070. Its one own change (bdf2011) is re-applied here with its tests, and the two findings that were open on it are dealt with. test-support only, no caller. One new unsafe call (the disposition, one fixed structure, its layout pinned by a test), no new package. - The object is opened through the handle-relative lookup of every child, with delete access and a share mode of read only, authenticated on that handle and then given the POSIX delete disposition on the same handle: no FILE_FLAG_DELETE_ON_CLOSE, no fallback to the classic disposition, a read-only file is refused. A directory is removed the same way and a directory with entries refuses (NotEmpty). The removal is reported as Removed(NotConfirmed): Windows offers no directory sync. A leftover removal is Unsupported, because Windows creates no quarantine directory. - The cases of remove.rs now run on Windows too. The directory case there expects the removal on Linux and on Windows and Unsupported on any other Unix system, which the Unix re-cut introduced and which would otherwise have failed the hosted Windows run. Findings carried from #1071: - Codex P2: ERROR_INVALID_FUNCTION, which an older Windows or a driver gives when it does not implement the information class, was not mapped and became a plain I/O error. Every code that means "no POSIX delete" now maps to Unsupported through one function, known_refusal, that a test pins (including that an unrelated code stays an error). - Sourcery: a writable memory-mapped view that another process created earlier and keeps after closing its handle is not stopped by the read-only sharing rule, so it can still change the authenticated bytes before the disposition. No sharing mode closes this; it is stated in the module documentation, the evidence document and the changelog as a same-user residual, not claimed closed, and not tested because a hosted run cannot be made to hold such a mapping reliably. Checked here only by compiling for x86_64-pc-windows-msvc (fmt, clippy -D warnings); the hosted Windows run is the evidence. The PR number in the changelog entry is set when this PR is opened. Issue #359. * docs(changelog): set the PR number of the Windows removal entry (#445) * docs(core): say that the removal binds the object, not its bytes, on Unix and on Windows (#445) CodeAnt asked whether the mapped-writer limit on Windows is acceptable for this slice. Looking at it showed that the Unix removal has the same limit in a wider form and never said so: there is no sharing mode, so a same-user process with a writable descriptor or a writable shared mapping can change the bytes between the authentication and the unlink. Both are now stated in the module documentation, the evidence document and the changelog as parts of the same-user residual, not closed. Documentation only. * fix(core): look at a directory again right before its disposition on Windows (#445) Codex P2 on 7c43e60: the share mode of a directory handle cannot stop a writer from converting the empty directory into a reparse point in place (the module already says so for the pinned directory), and the observer, a caller's code, runs between the check and the disposition. The disposition would then have removed the converted entry and reported it as Removed although the API refuses links. The held handle is now checked a second time right before the disposition: a junction made in that window is left, its target is untouched and the removal answers Redirected. This narrows the window and does not close it; a file cannot be converted because its handle shares no write access. Test: the directory is converted into a junction at the Checked stage with the existing convert_in_place helper. It runs on Windows only, so the hosted Windows run is the evidence (checked here by compiling for the Windows target); it is not mutation-checked locally. Documentation, changelog and evidence document say the same. test-support only, no new unsafe, no new package. Issue #359. * docs(core): limit the Windows write protection claim to handles (#445) CodeRabbit on 140e981: the changelog said no writer can change the authenticated bytes while its own entry notes that a writable mapped view that existed before can. The claim is now limited to handles that open the file, and the test header says the same. Wording only.
This branch was successfully deployed
1 participant
User description
Part of #359 (R-15 Gate 4, slice H1a-2), part 4 of 4, stacked on #1070.
test-supportonly: no production caller, no production deletion release.What this adds
Windows removal. The object is opened through the handle-relative lookup of every child (
NtCreateFile, pinned directory asRootDirectory), with delete access and a share mode of read only: a writer that already has the file open makes the open fail (Busy), no later writer can open it while the handle is held, and the name cannot be renamed, deleted or replaced. The caller authenticates the bytes on that handle, thenFileDispositionInfoEx(delete, POSIX semantics) is set on that same handle. NoFILE_FLAG_DELETE_ON_CLOSE, no fallback to the classic disposition (Unsupportedinstead), a read-only file is refused, a directory only while empty. There is no quarantine directory and none of the Unix window. Result durability isNotConfirmed(no directory sync through a read handle).FFI: one new
unsafecall (SetFileInformationByHandle, one fixedFILE_DISPOSITION_INFO_EX; no generic wrapper), SAFETY comment states handle lifetime, rights, structure, size (layout pinned by a test) and call-lifetime. No new package.Proof (hosted windows-latest is the oracle)
Rebind attempts (rename / delete / replace) fail while the handle is held; nobody can open the file for writing during the authentication and at the check; counter-proof: a handle opened with write sharing lets a writer change the bytes; a writer already open →
Busy, nothing changed; a delete-sharing reader does not stop the removal and the name is free at once; directory only while empty; read-only file refused; leftover removalUnsupported. The platform-neutral cases ofremove.rs(refused / failing / wrong-identity authentication, link / junction / directory / missing name, hard links, filled directory) now run on Windows too.Status:
H1A_2_WINDOWS_REMOVAL=IMPLEMENTED_TEST_SUPPORT_ONLY,H1_PROVED=NO,R1_PRODUCTION_RECLAMATION=NOT_ADMITTED,GATE4_TERMINAL=NO.Summary by Sourcery
Implement authenticated Windows removal in test-support while preserving fail-closed behavior and explicit durability limitations.
New Features:
Enhancements:
Documentation:
Tests:
Chores:
Summary by cubic
Implements Windows file and directory removal in
test-supportonly, where every removal previously refused withUnsupported.FileDispositionInfoEx(POSIX delete) on the same handle; a writer already holding the file makes removal failBusy.unsafecall (SetFileInformationByHandlewith one fixed structure whose layout is pinned by a test), with the SAFETY comment covering handle lifetime, rights, structure, and size.remove.rsnow run on Windows; no quarantine directory exists there, so leftover removal staysUnsupportedand durability reportsNotConfirmed.Written for commit 1900a73. Summary will update on new commits.
CodeAnt-AI Description
Add authenticated file and empty-directory removal on Windows for test-support
What Changed
Impact
✅ Prevents file changes between authentication and deletion✅ Preserves files when authentication fails or a writer is active✅ Avoids removing non-empty directories💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.