From f33087ac5e4b1eb6c4f82d0acfcfac27663a0e23 Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Fri, 13 Mar 2026 20:59:10 +0000 Subject: [PATCH 1/3] fix(coderd): render HTML error page for OIDC email validation failures Replace raw JSON httpapi.Write responses with site.RenderStaticErrorPage for three OIDC authentication error cases: - Email address not from an authorized domain - Malformed email (missing @ when domain restrictions are configured) - Email address not verified by the OIDC provider These errors occur during browser redirect flows, so users previously saw ugly JSON in their browser. Now they see the same styled static error page used by group allow-list and signups-disabled errors, with a 'Back to login' action button. --- coderd/userauth.go | 41 +++++++++--- coderd/userauth_test.go | 135 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 169 insertions(+), 7 deletions(-) diff --git a/coderd/userauth.go b/coderd/userauth.go index 42a8f3871b0da..8b23caa550975 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -44,6 +44,7 @@ import ( "github.com/coder/coder/v2/coderd/util/ptr" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/cryptorand" + "github.com/coder/coder/v2/site" ) type MergedClaimsSource string @@ -1343,8 +1344,17 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { verified, ok := verifiedRaw.(bool) if ok && !verified { if !api.OIDCConfig.IgnoreEmailVerified { - httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ - Message: fmt.Sprintf("Verify the %q email address on your OIDC provider to authenticate!", email), + site.RenderStaticErrorPage(rw, r, site.ErrorPageData{ + Status: http.StatusForbidden, + HideStatus: true, + Title: "Email not verified", + Description: fmt.Sprintf( + "Verify the %q email address on your OIDC provider to authenticate!", + email, + ), + Actions: []site.Action{ + {URL: "/login", Text: "Back to login"}, + }, }) return } @@ -1370,8 +1380,17 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { ok = false emailSp := strings.Split(email, "@") if len(emailSp) == 1 { - httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ - Message: fmt.Sprintf("Your email %q is not from an authorized domain! Please contact your administrator.", email), + site.RenderStaticErrorPage(rw, r, site.ErrorPageData{ + Status: http.StatusForbidden, + HideStatus: true, + Title: "Unauthorized email", + Description: fmt.Sprintf( + "Your email %q is not from an authorized domain! Please contact your administrator.", + email, + ), + Actions: []site.Action{ + {URL: "/login", Text: "Back to login"}, + }, }) return } @@ -1385,8 +1404,17 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { } } if !ok { - httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ - Message: fmt.Sprintf("Your email %q is not from an authorized domain! Please contact your administrator.", email), + site.RenderStaticErrorPage(rw, r, site.ErrorPageData{ + Status: http.StatusForbidden, + HideStatus: true, + Title: "Unauthorized email", + Description: fmt.Sprintf( + "Your email %q is not from an authorized domain! Please contact your administrator.", + email, + ), + Actions: []site.Action{ + {URL: "/login", Text: "Back to login"}, + }, }) return } @@ -1406,7 +1434,6 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { if ok { picture, _ = pictureRaw.(string) } - ctx = slog.With(ctx, slog.F("email", email), slog.F("username", username), slog.F("name", name)) user, link, err := findLinkedUser(ctx, api.Database, oidcLinkedID(idToken), email) diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index d78c77f45e741..0858264429d49 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -1107,6 +1107,13 @@ func TestUserOIDC(t *testing.T) { }, AllowSignups: true, StatusCode: http.StatusForbidden, + AssertResponse: func(t testing.TB, resp *http.Response) { + data, err := io.ReadAll(resp.Body) + require.NoError(t, err) + body := string(data) + require.Contains(t, body, "Email not verified") + require.Equal(t, "text/html; charset=utf-8", resp.Header.Get("Content-Type")) + }, }, { Name: "EmailNotAString", @@ -1144,6 +1151,14 @@ func TestUserOIDC(t *testing.T) { "coder.com", }, StatusCode: http.StatusForbidden, + AssertResponse: func(t testing.TB, resp *http.Response) { + data, err := io.ReadAll(resp.Body) + require.NoError(t, err) + body := string(data) + require.Contains(t, body, "Unauthorized email") + require.Contains(t, body, "is not from an authorized domain") + require.Equal(t, "text/html; charset=utf-8", resp.Header.Get("Content-Type")) + }, }, { Name: "EmailDomainWithLeadingAt", @@ -1170,6 +1185,14 @@ func TestUserOIDC(t *testing.T) { "@coder.com", }, StatusCode: http.StatusForbidden, + AssertResponse: func(t testing.TB, resp *http.Response) { + data, err := io.ReadAll(resp.Body) + require.NoError(t, err) + body := string(data) + require.Contains(t, body, "Unauthorized email") + require.Contains(t, body, "is not from an authorized domain") + require.Equal(t, "text/html; charset=utf-8", resp.Header.Get("Content-Type")) + }, }, { Name: "EmailDomainCaseInsensitive", @@ -2062,6 +2085,10 @@ func TestOIDCDomainErrorMessage(t *testing.T) { require.Contains(t, string(data), "is not from an authorized domain") require.Contains(t, string(data), "Please contact your administrator") + // Verify the response is a rendered HTML error page, not raw JSON. + require.Equal(t, "text/html; charset=utf-8", resp.Header.Get("Content-Type")) + require.Contains(t, string(data), "Unauthorized email") + require.Contains(t, string(data), "/login") for _, domain := range allowedDomains { require.NotContains(t, string(data), domain) @@ -2091,6 +2118,10 @@ func TestOIDCDomainErrorMessage(t *testing.T) { require.Contains(t, string(data), "is not from an authorized domain") require.Contains(t, string(data), "Please contact your administrator") + // Verify the response is a rendered HTML error page, not raw JSON. + require.Equal(t, "text/html; charset=utf-8", resp.Header.Get("Content-Type")) + require.Contains(t, string(data), "Unauthorized email") + require.Contains(t, string(data), "/login") for _, domain := range allowedDomains { require.NotContains(t, string(data), domain) @@ -2098,6 +2129,110 @@ func TestOIDCDomainErrorMessage(t *testing.T) { }) } +// TestOIDCErrorPageRendering verifies that OIDC authentication errors +// (unauthorized email domain, unverified email) render a user-friendly +// HTML error page instead of raw JSON. +func TestOIDCErrorPageRendering(t *testing.T) { + t.Parallel() + + // assertIsHTMLErrorPage checks that the response is a rendered static + // HTML error page with the expected title and description fragment. + assertIsHTMLErrorPage := func(t *testing.T, resp *http.Response, expectedTitle, expectedDescFragment string) { + t.Helper() + + data, err := io.ReadAll(resp.Body) + require.NoError(t, err) + body := string(data) + + // Should be HTML, not JSON. + require.Equal(t, "text/html; charset=utf-8", resp.Header.Get("Content-Type")) + require.Contains(t, body, "") + + // Should contain the expected title and description. + require.Contains(t, body, expectedTitle) + require.Contains(t, body, expectedDescFragment) + + // Should contain a "Back to login" link. + require.Contains(t, body, "/login") + require.Contains(t, body, "Back to login") + + // Should NOT contain JSON markers. + require.NotContains(t, body, `"message"`) + require.NotContains(t, body, `"detail"`) + } + + t.Run("UnauthorizedEmailDomain", func(t *testing.T) { + t.Parallel() + + fake := oidctest.NewFakeIDP(t, oidctest.WithServing()) + cfg := fake.OIDCConfig(t, nil, func(cfg *coderd.OIDCConfig) { + cfg.AllowSignups = true + cfg.EmailDomain = []string{"coder.com"} + }) + client := coderdtest.New(t, &coderdtest.Options{ + OIDCConfig: cfg, + }) + + claims := jwt.MapClaims{ + "email": "user@evil.corp", + "email_verified": true, + "sub": uuid.NewString(), + } + _, resp := fake.AttemptLogin(t, client, claims) + defer resp.Body.Close() + + require.Equal(t, http.StatusForbidden, resp.StatusCode) + assertIsHTMLErrorPage(t, resp, "Unauthorized email", "is not from an authorized domain") + }) + + t.Run("MalformedEmailNoDomain", func(t *testing.T) { + t.Parallel() + + fake := oidctest.NewFakeIDP(t, oidctest.WithServing()) + cfg := fake.OIDCConfig(t, nil, func(cfg *coderd.OIDCConfig) { + cfg.AllowSignups = true + cfg.EmailDomain = []string{"coder.com"} + }) + client := coderdtest.New(t, &coderdtest.Options{ + OIDCConfig: cfg, + }) + + claims := jwt.MapClaims{ + "email": "no-at-sign", + "email_verified": true, + "sub": uuid.NewString(), + } + _, resp := fake.AttemptLogin(t, client, claims) + defer resp.Body.Close() + + require.Equal(t, http.StatusForbidden, resp.StatusCode) + assertIsHTMLErrorPage(t, resp, "Unauthorized email", "is not from an authorized domain") + }) + + t.Run("EmailNotVerified", func(t *testing.T) { + t.Parallel() + + fake := oidctest.NewFakeIDP(t, oidctest.WithServing()) + cfg := fake.OIDCConfig(t, nil, func(cfg *coderd.OIDCConfig) { + cfg.AllowSignups = true + }) + client := coderdtest.New(t, &coderdtest.Options{ + OIDCConfig: cfg, + }) + + claims := jwt.MapClaims{ + "email": "kyle@kwc.io", + "email_verified": false, + "sub": uuid.NewString(), + } + _, resp := fake.AttemptLogin(t, client, claims) + defer resp.Body.Close() + + require.Equal(t, http.StatusForbidden, resp.StatusCode) + assertIsHTMLErrorPage(t, resp, "Email not verified", "Verify the") + }) +} + func TestOIDCSkipIssuer(t *testing.T) { t.Parallel() const primaryURLString = "https://primary.com" From 12231e9de7c1bd28fad9faabd413010c18d99e0e Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Mon, 16 Mar 2026 15:14:06 +0000 Subject: [PATCH 2/3] review: fold HTML error page assertions into existing tests Remove standalone TestOIDCErrorPageRendering and roll its checks (doctype, Back to login, no JSON markers) into the existing TestUserOIDC and TestOIDCDomainErrorMessage assertions. --- coderd/userauth_test.go | 135 +++++++--------------------------------- 1 file changed, 23 insertions(+), 112 deletions(-) diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index 0858264429d49..b5df59ae82bf8 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -1111,13 +1111,17 @@ func TestUserOIDC(t *testing.T) { data, err := io.ReadAll(resp.Body) require.NoError(t, err) body := string(data) - require.Contains(t, body, "Email not verified") + // Should be an HTML error page, not JSON. require.Equal(t, "text/html; charset=utf-8", resp.Header.Get("Content-Type")) + require.Contains(t, body, "") + require.Contains(t, body, "Email not verified") + require.Contains(t, body, "Verify the") + require.Contains(t, body, "Back to login") + require.NotContains(t, body, `"message"`) }, }, { - Name: "EmailNotAString", - IDTokenClaims: jwt.MapClaims{ + Name: "EmailNotAString", IDTokenClaims: jwt.MapClaims{ "email": 3.14159, "email_verified": false, "sub": uuid.NewString(), @@ -1155,9 +1159,13 @@ func TestUserOIDC(t *testing.T) { data, err := io.ReadAll(resp.Body) require.NoError(t, err) body := string(data) + // Should be an HTML error page, not JSON. + require.Equal(t, "text/html; charset=utf-8", resp.Header.Get("Content-Type")) + require.Contains(t, body, "") require.Contains(t, body, "Unauthorized email") require.Contains(t, body, "is not from an authorized domain") - require.Equal(t, "text/html; charset=utf-8", resp.Header.Get("Content-Type")) + require.Contains(t, body, "Back to login") + require.NotContains(t, body, `"message"`) }, }, { @@ -1189,9 +1197,13 @@ func TestUserOIDC(t *testing.T) { data, err := io.ReadAll(resp.Body) require.NoError(t, err) body := string(data) + // Should be an HTML error page, not JSON. + require.Equal(t, "text/html; charset=utf-8", resp.Header.Get("Content-Type")) + require.Contains(t, body, "") require.Contains(t, body, "Unauthorized email") require.Contains(t, body, "is not from an authorized domain") - require.Equal(t, "text/html; charset=utf-8", resp.Header.Get("Content-Type")) + require.Contains(t, body, "Back to login") + require.NotContains(t, body, `"message"`) }, }, { @@ -2087,8 +2099,10 @@ func TestOIDCDomainErrorMessage(t *testing.T) { require.Contains(t, string(data), "Please contact your administrator") // Verify the response is a rendered HTML error page, not raw JSON. require.Equal(t, "text/html; charset=utf-8", resp.Header.Get("Content-Type")) + require.Contains(t, string(data), "") require.Contains(t, string(data), "Unauthorized email") - require.Contains(t, string(data), "/login") + require.Contains(t, string(data), "Back to login") + require.NotContains(t, string(data), `"message"`) for _, domain := range allowedDomains { require.NotContains(t, string(data), domain) @@ -2120,119 +2134,16 @@ func TestOIDCDomainErrorMessage(t *testing.T) { require.Contains(t, string(data), "Please contact your administrator") // Verify the response is a rendered HTML error page, not raw JSON. require.Equal(t, "text/html; charset=utf-8", resp.Header.Get("Content-Type")) + require.Contains(t, string(data), "") require.Contains(t, string(data), "Unauthorized email") - require.Contains(t, string(data), "/login") - + require.Contains(t, string(data), "Back to login") + require.NotContains(t, string(data), `"message"`) for _, domain := range allowedDomains { require.NotContains(t, string(data), domain) } }) } -// TestOIDCErrorPageRendering verifies that OIDC authentication errors -// (unauthorized email domain, unverified email) render a user-friendly -// HTML error page instead of raw JSON. -func TestOIDCErrorPageRendering(t *testing.T) { - t.Parallel() - - // assertIsHTMLErrorPage checks that the response is a rendered static - // HTML error page with the expected title and description fragment. - assertIsHTMLErrorPage := func(t *testing.T, resp *http.Response, expectedTitle, expectedDescFragment string) { - t.Helper() - - data, err := io.ReadAll(resp.Body) - require.NoError(t, err) - body := string(data) - - // Should be HTML, not JSON. - require.Equal(t, "text/html; charset=utf-8", resp.Header.Get("Content-Type")) - require.Contains(t, body, "") - - // Should contain the expected title and description. - require.Contains(t, body, expectedTitle) - require.Contains(t, body, expectedDescFragment) - - // Should contain a "Back to login" link. - require.Contains(t, body, "/login") - require.Contains(t, body, "Back to login") - - // Should NOT contain JSON markers. - require.NotContains(t, body, `"message"`) - require.NotContains(t, body, `"detail"`) - } - - t.Run("UnauthorizedEmailDomain", func(t *testing.T) { - t.Parallel() - - fake := oidctest.NewFakeIDP(t, oidctest.WithServing()) - cfg := fake.OIDCConfig(t, nil, func(cfg *coderd.OIDCConfig) { - cfg.AllowSignups = true - cfg.EmailDomain = []string{"coder.com"} - }) - client := coderdtest.New(t, &coderdtest.Options{ - OIDCConfig: cfg, - }) - - claims := jwt.MapClaims{ - "email": "user@evil.corp", - "email_verified": true, - "sub": uuid.NewString(), - } - _, resp := fake.AttemptLogin(t, client, claims) - defer resp.Body.Close() - - require.Equal(t, http.StatusForbidden, resp.StatusCode) - assertIsHTMLErrorPage(t, resp, "Unauthorized email", "is not from an authorized domain") - }) - - t.Run("MalformedEmailNoDomain", func(t *testing.T) { - t.Parallel() - - fake := oidctest.NewFakeIDP(t, oidctest.WithServing()) - cfg := fake.OIDCConfig(t, nil, func(cfg *coderd.OIDCConfig) { - cfg.AllowSignups = true - cfg.EmailDomain = []string{"coder.com"} - }) - client := coderdtest.New(t, &coderdtest.Options{ - OIDCConfig: cfg, - }) - - claims := jwt.MapClaims{ - "email": "no-at-sign", - "email_verified": true, - "sub": uuid.NewString(), - } - _, resp := fake.AttemptLogin(t, client, claims) - defer resp.Body.Close() - - require.Equal(t, http.StatusForbidden, resp.StatusCode) - assertIsHTMLErrorPage(t, resp, "Unauthorized email", "is not from an authorized domain") - }) - - t.Run("EmailNotVerified", func(t *testing.T) { - t.Parallel() - - fake := oidctest.NewFakeIDP(t, oidctest.WithServing()) - cfg := fake.OIDCConfig(t, nil, func(cfg *coderd.OIDCConfig) { - cfg.AllowSignups = true - }) - client := coderdtest.New(t, &coderdtest.Options{ - OIDCConfig: cfg, - }) - - claims := jwt.MapClaims{ - "email": "kyle@kwc.io", - "email_verified": false, - "sub": uuid.NewString(), - } - _, resp := fake.AttemptLogin(t, client, claims) - defer resp.Body.Close() - - require.Equal(t, http.StatusForbidden, resp.StatusCode) - assertIsHTMLErrorPage(t, resp, "Email not verified", "Verify the") - }) -} - func TestOIDCSkipIssuer(t *testing.T) { t.Parallel() const primaryURLString = "https://primary.com" From c5ea34ec4f2c3900d0649a8eb678dc38a0b769f4 Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Mon, 16 Mar 2026 11:19:40 -0400 Subject: [PATCH 3/3] drive-by fix for log message for allowing unverified oidc email Copilot review noted a malformed logging statement. Updated to emit email as apparently intended. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- coderd/userauth.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/userauth.go b/coderd/userauth.go index 8b23caa550975..6b2aab6c533dd 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -1358,7 +1358,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { }) return } - logger.Warn(ctx, "allowing unverified oidc email %q") + logger.Warn(ctx, "allowing unverified oidc email", slog.F("email", email)) } }