Fix OIDC login CSRF by verifying sesskey on callback

[Patryk Mroczko (patmr7)]

No description provided.

[Copilot]

Pull request overview

This PR aims to harden the auth_oidc authorization-code login flow against CSRF/login-CSRF issues caused by SameSite=Lax session cookies not being sent on IdP response_mode=form_post callbacks, by introducing a dedicated CSRF cookie and verifying it against the stored state record.

Changes:

• Sets a dedicated auth_oidc_csrf cookie with SameSite=None; Secure; HttpOnly prior to initiating the OIDC auth request (when HTTPS is in use).
• Validates the callback by comparing the stored auth_oidc_state.sesskey to the CSRF cookie (falling back to sesskey() when no cookie is present).
• Clears the CSRF cookie after successful state validation in both standard auth and admin-consent callback handlers.

---

💡 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 (2)

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

• The CSRF cookie is only set when the site is considered HTTPS (is_https() or $CFG->sslproxy). However, the auth flow is hard-coded to use response_mode=form_post (see auth/oidc/classes/oidcclient.php), where the main session cookie may not be sent cross-site; in non-HTTPS deployments this means the callback will almost always fail the new sesskey check (falling back to sesskey() from a new session) and surface as "errorauthunknownstate". Consider either (a) switching to response_mode=query when HTTPS is unavailable, (b) failing fast with a clear admin-facing error that HTTPS is required for form_post + SameSite=None, or (c) conditionally relaxing the new check only when SameSite=None cookies cannot be used (with an explicit security note).

            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:409

• The CSRF validation and cookie-clearing logic is duplicated here and in handlecertadminconsentresponse(). This duplication increases the risk of the two paths drifting (e.g., cookie attributes, path calculation, or future token changes) and makes future security fixes easier to miss. Consider extracting this into a small shared helper (e.g., validate_state_sesskey_and_clear_cookie($staterec)) used by both methods.

        $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');
        }
        setcookie('auth_oidc_csrf', '', [
            'expires' => time() - 3600,
            'path' => parse_url($CFG->wwwroot, PHP_URL_PATH) ?: '/',
            'secure' => true,
            'httponly' => true,
            'samesite' => 'None',
        ]);

[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:407

• Same cookie-normalization issue here as above: $_COOKIE['auth_oidc_csrf'] may be an array or empty string, which can trigger notices on string-cast and cause unexpected failures. Ensure it's a non-empty string before comparing, otherwise fall back to sesskey().

        $csrftoken = $_COOKIE['auth_oidc_csrf'] ?? sesskey();

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

• $_COOKIE['auth_oidc_csrf'] is untrusted input and can be an array (e.g. if the client sends auth_oidc_csrf[]=...), which will raise notices when cast to string and passed into hash_equals(). Normalize to a non-empty string before use, otherwise fall back to sesskey() as intended.

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

        $csrftoken = $_COOKIE['auth_oidc_csrf'] ?? sesskey();

[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 as above: this conditional can skip the sesskey/CSRF binding on callbacks received over plain HTTP (when the CSRF cookie wasn't stored), leaving the login-CSRF vulnerability unaddressed in that configuration. It's safer to always enforce verify_csrf_cookie() and fail closed when validation can't be performed.

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

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

• The new CSRF-cookie flow introduces several important security/compatibility paths (cookie present, cookie missing with SameSite=Lax session present, cookie missing with session missing) but there are no PHPUnit tests covering this behavior. Adding tests would help prevent regressions in this callback validation logic.

    protected function verify_csrf_cookie(stdClass $staterec): void {
        global $DB;

        $csrfcookie = $_COOKIE['auth_oidc_csrf'] ?? null;
        $csrftoken = (is_string($csrfcookie) && $csrfcookie !== '') ? $csrfcookie : sesskey();
        $valid = hash_equals((string) $staterec->sesskey, (string) $csrftoken);