#Security improvements

@prudloff I think the idea was an Ajax or image request that sets something in key/value etc. which can then be checked the next time the page is visited.

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.

It would be so nice if the tests didn’t change. Are the test changes really necessary? Adding more expected assertions is fine, but changing the provider values reduces confidence in the change.

Thanks for picking this up, it's been sitting for a while.

I am using Drupal 8.9

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.

FWIW by default Wordpress removes a load of "special" characters from filenames:

https://github.com/WordPress/wordpress-develop/blob/6.7.2/src/wp-include...

function sanitize_file_name( $filename ) {

...

	$special_chars = array( '?', '[', ']', '/', '\\', '=', '<', '>', ':', ';', ',', "'", '"', '&', '$', '#', '*', '(', ')', '|', '~', '`', '!', '{', '}', '%', '+', '’', '«', '»', '”', '“', chr( 0 ) );

...

	$filename = str_replace( $special_chars, '', $filename );

Joomla seems to have decided to let the underlying OS decide what it will and will not allow in filenames :shrug:

https://github.com/joomla/joomla-cms/issues/33214

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().

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

What is the appropriate way now to throw a BadRequestHttpException that returns a themed (user-friendly) error page? We encountered this recently in the course of one of our custom modules throwing this exception only to find that we now get a blank page, whereas before we could present a themed error page (i.e., for the system.4xx route).

Having said that, I went to look for a definitive list of "dangerous characters" and ended up with quite a lot of "it depends".

This is not definitive, but e.g. https://stackoverflow.com/questions/15783701/which-characters-need-to-be...

There are some quite good lists using e.g. the shell escaping option that printf (and apparently ls) offer - e.g.

No, character % does not need to be escaped
No, character + does not need to be escaped
No, character - does not need to be escaped
No, character . does not need to be escaped
No, character / does not need to be escaped
No, character : does not need to be escaped
No, character = does not need to be escaped
No, character @ does not need to be escaped
No, character _ does not need to be escaped
Yes, character   needs to be escaped
Yes, character ! needs to be escaped
Yes, character " needs to be escaped
Yes, character # needs to be escaped
Yes, character $ needs to be escaped
Yes, character & needs to be escaped
Yes, character ' needs to be escaped
Yes, character ( needs to be escaped
Yes, character ) needs to be escaped
Yes, character * needs to be escaped
Yes, character , needs to be escaped
Yes, character ; needs to be escaped
Yes, character < needs to be escaped
Yes, character > needs to be escaped
Yes, character ? needs to be escaped
Yes, character [ needs to be escaped
Yes, character \ needs to be escaped
Yes, character ] needs to be escaped
Yes, character ^ needs to be escaped
Yes, character ` needs to be escaped
Yes, character { needs to be escaped
Yes, character | needs to be escaped
Yes, character } needs to be escaped

I feel like most of those make sense; and there are a fair number of characters that would be allowed.

Whether this would be more or less confusing / user friendly than just stripping all punctuation though is perhaps a usability question.

Looks like the code mostly uses regex; I wonder whether the punct character class would be any use. It'd remove the "safe" characters from the list above which we may not want to do.

To be clear what I'm looking for here is something that would remove all the dangerous shell characters but not limit users to strict ASCII alphanum without e.g. accented letters etc..

@kim.pepper although the existing options would remove all of the characters I highlighted as a concern, they do more than that.

I wouldn't advocate stripping filenames down to only ASCII / alpha-numeric characters (by default or as a mandatory setting) because of how restrictive that is for different languages.

I'm proposing a fairly limited number of forbidden characters - those which have significance for the shell - which would be a subset of what would be stripped by any of the existing options IIUC?

Thank you for reporting. Using an access code is inherently less secure than a username/password. It is a compromise in security in favor of convenience, and there is a warning about this on the module page.

I certainly agree that storing passwords in plain text is inappropriate. However, access codes are typically 4-6 digits. If they were hashed, they could be cracked in under a second, given someone has access to the database. Therefore, in relation to this module, I don't perceive hashing as a major security enhancement.

I'd recommend that we enforce the removal (/ substitution) of the most dangerous characters.

For removal of spaces, that'd be ok as a default (would people really want to turn it off?!)

All of these are available as optional settings. Are you saying we should default to safer settings, or enforce them?

Removing outdated beta phase copy and making it clear in the issue summary which issue is making this postponed.

Code comment improved.

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

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.