Skip to content

[FIX][UI]: Replace inline event handlers on Plugins page to survive innerHTML sanitizer#3288

Merged
crivetimihai merged 3 commits intomainfrom
3271-plugin-filters
Mar 6, 2026
Merged

[FIX][UI]: Replace inline event handlers on Plugins page to survive innerHTML sanitizer#3288
crivetimihai merged 3 commits intomainfrom
3271-plugin-filters

Conversation

@gcgoncalves
Copy link
Copy Markdown
Collaborator

🐛 Bug-fix PR

📌 Summary

Closes #3271

🔁 Reproduction Steps

  1. Open the Admin UI
  2. Navigate to the Plugins panel
  3. Edit the filters values or search by text for a plugin.

🐞 Root Cause

admin.js globally patches Element.prototype.innerHTML via installInnerHtmlGuard() (admin.js:532) as a security measure. The patched setter calls sanitizeHtmlForInsertion() (admin.js:461–505)
which strips every attribute beginning with "on" (line 485–488):

if (attrName.startsWith("on")) {
    element.removeAttribute(attribute.name); // strips oninput, onchange, onclick, onkeydown…
}

When the plugins tab is first clicked (admin.js:8266–8310), the handler fetches plugins_partial.html and injects it with:

pluginsPanel.innerHTML = html;   // line 8290 — triggers the guard

This silently strips every inline event handler in plugins_partial.html before any user interaction:

Attribute stripped. Element Effect
oninput="filterPlugins()" Search input Typing does nothing
onchange="filterPlugins()" (×5) Mode/Status/Hook/Tag/Author selects Changing dropdown does nothing
onclick="filterByHook/Tag/Author(...)" Statistics badges Clicking does nothing
onclick="showPluginDetails(...)" "View Details" buttons Modal never opens
onkeydown="handleKeydown(...)" All interactive items Keyboard navigation broken

window.filterPlugins and the other related functions are defined by initializePluginFunctions() (called right after), but there are no events bound to the DOM elements, so nothing calls them.

💡 Fix Description

Use two container-level event listeners in initializePluginFunctions():

  1. One input+change listener on the filter section — catches events from all child and elements via event bubbling. Calls filterPlugins() automatically for any filter interaction. Replaces the 6 stripped oninput/onchange inline attributes. One delegated click listener on #plugins-panel — handles badge clicks (hook/tag/author), "View Details", and modal close. It reads filter values from data-* attributes on the clicked element and replaces all stripped onclick inline attributes. 🧪 Verification Check Command Status Lint suite make lint Unit tests make test Coverage ≥ 80 % make coverage Manual regression no longer fails steps / screenshots ✅ Checklist [ ] Code formatted (make black isort pre-commit) [ ] No secrets/credentials committed

@gcgoncalves gcgoncalves marked this pull request as ready for review February 26, 2026 10:50
@gcgoncalves gcgoncalves force-pushed the 3271-plugin-filters branch 2 times, most recently from b2a8515 to 10bd8b5 Compare February 26, 2026 11:28
@gcgoncalves gcgoncalves changed the title Add event listener for plugin filters 3271 - Add event listener for plugin filters Feb 26, 2026
@gcgoncalves gcgoncalves force-pushed the 3271-plugin-filters branch 2 times, most recently from 77b4603 to a3e742e Compare February 27, 2026 10:06
@crivetimihai crivetimihai added this to the Release 1.0.0-RC2 milestone Feb 27, 2026
@crivetimihai crivetimihai changed the title 3271 - Add event listener for plugin filters [FIX][UI]: Replace inline event handlers on Plugins page to survive innerHTML sanitizer Feb 27, 2026
@crivetimihai crivetimihai added bug Something isn't working frontend Frontend development (HTML, CSS, JavaScript) javascript Javascript or typescript ui User Interface SHOULD P2: Important but not vital; high-value items that are not crucial for the immediate release labels Feb 27, 2026
@crivetimihai
Copy link
Copy Markdown
Member

