Skip to content

Commit 3dcd2ac

Browse files
ThomasK33ThomasK33
authored
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 <[email protected]>
1 parent f298316 commit 3dcd2ac

File tree

11 files changed

+174
-47
lines changed

11 files changed

+174
-47
lines changed

‎.claude/scripts/format.sh

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,87 @@
66

77
set -euo pipefail
88

9+
# A variable to memoize the command for canonicalizing paths.
10+
_CANONICALIZE_CMD=""
11+
12+
# canonicalize_path resolves a path to its absolute, canonical form.
13+
# It tries 'realpath' and 'readlink -f' in order.
14+
# The chosen command is memoized to avoid repeated checks.
15+
# If none of these are available, it returns an empty string.
16+
canonicalize_path() {
17+
local path_to_resolve="$1"
18+
19+
# If we haven't determined a command yet, find one.
20+
if [[ -z "$_CANONICALIZE_CMD" ]]; then
21+
if command -v realpath >/dev/null 2>&1; then
22+
_CANONICALIZE_CMD="realpath"
23+
elif command -v readlink >/dev/null 2>&1 && readlink -f . >/dev/null 2>&1; then
24+
_CANONICALIZE_CMD="readlink"
25+
else
26+
# No command found, so we can't resolve.
27+
# We set a "none" value to prevent re-checking.
28+
_CANONICALIZE_CMD="none"
29+
fi
30+
fi
31+
32+
# Now, execute the command.
33+
case "$_CANONICALIZE_CMD" in
34+
realpath)
35+
realpath "$path_to_resolve" 2>/dev/null
36+
;;
37+
readlink)
38+
readlink -f "$path_to_resolve" 2>/dev/null
39+
;;
40+
*)
41+
# This handles the "none" case or any unexpected error.
42+
echo ""
43+
;;
44+
esac
45+
}
46+
947
# Read JSON input from stdin
1048
input=$(cat)
1149

1250
# Extract the file path from the JSON input
1351
# Expected format: {"tool_input": {"file_path": "/absolute/path/to/file"}} or {"tool_response": {"filePath": "/absolute/path/to/file"}}
1452
file_path=$(echo "$input" | jq -r '.tool_input.file_path // .tool_response.filePath // empty')
1553

