fix(client): make target dirs writable before extracting#398
Conversation
a5821a0 to
e00d4e9
Compare
![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: e00d4e932a
ℹ️ 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".
| if mode := info.Mode().Perm(); mode&0o200 == 0 { | ||
| if err := os.Chmod(path, mode|0o200); err != nil { |
There was a problem hiding this comment.
When a read-only directory carries special permission bits such as setgid or sticky, storing only info.Mode().Perm() causes the first Chmod(path, mode|0o200) to clear those bits. A failed extract then restores the stripped mode, and a successful partial extract leaves absent directories stripped as well, so Extract no longer preserves directory modes for those inputs; keep the original mode's special bits when adding owner-write.
Useful? React with 👍 / 👎.
| var buf bytes.Buffer | ||
| assert.NoError(t, client.Archive(t.Context(), &buf, src, []string{"pkg"}, nil, 0)) | ||
|
|
||
| // A destination left read-only by a prior extract, as Hermit does. |
There was a problem hiding this comment.
This adds an explanatory comment inside a top-level Test* function. The repository instructions explicitly disallow comments in top-level tests, so please make the setup self-explanatory through helper names or assertions instead.
AGENTS.md reference: AGENTS.md:L19-L22
Useful? React with 👍 / 👎.
Extract restores over the directory a consumer previously extracted into. When that tree contains read-only directories (e.g. Hermit marks unpacked package trees read-only), GNU tar can't unlink the existing files to overwrite them — the parent directory isn't writable — and fails the whole extract with "tar: <path>: Cannot open: File exists". Callers then see the restore fail and fall back to a full reinstall. Add owner-write to every directory under the target before extraction. tar re-applies each archived directory's mode as it unpacks, so only directories absent from the incoming archive are left relaxed. If extraction fails, the original modes are restored so a failed restore doesn't leave a previously read-only tree writable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
e00d4e9 to
fd6f9c7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd6f9c7308
ℹ️ 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".
| if mode&0o200 == 0 { | ||
| if err := os.Chmod(path, mode|0o200); err != nil { |
There was a problem hiding this comment.
When a restore target contains a directory that is writable to the cachew process via group/other permissions but lacks the owner-write bit (for example a shared CI workspace owned by a service account with mode 0570), this condition still calls Chmod. A non-owner cannot chmod that directory, so Extract now fails before starting tar even though tar could have replaced files there using the existing group write permission; check effective writability or ownership before trying to relax the mode.
Useful? React with 👍 / 👎.
2 participants
Problem
Extractrestores a snapshot over the directory a consumer previously extracted into. When that existing tree contains read-only directories — e.g. Hermit marks unpacked package trees0555— GNU tar can't replace the existing files: tar replaces a file by unlinking and recreating it, and unlinking requires write permission on the parent directory. The whole extract fails with:Callers then see the restore fail and fall back to a full reinstall, defeating the cache.
tar --overwritedoesn't help: it avoids the unlink but then fails on the read-only files themselves withCannot open: Permission denied.Fix
Add owner-write to every directory under the target before launching tar. tar re-applies each archived directory's mode as it unpacks, so directories present in the incoming archive get their original permissions back.
If extraction fails — including a partial failure of the relaxing walk itself — the original directory modes are restored (best-effort) so a failed restore doesn't leave a previously read-only tree writable. Stat/walk/chmod errors during the relax step are wrapped and returned rather than swallowed, keeping the existing "restore failed, fall back to install" behavior.
Known limitation: partial archives
After a successful extract, pre-existing read-only directories that are absent from the incoming archive are left owner-writable. This is deliberate and documented on
Extract; the alternatives are worse:0555to0755would be re-locked, causing visible downstream failures. Leaving a dir owner-writable fails in the benign direction (owner-write is not a privilege boundary; the owner can chmod at any time).tar -tand parsing its quoted listing), which adds significant plumbing and new failure modes to a hot path.If precise mode fidelity for partial archives is wanted, happy to explore the tee/
tar -tapproach in a follow-up.Verification
TestExtractOverReadOnlyTreedoes a realArchive/Extractround trip: read-only (0555) source directories are archived, extracted over a destination containing a stale read-only file under0555directories, and the test asserts both that the file contents are replaced and that tar re-applies the archived0555directory modes.TestExtractFailureRestoresDirModesfeedsExtracta corrupt stream over a read-only tree and asserts the error is returned and the original0555modes are restored.archive.gochange stashed, it fails with the exacttar failed: exit status 2: tar: pkg/conf/cacerts: Cannot open: File existserror from the report.go test ./client/passes;gofmtandgolangci-lint run ./client/...are clean.