Problem/Motivation

The Drupal Security Team and other contributrors wrote some tests for the core issue mitigated in/by https://www.drupal.org/sa-core-2020-009

Now that a few months have passed since that release, we can add the tests to core in public.

Steps to reproduce

n/a

Proposed resolution

Add tests for SA-CORE-2020-009

Remaining tasks

Patch, review, commit...

User interface changes

n/a

API changes

n/a

Data model changes

n/a

Release notes snippet

n/a

CommentFileSizeAuthor
#8 3183301-8_d9.patch2.97 KBmcdruid
#8 interdiff-3183301-6-8_d9.txt485 bytesmcdruid
#6 3183301-6_d8.patch2.95 KBmcdruid
#6 interdiff-3183301-4-6_d8.txt773 bytesmcdruid
#6 3183301-6_d9.patch2.94 KBmcdruid
#6 interdiff-3183301-4-6_d9.txt773 bytesmcdruid
#4 3183301-4_d8.patch2.95 KBmcdruid
#4 interdiff-3183301-2-4_d8.txt1.05 KBmcdruid
#4 3183301-4_d9.patch2.94 KBmcdruid
#4 interdiff-3183301-2-4_d9.txt1.05 KBmcdruid
#2 3183301-2_d9.patch2.9 KBmcdruid
#2 3183301-2_d8.patch2.91 KBmcdruid
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mcdruid created an issue. See original summary.

mcdruid’s picture

mcdruid
Location 🇬🇧🇪🇺
CreditAttribution: mcdruid as a volunteer and at Acquia commented
Issue summary: View changes
Status: Active » Needs review
FileSize
3183301-2_d8.patch2.91 KB
3183301-2_d9.patch2.9 KB

D8 an D9 tests.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented 
+++ b/core/tests/Drupal/KernelTests/Core/Form/FormActionXssTest.php
@@ -0,0 +1,93 @@
+    $elements = $this->xpath('//form/@*[name()="action" or name()="injected"]');
+    $action = (string) $elements[0];
+    $injected = isset($elements[1]) ? (string) $elements[1] : FALSE;

Is it safe to rely on the attribute ordering like this? I suppose the action test will fail if it doesn't match, but something like this seems more readable:

    $elements = $this->xpath('//form');
    $action = $elements[0]->getAttribute('action');
    $injected = $elements[0]->getAttribute('injected');
mcdruid’s picture

mcdruid
Location 🇬🇧🇪🇺
CreditAttribution: mcdruid at Acquia commented
FileSize
interdiff-3183301-2-4_d9.txt1.05 KB
3183301-4_d9.patch2.94 KB
interdiff-3183301-2-4_d8.txt1.05 KB
3183301-4_d8.patch2.95 KB

Good idea, only problem is:

1) Drupal\KernelTests\Core\Form\FormActionXssTest::testFormActionXss
Error: Call to undefined method SimpleXMLElement::getAttribute()

We can do something similar without the getter.

mcdruid’s picture

mcdruid
Location 🇬🇧🇪🇺
CreditAttribution: mcdruid at Acquia commented

Oops typo:

    $injected = isset($elements[0]['injected']) ? (string) $elements[0]['injected']: FALSE;

 87 | ERROR | [x] Expected 1 space before ":"; 0 found

Could be fixed on commit or I can do new patches. I'll wait for test results first.

mcdruid’s picture

mcdruid
Location 🇬🇧🇪🇺
CreditAttribution: mcdruid at Acquia commented
FileSize
interdiff-3183301-4-6_d9.txt773 bytes
3183301-6_d9.patch2.94 KB
interdiff-3183301-4-6_d8.txt773 bytes
3183301-6_d8.patch2.95 KB

New patches it is then...

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

Oh, I'm so used to xpath in functional tests I forgot this was only SimpleXML in kernel tests.

mcdruid’s picture

mcdruid
Location 🇬🇧🇪🇺
CreditAttribution: mcdruid at Acquia commented
FileSize
interdiff-3183301-6-8_d9.txt485 bytes
3183301-8_d9.patch2.97 KB

Added cspell:ignore for D9.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Reviewed & tested by the community

Looks good to me now. Also confirmed that this test fails as expected on 9.0.5.

mcdruid credited -nrzr-.

mcdruid credited janusman.

mcdruid credited marcaddeo.

mcdruid credited markwittens.

mcdruid credited nathandentzau.

mcdruid’s picture

mcdruid
Location 🇬🇧🇪🇺
CreditAttribution: mcdruid at Acquia commented

Adding credit from the s.d.o issue pt.1

mcdruid credited David_Rothstein.

mcdruid credited Heine.

mcdruid credited Wim Leers.

mcdruid credited larowlan.

mcdruid credited pandaski.

mcdruid credited tim.plunkett.

mcdruid credited vijaycs85.

mcdruid credited xjm.

mcdruid’s picture

mcdruid
Location 🇬🇧🇪🇺
CreditAttribution: mcdruid at Acquia commented

Adding credit from the s.d.o issue pt.2

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Version: 9.2.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Fixed

I tested #8 locally on 8.9.x and it passed. Therefore I'm backporting that one to 8.9.x because the protected $modules is good and the cspell comment doesn't matter and if we make future security related changes to this test later having exactly the same code makes things simpler.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

Committed and pushed 4f29fd7f6c to 9.2.x and 7bd7fee7e9 to 9.1.x and 77c02d1863 to 8.9.x. Thanks!

  • alexpott committed 4f29fd7 on 9.2.x 
    Issue #3183301 by mcdruid, longwave, markwittens, nathandentzau,...

  • alexpott committed 7bd7fee on 9.1.x 
    Issue #3183301 by mcdruid, longwave, markwittens, nathandentzau,...

  • alexpott committed 77c02d1 on 8.9.x 
    Issue #3183301 by mcdruid, longwave, markwittens, nathandentzau,...

Status: Fixed » Closed (fixed)

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