Background information

Problem/Motivation

Passing user input to Htmx::on() could lead to an XSS vulnerability.
I think it should be pretty obvious to devs that this is a bad idea but the doc does not warn about it.

Steps to reproduce

Htmx::on('click', $userProvidedValue);

Proposed resolution

Add a warning to the docblock saying $action should never be provided by untrusted users.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#6 3614218-nr-bot_pawusol5.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3614218

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

prudloff created an issue. See original summary.

prudloff’s picture

prudloff commented
Issue tags: +Security improvements

velmir_taky made their first commit to this issue’s fork.

velmir_taky opened merge request !16570

velmir_taky’s picture

velmir_taky commented
Status: Active » Needs review

Added the warning on the $action param: HTMX runs it as-is, so it must never contain untrusted input (XSS). Docblock only, phpcs clean.
AI-assisted, verified locally.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3614218-nr-bot_pawusol5.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

velmir_taky’s picture

velmir_taky commented
Status: Needs work » Needs review

Rebased on main. on() also got the array form since I opened this, so the warning now sits in the method description instead of on $action - that way it covers both. Docblock only.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Summary and MR don't appear to line up.

velmir_taky’s picture

velmir_taky commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented

Can I ask if this was you or was AI used as before?

velmir_taky’s picture

velmir_taky commented

Mostly me on this one. It's two lines of docblock - I traced that on() drops $action straight into the data-hx-on-* attr htmx evals, so the warning has to go on the method, not just $action (the array form doesn't use $action at all). Used AI for the wording like I said in #5, that's it.

smustgrave’s picture

smustgrave commented

Rock on

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Can we change to "The inline scripts run as-is, so must never contain untrusted input."

Dropped "they" as that doesn't seem like correct language here. Also dropped the vulnerability to not declare it.

Also summary mentions this is about $action should the description not be attached to that variable vs floating above it?