Lille
Account created on 11 April 2019, about 6 years ago
#

Merge Requests

More

Recent comments

🇫🇷France prudloff Lille

I think comments have been addressed.

🇫🇷France prudloff Lille

Removed patches to focus on the MR.

🇫🇷France prudloff Lille

You don't need to create a new issue in that case, you can update the MR branch or create a new MR.

🇫🇷France prudloff Lille

prudloff changed the visibility of the branch 3443679-use-elementisempty-to to hidden.

🇫🇷France prudloff Lille

1.2.1 is compatible with Drupal 11 so marking as fixed.

🇫🇷France prudloff Lille

Reopening as this is still a problem.

🇫🇷France prudloff Lille

I think there could be a valid concern here.
The description of the class does state that this should not be used in HTML contexts but the name of the method could imply that it could be used to transform HTML into harmless plain text.
I have seen this method misused in custom code.
I also would not be surprised if a Google search like "drupal convert HTML to plaintext" points users to this method.

Maybe the description of the method could make it clearer that it is dangerous in HTML contexts?

🇫🇷France prudloff Lille

prudloff changed the visibility of the branch 2842858-basic-auth-module to hidden.

🇫🇷France prudloff Lille

prudloff changed the visibility of the branch 10.3.x to hidden.

🇫🇷France prudloff Lille

prudloff changed the visibility of the branch 10.4.x to hidden.

🇫🇷France prudloff Lille

prudloff changed the visibility of the branch 2940879-dont-automatically-set to hidden.

🇫🇷France prudloff Lille

prudloff changed the visibility of the branch 11.x to hidden.

🇫🇷France prudloff Lille

prudloff changed the visibility of the branch 9.2.x to hidden.

🇫🇷France prudloff Lille

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

🇫🇷France prudloff Lille

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

🇫🇷France prudloff Lille

I added and updated the patch from the private issue.
People from the "Fixed by" section of https://www.drupal.org/sa-core-2024-002 should probably be credited.

🇫🇷France prudloff Lille

Oh right, I didn't see that email_login_otp.routing.yml already uses the OTPForm case.

🇫🇷France prudloff Lille

Renaming the class would be a breaking change.
I think it is less disruptive to rename the file to match the class.

🇫🇷France prudloff Lille

I tried to rewrite the summary.

🇫🇷France prudloff Lille

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

🇫🇷France prudloff Lille

As alexpott said we filter dangerous attributes so most tags that would be dangerous otherwise are not.
However we can't guarantee that we remove dangerous attributes for every possible tag (for example we didn't remove srcdoc attributes on iframe until 🐛 Remove srcdoc attributes in Xss::filter() Active ).

So in order to keep this simple and not duplicate the list of safe tags, I think we should display the warning when allowing tags that are not in filterAdmin().
This does not mean that any additional tag would be dangerous, but that it could be because our XSS filter might not remove some attribute that could be dangerous on this specific tag.

🇫🇷France prudloff Lille

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

🇫🇷France prudloff Lille

I rebased the latest patch and tests are passing but I did not test manually.

🇫🇷France prudloff Lille

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

🇫🇷France prudloff Lille

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

🇫🇷France prudloff Lille

Are you sure? I can still see the duplicate here: https://git.drupalcode.org/project/email_tfa/-/blob/2.0.x/src/Form/Email...

    $form['#cache'] = ['max-age' => 0];
    if (!$status) {
      return $form;
    }

    $form['#validate'][] = '::emailTfaLoginFormValidate';
    $form['#cache'] = ['max-age' => 0];
🇫🇷France prudloff Lille

prudloff changed the visibility of the branch 3503383-potential-xss-vulnerability to hidden.

🇫🇷France prudloff Lille

