Skip to content

Commit 3db810c

Browse files
jdomeracki-coderf0ssel
jdomeracki-coder
and
f0ssel
authored
fix!: reject OIDC login when email_verified claim is non-bool or absent (#25713) (#26273)
Backport of #25713 to `release/2.29`. Cherry-picked with `git cherry-pick -x` (`d5b0e93c6c`); the commit message references the original PR. > [!NOTE] > Breaking change. See the original PR for the breaking-change details. <details> <summary>Cherry-pick conflict resolution</summary> The `coerceEmailVerified` helper, the FakeIDP default, and the new tests applied cleanly. `coderd/userauth.go` needed: - the fail-closed `email_verified` check resolved in favor of the original PR (this branch previously used a bool type assertion with `httpapi.Write`); - the `site` import added, and the "Email not verified" error rendered with this branch's `ErrorPageData` API (`AdditionalButtonLink` / `AdditionalButtonText`), because the `Actions` field does not exist on this branch. Build, `TestCoerceEmailVerified`, and the OIDC integration cases pass. </details> _Generated by Coder Agents on behalf of @jdomeracki-coder._ Co-authored-by: Garrett Delfosse <garrett@coder.com>
1 parent 069f6cf commit 3db810c

6 files changed

Lines changed: 264 additions & 35 deletions

File tree

‎coderd/coderdtest/oidctest/idp.go‎

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -213,8 +213,9 @@ type FakeIDP struct {
213213
hookAuthenticateClient func(t testing.TB, req *http.Request) (url.Values, error)
214214
serve bool
215215
// optional middlewares
216-
middlewares chi.Middlewares
217-
defaultExpire time.Duration
216+
middlewares chi.Middlewares
217+
defaultExpire time.Duration
218+
omitEmailVerifiedDefault bool
218219
}
219220

220221
func StatusError(code int, err error) error {
@@ -369,6 +370,15 @@ func WithIssuer(issuer string) func(*FakeIDP) {
369370
}
370371
}
371372

373+
// WithOmitEmailVerifiedDefault suppresses the default email_verified=true
374+
// injection in encodeClaims. Use this for tests that exercise the handler's
375+
// absent-claim rejection path.
376+
func WithOmitEmailVerifiedDefault() func(*FakeIDP) {
377+
return func(f *FakeIDP) {
378+
f.omitEmailVerifiedDefault = true
379+
}
380+
}
381+
372382
type With429Arguments struct {
373383
AllPaths bool
374384
TokenPath bool
@@ -882,6 +892,17 @@ func (f *FakeIDP) encodeClaims(t testing.TB, claims jwt.MapClaims) string {
882892
claims["iss"] = f.locked.Issuer()
883893
}
884894

895+
// Default email_verified to true so that tests that do not care
896+
// about the email_verified flow are not forced to set it.
897+
// Tests that need a different value can set it explicitly.
898+
// Use WithOmitEmailVerifiedDefault() to suppress this default
899+
// for tests that need to exercise the absent-claim path.
900+
if !f.omitEmailVerifiedDefault {
901+
if _, ok := claims["email_verified"]; !ok {
902+
claims["email_verified"] = true
903+
}
904+
}
905+
885906
signed, err := jwt.NewWithClaims(jwt.SigningMethodRS256, claims).SignedString(f.locked.PrivateKey())
886907
require.NoError(t, err)
887908

‎coderd/userauth.go‎

Lines changed: 69 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package coderd
33
import (
44
"context"
55
"database/sql"
6+
"encoding/json"
67
"errors"
78
"fmt"
89
"net/http"
@@ -45,6 +46,7 @@ import (
4546
"github.com/coder/coder/v2/coderd/util/ptr"
4647
"github.com/coder/coder/v2/codersdk"
4748
"github.com/coder/coder/v2/cryptorand"
49+
"github.com/coder/coder/v2/site"
4850
)
4951

5052
type MergedClaimsSource string
@@ -1325,18 +1327,38 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
13251327
return
13261328
}
13271329

1328-
verifiedRaw, ok := mergedClaims["email_verified"]
1329-
if ok {
1330-
verified, ok := verifiedRaw.(bool)
1331-
if ok && !verified {
1332-
if !api.OIDCConfig.IgnoreEmailVerified {
1333-
httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
1334-
Message: fmt.Sprintf("Verify the %q email address on your OIDC provider to authenticate!", email),
1335-
})
1336-
return
1337-
}
1338-
logger.Warn(ctx, "allowing unverified oidc email %q")
1330+
// Determine whether the email is verified. Default to unverified
1331+
// so that a missing claim or an unrecognized type is fail-closed.
1332+
emailVerified := false
1333+
verifiedRaw, hasVerifiedClaim := mergedClaims["email_verified"]
1334+
if hasVerifiedClaim {
1335+
v, coerceOK := coerceEmailVerified(verifiedRaw)
1336+
if coerceOK {
1337+
emailVerified = v
1338+
} else {
1339+
logger.Warn(ctx, "unrecognized email_verified claim type, treating as unverified",
1340+
slog.F("type", fmt.Sprintf("%T", verifiedRaw)),
1341+
slog.F("value", verifiedRaw),
1342+
)
1343+
}
1344+
}
1345+
1346+
if !emailVerified {
1347+
if !api.OIDCConfig.IgnoreEmailVerified {
1348+
site.RenderStaticErrorPage(rw, r, site.ErrorPageData{
1349+
Status: http.StatusForbidden,
1350+
HideStatus: true,
1351+
Title: "Email not verified",
1352+
Description: fmt.Sprintf(
1353+
"Verify the %q email address on your OIDC provider to authenticate!",
1354+
email,
1355+
),
1356+
AdditionalButtonLink: "/login",
1357+
AdditionalButtonText: "Back to login",
1358+
})
1359+
return
13391360
}
1361+
logger.Warn(ctx, "allowing unverified oidc email", slog.F("email", email))
13401362
}
13411363

13421364
// The username is a required property in Coder. We make a best-effort
@@ -2130,3 +2152,39 @@ func wrongLoginTypeHTTPError(user database.LoginType, params database.LoginType)
21302152
params, user, addedMsg),
21312153
}
21322154
}
2155+
2156+
// coerceEmailVerified attempts to convert an OIDC email_verified claim to a
2157+
// boolean. Some IdPs (e.g. SAML-to-OIDC bridges, certain Azure AD B2C
2158+
// configurations) return email_verified as a string ("true"/"false") or a
2159+
// number (1/0) rather than a native JSON boolean. This function handles
2160+
// those variants so that non-bool representations cannot silently bypass
2161+
// the verification check.
2162+
//
2163+
// Returns (value, true) on successful coercion, or (false, false) if the
2164+
// value is nil or an unrecognized type.
2165+
func coerceEmailVerified(v interface{}) (verified bool, ok bool) {
2166+
switch val := v.(type) {
2167+
case bool:
2168+
return val, true
2169+
case string:
2170+
b, err := strconv.ParseBool(val)
2171+
if err != nil {
2172+
return false, false
2173+
}
2174+
return b, true
2175+
case json.Number:
2176+
n, err := val.Int64()
2177+
if err != nil {
2178+
return false, false
2179+
}
2180+
return n != 0, true
2181+
case float64:
2182+
return val != 0, true
2183+
case int64:
2184+
return val != 0, true
2185+
case int:
2186+
return val != 0, true
2187+
default:
2188+
return false, false
2189+
}
2190+
}

