This module lets the user run PHPUnit, PHPStan, PHPCs and custom tests from the admin area of drupal. It assumes that all modules are put in contrib or custom folders in the modules folder. It provides a config page for PHPUnit where the admin can add variables to the phpunit.xml file and rebuild it when needed. It also provides a report page in the style of Recent log messages for all tests.
Installation
Install using composer.
1) install wikimedia/composer-merge-plugin
composer require wikimedia/composer-merge-plugin
In your composer.json in the root of your drupal installation add the following code block under extra after the installer-paths section. Depending on the installation you may have to omit the web/ directory.
"merge-plugin": {
"include": [
"web/modules/contrib/testsuite/composer.json"
],
"replace": false,
"ignore-duplicates": true
},
2) install drupal/testsuite
composer require drupal/testsuite
3) run composer update
This will download if not already downloaded all the dev packages needed for testsuite to work.
This uses the wikimedia/composer-merge-plugin. Read the page for more information on installation. Add modules/contrib/testsuite/composer.json or web/modules/contrib/testsuite/composer.json depending on your path.
Project link
https://www.drupal.org/project/testsuite
Manual reviews of other projects
Media Power BI
Field Extra
Time Clock
Kontainer
Error Reporting
CKEditor 5 Icons
Comment | File | Size | Author |
---|---|---|---|
#40 | duplicate code.png | 120.32 KB | slogar32 |
Issue fork projectapplications-3377246
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedComment #3
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedComment #4
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedComment #5
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedComment #6
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedComment #7
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedComment #8
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedComment #9
apadernoThank you for applying!
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 smother review.
The important notes are the following.
phpcs --standard=Drupal,DrupalPractice
on the project, which alone fixes most of what reviewers would report.To the 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.
Reviewer's task is describing what needs to be changed, not providing patches to fix what reported in a review.
Comment #10
vinaymahale CreditAttribution: vinaymahale as a volunteer and at SJ Innovation LLC commentedPlease set the default branch to 3.0.x
Also fix the below PHPCS issues:
Comment #11
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedHey thanks for the input on this. Can you please tell me how you configures phpcs and phpcbf because after installing and setting it all up I get conflicting reports between VScode terminal and the intel in the editor itself. When the editor reports it is good(No errors) the terminal reports a ton of errors and visa versa. I need to get this going for work. Thanks.
Comment #12
Grevil CreditAttribution: Grevil as a volunteer and at DROWL.de commented@Trigve Hagen, take a look at https://www.drupal.org/docs/develop/development-tools/editors-and-ides/c...!
Comment #13
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedAwesome thanks!
Comment #14
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedI finished up the tasks. Hows it look? Thanks
Comment #15
vishal.kadamComment #16
vishal.kadamFix phpcs issues.
Comment #17
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedGot them fixed. Should be ready now.
Comment #18
vishal.kadamFix phpcs issue.
Comment #19
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedJust fixed it Ready.
Comment #20
vishal.kadamRest looks fine to me.
Let’s wait for other reviewers to take a look and if everything goes fine, you will get the role.
Comment #21
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedThanks
Comment #22
apadernoComment #23
vinaymahale CreditAttribution: vinaymahale as a volunteer and at SJ Innovation LLC commentedNo issues found
Changing status to RTBC!
Comment #24
apadernosrc/Controller/TestSuiteController.php
The code is rendering complex markup using #markup, which is thought for simple markup. For complex markup #theme or #type should be used.
src/TestInterface.php
If it returns the module name, it cannot return a Boolean value.
The method description does not match with the return value description.
Comment #25
apadernoI am changing priority as per Issue priorities.
Comment #26
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedIve finished the security fixes and I'm ready for review again. Thanks
Comment #27
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedReady for review.
Comment #28
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedNeeds Review. Thanks.
Comment #29
apadernoComment #30
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedI need it reviewed Please.
Comment #31
vishal.kadamInconsistent drupal core requirement
FILE: composer.json
FILE: testsuite.info.yml
core_version_requirement: ^10 || ^11
Comment #32
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedFixed it Thanks.
Comment #33
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedI realy need this for work on a site. Thanks
Comment #34
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedReady for Review. Thanks. Do I need to do anything else? Also why where the 8 and 9 branches removed? I'm Ok with removing the 8 branch but I need the 9 branch.
Comment #35
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedAnybody There?
Comment #36
cmlara@Trigve Hagen
Per https://www.drupal.org/node/539608#s-application-review-timelines the priority should be Normal as the issue has only been in needs-review for 7 days (prior to the 7 day mark it should have been minor as the issue had been waiting in 'needs work' for over 5 weeks).
Perhaps you want to follow the procedure discussed in
https://www.drupal.org/docs/develop/managing-a-drupalorg-theme-module-or... to 'boost' your position, and perhaps learn more about writing secure code along the way? There are a number of other applications that have been waiting in needs review longer than your application has that could use some help moving forward.
Comment #37
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedThanks. Will do.
Comment #38
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedComment #39
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedReviewed other applications.
Comment #40
slogar32 CreditAttribution: slogar32 as a volunteer and at Agiledrop - Your Trusted Drupal Teammates commentedI reviewed the module code, here are my findings:
The description for a module is Hook implementations for the [module name] module. where [module name] is the module name given in the .info.yml file.
Function and method declarations are written on a single line.
So the above example should be something like:
If you want to keep the documentation comment with the corrected format. Don't forget to do this for all the constructors in the module.
object
for the @param descriptions in documentation comments, but instead the actual class/type of the parametershould be
It then makes sense for the @param description not to be the actual namescape with the class name, but an actual description (doesn't need to be same as in the example that I gave above).
You have it like this in the
TestInterface.php
, but you are missing the \ in front of the class with the namespace in the @param comment, it should be like this::Please check, that all of your @param in documentation comments are consistent, another example that should be fixed here in
GuzzleService.php
:Should be:
should be:
TestSuiteController.php
lines 281-300 are duplicated in lines 187-206 and lines 303-317 are duplicated in 209-223, both of these duplications could be one function. Also inConfigurationForm.php
, you have the same line twice in lines 351 and 352.should be:
and in the constructor:
. And when I just posted this example, you can probably replace the dirname nesting with the
constant, if you used this because of multi OS support, then just use the levels parameter of the dirname function instead of nesting it like this.
This is a full review of the code, when all of this is fixed I see no reason why this shouldn't be in the RBTC status. Please fix all of the occurrences of the issues mentioned above, I've just put some examples in the list. After you do this run phpcs again for Drupal and DrupalPractice standards. If you have any questions or need some help with the fixes just contact me directly on one of the platforms on my profile.
Comment #41
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedSome of these sound right so I'm working on them. Thanks.
Comment #42
slogar32 CreditAttribution: slogar32 as a volunteer and at Agiledrop - Your Trusted Drupal Teammates commentedThose are suggestions for better code, but I think numbers 1 and 2 are needed to pass this validation here, since those are direct Drupal standards (those two are quick fixes also). Depends on the people who have roles to confirm you for the opt-in validation. Good luck in any case!
Comment #43
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedI adjusted the duplicated code. I also adjusted MessengerInterface. And I adjusted the comments. The @file, core modules uses a description of what the module does. Check automated_cron, ban, basic_auth, big_pipe, etc.. Im just following their lead. Thanks for the review.
Comment #44
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedComment #45
cmlara@Trigve Hagen:
From #40.6: Is there a reason you are making recursive dirname calls and are not using
DIRECTORY_SEPARATOR
for path access?dirname(dirname(dirname(dirname(__DIR__)))) . '\sites\simpletest'
Removing PAreview Bonus as it has been utilized for the #40 review. You may review another batch of applications to set the bonus tag again (and keep it for the remainder) see https://www.drupal.org/docs/develop/managing-a-drupalorg-theme-module-or...
Comment #46
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedComment #47
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedUpdated code to use DIRECTORY_SEPARATOR. Finished 2 more reviews. Thanks.
Comment #48
cmlaraI have reached out directly to the applicant via email with a question.
This issue should remain in needs work until further notice.
Comment #49
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedReady again. Thanks
Comment #50
cmlaraThe above change of status was made after informing the applicant concerns still exist. However as the high level fault appears (on a cursory review) to have been solved I am not moving the issue back to needs work.
The majority of the technical details are currently being withheld following a standard 30 day after resolution disclosure policy.
Without going into technical details, I am concerned, based on interactions with the applicant and a cursory review of the code, that the applicant may not have a sufficiently firm grasp of native PHP Functions and Drupal API which would hinder their ability to write secure PHP code.
Comment #51
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedI was in a rush to get this done for work. The plan there is to only use it for testing not production. This module will be great for doing just that though. Finding out where code is buggy, testing new programmers on their skills. Definitely will slow down a bit but this module will be great for helping product owners and management find real talent and ensure code quality. I have learned a lot through this whole process. Thanks for making me a better programmer.
Comment #52
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedIve been doing an extensive remodel on this project that will be done in a couple of days. I'm adding phpcs and phpstan alongside phpunit, building all three as report pages with filter forms and adding databases for each one. I'm also going to build a Security best practices page for custom code. There is no use leaving this in this category for now. Moving to needs work. Thanks.
Comment #53
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedI'm also working on creating a vulnerability scan for packages and libraries.
Comment #54
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedFinished the work ready for review. Thanks.
Comment #55
cmlaraPublic Followup to #48 Remote Code Exploit discovered in code.
Note: this is only a partial review of the code between commits 856ea365..c5586106. Reviewers should check to see if any of the concerns below still exist or have been duplicated in the code as it appears the project has had a major re-write since initial reports.
On February 1st the maintainer was contacted via email to determine if it would be an appropriate location to discuss the details. After confirmation by the maintainer they were informed that the module appears to be vulnerable to a Remote Code Execution exploit via a GET request to the testsuite.phpunit_file_report route.
A user with the 'access site reports' permission could visit
admin/testsuite/phpunit/file/report/custom/anything/%3Btouch%20vulnerable.txt
.Upon request Drupal Core would call TestSuiteController::phpUnitFileReport() with the parameters from the GET request. There was no filtering of the the data provided in the GET request allowing arbitrary additions.
TestSuiteController::phpUnitFileReport() would call LoadResourceService::getStatment() to obtain a command statement and assign the result to $statement.
The sample request would create a $statement string of
'vendor/bin/phpunit --debug -c web/core web/modules/custom/anything/tests/src/;touch vulnerable.txt'
When executed on a *nix system two commands would execute, the first for PHPUnit against a directory, and the second to create a "vulnerable.txt" file.
TestSuiteContoller would next call $this->shellExecService->getData($statement) which directly called PHP's shell_exec($statement).
Sequence of discussion and fixes occurred as follows:
Mitigating factors:
A user must have the 'access site reports' permission which is considered a 'trusted' role. However nowhere is it indicated that this role could execute code on the server.
Absent from the fixes was:
Any use of escapeshellcmd() even though the helper method takes an arbitrarily formatted executable command.
Any use of escapeshellarg() to reduce the risk of errors in code propagating through to exploitable commands.
At no time did the maintainer consult on their fixes prior to publication.
At the time of the 'final commit' getStatment() could be called with
$filePath = '/some/path/;touch vulnerable.txt;'
as the preg_match() check contained no start/end of line restrictions allowing a vulnerable command to be generated however there was no currently known exploit path beyond nonresistant contrib extension.As the applicant did not discuss the above use of public methods to generate possibly exploitable calls care should be taken to validate that the applicant has not reproduced such issues during the rework of the code.
I would encourage the reviewers to ensure the applicant does indeed show a knowledge of writing Secure PHP code and adhering to the Drupal API without being prompted.
Note: This report includes information on unfixed concerns as the maintainer has already published their 'completed' fix and initial vulnerability disclosure. The maintainer provided no indication they intended to perform more directly related with the information they had been given. The unfixed concerns directly related to the initial concerns provided to the maintainer and were considered to be covered under the 30 day publication delay.
Adding the 'PAreview: security' tag to indicate that this application revealed a security concern. This tag should not be removed as it is used for statistical tracking.
Comment #56
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedI'll double check everything again. In the new code base I took out the ability to call a phpunit statement via an url. I will update the regex to have a start(^) and and end of string($). Thanks
Comment #57
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedAdded a fix for the regex in all calls that passed parameters to the getData function. Added the ^ at the beginning and the $ at the end. Updated the getData function with the escapeshellcmd() function. escapeshellarg() is adding quotes and stopping the execution of the statements. Since your explaining the vulnerability in detail here. Please call me if you do not want to reply here from now on. My email box gets stuffed with mail and they become easy to miss. Sorry but I missed the last email because of it. My phone number is on my email. Thanks.
Comment #58
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedNeeds Review
Comment #59
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedComment #60
Trigve Hagen CreditAttribution: Trigve Hagen as a volunteer commentedNeeds Review!