mirror of
https://github.com/coder/coder.git
synced 2026-06-02 20:48:20 +00:00
fix: OAuth2 cancel button in the authorization page not working (#24058)
Go's html/template has a built-in security filter (urlFilter) that only allows http, https, and mailto URL schemes. Any other scheme gets replaced with #ZgotmplZ. The OAuth2 app's callback URL uses custom URI scheme which the filter considers unsafe. For example the Coder JetBrains plugin exposes a callback URI with the scheme jetbrains:// - which was effectively changed by the template engine into #ZgotmplZ. Of course this is not an actual callback. When users clicked the cancel button nothing happened. The fix was simple - we now wrap the apps registered callback URI into htmltemplate.URL. Usually this needs some validation otherwise the linter will complain about it. The callback URI used by the Cancel logic is actually validated by our backend when the client app programmatically registered via the dynamic OAuth2 registration endpoints, so we refactored the validation around that code and re-used some of it in the Cancel handling to make sure we don't allow URIs like `javascript` and `data`, even though in theory these URIs were already validated. In addition, while testing this PR with https://github.com/coder/coder-jetbrains-toolbox/pull/209 I discovered that we are also not compliant with https://www.rfc-editor.org/rfc/rfc6749#section-4.1.2.1 which requires the server to attach the local state if it was provided by the client in the original request. Also it is optional but generally a good practice to include `error_description` in the error responses. In fact we follow this pattern for the other types of error responses. So this is not a one off. - resolves #20323 <img width="1485" height="771" alt="Cancel_page_with_invalid_uri" src="https://github.com/user-attachments/assets/5539d234-9ce3-4dda-b421-d023fc9aa99e" /> <img width="486" height="746" alt="Coder Toolbox handling the Cancel button" src="https://github.com/user-attachments/assets/acab71a6-d29c-4fa9-80ba-3c0095bbdc8f" /> <!-- If you have used AI to produce some or all of this PR, please ensure you have read our [AI Contribution guidelines](https://coder.com/docs/about/contributing/AI_CONTRIBUTING) before submitting. -->
This commit is contained in:
@@ -5,6 +5,7 @@ import (
|
||||
"database/sql"
|
||||
"encoding/hex"
|
||||
"errors"
|
||||
htmltemplate "html/template"
|
||||
"net/http"
|
||||
"net/url"
|
||||
"strings"
|
||||
@@ -146,12 +147,35 @@ func ShowAuthorizePage(accessURL *url.URL) http.HandlerFunc {
|
||||
cancel := params.redirectURL
|
||||
cancelQuery := params.redirectURL.Query()
|
||||
cancelQuery.Add("error", "access_denied")
|
||||
cancelQuery.Add("error_description", "The resource owner or authorization server denied the request")
|
||||
if params.state != "" {
|
||||
cancelQuery.Add("state", params.state)
|
||||
}
|
||||
cancel.RawQuery = cancelQuery.Encode()
|
||||
|
||||
cancelURI := cancel.String()
|
||||
if err := codersdk.ValidateRedirectURIScheme(cancel); err != nil {
|
||||
site.RenderStaticErrorPage(rw, r, site.ErrorPageData{
|
||||
Status: http.StatusBadRequest,
|
||||
HideStatus: false,
|
||||
Title: "Invalid Callback URL",
|
||||
Description: "The application's registered callback URL has an invalid scheme.",
|
||||
Actions: []site.Action{
|
||||
{
|
||||
URL: accessURL.String(),
|
||||
Text: "Back to site",
|
||||
},
|
||||
},
|
||||
})
|
||||
return
|
||||
}
|
||||
|
||||
site.RenderOAuthAllowPage(rw, r, site.RenderOAuthAllowData{
|
||||
AppIcon: app.Icon,
|
||||
AppName: app.Name,
|
||||
CancelURI: cancel.String(),
|
||||
AppIcon: app.Icon,
|
||||
AppName: app.Name,
|
||||
// #nosec G203 -- The scheme is validated by
|
||||
// codersdk.ValidateRedirectURIScheme above.
|
||||
CancelURI: htmltemplate.URL(cancelURI),
|
||||
RedirectURI: r.URL.String(),
|
||||
CSRFToken: nosurf.Token(r),
|
||||
Username: ua.FriendlyName,
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
package oauth2provider_test
|
||||
|
||||
import (
|
||||
htmltemplate "html/template"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"testing"
|
||||
@@ -20,7 +21,7 @@ func TestOAuthConsentFormIncludesCSRFToken(t *testing.T) {
|
||||
|
||||
site.RenderOAuthAllowPage(rec, req, site.RenderOAuthAllowData{
|
||||
AppName: "Test OAuth App",
|
||||
CancelURI: "https://coder.com/cancel",
|
||||
CancelURI: htmltemplate.URL("https://coder.com/cancel"),
|
||||
RedirectURI: "https://coder.com/oauth2/authorize?client_id=test",
|
||||
CSRFToken: csrfFieldValue,
|
||||
Username: "test-user",
|
||||
|
||||
@@ -75,6 +75,49 @@ func (req *OAuth2ClientRegistrationRequest) Validate() error {
|
||||
return nil
|
||||
}
|
||||
|
||||
// ValidateRedirectURIScheme reports whether the callback URL's scheme is
|
||||
// safe to use as a redirect target. It returns an error when the scheme
|
||||
// is empty, an unsupported URN, or one of the schemes that are dangerous
|
||||
// in browser/HTML contexts (javascript, data, file, ftp).
|
||||
//
|
||||
// Legitimate custom schemes for native apps (e.g. vscode://, jetbrains://)
|
||||
// are allowed.
|
||||
// ValidateRedirectURIScheme reports whether the callback URL's scheme is
|
||||
// safe to use as a redirect target. It returns an error when the scheme
|
||||
// is empty, an unsupported URN, or one of the schemes that are dangerous
|
||||
// in browser/HTML contexts (javascript, data, file, ftp).
|
||||
//
|
||||
// Legitimate custom schemes for native apps (e.g. vscode://, jetbrains://)
|
||||
// are allowed.
|
||||
func ValidateRedirectURIScheme(u *url.URL) error {
|
||||
return validateScheme(u)
|
||||
}
|
||||
|
||||
func validateScheme(u *url.URL) error {
|
||||
if u.Scheme == "" {
|
||||
return xerrors.New("redirect URI must have a scheme")
|
||||
}
|
||||
|
||||
// Handle special URNs (RFC 6749 section 3.1.2.1).
|
||||
if u.Scheme == "urn" {
|
||||
if u.String() == "urn:ietf:wg:oauth:2.0:oob" {
|
||||
return nil
|
||||
}
|
||||
return xerrors.New("redirect URI uses unsupported URN scheme")
|
||||
}
|
||||
|
||||
// Block dangerous schemes for security (not allowed by RFCs
|
||||
// for OAuth2).
|
||||
dangerousSchemes := []string{"javascript", "data", "file", "ftp"}
|
||||
for _, dangerous := range dangerousSchemes {
|
||||
if strings.EqualFold(u.Scheme, dangerous) {
|
||||
return xerrors.Errorf("redirect URI uses dangerous scheme %s which is not allowed", dangerous)
|
||||
}
|
||||
}
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
// validateRedirectURIs validates redirect URIs according to RFC 7591, 8252
|
||||
func validateRedirectURIs(uris []string, tokenEndpointAuthMethod OAuth2TokenEndpointAuthMethod) error {
|
||||
if len(uris) == 0 {
|
||||
@@ -91,27 +134,14 @@ func validateRedirectURIs(uris []string, tokenEndpointAuthMethod OAuth2TokenEndp
|
||||
return xerrors.Errorf("redirect URI at index %d is not a valid URL: %w", i, err)
|
||||
}
|
||||
|
||||
// Validate schemes according to RFC requirements
|
||||
if uri.Scheme == "" {
|
||||
return xerrors.Errorf("redirect URI at index %d must have a scheme", i)
|
||||
if err := validateScheme(uri); err != nil {
|
||||
return xerrors.Errorf("redirect URI at index %d: %w", i, err)
|
||||
}
|
||||
|
||||
// Handle special URNs (RFC 6749 section 3.1.2.1)
|
||||
// The urn:ietf:wg:oauth:2.0:oob scheme passed validation
|
||||
// above but needs no further checks.
|
||||
if uri.Scheme == "urn" {
|
||||
// Allow the out-of-band redirect URI for native apps
|
||||
if uriStr == "urn:ietf:wg:oauth:2.0:oob" {
|
||||
continue // This is valid for native apps
|
||||
}
|
||||
// Other URNs are not standard for OAuth2
|
||||
return xerrors.Errorf("redirect URI at index %d uses unsupported URN scheme", i)
|
||||
}
|
||||
|
||||
// Block dangerous schemes for security (not allowed by RFCs for OAuth2)
|
||||
dangerousSchemes := []string{"javascript", "data", "file", "ftp"}
|
||||
for _, dangerous := range dangerousSchemes {
|
||||
if strings.EqualFold(uri.Scheme, dangerous) {
|
||||
return xerrors.Errorf("redirect URI at index %d uses dangerous scheme %s which is not allowed", i, dangerous)
|
||||
}
|
||||
continue
|
||||
}
|
||||
|
||||
// Determine if this is a public client based on token endpoint auth method
|
||||
|
||||
@@ -40,7 +40,7 @@ CODER_EXPERIMENTS=oauth2
|
||||
2. Click **Create Application**
|
||||
3. Fill in the application details:
|
||||
- **Name**: Your application name
|
||||
- **Callback URL**: `https://yourapp.example.com/callback`
|
||||
- **Callback URL**: `https://yourapp.example.com/callback` (web) or `myapp://callback` (native/desktop)
|
||||
- **Icon**: Optional icon URL
|
||||
|
||||
### Method 2: Management API
|
||||
@@ -251,16 +251,31 @@ Add `oauth2` to your experiment flags: `coder server --experiments oauth2`
|
||||
|
||||
Ensure the redirect URI in your request exactly matches the one registered for your application.
|
||||
|
||||
### "Invalid Callback URL" on the consent page
|
||||
|
||||
If you see this error when authorizing, the registered callback URL uses a
|
||||
blocked scheme (`javascript:`, `data:`, `file:`, or `ftp:`). Update the
|
||||
application's callback URL to a valid scheme (see
|
||||
[Callback URL schemes](#callback-url-schemes)).
|
||||
|
||||
### "PKCE verification failed"
|
||||
|
||||
Verify that the `code_verifier` used in the token request matches the one used to generate the `code_challenge`.
|
||||
|
||||
## Callback URL schemes
|
||||
|
||||
Custom URI schemes (`myapp://`, `vscode://`, `jetbrains://`, etc.) are fully supported for native and desktop applications. The OS routes the redirect back to the registered application without requiring a running HTTP server.
|
||||
|
||||
The following schemes are blocked for security reasons: `javascript:`, `data:`, `file:`, `ftp:`.
|
||||
|
||||
## Security Considerations
|
||||
|
||||
- **Use HTTPS**: Always use HTTPS in production to protect tokens in transit
|
||||
- **Implement PKCE**: PKCE is mandatory for all authorization code clients
|
||||
(public and confidential)
|
||||
- **Validate redirect URLs**: Only register trusted redirect URIs for your applications
|
||||
- **Validate redirect URLs**: Only register trusted redirect URIs. Dangerous
|
||||
schemes (`javascript:`, `data:`, `file:`, `ftp:`) are blocked by the server,
|
||||
but custom URI schemes for native apps (`myapp://`) are permitted
|
||||
- **Rotate secrets**: Periodically rotate client secrets using the management API
|
||||
|
||||
## Limitations
|
||||
|
||||
+1
-1
@@ -801,7 +801,7 @@ func (jfs justFilesSystem) Open(name string) (fs.File, error) {
|
||||
type RenderOAuthAllowData struct {
|
||||
AppIcon string
|
||||
AppName string
|
||||
CancelURI string
|
||||
CancelURI htmltemplate.URL
|
||||
RedirectURI string
|
||||
CSRFToken string
|
||||
Username string
|
||||
|
||||
Reference in New Issue
Block a user