🇦🇹Austria @klausi

klausi → created an issue.

1. config schema is missing, see https://www.drupal.org/docs/drupal-apis/configuration-api/configuration-... →
2. Is there a reason why the permission "administer bmc" is marked as "restrict access"? Can you do arbitrary code execution with it? I see the config parameters being use with Drupal attributes, so everything should be escaped correctly. If there is no arbitrary code execution risk you could remove "restrict access" from the permission.

Otherwise looks good to me.

Thanks for your contribution, Dmitry!

I updated your account so you can opt into security advisory coverage now.

Here are some recommended readings to help with excellent maintainership:

• Dries → ' post on Responsible maintainers
• Best practices for creating and maintaining projects →
• Maintaining a drupal.org project with Git →
• Commit messages - providing history and credit →
• Release naming conventions → .
• Helping maintainers in the issue queues →

You can find lots more contributors chatting on Slack → or IRC → in #drupal-contribute. So, come hang out and stay involved → !

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review → . I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

Hm, I probably did not find that sniff. Good to know that it exists, but with the fork of the upstream PHPCS sniff we cover all our requirements for Drupal, so I think it is fine.

1. mosparo_integration_uninstall(): this should not be necessary, uninstalling a module already clears all caches.
2. modules/mosparo_contact/config/mosparo_contact.schema.yml: this should be in the schema subfolder, right?

Otherwise looks good to me.

Thanks for your contribution, Matthias!

I updated your account so you can opt into security advisory coverage now.

Here are some recommended readings to help with excellent maintainership:

• Dries → ' post on Responsible maintainers
• Best practices for creating and maintaining projects →
• Maintaining a drupal.org project with Git →
• Commit messages - providing history and credit →
• Release naming conventions → .
• Helping maintainers in the issue queues →

You can find lots more contributors chatting on Slack → or IRC → in #drupal-contribute. So, come hang out and stay involved → !

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review → . I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

closing as outdated.

As the coding standards issue was closed I'm closing this as well.

Also works for this code:

/**
 * @file field_tools.module
 * Contains useful tools for working with fields.
 */

/**
 * Implements hook_help().
 */
function field_tools_help($route_name, \Drupal\Core\Routing\RouteMatchInterface $route_match, \Drupal\Core\Routing\RouteMatchInterface2 $route_match2) {
}

which is fixed to

/**
 * @file
 * Field_tools.module
 * Contains useful tools for working with fields.
 */

use Drupal\Core\Routing\RouteMatchInterface;
use Drupal\Core\Routing\RouteMatchInterface2;

/**
 * Implements hook_help().
 */
function field_tools_help($route_name, RouteMatchInterface $route_match, RouteMatchInterface2 $route_match2) {
}

So that also seems to be more working.

Please check what phpcs config and coder version your test run uses.

closing as outdated.

Closing as outdated.

1. JS files: wow the AI generated comments you added are really bad. They don't explain why the JS file exists and what its purpose is. They just explain badly what the code does without giving a good summary that makes sense to a human. Please only use AI generated text when it makes sense and actually explains something.
2. OauthLogin::$currentUser: doc data type is wrong. Actually all datatypes of all your class variables are wrong, they are not string.

But that are not application blockers.

Thanks for your contribution, Ahmad!

I updated your account so you can opt into security advisory coverage now.

Here are some recommended readings to help with excellent maintainership:

• Dries → ' post on Responsible maintainers
• Best practices for creating and maintaining projects →
• Maintaining a drupal.org project with Git →
• Commit messages - providing history and credit →
• Release naming conventions → .
• Helping maintainers in the issue queues →

You can find lots more contributors chatting on Slack → or IRC → in #drupal-contribute. So, come hang out and stay involved → !

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review → . I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

Thanks for your contribution!

