Skip to content

fix: block unvetted plugin setup code from executing in main realm#7762

Open
rish106-hub wants to merge 1 commit into
sugarlabs:masterfrom
rish106-hub:fix/7046-block-unvetted-plugin-setup
Open

fix: block unvetted plugin setup code from executing in main realm#7762
rish106-hub wants to merge 1 commit into
sugarlabs:masterfrom
rish106-hub:fix/7046-block-unvetted-plugin-setup

Conversation

@rish106-hub

Copy link
Copy Markdown
Contributor

Description

processPluginData (js/utils/utils.js) applies the isVettedPlugin() trust gate to 5 of 8 plugin handler types (FLOW/ARG/PARAMETER/ONSTART/ONSTOP), but not to the three setup types — BLOCKPLUGINS, GLOBALS, ONLOAD. Those route through the local safeEval() closure, which checks only userConfirmed, then executes the code via a Blob <script> in the main realm with full activity/window access.

Result: an unvetted plugin that clears the confirm() dialog runs arbitrary JavaScript with full privileges through those three keys — bypassing the exact-match whitelist in Logo.safePluginExecute() that already protects the other handler types. This is the core privilege-escalation concern in #7046, and it presents as an inconsistency: one trust predicate applied everywhere except these three handlers.

Related Issue

Partially addresses #7046

PR Category

  • Bug Fix — Fixes a bug or incorrect behavior
  • Feature
  • Performance
  • Tests — Adds or updates test coverage
  • Documentation
  • CI/CD

Changes Made

  • js/utils/utils.js: added the isVettedPlugin(pluginSource) check to the safeEval closure. Unvetted BLOCKPLUGINS/GLOBALS/ONLOAD setup code is now refused (logged) instead of executed in the main realm. Vetted sources (built-in `plugins/`, `./plugins/`, `localStorage:plugins`) keep their existing execution path. Reuses the existing predicate — no new helper.
  • Exported `processPluginData` through the guarded CommonJS export path so the loader can be tested in Jest without changing browser behavior.
  • `js/utils/tests/utils-plugin-security.test.js` (new): asserts unvetted `GLOBALS`/`BLOCKPLUGINS` setup produces no Blob/script injection (with a warning), and that vetted setup still executes.

Testing Performed

  • Confirmed the new tests fail when the gate is removed (they catch the hole), pass with it.
  • `npx jest` — 179 suites / 6078 tests pass, zero regressions.
  • `npx eslint` and `npx prettier --check` on both changed files — clean.

Checklist

  • I have tested these changes locally and they work as expected.
  • I have added/updated tests that prove the effectiveness of these changes.
  • I have updated the documentation to reflect these changes, if applicable.
  • I have followed the project's coding style guidelines.
  • I have run `pnpm run lint` and `pnpm exec prettier --check .` with no errors.
  • I have addressed the code review feedback from the previous submission, if applicable.
  • I have enabled "Allow edits from maintainers".

Additional Notes for Reviewers

Scoped intentionally narrow: this closes the immediate main-realm code-execution path for unvetted plugins as a single-file change, rather than the full iframe-sandbox/CSP overhaul in #7046's acceptance criteria (which remains valuable as follow-up work). It supersedes #7081's approach on current master by reusing the existing `isVettedPlugin` predicate uniformly instead of adding a parallel helper.

Tradeoff: unvetted plugins become partially inert (palette/blocks register; setup JS blocked) until a sandbox lands. Fail-closed is the safe default for an educational tool and matches the existing whitelist behavior in `Logo.safePluginExecute()`.

@github-actions github-actions Bot added bug fix Fixes a bug or incorrect behavior tests Adds or updates test coverage size/M Medium: 50-249 lines changed area/javascript Changes to JS source files area/tests Changes to test files labels Jul 8, 2026
@codecov

codecov Bot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 54.70%. Comparing base (d09973a) to head (5707000).
⚠️ Report is 48 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7762      +/-   ##
==========================================
+ Coverage   54.60%   54.70%   +0.10%     
==========================================
  Files         172      172              
  Lines       57270    57273       +3     
==========================================
+ Hits        31272    31332      +60     
+ Misses      25998    25941      -57     

☔ 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/javascript Changes to JS source files area/tests Changes to test files bug fix Fixes a bug or incorrect behavior size/M Medium: 50-249 lines changed tests Adds or updates test coverage

Projects

Development

Successfully merging this pull request may close these issues.

1 participant