Problem/Motivation

Open this issue after #2478151: Shortcuts to pages generated by views are not recognized as added to the shortcutset and are being added multiple times
Shortcuts names for node view routes are not properly sanitized using quick link.
In the pictures below you can see what happens trying to add a node in the shorcut set





Proposed resolution

Properly escape shortcuts name in shortcut_preprocess_page()

Remaining tasks

Make a patch.

User interface changes

none

API changes

none

CommentFileSizeAuthor
#24 2478907-24.patch4.94 KBlokapujya
#20 interdiff.txt1005 byteswillzyx
#20 shortcuts_names_for-2478907-20.patch4.79 KBwillzyx
#13 shortcuts_names_for-2478907-13.patch4.37 KBwillzyx
#13 interdiff-5-13.txt4.69 KBwillzyx
#6 shortcuts_names_for-2478907-5.patch2.17 KBwillzyx
#6 shortcuts_names_for-2478907-5-test-only.patch1.7 KBwillzyx
#1 shortcuts_names_for-2478907-1.patch2.09 KBwillzyx
#1 shortcuts_names_for-2478907-1-test-only.patch1.62 KBwillzyx
shortcut-2.jpg71.87 KBwillzyx
shortcut-1.jpg59.07 KBwillzyx

Comments

willzyx’s picture

willzyx commented
StatusFileSize
new shortcuts_names_for-2478907-1-test-only.patch1.62 KB
new shortcuts_names_for-2478907-1.patch2.09 KB
willzyx’s picture

willzyx commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1: shortcuts_names_for-2478907-1.patch, failed testing.

The last submitted patch, 1: shortcuts_names_for-2478907-1-test-only.patch, failed testing.

willzyx’s picture

willzyx commented
Title: Shortcuts names for node view routes are not properly escaped (using quick link) » Shortcuts names for node view routes are not properly sanitized (using quick link)
Issue summary: View changes

sorry.. I didn't realize that to make this patch work is necessary that the changes in #2478151: Shortcuts to pages generated by views are not recognized as added to the shortcutset and are being added multiple times are committed.

willzyx’s picture

willzyx commented
Status: Needs work » Needs review
StatusFileSize
new shortcuts_names_for-2478907-5-test-only.patch1.7 KB
new shortcuts_names_for-2478907-5.patch2.17 KB

rerolled after #2478151: Shortcuts to pages generated by views are not recognized as added to the shortcutset and are being added multiple times

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented 
+++ b/core/modules/shortcut/shortcut.module
@@ -311,7 +311,7 @@ function shortcut_preprocess_page(&$variables) {
+      'name' => trim(strip_tags($variables['title'])),

Why not just use check_plain?

willzyx’s picture

willzyx commented

because check_plain doesn't remove markup inserted by the quick edit module (see IS)... but we can probably find a better solution
and also for semplicity we should create a function (::assertShortcutQuickLink()?) for test quick link

The last submitted patch, 6: shortcuts_names_for-2478907-5-test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 6: shortcuts_names_for-2478907-5.patch, failed testing.

Status: Needs work » Needs review

willzyx queued 6: shortcuts_names_for-2478907-5.patch for re-testing.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs review » Needs work

+1 to #8 so NW

willzyx’s picture

willzyx commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-5-13.txt4.69 KB
new shortcuts_names_for-2478907-13.patch4.37 KB

Added ::assertShortcutQuickLink() util method.
Looking at #5 'name' => trim(strip_tags($variables['title'])) is not a clean solution. We could use title_resolver to retrieve the original title of the page.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

Oh much better now. Let's wait for the tests. Can you please upload the new after screenshot?

Status: Needs review » Needs work

The last submitted patch, 13: shortcuts_names_for-2478907-13.patch, failed testing.

willzyx’s picture

willzyx commented

This is weird.. it seems title_resolver not work at all for pages generated by views.
@jibran should we open another issue for this?

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/modules/shortcut/shortcut.module
@@ -309,9 +309,14 @@ function shortcut_preprocess_page(&$variables) {
+    // Use the route's title instead of $variables['title'] since
+    // $variables['title'] may have been changed and may contains extraneous
+    // markup.
+    $title = \Drupal::service('title_resolver')->getTitle(\Drupal::request(), $route_match->getRouteObject());
+
     $query = array(
...
+      'name' => SafeMarkup::checkPlain($title),

but don't we want to use the actual title which was part of the page? the docs doesn't make that parts clear for me, at least.

willzyx’s picture

willzyx commented

for now the problem related to the solution used in patch seems to be that title_resolver not work at all for pages generated by views

but don't we want to use the actual title which was part of the page?

dawehner look at the IS. If we use $variables['title'] for node view routes, the markup created by quickedit that wraps title became part of the shortcut name and this is wrong. Of course documentation can be improved, suggestions are welcome

willzyx’s picture

willzyx commented

Looking at #2359901: Discourage $main_content['#title'] in favor of route titles and title callbacks and #2264043: Add back a simple page title getter the title_resolver approach is not applicable with the current API.
The only alternative I see feasible is use strip_tags but I do not think is the right approach
Any ideas?

willzyx’s picture

willzyx commented
Status: Needs work » Needs review
StatusFileSize
new shortcuts_names_for-2478907-20.patch4.79 KB
new interdiff.txt1005 bytes

Ended up with a mixed approach: Use title from title_resolver when available $variables['title'] otherwise.. don't think it is the optimal solution

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Issue tags: +SafeMarkup

Status: Needs review » Needs work

The last submitted patch, 20: shortcuts_names_for-2478907-20.patch, failed testing.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Issue tags: +Needs reroll
lokapujya’s picture

lokapujya
Primary language English
Location Boston
commented
Status: Needs work » Needs review
StatusFileSize
new 2478907-24.patch4.94 KB

Removed the SafeMarkup::checkPlain() because it's in a preprocess function.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Needs work

I think this should probably just use strip_tags() on $variables['title']?

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs work » Closed (duplicate)
Issue tags: -Needs reroll

Moved tests to #2672668: Page title with HTML tags shows HTML tags in shortcut label in the shortcut toolbar tray.