Skip to content

Commit e132b2c

Browse files
oauthex: tighten MatchesResource doc + drop whitespace tolerance
Review feedback from a re-read of RFC 9728 / RFC 3986: - RFC 9728 §3.3 normatively cites RFC 3986 §6.2.1 "simple string comparison" (byte-equal), not the trailing-slash form. The spec basis for slash tolerance is RFC 3986 §6.2.3 (scheme-based normalization), which lists empty-path → "/" among the normalizations permitted *but not required*. Reframe the doc accordingly so reviewers don't have to re-derive the citation. - Drop strings.TrimSpace. No RFC permits whitespace around a JWT aud URI; tolerating it silently accepts a malformed claim instead of surfacing it. If a real IdP ever emits whitespace, that's a bug in the IdP, not something a generic helper should paper over. - Document the §6.2.3 features deliberately NOT applied (host case fold, default-port elision) so callers know two distinct registered resources cannot collide via these normalizations. Added negative tests pinning each boundary. - Point byte-equal-strict callers at the direct == comparison so they know they don't need a separate helper. Behavior change: claims with surrounding whitespace now return false (previously returned true). The only realistic caller — token validation — already gets malformed-aud rejection elsewhere via jose/oidc parsers, so this surfaces upstream bugs rather than hiding them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent fdab699 commit e132b2c

2 files changed

Lines changed: 51 additions & 19 deletions

File tree

oauthex/audience.go

Lines changed: 23 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -6,27 +6,37 @@ package oauthex
66

77
import "strings"
88

9-
// MatchesResource reports whether any of `claims` matches `resource` under
10-
// the canonical-form (trailing slash) tolerance required by RFC 9728
11-
// (Protected Resource Metadata) and RFC 8707 (Resource Indicators). Both
12-
// canonical and non-trailing-slash forms compare equal.
9+
// MatchesResource reports whether any of claims matches resource under
10+
// RFC 3986 §6.2.3 scheme-based normalization, narrowed to the empty-path
11+
// case: a URI with an empty path is treated as equivalent to one with a
12+
// path of "/". All other URI components (scheme, host case, port, query,
13+
// fragment) must match exactly — this is intentionally narrower than the
14+
// full §6.2.3 rung, which would also fold scheme/host case and default
15+
// ports.
1316
//
14-
// Background: RFC 9728 §3.3 canonicalises the protected-resource identifier
15-
// with a trailing slash, but RFC 8707 resource indicators sometimes omit it,
16-
// and upstream IdPs vary in which form they emit in `aud` claims (Google
17-
// trims, Auth0 retains, claude.ai round-trips whichever it received). Strict
18-
// byte equality therefore fails routinely on legitimate setups. This helper
19-
// matches both forms while preserving exact-equality semantics — leading
20-
// whitespace, schemes, paths must still match.
17+
// RFC 9728 §3.3 normatively requires "simple string comparison" per
18+
// RFC 3986 §6.2.1 (byte-equal). Callers that need strict byte-equal
19+
// semantics should compare claims to resource directly:
20+
//
21+
// for _, c := range claims { if c == resource { return true } }
22+
//
23+
// Background: RFC 9728 §3.3 canonicalises the protected-resource
24+
// identifier with a trailing slash, but RFC 8707 resource indicators
25+
// sometimes omit it, and upstream IdPs vary in which form they emit in
26+
// `aud` claims (Google trims, Auth0 retains, claude.ai round-trips
27+
// whichever it received). Strict byte equality therefore fails routinely
28+
// on legitimate setups; this helper is the most common pragmatic relaxation
29+
// while keeping path/scheme/host strict so token confusion across distinct
30+
// resources still fails closed.
2131
//
2232
// Returns false when claims is empty.
2333
func MatchesResource(claims []string, resource string) bool {
24-
expected := strings.TrimRight(strings.TrimSpace(resource), "/")
34+
expected := strings.TrimRight(resource, "/")
2535
for _, c := range claims {
2636
if c == resource {
2737
return true
2838
}
29-
if strings.TrimRight(strings.TrimSpace(c), "/") == expected {
39+
if strings.TrimRight(c, "/") == expected {
3040
return true
3141
}
3242
}

oauthex/audience_test.go

Lines changed: 28 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -31,12 +31,6 @@ func TestMatchesResource(t *testing.T) {
3131
resource: "https://mcp.example.com/",
3232
want: true,
3333
},
34-
{
35-
name: "claim with surrounding whitespace",
36-
claims: []string{" https://mcp.example.com/ "},
37-
resource: "https://mcp.example.com",
38-
want: true,
39-
},
4034
{
4135
name: "multiple claims, one matches",
4236
claims: []string{"https://other.example.com/", "https://mcp.example.com"},
@@ -67,6 +61,34 @@ func TestMatchesResource(t *testing.T) {
6761
resource: "https://mcp.example.com/",
6862
want: false,
6963
},
64+
// The following document the intentional boundaries: §6.2.3 also
65+
// permits scheme/host case folding and default-port elision, but
66+
// MatchesResource deliberately does NOT, so two distinct
67+
// registered resources cannot collide via these normalizations.
68+
{
69+
name: "host case difference is not tolerated",
70+
claims: []string{"https://MCP.example.com/"},
71+
resource: "https://mcp.example.com/",
72+
want: false,
73+
},
74+
{
75+
name: "default-port elision is not tolerated",
76+
claims: []string{"https://mcp.example.com:443/"},
77+
resource: "https://mcp.example.com/",
78+
want: false,
79+
},
80+
{
81+
name: "query string difference is not tolerated",
82+
claims: []string{"https://mcp.example.com/?x=y"},
83+
resource: "https://mcp.example.com/",
84+
want: false,
85+
},
86+
{
87+
name: "surrounding whitespace is not tolerated (malformed claim)",
88+
claims: []string{" https://mcp.example.com/ "},
89+
resource: "https://mcp.example.com/",
90+
want: false,
91+
},
7092
}
7193
for _, tt := range tests {
7294
t.Run(tt.name, func(t *testing.T) {

0 commit comments

Comments
 (0)