1. JS files: please add file doc block comments at the top describing what the JS file does and why.
2. azure_oauth_sso_page_attachments(): you are adding your library to every single page on your Drupal site. Is that intentional? Or should it only be added on the login page?
3. azure_oauth_sso_custom_logout(): not every Drupal site uses language parameters in URLs. Are you sure the redirect URL is correct on sites without language in the URL?
4. class OauthLogin: member variables requestStack and oauthService are not defined, please add them.
5. config schema is missing, see https://www.drupal.org/docs/drupal-apis/configuration-api/configuration-... →
6. OauthLogin::loginUser(): this will not scale. You cannot load all user accounts of a site and then compare email addresses. This will break on sites with more than 10k users. You need to query for the specific email address instead.
7. getMyPhoto(): instead of hard-coding the image path you should get it from the user picture field settings. That way it is stored as if the user would have uploaded a picture. This also helps on sites that use the private file system to store user pictures.

I think the loading of all users on login is currently a blocker, as this would break any large drupal site.

1. config schema is missing, see https://www.drupal.org/docs/drupal-apis/configuration-api/configuration-... →
2. AdvancedSanitizeService::logger: doc block type is wrong

Otherwise looks good to me.

Thanks for your contribution, Ilya!

I updated your account so you can opt into security advisory coverage now.

Here are some recommended readings to help with excellent maintainership:

• Dries → ' post on Responsible maintainers
• Best practices for creating and maintaining projects →
• Maintaining a drupal.org project with Git →
• Commit messages - providing history and credit →
• Release naming conventions → .
• Helping maintainers in the issue queues →

You can find lots more contributors chatting on Slack → or IRC → in #drupal-contribute. So, come hang out and stay involved → !

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review → . I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

Hm, it looks like your account is already approved as git vetted user to make full projects. I could not find another project application under your username, so a drupal.org administrator has probably set that already for you?

Anyway, the code looks good.

Thanks for your contribution, Jeff!

Your account is already approved so you can opt into security advisory coverage.

Here are some recommended readings to help with excellent maintainership:

• Dries → ' post on Responsible maintainers
• Best practices for creating and maintaining projects →
• Maintaining a drupal.org project with Git →
• Commit messages - providing history and credit →
• Release naming conventions → .
• Helping maintainers in the issue queues →

You can find lots more contributors chatting on Slack → or IRC → in #drupal-contribute. So, come hang out and stay involved → !

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review → . I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

The fourth #D7Security newsletter is out! Watch me talk about D7Security in a recorded video https://gitlab.com/d7security/d7security/-/issues/2#note_1911714087

Thanks, can be merged after fixing the minor doc issue.

I added you as maintainer so you do this yourself :)

Thanks a lot for the explanation.

I would argue that your example is an indication of a code smell. Never name a class "Email", that is a very bad name on its own as it is very ambiguous. You should not solve bad naming by using aliases all over the place when your class is used.

Yes, that would mean repeating the namespace in the class name, but that is totally fine and much better. Don't optimize your fully qualified class name for shortness; it still should convey meaning on its own.

What would be a good example for the Email class name here? In my opinion: "Symfony\Component\Validator\Constraints\EmailConstraint", then "EmailConstraint" is much clearer that this is a validation constraint.

Same for serialization groups. Is "Groups" a good class name? Probably not. I like "SerializationGroups" much more.

Doing that consistently suddenly all your aliasing problems go away because you now you actually have good class names that mean something.

So much for my rant on this topic :-) I'm still open to support aliasing in Coder for attributes, since we probably can't convince the Symfony folks to use meaningful class names.

I looked into this and noticed that this was already fixed upstream, so I synced our sniff from PHPCS.

I also disabled the ScopeClosingBraceSniff for template files - it does not make sense to run them on templates, since the braces follow the HTML indentation there.

I added a test case to demonstrate that there is no error, not perfect as the sniff is now disabled for templates, but good enough.

Then I found some interesting behavior in Drupal core with multi-line function declarations, improved our handling to allow empty "{}" function bodies.

