Problem/Motivation

This is a public followup for SA-CORE-2025-004. Link field attributes were not properly sanitized, which could lead to XSS. This issue will add the test coverage used in the private security issue.

Proposed resolution

Add tests from private issue: https://security.drupal.org/node/169733 (restricted access)

Related private MR: https://git.drupalcode.org/security/drupal-security/-/merge_requests/10

Remaining tasks

NR.

Issue fork drupal-3530149

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

xjm created an issue. See original summary.

xjm credited benjifisher.

xjm credited bramdriesen.

xjm credited larowlan.

xjm credited longwave.

xjm credited mcdruid.

xjm’s picture

xjm
she/her
Primary language English
commented

Adding credits for folks who contributed to the development and review of the tests on the private issue.

xjm opened merge request !12377

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Active » Needs review
xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Needs work

Looks like the string typehinting worked. However, I found a newer version of the tests with additional test coverage in a private repo that was not posted to the private issue as directly. Will update the MR shortly.

xjm credited alexpott.

xjm credited catch.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs work » Needs review

Adding credits from additional reviewers on the private MR.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes
xjm’s picture

xjm
she/her
Primary language English
commented

The things are green. I gave the tests a quick once-over and they look in order to me.

xjm’s picture

xjm
she/her
Primary language English
commented

.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +Security improvements

Forgot the tag.

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

smustgrave opened merge request !12416

smustgrave’s picture

smustgrave commented

Opened an MR with 2024-004 revert

Unit test fails with

  Line   core/modules/link/tests/src/Unit/AttributeXssTest.php                
 ------ --------------------------------------------------------------------- 
  16     @covers value \Drupal\link\AttributeXss references an invalid class  
         or function.                                                         
         🪪 phpunit.covers                                                    
  24     Call to static method sanitizeAttributes() on an unknown class       
         Drupal\link\AttributeXss.                                            
         🪪 class.notFound                                                    
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols

kernel tests don't run is it needed to opened a 3rd MR without the unit test?

smustgrave changed the visibility of the branch 3530149-revert-changes-2024-004 to hidden.

smustgrave’s picture

smustgrave commented

Deleted the Unit test from my MR since we saw it fail, lets see if the kernel does now.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

kernel failed

    
    FF                                                                  2 / 2 (100%)
    
    Time: 00:04.448, Memory: 8.00 MB
    
    Link Formatter (Drupal\Tests\link\Kernel\LinkFormatter)
     ✘ Link formatter with default·formatter
       ┐
       ├ Failed asserting that 'Hello world' [ASCII](length: 101) contains "" [ASCII](length: 49).
       │
       │ /builds/issue/drupal-3530149/core/modules/link/tests/src/Kernel/LinkFormatterTest.php:118
       ┴
     ✘ Link formatter with separate·link·text·and·URL
       ┐
       ├ Failed asserting that 'Hello world\n                                     
       ├ https://www.drupal.org/\n
       ├ ' [ASCII](length: 126) contains "" [ASCII](length: 49).
       │
       │ /builds/issue/drupal-3530149/core/modules/link/tests/src/Kernel/LinkFormatterTest.php:118

MenuLinkTest failed with

      ┐
       ├ Failed asserting that 'Link test' [ASCII](length: 99) contains "" [ASCII](length: 49).
       │
       │ /builds/issue/drupal-3530149/core/modules/menu_link_content/tests/src/Kernel/MenuLinksTest.php:505

So believe this shows the test coverage is good

xjm’s picture

xjm
she/her
Primary language English
commented

Saving credits. Nice work on the tests.

  • catch committed 03421184 on 11.2.x 
    Issue #3530149 by xjm, smustgrave, larowlan, bramdriesen, benjifisher,...

  • catch committed 3f6e2356 on 11.x 
    Issue #3530149 by xjm, smustgrave, larowlan, bramdriesen, benjifisher,...

  • catch committed 7e9c1872 on 10.6.x 
    Issue #3530149 by xjm, smustgrave, larowlan, bramdriesen, benjifisher,...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 10.6.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x and cherry-picked to 11.2.x, since this cherry-picked cleanly also cherry-picked back to 10.6.x - it might help if we need to adjust this again and backport that, thanks!

catch closed merge request !12377

smustgrave closed merge request !12416

xjm’s picture

xjm
she/her
Primary language English
commented
Version: 10.6.x-dev » 10.5.x-dev

I think this also belongs in 10.5.x, but after having had an oopsidaisy with the other issue, will double-check the pipeline first.

xjm’s picture

xjm
she/her
Primary language English
commented
Version: 10.5.x-dev » 10.6.x-dev
Status: Fixed » Patch (to be ported)

I think this might be broken on 10.6.x, so let's try a revert and a 10.6.x MR.

  • xjm committed 143aeb1d on 10.6.x 
    Revert "Issue #3530149 by xjm, smustgrave, larowlan, bramdriesen,...

xjm opened merge request !12558

xjm’s picture

xjm
she/her
Primary language English
commented

Apparently it was unrelated. So let's try again with the backport to 10.6.x and 10.5.x.

xjm’s picture

xjm
she/her
Primary language English
commented
Version: 10.6.x-dev » 10.5.x-dev

xjm opened merge request !12559

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Patch (to be ported) » Reviewed & tested by the community

Both backports pass, so it was still lingering issues from the other bad backports. So, committing these backports now.

  • xjm committed 88573a49 on 10.6.x 
    Issue #3530149 by xjm, smustgrave, catch, benjifisher, bramdriesen,...

  • xjm committed 0ee07af8 on 10.5.x 
    Issue #3530149 by xjm, smustgrave, catch, benjifisher, bramdriesen,...
xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Back to fixed, this time providing test coverage on all four branches. Thanks!

xjm closed merge request !12559

xjm closed merge request !12558

Status: Fixed » Closed (fixed)

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