Background information

Problem/Motivation

form.js passes user input to $():

const $target = $(`#${hash}`);

I don't think this can be exploited in modern jQuery, it triggers a syntax error if hash is not a valid ID selector.

However passing unsanitized user input to $() is a bad practice, we should probably sanitize the hash to make sure it does not contain dangerous characters.

Steps to reproduce

Proposed resolution

An easy fix could be to use document.getElementById() to make sure the hash is interpreted as an ID.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3617238

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 opened merge request !16728

prudloff’s picture

prudloff commented
Status: Active » Needs review
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Related issues: +#2395065: Certain URL fragments cause javascript error

Not exactly the same issue but closely related to #2395065: Certain URL fragments cause javascript error

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Actually I think this is the same issue and this solves it in a better way, we could port the test from there to here.

longwave-bot made their first commit to this issue’s fork.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Ported the test from the linked issue, and added a happy path test while I was there, as this functionality previously had no test coverage that I could see.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Ran the test-only for the ported tests


/form-test/url|Error: Syntax error, unrecognized expression: #edit-contact/broken
    at T.error (http://web/core/assets/vendor/jquery/jquery.min.js?v=4.0.0:2:2495)
    at U (http://web/core/assets/vendor/jquery/jquery.min.js?v=4.0.0:2:5519)
    at Y (http://web/core/assets/vendor/jquery/jquery.min.js?v=4.0.0:2:6042)
    at eq (http://web/core/assets/vendor/jquery/jquery.min.js?v=4.0.0:2:17998)
    at T.ew [as find] (http://web/core/assets/vendor/jquery/jquery.min.js?v=4.0.0:2:9403)
    at T.fn.init.find (http://web/core/assets/vendor/jquery/jquery.min.js?v=4.0.0:2:19731)
    at new T.fn.init (http://web/core/assets/vendor/jquery/jquery.min.js?v=4.0.0:2:20630)
    at T (http://web/core/assets/vendor/jquery/jquery.min.js?v=4.0.0:2:1002)
    at HTMLAnchorElement.handleFragmentLinkClickOrHashChange (http://web/core/misc/form.js?v=12.0-dev:300:23)
    at HTMLAnchorElement.debounced (http://web/core/misc/debounce.js?v=12.0-dev:46:21)
/var/www/html/core/tests/Drupal/FunctionalJavascriptTests/WebDriverTestBase.php:128
/var/www/html/core/modules/system/tests/src/FunctionalJavascript/Form/FormUrlFragmentTest.php:35

Not sure if #2395065: Certain URL fragments cause javascript error should be closed? That ticket does 1 more change but with the test coverage added here that issue isn't appearing.

Will let committer decide.