Merged https://github.com/pfrenssen/coder/pull/229 .

I think this is ok.

I see you don't have the git vetted user role, but you are on drupal.org for quite some time and have some credits.

Unfortunately attackers use open source libraries as maintainers to spread malware these days, so we have to vet people a bit before making them maintainers. Can you ask one of your colleagues from DevBranch to post here to confirm we can trust you? For example @bohart, who I know from some years ago.

Thank you for your patience!

thanks, the next step is to add a test case so that we can confirm that we are actually fixing something.

Can you add that to your PR?

Happy to hear it is working - we have also deployed it over our work codebases and are quite happy with the auto fixes.

Thanks! You probably fixed the missing comment now, but if I remove it and run the complete fixer then it does the right thing:

cd action_link
~/workspace/coder/vendor/bin/phpcbf --standard=Drupal --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml .

Where ~/workspace/coder/ is my Coder installation.

Check the documentation how to run Coder phpcs and phpcbf at https://www.drupal.org/docs/contributed-modules/code-review-module/php-c... →

Marking this support request as fixed.

VariableAnalysisSniff is now used from sirbrillig/phpcs-variable-analysis and I assume this is fixed.

Let me know if not!

Can you post the source code file or a link to it?

My immediate guess would be that you might run the Drupal core phpcs configuration with phpcbf? That might not work because some sniffs are disabled so the fixing is incomplete. Instead, please run the full Drupal Coder config for fixing files.

Merged the latter one.

Found a better solution by forking our own function declaration multi-line sniff in https://github.com/pfrenssen/coder/pull/226

pull request: https://github.com/pfrenssen/coder/pull/225

This change was not working fully, so I pushed a better follow-up that uses the default language.

This was implemented in Coder, only the trailing comma rule is missing for now. Opened ✨ Validate trailing comma in multi-line function declaration Active for that.

Created ✨ Validate trailing comma in multi-line function declaration Active .

klausi → created an issue.

Makes sense, I pushed the configuration change to validate the indentation of mulit-line function declaration.

Missing is the trailing comma validation for the last parameter, did not find anything in the upstream Squiz.Functions.MultiLineFunctionDeclaration sniff. Will open a new issue for that.

Thanks for reporting!

Exactly, it reports consistently that the closing brace needs to be where the opening brace currently is.

That does not matter when you run phpcbf it will fix everything correctly for you.

So I don't think we need to do anything here.

Setting to "needs work" to get some answers on my previous comments.

Thanks for reporting!

We don't need to throw out this check, we can modernize if from the upstream Squiz standard which already implemented closure support.

It was a bit of a riddle to figure out, but thanks to our test cases I got it working: https://github.com/pfrenssen/coder/pull/223

I had to disable one test that did even a bit more semantics checking, which should really be done by phpstan.

And merged!

Tests are failing with "PHP Fatal error: Call to undefined function rules_event_invocation_enabled() in /var/www/html/sites/all/modules/rules/tests/rules.test on line 2285"

Not sure how unit tests work in Drupal 7, maybe we need to include code or dependencies somehow?

Thanks, pull request looks good to me!

I don't see anything else missing here, so we can go ahead with this?

drunken monkey → credited klausi → .

Looks good, thanks!

Thank you all! Posted the slides of my D7security presentation here: https://klau.si/blog/d7security-drupal-austria-meetup/

poker10 → credited klausi → .

Yes please, https://www.conventionalcommits.org/en/v1.0.0/#summary is exactly the Angular git commit format I proposed 7 years ago in #2802947: [meta] Use the Git commit message format from AngularJS → . Happy to see they made a specification out of it!

Thanks Gregor! Not sure if I should set this to RTBC, as it is only D7.

Nice, I think it is ok that we are only testing this with a unit test. In PHP you cannot even produce a header() call with a multi-line header, so this is hard to test with a functional test.

Tagging for PHP 8.1

Thanks, looks good to me!

