From f79a36469e47778272478d9607afca1a4e26902f Mon Sep 17 00:00:00 2001 From: Atif Ali Date: Wed, 16 Sep 2026 21:06:31 +0500 Subject: [PATCH] fix: relax OAuth2 app form validation for names and callback URLs (#29333) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem The OAuth2 application settings form rejects application names with spaces and callbacks using native application schemes. A VS Code extension client registered through DCR exposed this: `VS Code Coder Extension` and `vscode://coder.coder-remote/oauth/callback` were already stored, but the admin form prevented editing them. ## Fix - Allow application names with spaces and punctuation, limited to 64 UTF-8 bytes on both client and server. - Allow native callback schemes while rejecting dangerous schemes, unsupported URNs, opaque callbacks such as `localhost:3000`, and empty targets such as `vscode:`. HTTP(S) callbacks still require a host. - Reuse the public-client registration policy when admins update public clients, so `mailto`, `tel`, `sms`, external HTTP, and fragments remain rejected. - Trim surrounding whitespace from names and callbacks when submitting, without stripping spaces while typing. - Update the admin API request validators too, so accepted form values can be saved. The server remains authoritative for URL parsing. Public-client edits reuse the existing DCR redirect validator using the stored client type; confidential admin-created applications retain their existing HTTP policy. Native host/path callbacks and the supported OOB URN remain accepted. ## Validation - Vitest component tests assert create/edit submission payloads, whitespace normalization, Unicode byte limits, and rejection of unsafe or malformed callbacks. - Storybook stories cover accepted native-client values and invalid callback states. - Go tests cover name boundaries and accepted/rejected callback values through both admin create and update requests. - `make pre-commit` and targeted tests run before pushing; CI checked on each revision.
Implementation decisions - Replace the shared slug validator only for OAuth2 application names; leave user, workspace, and template names unchanged. - Share the backend's 64-byte name limit with the frontend through the generated `OAuth2AppNameMaxBytes` constant. - Reuse `codersdk.ValidateRedirectURIScheme` for common admin callback validation. Public-client updates additionally call `codersdk.ValidateRedirectURIs`; confidential admin HTTP behavior is preserved. - Normalize name and callback whitespace in the submitted payload rather than on each keystroke. - Keep behavior assertions in Vitest and use Storybook for visual states.
Closes ENG-3342 > 🤖 This PR was created with the help of Coder Agents, and needs a human review. 🧑💻 --- coderd/httpapi/httpapi.go | 48 +++- coderd/oauth2provider/apps.go | 12 + coderd/oauth2provider/provider_test.go | 173 +++++++++++++- codersdk/name.go | 22 ++ codersdk/name_test.go | 39 +++ codersdk/oauth2.go | 4 +- codersdk/oauth2_validation.go | 6 +- site/src/api/typesGenerated.ts | 7 + .../CreateOAuth2AppPageView.stories.tsx | 31 ++- .../EditOAuth2AppPageView.stories.tsx | 25 ++ .../OAuth2AppForm.test.tsx | 225 ++++++++++++++++++ .../OAuth2AppsSettingsPage/OAuth2AppForm.tsx | 90 +++++-- 12 files changed, 631 insertions(+), 51 deletions(-) create mode 100644 site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/OAuth2AppForm.test.tsx diff --git a/coderd/httpapi/httpapi.go b/coderd/httpapi/httpapi.go index c78e4b386b..076966c3e4 100644 --- a/coderd/httpapi/httpapi.go +++ b/coderd/httpapi/httpapi.go @@ -9,6 +9,7 @@ import ( "flag" "fmt" "net/http" + "net/url" "reflect" "strings" "time" @@ -49,13 +50,56 @@ func init() { valid := codersdk.NameValid(str) return valid == nil } - for _, tag := range []string{"username", "organization_name", "template_name", "workspace_name", "oauth2_app_name"} { + for _, tag := range []string{"username", "organization_name", "template_name", "workspace_name"} { err := Validate.RegisterValidation(tag, nameValidator) if err != nil { panic(err) } } + oauth2AppNameValidator := func(fl validator.FieldLevel) bool { + str, ok := fl.Field().Interface().(string) + if !ok { + return false + } + return codersdk.OAuth2AppNameValid(str) == nil + } + err := Validate.RegisterValidation("oauth2_app_name", oauth2AppNameValidator) + if err != nil { + panic(err) + } + + // oauth2_callback_url validates the common callback target shape for OAuth2 + // app administration. Public clients receive the additional DCR redirect URI + // policy in the handler after their stored client type is available. + oauth2CallbackURLValidator := func(fl validator.FieldLevel) bool { + str, ok := fl.Field().Interface().(string) + if !ok { + return false + } + u, err := url.Parse(str) + if err != nil { + return false + } + if err := codersdk.ValidateRedirectURIScheme(u); err != nil { + return false + } + if u.Scheme == "urn" { + return true + } + if (u.Scheme == "http" || u.Scheme == "https") && u.Host == "" { + return false + } + if u.Opaque != "" || (u.Host == "" && u.Path == "") { + return false + } + return true + } + err = Validate.RegisterValidation("oauth2_callback_url", oauth2CallbackURLValidator) + if err != nil { + panic(err) + } + displayNameValidator := func(fl validator.FieldLevel) bool { f := fl.Field().Interface() str, ok := f.(string) @@ -81,7 +125,7 @@ func init() { valid := codersdk.TemplateVersionNameValid(str) return valid == nil } - err := Validate.RegisterValidation("template_version_name", templateVersionNameValidator) + err = Validate.RegisterValidation("template_version_name", templateVersionNameValidator) if err != nil { panic(err) } diff --git a/coderd/oauth2provider/apps.go b/coderd/oauth2provider/apps.go index 046f615670..931c7fe04e 100644 --- a/coderd/oauth2provider/apps.go +++ b/coderd/oauth2provider/apps.go @@ -143,6 +143,18 @@ func UpdateApp(db database.Store, accessURL *url.URL, auditor *audit.Auditor, lo if !httpapi.Read(ctx, rw, r, &req) { return } + if app.IsPublic() { + if err := codersdk.ValidateRedirectURIs([]string{req.CallbackURL}, codersdk.OAuth2ClientTypePublic); err != nil { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Validation failed.", + Validations: []codersdk.ValidationError{{ + Field: "callback_url", + Detail: err.Error(), + }}, + }) + return + } + } app, err := db.UpdateOAuth2ProviderAppByID(ctx, database.UpdateOAuth2ProviderAppByIDParams{ ID: app.ID, UpdatedAt: dbtime.Now(), diff --git a/coderd/oauth2provider/provider_test.go b/coderd/oauth2provider/provider_test.go index 7d18def6b1..2aedb22c02 100644 --- a/coderd/oauth2provider/provider_test.go +++ b/coderd/oauth2provider/provider_test.go @@ -2,7 +2,11 @@ package oauth2provider_test import ( "context" + "errors" "fmt" + "net/http" + "slices" + "strings" "testing" "time" @@ -36,16 +40,16 @@ func TestOAuth2ProviderAppValidation(t *testing.T) { }, }, { - name: "NameSpaces", + name: "NameTooLong", req: codersdk.PostOAuth2ProviderAppRequest{ - Name: "foo bar", + Name: strings.Repeat("a", 65), CallbackURL: "http://localhost:3000", }, }, { - name: "NameTooLong", + name: "NameLeadingSpace", req: codersdk.PostOAuth2ProviderAppRequest{ - Name: "too loooooooooooooooooooooooooong", + Name: " foo", CallbackURL: "http://localhost:3000", }, }, @@ -55,13 +59,6 @@ func TestOAuth2ProviderAppValidation(t *testing.T) { Name: "foo", }, }, - { - name: "URLLocalhostNoScheme", - req: codersdk.PostOAuth2ProviderAppRequest{ - Name: "foo", - CallbackURL: "localhost:3000", - }, - }, { name: "URLNoScheme", req: codersdk.PostOAuth2ProviderAppRequest{ @@ -125,6 +122,148 @@ func TestOAuth2ProviderAppValidation(t *testing.T) { } }) + t.Run("AcceptsDCRValues", func(t *testing.T) { + t.Parallel() + + client := coderdtest.New(t, nil) + _ = coderdtest.CreateFirstUser(t, client) + ctx := testutil.Context(t, testutil.WaitLong) + + // Values registered through Dynamic Client Registration (RFC 7591), + // such as a name with spaces and a custom native-app callback scheme, + // must be creatable and editable via the admin OAuth2 app settings. + app, err := client.PostOAuth2ProviderApp(ctx, codersdk.PostOAuth2ProviderAppRequest{ + Name: "VS Code Coder Extension", + CallbackURL: "vscode://coder.coder-remote/oauth/callback", + }) + require.NoError(t, err) + require.Equal(t, "VS Code Coder Extension", app.Name) + require.Equal(t, "vscode://coder.coder-remote/oauth/callback", app.CallbackURL) + + updated, err := client.PutOAuth2ProviderApp(ctx, app.ID, codersdk.PutOAuth2ProviderAppRequest{ + Name: "Cursor (MCP)", + CallbackURL: "cursor://anysphere.cursor-mcp/oauth/callback", + }) + require.NoError(t, err) + require.Equal(t, "Cursor (MCP)", updated.Name) + require.Equal(t, "cursor://anysphere.cursor-mcp/oauth/callback", updated.CallbackURL) + }) + + t.Run("CallbackURLSchemes", func(t *testing.T) { + t.Parallel() + + client := coderdtest.New(t, nil) + _ = coderdtest.CreateFirstUser(t, client) + + tests := []struct { + name string + callbackURL string + valid bool + }{ + {name: "OpaqueLocalhost", callbackURL: "localhost:3000"}, + {name: "SchemeOnly", callbackURL: "vscode:"}, + {name: "ShortSchemeOnly", callbackURL: "a:"}, + {name: "EmptyNativeTarget", callbackURL: "vscode://"}, + {name: "NativeScheme", callbackURL: "vscode://coder.coder-remote/oauth/callback", valid: true}, + {name: "PathOnlyNativeScheme", callbackURL: "com.example.app:/oauth2redirect", valid: true}, + {name: "UppercaseOOBURN", callbackURL: "URN:ietf:wg:oauth:2.0:oob", valid: true}, + {name: "MalformedHTTP", callbackURL: "http:foo"}, + {name: "HostlessHTTP", callbackURL: "http:/callback"}, + {name: "HostlessHTTPS", callbackURL: "https:///callback"}, + {name: "DangerousSchemeMixedCase", callbackURL: "JaVaScRiPt:alert(1)"}, + {name: "DangerousDataSchemeMixedCase", callbackURL: "DaTa:text/plain,invalid"}, + {name: "DangerousFileSchemeMixedCase", callbackURL: "FiLe:///tmp/invalid"}, + {name: "DangerousFTPSchemeMixedCase", callbackURL: "FtP://example.com/invalid"}, + {name: "UnsupportedURN", callbackURL: "urn:example:invalid"}, + {name: "UnsupportedURNMixedCase", callbackURL: "URN:example:invalid"}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + app, err := client.PostOAuth2ProviderApp(ctx, codersdk.PostOAuth2ProviderAppRequest{ + Name: testutil.GetRandomName(t), + CallbackURL: "https://example.com/callback", + }) + require.NoError(t, err) + + _, postErr := client.PostOAuth2ProviderApp(ctx, codersdk.PostOAuth2ProviderAppRequest{ + Name: testutil.GetRandomName(t), + CallbackURL: test.callbackURL, + }) + if test.valid { + require.NoError(t, postErr) + } else { + requireCallbackURLValidationError(t, postErr) + } + + _, putErr := client.PutOAuth2ProviderApp(ctx, app.ID, codersdk.PutOAuth2ProviderAppRequest{ + Name: testutil.GetRandomName(t), + CallbackURL: test.callbackURL, + }) + if test.valid { + require.NoError(t, putErr) + } else { + requireCallbackURLValidationError(t, putErr) + } + }) + } + }) + + t.Run("PublicDCRCallbackURLPolicy", func(t *testing.T) { + t.Parallel() + + client := coderdtest.New(t, nil) + _ = coderdtest.CreateFirstUser(t, client) + oauth2providertest.EnableDCR(t, client) + + tests := []struct { + name string + callbackURL string + valid bool + }{ + {name: "Mailto", callbackURL: "mailto:user@example.com"}, + {name: "MailtoHierarchical", callbackURL: "mailto://user@example.com"}, + {name: "TelHierarchical", callbackURL: "tel:/15551234567"}, + {name: "SMSHierarchical", callbackURL: "sms:/15551234567"}, + {name: "Tel", callbackURL: "tel:+15551234567"}, + {name: "SMS", callbackURL: "sms:+15551234567"}, + {name: "ExternalHTTP", callbackURL: "http://example.com/callback"}, + {name: "Fragment", callbackURL: "https://example.com/callback#fragment"}, + {name: "LoopbackHTTP", callbackURL: "http://127.0.0.1:8080/callback", valid: true}, + {name: "Native", callbackURL: "com.example.app:/oauth2redirect", valid: true}, + {name: "HTTPS", callbackURL: "https://example.com/updated-callback", valid: true}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + registered, err := client.PostOAuth2ClientRegistration(ctx, codersdk.OAuth2ClientRegistrationRequest{ + ClientName: testutil.GetRandomName(t), + RedirectURIs: []string{"https://example.com/callback"}, + TokenEndpointAuthMethod: codersdk.OAuth2TokenEndpointAuthMethodNone, + }) + require.NoError(t, err) + appID, err := uuid.Parse(registered.ClientID) + require.NoError(t, err) + + updated, err := client.PutOAuth2ProviderApp(ctx, appID, codersdk.PutOAuth2ProviderAppRequest{ + Name: testutil.GetRandomName(t), + CallbackURL: test.callbackURL, + }) + if !test.valid { + requireCallbackURLValidationError(t, err) + return + } + require.NoError(t, err) + require.Equal(t, test.callbackURL, updated.CallbackURL) + require.Equal(t, codersdk.OAuth2ClientTypePublic, updated.ClientType) + }) + } + }) + t.Run("DuplicateNames", func(t *testing.T) { t.Parallel() client := coderdtest.New(t, nil) @@ -422,7 +561,17 @@ func TestOAuth2ProviderAppOperations(t *testing.T) { }) } -// Helper functions +func requireCallbackURLValidationError(t *testing.T, err error) { + t.Helper() + + require.Error(t, err) + var apiErr *codersdk.Error + require.True(t, errors.As(err, &apiErr)) + require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) + require.True(t, slices.ContainsFunc(apiErr.Validations, func(validation codersdk.ValidationError) bool { + return validation.Field == "callback_url" + }), "expected callback_url validation error, got: %+v", apiErr.Validations) +} type provisionedApps struct { Default codersdk.OAuth2ProviderApp diff --git a/codersdk/name.go b/codersdk/name.go index b044fee7e8..f7cde93f42 100644 --- a/codersdk/name.go +++ b/codersdk/name.go @@ -59,6 +59,28 @@ func NameValid(str string) error { return nil } +// OAuth2AppNameMaxBytes is the maximum UTF-8 byte length of an OAuth2 +// application name. +const OAuth2AppNameMaxBytes = 64 + +// OAuth2AppNameValid returns whether the input string is a valid OAuth2 +// application name: a non-empty string of at most OAuth2AppNameMaxBytes UTF-8 +// bytes with no leading or trailing whitespace. Unlike NameValid, it allows +// spaces and other characters that appear in dynamically registered client +// names such as "VS Code Coder Extension". +func OAuth2AppNameValid(str string) error { + if len(str) < 1 { + return xerrors.New("must be >= 1 character") + } + if len(str) > OAuth2AppNameMaxBytes { + return xerrors.Errorf("must be <= %d bytes", OAuth2AppNameMaxBytes) + } + if strings.TrimSpace(str) != str { + return xerrors.New("must not have leading or trailing whitespace") + } + return nil +} + // TemplateVersionNameValid returns whether the input string is a valid template version name. func TemplateVersionNameValid(str string) error { if len(str) > 64 { diff --git a/codersdk/name_test.go b/codersdk/name_test.go index b4903846c4..5b517f5e32 100644 --- a/codersdk/name_test.go +++ b/codersdk/name_test.go @@ -1,6 +1,7 @@ package codersdk_test import ( + "fmt" "strings" "testing" @@ -287,3 +288,41 @@ func TestGroupNameValid(t *testing.T) { }) } } + +func TestOAuth2AppNameValid(t *testing.T) { + t.Parallel() + + testCases := []struct { + Name string + Valid bool + }{ + {"", false}, + {"a", true}, + {"my-app", true}, + // DCR-registered clients use spaces and other characters that the + // slug-style name validator rejects. + {"VS Code Coder Extension", true}, + {"Cursor (MCP)", true}, + {"new", true}, + {"create", true}, + {" leading", false}, + {"trailing ", false}, + {" ", false}, + {strings.Repeat("a", codersdk.OAuth2AppNameMaxBytes), true}, + {strings.Repeat("a", codersdk.OAuth2AppNameMaxBytes+1), false}, + {strings.Repeat("é", codersdk.OAuth2AppNameMaxBytes/2), true}, + {strings.Repeat("é", codersdk.OAuth2AppNameMaxBytes/2+1), false}, + } + for _, testCase := range testCases { + t.Run(testCase.Name, func(t *testing.T) { + t.Parallel() + err := codersdk.OAuth2AppNameValid(testCase.Name) + assert.Equal(t, testCase.Valid, err == nil, + "Test case %q failed: expected valid=%t but got error: %v", + testCase.Name, testCase.Valid, err) + if len(testCase.Name) > codersdk.OAuth2AppNameMaxBytes { + assert.EqualError(t, err, fmt.Sprintf("must be <= %d bytes", codersdk.OAuth2AppNameMaxBytes)) + } + }) + } +} diff --git a/codersdk/oauth2.go b/codersdk/oauth2.go index e5f68bc25a..458da2b783 100644 --- a/codersdk/oauth2.go +++ b/codersdk/oauth2.go @@ -80,7 +80,7 @@ func (c *Client) OAuth2ProviderApp(ctx context.Context, id uuid.UUID) (OAuth2Pro type PostOAuth2ProviderAppRequest struct { Name string `json:"name" validate:"required,oauth2_app_name"` - CallbackURL string `json:"callback_url" validate:"required,http_url"` + CallbackURL string `json:"callback_url" validate:"required,oauth2_callback_url"` Icon string `json:"icon" validate:"omitempty"` } @@ -101,7 +101,7 @@ func (c *Client) PostOAuth2ProviderApp(ctx context.Context, app PostOAuth2Provid type PutOAuth2ProviderAppRequest struct { Name string `json:"name" validate:"required,oauth2_app_name"` - CallbackURL string `json:"callback_url" validate:"required,http_url"` + CallbackURL string `json:"callback_url" validate:"required,oauth2_callback_url"` Icon string `json:"icon" validate:"omitempty"` } diff --git a/codersdk/oauth2_validation.go b/codersdk/oauth2_validation.go index 70d36b4a74..8e8f3f9ec4 100644 --- a/codersdk/oauth2_validation.go +++ b/codersdk/oauth2_validation.go @@ -18,7 +18,7 @@ func (req *OAuth2ClientRegistrationRequest) Validate() error { // The client type is derived once, by DetermineClientType, so which RFC 8252 // rules apply here cannot drift from what gets stored in client_type. - if err := validateRedirectURIs(req.RedirectURIs, req.DetermineClientType()); err != nil { + if err := ValidateRedirectURIs(req.RedirectURIs, req.DetermineClientType()); err != nil { return xerrors.Errorf("invalid redirect_uris: %w", err) } @@ -140,11 +140,11 @@ func validateScheme(u *url.URL) error { return nil } -// validateRedirectURIs validates redirect URIs according to RFC 7591, 8252. +// ValidateRedirectURIs validates redirect URIs according to RFC 7591, 8252. // clientType selects which rules apply and is derived by DetermineClientType, // the single owner of that mapping, so this cannot disagree with the type the // app is stored as. -func validateRedirectURIs(uris []string, clientType OAuth2ClientType) error { +func ValidateRedirectURIs(uris []string, clientType OAuth2ClientType) error { if len(uris) == 0 { return xerrors.New("at least one redirect URI is required") } diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index d7b1c3acad..ff0e84939b 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -6484,6 +6484,13 @@ export interface OAuth2AppEndpoints { readonly device_authorization: string; } +// From codersdk/name.go +/** + * OAuth2AppNameMaxBytes is the maximum UTF-8 byte length of an OAuth2 + * application name. + */ +export const OAuth2AppNameMaxBytes = 64; + // From codersdk/oauth2.go /** * OAuth2AuthorizationServerMetadata represents RFC 8414 OAuth 2.0 Authorization Server Metadata. diff --git a/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/CreateOAuth2AppPageView.stories.tsx b/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/CreateOAuth2AppPageView.stories.tsx index 72744a8ae4..a6dd4a1205 100644 --- a/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/CreateOAuth2AppPageView.stories.tsx +++ b/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/CreateOAuth2AppPageView.stories.tsx @@ -80,20 +80,29 @@ export const WithValidationError: Story = { }, }; -export const InvalidName: Story = { +export const InvalidCallbackURL: Story = { play: async ({ canvasElement }) => { const canvas = within(canvasElement); - const nameInput = await canvas.findByLabelText(/^name/i); - await userEvent.type(nameInput, "Foo@Application"); + await userEvent.type(await canvas.findByLabelText(/^name/i), "test-app"); + const callbackInput = canvas.getByLabelText(/callback url/i); + // oxlint-disable-next-line eslint/no-script-url -- Deliberately invalid input exercises callback URL rejection. + await userEvent.type(callbackInput, "javascript:alert(1)"); + await userEvent.tab(); + }, +}; + +export const DynamicallyRegisteredValues: Story = { + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + await userEvent.type( + await canvas.findByLabelText(/^name/i), + "VS Code Coder Extension", + ); + await userEvent.type( + canvas.getByLabelText(/callback url/i), + "vscode://coder.coder-remote/oauth/callback", + ); await userEvent.tab(); - await expect( - await canvas.findByText( - /special characters \(e\.g\.: !, @, #\) are not supported/i, - ), - ).toBeVisible(); - await expect( - canvas.getByRole("button", { name: /create application/i }), - ).toBeDisabled(); }, }; diff --git a/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/EditOAuth2AppPageView.stories.tsx b/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/EditOAuth2AppPageView.stories.tsx index 175d31bbe8..6cc95e4f16 100644 --- a/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/EditOAuth2AppPageView.stories.tsx +++ b/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/EditOAuth2AppPageView.stories.tsx @@ -155,6 +155,31 @@ export const DeleteDialogOpen: Story = { }, }; +export const DynamicallyRegisteredValues: Story = { + parameters: { + queries: [ + { + key: oauth2ProviderAppKey(appId), + data: { + ...mockApp, + name: "VS Code Coder Extension", + callback_url: "vscode://coder.coder-remote/oauth/callback", + }, + }, + { + key: oauth2ProviderAppSecretsKey(appId), + data: MockOAuth2ProviderAppSecrets, + }, + ], + }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + const nameField = await canvas.findByLabelText(/^name/i); + await userEvent.clear(nameField); + await userEvent.type(nameField, "Cursor (MCP)"); + }, +}; + export const PublicClient: Story = { parameters: { queries: [ diff --git a/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/OAuth2AppForm.test.tsx b/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/OAuth2AppForm.test.tsx new file mode 100644 index 0000000000..3477455083 --- /dev/null +++ b/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/OAuth2AppForm.test.tsx @@ -0,0 +1,225 @@ +import { act, screen, waitFor } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { + MockOAuth2ProviderAppPublic, + MockOAuth2ProviderApps, +} from "#/testHelpers/entities"; +import { render } from "#/testHelpers/renderHelpers"; +import { OAuth2AppForm } from "./OAuth2AppForm"; + +describe("OAuth2AppForm", () => { + it.each([ + "VS Code Coder Extension", + " VS Code Coder Extension", + "VS Code Coder Extension ", + " VS Code Coder Extension ", + ])("submits a trimmed name for %j", async (name) => { + const user = userEvent.setup(); + const onSubmit = vi.fn(); + + render( + , + ); + + await user.type(screen.getByLabelText(/^name/i), name); + await user.type( + screen.getByLabelText(/callback url/i), + "vscode://coder.coder-remote/oauth/callback", + ); + await user.click( + screen.getByRole("button", { name: /create application/i }), + ); + + await waitFor(() => { + expect(onSubmit).toHaveBeenCalledWith({ + name: "VS Code Coder Extension", + callback_url: "vscode://coder.coder-remote/oauth/callback", + icon: "", + }); + }); + }); + + it("submits edited dynamically registered client values", async () => { + const user = userEvent.setup(); + const onSubmit = vi.fn(); + const app = { + ...MockOAuth2ProviderApps[0], + name: "VS Code Coder Extension", + callback_url: "vscode://coder.coder-remote/oauth/callback", + }; + + render( + , + ); + + await user.clear(screen.getByLabelText(/^name/i)); + await user.type(screen.getByLabelText(/^name/i), " Cursor MCP Extension "); + await user.click( + screen.getByRole("button", { name: /update application/i }), + ); + + await waitFor(() => { + expect(onSubmit).toHaveBeenCalledWith({ + name: "Cursor MCP Extension", + callback_url: "vscode://coder.coder-remote/oauth/callback", + icon: app.icon, + }); + }); + }); + + it.each([ + // oxlint-disable-next-line eslint/no-script-url -- Deliberately invalid input exercises callback URL rejection. + "javascript:alert(1)", + // oxlint-disable-next-line eslint/no-script-url -- Scheme rejection is case insensitive. + "JaVaScRiPt:alert(1)", + "data:text/plain,hello", + "file:///tmp/callback", + "ftp://example.com/callback", + "urn:example:callback", + "http:foo", + "https:/example.com", + "http:///example.com", + "localhost:3000", + "vscode:", + "a:", + "vscode://", + "mailto:a@b", + "tel:+1234", + "sms:+1234", + ])("does not submit invalid callback %j", async (callback) => { + const user = userEvent.setup(); + const onSubmit = vi.fn(); + + render( + , + ); + + await user.type(screen.getByLabelText(/^name/i), "test-app"); + const callbackURL = screen.getByLabelText(/callback url/i); + await user.type(callbackURL, callback); + await user.click( + screen.getByRole("button", { name: /create application/i }), + ); + + await act(async () => {}); + expect(onSubmit).not.toHaveBeenCalled(); + }); + + it.each([ + " vscode://coder.coder-remote/oauth/callback", + "vscode://coder.coder-remote/oauth/callback ", + " vscode://coder.coder-remote/oauth/callback ", + "URN:ietf:wg:oauth:2.0:oob", + "com.example.app:/oauth2redirect", + "cursor://anysphere.cursor-mcp/oauth/callback", + ])("submits trimmed callback %j", async (callback) => { + const user = userEvent.setup(); + const onSubmit = vi.fn(); + render( + , + ); + await user.type(screen.getByLabelText(/^name/i), "OAuth App"); + await user.type(screen.getByLabelText(/callback url/i), callback); + await user.click( + screen.getByRole("button", { name: /create application/i }), + ); + await waitFor(() => + expect(onSubmit).toHaveBeenCalledWith({ + name: "OAuth App", + callback_url: callback.trim(), + icon: "", + }), + ); + }); + + it.each([ + { name: "é".repeat(32), valid: true }, + { name: "a" + "é".repeat(32), valid: false }, + { name: "é".repeat(33), valid: false }, + { name: "界".repeat(21), valid: true }, + { name: "界".repeat(22), valid: false }, + ])("enforces the UTF-8 byte limit for $name", async ({ name, valid }) => { + const user = userEvent.setup(); + const onSubmit = vi.fn(); + render( + , + ); + await user.type(screen.getByLabelText(/^name/i), name); + await user.type( + screen.getByLabelText(/callback url/i), + "https://example.com/callback", + ); + await user.click( + screen.getByRole("button", { name: /create application/i }), + ); + await act(async () => {}); + if (valid) { + await waitFor(() => + expect(onSubmit).toHaveBeenCalledWith({ + name, + callback_url: "https://example.com/callback", + icon: "", + }), + ); + } else { + expect(onSubmit).not.toHaveBeenCalled(); + } + }); + + it.each([ + { callback: "mailto:a@b", valid: false }, + { callback: "mailto://a@b", valid: false }, + { callback: "tel:+1234", valid: false }, + { callback: "tel:/1234", valid: false }, + { callback: "sms:+1234", valid: false }, + { callback: "sms:/1234", valid: false }, + { callback: "http://example.com/callback", valid: false }, + { callback: "https://example.com/callback#fragment", valid: false }, + { callback: "http://localhost:3000/callback", valid: true }, + { callback: "http://127.0.0.1:3000/callback", valid: true }, + { callback: "http://[::1]:3000/callback", valid: true }, + { callback: "https://example.com/callback", valid: true }, + { callback: "vscode://coder.coder-remote/oauth/callback", valid: true }, + { callback: "com.example.app:/oauth2redirect", valid: true }, + ])( + "validates public client callback $callback", + async ({ callback, valid }) => { + const user = userEvent.setup(); + const onSubmit = vi.fn(); + render( + , + ); + await user.clear(screen.getByLabelText(/callback url/i)); + await user.type( + screen.getByLabelText(/callback url/i), + callback.replaceAll("[", "[["), + ); + await user.type(screen.getByLabelText(/^name/i), " updated"); + await user.click( + screen.getByRole("button", { name: /update application/i }), + ); + await act(async () => {}); + if (valid) { + await waitFor(() => + expect(onSubmit).toHaveBeenCalledWith({ + name: `${MockOAuth2ProviderAppPublic.name} updated`, + callback_url: callback, + icon: MockOAuth2ProviderAppPublic.icon, + }), + ); + } else { + expect(onSubmit).not.toHaveBeenCalled(); + } + }, + ); +}); diff --git a/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/OAuth2AppForm.tsx b/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/OAuth2AppForm.tsx index b561c40c1a..68ddd71839 100644 --- a/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/OAuth2AppForm.tsx +++ b/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/OAuth2AppForm.tsx @@ -4,6 +4,7 @@ import { type FC, useEffect, useRef } from "react"; import { Link } from "react-router"; import * as Yup from "yup"; import type * as TypesGen from "#/api/typesGenerated"; +import { OAuth2AppNameMaxBytes } from "#/api/typesGenerated"; import { ErrorAlert } from "#/components/Alert/ErrorAlert"; import { Button } from "#/components/Button/Button"; import { ConfirmDialog } from "#/components/Dialog/ConfirmDialog/ConfirmDialog"; @@ -13,12 +14,7 @@ import { IconField } from "#/components/IconField/IconField"; import { Label } from "#/components/Label/Label"; import { Spinner } from "#/components/Spinner/Spinner"; import { useUnsavedChangesPrompt } from "#/hooks/useUnsavedChangesPrompt"; -import { - getFormHelpers, - iconValidator, - nameValidator, - onChangeTrimmed, -} from "#/utils/formUtils"; +import { getFormHelpers, iconValidator } from "#/utils/formUtils"; type OAuth2AppFormValues = { name: string; @@ -38,28 +34,77 @@ type OAuth2AppFormProps = { const BACK_HREF = "/deployment/oauth2-provider/apps"; -const isHttpUrl = (value: string | undefined): boolean => { +// Mirror codersdk.ValidateRedirectURIScheme and httpapi's oauth2_callback_url. +// The server remains authoritative for URL syntax differences between parsers. +// oxlint-disable-next-line eslint/no-script-url -- This blocklist rejects the scheme; it is never used as a navigation target. +const DANGEROUS_CALLBACK_SCHEMES = ["javascript:", "data:", "file:", "ftp:"]; + +const isValidCallbackURL = ( + value: string | undefined, + isPublicClient: boolean, +): boolean => { if (!value) { return false; } try { const url = new URL(value); - return url.protocol === "http:" || url.protocol === "https:"; + if (url.protocol === "urn:") { + return url.href === "urn:ietf:wg:oauth:2.0:oob"; + } + if (DANGEROUS_CALLBACK_SCHEMES.includes(url.protocol)) { + return false; + } + const target = value.slice(value.indexOf(":") + 1); + if (!target.startsWith("/") || (!url.host && !url.pathname)) { + return false; + } + if (isPublicClient) { + if ( + value.includes("#") || + ["mailto:", "tel:", "sms:"].includes(url.protocol) + ) { + return false; + } + if ( + url.protocol === "http:" && + !["localhost", "127.0.0.1", "[::1]"].includes(url.hostname) + ) { + return false; + } + } + if ( + (url.protocol === "http:" || url.protocol === "https:") && + !/^https?:\/\/[^/\\\s]/i.test(value) + ) { + return false; + } + return true; } catch { return false; } }; -const validationSchema = Yup.object({ - name: nameValidator("Name"), - callback_url: Yup.string() - .trim() - .required("Please enter a callback URL.") - .test("http-url", "Callback URL must be a valid URL.", (value) => - isHttpUrl(value), - ), - icon: iconValidator, -}); +const validationSchema = (isPublicClient: boolean) => + Yup.object({ + name: Yup.string() + .trim() + .required("Please enter a name.") + .test( + "name-byte-length", + `Name cannot be longer than ${OAuth2AppNameMaxBytes} UTF-8 bytes.`, + (value) => + new TextEncoder().encode(value).length <= OAuth2AppNameMaxBytes, + ), + callback_url: Yup.string() + .trim() + .required("Please enter a callback URL.") + .test( + "valid-callback-url", + "Please enter a valid callback URL.", + (value) => isValidCallbackURL(value, isPublicClient), + ), + icon: iconValidator, + }); export const OAuth2AppForm: FC = ({ app, @@ -77,11 +122,15 @@ export const OAuth2AppForm: FC = ({ callback_url: app?.callback_url ?? defaultValues?.callback_url ?? "", icon: app?.icon ?? defaultValues?.icon ?? "", }, - validationSchema, + validationSchema: validationSchema(app?.client_type === "public"), validateOnMount: true, onSubmit: async (values) => { didSubmit.current = true; - await onSubmit(values); + await onSubmit({ + ...values, + name: values.name.trim(), + callback_url: values.callback_url.trim(), + }); }, }); const getFieldHelpers = getFormHelpers(form, error); @@ -124,7 +173,6 @@ export const OAuth2AppForm: FC = ({ label="Name" description="The name of your Coder app." disabled={formDisabled} - onChange={onChangeTrimmed(form)} autoFocus required />