From 3dcd2acf1d500c936307dc3cfa046fe950871e42 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Mon, 7 Jul 2025 19:57:32 +0200 Subject: [PATCH] fix: return 404 instead of 401 for missing OAuth2 apps (#18755) ## Problem Users were being automatically logged out when deleting OAuth2 applications. ## Root Cause 1. User deletes OAuth2 app successfully 2. React Query automatically refetches the app data 3. Management API incorrectly returned **401 Unauthorized** for the missing app 4. Frontend axios interceptor sees 401 and calls `signOut()` 5. User gets logged out unexpectedly ## Solution - Change management API to return **404 Not Found** for missing OAuth2 apps - OAuth2 protocol endpoints continue returning 401 per RFC 6749 - Rename `writeInvalidClient` to `writeClientNotFound` for clarity ## Additional Changes - Add conditional OAuth2 navigation when experiment is enabled or in dev builds - Add `isDevBuild()` utility and `buildInfo` to dashboard context - Minor improvements to format script and warning dialogs Signed-off-by: Thomas Kosiewski --- .claude/scripts/format.sh | 74 +++++++++++++++++++ .mcp.json | 70 +++++++++--------- coderd/httpmw/oauth2.go | 16 ++-- .../modules/dashboard/DashboardProvider.tsx | 7 ++ .../modules/management/DeploymentSidebar.tsx | 5 +- .../management/DeploymentSidebarView.tsx | 16 +++- .../EditOAuth2AppPageView.tsx | 1 + site/src/pages/UserSettingsPage/Sidebar.tsx | 9 ++- .../WorkspacePage/WorkspacePage.test.tsx | 5 ++ site/src/testHelpers/storybook.tsx | 5 ++ site/src/utils/buildInfo.ts | 13 ++++ 11 files changed, 174 insertions(+), 47 deletions(-) diff --git a/.claude/scripts/format.sh b/.claude/scripts/format.sh index 4917de9afc..4d57c8cf17 100755 --- a/.claude/scripts/format.sh +++ b/.claude/scripts/format.sh @@ -6,6 +6,44 @@ set -euo pipefail +# A variable to memoize the command for canonicalizing paths. +_CANONICALIZE_CMD="" + +# canonicalize_path resolves a path to its absolute, canonical form. +# It tries 'realpath' and 'readlink -f' in order. +# The chosen command is memoized to avoid repeated checks. +# If none of these are available, it returns an empty string. +canonicalize_path() { + local path_to_resolve="$1" + + # If we haven't determined a command yet, find one. + if [[ -z "$_CANONICALIZE_CMD" ]]; then + if command -v realpath >/dev/null 2>&1; then + _CANONICALIZE_CMD="realpath" + elif command -v readlink >/dev/null 2>&1 && readlink -f . >/dev/null 2>&1; then + _CANONICALIZE_CMD="readlink" + else + # No command found, so we can't resolve. + # We set a "none" value to prevent re-checking. + _CANONICALIZE_CMD="none" + fi + fi + + # Now, execute the command. + case "$_CANONICALIZE_CMD" in + realpath) + realpath "$path_to_resolve" 2>/dev/null + ;; + readlink) + readlink -f "$path_to_resolve" 2>/dev/null + ;; + *) + # This handles the "none" case or any unexpected error. + echo "" + ;; + esac +} + # Read JSON input from stdin input=$(cat) @@ -13,6 +51,42 @@ input=$(cat) # Expected format: {"tool_input": {"file_path": "/absolute/path/to/file"}} or {"tool_response": {"filePath": "/absolute/path/to/file"}} file_path=$(echo "$input" | jq -r '.tool_input.file_path // .tool_response.filePath // empty') +# Secure path canonicalization to prevent path traversal attacks +# Resolve repo root to an absolute, canonical path. +repo_root_raw="$(cd "$(dirname "$0")/../.." && pwd)" +repo_root="$(canonicalize_path "$repo_root_raw")" +if [[ -z "$repo_root" ]]; then + # Fallback if canonicalization fails + repo_root="$repo_root_raw" +fi + +# Resolve the input path to an absolute path +if [[ "$file_path" = /* ]]; then + # Already absolute + abs_file_path="$file_path" +else + # Make relative paths absolute from repo root + abs_file_path="$repo_root/$file_path" +fi + +# Canonicalize the path (resolve symlinks and ".." segments) +canonical_file_path="$(canonicalize_path "$abs_file_path")" + +# Check if canonicalization failed or if the resolved path is outside the repo +if [[ -z "$canonical_file_path" ]] || { [[ "$canonical_file_path" != "$repo_root" ]] && [[ "$canonical_file_path" != "$repo_root"/* ]]; }; then + echo "Error: File path is outside repository or invalid: $file_path" >&2 + exit 1 +fi + +# Handle the case where the file path is the repository root itself. +if [[ "$canonical_file_path" == "$repo_root" ]]; then + echo "Warning: Formatting the repository root is not a supported operation. Skipping." >&2 + exit 0 +fi + +# Convert back to relative path from repo root for consistency +file_path="${canonical_file_path#"$repo_root"/}" + if [[ -z "$file_path" ]]; then echo "Error: No file path provided in input" >&2 exit 1 diff --git a/.mcp.json b/.mcp.json index 12820f97ce..3f3734e4fe 100644 --- a/.mcp.json +++ b/.mcp.json @@ -1,36 +1,36 @@ { - "mcpServers": { - "go-language-server": { - "type": "stdio", - "command": "go", - "args": [ - "run", - "github.com/isaacphi/mcp-language-server@latest", - "-workspace", - "./", - "-lsp", - "go", - "--", - "run", - "golang.org/x/tools/gopls@latest" - ], - "env": {} - }, - "typescript-language-server": { - "type": "stdio", - "command": "go", - "args": [ - "run", - "github.com/isaacphi/mcp-language-server@latest", - "-workspace", - "./site/", - "-lsp", - "pnpx", - "--", - "typescript-language-server", - "--stdio" - ], - "env": {} - } - } -} + "mcpServers": { + "go-language-server": { + "type": "stdio", + "command": "go", + "args": [ + "run", + "github.com/isaacphi/mcp-language-server@latest", + "-workspace", + "./", + "-lsp", + "go", + "--", + "run", + "golang.org/x/tools/gopls@latest" + ], + "env": {} + }, + "typescript-language-server": { + "type": "stdio", + "command": "go", + "args": [ + "run", + "github.com/isaacphi/mcp-language-server@latest", + "-workspace", + "./site/", + "-lsp", + "pnpx", + "--", + "typescript-language-server", + "--stdio" + ], + "env": {} + } + } +} \ No newline at end of file diff --git a/coderd/httpmw/oauth2.go b/coderd/httpmw/oauth2.go index 16dc517b10..28e6400c8a 100644 --- a/coderd/httpmw/oauth2.go +++ b/coderd/httpmw/oauth2.go @@ -225,7 +225,7 @@ func ExtractOAuth2ProviderAppWithOAuth2Errors(db database.Store) func(http.Handl type errorWriter interface { writeMissingClientID(ctx context.Context, rw http.ResponseWriter) writeInvalidClientID(ctx context.Context, rw http.ResponseWriter, err error) - writeInvalidClient(ctx context.Context, rw http.ResponseWriter) + writeClientNotFound(ctx context.Context, rw http.ResponseWriter) } // codersdkErrorWriter writes standard codersdk errors for general API endpoints @@ -244,9 +244,13 @@ func (*codersdkErrorWriter) writeInvalidClientID(ctx context.Context, rw http.Re }) } -func (*codersdkErrorWriter) writeInvalidClient(ctx context.Context, rw http.ResponseWriter) { - httpapi.Write(ctx, rw, http.StatusUnauthorized, codersdk.Response{ - Message: "Invalid OAuth2 client.", +func (*codersdkErrorWriter) writeClientNotFound(ctx context.Context, rw http.ResponseWriter) { + // Management API endpoints return 404 for missing OAuth2 apps (proper REST semantics). + // This differs from OAuth2 protocol endpoints which return 401 "invalid_client" per RFC 6749. + // Returning 401 here would trigger the frontend's automatic logout interceptor when React Query + // refetches a deleted app, incorrectly logging out users who just deleted their own OAuth2 apps. + httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{ + Message: "OAuth2 application not found.", }) } @@ -261,7 +265,7 @@ func (*oauth2ErrorWriter) writeInvalidClientID(ctx context.Context, rw http.Resp httpapi.WriteOAuth2Error(ctx, rw, http.StatusUnauthorized, "invalid_client", "The client credentials are invalid") } -func (*oauth2ErrorWriter) writeInvalidClient(ctx context.Context, rw http.ResponseWriter) { +func (*oauth2ErrorWriter) writeClientNotFound(ctx context.Context, rw http.ResponseWriter) { httpapi.WriteOAuth2Error(ctx, rw, http.StatusUnauthorized, "invalid_client", "The client credentials are invalid") } @@ -308,7 +312,7 @@ func extractOAuth2ProviderAppBase(db database.Store, errWriter errorWriter) func app, err := db.GetOAuth2ProviderAppByID(ctx, appID) if httpapi.Is404Error(err) { - errWriter.writeInvalidClient(ctx, rw) + errWriter.writeClientNotFound(ctx, rw) return } if err != nil { diff --git a/site/src/modules/dashboard/DashboardProvider.tsx b/site/src/modules/dashboard/DashboardProvider.tsx index 7eae04befa..87340489d7 100644 --- a/site/src/modules/dashboard/DashboardProvider.tsx +++ b/site/src/modules/dashboard/DashboardProvider.tsx @@ -1,9 +1,11 @@ import { appearance } from "api/queries/appearance"; +import { buildInfo } from "api/queries/buildInfo"; import { entitlements } from "api/queries/entitlements"; import { experiments } from "api/queries/experiments"; import { organizations } from "api/queries/organizations"; import type { AppearanceConfig, + BuildInfoResponse, Entitlements, Experiment, Organization, @@ -21,6 +23,7 @@ export interface DashboardValue { entitlements: Entitlements; experiments: Experiment[]; appearance: AppearanceConfig; + buildInfo: BuildInfoResponse; organizations: readonly Organization[]; showOrganizations: boolean; canViewOrganizationSettings: boolean; @@ -36,12 +39,14 @@ export const DashboardProvider: FC = ({ children }) => { const entitlementsQuery = useQuery(entitlements(metadata.entitlements)); const experimentsQuery = useQuery(experiments(metadata.experiments)); const appearanceQuery = useQuery(appearance(metadata.appearance)); + const buildInfoQuery = useQuery(buildInfo(metadata["build-info"])); const organizationsQuery = useQuery(organizations()); const error = entitlementsQuery.error || appearanceQuery.error || experimentsQuery.error || + buildInfoQuery.error || organizationsQuery.error; if (error) { @@ -52,6 +57,7 @@ export const DashboardProvider: FC = ({ children }) => { !entitlementsQuery.data || !appearanceQuery.data || !experimentsQuery.data || + !buildInfoQuery.data || !organizationsQuery.data; if (isLoading) { @@ -70,6 +76,7 @@ export const DashboardProvider: FC = ({ children }) => { entitlements: entitlementsQuery.data, experiments: experimentsQuery.data, appearance: appearanceQuery.data, + buildInfo: buildInfoQuery.data, organizations: organizationsQuery.data, showOrganizations, canViewOrganizationSettings: diff --git a/site/src/modules/management/DeploymentSidebar.tsx b/site/src/modules/management/DeploymentSidebar.tsx index b202b46f3d..8c3138e769 100644 --- a/site/src/modules/management/DeploymentSidebar.tsx +++ b/site/src/modules/management/DeploymentSidebar.tsx @@ -8,7 +8,8 @@ import { DeploymentSidebarView } from "./DeploymentSidebarView"; */ export const DeploymentSidebar: FC = () => { const { permissions } = useAuthenticated(); - const { entitlements, showOrganizations } = useDashboard(); + const { entitlements, showOrganizations, experiments, buildInfo } = + useDashboard(); const hasPremiumLicense = entitlements.features.multiple_organizations.enabled; @@ -17,6 +18,8 @@ export const DeploymentSidebar: FC = () => { permissions={permissions} showOrganizations={showOrganizations} hasPremiumLicense={hasPremiumLicense} + experiments={experiments} + buildInfo={buildInfo} /> ); }; diff --git a/site/src/modules/management/DeploymentSidebarView.tsx b/site/src/modules/management/DeploymentSidebarView.tsx index 3576f96f3c..5dfc86593d 100644 --- a/site/src/modules/management/DeploymentSidebarView.tsx +++ b/site/src/modules/management/DeploymentSidebarView.tsx @@ -1,3 +1,4 @@ +import type { BuildInfoResponse, Experiment } from "api/typesGenerated"; import { Sidebar as BaseSidebar, SettingsSidebarNavItem as SidebarNavItem, @@ -6,12 +7,15 @@ import { Stack } from "components/Stack/Stack"; import { ArrowUpRight } from "lucide-react"; import type { Permissions } from "modules/permissions"; import type { FC } from "react"; +import { isDevBuild } from "utils/buildInfo"; interface DeploymentSidebarViewProps { /** Site-wide permissions. */ permissions: Permissions; showOrganizations: boolean; hasPremiumLicense: boolean; + experiments: Experiment[]; + buildInfo: BuildInfoResponse; } /** @@ -22,6 +26,8 @@ export const DeploymentSidebarView: FC = ({ permissions, showOrganizations, hasPremiumLicense, + experiments, + buildInfo, }) => { return ( @@ -47,10 +53,12 @@ export const DeploymentSidebarView: FC = ({ External Authentication )} - {/* Not exposing this yet since token exchange is not finished yet. - - OAuth2 Applications - */} + {permissions.viewDeploymentConfig && + (experiments.includes("oauth2") || isDevBuild(buildInfo)) && ( + + OAuth2 Applications + + )} {permissions.viewDeploymentConfig && ( Network )} diff --git a/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/EditOAuth2AppPageView.tsx b/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/EditOAuth2AppPageView.tsx index 11a257c041..6c5490275b 100644 --- a/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/EditOAuth2AppPageView.tsx +++ b/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/EditOAuth2AppPageView.tsx @@ -141,6 +141,7 @@ export const EditOAuth2AppPageView: FC = ({ confirmLoading={mutatingResource.deleteApp} name={app.name} entity="OAuth2 application" + info="Deleting this OAuth2 application will immediately invalidate all active sessions and API keys associated with it. Users currently authenticated through this application will be logged out and need to re-authenticate." onConfirm={() => deleteApp(app.name)} onCancel={() => setShowDelete(false)} /> diff --git a/site/src/pages/UserSettingsPage/Sidebar.tsx b/site/src/pages/UserSettingsPage/Sidebar.tsx index 5503f32799..fd9b342227 100644 --- a/site/src/pages/UserSettingsPage/Sidebar.tsx +++ b/site/src/pages/UserSettingsPage/Sidebar.tsx @@ -13,17 +13,19 @@ import { FingerprintIcon, KeyIcon, LockIcon, + ShieldIcon, UserIcon, } from "lucide-react"; import { useDashboard } from "modules/dashboard/useDashboard"; import type { FC } from "react"; +import { isDevBuild } from "utils/buildInfo"; interface SidebarProps { user: User; } export const Sidebar: FC = ({ user }) => { - const { entitlements } = useDashboard(); + const { entitlements, experiments, buildInfo } = useDashboard(); const showSchedulePage = entitlements.features.advanced_template_scheduling.enabled; @@ -43,6 +45,11 @@ export const Sidebar: FC = ({ user }) => { External Authentication + {(experiments.includes("oauth2") || isDevBuild(buildInfo)) && ( + + OAuth2 Applications + + )} {showSchedulePage && ( Schedule diff --git a/site/src/pages/WorkspacePage/WorkspacePage.test.tsx b/site/src/pages/WorkspacePage/WorkspacePage.test.tsx index 67a1a460dc..ad320018da 100644 --- a/site/src/pages/WorkspacePage/WorkspacePage.test.tsx +++ b/site/src/pages/WorkspacePage/WorkspacePage.test.tsx @@ -13,6 +13,7 @@ import type { FC } from "react"; import { type Location, useLocation } from "react-router-dom"; import { MockAppearanceConfig, + MockBuildInfo, MockDeploymentConfig, MockEntitlements, MockFailedWorkspace, @@ -554,6 +555,10 @@ describe("WorkspacePage", () => { appearance: MockAppearanceConfig, entitlements: MockEntitlements, experiments: [], + buildInfo: { + ...MockBuildInfo, + version: "v0.0.0-test", + }, organizations: [MockOrganization], showOrganizations: true, canViewOrganizationSettings: true, diff --git a/site/src/testHelpers/storybook.tsx b/site/src/testHelpers/storybook.tsx index 4b2ba94bd2..beceaf099b 100644 --- a/site/src/testHelpers/storybook.tsx +++ b/site/src/testHelpers/storybook.tsx @@ -18,6 +18,7 @@ import type { FC } from "react"; import { useQueryClient } from "react-query"; import { MockAppearanceConfig, + MockBuildInfo, MockDefaultOrganization, MockDeploymentConfig, MockEntitlements, @@ -56,6 +57,10 @@ export const withDashboardProvider = ( entitlements, experiments, appearance: MockAppearanceConfig, + buildInfo: { + ...MockBuildInfo, + version: "v0.0.0-test", + }, organizations, showOrganizations, canViewOrganizationSettings, diff --git a/site/src/utils/buildInfo.ts b/site/src/utils/buildInfo.ts index 370ebb9338..ade54b6357 100644 --- a/site/src/utils/buildInfo.ts +++ b/site/src/utils/buildInfo.ts @@ -22,3 +22,16 @@ export const getStaticBuildInfo = () => { return CACHED_BUILD_INFO; }; + +// Check if the current build is a development build. +// Development builds have versions containing "-devel" or "v0.0.0". +// This matches the backend's buildinfo.IsDev() logic. +export const isDevBuild = (input: BuildInfoResponse): boolean => { + const version = input.version; + if (!version) { + return false; + } + + // Check for dev version pattern (contains "-devel") or no version (v0.0.0) + return version.includes("-devel") || version === "v0.0.0"; +};