fix: Update redirect URI checks to use REQUEST_URI only#23
fix: Update redirect URI checks to use REQUEST_URI only#23LeoAdamek wants to merge 1 commit intologto-io:masterfrom
REQUEST_URI only#23Conversation
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
2 participants
Fixes #22