Problem/Motivation

    Editor Security (Drupal\Tests\editor\Functional\EditorSecurity)
     ✔ Initial security
     ✔ Switching security
     ✘ Editor xss filter override
       ┐
       ├ Behat\Mink\Exception\ExpectationException: The field "edit-body-0-value" value is "Hello, Dumbo Octopus!alert(0)", but "Hello, Dumbo Octopus!alert(0)" expected.
       │
       │ /builds/issue/drupal-3069442/vendor/behat/mink/src/WebAssert.php:888
       │ /builds/issue/drupal-3069442/vendor/behat/mink/src/WebAssert.php:781
       │ /builds/issue/drupal-3069442/core/tests/Drupal/Tests/WebAssert.php:991
       │ /builds/issue/drupal-3069442/core/modules/editor/tests/src/Functional/EditorSecurityTest.php:454
       ┴
    
    FAILURES!
    Tests: 3, Assertions: 131, Failures: 1.

Steps to reproduce

Example: https://git.drupalcode.org/issue/drupal-3069442/-/jobs/3468066#L1429

Proposed resolution

Replace calls to State with calls to KeyValue.

The root cause of this issue is discussed in #3496257: Race conditions in CacheCollector/State (again)

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3496405

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

spokje created an issue. See original summary.

spokje opened merge request !10728

spokje opened merge request !10729

spokje opened merge request !10731

spokje’s picture

spokje
Primary language Dutch
commented
Status: Active » Needs review

MR !10728 (which is basically \Drupal\Tests\editor\Functional\EditorSecurityTest::testEditorXssFilterOverride and some pipeline changes to make it, and only it, run 7500 times) gives us 325 fails out of the 10000runs.

MR !10729 shows that changing \Drupal::state()->set()/get() to Drupal::keyValue()->set()/get(); passes a 10000 times run without errors.

MR !10731 contains the changes we want committed after review.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Reviewed MR 10731 and update to keyValue makes sense

MR !10729 shows that changing \Drupal::state()->set()/get() to Drupal::keyValue()->set()/get(); passes a 10000 times run without errors.

Awesome!

  • catch committed 3e2ec19f on 11.x 
    Issue #3496405 by spokje: [random test failure] EditorSecurityTest::...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 11.0.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed/pushed to 11.x and cherry-picked to 11.1.x, thanks!

Looks like this needs a backport for 11.0.x/10.5.x.

catch closed merge request !10731

  • catch committed 658fcf5e on 11.1.x 
    Issue #3496405 by spokje: [random test failure] EditorSecurityTest::...

spokje closed merge request !10729

spokje closed merge request !10728

spokje opened merge request !10758

spokje’s picture

spokje
Primary language Dutch
commented

Looks like HEAD broken on 11.0.x: https://www.drupal.org/project/drupal/issues/3490710#comment-15922094, so we're stalled here.

spokje’s picture

spokje
Primary language Dutch
commented
Status: Patch (to be ported) » Needs review

HEAD fixed, MR for 11.0.x is up

  • catch committed 164509d4 on 10.3.x 
    Issue #3496405 by spokje: [random test failure] EditorSecurityTest::...

  • catch committed 1e789177 on 10.4.x 
    Issue #3496405 by spokje: [random test failure] EditorSecurityTest::...

  • catch committed d1fadb33 on 10.5.x 
    Issue #3496405 by spokje: [random test failure] EditorSecurityTest::...

  • catch committed 6e3f47d7 on 11.0.x 
    Issue #3496405 by spokje: [random test failure] EditorSecurityTest::...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.0.x-dev » 10.3.x-dev
Status: Needs review » Fixed

Committed/pushed to 10.5.x/10.4.x/11.0.x/10.3.x, thanks!

catch closed merge request !10758

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Replace calls to State with calls to KeyValue.

The root cause of this issue is discussed in #3496257: Race conditions in CacheCollector/State (again)

🤯

As a fellow maintainer of the editor module, I reviewed this commit. And I don't understand what the change is about. I'll follow that issue then 😳

Status: Fixed » Closed (fixed)

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