‎coderd/userauth_internal_test.go‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
package coderd
2+
3+
import (
4+
"encoding/json"
5+
"testing"
6+
7+
"github.com/stretchr/testify/assert"
8+
)
9+
10+
func TestCoerceEmailVerified(t *testing.T) {
11+
t.Parallel()
12+
13+
tests := []struct {
14+
name string
15+
input interface{}
16+
wantBool bool
17+
wantOK bool
18+
}{
19+
// Native booleans
20+
{name: "BoolTrue", input: true, wantBool: true, wantOK: true},
21+
{name: "BoolFalse", input: false, wantBool: false, wantOK: true},
22+
23+
// Strings
24+
{name: "StringTrue", input: "true", wantBool: true, wantOK: true},
25+
{name: "StringFalse", input: "false", wantBool: false, wantOK: true},
26+
{name: "StringOne", input: "1", wantBool: true, wantOK: true},
27+
{name: "StringZero", input: "0", wantBool: false, wantOK: true},
28+
{name: "StringTRUE", input: "TRUE", wantBool: true, wantOK: true},
29+
{name: "StringFALSE", input: "FALSE", wantBool: false, wantOK: true},
30+
{name: "StringT", input: "t", wantBool: true, wantOK: true},
31+
{name: "StringF", input: "f", wantBool: false, wantOK: true},
32+
{name: "StringInvalid", input: "invalid", wantBool: false, wantOK: false},
33+
{name: "StringEmpty", input: "", wantBool: false, wantOK: false},
34+
35+
// json.Number (when decoder uses UseNumber)
36+
{name: "JSONNumberOne", input: json.Number("1"), wantBool: true, wantOK: true},
37+
{name: "JSONNumberZero", input: json.Number("0"), wantBool: false, wantOK: true},
38+
{name: "JSONNumberInvalid", input: json.Number("abc"), wantBool: false, wantOK: false},
39+
40+
// float64 (default JSON numeric type)
41+
{name: "Float64One", input: float64(1), wantBool: true, wantOK: true},
42+
{name: "Float64Zero", input: float64(0), wantBool: false, wantOK: true},
43+
44+
// Integer types
45+
{name: "IntOne", input: int(1), wantBool: true, wantOK: true},
46+
{name: "IntZero", input: int(0), wantBool: false, wantOK: true},
47+
{name: "Int64One", input: int64(1), wantBool: true, wantOK: true},
48+
{name: "Int64Zero", input: int64(0), wantBool: false, wantOK: true},
49+
50+
// Nil and unsupported types
51+
{name: "Nil", input: nil, wantBool: false, wantOK: false},
52+
{name: "Slice", input: []string{}, wantBool: false, wantOK: false},
53+
{name: "Map", input: map[string]string{}, wantBool: false, wantOK: false},
54+
}
55+
56+
for _, tc := range tests {
57+
t.Run(tc.name, func(t *testing.T) {
58+
t.Parallel()
59+
60+
gotBool, gotOK := coerceEmailVerified(tc.input)
61+
assert.Equal(t, tc.wantBool, gotBool, "bool value mismatch")
62+
assert.Equal(t, tc.wantOK, gotOK, "ok value mismatch")
63+
})
64+
}
65+
}

0 commit comments

Comments
 (0)