Fix 7zip bin permissions in docker file #802 - #803
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughArchive extraction now validates archives, routes RAR files through ChangesArchive extraction and import cleanup
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ImportManager
participant ArchiveService
participant bsdtar
participant 7za
ImportManager->>ArchiveService: extract archive
ArchiveService->>bsdtar: test and extract RAR
ArchiveService->>7za: test and extract non-RAR archive
ArchiveService-->>ImportManager: return extracted file paths
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Dockerfile to explicitly set the executable permission on 7zip-bin binaries to prevent runtime EACCES errors. The feedback suggests combining this new RUN command with the preceding npm prune command into a single instruction to minimize the number of layers in the final Docker image.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Wait for confirmation of user |
The bundled 7za binary cannot decode RAR archives, so .rar downloads silently produced an empty *_extracted folder instead of failing loudly. Route .rar through the system unrar binary (apk add unrar), validate archives before creating any output directory, and clean up on failure in ImportManager's extractIfArchive/confirmImport/processImport paths.
03ea91c to
aac264e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Dockerfile`:
- Around line 38-42: Remove all trailing whitespace after the line-continuation
backslashes in the Dockerfile RUN instruction, especially the continuation
following “npm prune --omit=dev”, so the chained find/chmod command remains part
of the same Docker instruction.
- Around line 28-32: Update the Dockerfile dependency installation so the unrar
extractor is obtained through a supported non-apk installation path or replaced
with a supported extraction dependency; remove reliance on apk add unrar while
preserving installation of su-exec, shadow, Python, and Apprise.
In `@server/services/ArchiveService.ts`:
- Around line 53-83: Update the unrar argument construction in the relevant
archive test/extraction flows to insert `--` before the test operand and before
both archive and output-directory operands, preventing downloaded paths
beginning with `-` from being parsed as switches. Then update the corresponding
`execFile` argument assertions in the archive service tests to expect the new
separators.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fa02af37-cb6e-4354-9b9a-6b206de80461
📒 Files selected for processing (5)
Dockerfileserver/__tests__/archive_service.test.tsserver/__tests__/import_manager.test.tsserver/services/ArchiveService.tsserver/services/ImportManager.ts
Alpine 3.24 (node:26-alpine) no longer ships a standalone unrar package in main or community, so `apk add unrar` fails CI's docker-build check. Alpine's libarchive-tools package (bsdtar) is statically linked with archive_read_support_format_rar and _rar5, covering both legacy and RAR5 archives including multi-volume sets, without depending on RARLAB's non-free unrar binary.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/services/ArchiveService.ts (1)
18-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting shared fixed-path binary resolution logic.
The comment notes this "Mirrors server/apprise.ts's resolveAppriseBinary" — the candidate-list + env-override +
accessSynccaching pattern is duplicated verbatim in spirit between the two files. A small shared helper (e.g.resolveFixedBinary(candidates, envVar)) would remove this duplication for future additions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/services/ArchiveService.ts` around lines 18 - 49, Extract the duplicated fixed-path binary lookup logic from resolveBsdtarBinary and the corresponding resolveAppriseBinary implementation into a shared helper, such as resolveFixedBinary, accepting candidate paths and the environment-variable name. Preserve environment override ordering, executable checks via accessSync, and cached resolution behavior for both callers, then update each resolver to use the shared helper.
♻️ Duplicate comments (1)
Dockerfile (1)
39-43: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winTrailing whitespace after the
\line-continuation breaks this RUN instruction.Line 42 ends with
&& \followed by trailing spaces. Since the backslash is no longer the last character on the line, Docker will not join Line 43 into thisRUN; the shell receives a dangling&&with no following command (syntax error), and the standalonefind ...line becomes an unrecognized Dockerfile instruction. This breaks the build and defeats the PR's goal of making7z*executable. This is the same defect flagged (and left unaddressed) in a prior review round on this file.🐛 Proposed fix — strip trailing whitespace after `\`
-RUN npm prune --omit=dev && \ - find node_modules/7zip-bin -type f -name "7z*" -exec chmod +x {} + +RUN npm prune --omit=dev && \ + find node_modules/7zip-bin -type f -name "7z*" -exec chmod +x {} +#!/bin/bash # Confirm trailing whitespace after line-continuation backslashes grep -nP '\\[ \t]+$' Dockerfile || echo "no trailing-whitespace-after-backslash found" sed -n '38,43p' Dockerfile | cat -A🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile` around lines 39 - 43, Remove all trailing spaces after the line-continuation backslash in the RUN instruction containing npm prune and find. Ensure the backslash is the final character on the continuation line so the find command remains part of the same Dockerfile RUN instruction.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@server/services/ArchiveService.ts`:
- Around line 18-49: Extract the duplicated fixed-path binary lookup logic from
resolveBsdtarBinary and the corresponding resolveAppriseBinary implementation
into a shared helper, such as resolveFixedBinary, accepting candidate paths and
the environment-variable name. Preserve environment override ordering,
executable checks via accessSync, and cached resolution behavior for both
callers, then update each resolver to use the shared helper.
---
Duplicate comments:
In `@Dockerfile`:
- Around line 39-43: Remove all trailing spaces after the line-continuation
backslash in the RUN instruction containing npm prune and find. Ensure the
backslash is the final character on the continuation line so the find command
remains part of the same Dockerfile RUN instruction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e737564a-05c4-43c8-8e74-c8138fe21b45
📒 Files selected for processing (3)
Dockerfileserver/__tests__/archive_service.test.tsserver/services/ArchiveService.ts
Remove trailing whitespace after Dockerfile RUN line continuation (was breaking the chmod chain from npm prune), sort apk package names alphanumerically, and prefer node: prefixed built-in imports in ArchiveService and its tests.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/__tests__/archive_service.test.ts (1)
293-293: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRestore
BSDTAR_PATHafter each test.This test file deletes
BSDTAR_PATHinbeforeEachbut assigns it in the RAR tests, so a final assignment can persist for later tests in the same worker. AddafterEach(() => delete process.env.BSDTAR_PATH)here or restore it after assignment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/__tests__/archive_service.test.ts` at line 293, Ensure the RAR tests clean up the BSDTAR_PATH environment variable after each test by adding an afterEach cleanup or restoring its prior value after the assignment near fakeBsdtarPath. Preserve the existing test setup while preventing BSDTAR_PATH from leaking into later tests.server/services/ArchiveService.ts (1)
113-120: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReturn only regular files.
listExtractedFiles()only skips directories, soextractWithBsdtar()can return symlink and special-entry paths from the target directory. Useentry.isFile()to filter out non-regular entries before returning paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/services/ArchiveService.ts` around lines 113 - 120, Update the directory traversal in listExtractedFiles() to add paths only when entry.isFile() is true, while continuing to recurse into directories via walk(). Exclude symlinks and other special entries from results returned to extractWithBsdtar().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Dockerfile`:
- Around line 32-33: Pin every dependency introduced by the Dockerfile RUN
command, including libarchive-tools, py3-pip, python3, shadow, su-exec, and
apprise. Use explicit apk package versions and a pinned apprise version or
constraints file so repeated builds resolve identical image-layer dependencies.
In `@server/services/ArchiveService.ts`:
- Around line 1-7: Reorder the imports in ArchiveService so the built-in
node:path import is grouped with the other node: imports at the top, before
third-party modules such as node-7z and 7zip-bin; leave the imported symbols and
behavior unchanged.
---
Outside diff comments:
In `@server/__tests__/archive_service.test.ts`:
- Line 293: Ensure the RAR tests clean up the BSDTAR_PATH environment variable
after each test by adding an afterEach cleanup or restoring its prior value
after the assignment near fakeBsdtarPath. Preserve the existing test setup while
preventing BSDTAR_PATH from leaking into later tests.
In `@server/services/ArchiveService.ts`:
- Around line 113-120: Update the directory traversal in listExtractedFiles() to
add paths only when entry.isFile() is true, while continuing to recurse into
directories via walk(). Exclude symlinks and other special entries from results
returned to extractWithBsdtar().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 42740df9-822a-483b-8384-8e2479393d1e
📒 Files selected for processing (3)
Dockerfileserver/__tests__/archive_service.test.tsserver/services/ArchiveService.ts
Pin apprise to 1.9.4 via pip (PyPI retains all versions, so this is safe long-term). Left the apk packages unpinned since Alpine's apk repos only keep the current build of each package per release branch - pinning to an exact NVR would make the build fail as soon as any of those packages gets a routine security bump.
|
/deploy tag=bsdtar Deployment Summary
Pull and run the image:docker pull ghcr.io/doezer/questarr:$TAG
docker run -d -p 5000:5000 \
-e POSTGRES_PASSWORD=your_password \
-e IGDB_CLIENT_ID=your_client_id \
-e IGDB_CLIENT_SECRET=your_client_secret \
ghcr.io/doezer/questarr:$TAGInspect the SBOM attestation:docker buildx imagetools inspect ghcr.io/doezer/questarr:$TAG --format '{{ json (index .SBOM "linux/amd64").SPDX }}' |
|
/deploy tag=bsdtar Deployment Summary
Pull and run the image:docker pull ghcr.io/doezer/questarr:$TAG
docker run -d -p 5000:5000 \
-e POSTGRES_PASSWORD=your_password \
-e IGDB_CLIENT_ID=your_client_id \
-e IGDB_CLIENT_SECRET=your_client_secret \
ghcr.io/doezer/questarr:$TAGInspect the SBOM attestation:docker buildx imagetools inspect ghcr.io/doezer/questarr:$TAG --format '{{ json (index .SBOM "linux/amd64").SPDX }}' |
|
/deploy tag=bsdtar Deployment Summary
Pull and run the image:docker pull ghcr.io/doezer/questarr:$TAG
docker run -d -p 5000:5000 \
-e POSTGRES_PASSWORD=your_password \
-e IGDB_CLIENT_ID=your_client_id \
-e IGDB_CLIENT_SECRET=your_client_secret \
ghcr.io/doezer/questarr:$TAGInspect the SBOM attestation:docker buildx imagetools inspect ghcr.io/doezer/questarr:$TAG --format '{{ json (index .SBOM "linux/amd64").SPDX }}' |
The confirmImport catch handler routes failures to manual_review_required (added in #840) so failed imports stay actionable and retryable, instead of a terminal 'error' status. The extraction-failure test added by this PR still asserted the old 'error' status, causing CI to fail.
Fix stale test assertion causing CI failure on #803
|
|
/deploy tag=bsdtar Deployment Summary
Pull and run the image:docker pull ghcr.io/doezer/questarr:$TAG
docker run -d -p 5000:5000 \
-e POSTGRES_PASSWORD=your_password \
-e IGDB_CLIENT_ID=your_client_id \
-e IGDB_CLIENT_SECRET=your_client_secret \
ghcr.io/doezer/questarr:$TAGInspect the SBOM attestation:docker buildx imagetools inspect ghcr.io/doezer/questarr:$TAG --format '{{ json (index .SBOM "linux/amd64").SPDX }}' |
2 participants
This pull request updates the
Dockerfileto address a runtime permissions issue with the7zip-binpackage. Specifically, it ensures that the7zabinary is executable, which prevents extraction failures due to permission errors.Build and runtime reliability:
RUN findcommand to explicitly set the executable bit on all7z*files withinnode_modules/7zip-bin, ensuring the7zabinary is runnable and preventing EACCES errors at runtime.Summary by CodeRabbit
libarchive-toolsand ensuring bundled 7-Zip executables have correct permissions to prevent runtime access errors.