Fix OIDC login CSRF by verifying sesskey on callback

[Patryk Mroczko (patmr7)]

No description provided.

[Copilot]

Pull request overview

This PR hardens the OIDC auth-code login flow against CSRF when the IdP uses cross-site form_post callbacks and the primary Moodle session cookie may not be sent due to SameSite=Lax behavior. It does this by setting a dedicated SameSite=None CSRF cookie during the outbound auth request and validating it on the callback before consuming the stored OIDC state.

Changes:

• Set an auth_oidc_csrf cookie (SameSite=None; Secure; HttpOnly) before initiating the OIDC auth request.
• On callback, verify the stored state’s sesskey against the CSRF cookie value (and reject/expire state on mismatch).
• Clear the CSRF cookie after successful validation in both the normal auth response and the admin-consent response handlers.

Suppressed comments (2)

auth/oidc/classes/loginflow/authcode.php:401

• The sesskey/CSRF-token comparison uses a direct string inequality, which can leak timing information and is less robust than a constant-time comparison. Use hash_equals() for comparing secret tokens.

        $csrftoken = $_COOKIE['auth_oidc_csrf'] ?? sesskey();
        if ($staterec->sesskey !== $csrftoken) {
            $DB->delete_records('auth_oidc_state', ['id' => $staterec->id]);
            throw new moodle_exception('errorauthunknownstate', 'auth_oidc');
        }

auth/oidc/classes/loginflow/authcode.php:406

