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.

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

lorispath leaks into PSR7 request on Apache server #9049

Closed
MaximeMulder opened this issue Feb 12, 2024 · 2 comments · Fixed by #9055
Closed

lorispath leaks into PSR7 request on Apache server #9049

MaximeMulder opened this issue Feb 12, 2024 · 2 comments · Fixed by #9055
Labels
Bug PR or issue introducing/requiring bug fixes (not mutually exclusive with the Feature label)

Comments

@MaximeMulder
Copy link
Contributor

Describe the bug

The URL modification by mod_rewrite, which transforms loris.ca/foo/bar into loris.ca?lorispath=foo/bar, seems to leak into $_SERVER and consequently into the PSR7 $request object on Apache server. This means that lorispath is accessible in getQueryParams() and more importantly that $request->getUri()->__toString() is not usable to get the current request URL.

To Reproduce

  1. Have a setup with Apache server
  2. Inspect the PSR7 $request object.

We reproduced this behaviour on both my's and @ridz1208's machines.

Thoughts

I am not sure yet what are the possible fixes to this unintended behaviour. Either way, here are the potential solutions I could think of:

  1. Change the Apache/PHP configuration to prevent the rewritten URL from leaking.
  2. Manually create the $request object to use the original request URL.
  3. Do not rewrite the URL (if possible).
  4. Ignore the leak (the original URL is still accessible in $_SERVER['REQUEST_URI']).
@MaximeMulder MaximeMulder added the Bug PR or issue introducing/requiring bug fixes (not mutually exclusive with the Feature label) label Feb 12, 2024
@driusan
Copy link
Collaborator

driusan commented Feb 12, 2024

As far as I know 3 is not possible. The rewrite is how URLs with paths get transformed from "/foo/bar" to the LORIS entry point.

Using _$SERVER is also not an option, we should not be using PHP superglobals anywhere.

@driusan
Copy link
Collaborator

driusan commented Feb 13, 2024

I looked over this with @ridz1208 and it seems like the path and query params are correct in the PSR7 object, it's just that it also has the lorispath query parameter (which was added by mod_rewrite) set.

I think the solution is just to modify (or rather create a new, since they're immutable) request object right after its created in index.php (around here: https://github.com/aces/Loris/blob/main/htdocs/index.php#L42) to remove the lorispath query parameter before passing it along to the LORIS router.

driusan pushed a commit that referenced this issue Feb 27, 2024
Removes lorispath from the PSR7 request's query parameters and uri query string (both must be updated separately).

Resolves #9049
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug PR or issue introducing/requiring bug fixes (not mutually exclusive with the Feature label)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants