Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix CSRF validation for remotecontrol API route #3599

Closed
wants to merge 3 commits into from
Closed

Fix CSRF validation for remotecontrol API route #3599

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d8b30f5
Fix "No CSRF" route for remotecontrol API
edgarrmondragon Nov 3, 2023
ae12ad1
Update tests
edgarrmondragon Nov 4, 2023
d4b0490
Accept optional leading slash
edgarrmondragon Nov 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion application/config/internal.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@
'noCsrfValidationParams' => array(),
'noCsrfValidationRoutes' => array(
'rest',
'remotecontrol',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like a breaking change to expect users to update this if they have overridden this in their config, so I wonder if the maintainers feel strongly about relaxing the regex in application/core/LSHttpRequest.php instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

surely if a user override this, they extend the array, not replace it

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like a breaking change to expect users to update this if they have overridden this in their config, so I wonder if the maintainers feel strongly about relaxing the regex in application/core/LSHttpRequest.php instead.

The commit was done to be sure to allow only needed route :). Relaxing : less CRSF control. I think we must keep current regexp.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I agree, it needs to stay as the more restrictive regexp

the issue before is that routes that match a portion of the routes exempted from csrf checking would also be exempt

ie

  1. rest/v1/session
  2. admin/menus/sa/restore

both are exempted because they contain "rest" anywhere

this is definitely not desirable for security

'admin/remotecontrol',
'plugins/unsecure',
),
'csrfCookie' => array(
Expand Down
2 changes: 1 addition & 1 deletion application/core/LSHttpRequest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,6 @@ public static function routeMatchesNoCsrfValidationRule($route, $rule)
// For example the routes "rest" (in the case of "index.php/rest?...") or "rest/..." (in the case of
// "index.php/rest/...") should be matched by the rule "rest", but the route "admin/menus/sa/restore"
// should not.
return preg_match('#^' . $rule . '$|^' . $rule . '/#', (string) $route);
return preg_match('#^/?' . $rule . '$|^/?' . $rule . '/#', (string) $route);
}
}
5 changes: 3 additions & 2 deletions tests/unit/CsrfHttpRequestTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,9 @@ public function testRestRoutesSkipCsrfValidation()
public function testRemoteControlRoutesSkipCsrfValidation()
{
$routes = array(
'remotecontrol/actionOnItemById/15',
'remotecontrol/action',
'admin/remotecontrol/actionOnItemById/15',
'admin/remotecontrol/action',
'/admin/remotecontrol',
);

foreach ($routes as $route) {
Expand Down
Loading