mirror of
https://github.com/coder/coder.git
synced 2026-10-16 14:48:56 +08:00
fix: match OAuth2 redirect_uri against every registered redirect URI (#29014)
- Fix OAuth2 redirect validation to accept any registered URI, resolving Cursor’s rejection when using its second callback (fixes #28910; stacked on #29013). - Deduplicate primary and alternate callbacks, validate every registered entry, preserve primary-callback defaults and existing code-to-token redirect checks, and stop exposing registered URLs in errors. - Add regression tests and update documentation; defer admin removal of registered URIs (PLAT-619) and requiring redirect_uri when multiple URIs are registered (PLAT-620) to follow-ups.
This commit is contained in:
@@ -6,6 +6,7 @@ import (
|
||||
"errors"
|
||||
"fmt"
|
||||
"net/url"
|
||||
"slices"
|
||||
"strconv"
|
||||
"strings"
|
||||
"time"
|
||||
@@ -219,25 +220,28 @@ func (p *QueryParamParser) UUIDs(vals url.Values, def []uuid.UUID, queryParam st
|
||||
})
|
||||
}
|
||||
|
||||
func (p *QueryParamParser) RedirectURL(vals url.Values, base *url.URL, queryParam string) *url.URL {
|
||||
v, err := parseQueryParam(p, vals, url.Parse, base, queryParam)
|
||||
// RedirectURL parses queryParam as a URL and requires it to match the primary
|
||||
// callback or one of the alternates. When the param is absent it returns primary.
|
||||
func (p *QueryParamParser) RedirectURL(vals url.Values, primary *url.URL, alternates []*url.URL, queryParam string) *url.URL {
|
||||
v, err := parseQueryParam(p, vals, url.Parse, primary, queryParam)
|
||||
if err != nil {
|
||||
p.Errors = append(p.Errors, codersdk.ValidationError{
|
||||
Field: queryParam,
|
||||
Detail: fmt.Sprintf("Query param %q must be a valid url: %s", queryParam, err.Error()),
|
||||
})
|
||||
// url.Parse returns a nil URL alongside its error, so the comparison
|
||||
// below would panic. base stands in: p.Errors is already non-empty, so
|
||||
// every caller rejects the request before reading this.
|
||||
return base
|
||||
// below would panic. primary stands in: p.Errors is already non-empty,
|
||||
// so every caller rejects the request before reading this.
|
||||
return primary
|
||||
}
|
||||
|
||||
// OAuth 2.1 §2.3.1 requires an exact match. RFC 8252 §7.3 excepts the port
|
||||
// of a loopback redirect URI; the comparator owns that rule.
|
||||
if !codersdk.RedirectURIMatches(v, base) {
|
||||
// OAuth 2.1 §2.3.1 requires an exact match against a registered URI. RFC 8252
|
||||
// §7.3 excepts the port of a loopback redirect URI; the comparator owns that rule.
|
||||
matches := func(a *url.URL) bool { return codersdk.RedirectURIMatches(v, a) }
|
||||
if !matches(primary) && !slices.ContainsFunc(alternates, matches) {
|
||||
p.Errors = append(p.Errors, codersdk.ValidationError{
|
||||
Field: queryParam,
|
||||
Detail: fmt.Sprintf("Query param %q must match %s; only the port of a loopback URI may differ", queryParam, base),
|
||||
Detail: fmt.Sprintf("Query param %q must match one of the application's registered redirect URIs; only the port of a loopback URI may differ", queryParam),
|
||||
})
|
||||
}
|
||||
|
||||
|
||||
@@ -596,7 +596,7 @@ func TestRedirectURL(t *testing.T) {
|
||||
t.Run("Omitted", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
parser := httpapi.NewQueryParamParser()
|
||||
got := parser.RedirectURL(url.Values{}, base, "redirect_uri")
|
||||
got := parser.RedirectURL(url.Values{}, base, nil, "redirect_uri")
|
||||
require.Empty(t, parser.Errors)
|
||||
require.Equal(t, base.String(), got.String())
|
||||
})
|
||||
@@ -605,7 +605,7 @@ func TestRedirectURL(t *testing.T) {
|
||||
t.Parallel()
|
||||
parser := httpapi.NewQueryParamParser()
|
||||
vals := url.Values{"redirect_uri": []string{base.String()}}
|
||||
got := parser.RedirectURL(vals, base, "redirect_uri")
|
||||
got := parser.RedirectURL(vals, base, nil, "redirect_uri")
|
||||
require.Empty(t, parser.Errors)
|
||||
require.Equal(t, base.String(), got.String())
|
||||
})
|
||||
@@ -614,7 +614,7 @@ func TestRedirectURL(t *testing.T) {
|
||||
t.Parallel()
|
||||
parser := httpapi.NewQueryParamParser()
|
||||
vals := url.Values{"redirect_uri": []string{"https://evil.example.com/steal"}}
|
||||
parser.RedirectURL(vals, base, "redirect_uri")
|
||||
parser.RedirectURL(vals, base, nil, "redirect_uri")
|
||||
require.Len(t, parser.Errors, 1)
|
||||
require.Contains(t, parser.Errors[0].Detail, "must match")
|
||||
})
|
||||
@@ -627,7 +627,7 @@ func TestRedirectURL(t *testing.T) {
|
||||
parser := httpapi.NewQueryParamParser()
|
||||
vals := url.Values{"redirect_uri": []string{raw}}
|
||||
require.NotPanics(t, func() {
|
||||
got := parser.RedirectURL(vals, base, "redirect_uri")
|
||||
got := parser.RedirectURL(vals, base, nil, "redirect_uri")
|
||||
require.NotNil(t, got, "a nil URL would panic in the caller")
|
||||
require.Equal(t, base.String(), got.String())
|
||||
}, "redirect_uri=%q must not panic", raw)
|
||||
@@ -648,7 +648,7 @@ func TestRedirectURL(t *testing.T) {
|
||||
|
||||
parser := httpapi.NewQueryParamParser()
|
||||
vals := url.Values{"redirect_uri": []string{presented}}
|
||||
got := parser.RedirectURL(vals, registered, "redirect_uri")
|
||||
got := parser.RedirectURL(vals, registered, nil, "redirect_uri")
|
||||
require.Empty(t, parser.Errors, "host %s", host)
|
||||
require.Equal(t, presented, got.String(), "host %s", host)
|
||||
}
|
||||
@@ -662,7 +662,7 @@ func TestRedirectURL(t *testing.T) {
|
||||
|
||||
parser := httpapi.NewQueryParamParser()
|
||||
vals := url.Values{"redirect_uri": []string{presented}}
|
||||
got := parser.RedirectURL(vals, registered, "redirect_uri")
|
||||
got := parser.RedirectURL(vals, registered, nil, "redirect_uri")
|
||||
require.Empty(t, parser.Errors)
|
||||
require.Equal(t, presented, got.String())
|
||||
})
|
||||
@@ -675,10 +675,10 @@ func TestRedirectURL(t *testing.T) {
|
||||
require.NoError(t, err)
|
||||
parser := httpapi.NewQueryParamParser()
|
||||
vals := url.Values{"redirect_uri": []string{"http://127.0.0.1:53219/other"}}
|
||||
parser.RedirectURL(vals, registered, "redirect_uri")
|
||||
parser.RedirectURL(vals, registered, nil, "redirect_uri")
|
||||
require.Len(t, parser.Errors, 1)
|
||||
require.Equal(t, "redirect_uri", parser.Errors[0].Field)
|
||||
require.Contains(t, parser.Errors[0].Detail, "must match")
|
||||
require.Contains(t, parser.Errors[0].Detail, "must match one of")
|
||||
})
|
||||
|
||||
// A non-loopback registration keeps the exact match, port included.
|
||||
@@ -686,8 +686,41 @@ func TestRedirectURL(t *testing.T) {
|
||||
t.Parallel()
|
||||
parser := httpapi.NewQueryParamParser()
|
||||
vals := url.Values{"redirect_uri": []string{"https://app.example.com:8443/callback"}}
|
||||
parser.RedirectURL(vals, base, "redirect_uri")
|
||||
parser.RedirectURL(vals, base, nil, "redirect_uri")
|
||||
require.Len(t, parser.Errors, 1)
|
||||
require.Contains(t, parser.Errors[0].Detail, "must match")
|
||||
})
|
||||
|
||||
// RFC 6749 §3.1.2.3: the presented URI must match one of the registered URIs,
|
||||
// not only the first.
|
||||
t.Run("MatchesAnyRegisteredEntry", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
second, err := url.Parse("https://www.cursor.com/agents/mcp/oauth/callback")
|
||||
require.NoError(t, err)
|
||||
alternates := []*url.URL{second}
|
||||
|
||||
for _, want := range []*url.URL{base, second} {
|
||||
parser := httpapi.NewQueryParamParser()
|
||||
vals := url.Values{"redirect_uri": []string{want.String()}}
|
||||
got := parser.RedirectURL(vals, base, alternates, "redirect_uri")
|
||||
require.Empty(t, parser.Errors, want.String())
|
||||
require.Equal(t, want.String(), got.String())
|
||||
}
|
||||
|
||||
parser := httpapi.NewQueryParamParser()
|
||||
vals := url.Values{"redirect_uri": []string{"https://evil.example.com/steal"}}
|
||||
parser.RedirectURL(vals, base, alternates, "redirect_uri")
|
||||
require.Len(t, parser.Errors, 1)
|
||||
require.Contains(t, parser.Errors[0].Detail, "must match one of")
|
||||
})
|
||||
|
||||
t.Run("OmittedWithSeveralRegisteredDefaultsToPrimary", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
second, err := url.Parse("https://www.cursor.com/agents/mcp/oauth/callback")
|
||||
require.NoError(t, err)
|
||||
parser := httpapi.NewQueryParamParser()
|
||||
got := parser.RedirectURL(url.Values{}, base, []*url.URL{second}, "redirect_uri")
|
||||
require.Empty(t, parser.Errors)
|
||||
require.Equal(t, base.String(), got.String())
|
||||
})
|
||||
}
|
||||
|
||||
@@ -307,7 +307,7 @@ func extractAuthorizeParams(r *http.Request, logger slog.Logger, app database.OA
|
||||
// response_type and client_id are always required.
|
||||
p.RequiredNotEmpty("response_type", "client_id")
|
||||
|
||||
response, err := newAuthorizeResponse(p, vals, app.CallbackURL)
|
||||
response, err := newAuthorizeResponse(p, vals, app)
|
||||
if err != nil {
|
||||
return authorizeParams{}, &authorizeFailure{corruptCallback: err}
|
||||
}
|
||||
@@ -445,11 +445,31 @@ type authorizeResponse struct {
|
||||
state string
|
||||
}
|
||||
|
||||
// newAuthorizeResponse parses the app's registered callback, checks it,
|
||||
// exact-matches any redirect_uri the client sent against it, and reads the
|
||||
// state to echo back.
|
||||
// registeredRedirectURIs returns the app's primary callback and its other
|
||||
// registered redirect URIs, parsed and deduplicated. CallbackURL is the primary
|
||||
// because admin-created apps have an empty RedirectUris list.
|
||||
func registeredRedirectURIs(app database.OAuth2ProviderApp) (primary *url.URL, alternates []*url.URL, err error) {
|
||||
primary, err = url.Parse(app.CallbackURL)
|
||||
if err != nil {
|
||||
return nil, nil, xerrors.Errorf("parse callback URL %q: %w", app.CallbackURL, err)
|
||||
}
|
||||
for _, s := range slice.Unique(app.RedirectUris) {
|
||||
if s == app.CallbackURL {
|
||||
continue
|
||||
}
|
||||
u, err := url.Parse(s)
|
||||
if err != nil {
|
||||
return nil, nil, xerrors.Errorf("parse registered redirect URI %q: %w", s, err)
|
||||
}
|
||||
alternates = append(alternates, u)
|
||||
}
|
||||
return primary, alternates, nil
|
||||
}
|
||||
|
||||
// newAuthorizeResponse checks the app's registered redirect URIs, matches any
|
||||
// redirect_uri the client sent against them, and reads the state to echo back.
|
||||
//
|
||||
// The scheme is checked on the registered URL rather than on the match's result,
|
||||
// The scheme is checked on the registered URLs rather than on the match's result,
|
||||
// because p.RedirectURL returns the client's URI when the match fails, and
|
||||
// answering 500 for a scheme the client chose would blame the app for a request
|
||||
// it did not make. It is checked before the match so no parse outcome can reach
|
||||
@@ -458,16 +478,18 @@ type authorizeResponse struct {
|
||||
// A returned error means the registration itself is unusable, which is server
|
||||
// state. A mismatch is the client's mistake and joins the other parameter
|
||||
// failures in p.Errors.
|
||||
func newAuthorizeResponse(p *httpapi.QueryParamParser, vals url.Values, registered string) (authorizeResponse, error) {
|
||||
registeredURL, err := url.Parse(registered)
|
||||
func newAuthorizeResponse(p *httpapi.QueryParamParser, vals url.Values, app database.OAuth2ProviderApp) (authorizeResponse, error) {
|
||||
primary, alternates, err := registeredRedirectURIs(app)
|
||||
if err != nil {
|
||||
return authorizeResponse{}, err
|
||||
}
|
||||
if err := codersdk.ValidateRedirectURIScheme(registeredURL); err != nil {
|
||||
return authorizeResponse{}, err
|
||||
for _, u := range append([]*url.URL{primary}, alternates...) {
|
||||
if err := codersdk.ValidateRedirectURIScheme(u); err != nil {
|
||||
return authorizeResponse{}, err
|
||||
}
|
||||
}
|
||||
|
||||
callback := p.RedirectURL(vals, registeredURL, "redirect_uri")
|
||||
callback := p.RedirectURL(vals, primary, alternates, "redirect_uri")
|
||||
response := authorizeResponse{state: p.String(vals, "", "state")}
|
||||
// The field, not a count of errors across these two lines: reading state
|
||||
// can fail too, and that failure belongs to the client's callback rather
|
||||
|
||||
@@ -514,6 +514,10 @@ func TestConsentScopes(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func appWithCallback(callback string) database.OAuth2ProviderApp {
|
||||
return database.OAuth2ProviderApp{CallbackURL: callback}
|
||||
}
|
||||
|
||||
// TestNewAuthorizeResponse covers the two preconditions the constructor exists
|
||||
// to run together, and which of them is the server's fault.
|
||||
func TestNewAuthorizeResponse(t *testing.T) {
|
||||
@@ -528,7 +532,7 @@ func TestNewAuthorizeResponse(t *testing.T) {
|
||||
response, err := newAuthorizeResponse(p, url.Values{
|
||||
"redirect_uri": {registered},
|
||||
"state": {"abc123"},
|
||||
}, registered)
|
||||
}, appWithCallback(registered))
|
||||
|
||||
require.NoError(t, err)
|
||||
require.Empty(t, p.Errors)
|
||||
@@ -541,7 +545,7 @@ func TestNewAuthorizeResponse(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
p := httpapi.NewQueryParamParser()
|
||||
response, err := newAuthorizeResponse(p, url.Values{}, registered)
|
||||
response, err := newAuthorizeResponse(p, url.Values{}, appWithCallback(registered))
|
||||
|
||||
require.NoError(t, err)
|
||||
require.Empty(t, p.Errors)
|
||||
@@ -555,7 +559,7 @@ func TestNewAuthorizeResponse(t *testing.T) {
|
||||
p := httpapi.NewQueryParamParser()
|
||||
response, err := newAuthorizeResponse(p, url.Values{
|
||||
"redirect_uri": {"https://elsewhere.example/cb"},
|
||||
}, registered)
|
||||
}, appWithCallback(registered))
|
||||
|
||||
// The client's mistake, so it joins the parser's other errors rather
|
||||
// than becoming a server fault.
|
||||
@@ -572,7 +576,7 @@ func TestNewAuthorizeResponse(t *testing.T) {
|
||||
p := httpapi.NewQueryParamParser()
|
||||
response, err := newAuthorizeResponse(p, url.Values{
|
||||
"redirect_uri": {"javascript:alert(1)"},
|
||||
}, registered)
|
||||
}, appWithCallback(registered))
|
||||
|
||||
require.NoError(t, err, "the app registered a usable callback; the client did not send one")
|
||||
require.NotEmpty(t, p.Errors)
|
||||
@@ -583,7 +587,7 @@ func TestNewAuthorizeResponse(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
p := httpapi.NewQueryParamParser()
|
||||
response, err := newAuthorizeResponse(p, url.Values{}, "javascript:alert(1)")
|
||||
response, err := newAuthorizeResponse(p, url.Values{}, appWithCallback("javascript:alert(1)"))
|
||||
|
||||
require.Error(t, err)
|
||||
require.Empty(t, p.Errors, "the registration is rejected before any parameter is read")
|
||||
@@ -594,7 +598,7 @@ func TestNewAuthorizeResponse(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
p := httpapi.NewQueryParamParser()
|
||||
response, err := newAuthorizeResponse(p, url.Values{}, "http://a b")
|
||||
response, err := newAuthorizeResponse(p, url.Values{}, appWithCallback("http://a b"))
|
||||
|
||||
require.Error(t, err, "a registration that does not parse is the same class as one this server rejects")
|
||||
require.Empty(t, p.Errors)
|
||||
@@ -611,7 +615,7 @@ func TestNewAuthorizeResponse(t *testing.T) {
|
||||
response, err := newAuthorizeResponse(p, url.Values{
|
||||
"redirect_uri": {presented},
|
||||
"state": {"abc123"},
|
||||
}, "http://127.0.0.1/callback")
|
||||
}, appWithCallback("http://127.0.0.1/callback"))
|
||||
|
||||
require.NoError(t, err)
|
||||
require.Empty(t, p.Errors)
|
||||
@@ -752,3 +756,65 @@ func TestCarveOutDelivery(t *testing.T) {
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestRegisteredRedirectURIs(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
strs := func(t *testing.T, app database.OAuth2ProviderApp) []string {
|
||||
t.Helper()
|
||||
primary, alternates, err := registeredRedirectURIs(app)
|
||||
require.NoError(t, err)
|
||||
out := []string{primary.String()}
|
||||
for _, u := range alternates {
|
||||
out = append(out, u.String())
|
||||
}
|
||||
return out
|
||||
}
|
||||
|
||||
t.Run("AdminAppHasOnlyTheCallback", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
got := strs(t, database.OAuth2ProviderApp{
|
||||
CallbackURL: "https://app.example.com/callback",
|
||||
RedirectUris: []string{},
|
||||
})
|
||||
require.Equal(t, []string{"https://app.example.com/callback"}, got)
|
||||
})
|
||||
|
||||
t.Run("PrimaryFirstAndDeduplicated", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
got := strs(t, database.OAuth2ProviderApp{
|
||||
CallbackURL: "cursor://anysphere.cursor-mcp/oauth/callback",
|
||||
RedirectUris: []string{
|
||||
"cursor://anysphere.cursor-mcp/oauth/callback",
|
||||
"https://www.cursor.com/agents/mcp/oauth/callback",
|
||||
},
|
||||
})
|
||||
require.Equal(t, []string{
|
||||
"cursor://anysphere.cursor-mcp/oauth/callback",
|
||||
"https://www.cursor.com/agents/mcp/oauth/callback",
|
||||
}, got)
|
||||
})
|
||||
|
||||
// An admin edit rewrites CallbackURL without touching RedirectUris.
|
||||
t.Run("EditedCallbackIsIncluded", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
got := strs(t, database.OAuth2ProviderApp{
|
||||
CallbackURL: "https://new.example.com/callback",
|
||||
RedirectUris: []string{"https://a.example.com/cb", "https://b.example.com/cb"},
|
||||
})
|
||||
require.Equal(t, []string{
|
||||
"https://new.example.com/callback",
|
||||
"https://a.example.com/cb",
|
||||
"https://b.example.com/cb",
|
||||
}, got)
|
||||
})
|
||||
|
||||
t.Run("UnparsableEntryIsAnError", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
_, _, err := registeredRedirectURIs(database.OAuth2ProviderApp{
|
||||
CallbackURL: "https://app.example.com/callback",
|
||||
RedirectUris: []string{"http://a b"},
|
||||
})
|
||||
require.Error(t, err)
|
||||
})
|
||||
}
|
||||
|
||||
@@ -1120,3 +1120,66 @@ func TestOAuth2AuthorizeLoopbackRedirectPort(t *testing.T) {
|
||||
requireTokenResponse(t, status, body)
|
||||
})
|
||||
}
|
||||
|
||||
// RFC 6749 §3.1.2.3: the presented redirect_uri must match one of the
|
||||
// registered URIs, not only the first. Cursor registers a desktop deep link
|
||||
// and a web callback and presents the second.
|
||||
func TestOAuth2AuthorizeAnyRegisteredRedirectURI(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
client := coderdtest.New(t, nil)
|
||||
_ = coderdtest.CreateFirstUser(t, client)
|
||||
oauth2providertest.EnableDCR(t, client)
|
||||
ctx := testutil.Context(t, testutil.WaitLong)
|
||||
|
||||
const (
|
||||
first = "cursor://anysphere.cursor-mcp/oauth/callback"
|
||||
second = "https://www.cursor.com/agents/mcp/oauth/callback"
|
||||
)
|
||||
app := oauth2providertest.RegisterPublicClientWithRedirectURIs(t, client, "cursor", first, second)
|
||||
|
||||
verifier, challenge := oauth2providertest.GeneratePKCE(t)
|
||||
query := authorizeQuery(t, app.ClientID, "")
|
||||
query.Set("code_challenge", challenge)
|
||||
query.Set("redirect_uri", second)
|
||||
|
||||
get := sendAuthorizeRequest(ctx, t, client, http.MethodGet, query)
|
||||
defer get.Body.Close()
|
||||
require.Equal(t, http.StatusOK, get.StatusCode, readBody(t, get))
|
||||
|
||||
post := sendAuthorizeRequest(ctx, t, client, http.MethodPost, query)
|
||||
defer post.Body.Close()
|
||||
require.Equal(t, http.StatusFound, post.StatusCode, readBody(t, post))
|
||||
location, err := url.Parse(post.Header.Get("Location"))
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, second, location.Scheme+"://"+location.Host+location.Path,
|
||||
"the code must go to the entry that matched")
|
||||
code := location.Query().Get("code")
|
||||
require.NotEmpty(t, code, "authorization did not issue a code")
|
||||
|
||||
exchange := func(redirectURI string) (int, string) {
|
||||
form := url.Values{}
|
||||
form.Set("grant_type", "authorization_code")
|
||||
form.Set("client_id", app.ClientID)
|
||||
form.Set("code", code)
|
||||
form.Set("redirect_uri", redirectURI)
|
||||
form.Set("code_verifier", verifier)
|
||||
return postTokenRequest(ctx, t, client, form)
|
||||
}
|
||||
|
||||
// RFC 6749 §4.1.3: the exchange is bound to the entry the code was issued
|
||||
// to, even though the other entry is also registered.
|
||||
status, body := exchange(first)
|
||||
requireTokenGrantError(t, status, body)
|
||||
|
||||
status, body = exchange(second)
|
||||
requireTokenResponse(t, status, body)
|
||||
|
||||
// An unregistered URI is still refused on Coder, with no redirect.
|
||||
query.Set("redirect_uri", "https://not-registered.example/callback")
|
||||
resp := sendAuthorizeRequest(ctx, t, client, http.MethodGet, query)
|
||||
defer resp.Body.Close()
|
||||
require.Equal(t, http.StatusBadRequest, resp.StatusCode)
|
||||
require.Empty(t, resp.Header.Get("Location"))
|
||||
require.Contains(t, readBody(t, resp), "must match one of")
|
||||
}
|
||||
|
||||
@@ -82,10 +82,17 @@ func CreateTestOAuth2App(t *testing.T, client *codersdk.Client) (*codersdk.OAuth
|
||||
// first, and needs owner-level permissions to do so.
|
||||
func RegisterPublicClient(t *testing.T, client *codersdk.Client, name, redirectURI string) codersdk.OAuth2ClientRegistrationResponse {
|
||||
t.Helper()
|
||||
return RegisterPublicClientWithRedirectURIs(t, client, name, redirectURI)
|
||||
}
|
||||
|
||||
// RegisterPublicClientWithRedirectURIs registers a public client with every
|
||||
// redirect URI given. The first becomes the primary callback.
|
||||
func RegisterPublicClientWithRedirectURIs(t *testing.T, client *codersdk.Client, name string, redirectURIs ...string) codersdk.OAuth2ClientRegistrationResponse {
|
||||
t.Helper()
|
||||
|
||||
ctx := testutil.Context(t, testutil.WaitLong)
|
||||
resp, err := client.PostOAuth2ClientRegistration(ctx, codersdk.OAuth2ClientRegistrationRequest{
|
||||
RedirectURIs: []string{redirectURI},
|
||||
RedirectURIs: redirectURIs,
|
||||
ClientName: fmt.Sprintf("%s-%s", name, testutil.MustRandString(t, 10)),
|
||||
TokenEndpointAuthMethod: codersdk.OAuth2TokenEndpointAuthMethodNone,
|
||||
})
|
||||
|
||||
@@ -155,7 +155,7 @@ func scopeStringToAPIKeyScopes(scope string) (database.APIKeyScopes, error) {
|
||||
// extractTokenRequest parses and validates the /oauth2/tokens form. It takes
|
||||
// the app because whether client_secret is required depends on the client
|
||||
// type.
|
||||
func extractTokenRequest(r *http.Request, callbackURL *url.URL, app database.OAuth2ProviderApp) (codersdk.OAuth2TokenRequest, []codersdk.ValidationError, error) {
|
||||
func extractTokenRequest(r *http.Request, primary *url.URL, alternates []*url.URL, app database.OAuth2ProviderApp) (codersdk.OAuth2TokenRequest, []codersdk.ValidationError, error) {
|
||||
p := httpapi.NewQueryParamParser()
|
||||
err := r.ParseForm()
|
||||
if err != nil {
|
||||
@@ -229,7 +229,7 @@ func extractTokenRequest(r *http.Request, callbackURL *url.URL, app database.OAu
|
||||
}
|
||||
|
||||
// Validate redirect URI - errors are added to p.Errors.
|
||||
_ = p.RedirectURL(vals, callbackURL, "redirect_uri")
|
||||
_ = p.RedirectURL(vals, primary, alternates, "redirect_uri")
|
||||
|
||||
// Validate resource parameter syntax (RFC 8707): must be absolute URI without fragment.
|
||||
if err := validateResourceParameter(req.Resource); err != nil {
|
||||
@@ -262,7 +262,7 @@ func Tokens(db database.Store, lifetimes codersdk.SessionLifetime, logger slog.L
|
||||
ctx := r.Context()
|
||||
app := httpmw.OAuth2ProviderApp(r)
|
||||
|
||||
callbackURL, err := url.Parse(app.CallbackURL)
|
||||
primary, alternates, err := registeredRedirectURIs(app)
|
||||
if err != nil {
|
||||
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
|
||||
Message: "Failed to validate form values.",
|
||||
@@ -271,7 +271,7 @@ func Tokens(db database.Store, lifetimes codersdk.SessionLifetime, logger slog.L
|
||||
return
|
||||
}
|
||||
|
||||
req, validationErrs, err := extractTokenRequest(r, callbackURL, app)
|
||||
req, validationErrs, err := extractTokenRequest(r, primary, alternates, app)
|
||||
if err != nil {
|
||||
if errors.Is(err, errConflictingClientAuth) {
|
||||
writeTokenError(ctx, rw, http.StatusBadRequest, codersdk.OAuth2ErrorCodeInvalidRequest, "Conflicting client credentials between Authorization header and request body")
|
||||
|
||||
@@ -430,7 +430,7 @@ func TestExtractTokenParams_Scopes(t *testing.T) {
|
||||
}
|
||||
|
||||
// Extract token request
|
||||
tokenReq, validationErrs, err := extractTokenRequest(req, callbackURL, confidentialApp)
|
||||
tokenReq, validationErrs, err := extractTokenRequest(req, callbackURL, nil, confidentialApp)
|
||||
|
||||
// Verify no errors occurred
|
||||
require.NoError(t, err, "extractTokenRequest should not return error for: %s", tc.description)
|
||||
@@ -493,7 +493,7 @@ func TestExtractTokenParams_ScopesURLEncoded(t *testing.T) {
|
||||
}
|
||||
|
||||
// Extract token request
|
||||
tokenReq, validationErrs, err := extractTokenRequest(req, callbackURL, confidentialApp)
|
||||
tokenReq, validationErrs, err := extractTokenRequest(req, callbackURL, nil, confidentialApp)
|
||||
|
||||
// Verify no errors
|
||||
require.NoError(t, err)
|
||||
@@ -576,7 +576,7 @@ func TestExtractTokenParams_ScopesEdgeCases(t *testing.T) {
|
||||
Form: form,
|
||||
}
|
||||
|
||||
tokenReq, validationErrs, err := extractTokenRequest(req, callbackURL, confidentialApp)
|
||||
tokenReq, validationErrs, err := extractTokenRequest(req, callbackURL, nil, confidentialApp)
|
||||
|
||||
require.NoError(t, err, "extractTokenRequest should not error for: %s", tc.description)
|
||||
require.Empty(t, validationErrs)
|
||||
@@ -816,7 +816,7 @@ func TestExtractTokenRequest_ClientSecretRequirement(t *testing.T) {
|
||||
Form: form,
|
||||
}
|
||||
|
||||
_, validationErrs, err := extractTokenRequest(req, callbackURL, tc.app)
|
||||
_, validationErrs, err := extractTokenRequest(req, callbackURL, nil, tc.app)
|
||||
|
||||
if tc.wantErrorField == "" {
|
||||
require.NoError(t, err)
|
||||
@@ -852,7 +852,7 @@ func TestRefreshTokenGrant_Scopes(t *testing.T) {
|
||||
Form: form,
|
||||
}
|
||||
|
||||
tokenReq, validationErrs, err := extractTokenRequest(req, callbackURL, confidentialApp)
|
||||
tokenReq, validationErrs, err := extractTokenRequest(req, callbackURL, nil, confidentialApp)
|
||||
|
||||
require.NoError(t, err)
|
||||
require.Empty(t, validationErrs)
|
||||
|
||||
@@ -112,6 +112,10 @@ Disabling only blocks *new* self-registrations. Applications that already
|
||||
registered while it was enabled keep authorizing and exchanging tokens
|
||||
normally; disabling does not revoke or otherwise affect them.
|
||||
|
||||
A registration may list several `redirect_uris`.
|
||||
A request may present any of them, and the code it receives can only be exchanged with that same URI.
|
||||
The first entry is the primary callback: it is what the web UI shows for the application, and what a request that omits `redirect_uri` is sent to.
|
||||
|
||||
## Integration Patterns
|
||||
|
||||
### Client Authentication Methods
|
||||
@@ -398,7 +402,7 @@ Add `oauth2` to your experiment flags: `coder server --experiments oauth2`
|
||||
|
||||
### "Invalid redirect_uri"
|
||||
|
||||
Ensure the redirect URI in your request exactly matches the one registered for your application.
|
||||
Ensure the redirect URI in your request exactly matches one of the redirect URIs registered for your application.
|
||||
The one exception is the port of a loopback `http://` redirect URI (`localhost`, `127.0.0.1`, `[::1]`), which may differ from the registered one.
|
||||
Refer to the note under [Client Authentication Methods](#client-authentication-methods).
|
||||
|
||||
@@ -566,7 +570,7 @@ A misspelled parameter is ignored on the same rule, so what you see is the failu
|
||||
|
||||
Two failures stay on Coder rather than reaching your callback, because in both cases the callback is not yet trustworthy:
|
||||
|
||||
- A `redirect_uri` that does not parse, or that does not exactly match the one registered for the application.
|
||||
- A `redirect_uri` that does not parse, or that does not exactly match one of the redirect URIs registered for the application.
|
||||
Redirecting to it would defeat the check that just rejected it, so Coder answers 400 (see ["Invalid redirect_uri"](#invalid-redirect_uri)).
|
||||
- A `client_id` sent more than once, or one that does not name the application the callback was matched against.
|
||||
Coder cannot tell whose registration it is about to redirect to.
|
||||
|
||||
Reference in New Issue
Block a user