Problem/Motivation

Go to /admin/content and click Delete for some node entity.
Now you are on the Confirm page, where the Cancel Url points to '/content' instead to '/admin/content'.

The problem relies in ConfirmFormHelper::buildCancelLink, where from the query we get destination = '/admin/content', but before calling Url::fromUserInput for this path, we add a slash infront of the path. So we call Url::fromUserInput with the path '//admin/content' which returns '/content'. If we do not add the slash in front of the path, then the returned url points correctly to '/admin/content'.

Proposed resolution

Do not prepend a slash to the destination path in ConfirmFormHelper::buildCancelLink.

Remaining tasks

none

User interface changes

none

API changes

none

CommentFileSizeAuthor
#10 2501655-url-10-PASS.patch1.91 KBtim.plunkett
fix_cancel_links.patch714 byteshchonov
#4 cancel_urls_for_delete-2501655-4.patch732 byteswillzyx
#8 2501655-url-8-PASS.patch1.2 KBtim.plunkett
#8 2501655-url-8-FAIL.patch1.2 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, fix_cancel_links.patch, failed testing.

willzyx’s picture

willzyx CreditAttribution: willzyx commented

I cannot replicate the issue.. and looking at Url::fromUserInput

   * @throws \InvalidArgumentException
   *   Thrown when the user input does not begin with one of the following
   *   characters: '/', '?', or '#'.
hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented

@willzyx I made a new checkout and I still have the same problem.

What url to points the cancel button under /node/1/delete?destination=/admin/content in your installation? You go to this path when you point to /admin/content and from the table select delete for one of the node entities. Caution, do not go to the node entity and then to the local task delete, but select delete from the operations column from the table under /admin/content.

I tested with the node and the term entities and for both I get in ConfirmFormHelper::buildCancelLink the path with a leading slash and then a second one slash is being prepended.

willzyx’s picture

willzyx CreditAttribution: willzyx commented
Status: Needs work » Needs review
FileSize
cancel_urls_for_delete-2501655-4.patch732 bytes

@hchonov ok i got it. Running installation in a subdirectory (http://drupal.dev/drupal/) like I did when i tested the back link works as expected (and also the testbot runs installation in a subfolder).
If the docroot of the web server coincides with installation folder (http://drupal.dev/) the issue is reproducible.

this sadly underlines that there is not complete tests coverage for these cases

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented

Is this issue maybe related? It address also double slashes... #2504141: Information disclosure/open redirect vulnerability via blocks that contain a form

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Issue tags: +Needs tests

Very nice catch!

Let's make sure we add an automated test here so we don't break this again.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented

This also fixes #2504671: /admin/content delete VBO cancel link goes to /content
I'm going to try my hand at some test coverage

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Issue tags: -Needs tests
FileSize
2501655-url-8-FAIL.patch1.2 KB
2501655-url-8-PASS.patch1.2 KB

We had test coverage, it just wasn't complete enough.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Title: Cancel Urls for Delete Confirm Forms on Entity Lists are broken » ConfirmFormHelper::buildCancelLink() incorrectly handles ?destination URLs with a leading slash
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
FileSize
2501655-url-10-PASS.patch1.91 KB

Uploaded the FAIL patch twice.

The last submitted patch, 8: 2501655-url-8-FAIL.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Tag1 Consulting commented
Status: Needs review » Reviewed & tested by the community

Cool!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Fixed

Awesome, thanks!

Committed and pushed to 8.0.x.

  • webchick committed 15a3f2d on 8.0.x 
    Issue #2501655 by tim.plunkett, hchonov, willzyx: ConfirmFormHelper::...

The last submitted patch, 8: 2501655-url-8-PASS.patch, failed testing.

The last submitted patch, 8: 2501655-url-8-FAIL.patch, failed testing.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.