Skip to content

Commit 9997b55

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 f298316 commit 9997b55

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)