• Clearing the CSRF cookie immediately after one callback can break parallel OIDC auth flows in multiple tabs/windows (the first callback clears the cookie before the second form_post callback arrives, and the session cookie may be blocked by SameSite=Lax). Consider keeping the cookie as a session cookie (it is overwritten on the next auth request anyway) rather than deleting it here.

        setcookie('auth_oidc_csrf', '', [
            'expires' => time() - 3600,
            'path' => parse_url($CFG->wwwroot, PHP_URL_PATH) ?: '/',
            'secure' => true,
            'httponly' => true,

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (4)

auth/oidc/classes/loginflow/authcode.php:407

• Cookie deletion is unconditional and always uses secure=true, but the cookie itself is only created when is_https() or $CFG->sslproxy is true. Align the deletion logic with the creation condition to avoid emitting Secure Set-Cookie headers on non-HTTPS requests and to keep behavior consistent across deployments.

        setcookie('auth_oidc_csrf', '', [
            'expires' => time() - 3600,
            'path' => parse_url($CFG->wwwroot, PHP_URL_PATH) ?: '/',
            'secure' => true,
            'httponly' => true,

auth/oidc/classes/loginflow/authcode.php:326

• The new sesskey vs auth_oidc_csrf verification is security-critical and should be covered by phpunit tests (success path with matching cookie, and failure path when cookie/session do not match), especially given response_mode=form_post and the SameSite=Lax session cookie behavior referenced in the comment.

        $csrftoken = $_COOKIE['auth_oidc_csrf'] ?? sesskey();
        if (!hash_equals((string)$staterec->sesskey, (string)$csrftoken)) {
            $DB->delete_records('auth_oidc_state', ['id' => $staterec->id]);
            throw new moodle_exception('errorauthunknownstate', 'auth_oidc');
        }

auth/oidc/classes/loginflow/authcode.php:240

• The CSRF cookie is only set in handleredirect()’s “initial login request” branch, but auth flows can also be initiated via initiateauthrequest() from other entry points (e.g. auth/oidc/ucp.php action=connectlogin). Because the OIDC client uses response_mode=form_post, the main session cookie may not be sent on the cross-site POST callback; without also setting auth_oidc_csrf for those initiations, the new sesskey comparison in handleauthresponse() will reject legitimate callbacks.

            // Set a dedicated CSRF cookie with SameSite=None so it is sent back on
            // cross-site form_post callbacks from the IdP, which SameSite=Lax on the
            // main session cookie (MDL-83526) would otherwise block.
            if (is_https() || !empty($CFG->sslproxy)) {
                $cookiepath = parse_url($CFG->wwwroot, PHP_URL_PATH) ?: '/';
                setcookie('auth_oidc_csrf', sesskey(), [
                    'expires' => time() + 5 * MINSECS,
                    'path' => $cookiepath,
                    'secure' => true,
                    'httponly' => true,
                    'samesite' => 'None',
                ]);
            }

auth/oidc/classes/loginflow/authcode.php:333

• Cookie deletion is unconditional and always uses secure=true, but the cookie itself is only created when is_https() or $CFG->sslproxy is true. Align the deletion logic with the creation condition to avoid emitting Secure Set-Cookie headers on non-HTTPS requests and to keep behavior consistent across deployments.

This issue also appears on line 403 of the same file.

        setcookie('auth_oidc_csrf', '', [
            'expires' => time() - 3600,
            'path' => parse_url($CFG->wwwroot, PHP_URL_PATH) ?: '/',
            'secure' => true,
            'httponly' => true,

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (2)

auth/oidc/classes/loginflow/authcode.php:411

• This introduces a new security-critical behavior (verifying the stored sesskey against a dedicated SameSite=None cookie) but there is no automated test coverage ensuring (1) mismatched/missing cookie rejects the callback and deletes state, and (2) valid cookie allows the flow and clears the cookie. Adding a PHPUnit test would help prevent regressions across browser/callback-mode changes.

        $csrftoken = $_COOKIE['auth_oidc_csrf'] ?? sesskey();
        if (!hash_equals((string)$staterec->sesskey, (string)$csrftoken)) {
            $DB->delete_records('auth_oidc_state', ['id' => $staterec->id]);
            throw new moodle_exception('errorauthunknownstate', 'auth_oidc');
        }

auth/oidc/classes/loginflow/authcode.php:302

• The CSRF cookie name/path/options are duplicated across set_csrf_cookie() and both callback handlers. This makes future changes (e.g., adjusting expiry/path/samesite) easy to miss in one location. Consider centralizing these values (e.g., a class constant for the cookie name and a small helper like clear_csrf_cookie($cookiepath)) and reusing it in both handle*response methods.

    protected function set_csrf_cookie(): void {
        global $CFG;
        if (is_https() || !empty($CFG->sslproxy)) {
            $cookiepath = parse_url($CFG->wwwroot, PHP_URL_PATH) ?: '/';
            setcookie('auth_oidc_csrf', sesskey(), [
                'expires' => time() + 5 * MINSECS,
                'path' => $cookiepath,
                'secure' => true,
                'httponly' => true,
                'samesite' => 'None',
            ]);
        }

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

auth/oidc/classes/loginflow/authcode.php:443

• Same issue as the cert-admin-consent handler: the HTTPS/sslproxy/cookie conditional can skip CSRF verification on non-HTTPS sites, leaving the original CSRF exposure. If the goal is to fix CSRF, prefer enforcing verification unconditionally at the callback handler.

        if (is_https() || !empty($CFG->sslproxy) || !empty($_COOKIE['auth_oidc_csrf'])) {
            $this->verify_csrf_cookie($staterec);
        }

auth/oidc/classes/loginflow/authcode.php:298

• The CSRF cookie expires after 5 minutes, but state records are only cleaned up asynchronously (cron) and the callback code does not check timecreated. If cron is delayed or the user takes longer than 5 minutes to complete the IdP login, the state may still be present while the cookie has expired, causing a hard-to-debug login failure (cookie missing -> sesskey() fallback -> mismatch on form_post). Consider extending the cookie expiry to include some buffer beyond the cleanup window.

            'expires' => time() + 5 * MINSECS,

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (1)

auth/oidc/classes/loginflow/authcode.php:376

• The CSRF applicability guard is_https() || !empty($CFG->sslproxy) || !empty($_COOKIE['auth_oidc_csrf']) is duplicated verbatim here and again in handleauthresponse() (lines 441-443). Since this guard decides whether the CSRF verification runs at all, any future edit that updates one copy but not the other would silently disable protection in one of the two callback paths. Consider centralizing this decision — e.g. move the applicability check inside verify_csrf_cookie() (returning early when it does not apply) and call it unconditionally from both handlers, so the two paths cannot diverge.

        if (is_https() || !empty($CFG->sslproxy) || !empty($_COOKIE['auth_oidc_csrf'])) {
            $this->verify_csrf_cookie($staterec);
        }