Yes, why not! I would assume there is already a sniff out there in another standard (Squiz or Slevomat maybe?) that would check for this.

Thanks for reporting! Found the problem: your method call does not follow the exact upper/lower case spelling of the method. I pushed a fix to ignore case sensitivity, same as PHP does.

Thanks for the fixes!

kontainer.create_media: ah I see, did not see the access check later in the service. I would recommend to untangle that and do the access checking explicitly on the routing layer, then there is a lower risk that this gets lost in a refactoring for example.

Generating URLs with CSRF token: it works like this, which you can use similarliy for your ajaxTurstedUrl

use Drupal\Core\Url;
$route_name = 'system.run_cron';
$url = Url::fromRoute($route_name);
print $url->toString();

Which will output /admin/reports/status/run-cron?token=QJoWTyS5Bcf8KP_gvhmjUwm-s1rlrvSqL3XHgVsxEKQ
Or do you need to CSRF token standalone for some reason? It is only every useful in combination with the path?

src/Authentication/Provider/KontainerAuth.php: hash_equals(): Caution: It is important to provide the user-supplied string as the second parameter, rather than the first. You are doing that wrong. But I think it is very hard to exploit this with a timing attack, so would consider this a security hardening and not a security blocker.

Otherwise looks good to me!

Thanks for your contribution, Domen!

I updated your account so you can opt into security advisory coverage now.

Here are some recommended readings to help with excellent maintainership:

• Dries → ' post on Responsible maintainers
• Best practices for creating and maintaining projects →
• Maintaining a drupal.org project with Git →
• Commit messages - providing history and credit →
• Release naming conventions → .
• Helping maintainers in the issue queues →

You can find lots more contributors chatting on Slack → or IRC → in #drupal-contribute. So, come hang out and stay involved → !

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review → . I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

jurgenhaas → credited klausi → .

Looks good, thanks!

Patch.

klausi → created an issue.

Adding credits.

Marking this as fixed as it was committed.

Marking this as fixed, please set to fixed when you commit patches and reference the issue number in the commit message.

Merged, thanks!

Merged, thanks!

Committed, thanks!

Looks good, thanks!

quietone → credited klausi → .

manual review:

1. rote kontainer.delete_media_types: this is vulnerable to CSRF exploits as this is a destructive operation. You need to protect the route with a CSRF token or confirmation form.
2. KontainerAuth::authenticate(): this is vulnerable to timing attacks. When comparing secret strings you need to use the hash_equals() function instead of "===".
3. KontainerReferenceItemWidget::formElement(): there should be a way to generate URLs with manually setting up the CSRF token. If you use the normal URL generator it should spit out the URL including the CSRF token, as Drupal knows that the route should be CSRF protected.
4. kontainer.create_media: This route is missing access control? What permission does the user need to create media? The CSRF token is not an access protection, it is a validation of user intent. Looks like an access bypass vulnerability.

Please fix the 3 security vulnerabilities and check your code for other similar issues. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

manual review:
config schema is missing, see https://www.drupal.org/docs/drupal-apis/configuration-api/configuration-... →

Otherwise looks good to me.

Thanks for your contribution, seeduardo!

I updated your account so you can opt into security advisory coverage now.

Here are some recommended readings to help with excellent maintainership:

• Dries → ' post on Responsible maintainers
• Best practices for creating and maintaining projects →
• Maintaining a drupal.org project with Git →
• Commit messages - providing history and credit →
• Release naming conventions → .
• Helping maintainers in the issue queues →

You can find lots more contributors chatting on Slack → or IRC → in #drupal-contribute. So, come hang out and stay involved → !

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review → . I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

manual review:

