Skip to content

fix(security): refuse top-level asset loads and cap screenshot reads - #128

Merged
erseco merged 1 commit into
mainfrom
fix/asset-and-zip-hardening
Sep 26, 2026
Merged

erseco merged 1 commit into
mainfrom
fix/asset-and-zip-hardening

Conversation

@erseco

@erseco erseco commented Sep 26, 2026

Copy link
Copy Markdown
Contributor

Stack 1/5. Stacked PRs from a code-modernization pass (security first).

What

  • Asset fallback: AssetController::fetch served package HTML on the Nextcloud origin with no sandbox. A shared .elpx whose /asset/<id>/index.html was opened as a top-level page would run its scripts with the viewer's session. The route now answers 403 when Sec-Fetch-Dest: document. The viewer iframe sends iframe, so the fallback keeps working. Browsers without Fetch Metadata still pass, so this is a stopgap until the opaque-origin work in Secure opaque-origin viewer and editor preview #68 lands.
  • Screenshot reads: screenshot.png reads were only capped by the 500 MB package limit. getFromName() reserves the declared size up front, so one crafted package could fatal every thumbnail run (cron, Files grid). ZipEntryService::readEntry takes an optional per-read cap. The preview provider and ThumbnailController pass 10 MB, and the thumbnail route maps an oversized screenshot to 404 instead of a 500.

Not in scope

The same-origin iframe sandbox (allow-scripts + allow-same-origin) is covered by #68.

Verification

composer install                 exit 0
npm install                      exit 0
npm run typecheck                exit 0
npm test                         Tests 121 passed (121)
npm run lint                     exit 0
npm run build                    ✓ built
make architecture-check          Architecture records OK — 3 records, 0 changes.
make -n download-editor … typecheck   exit 0
vendor/bin/phpunit               OK (115 tests, 325 assertions)
composer cs:check                Found 0 of 32 files that can be fixed
git diff --check                 clean

The server asset fallback served package HTML under the Nextcloud origin
with no sandbox, so a crafted shared .elpx opened directly at
/apps/exelearning/asset/<id>/index.html ran its scripts as the victim.
Refuse requests whose Sec-Fetch-Dest is 'document'; the viewer iframe
sends 'iframe' and keeps working.

screenshot.png reads were only bounded by the 500 MB package limit, and
getFromName() reserves the declared size up front, so one package could
fatal every thumbnail run. Cap that read at 10 MB and map an oversized
screenshot to 404 in ThumbnailController.
@github-actions

Copy link
Copy Markdown
Contributor

Preview this PR in the Nextcloud Playground

Open this PR in the Nextcloud Playground

A fresh Nextcloud boots in your browser with this branch's exelearning app installed and enabled (log in as admin / admin). Two sample .elpx are seeded under exelearning-samples/ in Files — click one to open the viewer.

eXeLearning editor: v4.0.5 (overlaid at boot from the upstream release).

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.79%. Comparing base (8e1a47b) to head (b04449b).

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #128      +/-   ##
============================================
+ Coverage     93.75%   93.79%   +0.03%     
- Complexity      150      152       +2     
============================================
  Files            22       22              
  Lines           657      661       +4     
  Branches         54       54              
============================================
+ Hits            616      620       +4     
  Misses           34       34              
  Partials          7        7              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@erseco
erseco added this pull request to stack #130 September 26, 2026 06:14
@erseco
erseco merged commit 055e514 into main Sep 26, 2026
16 checks passed
@erseco
erseco deleted the fix/asset-and-zip-hardening branch September 26, 2026 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants