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) {