nginx mis-identifying requested paths

[dustinbrisebois]

Greetings

We have noticed that in some cases the requeted route here

https://github.com/FMCorz/moodle-block_xp/blame/master/classes/local/routing/router.php#L68

Is not necessarily being caught, in these cases the return is "/" In this case, adding this in the line between 68 and 69.

$uri = $uri == '/' ? substr($_SERVER['REQUEST_URI'], strlen($_SERVER['DOCUMENT_URI'])) : $uri;

Would add an extra safe wrapper to grab the REQ_URI path and split the difference.

[FMCorz]

Thank you for the report @dustinbrisebois.

Would you please describe the cases which lead to the path being incorrect?

The logic in get_route_url is pretty much a copy of the logic in Moodle's get_file_argument which is Moodle's own routing for file downloads.

Could it be an issue with nginx configuration? See: https://levelup.branchup.tech/docs/article/nginx-slasharguments

[dustinbrisebois]

Frederic, yes the problem was found on Totara 12 (using a Moodle 3.2/3.3 core) - not sure why the route URL's were not being caught but this extra line acts as a security wrapper just in case it isn't being picked up.

[FMCorz]

Would you be able to provide URLs that were not being caught?

While this extra line may be working for you, it essentially negates validation the work done in get_route_url, therefore there must be an issue in the latter.

Would you also be able to test your configuration as per the instructions in this comment? #79 (comment)

Thank you!

[dustinbrisebois]

Frederic

In our case, visiting anywhere such as router.php\infos\1 the output for $uri was just an empty slash, so this supplements the work done in get_route_url if the return value is empty (or just a slash).

Totara behaves a bit differently than Moodle does, yes we use the same NGINX configuration for both,

So the line I added, checks if the $uri expected route is empty (thus the slash), if its a slash, it gets the difference of the two strings (REQUEST_URI and DOCUMENT_URI) this should result in the end-string arguement that SHOULD have been returned by get_route_url.

https://www.php.net/manual/en/function.parse-url.php may provide some cleanup options here

[FMCorz]

Hi @dustinbrisebois,

The function get_file_argument from Totara and Moodle core are identical, and thus do not behave differently when it comes to parsing the URL for pluginfile.php. Our plugin's get_route_url method is an adapted copy/paste of the core function, and should be returning the route correctly. If it does not, then there is a bug in the function itself. Catching the / and ignoring that a path was not correctly identified is a workaround that negates the following validating code:

        if (empty($relativepath) || $relativepath[0] !== '/') {
            return '/';
        }

Moreover, if Moodle and Totara accurately identify routes through pluginfile.php without the need to look at DOCUMENT_URI, then Level up! should not be using the latter either.

I'm more than happy to fix this issue, but without replication steps it is quite difficult. Would you please share your Nginx configuration, as well as running the test mentioned in this comment: #79 (comment).

Thank you for your collaboration.