Skip to content

fix: return 404 instead of 401 for missing OAuth2 apps #18755

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 7, 2025
+174 −47
Merged

fix: return 404 instead of 401 for missing OAuth2 apps #18755

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 74 additions & 0 deletions .claude/scripts/format.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,87 @@

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)

# Extract the file path from the JSON input
# 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
Expand Down
70 changes: 35 additions & 35 deletions .mcp.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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": {}
}
}
}
16 changes: 10 additions & 6 deletions coderd/httpmw/oauth2.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.",
})
}

Expand All @@ -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")
}

Expand Down Expand Up @@ -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 {
Expand Down
7 changes: 7 additions & 0 deletions site/src/modules/dashboard/DashboardProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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,
Expand All @@ -21,6 +23,7 @@ export interface DashboardValue {
entitlements: Entitlements;
experiments: Experiment[];
appearance: AppearanceConfig;
buildInfo: BuildInfoResponse;
organizations: readonly Organization[];
showOrganizations: boolean;
canViewOrganizationSettings: boolean;
Expand All @@ -36,12 +39,14 @@ export const DashboardProvider: FC<PropsWithChildren> = ({ 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) {
Expand All @@ -52,6 +57,7 @@ export const DashboardProvider: FC<PropsWithChildren> = ({ children }) => {
!entitlementsQuery.data ||
!appearanceQuery.data ||
!experimentsQuery.data ||
!buildInfoQuery.data ||
!organizationsQuery.data;

if (isLoading) {
Expand All @@ -70,6 +76,7 @@ export const DashboardProvider: FC<PropsWithChildren> = ({ children }) => {
entitlements: entitlementsQuery.data,
experiments: experimentsQuery.data,
appearance: appearanceQuery.data,
buildInfo: buildInfoQuery.data,
organizations: organizationsQuery.data,
showOrganizations,
canViewOrganizationSettings:
Expand Down
5 changes: 4 additions & 1 deletion site/src/modules/management/DeploymentSidebar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -17,6 +18,8 @@ export const DeploymentSidebar: FC = () => {
permissions={permissions}
showOrganizations={showOrganizations}
hasPremiumLicense={hasPremiumLicense}
experiments={experiments}
buildInfo={buildInfo}
/>
);
};
16 changes: 12 additions & 4 deletions site/src/modules/management/DeploymentSidebarView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import type { BuildInfoResponse, Experiment } from "api/typesGenerated";
import {
Sidebar as BaseSidebar,
SettingsSidebarNavItem as SidebarNavItem,
Expand All @@ -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;
}

/**
Expand All @@ -22,6 +26,8 @@ export const DeploymentSidebarView: FC<DeploymentSidebarViewProps> = ({
permissions,
showOrganizations,
hasPremiumLicense,
experiments,
buildInfo,
}) => {
return (
<BaseSidebar>
Expand All @@ -47,10 +53,12 @@ export const DeploymentSidebarView: FC<DeploymentSidebarViewProps> = ({
External Authentication
</SidebarNavItem>
)}
{/* Not exposing this yet since token exchange is not finished yet.
<SidebarNavItem href="oauth2-provider/apps">
OAuth2 Applications
</SidebarNavItem>*/}
{permissions.viewDeploymentConfig &&
(experiments.includes("oauth2") || isDevBuild(buildInfo)) && (
<SidebarNavItem href="/deployment/oauth2-provider/apps">
OAuth2 Applications
</SidebarNavItem>
)}
{permissions.viewDeploymentConfig && (
<SidebarNavItem href="/deployment/network">Network</SidebarNavItem>
)}
Expand Down
1 change: 1 addition & 0 deletions site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/EditOAuth2AppPageView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ export const EditOAuth2AppPageView: FC<EditOAuth2AppProps> = ({
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)}
/>
Expand Down
9 changes: 8 additions & 1 deletion site/src/pages/UserSettingsPage/Sidebar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<SidebarProps> = ({ user }) => {
const { entitlements } = useDashboard();
const { entitlements, experiments, buildInfo } = useDashboard();
const showSchedulePage =
entitlements.features.advanced_template_scheduling.enabled;

Expand All @@ -43,6 +45,11 @@ export const Sidebar: FC<SidebarProps> = ({ user }) => {
<SidebarNavItem href="external-auth" icon={GitIcon}>
External Authentication
</SidebarNavItem>
{(experiments.includes("oauth2") || isDevBuild(buildInfo)) && (
<SidebarNavItem href="oauth2-provider" icon={ShieldIcon}>
OAuth2 Applications
</SidebarNavItem>
)}
{showSchedulePage && (
<SidebarNavItem href="schedule" icon={CalendarCogIcon}>
Schedule
Expand Down
5 changes: 5 additions & 0 deletions site/src/pages/WorkspacePage/WorkspacePage.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import type { FC } from "react";
import { type Location, useLocation } from "react-router-dom";
import {
MockAppearanceConfig,
MockBuildInfo,
MockDeploymentConfig,
MockEntitlements,
MockFailedWorkspace,
Expand Down Expand Up @@ -554,6 +555,10 @@ describe("WorkspacePage", () => {
appearance: MockAppearanceConfig,
entitlements: MockEntitlements,
experiments: [],
buildInfo: {
...MockBuildInfo,
version: "v0.0.0-test",
},
organizations: [MockOrganization],
showOrganizations: true,
canViewOrganizationSettings: true,
Expand Down
Loading
Loading