54+
# Secure path canonicalization to prevent path traversal attacks
55+
# Resolve repo root to an absolute, canonical path.
56+
repo_root_raw="$(cd "$(dirname "$0")/../.." && pwd)"
57+
repo_root="$(canonicalize_path "$repo_root_raw")"
58+
if [[ -z "$repo_root" ]]; then
59+
# Fallback if canonicalization fails
60+
repo_root="$repo_root_raw"
61+
fi
62+
63+
# Resolve the input path to an absolute path
64+
if [[ "$file_path" = /* ]]; then
65+
# Already absolute
66+
abs_file_path="$file_path"
67+
else
68+
# Make relative paths absolute from repo root
69+
abs_file_path="$repo_root/$file_path"
70+
fi
71+
72+
# Canonicalize the path (resolve symlinks and ".." segments)
73+
canonical_file_path="$(canonicalize_path "$abs_file_path")"
74+
75+
# Check if canonicalization failed or if the resolved path is outside the repo
76+
if [[ -z "$canonical_file_path" ]] || { [[ "$canonical_file_path" != "$repo_root" ]] && [[ "$canonical_file_path" != "$repo_root"/* ]]; }; then
77+
echo "Error: File path is outside repository or invalid: $file_path" >&2
78+
exit 1
79+
fi
80+
81+
# Handle the case where the file path is the repository root itself.
82+
if [[ "$canonical_file_path" == "$repo_root" ]]; then
83+
echo "Warning: Formatting the repository root is not a supported operation. Skipping." >&2
84+
exit 0
85+
fi
86+
87+
# Convert back to relative path from repo root for consistency
88+
file_path="${canonical_file_path#"$repo_root"/}"
89+
1690
if [[ -z "$file_path" ]]; then
1791
echo "Error: No file path provided in input" >&2
1892
exit 1

‎.mcp.json

Lines changed: 35 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -1,36 +1,36 @@
11
{
2-
"mcpServers": {
3-
"go-language-server": {
4-
"type": "stdio",
5-
"command": "go",
6-
"args": [
7-
"run",
8-
"github.com/isaacphi/mcp-language-server@latest",
9-
"-workspace",
10-
"./",
11-
"-lsp",
12-
"go",
13-
"--",
14-
"run",
15-
"golang.org/x/tools/gopls@latest"
16-
],
17-
"env": {}
18-
},
19-
"typescript-language-server": {
20-
"type": "stdio",
21-
"command": "go",
22-
"args": [
23-
"run",
24-
"github.com/isaacphi/mcp-language-server@latest",
25-
"-workspace",
26-
"./site/",
27-
"-lsp",
28-
"pnpx",
29-
"--",
30-
"typescript-language-server",
31-
"--stdio"
32-
],
33-
"env": {}
34-
}
35-
}
36-
}
2+
"mcpServers": {
3+
"go-language-server": {
4+
"type": "stdio",
5+
"command": "go",
6+
"args": [
7+
"run",
8+
"github.com/isaacphi/mcp-language-server@latest",
9+
"-workspace",
10+
"./",
11+
"-lsp",
12+
"go",
13+
"--",
14+
"run",
15+
"golang.org/x/tools/gopls@latest"
16+
],
17+
"env": {}
18+
},
19+
"typescript-language-server": {
20+
"type": "stdio",
21+
"command": "go",
22+
"args": [
23+
"run",
24+
"github.com/isaacphi/mcp-language-server@latest",
25+
"-workspace",
26+
"./site/",
27+
"-lsp",
28+
"pnpx",
29+
"--",
30+
"typescript-language-server",
31+
"--stdio"
32+
],
33+
"env": {}
34+
}
35+
}
36+
}

‎coderd/httpmw/oauth2.go

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -225,7 +225,7 @@ func ExtractOAuth2ProviderAppWithOAuth2Errors(db database.Store) func(http.Handl
225225
type errorWriter interface {
226226
writeMissingClientID(ctx context.Context, rw http.ResponseWriter)
227227
writeInvalidClientID(ctx context.Context, rw http.ResponseWriter, err error)
228-
writeInvalidClient(ctx context.Context, rw http.ResponseWriter)
228+
writeClientNotFound(ctx context.Context, rw http.ResponseWriter)
229229
}
230230

231231
// codersdkErrorWriter writes standard codersdk errors for general API endpoints
@@ -244,9 +244,13 @@ func (*codersdkErrorWriter) writeInvalidClientID(ctx context.Context, rw http.Re
244244
})
245245
}
246246

247-
func (*codersdkErrorWriter) writeInvalidClient(ctx context.Context, rw http.ResponseWriter) {
248-
httpapi.Write(ctx, rw, http.StatusUnauthorized, codersdk.Response{
249-
Message: "Invalid OAuth2 client.",
247+
func (*codersdkErrorWriter) writeClientNotFound(ctx context.Context, rw http.ResponseWriter) {
248+
// Management API endpoints return 404 for missing OAuth2 apps (proper REST semantics).
249+
// This differs from OAuth2 protocol endpoints which return 401 "invalid_client" per RFC 6749.
250+
// Returning 401 here would trigger the frontend's automatic logout interceptor when React Query
251+
// refetches a deleted app, incorrectly logging out users who just deleted their own OAuth2 apps.
252+
httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{
253+
Message: "OAuth2 application not found.",
250254
})
251255
}
252256

@@ -261,7 +265,7 @@ func (*oauth2ErrorWriter) writeInvalidClientID(ctx context.Context, rw http.Resp
261265
httpapi.WriteOAuth2Error(ctx, rw, http.StatusUnauthorized, "invalid_client", "The client credentials are invalid")
262266
}
263267

264-
func (*oauth2ErrorWriter) writeInvalidClient(ctx context.Context, rw http.ResponseWriter) {
268+
func (*oauth2ErrorWriter) writeClientNotFound(ctx context.Context, rw http.ResponseWriter) {
265269
httpapi.WriteOAuth2Error(ctx, rw, http.StatusUnauthorized, "invalid_client", "The client credentials are invalid")
266270
}
267271

@@ -308,7 +312,7 @@ func extractOAuth2ProviderAppBase(db database.Store, errWriter errorWriter) func
308312

309313
app, err := db.GetOAuth2ProviderAppByID(ctx, appID)
310314
if httpapi.Is404Error(err) {
311-
errWriter.writeInvalidClient(ctx, rw)
315+
errWriter.writeClientNotFound(ctx, rw)
312316
return
313317
}
314318
if err != nil {

‎site/src/modules/dashboard/DashboardProvider.tsx

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
11
import { appearance } from "api/queries/appearance";
2+
import { buildInfo } from "api/queries/buildInfo";
23
import { entitlements } from "api/queries/entitlements";
34
import { experiments } from "api/queries/experiments";
45
import { organizations } from "api/queries/organizations";
56
import type {
67
AppearanceConfig,
8+
BuildInfoResponse,
79
Entitlements,
810
Experiment,
911
Organization,
@@ -21,6 +23,7 @@ export interface DashboardValue {
2123
entitlements: Entitlements;
2224
experiments: Experiment[];
2325
appearance: AppearanceConfig;
26+
buildInfo: BuildInfoResponse;
2427
organizations: readonly Organization[];
2528
showOrganizations: boolean;
2629
canViewOrganizationSettings: boolean;
@@ -36,12 +39,14 @@ export const DashboardProvider: FC<PropsWithChildren> = ({ children }) => {
3639
const entitlementsQuery = useQuery(entitlements(metadata.entitlements));
3740
const experimentsQuery = useQuery(experiments(metadata.experiments));
3841
const appearanceQuery = useQuery(appearance(metadata.appearance));
42+
const buildInfoQuery = useQuery(buildInfo(metadata["build-info"]));
3943
const organizationsQuery = useQuery(organizations());
4044

4145
const error =
4246
entitlementsQuery.error ||
4347
appearanceQuery.error ||
4448
experimentsQuery.error ||
49+
buildInfoQuery.error ||
4550
organizationsQuery.error;
4651

4752
if (error) {
@@ -52,6 +57,7 @@ export const DashboardProvider: FC<PropsWithChildren> = ({ children }) => {
5257
!entitlementsQuery.data ||
5358
!appearanceQuery.data ||
5459
!experimentsQuery.data ||
60+
!buildInfoQuery.data ||
5561
!organizationsQuery.data;
5662

5763
if (isLoading) {
@@ -70,6 +76,7 @@ export const DashboardProvider: FC<PropsWithChildren> = ({ children }) => {
7076
entitlements: entitlementsQuery.data,
7177
experiments: experimentsQuery.data,
7278
appearance: appearanceQuery.data,
79+
buildInfo: buildInfoQuery.data,
7380
organizations: organizationsQuery.data,
7481
showOrganizations,
7582
canViewOrganizationSettings:

‎site/src/modules/management/DeploymentSidebar.tsx

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,8 @@ import { DeploymentSidebarView } from "./DeploymentSidebarView";
88
*/
99
export const DeploymentSidebar: FC = () => {
1010
const { permissions } = useAuthenticated();
11-
const { entitlements, showOrganizations } = useDashboard();
11+
const { entitlements, showOrganizations, experiments, buildInfo } =
12+
useDashboard();
1213
const hasPremiumLicense =
1314
entitlements.features.multiple_organizations.enabled;
1415

@@ -17,6 +18,8 @@ export const DeploymentSidebar: FC = () => {
1718
permissions={permissions}
1819
showOrganizations={showOrganizations}
1920
hasPremiumLicense={hasPremiumLicense}
21+
experiments={experiments}
22+
buildInfo={buildInfo}
2023
/>
2124
);
2225
};

‎site/src/modules/management/DeploymentSidebarView.tsx

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import type { BuildInfoResponse, Experiment } from "api/typesGenerated";
12
import {
23
Sidebar as BaseSidebar,
34
SettingsSidebarNavItem as SidebarNavItem,
@@ -6,12 +7,15 @@ import { Stack } from "components/Stack/Stack";
67
import { ArrowUpRight } from "lucide-react";
78
import type { Permissions } from "modules/permissions";
89
import type { FC } from "react";
10+
import { isDevBuild } from "utils/buildInfo";
911

1012
interface DeploymentSidebarViewProps {
1113
/** Site-wide permissions. */
1214
permissions: Permissions;
1315
showOrganizations: boolean;
1416
hasPremiumLicense: boolean;
17+
experiments: Experiment[];
18+
buildInfo: BuildInfoResponse;
1519
}
1620

1721
/**
@@ -22,6 +26,8 @@ export const DeploymentSidebarView: FC<DeploymentSidebarViewProps> = ({
2226
permissions,
2327
showOrganizations,
2428
hasPremiumLicense,
29+
experiments,
30+
buildInfo,
2531
}) => {
2632
return (
2733
<BaseSidebar>
@@ -47,10 +53,12 @@ export const DeploymentSidebarView: FC<DeploymentSidebarViewProps> = ({
4753
External Authentication
4854
</SidebarNavItem>
4955
)}
50-
{/* Not exposing this yet since token exchange is not finished yet.
51-
<SidebarNavItem href="oauth2-provider/apps">
52-
OAuth2 Applications
53-
</SidebarNavItem>*/}
56+
{permissions.viewDeploymentConfig &&
57+
(experiments.includes("oauth2") || isDevBuild(buildInfo)) && (
58+
<SidebarNavItem href="/deployment/oauth2-provider/apps">
59+
OAuth2 Applications
60+
</SidebarNavItem>
61+
)}
5462
{permissions.viewDeploymentConfig && (
5563
<SidebarNavItem href="/deployment/network">Network</SidebarNavItem>
5664
)}

‎site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/EditOAuth2AppPageView.tsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,7 @@ export const EditOAuth2AppPageView: FC<EditOAuth2AppProps> = ({
141141
confirmLoading={mutatingResource.deleteApp}
142142
name={app.name}
143143
entity="OAuth2 application"
144+
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."
144145
onConfirm={() => deleteApp(app.name)}
145146
onCancel={() => setShowDelete(false)}
146147
/>

‎site/src/pages/UserSettingsPage/Sidebar.tsx

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,17 +13,19 @@ import {
1313
FingerprintIcon,
1414
KeyIcon,
1515
LockIcon,
16+
ShieldIcon,
1617
UserIcon,
1718
} from "lucide-react";
1819
import { useDashboard } from "modules/dashboard/useDashboard";
1920
import type { FC } from "react";
21+
import { isDevBuild } from "utils/buildInfo";
2022

2123
interface SidebarProps {
2224
user: User;
2325
}
2426

2527
export const Sidebar: FC<SidebarProps> = ({ user }) => {
26-
const { entitlements } = useDashboard();
28+
const { entitlements, experiments, buildInfo } = useDashboard();
2729
const showSchedulePage =
2830
entitlements.features.advanced_template_scheduling.enabled;
2931

@@ -43,6 +45,11 @@ export const Sidebar: FC<SidebarProps> = ({ user }) => {
4345
<SidebarNavItem href="external-auth" icon={GitIcon}>
4446
External Authentication
4547
</SidebarNavItem>
48+
{(experiments.includes("oauth2") || isDevBuild(buildInfo)) && (
49+
<SidebarNavItem href="oauth2-provider" icon={ShieldIcon}>
50+
OAuth2 Applications
51+
</SidebarNavItem>
52+
)}
4653
{showSchedulePage && (
4754
<SidebarNavItem href="schedule" icon={CalendarCogIcon}>
4855
Schedule

‎site/src/pages/WorkspacePage/WorkspacePage.test.tsx

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import type { FC } from "react";
1313
import { type Location, useLocation } from "react-router-dom";
1414
import {
1515
MockAppearanceConfig,
16+
MockBuildInfo,
1617
MockDeploymentConfig,
1718
MockEntitlements,
1819
MockFailedWorkspace,
@@ -554,6 +555,10 @@ describe("WorkspacePage", () => {
554555
appearance: MockAppearanceConfig,
555556
entitlements: MockEntitlements,
556557
experiments: [],
558+
buildInfo: {
559+
...MockBuildInfo,
560+
version: "v0.0.0-test",
561+
},
557562
organizations: [MockOrganization],
558563
showOrganizations: true,
559564
canViewOrganizationSettings: true,

0 commit comments

Comments
 (0)