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.
CommentFileSizeAuthor
#22 cron_example-access_checking-2585581-8-22.patch1.3 KBandrew.mikhailov
#20 cron_example-access_checking-2585581-8-20.patch1.46 KBandrew.mikhailov
#18 cron_example-access_checking-2585581-8-17.patch1.46 KBandrew.mikhailov
#18 interdiff.txt1.46 KBandrew.mikhailov
#16 cron_example-access_checking-2585581-16-8.patch1.31 KBsumthief
#11 cron_example-access_checking-2585581-11-8.patch1.25 KBsumthief
#9 cron_example-access_checking-2585581-9-8.patch1.3 KBsumthief
#2 cron_example-access_checking-2585581-2-8.patch396 bytessumthief

Comments

Mile23 created an issue. See original summary.

sumthief’s picture

sumthief commented
Status: Active » Needs review
StatusFileSize
new cron_example-access_checking-2585581-2-8.patch396 bytes
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs review » Needs work
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: cron_example-access_checking-2585581-2-8.patch, failed testing.

The last submitted patch, 2: cron_example-access_checking-2585581-2-8.patch, failed testing.

sumthief’s picture

sumthief commented
Status: Needs work » Needs review

Need manually review.

Status: Needs review » Needs work

The last submitted patch, 2: cron_example-access_checking-2585581-2-8.patch, failed testing.

sumthief’s picture

sumthief commented
Status: Needs work » Needs review
StatusFileSize
new cron_example-access_checking-2585581-9-8.patch1.3 KB

Update tests in patch #2.

marvil07’s picture

marvil07 commented
Status: Needs review » Postponed (maintainer needs more info)

How was the 2nd and 3rd hunk related t the issue here? (i.e. use minimal profile and give admin permission to the test user)

sumthief’s picture

sumthief commented
StatusFileSize
new cron_example-access_checking-2585581-11-8.patch1.25 KB

Thanks 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.

sumthief’s picture

sumthief commented
Status: Postponed (maintainer needs more info) » Needs review

The last submitted patch, 2: cron_example-access_checking-2585581-2-8.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 11: cron_example-access_checking-2585581-11-8.patch, failed testing.

The last submitted patch, 11: cron_example-access_checking-2585581-11-8.patch, failed testing.

sumthief’s picture

sumthief commented
StatusFileSize
new cron_example-access_checking-2585581-16-8.patch1.31 KB

Update patch.

sumthief’s picture

sumthief commented
Status: Needs work » Needs review
andrew.mikhailov’s picture

andrew.mikhailov commented
StatusFileSize
new interdiff.txt1.46 KB
new cron_example-access_checking-2585581-8-17.patch1.46 KB

Hello!
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.

Status: Needs review » Needs work

The last submitted patch, 18: cron_example-access_checking-2585581-8-17.patch, failed testing.

andrew.mikhailov’s picture

andrew.mikhailov commented
Status: Needs work » Needs review
StatusFileSize
new cron_example-access_checking-2585581-8-20.patch1.46 KB

Sorry, old my problem)
I've created patch via phpStorm)
I removed unnecessary module 'system' from test)
Best regards.

Status: Needs review » Needs work

The last submitted patch, 20: cron_example-access_checking-2585581-8-20.patch, failed testing.

andrew.mikhailov’s picture

andrew.mikhailov commented
Status: Needs work » Needs review
StatusFileSize
new cron_example-access_checking-2585581-8-22.patch1.3 KB

I removed unnecessary module 'system' from test)

Seems it was wrong...
One more time, don't forget apply this patch from Grigoriy.
Best regards.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs review » Fixed 
+++ b/cron_example/src/Tests/CronExampleTestCase.php
@@ -37,7 +37,7 @@ class CronExampleTestCase extends WebTestBase {
+  public static $modules = ['cron_example', 'system', 'node'];

We don't need system, but we do need node since it provides 'access content.'

Fixed on commit.

Thanks!

sumthief’s picture

sumthief commented
Status: Fixed » Needs work

@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.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Fixed

The patch in #20 says

+++ b/cron_example/cron_example.routing.yml	(revision )
@@ -4,4 +4,4 @@
   requirements:
-    _access: 'TRUE'
+    _access: 'access content'

...which should be '_permission'.

The 'administer site configuration' permission doesn't come from the system module.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.