OpenModalDialogWithUrl command

Closes #3408738

added 19 commits

• 2d3c24fa...0f1bc004 - 18 commits from branch project:11.x
• b4dc53aa - OpenModalDialogWithUrl command and test

Compare with previous version

added 1 commit

• 74eab13f - Add optional trailing comma in OpenModalDialogWithUrl::__construct

Compare with previous version

Why does this not extend \Drupal\Core\Ajax\OpenModalDialogCommand?

If it doesn't, we should document the relationship and difference in the docblock

We are not reusing anything from \Drupal\Core\Ajax\OpenModalDialogCommand class so it's not required according to me.

That's not an answer

These are clearly conceptually related. Plus, this is clearly using the same JS. So the relationship must be documented.

Otherwise it becomes even more confusing/difficult for a developer to choose between these.

This is done intentionally as we don't want the public methods and it's much more similar to the RedirectCommandthan the OpenModalDialogCommand , well does open the url in a modal.

but we could add a comment to link them as means to open modals still OpenModalDialogCommand and OpenModalDialogWithUrl

Yes, please link to them with an @see and describe the difference.

Which public methods do you not want? Please list them and describe why they're unwanted in this docblock.

I added a comment on OpenModalDialogWithUrl and reference to the AjaxCommandsTest which might be helpful.

My feedback on !4773 has not been addressed/incorporated here AFAICT.

added 61 commits

• 74eab13f...3ae37397 - 58 commits from branch project:11.x
• 3affa123 - OpenModalDialogWithUrl command and test
• 64f81d96 - Add optional trailing comma in OpenModalDialogWithUrl::__construct
• 49db1f92 - Addressed feedbacks related to consistent use of naming

Compare with previous version

Wait Does this allow opening arbitrary URLs in modals? If so, is that safe? This would then appear to be a new attack surface?!

Why would this be any different that OpenModalDialogCommand , where it just takes in the content instead of the url, this isn't exposed to user input anyways.

Why would this be any different that OpenModalDialogCommand , where it just takes in the content instead of the url

The difference is exactly what you say: content being passed in instead of a URL. A URL being allowed means this creates the potential for embedding a remote site inside a Drupal dialog. That's the new attack surface.

That alone wouldn't be enough, but combined with the fact that there's a pure client-side way to trigger these modal dialogs (using Drupal.AjaxCommands.prototype.openModalDialogWithUrl), that'd mean no server-side code is necessary to trick Drupal into loading external content!

@wimleers How is this different from calling

    const elementSettings = {
      progress: { type: 'throbber' },
      dialogType: 'modal',
      url: 'http://a-very-bad-url.io',
      httpMethod: 'GET',
    };
    Drupal.ajax(elementSettings).execute();

changed this line in version 9 of the diff

I'm reviewing this with https://www.drupal.org/node/2532212 in mind, which was done for SA-CORE-2015-002 — see https://www.drupal.org/forum/newsletters/security-advisories-for-drupal-core/2015-10-21/drupal-core-overlay-less-critical.

The code you're citing is JS code. I agree that the risk there is limited/non-existent: Drupal strictly forbids inline <script> tags, and if anybody manages to load arbitrary JS, well, then all bets are off

I'm concerned about how PHP code using this could be susceptible to an open redirect. The sample code in this MR:

        $response->addCommand(new OpenModalDialogWithUrl(Url::fromRoute('ajax_test.dialog_form')->toString(), []));

That one is fine, because it hard-codes the URL. But what if it's based on some parameter?

IOW: I'm concerned about how one could construct a request to a Drupal route by creating content like <p>See <a href="/some-controller/?a-sneaky-query-string">this other page</a>!</p>, which could then do:

        $response->addCommand(new OpenModalDialogWithUrl(Url::fromUri(…), $some_param, []));

The difference is that OpenModalDialogWithUrl fetches HTML from a URL, whereas the existing OpenModalDialogCommand is given HTML.

That's why I think that OpenModalDialogWithUrl needs protection similar to \Drupal\Core\Routing\LocalAwareRedirectResponseTrait::isLocal().

Thank you, that helps me understand your concerns. I agree that we could potentially add hardening for that scenario. The reason I thought this might not be strictly required for a commit was that \Drupal\Core\Ajax\RedirectCommand does not yet have similar hardening either. That might have been an oversight in SA-CORE-2015-002, so I'm wondering if we could open a follow-up to do the hardening for both commands at once?

There's an important difference though:

• the existing RedirectCommand changes the entire viewport and the URL in the browser bar (all it does is set window.location). So there are multiple ways that the end user can see that they're no longer on the original website
• the new OpenModalDialogWithUrlCommand does not change the entire viewport, the URL stays the same. If a malicious remote URL is loaded instead of a trusted local one, **there is no way at all to see this! **

For that reason, I think it's commit-blocking here, because the risk is much greater.

I checked SA-CORE-2015-002 again and it looks like there was an actual attack vector with the redirect commands because the Field UI allowed users to pass redirect URLs as part of query parameters.

In this use case, this is all hypothetical because it would require the attacker first gain a way to pass URLs for this command. I'm not saying we shouldn't implement such hardening at all, but I'm just saying that it might not have to block this issue from moving forward.

I haven't thought through the relative security implications of this properly, but I think for adding hardening, it can take the logic from LocalAwareRedirectResponseTrait::isLocal() and throw a LogicException or similar if that fails.

If that's easy to implement, to me it makes sense to do that here (it might be quicker than deciding whether to do it here or not), and then apply the same hardening to RedirectCommand in a separte issue.

My feedback on !4773 has not been addressed/incorporated here AFAICT.

added 1 commit

• 4c5edf0d - change to throbber

Compare with previous version

added 241 commits

• 4c5edf0d...870e5c1c - 237 commits from branch project:11.x
• 5a90c005 - OpenModalDialogWithUrl command and test
• f06b6e1c - Add optional trailing comma in OpenModalDialogWithUrl::__construct
• f0a26172 - Addressed feedbacks related to consistent use of naming
• f26be8c3 - change to throbber

Compare with previous version

My feedback on !4773 has not been addressed/incorporated here AFAICT.

added 1 commit

• d058a149 - better comment

Compare with previous version

added 1 commit

• 121b7dff - phpcs

Compare with previous version

added 1 commit

• 6c719331 - new test and remove other one

Compare with previous version