Restrict workspace state on Windows, where the declared modes do nothing - #9
Merged
Merged
Conversation
The store creates 0o700 directories and 0o600 files to keep container endpoint tokens to one account, and Windows implements neither: chmod is close to a no-op and every file reports 0o666. Under a default profile the store is private anyway, by inheritance — which is why this went unnoticed. Point OPENCHAMBER_WORKSPACE_STATE_DIR at a second drive or a shared folder and it inherits that location's permissions instead, and nothing objects. Windows states this with an access list, which Node cannot set, so the state root gets one through icacls and everything created beneath it inherits the result. Once per root, not once per write. Three details are load-bearing. SYSTEM and Administrators are named by security identifier, because their names are localised and "Administrators" does not exist on a Ukrainian install. Both system tools are named by absolute path, because Git for Windows ships a POSIX whoami that rejects /user and usually comes first on PATH — the same shadowing that made its tar read C:\ as a remote host. And the list is emptied before it is rebuilt: /inheritance:r drops what the parent contributed and /grant:r replaces only the principals it names, so an entry someone added explicitly for a third party would otherwise survive being "restricted". It runs through child_process rather than the shared runner. Redaction and provider attribution mean nothing for an OS primitive, and provider suites replace that runner wholesale — routing through it left the store unable to write a file in every test that mocks a container CLI. The state store test no longer skips itself on Windows. It asks each platform about the mechanism that actually restricts the store there, and it opens its own root to Everyone first: a directory made under %TEMP% is already private, so without that the assertion passes whether or not the code protects anything. Verified by disabling the protection and watching both suites fail.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The state store creates
0o700directories and0o600files so container endpoint tokens stay with one account. Windows implements neither —chmodis close to a no-op there and every file reports0o666.Under a default profile the store is private regardless, because it inherits the profile's permissions. That is why this went unnoticed, and it is also why it cannot be relied on: point
OPENCHAMBER_WORKSPACE_STATE_DIRat a second drive or a shared folder and the store inherits that location's permissions instead, with nothing in the code objecting.Windows expresses this with an access list, which Node cannot set. The state root now gets one through
icacls, and everything created beneath it inherits the result — once per root, not once per write.Details that are load-bearing
Administratorsdoes not exist on a Ukrainian or German install.whoamithat rejects/user, and it usually comes first onPATH— the same shadowing that made itstarreadC:\…as a remote host and broke workspace creation earlier on this branch. Found here the same way: the first run failed withwhoami: extra operand '/user'./inheritance:rdrops what the parent contributed and/grant:rreplaces only the principals it names, so an entry someone added explicitly for a third party would otherwise survive being "restricted".child_process, not the shared runner. Redaction and provider attribution mean nothing for an OS primitive, and provider suites replace that runner wholesale — routing through it left the state store unable to write a file in every test that mocks a container CLI.Testing
The state store test no longer skips itself on Windows; it asks each platform about the mechanism that actually restricts the store there. Both new suites open their own directory to Everyone first, because a directory made under
%TEMP%is already private and the assertion would otherwise pass whether or not the code protects anything.Verified by disabling the protection and confirming both suites fail, then restoring it. Full suite on Windows: 151 passed, 3 skipped, 0 failed.