🇫🇷France @prudloff

greggles → credited prudloff → .

greggles → credited prudloff → .

poker10 → credited prudloff → .

The MR suits our need but I guess it could be a breaking change so we might want to add a setting for this.

prudloff → created an issue.

prudloff → created an issue.

prudloff → created an issue.

poker10 → credited prudloff → .

I guess using ~ would be an acceptable compromise.

Wouldn't it make sense to use a Composer constraint like ^1.20.1?
This way we don't have to update the module every time a new release of tarteaucitron is published.

prudloff → created an issue.

prudloff → created an issue.

We are using the patch on several projects and it seems to work correctly.

@smustgrave I can't see your comment on the MR?

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.