Thanks, approach makes sense. Can you add an automated test case?
Thanks, approach makes sense to me!
Can we add an automated test case?
merged, thanks!
No thanks bot, we are already Drupal 11 compatible.
Thanks, this is a bit dangerous as then the query tag could be supplied from user input if the developer sets it up wrong. We just fixed a similar minor security weakness in 🐛 Wrong usage of EntityQuery data producer can lead to SQL access bypass Active with the entity_query producer.
Could we lock this down somehow so that developers cannot expose this by accident?
Merged, thanks!
Thanks for submitting!
The block configuration form is vulnerable to XSS exploits. Any editor user that has the permission "Administer blocks" can insert malicious scripts into the Login Text field like <script>alert('XSS');</script>.
The permission "Administer blocks" is not restricted for access and often given to content editors. They should not be able to take over the site with an XSS attack.
Solution: do you need to use the Markup class? If you just pass your assembled string to #markup then Drupal will sanitize bad scripts for you? Or does it then strip your i HTML tag? Then you could use Xss::filter() to only allow some tags you need.
And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
Released: https://www.drupal.org/project/coder/releases/8.3.26 →
Thanks everyone!
Sorry, was busy with other things. Thanks a lot for stepping up Arkener, love the Drupal community!
I will make a release now and explain in the release notes how people can add a snippet to their phpcs.xml file to keep sorted use statements. It is very useful in teams to better avoid git merge conflicts.
jjchinquist → credited klausi → .
Very sorry for the disruption this caused.
The coding standards issue in question has been worked on for 12 years, I don't want to wait that long in Coder for adopting useful standards.
Now it turns out that the case-sensitivity of the standard causes pain - probably best if we open a new issue to weigh pros and cons of a revert.
In the meantime please use workarounds:
* Disable this sniff in your project
* Configure phpcbf in your Phpstorm to auto-fix the use statement ordering so that you don't have to think about it.
Hm, sorry that this caused problems for many people :-|
I think it is probably best to open a new Coder issue to carefully list the pros and cons of a revert, and also workarounds like configuring phpcbf in Phpstorm or disabling this sniff so that you don't have a problem. Some projects have adopted the case-sensitive standard already, so a revert creates work again for them.
Vscode: line sorting is almost correct in our case-sensitive standard and wrong in our case-insensitive standard.
Phpstorm: line sorting is correct in the case-sensititve standard, but "optimize import" is wrong in the case sensitive standard. Reversed for the case-insensitive standard.
Textmate: line sorting is wrong in the case-sensititve standard.
Difficult choice how and where we want to be wrong with our standard. I would say Vscode is more important than Textmate for example, but a hard choice.
Having no sorting standard for use statements is even worse, makes it harder to find use statements in longer lists.
The 5th D7security newsletter is out: https://www.d7security.org/blog/10-09-2024-d7security-team-newsletter/
jjchinquist → credited klausi → .
Merge request created. @kingdutch can you review?
Merged.
Marking this as done.
Uploading patch file for composer patches.
And merged, should appear here soon.
Thanks for the review, good point about the menu itself.
I added the cache context for the menu as well. Not sure if this even has an effect, as I did not understand the connection between menu and menu links. But I think further testing on adding/removing menu links and caching is out of scope for this issue. I also think menus don't change that often, so should only be a minor issue.
Let's fix the inaccessible links here and postpone any further work to a new issue if it is a problem.
Thanks, can you add a simple kernel test so that we also have test coverage here?
@andriy khomych I added cache context and 2 test cases.
Do you need the access result neutral check? We should not allow neutral access results, right? Only menu links where the user has access should be returned.
Hm, I'm not sure we should do this.
Comments in general are a good thing - they explain what code does. Maybe you want to summarize why you have that constructor and what it is doing?
I think you never want to forbid comments, they should always be allowed on any function.
Sure, constructors are often obvious, so we allow you to omit the function comment. But forbidding it? I think no.
Started with some unfinished test coverage, not done yet.
No perl command needed, you can run phpcbf and it will bulk fix all use statement ordering for you.
Thanks!
Next steps:
* we need steps to reproduce in the issue summary. When, why and how does this happen?
* We need to document in code why we introduce the render context there
* We need a new automated test to demonstrate we fixed something.
A workaround is to install the Drupal coding Standard in phpstorm with newest Coder, then the use statements will be ordered correctly.
I think we cannot revert the Coder change now as people are already adopting this new standard.
Please push phpstorm to fix this issue: https://youtrack.jetbrains.com/issue/WI-26078/Automatically-added-use-st...
OK, that was quick and we already changed the regex to only match for the colon.
Closing this as fixed again, please make a new issue if you want to address the too relaxed edge case :-)
Thanks, good points.
For the too relaxed edge case: I think we don't care. I assume it would make the sniff code more complex to detect/split comments. But I'm very happy to review and accept pull requests that try to address this!
For the additional keywords: I think your proposal makes sense to just match for the colon at the end of the regex pattern and add "spellchecker". I will check if I can do that quickly now.
Merged, thanks! Also added a small test case.
All done here, new release created and module is supported again https://gitlab.com/d7security/autocomplete_deluxe/-/releases/7.x-2.4
Also updated the project page.
jjchinquist → credited klausi → .
klausi → created an issue. See original summary → .
Merge conflicts needs to be resolved.
Can we add test coverage for this? Are there other tests already where this would fit in?
Same problem on D7: ✨ [D7] Option to do not delete the job on exception Needs review
Patch.
Ah, looks like issue credits can only be assigned by maintainers here. I probably see the checkboxes because I'm a project admin, but can't set them.
Anyway, if you want you can set issue credits for appreciating the work :)
Nice, thanks a lot!
I'll assign issue credits for the 3 of us for reporting and resolving this, I hope that is ok!
Thanks for reporting! Created upstream fix: https://github.com/PHPCSStandards/PHP_CodeSniffer/pull/604
Not sure if we should fork this into Coder in the meantime.
Merged 📌 Configure use-statement sniff to be case sensitive Active in Coder to switch on case-sensitive sorting.
Escalating this again to major, as we still have no coding standard although this rule is running in Coder for a year now.
I like the proposed text of @quiteone in #67.
Agreed, pushed the config for that.
Thanks, can you make a pull request at https://github.com/pfrenssen/coder/pulls so that we see the automated test cases run?
It looks like in the Drupal core phpcs.xml.dist config you forgot to add the CSS file extensions that you want to check.
<arg name="extensions" value="php,module,inc,install,test,profile,theme,css,info,txt,md,yml"/>
Maybe there is generated CSS in core that we don't want to check and that's why it was never enabled?
Wrapper sniff: that is an interesting idea, how could we do that without breaking existing configurations? Then both sniffs would run and duplicate the errors messages, could we prevent that?
Otherwise I agree with jonathan1055, we should add those names as exceptions to Drupal core's cspell config.
Fixed the upper case cspell and merged it, linked the other issue by accident in the commit.
Thanks for reporting, I pushed a fix!
I think we are mostly done here for now. I left the old Github issues and pull requests open, but new ones will be closed immediately with a pointer to the drupal.org issues queue. Not sure yet what we should do with them, I'm sure there is relevant stuff there that should be picked up.
I assigned issue credits for all 4.x commits that were done in the year 2024.
klausi → created an issue. See original summary → .
klausi → created an issue. See original summary → .
Implemented Github actions to close issues an PRs.
TODO:
1, Fix PHPStan and PHPCS in d.-o testing
2. Make some artificial drupal.org issues to give credit for some of the most recent contributions
Yep, this was done. Thanks a lot!
Thanks for the hint, updated the version number in the user guide!
Happy to hear that it works for you :)
Released at https://gitlab.com/d7security/views_slideshow/-/releases/7.x-3.11
Follow the user guide to get updates from d7security: https://www.d7security.org/docs/general-information/user-guide
@maintainers: can you add the following to the project page?
<h2>Drupal 7 version</h2>
The Drupal 7 version of the Views Slideshow module is not supported on drupal.org anymore. The <a href="https://www.d7security.org/">D7Security group</a> has taken over maintainership <a href="https://gitlab.com/d7security/views_slideshow">on Gitlab</a>. Follow the <a href="https://www.d7security.org/docs/general-information/user-guide">D7Security user guide</a> to continue receiving security update notifications for the Views Slideshow module.
Support for drupal 7 modules cannot be enabled again. We will have to move this module to d7security to continue support there.
GraphQL 3.x is in maintaince mode, please upgrade to 8.x.4.x.
GraphQL 3.x is in maintaince mode, please upgrade to 8.x.4.x.
Obsolete.
Obsolete.
Obsolete.
Started the move:
1. Enabled issues at
https://www.drupal.org/project/issues/graphql?categories=All →
2. Removed Github actions for testing and mirroring
3. Enabled auto mirror from drupal.org to Github
4. Assigned first issue credit to myself for making the move
https://www.drupal.org/project/graphql/issues/3467189
📌
Implement automated testing on drupal.org Gitlab
Fixed
TODO:
1. find some Github action that auto-closes new Github issues and pull requests, pointing people to drupal.org
2. Make some artificial drupal.org issues to give credit for some of the most recent contributions
Done!
PR ready: https://github.com/pfrenssen/coder/pull/233
Places where we have misspelled sniffs and error codes - made ignore rules for them for now:
* SeletorSingleLine
* ColourDefinitionSniff
* TeamplateSpacingAfterComment
* UnecessaryFileDeclaration
* TforValue
* TrhowsCommentIndentation
Opened 📌 Enable cspell checking in Coder Active to track that.
I think enabling cspell checking in Coder is a very good idea, so that we at least prevent future spelling problems. Started a draft PR at https://github.com/pfrenssen/coder/pull/233 , not finished yet. Should probably be done as separate issue and then we can continue the discussion here what to do about already established sniff names and error codes.
Coder sniff names are internal error codes - sometimes we have to build camel case word combinations to make the error code useful.
So we cannot fix the sniff names, because people rely on the codes now in their configuration. And we should not change error codes just to make cspell happy.
So that leaves us with the option to exclude those words in cspell, where would be a good place for that? Probably Drupal core's dictionary file? Is is possible to put long dot works like "DrupalPractice.General.OptionsT.TforValue" there?
I agree - and I don't think we will need @var comments in the future as now you must always specify the type of your class property as type hint (at least for new code). Eventually we can remove the @var coding standard and enforce property type hints with a sniff from Slevomat.
valthebald → credited klausi → .
Oh hm, @jenlampton replied in Slack that the Backdrop fix is not necessary for Drupal 7 because Views UI has an extra XSS filter that Backdrop does not have anymore. She is right, I tried to exploit this but could not get XSS to trigger when adding a field in the Views UI.
Is there any other place where these field labels are displayed?
Very sorry for the noise here, I should have tested my assumptions fully before escalating here. Downgrading the priority again and setting this back to active.
For the Views module in Drupal 7 I think we should not remove the extra XSS filter in Views UI, even if there could be some double escaping then. Keeping the potential double escaping is the secure choice at this point in the late D7 cycle.
Let me know if you see the XSS trigger and where exactly!
It is a security issue, but it falls outside of the security advisory policy of the Drupal Security team. The vulnerability can only be exploited if the attacker has an elevated permission "administer fields". Therefore we can fix this in public (not sure why Backdrop issued an advisory).
Clarified that in the issue summary
Pushed that.
Merged, thanks!
Ported patch from backdrop.
This was released as backdrop security fix in https://backdropcms.org/security/backdrop-sa-core-2024-001
Raising priority to critical as this is a small XSS vulnerability, but fortunately can only be exploited by trusted admins as Damien said.
Setting to "needs work" as I think we should do the same fix as Backdrop.
merged, thanks!