Repository navigation
feat(core): remove on Windows through the authenticated handle (#445) - #1075
Conversation
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.
🤖 CodeAnt AI — Review Status
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
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. |
Reviewer's GuideThis PR replaces the Windows removal stub with test-support-only authenticated handle deletion: files and empty directories are opened relative to the pinned directory, protected by restrictive sharing, authenticated where applicable, and deleted via Sequence diagram for authenticated Windows removalsequenceDiagram
participant Caller
participant PinnedDir
participant Windows
participant Authenticator
participant Disposition
Caller->>PinnedDir: remove_file(name, authenticate, observer)
PinnedDir->>Windows: nt.open_relative(handle, name, DELETE, FILE_SHARE_READ)
Windows-->>PinnedDir: File handle
PinnedDir->>Authenticator: authenticate(PinnedFile)
alt authentication refused
Authenticator-->>PinnedDir: false
PinnedDir-->>Caller: Refused
else authentication succeeds
Authenticator-->>PinnedDir: true
PinnedDir->>Disposition: set_posix_delete(file)
Disposition->>Windows: SetFileInformationByHandle(FileDispositionInfoEx)
alt POSIX delete succeeds
Windows-->>Disposition: success
Disposition-->>Caller: Removed(NotConfirmed)
else directory is not empty
Windows-->>Disposition: ERROR_DIR_NOT_EMPTY
Disposition->>Windows: known_refusal(code)
Disposition-->>Caller: NotEmpty
else POSIX delete is unsupported
Windows-->>Disposition: ERROR_INVALID_PARAMETER, ERROR_NOT_SUPPORTED, or ERROR_INVALID_FUNCTION
Disposition->>Windows: known_refusal(code)
Disposition-->>Caller: Unsupported
end
end
Caller->>PinnedDir: remove_dir(name, observer)
PinnedDir->>Windows: nt.open_relative(handle, name, DELETE, FILE_SHARE_READ | FILE_SHARE_WRITE)
Windows-->>PinnedDir: empty directory handle
PinnedDir->>Disposition: set_posix_delete(handle)
Disposition->>Windows: SetFileInformationByHandle(FileDispositionInfoEx)
Windows-->>Disposition: success
Disposition-->>Caller: Removed(NotConfirmed)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
🏁 CodeAnt Quality Gate ResultsCommit: ✅ Overall Status: PASSEDQuality Gate Details
|
codeant-ai
Bot
added
the
size:L
|
Warning Review limit reachedEnable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Next included review available in 31 minutes. View limit details
📝 Walkthrough
Priority: ⬇️ Low Merge Risk: 🔵 Low · up to
|
![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.
|
[check-pr-size] PR size is over the target tier (normal profile): 8 files, 517 meaningful lines, 5 commits — limit ≤8 files / ≤400 lines / ≤6 commits. Consider splitting into smaller, independently reviewable PRs. |
CodeAnt PR Risk: Low Risk
Assessed commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…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.
|
Disposition of the CodeAnt "PR Risk: Medium" note on The limit is real and is not closed here. The read-only share mode stops every writer that opens the file after the removal opened it, and the hosted Windows run pins that (the counter-proof shows what write sharing would have let through). It cannot stop a writable view that another process created earlier and keeps after closing its handle: the sharing rule is about handles. I found no Windows mechanism that closes it. Whether it is acceptable for this slice is covered by the recorded decision on the same-user residual: it may stand as documentation for a What I added because of the question ( CodeAnt's risk note is a top-level comment without a thread, so it stays as it is; this is the written disposition. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![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: 7c43e60606
ℹ️ 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".
…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.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: qnbs/WorldScript-Studio/.coderabbit.yaml
- Review profile: CHILL
- Plan: Essentials
- Run ID:
b6621323-45a0-4162-915c-660accece7cc
📒 Files selected for processing: 6
CHANGELOG.mdcrates/worldscript-secure-storage/src/pinned/removal.rscrates/worldscript-secure-storage/src/pinned/tests/remove_windows.rscrates/worldscript-secure-storage/src/pinned/tests/windows_redirect.rscrates/worldscript-secure-storage/src/pinned/windows/removal.rsdocs/native/r15/GATE4D-JOURNAL-DURABLE-EVIDENCE.md
🚧 Files skipped from review as they are similar to previous changes: 1
- crates/worldscript-secure-storage/src/pinned/removal.rs
Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.
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.
|
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.
|
@codex review One request: the Codex review failed technically on the last two heads ( |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This branch was successfully deployed
1 participant
What this is
A fresh re-cut of #1071 on
main(52eb9f6f), which now carries the Unix hardening (#1073) and the Unix directory removal and leftover recovery (#1074). Not a retarget: #1071 was stacked on the history of the squash-merged #1069 and #1070. Its one own change (bdf2011f) is re-applied with its tests; one signed commit onmain. #1071 stays frozen until this PR is integrated and is then closed as superseded, not merged. No history was rewritten.test-supportonly: no caller. One newunsafecall (the disposition: one fixed structure, its layout pinned by a test); no new package, no new dependency feature. Slice H1a-2 part 4, part of #359 (Linear QNB-168).What it adds
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).Removed(NotConfirmed): Windows offers no directory sync. A leftover removal isUnsupported, because Windows creates no quarantine directory.remove.rsnow run on Windows too.The 2 findings that were open on #1071
ERROR_INVALID_FUNCTION(an older Windows or a driver that does not implement the information class) was not mapped and became a plain I/O errorERROR_INVALID_PARAMETER,ERROR_NOT_SUPPORTED,ERROR_INVALID_FUNCTION) maps toUnsupportedthrough one function,known_refusal, that a test pins; an unrelated code (ERROR_ACCESS_DENIED) and no code stay errors.Findings raised on this PR itself
140e981a): the held handle is looked at again right before the disposition; a junction made in that window is left, its target is untouched, the answer isRedirected. Narrows the window, does not close it. Testa_directory_converted_into_a_junction_after_the_check_is_not_deleted, which passes on the hosted Windows runner (Windows only, not mutation-checked locally).What is bound is the object, not its bytes (found while answering CodeAnt)
authenticatereads the open file and the removal then removes that object. On Windows the read-only share mode stops every writer that opens the file, but not a writable memory-mapped view that another process created earlier. On Unix 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; the Unix removal never said so. Both are now stated (module documentation, evidence document, changelog) as parts of the same-user residual, not closed. Documentation only (7c43e606).What changed from the original because of the Unix re-cuts
The directory case of
remove.rsexpects the removal on Linux and on Windows, andUnsupportedon any other Unix system (the Unix directory removal is Linux only, see #1074). Without that, the shared test would have failed the hosted Windows run.Limits, stated
Checked here only by compiling for
x86_64-pc-windows-msvc(cargo fmt --check,cargo clippy --all-targets -D warnings; the host andaarch64-apple-darwintoo). The hosted Windows run is the evidence: on the first head (a57f8ad5) the Windows, macOS and Ubuntu evidence jobs passed, and the Windows run executed all 11remove_windowstests including the new classification test and the shared directory case; the later heads add documentation and the directory revalidation, and the hosted Windows run of the current head (140e981a) passed its Windows removal tests including the new junction test. A file system without POSIX delete semantics isUnsupported, which the hosted runner does not exercise. The same-user residual (the Unixunlinkat(name)window and link-count heuristic, a foreign entry's identity without a held handle, and here the mapped writer) is unchanged and not accepted for production; owner-gated before any production use (H1b, see #359).pr:budgetagainstmain:CAUTION,OK, 4 commits, 8 files, 516 meaningful lines; canonicalpnpm run ci:prepushpassed. CI, CodeQL and independent review of the current head are pending; nothing from #1071 carries over.Part of #359.
Summary by Sourcery
Implement authenticated Windows removal for files and empty directories while preserving fail-closed behavior and clearly reporting unsupported platform semantics.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: