Skip to content

Commit f0117a5

Browse files
rliebzericchiang
authored andcommitted
oidc: use %w verb for wrapping errors
Errors wrapped using %v cannot be unwrapped. This means if there is an underlying error such as `context.Canceled`, a caller cannot reliably discover that error using any method other than string comparison. By swapping to the %w verb, `errors.Is` and `errors.As` become valuable tools for error discovery and behavior differentiation. While by convention, some of the errors like `fetching keys %v` should probably be changed to `fetching keys: %w` (with the `:` character), this change opts not to do that to help preserve backward compatibility for external error handling that uses string comparison today.
1 parent b203e58 commit f0117a5

File tree

2 files changed

+39
-3
lines changed

2 files changed

+39
-3
lines changed

oidc/jwks.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,7 @@ func (r *RemoteKeySet) verify(ctx context.Context, jws *jose.JSONWebSignature) (
159159
// https://openid.net/specs/openid-connect-core-1_0.html#RotateSigKeys
160160
keys, err := r.keysFromRemote(ctx)
161161
if err != nil {
162-
return nil, fmt.Errorf("fetching keys %v", err)
162+
return nil, fmt.Errorf("fetching keys %w", err)
163163
}
164164

165165
for _, key := range keys {
@@ -228,7 +228,7 @@ func (r *RemoteKeySet) updateKeys() ([]jose.JSONWebKey, error) {
228228

229229
resp, err := doRequest(r.ctx, req)
230230
if err != nil {
231-
return nil, fmt.Errorf("oidc: get keys failed %v", err)
231+
return nil, fmt.Errorf("oidc: get keys failed %w", err)
232232
}
233233
defer resp.Body.Close()
234234

oidc/jwks_test.go

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"crypto/rand"
1010
"crypto/rsa"
1111
"encoding/json"
12+
"errors"
1213
"fmt"
1314
"net/http"
1415
"net/http/httptest"
@@ -138,6 +139,41 @@ func TestMismatchedKeyID(t *testing.T) {
138139
testKeyVerify(t, key2, bad, key1, key2)
139140
}
140141

142+
func TestKeyVerifyContextCanceled(t *testing.T) {
143+
ctx, cancel := context.WithCancel(context.Background())
144+
defer cancel()
145+
146+
payload := []byte("a secret")
147+
148+
good := newECDSAKey(t)
149+
jws, err := jose.ParseSigned(good.sign(t, payload))
150+
if err != nil {
151+
t.Fatal(err)
152+
}
153+
154+
ch := make(chan struct{})
155+
defer close(ch)
156+
157+
s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
158+
<-ch
159+
}))
160+
defer s.Close()
161+
162+
rks := newRemoteKeySet(ctx, s.URL, nil)
163+
164+
cancel()
165+
166+
// Ensure the token verifies.
167+
_, err = rks.verify(ctx, jws)
168+
if err == nil {
169+
t.Fatal("expected context canceled, got nil error")
170+
}
171+
172+
if !errors.Is(err, context.Canceled) {
173+
t.Errorf("expected error to be %q got %q", context.Canceled, err)
174+
}
175+
}
176+
141177
func testKeyVerify(t *testing.T, good, bad *signingKey, verification ...*signingKey) {
142178
ctx, cancel := context.WithCancel(context.Background())
143179
defer cancel()
@@ -259,7 +295,7 @@ func BenchmarkVerify(b *testing.B) {
259295

260296
key := newRSAKey(b)
261297

262-
now := time.Date(2022, 01, 29, 0, 0, 0, 0, time.UTC)
298+
now := time.Date(2022, 1, 29, 0, 0, 0, 0, time.UTC)
263299
exp := now.Add(time.Hour)
264300
payload := []byte(fmt.Sprintf(`{
265301
"iss": "https://example.com",

0 commit comments

Comments
 (0)