Thanks @gcgoncalves. Excellent root cause analysis — the innerHTML sanitizer stripping inline on* handlers is the same pattern affecting LLM Chat (#3303). The addEventListener approach is the correct fix. This should be the pattern for all future HTML templates to avoid regression.

@Lang-Akshay Lang-Akshay self-requested a review February 27, 2026 15:40
@Lang-Akshay
Copy link
Copy Markdown
Collaborator

Thanks for the PR @gcgoncalves.

Here are the review findings

Detailed Criteria Assessment — Issue #3271

Issue Criterion Status Notes
#3271 Search input triggers filtering ✅ Done Delegated input listener on #plugin-filters replaces stripped oninput
#3271 Dropdown selects trigger filtering ✅ Done Delegated change listener on #plugin-filters replaces 5 stripped onchange attrs
#3271 Hook/Tag/Author badges filter on click ✅ Done Delegated click listener on #plugins-panel reads data-filter-* attributes
#3271 "View Details" button opens modal ✅ Done data-show-plugin attribute + delegated click
#3271 Modal close button works ✅ Done data-close-plugin-modal attribute + delegated click
#3271 Keyboard activation of badges ⚠️ Partial onkeydown="handleKeydown(…)" removed but no delegated keydown listener added — keyboard-only users can no longer activate filter badges via Enter/Space

⚠️ The onkeydown + handleKeydown() combo (defined at admin.js:32649) lets keyboard-only users press Enter or Space on a focused badge to trigger the same action as a click:

This PR removed both onclick and onkeydown from every badge element and replaced onclick with a delegated click listener on #plugins-panel. But it only added a delegated click listener — there's no corresponding delegated keydown listener. The role="button" tabindex="0" attributes are still there, so the badges are still focusable via Tab, but pressing Enter or Space on them does nothing.

@gcgoncalves gcgoncalves force-pushed the 3271-plugin-filters branch from c4f13f6 to 9e7bc65 Compare March 5, 2026 09:45
@gcgoncalves gcgoncalves force-pushed the 3271-plugin-filters branch 3 times, most recently from f0ec81a to bf807c4 Compare March 5, 2026 15:18
@gcgoncalves gcgoncalves added release-fix Critical bugfix required for the release labels Mar 5, 2026
Copy link
Copy Markdown
Collaborator

@marekdano marekdano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good approach to use data attributes in this case!

Manually tested, and it works as expected.

LGTM 🚀

@gcgoncalves gcgoncalves requested a review from marekdano March 6, 2026 08:49
marekdano
marekdano previously approved these changes Mar 6, 2026
Signed-off-by: Gabriel Costa <gabrielcg@proton.me>
…iew polyfill

- Remove unused imports (beforeEach) and destructured variables (card1, card2, hookFilter)
- Add HTMLElement.prototype.scrollIntoView polyfill for JSDOM environment
- Revert unrelated package-lock.json changes

Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
…controls

The delegated keydown listener on #plugins-panel was calling
preventDefault() unconditionally for Enter and Space before checking
whether the target was actually an actionable element (badge, button).
This blocked typing spaces in the search box and pressing Enter on
select dropdowns.

Fix: dispatchPluginAction() now returns a boolean indicating whether
it matched an action. The keydown handler only calls preventDefault()
when an action was actually dispatched.

Add regression tests: Space in #plugin-search and Enter on
#plugin-mode-filter must not have defaultPrevented set.

Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
@crivetimihai
Copy link
Copy Markdown
Member

Maintainer Review

Rebased onto main (clean, no conflicts) and applied the following fixes:

1. Keydown regression fix (critical)

The delegated keydown handler on #plugins-panel called e.preventDefault() unconditionally for Enter and Space before checking whether the target was an actionable element. This swallowed Space in the search box (typing a b produced ab) and blocked Enter on <select> dropdowns.

Fix: dispatchPluginAction() now returns a boolean. The keydown handler only calls preventDefault() when an action was actually dispatched — native form controls keep their default behavior.

2. Test cleanup

  • Removed unused import (beforeEach) and unused destructured variables (card1, card2, hookFilter) flagged by eslint.
  • Added HTMLElement.prototype.scrollIntoView = () => {} polyfill — JSDOM doesn't implement this, and filterByHook/Tag/Author call it, causing TypeError in stderr on every badge test.
  • Added 2 regression tests: Space in #plugin-search and Enter on #plugin-mode-filter must not have defaultPrevented set.

3. Removed noise

  • Reverted unrelated package-lock.json changes ("peer": true markers from a different npm version).
  • Reverted stray docker-compose-embedded.yml whitespace change.

Test results

  • 18/18 in admin-plugin-filters.test.js (16 original + 2 new regression tests)
  • 805/805 across the full JS test suite

Copy link
Copy Markdown
Member

@crivetimihai crivetimihai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. The event delegation approach is correct, consistent with existing patterns (token panel, tool-ops panel), and more CSP-friendly than inline handlers. Fixed the keydown regression where Space/Enter was swallowed in form controls. All 805 JS tests pass.

@crivetimihai crivetimihai merged commit f02611a into main Mar 6, 2026
47 checks passed
@crivetimihai crivetimihai deleted the 3271-plugin-filters branch March 6, 2026 23:13
@crivetimihai crivetimihai self-assigned this Mar 9, 2026
MohanLaksh pushed a commit that referenced this pull request Mar 12, 2026
…nnerHTML sanitizer (#3288)

* Add event listener for plugin filters

Signed-off-by: Gabriel Costa <gabrielcg@proton.me>

* fix(tests): clean up plugin filter tests — lint fixes and scrollIntoView polyfill

- Remove unused imports (beforeEach) and destructured variables (card1, card2, hookFilter)
- Add HTMLElement.prototype.scrollIntoView polyfill for JSDOM environment
- Revert unrelated package-lock.json changes

Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

* fix(ui): prevent keydown handler from swallowing Space/Enter in form controls

The delegated keydown listener on #plugins-panel was calling
preventDefault() unconditionally for Enter and Space before checking
whether the target was actually an actionable element (badge, button).
This blocked typing spaces in the search box and pressing Enter on
select dropdowns.

Fix: dispatchPluginAction() now returns a boolean indicating whether
it matched an action. The keydown handler only calls preventDefault()
when an action was actually dispatched.

Add regression tests: Space in #plugin-search and Enter on
#plugin-mode-filter must not have defaultPrevented set.

Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

---------

Signed-off-by: Gabriel Costa <gabrielcg@proton.me>
Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
Co-authored-by: Mihai Criveti <crivetimihai@gmail.com>
Yosiefeyob pushed a commit that referenced this pull request Mar 13, 2026
…nnerHTML sanitizer (#3288)

* Add event listener for plugin filters

Signed-off-by: Gabriel Costa <gabrielcg@proton.me>

* fix(tests): clean up plugin filter tests — lint fixes and scrollIntoView polyfill

- Remove unused imports (beforeEach) and destructured variables (card1, card2, hookFilter)
- Add HTMLElement.prototype.scrollIntoView polyfill for JSDOM environment
- Revert unrelated package-lock.json changes

Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

* fix(ui): prevent keydown handler from swallowing Space/Enter in form controls

The delegated keydown listener on #plugins-panel was calling
preventDefault() unconditionally for Enter and Space before checking
whether the target was actually an actionable element (badge, button).
This blocked typing spaces in the search box and pressing Enter on
select dropdowns.

Fix: dispatchPluginAction() now returns a boolean indicating whether
it matched an action. The keydown handler only calls preventDefault()
when an action was actually dispatched.

Add regression tests: Space in #plugin-search and Enter on
#plugin-mode-filter must not have defaultPrevented set.

Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

---------

Signed-off-by: Gabriel Costa <gabrielcg@proton.me>
Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
Co-authored-by: Mihai Criveti <crivetimihai@gmail.com>
Signed-off-by: Yosief Eyob <yosiefogbazion@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working frontend Frontend development (HTML, CSS, JavaScript) javascript Javascript or typescript release-fix Critical bugfix required for the release SHOULD P2: Important but not vital; high-value items that are not crucial for the immediate release ui User Interface

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG][UI]: Plugins page filters and search not working

4 participants