Skip to content

Commit 932c04f

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 932c04f

File tree

11 files changed

+187
-49
lines changed

11 files changed

+187
-49
lines changed

‎.claude/scripts/format.sh

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,102 @@
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', 'readlink -f', 'python3', 'python', or 'perl' 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+
elif command -v python3 >/dev/null 2>&1; then
26+
_CANONICALIZE_CMD="python3"
27+
elif command -v python >/dev/null 2>&1; then
28+
_CANONICALIZE_CMD="python"
29+
elif command -v perl >/dev/null 2>&1; then
30+
_CANONICALIZE_CMD="perl"
31+
else
32+
# No command found, so we can't resolve.
33+
# We set a "none" value to prevent re-checking.
34+
_CANONICALIZE_CMD="none"
35+
fi
36+
fi
37+
38+
# Now, execute the command.
39+
case "$_CANONICALIZE_CMD" in
40+
realpath)
41+
realpath "$path_to_resolve" 2>/dev/null
42+
;;
43+
readlink)
44+
readlink -f "$path_to_resolve" 2>/dev/null
45+
;;
46+
python3)
47+
python3 -c "import os, sys; print(os.path.realpath(sys.argv[1]))" "$path_to_resolve" 2>/dev/null
48+
;;
49+
python)
50+
python -c "import os, sys; print(os.path.realpath(sys.argv[1]))" "$path_to_resolve" 2>/dev/null
51+
;;
52+
perl)
53+
perl -e 'use Cwd "abs_path"; print abs_path(shift)' "$path_to_resolve" 2>/dev/null
54+
;;
55+
*)
56+
# This handles the "none" case or any unexpected error.
57+
echo ""
58+
;;
59+
esac
60+
}
61+
962
# Read JSON input from stdin
1063
input=$(cat)
1164

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

69+
# Secure path canonicalization to prevent path traversal attacks
70+
# Resolve repo root to an absolute, canonical path.
71+
repo_root_raw="$(cd "$(dirname "$0")/../.." && pwd)"
72+
repo_root="$(canonicalize_path "$repo_root_raw")"
73+
if [[ -z "$repo_root" ]]; then
74+
# Fallback if canonicalization fails
75+
repo_root="$repo_root_raw"
76+
fi
77+
78+
# Resolve the input path to an absolute path
79+
if [[ "$file_path" = /* ]]; then
80+
# Already absolute
81+
abs_file_path="$file_path"
82+
else
83+
# Make relative paths absolute from repo root
84+
abs_file_path="$repo_root/$file_path"
85+
fi
86+
87+
# Canonicalize the path (resolve symlinks and ".." segments)
88+
canonical_file_path="$(canonicalize_path "$abs_file_path")"
89+
90+
# Check if canonicalization failed or if the resolved path is outside the repo
91+
if [[ -z "$canonical_file_path" ]] || { [[ "$canonical_file_path" != "$repo_root" ]] && [[ "$canonical_file_path" != "$repo_root"/* ]]; }; then
92+
echo "Error: File path is outside repository or invalid: $file_path" >&2
93+
exit 1
94+
fi
95+
96+
# Handle the case where the file path is the repository root itself.
97+
if [[ "$canonical_file_path" == "$repo_root" ]]; then
98+
echo "Warning: Formatting the repository root is not a supported operation. Skipping." >&2
99+
exit 0
100+
fi
101+
102+
# Convert back to relative path from repo root for consistency
103+
file_path="${canonical_file_path#"$repo_root"/}"
104+
16105
if [[ -z "$file_path" ]]; then
17106
echo "Error: No file path provided in input" >&2
18107
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,

0 commit comments

Comments
 (0)