🇫🇷France @prudloff

The tests have to change (well at least the expected values) because attribute values that were not sanitized before now have their protocol removed.
However, I adjusted the MR a bit to limit the changes in tests.

I rebased the MR.

Is this still useful? Do we still use libraries that support these headers?

Do we have a standardized way to add JS checks to /admin/reports/status?
I think this page only contains server-side checks.

I'm not sure this can be fixed in CommentDefaultFormatter.

If ✨ Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter() Needs work adds a generic system to alter access in entity queries, then custom code could alter access in comment queries and it would hopefully apply everywhere including in CommentStorage::loadThread().

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

It looks like this was fixed: https://git.drupalcode.org/project/gtm/-/commit/45060a0246f0159e8ffcec40...

We could parse the string as HTML and use textContent property from DOMNode and hope it does things correctly with html5.

I did a quick test like this:

$dom = \Drupal\Component\Utility\Html::load('<p>Giraffes.</p><p>Wombats.</p>'); echo $dom->textContent;

But it seems it never adds any space.

A more robust solution would be to use the html2text library but it would be a bit overkill to add a new dependency just for this.

Fixed a typo

prudloff → created an issue.

greggles → credited prudloff → .

greggles → credited prudloff → .

No answer from maintainer so moving to project ownership queue.

I think the bot tested the patch instead of the MR.
The MR does not have conflicts.

We don't need to duplicate the drupal/core requirement in composer.json, drupal.org generates it based on core_version_requirement when creating the Composer package.

prudloff → created an issue.

prudloff → created an issue.

prudloff → created an issue.

danrod → credited prudloff → .

This looks like a duplicate of #3098473: Add a dependency on migrate_tools for logging, --sync support → .

prudloff → created an issue.

prudloff → created an issue.

Sorry, I always forget that createFromRenderArray() only takes the root #cache and does not bubble lower cache metadata.

We can probably use ViewExecutable::getCacheTags() for cache tags but we could also need to get cache contexts and max age from the rendered view.
The views_ajax_get module uses createFromRenderArray($view->element), I wonder if that's enough.

@berdir you are right, we should also add cache contexts for query parameters that are explicitly used in the controller.

The bot says the patch does not apply but I don't see any conflict on the MR.
I rebased to see if it works better.

I rebased to make the bot happy.

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

I rebased the MR.

prudloff → created an issue.

prudloff → created an issue.

I think the MIT license is compatible with GPL 2: https://www.drupal.org/node/1475972#gplv2-compatible-licenses →

However, it is a bad practice to duplicate library code so we should still try to remove it.

I added logging if setExpires() fails.

And I stopped using the page cache max age setting as @berdir suggested.

I fixed the conflict (it was caused by 🐛 Pagination not working correctly in AJAX view with exposed filters Needs review ).

I also noticed we were mixing two ways to remove parameters from the query (a regex and parsing it as an array). I moved everything to the new way.

I tried reverting the camel case changes locally but it makes yarn run lint:core-js-passing complain:

/media/pierre/Projets/workspace/drupal-core/core/modules/views/js/ajax_view.js
  176:20  error  Identifier '$exposed_form' is not in camel case  camelcase
  182:5   error  Insert `··`                                      prettier/prettier
  182:5   error  Identifier '$exposed_form' is not in camel case  camelcase

I merged this in ✨ Enable bookmarking of AJAX views Needs work and I believe the failing PaginationAJAXTest shows the new regex might cut query parameters that contain "page".
For example '?items_per_page=gezgez&foo=bar'.replace(/q=[^&]+&?|page=[^&]+&?|&?render=[^&]+/, '') will return "?items_per_foo=bar".
Should I open a followup?

I updated the summary but now the MR has a conflict.

I updated the summary.

I added a test.

prudloff → created an issue.

I answered the questions.
If this needs fixing, we should probably fix it in JsonApiResource as well?

prudloff → created an issue.

Failing AssetAggregationAcrossPagesTest seems unrelated.

Would that be a better title?

See the comments in the MR

I don't see any comment on the MR.

I created a MR from the patch to make this easier to review.
phpcs reports a problem: https://git.drupalcode.org/project/views_node_access_filter/-/jobs/4764202

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

I don't think it is possible.
What would be your use case here?

Sorry I did not see your MR.
This is fixed: https://git.drupalcode.org/project/views_node_access_filter/-/commit/469...

prudloff → created an issue.

I created a followup issue for the phpstan error (because it requires refactoring the code): 📌 \Drupal calls should be avoided in classes Active

prudloff → created an issue.

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

prudloff → created an issue.

I just noticed Drupal does not require the intl extension.
NoSuspiciousCharacters will throw an exception if this extension is not available.

We could either:

• Require the intl extension
• Skip this constraint if the extension is not available

I agree we should try to avoid _access: 'TRUE' but this route does not return anything sensitive so I am not sure why and how we should protect it.

prudloff → created an issue.

prudloff → created an issue.

Marking as postponed until ✨ Register symfony's mime guessers if they are supported Needs work is fixed.
Then we can see if the solution implemented by the other issue is enough or if it is still needs hardening.

Not sure about the automatic extension change tho.

I agree, we could simply refuse the file if the detected content type does not match an allowed extension.

I did a quick manual test and I think this is still a concern.

I rebased the patch.
I'm not sure the failing AssetAggregationAcrossPagesTest is related?

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

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

