fix: Update Redirect URI checks for different runtimes#23
Conversation
Fixes issues on runtimes which don't set `SERVER_NAME`, relies now only on `REQUEST_URI`
| // Some loose checks: ensure the host and path of the current request URL matches what was set when initiating the sign-in. | ||
| if ( | ||
| parse_url($signInSession->redirectUri, PHP_URL_HOST) !== ($_SERVER['SERVER_NAME'] ?? null) || | ||
| parse_url($signInSession->redirectUri, PHP_URL_HOST) !== parse_url($_SERVER['REQUEST_URI'], PHP_URL_HOST) || |
There was a problem hiding this comment.
parse_url($_SERVER['REQUEST_URI'], PHP_URL_HOST) looks problematic here. In a typical PHP request, $_SERVER['REQUEST_URI'] only contains the path and query string, such as /callback?code=..., so PHP_URL_HOST usually resolves to null. That means this change can turn a valid callback into a host mismatch and make handleSignInCallback() throw unexpectedly.
So while this PR is trying to address the RoadRunner case where $_SERVER['SERVER_NAME'] may be missing, the current fix risks breaking the common PHP/FPM flow. In other words, it may solve the original issue in one runtime by introducing a regression in more standard environments.
I think a safer approach would be to fall back to HTTP_HOST / SERVER_NAME, or skip the host check when the runtime does not expose host information, while still keeping the path validation.
Also, could you add tests for this? At minimum, it would be good to cover:
- the original scenario this PR is trying to fix:
SERVER_NAMEmissing but callback still succeeds when the request matches - the normal callback case in a typical PHP environment, to make sure this change does not introduce a regression
There was a problem hiding this comment.
Okay, I'll give this a look. 👍🏽
LeoAdamek
changed the title
REQUEST_URI only
|
I've updated the change so that it tries to get the server host in the following order:
I've added a test that mimicks the roadrunner runtime's behaviour, so this should cover both scenarios. |
2 participants
Fixes #22