Skip to content

Commit 7fac49d

Browse files
ThomasK33ThomasK33
committed
fix: return 404 instead of 401 for missing OAuth2 apps in management API
This prevents users from being logged out when deleting OAuth2 apps. The frontend interceptor triggers logout on 401 responses, but React Query refetches deleted apps and should get 404, not 401. Also adds conditional OAuth2 navigation when experiment is enabled. Change-Id: I48886144883539b7c51307f2a500f95be31dd383 Signed-off-by: Thomas Kosiewski <[email protected]>
1 parent 369bccd commit 7fac49d

File tree

11 files changed

+156
-49
lines changed

11 files changed

+156
-49
lines changed

‎.claude/scripts/format.sh

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

77
set -euo pipefail
88

9+
# canonicalize_path resolves a path to its absolute, canonical form.
10+
# It tries 'realpath', 'readlink -f', 'python3', 'python', or 'perl' in order.
11+
# If none of these are available, it returns an empty string.
12+
canonicalize_path() {
13+
local path_to_resolve="$1"
14+
local resolved_path=""
15+
16+
if command -v realpath >/dev/null 2>&1; then
17+
resolved_path="$(realpath "$path_to_resolve" 2>/dev/null)"
18+
elif command -v readlink >/dev/null 2>&1 && readlink -f . >/dev/null 2>&1; then
19+
resolved_path="$(readlink -f "$path_to_resolve" 2>/dev/null)"
20+
elif command -v python3 >/dev/null 2>&1; then
21+
resolved_path="$(python3 -c "import os, sys; print(os.path.realpath(sys.argv[1]))" "$path_to_resolve" 2>/dev/null)"
22+
elif command -v python >/dev/null 2>&1; then
23+
resolved_path="$(python -c "import os, sys; print(os.path.realpath(sys.argv[1]))" "$path_to_resolve" 2>/dev/null)"
24+
elif command -v perl >/dev/null 2>&1; then
25+
resolved_path="$(perl -e 'use Cwd "abs_path"; print abs_path(shift)' "$path_to_resolve" 2>/dev/null)"
26+
fi
27+
echo "$resolved_path"
28+
}
29+
930
# Read JSON input from stdin
1031
input=$(cat)
1132

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

37+
# Secure path canonicalization to prevent path traversal attacks
38+
# Resolve repo root to an absolute, canonical path.
39+
repo_root_raw="$(cd "$(dirname "$0")/../.." && pwd)"
40+
repo_root="$(canonicalize_path "$repo_root_raw")"
41+
if [[ -z "$repo_root" ]]; then
42+
# Fallback if canonicalization fails
43+
repo_root="$repo_root_raw"
44+
fi
45+
46+
# Resolve the input path to an absolute path
47+
if [[ "$file_path" = /* ]]; then
48+
# Already absolute
49+
abs_file_path="$file_path"
50+
else
51+
# Make relative paths absolute from repo root
52+
abs_file_path="$repo_root/$file_path"
53+
fi
54+
55+
# Canonicalize the path (resolve symlinks and ".." segments)
56+
canonical_file_path="$(canonicalize_path "$abs_file_path")"
57+
58+
# Check if canonicalization failed or if the resolved path is outside the repo
59+
if [[ -z "$canonical_file_path" ]] || { [[ "$canonical_file_path" != "$repo_root" ]] && [[ "$canonical_file_path" != "$repo_root"/* ]]; }; then
60+
echo "Error: File path is outside repository or invalid: $file_path" >&2
61+
exit 1
62+
fi
63+
64+
# Handle the case where the file path is the repository root itself.
65+
if [[ "$canonical_file_path" == "$repo_root" ]]; then
66+
echo "Warning: Formatting the repository root is not a supported operation. Skipping." >&2
67+
exit 0
68+
fi
69+
70+
# Convert back to relative path from repo root for consistency
71+
file_path="${canonical_file_path#"$repo_root"/}"
72+
1673
if [[ -z "$file_path" ]]; then
1774
echo "Error: No file path provided in input" >&2
1875
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="⚠️ Warning: 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: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,11 @@
11
import { screen, waitFor, within } from "@testing-library/react";
22
import userEvent from "@testing-library/user-event";
33
import * as apiModule from "api/api";
4-
import type { TemplateVersionParameter, Workspace } from "api/typesGenerated";
4+
import type {
5+
BuildInfoResponse,
6+
TemplateVersionParameter,
7+
Workspace,
8+
} from "api/typesGenerated";
59
import MockServerSocket from "jest-websocket-mock";
610
import {
711
DashboardContext,
@@ -554,6 +558,7 @@ describe("WorkspacePage", () => {
554558
appearance: MockAppearanceConfig,
555559
entitlements: MockEntitlements,
556560
experiments: [],
561+
buildInfo: { version: "v0.0.0-test" } as BuildInfoResponse,
557562
organizations: [MockOrganization],
558563
showOrganizations: true,
559564
canViewOrganizationSettings: true,

‎site/src/testHelpers/storybook.tsx

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import type { StoryContext } from "@storybook/react";
22
import { withDefaultFeatures } from "api/api";
33
import { getAuthorizationKey } from "api/queries/authCheck";
44
import { hasFirstUserKey, meKey } from "api/queries/users";
5-
import type { Entitlements } from "api/typesGenerated";
5+
import type { BuildInfoResponse, Entitlements } from "api/typesGenerated";
66
import { GlobalSnackbar } from "components/GlobalSnackbar/GlobalSnackbar";
77
import {
88
ProxyContext,
@@ -56,6 +56,7 @@ export const withDashboardProvider = (
5656
entitlements,
5757
experiments,
5858
appearance: MockAppearanceConfig,
59+
buildInfo: { version: "v0.0.0-test" } as BuildInfoResponse,
5960
organizations,
6061
showOrganizations,
6162
canViewOrganizationSettings,

0 commit comments

Comments
 (0)