Skip to content

Commit 335df24

Browse files
jdomeracki-coderf0ssel
jdomeracki-coder
and
f0ssel
authored
fix!: reject OIDC login when email_verified claim is non-bool or absent (#25713) (#26182)
Backport of #25713 to `release/2.33`. Cherry-picked with `git cherry-pick -x` (`d5b0e93c6c`); the commit body references the original PR. _Generated by Coder Agents on behalf of @jdomeracki-coder._ Co-authored-by: Garrett Delfosse <garrett@coder.com>
1 parent 36c011d commit 335df24

6 files changed

Lines changed: 264 additions & 44 deletions

File tree

‎coderd/coderdtest/oidctest/idp.go‎

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -216,8 +216,9 @@ type FakeIDP struct {
216216
hookAuthenticateClient func(t testing.TB, req *http.Request) (url.Values, error)
217217
serve bool
218218
// optional middlewares
219-
middlewares chi.Middlewares
220-
defaultExpire time.Duration
219+
middlewares chi.Middlewares
220+
defaultExpire time.Duration
221+
omitEmailVerifiedDefault bool
221222
}
222223

223224
func StatusError(code int, err error) error {
@@ -378,6 +379,15 @@ func WithIssuer(issuer string) func(*FakeIDP) {
378379
}
379380
}
380381

382+
// WithOmitEmailVerifiedDefault suppresses the default email_verified=true
383+
// injection in encodeClaims. Use this for tests that exercise the handler's
384+
// absent-claim rejection path.
385+
func WithOmitEmailVerifiedDefault() func(*FakeIDP) {
386+
return func(f *FakeIDP) {
387+
f.omitEmailVerifiedDefault = true
388+
}
389+
}
390+
381391
type With429Arguments struct {
382392
AllPaths bool
383393
TokenPath bool
@@ -907,6 +917,17 @@ func (f *FakeIDP) encodeClaims(t testing.TB, claims jwt.MapClaims) string {
907917
claims["iss"] = f.locked.Issuer()
908918
}
909919

920+
// Default email_verified to true so that tests that do not care
921+
// about the email_verified flow are not forced to set it.
922+
// Tests that need a different value can set it explicitly.
923+
// Use WithOmitEmailVerifiedDefault() to suppress this default
924+
// for tests that need to exercise the absent-claim path.
925+
if !f.omitEmailVerifiedDefault {
926+
if _, ok := claims["email_verified"]; !ok {
927+
claims["email_verified"] = true
928+
}
929+
}
930+
910931
signed, err := jwt.NewWithClaims(jwt.SigningMethodRS256, claims).SignedString(f.locked.PrivateKey())
911932
require.NoError(t, err)
912933

‎coderd/userauth.go‎

Lines changed: 69 additions & 20 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"
@@ -1348,29 +1349,41 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
13481349
return
13491350
}
13501351

1351-
verifiedRaw, ok := mergedClaims["email_verified"]
1352-
if ok {
1353-
verified, ok := verifiedRaw.(bool)
1354-
if ok && !verified {
1355-
if !api.OIDCConfig.IgnoreEmailVerified {
1356-
site.RenderStaticErrorPage(rw, r, site.ErrorPageData{
1357-
Status: http.StatusForbidden,
1358-
HideStatus: true,
1359-
Title: "Email not verified",
1360-
Description: fmt.Sprintf(
1361-
"Verify the %q email address on your OIDC provider to authenticate!",
1362-
email,
1363-
),
1364-
Actions: []site.Action{
1365-
{URL: "/login", Text: "Back to login"},
1366-
},
1367-
})
1368-
return
1369-
}
1370-
logger.Warn(ctx, "allowing unverified oidc email", slog.F("email", email))
1352+
// Determine whether the email is verified. Default to unverified
1353+
// so that a missing claim or an unrecognized type is fail-closed.
1354+
emailVerified := false
1355+
verifiedRaw, hasVerifiedClaim := mergedClaims["email_verified"]
1356+
if hasVerifiedClaim {
1357+
v, coerceOK := coerceEmailVerified(verifiedRaw)
1358+
if coerceOK {
1359+
emailVerified = v
1360+
} else {
1361+
logger.Warn(ctx, "unrecognized email_verified claim type, treating as unverified",
1362+
slog.F("type", fmt.Sprintf("%T", verifiedRaw)),
1363+
slog.F("value", verifiedRaw),
1364+
)
13711365
}
13721366
}
13731367

1368+
if !emailVerified {
1369+
if !api.OIDCConfig.IgnoreEmailVerified {
1370+
site.RenderStaticErrorPage(rw, r, site.ErrorPageData{
1371+
Status: http.StatusForbidden,
1372+
HideStatus: true,
1373+
Title: "Email not verified",
1374+
Description: fmt.Sprintf(
1375+
"Verify the %q email address on your OIDC provider to authenticate!",
1376+
email,
1377+
),
1378+
Actions: []site.Action{
1379+
{URL: "/login", Text: "Back to login"},
1380+
},
1381+
})
1382+
return
1383+
}
1384+
logger.Warn(ctx, "allowing unverified oidc email", slog.F("email", email))
1385+
}
1386+
13741387
// The username is a required property in Coder. We make a best-effort
13751388
// attempt at using what the claims provide, but if that fails we will
13761389
// generate a random username.
@@ -2235,3 +2248,39 @@ func wrongLoginTypeHTTPError(user database.LoginType, params database.LoginType)
22352248
params, user, addedMsg),
22362249
}
22372250
}
2251+
2252+
// coerceEmailVerified attempts to convert an OIDC email_verified claim to a
2253+
// boolean. Some IdPs (e.g. SAML-to-OIDC bridges, certain Azure AD B2C
2254+
// configurations) return email_verified as a string ("true"/"false") or a
2255+
// number (1/0) rather than a native JSON boolean. This function handles
2256+
// those variants so that non-bool representations cannot silently bypass
2257+
// the verification check.
2258+
//
2259+
// Returns (value, true) on successful coercion, or (false, false) if the
2260+
// value is nil or an unrecognized type.
2261+
func coerceEmailVerified(v interface{}) (verified bool, ok bool) {
2262+
switch val := v.(type) {
2263+
case bool:
2264+
return val, true
2265+
case string:
2266+
b, err := strconv.ParseBool(val)
2267+
if err != nil {
2268+
return false, false
2269+
}
2270+
return b, true
2271+
case json.Number:
2272+
n, err := val.Int64()
2273+
if err != nil {
2274+
return false, false
2275+
}
2276+
return n != 0, true
2277+
case float64:
2278+
return val != 0, true
2279+
case int64:
2280+
return val != 0, true
2281+
case int:
2282+
return val != 0, true
2283+
default:
2284+
return false, false
2285+
}
2286+
}

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