Add opt-in owner permission normalization to @tryghost/zip extract#761
Add opt-in owner permission normalization to @tryghost/zip extract#761sagzy wants to merge 3 commits into
Conversation
- Adds an `ensureOwnerPermissions` option (default `false`) to `extract()` - When enabled, normalizes extracted entry permissions so the owner can always read/move/remove the result: directories gain at least owner rwx and files gain at least owner rw, while existing execute/group/world bits are preserved - Fixes archives containing read-only directories (e.g. `dr-xr-xr-x`/`0555`) that otherwise fail to extract because nested files cannot be written into them - Normalization mutates only the in-memory entry handed to extract-zip; the source zip is never rewritten or chmodded, and it runs after the existing size-limit, symlink and filename checks - Defaults are unchanged for existing callers Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 22 minutes and 11 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #761 +/- ##
==========================================
- Coverage 98.04% 97.54% -0.51%
==========================================
Files 84 3 -81
Lines 2759 122 -2637
Branches 506 26 -480
==========================================
- Hits 2705 119 -2586
+ Misses 12 1 -11
+ Partials 42 2 -40 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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: eaef85d500
ℹ️ 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".
…ions Previously, entries with no POSIX mode bits early-returned from ensureOwnerPermissions and were handed to extract-zip as mode 0. When the caller combined `ensureOwnerPermissions: true` with a `defaultDirMode`/ `defaultFileMode` that omits owner bits (e.g. 0o555/0o444), extract-zip synthesized that owner-less default, so a no-POSIX-mode directory/file could still be created without owner write — violating the option's guarantee. Now zero-mode entries resolve the same default extract-zip would apply (mirroring its getExtractedMode fallback) and then have the owner bits OR-ed in, so the owner can always read/move/traverse the extracted tree regardless of the supplied defaults. The default case (no custom defaults) is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The JSDoc for `defaultDirMode`, `defaultFileMode`, `onEntry`, and the `limits`
sub-fields omitted the `[ ]` optional markers, so the types generated from the
JSDoc treated them as required. TypeScript consumers (e.g. gscan) could not call
`extract(zip, dest, {ensureOwnerPermissions: true})` without also supplying those
params. They all have defaults or are "if present", so they are now correctly
marked optional.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
3 participants
Summary
Adds an opt-in
ensureOwnerPermissionsoption to@tryghost/zip'sextract()that normalizes extracted entry permissions beforeextract-zipwrites them. This fixes archives that contain read-only directories (e.g.dr-xr-xr-x/0555) without changing default behavior for existing callers.Why
Some user-supplied archives (e.g. theme zips) store directories as read-only (
0555). Whenextract-ziprecreates such a directory first and then tries to write a nested file into it, extraction fails withEACCES. Even when a read-only directory has no children, it lands on disk without owner write, so the caller can't subsequently move or remove the extracted tree.A recursive post-extract
chmodis not a robust primary fix, because the bad directory modes break extraction before any post-processing could run. Normalizing per-entry, just before the write, fixes the failure at the source.What changed
packages/zip/lib/extract.js:options.ensureOwnerPermissions?: boolean(defaultfalse).true, each entry's mode is normalized in the wrappedonEntrycallback:rwx(0o700).rw(0o600).extract-zipexactly (POSIX dir bit, trailing slash, Windows directory attribute); inferred directories without POSIX type bits get the directory type bit set.extract-zip's own default modes still apply.extract-zipis mutated — the original zip is never rewritten or chmodded.onEntry, so all existing safety checks still apply. No async work is added insideonEntry(extract-zip does not await it).throwOnSymlinksto share the newgetEntryModehelper and mode constants (behavior unchanged).packages/zip/README.md: documented the new option and its intended use (trusted temporary extraction of user-supplied archives where the caller must be able to read, move, and remove the extracted tree).Tests
Added Vitest coverage that builds zips programmatically with
archiver(no dependency on the systemzip):ensureOwnerPermissions: truesuccessfully extracts a zip with a0555directory, a nested file, a read-only file (owner write added), and an already-writable file (left untouched).SYMLINK_NOT_ALLOWEDwith the option enabled.pnpm --filter @tryghost/zip test→ 21 passing; lint andoxfmt --checkclean.extract.jsis at 100% line / 98% branch coverage.Notes for reviewers
ensureOwnerPermissionsin a follow-up when validating uploaded theme zips.extract-zipmasks the on-disk mode to0o777, so no setuid/setgid/sticky escalation is possible.