From 4cbe0e6b8f8db53eb4b7906dc3373af3089781d7 Mon Sep 17 00:00:00 2001 From: Tom Elliott Date: Mon, 17 Aug 2026 19:05:03 -0500 Subject: [PATCH] Read query parameters through Route::queryParam() Both flow entry points are reached by an internal rewrite to api/index.php. On nginx that rewrite handed the router an EMPTY query string, so filter_input(INPUT_GET, 'provider') returned null and start() refused a configured, enabled provider as "Unknown identity provider" -- the plugin could not sign anybody in on any nginx install. callback() reads state, code and error the same way, so the flow could not have completed either. fogproject#1163 gives the installer's vhost $is_args$args, which fixes it at the source, and makes Route::queryParam() public. That method recovers the value from REQUEST_URI when $_GET is empty, which is what carries every server that has not re-run the installer -- an existing vhost is not rewritten by anything. Apache carried QSA all along and never had the problem, and this code cannot tell which it is running under, so it uses the method that is right for both. tests/oidc-flow-safety.test.php gains a gate: no filter_input(INPUT_GET) anywhere in the flow, and all four parameters read through Route::queryParam(). It strips comments first, because this file's own docblock names the wrong call so a reader knows what not to write, and a gate that reads its own documentation as a violation is a gate nobody can document. Four mutations verified, all caught. Verified end to end against a real Keycloak: sign-in provisions the user, records the identity, and grants the role and user group from the groups claim; removing the group at the provider and signing in again revokes both. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017aBSWrDArXHTpKWkkN27LR --- oidc/class/oidcflow.class.php | 22 ++++++++++++++--- tests/oidc-flow-safety.test.php | 43 +++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 4 deletions(-) diff --git a/oidc/class/oidcflow.class.php b/oidc/class/oidcflow.class.php index 2e1558e..6c4927e 100644 --- a/oidc/class/oidcflow.class.php +++ b/oidc/class/oidcflow.class.php @@ -81,6 +81,17 @@ class OIDCFlow extends FOGBase /** * Send the browser to the provider. * + * Every query parameter here and in callback() is read through + * Route::queryParam(), never filter_input(INPUT_GET). A route under + * /ext/ is reached by an internal rewrite to api/index.php, and on + * nginx that rewrite used to hand the router an EMPTY query string -- + * so ?provider=3 arrived as nothing and a configured, enabled provider + * was refused as "Unknown identity provider". fogproject#1163 gives the + * installer's vhost $is_args$args, but a server that has not re-run the + * installer keeps the old one, and queryParam() is what recovers the + * value from REQUEST_URI on those. Apache carries QSA and never had the + * problem; this code cannot tell which it is running under. + * * @return void */ public static function start() @@ -88,7 +99,10 @@ public static function start() self::_session(); try { $provider = self::_enabledProvider( - (int)filter_input(INPUT_GET, 'provider', FILTER_VALIDATE_INT) + (int)filter_var( + (string)Route::queryParam('provider'), + FILTER_VALIDATE_INT + ) ); $config = self::_discover($provider); @@ -160,7 +174,7 @@ public static function callback() _('The sign-in took too long; please start again') ); } - $error = trim((string)filter_input(INPUT_GET, 'error')); + $error = trim((string)Route::queryParam('error')); if ('' !== $error) { // The provider's own words, which are the useful ones -- // 'access_denied' means somebody pressed cancel. @@ -171,13 +185,13 @@ public static function callback() ) ); } - $state = (string)filter_input(INPUT_GET, 'state'); + $state = (string)Route::queryParam('state'); if (!hash_equals((string)$flow['state'], $state)) { // Constant time, and the message says nothing about which // half was wrong. throw new \Exception(_('The sign-in could not be verified')); } - $code = (string)filter_input(INPUT_GET, 'code'); + $code = (string)Route::queryParam('code'); if ('' === $code) { throw new \Exception(_('The identity provider sent no code')); } diff --git a/tests/oidc-flow-safety.test.php b/tests/oidc-flow-safety.test.php index 20b4957..9277294 100644 --- a/tests/oidc-flow-safety.test.php +++ b/tests/oidc-flow-safety.test.php @@ -644,6 +644,49 @@ function methodBody($src, $method) } } +/* + * N. Query parameters come from Route::queryParam(), never filter_input(). + * + * Both entry points are reached by an internal rewrite to api/index.php. + * On nginx that rewrite handed the router an EMPTY query string, so + * filter_input(INPUT_GET, 'provider') returned null and start() refused a + * configured, enabled provider as "Unknown identity provider"; callback() + * would have lost state, code and error the same way. fogproject#1163 + * fixes the vhost, but only for a server that re-runs the installer, and + * Route::queryParam() is what recovers the value from REQUEST_URI on every + * server that does not. + */ +$flowCode = ''; +foreach (token_get_all($flowSrc) as $tok) { + // Comments stripped first: this file's own docblock names the wrong + // call so a reader knows what not to write, and a gate that reads its + // own documentation as a violation is a gate nobody can document. + if (is_array($tok) + && ($tok[0] === T_COMMENT || $tok[0] === T_DOC_COMMENT) + ) { + continue; + } + $flowCode .= is_array($tok) ? $tok[1] : $tok; +} +if (false !== strpos($flowCode, 'filter_input(INPUT_GET')) { + fail( + 'OIDCFlow reads a query parameter with filter_input(INPUT_GET, ...),' + . ' which is empty on a routed request behind an nginx vhost that' + . ' predates fogproject#1163 -- use Route::queryParam()' + ); +} +foreach (['provider', 'error', 'state', 'code'] as $param) { + if (false === strpos($flowCode, "Route::queryParam('" . $param . "')")) { + fail( + sprintf( + 'OIDCFlow no longer reads the %s query parameter through' + . ' Route::queryParam()', + $param + ) + ); + } +} + if (count($fails) > 0) { fwrite(STDERR, 'FAIL: ' . count($fails) . " problem(s):\n"); foreach ($fails as $f) {