Harden archive extraction: zip-slip protection + exit-code error handling

[kaibae19]

Summary

Two small, independent fixes to ExtractionService.php:

1. Reject archive entries whose paths would escape the destination. Modern libzip already blocks traversal in ZipArchive::extractTo, but the unrar and 7za codepaths blindly trust archive entry paths. A crafted archive with an entry named e.g. ../../etc/something would write outside $extractTo. The first commit adds a per-format pre-extraction listing pass (libzip indices for ZIP, unrar lb for RAR, 7za l -ba -slt for the rest) and refuses to extract anything if an entry has a leading / or any .. path component.

2. Use real exit codes to detect tool failure instead of sizeof($output) <= 4/<= 5. The current heuristic can both miss real failures (verbose error output makes a failed extract look successful) and produce false positives (a successful extract of a small archive looks like a failure). The second commit checks $return, captures stderr into the output, and logs the full tool output on failure.

The commits are independent and can be cherry-picked separately.

Security review

A full security review of this branch (codebase walk, composer audit, npm audit, psalm, and a targeted look at the patched diff) came back clean — no XSS / SSRF / CSRF / command-injection findings and no static-analysis or dependency-audit regressions. Full report in the first comment on this PR.

Test plan

• Manual: ZIP containing an entry named ../escape.txt is refused with "Archive contains an unsafe path..." (verified locally via the OCS execute route).
• Manual: tar containing ../escape.txt (extractOther codepath via 7za) is refused.
• Manual: benign nested archives extract as before.
• Manual: small valid archive no longer trips the sizeof <= N false-failure path.
• CI / psalm: lint clean against existing baseline.

Notes

• No new dependencies. unrar lb and 7za l -ba -slt are part of the same unrar / p7zip-full packages the existing extraction commands already require.
• The unrar lb call assumes proprietary unrar (matching the existing unrar x ... -R ... -o+ syntax). If you'd like the RAR path to also work on unrar-free, happy to add a second listing strategy.
• No behavior change for the happy path on any format.
• I'm a downstream user, not a regular contributor — happy to refine in any direction the maintainers prefer (split smaller, larger test plan, doc updates, etc.).

[kaibae19]

Security review of harden-extraction

Full review before flipping this PR ready. Scope: the full app, not just the diff. Approach: codebase walk + dependency audits + static analysis.

Findings

Check	Result
PHP sink grep (exec / eval / include $var / preg_replace /e / create_function / etc.)	Only the four escapeshellarg-wrapped exec() calls in ExtractionService.php (two new for listing, two existing for extraction). Nothing else.
Patched ExtractionService.php shell safety	$file and $extractTo are server-resolved local-FS paths (IRootFolder → getLocalFile), wrapped with escapeshellarg. No user-controlled string reaches the shell unquoted.
JS XSS sink grep (v-html / innerHTML / outerHTML / dangerouslySetInnerHTML / eval / Function( / document.write)	None.
ExtractionController.php authn / authz	#[NoAdminRequired] is correct for a per-user operation. Path resolution goes through $this->userFolder = $this->rootFolder->getUserFolder($this->userId) — a user can only target their own files.
CSRF posture of /api/v1/extraction/execute	Inherited via OCSController → built-in OCS-APIRequest header / CSRF-token requirement. No #[NoCSRFRequired]. Safe.
Application.php, LoadExtractActions.php	Only Util::addScript / Util::addStyle. No attack surface.
Capabilities.php	Static feature list plus appManager->getAppVersion(). No user input.
extract-action.ts (frontend)	Builds the POST payload from Node.attributes (already validated by NC), posts via @nextcloud/axios (auto CSRF). On success constructs a Folder from the response; every field consumed (fileId, source, root, owner, permissions, mtime, mount-type, owner-display-name) is server-generated. No DOM injection.
templates/	content/index.php is <h1>Hello world</h1>; settings/index.php is an empty scaffold. Cosmetic dead code — flagged for cleanup but no security concern.
composer audit	No security vulnerability advisories found (28 packages).
npm audit --omit=dev --audit-level=high	0 vulnerabilities (the audit-fix was applied during the local build before pushing).
psalm against the project's psalm.xml + baseline	0 errors. The two commits introduce no static-analysis regressions.

Verdict

No XSS, SSRF, CSRF, command-injection, or path-traversal findings in the diff or the surrounding code. The two commits are safe to land as-is from a security standpoint.

Non-security observations surfaced during the review (not blocking, FYI)

1. src/actions/extract-action.ts:73 — window.OCP.Files.Router.goToRoute({..., fileid: data.fileId }, ...) reads data.fileId, but the server returns it under data.ocs.data.extracted.fileId. Looks like a navigation bug (post-extract redirect probably no-ops), not security. Out of scope here.
2. templates/content/index.php — literal <h1>Hello world</h1>. Cleanup target.
3. Pre-existing items worth tracking separately: no decompression-bomb / archive-size cap; ExtractionController.php imports private OC\Files\Filesystem for isFileBlacklisted, which may break across NC minor bumps.

Happy to file the non-security observations as separate issues if useful.