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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+187 −49
Open

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
89 changes: 89 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,102 @@

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', 'readlink -f', 'python3', 'python', or 'perl' 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"
elif command -v python3 >/dev/null 2>&1; then
_CANONICALIZE_CMD="python3"
elif command -v python >/dev/null 2>&1; then
_CANONICALIZE_CMD="python"
elif command -v perl >/dev/null 2>&1; then
_CANONICALIZE_CMD="perl"
Comment on lines +26 to +30
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to run python or perl interpreters at all here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are fallbacks in case realpath or readlink are not available.

While working with CC hooks, I noticed that their file paths are wonky. Sometimes, they are relative, and sometimes, they are absolute, often a misformulation of both.

So this is just in case someone develops coder on an env that doesn’t have realpath or readlink.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is just in case someone develops coder on an env that doesn’t have realpath or readlink.

Can you give an example? I see these available in Nix and they're also available on darwin via Brew / coreutils.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

windows

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless we don't care about Windows development, then I'm happy to zap this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this is a CC hook, and CC only works on Linux/macOS.

I'll yeet the fallbacks later.

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
;;
python3)
python3 -c "import os, sys; print(os.path.realpath(sys.argv[1]))" "$path_to_resolve" 2>/dev/null
;;
python)
python -c "import os, sys; print(os.path.realpath(sys.argv[1]))" "$path_to_resolve" 2>/dev/null
;;
perl)
perl -e 'use Cwd "abs_path"; print abs_path(shift)' "$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="⚠️ 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."
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this message is displayed inside of a DeleteDialog, which already has a style to warn users, we don't need to prefix it with "⚠️ Warning:".

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
7 changes: 6 additions & 1 deletion site/src/pages/WorkspacePage/WorkspacePage.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
import { screen, waitFor, within } from "@testing-library/react";
import userEvent from "@testing-library/user-event";
import * as apiModule from "api/api";
import type { TemplateVersionParameter, Workspace } from "api/typesGenerated";
import type {
BuildInfoResponse,
TemplateVersionParameter,
Workspace,
} from "api/typesGenerated";
import MockServerSocket from "jest-websocket-mock";
import {
DashboardContext,
Expand Down Expand Up @@ -554,6 +558,7 @@ describe("WorkspacePage", () => {
appearance: MockAppearanceConfig,
entitlements: MockEntitlements,
experiments: [],
buildInfo: { version: "v0.0.0-test" } as BuildInfoResponse,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: You can use a more complete build info object, from our existent mocks, to avoid casting partial values using as. The advantage on that, IMHO, is to have a more reliable value. Time to time, I face some bugs in tests because the value types are correct, but the value is missing some properties.

To "fix" that you could do:

import { MockBuildInfo } from "testHelpers/entities"

return <DashboardContext.Provider
          value={{
            buildInfo: { 
              ..MockBuildInfo,
              version: "v0.0.0-test"
            }
          }}
        />

organizations: [MockOrganization],
showOrganizations: true,
canViewOrganizationSettings: true,
Expand Down
Loading
Loading