Is this still a concern?
I think the issue summary would need to be updated to explain why this should be removed.
Also I guess the first step would be to simply deprecate it?

Added a basic implementation.
The main caveat is that it heavily depends on the languages activated on the website (because that's the way NoSuspiciousCharacters and Spoofchecker work).
If we find this too restrictive, we could also use NoSuspiciousCharacters without restricting the allowed locales, it would still be useful to detect mixed character sets and invisible characters.

I'm not sure the failing AssetAggregationAcrossPagesTest is related?

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

I suppose we should merge 📌 SessionCacheContext class should implement CacheContextInterface Needs work first then this issue can focus on adding the 0 max age fix when there is no session.

prudloff → changed the visibility of the branch 3442009-oop-hooks-using to hidden.

prudloff → changed the visibility of the branch drupal-3442009 to hidden.

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

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

The PHP intl extension provides a Spoofchecker::areConfusable() method that could be used for this but comparing the news username to every existing user would probably be expensive.

Symfony also provides a NoSuspiciousCharacters characters constraint (which uses Spoofchecker::isSuspicious() internally): https://symfony.com/doc/current/reference/constraints/NoSuspiciousCharac...
This is probably a good solution but it requires setting the expected locales (having Cyrillic letters in usernames on a Russian websites would not be suspicious for example). I supposed we could configure it using the enabled locale on the website.

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

I agree this is important and would help prevent some XSS vulnerabilities.
I rebased the latest patch and adjusted some things to make tests pass.

However, I see two potential problems with the solution:

• It would require maintaining a long list of attributes in which protocols should not be filtered (see 🐛 XSS attribute filtering is inconsistent and strips valid attributes Needs work ).
  This is an existing problem with the XSS filter, but filtering attributes everywhere makes it more visible.
• Using MarkupInterface to explicitly bypass the filter seems a bit weird semantically, because an attribute value is not really markup.
  (When this was proposed the interface was still called SafeStringInterface.)

Also filtering the style attribute would break too many places in core where it is used (because every CSS rule contains the : character) and I don't think it is the job stripDangerousProtocols() to remove dangerous URLs in CSS.
Removing dangerous CSS could be implemented later in ✨ Allow inline style to certain html elements despite of "Limit allowed HTML tags and correct faulty HTML" filter turned on Active .

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

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

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

🐛 Allow explicit bubbling of cacheability metadata inside Twig template (when accessing data from instead of rendering render arrays) Needs work is a similar discussion about cache metadata.

#2579941: Let Dynamic Page Cache warn developers when a controller's render array does not have #cache set → was closed without implementing anything.
So I guess it would make sense to close this as well?

I see the Symfony issues have been closed as wontfix.
Should we close this as well?

Renamed to better correspond to the current issue summary

The link containing the explanation in the IS is broken (but it can be found on the Internet Archive: https://web.archive.org/web/20140927053616/http://palizine.plynt.com/iss...).

I did a quick test and I confirm the problem still exists.

Instead of the redirect solution, I think we could implement a JavaScript solution:

    if ( window.history.replaceState ) {
        // This makes the next reload use a GET request.
        window.history.replaceState( null, null, window.location.href );
    }

Of course this would only work for users with JS enabled, but since this is a security hardening it might be acceptable.

I refactored the tests to avoid avoid having methods that are only called once.

We could have a look at how WordPress handles this: https://developer.wordpress.org/reference/functions/wp_check_filetype_an...
They maintain a list of file extensions and corresponding MIME types. (This list can be altered with hooks.)
When a file is uploaded, its MIME type is detected with fileinfo and WP checks if this type is allowed in the current context. If there is mismatch between the type and the extension, the file is renamed to use the proper extension for the detected type.

So I guess we could do something similar: use FileinfoMimeTypeGuesser to check the MIME type and then use our MIME type mapping to check if it corresponds to an allowed file extension.

We also got bitten by this on a website where bot requests expanded massively the size of the SQL database.
The database_cache_max_rows setting helps with this but it is not enforced in real time.

Our current solution to this is :

• switch to a Redis cache and set a memory limit
• add rate limit on the server

I agree this should be improved.
We handle this on our websites by restricting access to install.php in Apache config, but a solution that works for everyone would be nice.

If I understand how this attack works correctly, I think setting SameSite=Lax or Strict on cookies mitigates BREACH attacks.
I could not find a lot of information about this but it is mentioned here: https://www.sjoerdlangkemper.nl/2016/11/07/current-state-of-breach-attack/
Since 📌 Set SameSite on session cookies Fixed , Drupal now uses SameSite=Lax by default so we might not need a specific protection against BREACH anymore.

Also FWIW, here is what we implemented on our servers: we don't compress responses that have Cache-Control: private.
The reasoning is that:

• A response that contains sensitive data should already have Cache-Control: private (because we don't want our reverse proxy to cache it).
• This basically only disables compression for logged-in users or some very specific uncacheable pages which is an acceptable tradeoff for us.

But it would not be a good universal solution, for example if most of your users are logged-in.

Maybe a duplicate of 💬 Page cache isn't invalidated Needs review ?

prudloff → created an issue.

I added a basic description of user interface changes.

@berdir I understand your point but I think people using cache_control_override often use a reverse proxy and a high max age setting.

However I agree the patch should not use the cache.page.max_age setting from core.
Since #3003716: Coerce bubbled max-age to a floor or ceiling → , cache_control_override has a specific max age setting different from the core one and the patch should use that.