Fix CSRF validation for remotecontrol API route

[edgarrmondragon]

Fixes an error introduced in #3588. The changes made the remote control route, admin/remotecontrol, no longer skip CSRF validation.

I can report the bug in https://bugs.limesurvey.org, but this error is present only in the master branch and I thought it quicker to send a PR.

Fixed issue #19220

[edgarrmondragon]

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.

[mfavetti]

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

[Shnoulle]

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.

[mfavetti]

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

[mfavetti]

i don't think this is sufficient to fix the regression, please see my note: https://bugs.limesurvey.org/view.php?id=19220

[Shnoulle]

i don't think this is sufficient to fix the regression, please see my note: https://bugs.limesurvey.org/view.php?id=19220

I think it's OK here, why not sufficient ?

[mfavetti]

only fix get, not path routing

[edgarrmondragon]

only fix get, not path routing

@mfavetti Let me know if d4b0490 addresses that 🙂

[mfavetti]

only fix get, not path routing

@mfavetti Let me know if d4b0490 addresses that 🙂

okay remote control works now using get and path routing
i think rest didn't work before on get anyways, path worked on gabrieljenik's original code
not sure how to test plugin/unsecure, didn't work much with plugins yet (basically can someone answer if this is the actual path or is it expanded to something like admin/plugin/unsecure?

weird to note that /admin/remotecontrol -> /admin/remotecontrol but /rest/v1/session -> rest/v1/session
why is the handling of leading slash different??

if it was the same then you would just have needed to fix remotecontrol -> admin/remotecontrol and no need to worry about the leading slash

[Shnoulle]

only fix get, not path routing

It must be done on route, no dependant on path or get , then fix this part :

LimeSurvey/application/core/LSHttpRequest.php

Line 158 in 70b0989

$route = Yii::app()->getUrlManager()->parseUrl($this);

if needed ?

It seems OK : https://www.yiiframework.com/doc/api/1.1/CUrlManager#parseUrl-detail

the route (controllerID/actionID) and perhaps GET parameters in path format.

Fix BOTH routing

[Shnoulle]

only fix get, not path routing

@mfavetti Let me know if d4b0490 addresses that 🙂

Strange ? Seems parseUrl function is broken ?

In my opinon: route for
example.org/controller/action or example.org/?r=controller/action but be same (controller/action)

And route example.org//controller/action or example.org//?r=controller/action : broken (/controller)

It's not the case ?

(then maybe best is to extend it :

    public function parseUrl($request)
    {
        return ltrim(parent::parseUrl($request), "/");
    }

[gabrieljenik]

I think it is OK like this.
I would feel better if there would be a full RC test end to end that connects to the server, which is missing. There are functional tests that connect to webdirver, but not tests that connect to RC endpoint

[gabrieljenik]

The offedning PR was reverted.
This PR, right now, needs conflict resolution.
Maybe this should be closed and incorporated in a new PR, related to the original fix.

[gabrieljenik]

I will be working on this. Probbly on a new PR with a proper name

[edgarrmondragon]

@gabrieljenik gotcha. Feel free to close this.