1. project page is too short. What does "Provides private entity fields functionality." mean? What does the module do? What is the use case?
2. field_extra_form_alter(): You load your config on every single form in Drupal. Instead, it would be better to only load the config after you checked that this is an entity form.
3. field_extra_schema(): I would not combine key of multiple properties and instead have a column for entity type and field name, to be better query-able in the future.
4. field_extra_schema(): a primary key is already an index, no need to define it as index again.
5. field_extra_entity_field_access(): Cache context information is missing. Probably all access results are cachePerUser(), right?
6. FieldExtraForm::buildForm(): Don't put class names in strings, use PHP ::class syntax

Otherwise looks good to me!

Thanks for your contribution, Viktor!

I updated your account so you can opt into security advisory coverage now.

Here are some recommended readings to help with excellent maintainership:

• Dries → ' post on Responsible maintainers
• Best practices for creating and maintaining projects →
• Maintaining a drupal.org project with Git →
• Commit messages - providing history and credit →
• Release naming conventions → .
• Helping maintainers in the issue queues →

You can find lots more contributors chatting on Slack → or IRC → in #drupal-contribute. So, come hang out and stay involved → !

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review → . I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

Thanks for reporting!

Not sure we should do something here as the Drupal coding standards say: "PHP allows classes to be aliased when they are imported into a namespace. In general that should only be done to avoid a name collision." https://www.drupal.org/docs/develop/coding-standards/namespaces#s-class-... →

In your example there is no name collision. I don't think it is good practice from Symfony to alias "Constraints" to "Assert", which can be super confusing as there is no Assert namespace anywhere.

Would like to hear more opinions on this!

Thanks for reporting, I pushed a fix!

Forgot credits.

Ok, then let's do this! Thanks a lot for your patience and rerolling.

I'm not a big fan of renaming sniffs as then this will break all phpcs configurations that exclude this sniff for example.

What could we do to preserve compatibility?

Also instead of doing this: How easy is it to make an exception in Drupal core in the phpcs.xml.dist config file so that the spell checker ignores this instance?

Side note: Coder is developed on Github, please create pull requests there once we come to a conclusion here. https://github.com/pfrenssen/coder/pulls

merged, thanks!

Looks good, thanks!

Patch.

klausi → created an issue.

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

I don't think the patch from #13 is the right approach. There can always be malformed requests by bad crawlers, so the page callback needs to be written in a robust way to not throw PHP warnings.

Patch from #1 is the correct approach IMO.

Setting this back to RTBC for patch #1, which is a straight forward fix.

Hey nice find!

We could copy the code from Symfony Process for the test, but not sure if it is worth it to try to port that somehow?

I would say we can commit this without test, since it does not affect end users and is just for command line invocations.

Merge request created.

klausi → created an issue.

The second #d7security newsletter is out - podcast episode, new members and more!
https://gitlab.com/d7security/d7security/-/issues/2#note_1761723679

Thanks, looks good!

Nice, thanks!

Raising priority as this could lead to nasty denial of service attacks.

Patch.

klausi → created an issue.

I think it is fine to have it also in good.module to mirror what is in bad.module, so can stay as is.

Thanks for reporting!

Can you isolate what the triggering code is and post it here? Looks like a CSS file sniff, are you running Coder on some minified CSS files maybe? Best create a phpcs.xml.dist config file and specify which paths should not be checked. https://www.drupal.org/docs/contributed-modules/code-review-module/php-c... →

Thanks, just one minor comments from me.

Could this disrupt custom Drupal projects where developers use trigger_error() for deprecations in their own project? Maybe this is not common. Developers can also disable individual rules, so there are workarounds in such a case.

In my projects we don't use trigger_error() and mostly rely on custom PHPStan rules that flag deprecated function invocations.

Merged, thanks!

We could be more fancy with the testing, but I think it is ok to rely on upstream Slevomat to do it fully.

Merged, thanks!

the check_plain() fixes are not correct, as the content is already escaped there.

Merged, thanks!

Merged, thanks!

merged, thanks!

Merged, thanks!

Merged, thanks!

Merged, thanks!

Added PHP 7.0 as minimum requirement to the info file because of the "??" operator.