- Issue created by @kevin.brocatus
- 🇮🇳India vishal.kadam Mumbai
Thank you for applying! Reviewers will review the project files, describing what needs to be changed.
Please read Review process for security advisory coverage: What to expect → for more details and Security advisory coverage application checklist → to understand what reviewers look for. Tips for ensuring a smooth review gives some hints for a smoother review.
To reviewers: Please read How to review security advisory coverage applications → , What to cover in an application review → , and Drupal.org security advisory coverage application workflow → .
While this application is open, only the user who opened the application can make commits to the project used for the application.
Reviewers only describe what needs to be changed; they don't provide patches to fix what reported in a review.
- 🇮🇳India vinaymahale
Ran PHPCS tests. No major issues found except for a few lines exceeding characters which could be understood.
Let's wait for other reviewers! - 🇳🇱Netherlands kevin.brocatus
I appreciate the feedback @vinaymahale! I have fixed the characters limit in the README file too.
- 🇳🇱Netherlands willempje2
Just going through list given in the "PAReview: Review Template" and check what could be applicable to this module:
Code long/complex enough for review
Application has more than 120 lines and 5 function or class method definitionsSecure code
Twig templates correctly used
Placeholders are correctly used for database storageJavascript (jQuery) and Drupal.checkPlain()
This might be issue, my jQuery knowledge is limited but there seems to be values outputted in DOM that are not sanitized.
See: https://git.drupalcode.org/project/virtual_select/-/blob/1.0.x/js/virtua...
And: https://www.drupal.org/docs/security-in-drupal/writing-secure-code-for-d... → - 🇳🇱Netherlands kevin.brocatus
Thanks for reviewing @Willempje2. As far as I am concerned your suspicion about the jQuery .val() shouldn't case any security issues.
- The value that is being set is put into a value attribute, not directly into the DOM.
- The value that is being set comes directly from another select list which has been sanitized by Drupal.
- Sanitizing the value could actually cause issues as
<div>value</div>
is a valid select key.
- 🇮🇹Italy apaderno Brescia, 🇮🇹
I am removing the reported manual reviews for other applications, as per Review bonus → , since this application has been already reviewed.
Keep in mind that, as per the Review bonus documentation page, applications are reviewed before others when they use the PAreview: review bonus issue tag.
- 🇮🇹Italy apaderno Brescia, 🇮🇹
If I execute code similar to the following one, in a page that contains a basic single-line text field, I would not see a dialog box appear.
$("input:text").val("<" + "script>alert('I did it')</" + "script>");
Clearly, if what entered in that text field is then used to create content in a page, it should be sanitized.
- 🇮🇹Italy apaderno Brescia, 🇮🇹
Thank you for your contribution! I am going to update your account.
These 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 more contributors chatting on the Slack → #contribute channel. So, come hang out and stay involved → .
Thank you, 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.I thank all the reviewers.
- Assigned to apaderno
- Status changed to Fixed
about 1 year ago 11:16am 27 September 2023 Automatically closed - issue fixed for 2 weeks with no activity.