- Notifications
You must be signed in to change notification settings - Fork927
feat: oauth2 - add authorization server metadata endpoint and PKCE support#18548
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
ThomasK33 commentedJun 24, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
8f890ec to018694aCompare018694a to3daa2abCompare3daa2ab tob50e322Compare| // @Success302 | ||
| // @Router /oauth2/authorize [post] | ||
| // @Success200 "Returns HTML authorization page" | ||
| // @Router /oauth2/authorize [get] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This looks like a breaking change to me. We should still support the GET /oauth2/authorize -> 302
I'm happy for us to support both methods, but I'd like the existing GET endpoint to behave the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The OAuth 2 endpoints were never part of a stable release, only added to the muxer/router in dev builds, so not a breaking change per se.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for clarifying, I completely missed that!
Confirmed on 2.23 release build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think the redirect here would be the spec compliant thing to do, but then I'm a bit too lazy to have to redirect to another page, to do the same thing.
I'm not against it if you think it makes more sense though... just lazy 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This were not shipped in a release?
I thought backstage relied on these oauth endpoints?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nope, you have the OAuth 2 middleware applied to all OAuth 2-related routes, which just returned a status forbidden for non-dev builds.
Lines 20 to 31 in7a3a6d4
| func (*API)oAuth2ProviderMiddleware(next http.Handler) http.Handler { | |
| returnhttp.HandlerFunc(func(rw http.ResponseWriter,r*http.Request) { | |
| if!buildinfo.IsDev() { | |
| httpapi.Write(r.Context(),rw,http.StatusForbidden, codersdk.Response{ | |
| Message:"OAuth2 provider is under development.", | |
| }) | |
| return | |
| } | |
| next.ServeHTTP(rw,r) | |
| }) | |
| } |
| origin := r.Header.Get(httpmw.OriginHeader) | ||
| originU, err := url.Parse(origin) | ||
| if err != nil { | ||
| httpapi.Write(r.Context(), rw, http.StatusBadRequest, codersdk.Response{ | ||
| Message: "Invalid origin header.", | ||
| Detail: err.Error(), | ||
| }) | ||
| return | ||
| } | ||
| referer := r.Referer() | ||
| refererU, err := url.Parse(referer) | ||
| if err != nil { | ||
| httpapi.Write(r.Context(), rw, http.StatusBadRequest, codersdk.Response{ | ||
| Message: "Invalid referer header.", | ||
| Detail: err.Error(), | ||
| }) | ||
| return | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why are these checks being removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The checks were removed due to a redesign of the OAuth2 flow for improved standardization and security.
The old approach (removed code), relied on fragile referer and origin header checks. There used header parsing to verify if requests "came from self." and added a redirected=true parameter for loop detection. (Which isn't not part of any OAuth spec). And it rendered us vulnerable to header spoofing.
The new approach (current code) uses standard HTTP methods (GET for consent and POST for authorization), and when users clicks the "Allow" button, a POST request is submitted from the form.Allowing us to be compliant with the standard OAuth2 authorization flow, and also allowing us to potentially reuse the CSRF middleware in the future if necessary.
Also, it simplifies the logic by removing a good bit of complexity with the redirects and state encoding with URL query params and potentially error-prone header validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
AhhhGET andPOST to prevent the infinite loop issue.
I would need to do more manual testing of other providers, but I don't recall seeing aPOST request to/authorize in the spec. Do you have a link or anything I could refer to ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The authorization server MUST support the use of the HTTP "GET"
method [RFC2616] for the authorization endpoint and MAY support the
use of the "POST" method as well.
| // url.Parse() allows empty URLs, which is fine because the origin is not | ||
| // always set by browsers (or other tools like cURL). If the origin does | ||
| // exist, we will make sure it matches. We require `referer` to be set at | ||
| // a minimum in order to detect whether "allow" has been pressed, however. | ||
| cameFromSelf := (origin == "" || originU.Hostname() == accessURL.Hostname()) && | ||
| refererU.Hostname() == accessURL.Hostname() && | ||
| refererU.Path == "/oauth2/authorize" | ||
| // If we were redirected here from this same page it means the user | ||
| // pressed the allow button so defer to the authorize handler which | ||
| // generates the code, otherwise show the HTML allow page. | ||
| // TODO: Skip this step if the user has already clicked allow before, and | ||
| // we can just reuse the token. | ||
| if cameFromSelf { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same, why is this being removed?
Uh oh!
There was an error while loading.Please reload this page.
scripts/oauth2/README.md Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd like to see all of this moved instead to anidentityprovidertest package and migrated to Go. Having once-off scripts to test is fine, but they won't get run on each commit. Adding a feature like this in a hot code path needs to be tested all the time.
3dd6c7e to224784aCompare224784a toc2d85d9Comparec2d85d9 to69eb5c8Comparece6aacd to0efb35bCompare69eb5c8 to80c695bCompare0efb35b to0218619Compare80c695b to03c4724Compare03c4724 to870e5ebCompare…port- Add /.well-known/oauth-authorization-server metadata endpoint (RFC 8414)- Implement PKCE support with S256 method for enhanced security- Add resource parameter support (RFC 8707) for token binding- Add OAuth2-compliant error responses with proper error codes- Fix authorization UI to use POST-based consent instead of GET redirects- Add comprehensive OAuth2 test scripts and interactive test server- Update CLAUDE.md with OAuth2 development guidelinesDatabase changes:- Add migration 000341: code_challenge, resource_uri, audience fields- Update audit table for new OAuth2 fieldsOAuth2 provider remains development-only (requires --dev flag).Change-Id: Ifbd0d9a543d545f9f56ecaa77ff2238542ff954aSigned-off-by: Thomas Kosiewski <tk@coder.com>
870e5eb todd622d0CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Overall things look ok to me 👍
Thepost to/authorize is a good way to prevent the infinite redirect loop issue. I do not see that in the spec, so it is new to me.
Is there a reason some of the testing lives as bash scripts?
| // errorWriter interface abstracts different error response formats. | ||
| // This uses the Strategy pattern to avoid a control flag (useOAuth2Errors bool) | ||
| // which was flagged by the linter as an anti-pattern. Instead of duplicating | ||
| // the entire function logic or using a boolean parameter, we inject the error | ||
| // handling behavior through this interface. | ||
| typeerrorWriterinterface { | ||
| writeMissingClientID(ctx context.Context,rw http.ResponseWriter) | ||
| writeInvalidClientID(ctx context.Context,rw http.ResponseWriter,errerror) | ||
| writeInvalidClient(ctx context.Context,rw http.ResponseWriter) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is kind of nifty. Why do we need to sendcodersdk.Response errors at all though?
Can/authorize just return the oauth compliant errors?
| // TestCodeChallenge2 is the generated challenge for TestCodeVerifier2 | ||
| var TestCodeChallenge2 = GenerateCodeChallenge(TestCodeVerifier2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Unused code
| func AuthorizeOAuth2App(t *testing.T, client *codersdk.Client, baseURL string, params AuthorizeParams) string { | ||
| t.Helper() | ||
| ctx := testutil.Context(t, testutil.WaitLong) | ||
| // Build authorization URL | ||
| authURL, err := url.Parse(baseURL + "/oauth2/authorize") | ||
| require.NoError(t, err, "failed to parse authorization URL") | ||
| query := url.Values{} | ||
| query.Set("client_id", params.ClientID) | ||
| query.Set("response_type", params.ResponseType) | ||
| query.Set("redirect_uri", params.RedirectURI) | ||
| query.Set("state", params.State) | ||
| if params.CodeChallenge != "" { | ||
| query.Set("code_challenge", params.CodeChallenge) | ||
| query.Set("code_challenge_method", params.CodeChallengeMethod) | ||
| } | ||
| if params.Resource != "" { | ||
| query.Set("resource", params.Resource) | ||
| } | ||
| if params.Scope != "" { | ||
| query.Set("scope", params.Scope) | ||
| } | ||
| authURL.RawQuery = query.Encode() | ||
| // Create POST request to authorize endpoint (simulating user clicking "Allow") | ||
| req, err := http.NewRequestWithContext(ctx, "POST", authURL.String(), nil) | ||
| require.NoError(t, err, "failed to create authorization request") | ||
| // Add session token | ||
| req.Header.Set("Coder-Session-Token", client.SessionToken()) | ||
| // Perform request | ||
| httpClient := &http.Client{ | ||
| CheckRedirect: func(_ *http.Request, _ []*http.Request) error { | ||
| // Don't follow redirects, we want to capture the redirect URL | ||
| return http.ErrUseLastResponse | ||
| }, | ||
| } | ||
| resp, err := httpClient.Do(req) | ||
| require.NoError(t, err, "failed to perform authorization request") | ||
| defer resp.Body.Close() | ||
| // Should get a redirect response (either 302 Found or 307 Temporary Redirect) | ||
| require.True(t, resp.StatusCode == http.StatusFound || resp.StatusCode == http.StatusTemporaryRedirect, | ||
| "expected redirect response, got %d", resp.StatusCode) | ||
| // Extract redirect URL | ||
| location := resp.Header.Get("Location") | ||
| require.NotEmpty(t, location, "missing Location header in redirect response") | ||
| // Parse redirect URL to extract authorization code | ||
| redirectURL, err := url.Parse(location) | ||
| require.NoError(t, err, "failed to parse redirect URL") | ||
| code := redirectURL.Query().Get("code") | ||
| require.NotEmpty(t, code, "missing authorization code in redirect URL") | ||
| // Verify state parameter | ||
| returnedState := redirectURL.Query().Get("state") | ||
| require.Equal(t, params.State, returnedState, "state parameter mismatch") | ||
| return code | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Some of this exists inoidctest
Might be worth merging this package with that?
| origin := r.Header.Get(httpmw.OriginHeader) | ||
| originU, err := url.Parse(origin) | ||
| if err != nil { | ||
| httpapi.Write(r.Context(), rw, http.StatusBadRequest, codersdk.Response{ | ||
| Message: "Invalid origin header.", | ||
| Detail: err.Error(), | ||
| }) | ||
| return | ||
| } | ||
| referer := r.Referer() | ||
| refererU, err := url.Parse(referer) | ||
| if err != nil { | ||
| httpapi.Write(r.Context(), rw, http.StatusBadRequest, codersdk.Response{ | ||
| Message: "Invalid referer header.", | ||
| Detail: err.Error(), | ||
| }) | ||
| return | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
AhhhGET andPOST to prevent the infinite loop issue.
I would need to do more manual testing of other providers, but I don't recall seeing aPOST request to/authorize in the spec. Do you have a link or anything I could refer to ?
| ifve.Field=="grant_type" { | ||
| writeOAuth2Error(ctx,rw,http.StatusBadRequest,"unsupported_grant_type","The grant type is missing or unsupported") | ||
| return | ||
| } | ||
| // Check for missing required parameters for authorization_code grant | ||
| ifve.Field=="code"||ve.Field=="client_id"||ve.Field=="client_secret" { | ||
| writeOAuth2Error(ctx,rw,http.StatusBadRequest,"invalid_request",fmt.Sprintf("Missing required parameter: %s",ve.Field)) | ||
| return | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Does it matter which comes first? Does 1 have any priority?
I just wonder if this can be made more deterministic, rather than relying on the validation order. (Which I guess has some static order based on the functionextractTokenParameters)
| ifve.Field=="grant_type" { | |
| writeOAuth2Error(ctx,rw,http.StatusBadRequest,"unsupported_grant_type","The grant type is missing or unsupported") | |
| return | |
| } | |
| // Check for missing required parameters for authorization_code grant | |
| ifve.Field=="code"||ve.Field=="client_id"||ve.Field=="client_secret" { | |
| writeOAuth2Error(ctx,rw,http.StatusBadRequest,"invalid_request",fmt.Sprintf("Missing required parameter: %s",ve.Field)) | |
| return | |
| } | |
| ifslices.ContainsFunc(validationErrs,func(validationError codersdk.ValidationError)bool { | |
| returnvalidationError.Field=="grant_type" | |
| }) { | |
| writeOAuth2Error(ctx,rw,http.StatusBadRequest,"unsupported_grant_type","The grant type is missing or unsupported") | |
| return | |
| } | |
| ifslices.ContainsFunc(validationErrs,func(validationError codersdk.ValidationError)bool { | |
| returnvalidationError.Field==ve.Field=="code"||ve.Field=="client_id"||ve.Field=="client_secret" | |
| }) { | |
| writeOAuth2Error(ctx,rw,http.StatusBadRequest,"invalid_request",fmt.Sprintf("Missing required parameter: %s",ve.Field)) | |
| return | |
| } |
| iferrors.Is(err,errBadSecret) { | ||
| writeOAuth2Error(ctx,rw,http.StatusUnauthorized,"invalid_client","The client credentials are invalid") | ||
| return | ||
| } | ||
| iferrors.Is(err,errBadCode) { | ||
| writeOAuth2Error(ctx,rw,http.StatusBadRequest,"invalid_grant","The authorization code is invalid or expired") | ||
| return | ||
| } | ||
| iferrors.Is(err,errInvalidPKCE) { | ||
| writeOAuth2Error(ctx,rw,http.StatusBadRequest,"invalid_grant","The PKCE code verifier is invalid") | ||
| return | ||
| } | ||
| iferrors.Is(err,errBadToken) { | ||
| writeOAuth2Error(ctx,rw,http.StatusBadRequest,"invalid_grant","The refresh token is invalid or expired") | ||
| return | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is unfortunate, but I get it.
| // @Success302 | ||
| // @Router /oauth2/authorize [post] | ||
| // @Success200 "Returns HTML authorization page" | ||
| // @Router /oauth2/authorize [get] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This were not shipped in a release?
I thought backstage relied on these oauth endpoints?
| ### `setup-test-app.sh` | ||
| Creates a test OAuth2 application and outputs environment variables. | ||
| Usage: | ||
| ```bash | ||
| eval $(./scripts/oauth2/setup-test-app.sh) | ||
| echo "Client ID: $CLIENT_ID" | ||
| ``` | ||
| ### `cleanup-test-app.sh` | ||
| Deletes a test OAuth2 application. | ||
| Usage: | ||
| ```bash | ||
| ./scripts/oauth2/cleanup-test-app.sh $CLIENT_ID | ||
| # Or if CLIENT_ID is set as environment variable: | ||
| ./scripts/oauth2/cleanup-test-app.sh | ||
| ``` | ||
| ### `generate-pkce.sh` | ||
| Generates PKCE code verifier and challenge for manual testing. | ||
| Usage: | ||
| ```bash | ||
| ./scripts/oauth2/generate-pkce.sh | ||
| ``` | ||
| ### `test-manual-flow.sh` | ||
| Launches a local Go web server to test the OAuth2 flow interactively. The server automatically handles the OAuth2 callback and token exchange, providing a user-friendly web interface with results. | ||
| Usage: | ||
| ```bash | ||
| # First set up an app | ||
| eval $(./scripts/oauth2/setup-test-app.sh) | ||
| # Then run the test server | ||
| ./scripts/oauth2/test-manual-flow.sh | ||
| ``` | ||
| Features: | ||
| - Starts a local web server on port 9876 | ||
| - Automatically captures the authorization code | ||
| - Performs token exchange without manual intervention | ||
| - Displays results in a clean web interface | ||
| - Shows example API calls you can make with the token |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why are these tests in bash? They will not be run in CI then right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That's right, they won't be run in CI.
They are in Bash because I am testing the actual browser-based flow of a user authorizing and clicking the button on the authorize page. Automating that manual e2e test with something like Puppeteer or other headless browsers seemed a bit overkill to me, at least for now.
I'm going to pull copilot in as a reviewer. It's caught some typos in my larger PRs in the past. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull Request Overview
This PR implements critical OAuth2 features to improve standards compliance in Coder’s authorization server. Key changes include adding a /.well-known/oauth-authorization-server metadata endpoint, full PKCE support within the authorization and token exchange flows, and enhanced handling of the resource parameter for token binding.
Reviewed Changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| site/static/oauth2allow.html | Replaces an anchor link with a POST form for the Allow action to improve security |
| site/src/api/typesGenerated.ts | Adds the OAuth2AuthorizationServerMetadata type definition for client discovery |
| scripts/oauth2/* | Introduces extensive test scripts for OAuth2 metadata, PKCE, resource parameter flows, and manual testing |
| coderd/* | Updates OAuth2 flows, authorization handlers, token exchange logic and introduces PKCE and resource parameter support |
| enterprise/audit/table.go, docs/*, and others | Updates database models, queries, and documentation to support new OAuth2 fields and behavior |
Comments suppressed due to low confidence (3)
coderd/identityprovider/authorize.go:90
- [nitpick] Consider adding an inline comment to explain that when a code_challenge is provided but no code_challenge_method is specified, the method defaults to 'S256'. This improves code clarity for future maintainers.
if params.codeChallenge != "" {coderd/oauth2_test.go:433
- [nitpick] Verify that the error message ‘invalid_client’ is used consistently for client credential failures and aligns with OAuth2 standards. Consistency in error responses will aid clients in correctly interpreting error scenarios.
tokenError: "invalid_client",site/static/oauth2allow.html:163
- Replacing the anchor link with a POST form for the 'Allow' action enhances security by mitigating CSRF risks. Ensure that the server-side endpoint handling this form validates POST requests appropriately.
<form method="POST">
Uh oh!
There was an error while loading.Please reload this page.
Summary
This PR implements critical MCP OAuth2 compliance features for Coder's authorization server, adding PKCE support, resource parameter handling, and OAuth2 server metadata discovery. This brings Coder's OAuth2 implementation significantly closer to production readiness for MCP (Model Context Protocol)
integrations.
What's Added
OAuth2 Authorization Server Metadata (RFC 8414)
/.well-known/oauth-authorization-serverendpoint for automatic client discoveryPKCE Support (RFC 7636)
code_challengeandcode_challenge_methodparameters to authorization flowcode_verifiervalidation in token exchangeResource Parameter Support (RFC 8707)
resourceparameter to authorization and token endpointsEnhanced OAuth2 Error Handling
{"error": "code", "error_description": "details"}Authorization UI Improvements
Why This Matters
For MCP Integration: MCP requires OAuth2 authorization servers to support PKCE, resource parameters, and metadata discovery. Without these features, MCP clients cannot securely authenticate with Coder.
For Security: PKCE prevents authorization code interception attacks, especially critical for public clients. Resource binding ensures tokens are only valid for intended services.
For Standards Compliance: These are widely adopted OAuth2 extensions that improve interoperability with modern OAuth2 clients.
Database Changes
code_challenge,code_challenge_method,resource_uritooauth2_provider_app_codesaudiencefield tooauth2_provider_app_tokensfor resource bindingTest Coverage
coderd/identityprovider/pkce_test.gocoderd/oauth2_metadata_test.goTesting Instructions
Breaking Changes
None. All changes maintain backward compatibility with existing OAuth2 flows.
Change-Id: Ifbd0d9a543d545f9f56ecaa77ff2238542ff954a
Signed-off-by: Thomas Kosiewskitk@coder.com