Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
cron_example module doesn't demonstrate basic security best practices on its routes.
We need to change that.
In the routes.yml file, it currently says:
requirements:
_access: 'TRUE'
This allows anyone with access to the site to see the page, which opens up other possible security concerns.
Proposed resolution
- Change the routes.yml file to say something like this:
requirements: _permission: 'access content' - Update the tool menu test to reflect that this route is not visible to anonymous users, and *is* visible once a user with 'access content' permissions has been logged in.
- Amend any tests which use these routes to log in a user who can access them.
| Comment | File | Size | Author |
|---|---|---|---|
| #22 | cron_example-access_checking-2585581-8-22.patch | 1.3 KB | Andrew.Mikhailov |
| |||
| #20 | cron_example-access_checking-2585581-8-20.patch | 1.46 KB | Andrew.Mikhailov |
| |||
| #18 | cron_example-access_checking-2585581-8-17.patch | 1.46 KB | Andrew.Mikhailov |
| |||
| #18 | interdiff.txt | 1.46 KB | Andrew.Mikhailov |
| #16 | cron_example-access_checking-2585581-16-8.patch | 1.31 KB | sumthief |
| |||
Comments
Comment #2
sumthief CreditAttribution: sumthief as a volunteer and at DrupalJedi commentedComment #3
Mile23Comment #4
Mile23Comment #7
sumthief CreditAttribution: sumthief as a volunteer and at DrupalJedi commentedNeed manually review.
Comment #9
sumthief CreditAttribution: sumthief as a volunteer and at DrupalJedi commentedUpdate tests in patch #2.
Comment #10
marvil07 CreditAttribution: marvil07 as a volunteer commentedHow was the 2nd and 3rd hunk related t the issue here? (i.e. use minimal profile and give admin permission to the test user)
Comment #11
sumthief CreditAttribution: sumthief as a volunteer and at DrupalJedi commentedThanks for your response. The test failed cause it can't find 'Run cron now' button: because it'll shown only for user with 'administer site configuration' permission. So including profile is one of ways to provide this permission for user creating process. But it's incorrect. So there is new patch.
Comment #12
sumthief CreditAttribution: sumthief as a volunteer and at DrupalJedi commentedComment #16
sumthief CreditAttribution: sumthief as a volunteer and at DrupalJedi commentedUpdate patch.
Comment #17
sumthief CreditAttribution: sumthief as a volunteer and at DrupalJedi commentedComment #18
Andrew.Mikhailov CreditAttribution: Andrew.Mikhailov at DrupalJedi commentedHello!
Grigory's patch correct but not quite, you need to define user's permissions in setUp method.
Please check interdiff and apply this patch from Grigory.
Thank you.
Best regards.
Comment #20
Andrew.Mikhailov CreditAttribution: Andrew.Mikhailov at DrupalJedi commentedSorry, old my problem)
I've created patch via phpStorm)
I removed unnecessary module 'system' from test)
Best regards.
Comment #22
Andrew.Mikhailov CreditAttribution: Andrew.Mikhailov at DrupalJedi commentedSeems it was wrong...
One more time, don't forget apply this patch from Grigoriy.
Best regards.
Comment #24
Mile23We don't need system, but we do need node since it provides 'access content.'
Fixed on commit.
Thanks!
Comment #25
sumthief CreditAttribution: sumthief as a volunteer and at DrupalJedi commented@Mile23, why we don't need 'system'? It provides permision 'administer site configuration'.
According to patch from #20: It has no 'system' in module list, but tests failed because there are no button 'Run cron now'. I think it caused by missing permission (in form builder there is the condition that checks the permission and doesn't create form element if user has no permission). But patch from #22 (or 18) absolutely identical to patch from #20 excluding existing 'system' in module list. And patches from #22 and #18 passed tests succesfully. So I think we should include system in module list.
Comment #26
Mile23The patch in #20 says
...which should be '_permission'.
The 'administer site configuration' permission doesn't come from the system module.