@sd123 this is out of the scope of this issue. Feel free to open a separate issue about this (if there isn't one already).

🇫🇷France prudloff Lille

prudloff changed the visibility of the branch 2844620-finishresponsesubscriber-need-warningerror to hidden.

🇫🇷France prudloff Lille

@nicxvan does your MR also solve the problem?
If so, it should have tests (you might be able to reuses the tests from the other MR).

🇫🇷France prudloff Lille

prudloff changed the visibility of the branch 2118743-twig-debug-output-d10 to hidden.

🇫🇷France prudloff Lille

I had to use a trim because the browser can send \r\n line breaks and PHP_EOL can be just \n depending on the OS. So it could leave dangling \r characters.

🇫🇷France prudloff Lille

prudloff changed the visibility of the branch 2.0.x to hidden.

🇫🇷France prudloff Lille

prudloff changed the visibility of the branch 1587536-js-aggregation-async-defer-attributes-10.1.x to hidden.

🇫🇷France prudloff Lille

this is where aggregate js/css checkboxes are

The aggregate setting is not the same as the gzip/compress setting (which as far as I can tell does not have a UI).

🇫🇷France prudloff Lille

The CR seems to contain some hallucinations:

  • The name of the new key is compress, not compression.
  • It talks about a setting on /admin/config/development/performance but I see no such setting.
  • The name of the update hook is incorrect.
🇫🇷France prudloff Lille

I updated the MR to use a single setting for both gzip and brotli (and future algorithms).
I also added an upgrade path.

We still need a change record I think.

🇫🇷France prudloff Lille

I rebased the latest patch and tests are green.
But I did some manual testing and I think escaping the options is still needed.

For example if I create a role named <b>foo</b> then browse to /admin/config/content/formats/manage/basic_html.
Without the patch:

With the patch:

I think we automatically sanitize #options by calling Xss::filter() on it (which removes dangerous markup) so not escaping the options would not be dangerous but role names are not really supposed to contain HTML so interpreting it here could be confusing for admins.

So I think this could be closed as wontfix.

@smustgrave I think #6 is about explaining a test failure, not steps to reproduce this issue.
The test failed because the previous patch removed escaping in Attachment.php and Feed.php which causes a breaking change.
(I did not keep changes to these files in the MR.)

🇫🇷France prudloff Lille

We no longer ship web.config: https://www.drupal.org/node/3440842
And according to #9 this can't be fixed in .htaccess.

So I guess we can't really do anything except document somewhere that we recommend disabling TraceEnable in Apache config?

Also the Apache doc (https://httpd.apache.org/docs/2.4/en/mod/core.html#traceenable) says this:

Despite claims to the contrary, enabling the TRACE method does not expose any security vulnerability in Apache httpd. The TRACE method is defined by the HTTP/1.1 specification and implementations are expected to support it.

So is this still a concern?
It would be nice to have a proof of concept.

🇫🇷France prudloff Lille

Rebased and fixed failing tests.

If we can achieve to have just: <input disabled> , then it can be a bit cleaner, but not sure if this can be achieved easily. Other option is to close this as Works as designed. Thanks!

Maybe something changed in core but I tested this and boolean attributes are displayed like you suggest.

🇫🇷France prudloff Lille

I had a look and only found this call that could easily be removed. But I might have missed some others.

I think several calls have been removed over the years ( #2569381: Drupal\views\Plugin\views\area\Result does an unnecessary XSS::adminFilter() for example).

@duaelfr I'm not sure this is related. I think there was some issue about being able to set the list of allowed tags in view templates but I can't find it.

🇫🇷France prudloff Lille

The module has a Drupal 11 compatible release so marking as fixed.

🇫🇷France prudloff Lille

The module has a Drupal 11 compatible release so marking as fixed.

🇫🇷France prudloff Lille

The module has a Drupal 11 compatible release so marking as fixed.

🇫🇷France prudloff Lille

The module has a Drupal 11 compatible release so marking as fixed.

🇫🇷France prudloff Lille

The module has a Drupal 11 compatible release so marking as fixed.

🇫🇷France prudloff Lille

prudloff changed the visibility of the branch 2061377-image-style to hidden.

🇫🇷France prudloff Lille

prudloff changed the visibility of the branch 2061377-image-style-9.4 to hidden.

🇫🇷France prudloff Lille

prudloff changed the visibility of the branch 2061377-image-style-10.0 to hidden.

🇫🇷France prudloff Lille

prudloff changed the visibility of the branch 2061377-image-style-9.5 to hidden.

🇫🇷France prudloff Lille

prudloff changed the visibility of the branch 11.x to hidden.

Production build 0.71.5 2024