mirror of
https://github.com/coder/coder.git
synced 2025-07-13 21:36:50 +00:00
fix: return 404 instead of 401 for missing OAuth2 apps (#18755)
## Problem Users were being automatically logged out when deleting OAuth2 applications. ## Root Cause 1. User deletes OAuth2 app successfully 2. React Query automatically refetches the app data 3. Management API incorrectly returned **401 Unauthorized** for the missing app 4. Frontend axios interceptor sees 401 and calls `signOut()` 5. User gets logged out unexpectedly ## Solution - Change management API to return **404 Not Found** for missing OAuth2 apps - OAuth2 protocol endpoints continue returning 401 per RFC 6749 - Rename `writeInvalidClient` to `writeClientNotFound` for clarity ## Additional Changes - Add conditional OAuth2 navigation when experiment is enabled or in dev builds - Add `isDevBuild()` utility and `buildInfo` to dashboard context - Minor improvements to format script and warning dialogs Signed-off-by: Thomas Kosiewski <tk@coder.com>
This commit is contained in:
@ -6,6 +6,44 @@
|
||||
|
||||
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)
|
||||
|
||||
@ -13,6 +51,42 @@ input=$(cat)
|
||||
# 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
|
||||
|
70
.mcp.json
70
.mcp.json
@ -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": {}
|
||||
}
|
||||
}
|
||||
}
|
@ -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
|
||||
@ -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.",
|
||||
})
|
||||
}
|
||||
|
||||
@ -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")
|
||||
}
|
||||
|
||||
@ -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 {
|
||||
|
@ -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,
|
||||
@ -21,6 +23,7 @@ export interface DashboardValue {
|
||||
entitlements: Entitlements;
|
||||
experiments: Experiment[];
|
||||
appearance: AppearanceConfig;
|
||||
buildInfo: BuildInfoResponse;
|
||||
organizations: readonly Organization[];
|
||||
showOrganizations: boolean;
|
||||
canViewOrganizationSettings: boolean;
|
||||
@ -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) {
|
||||
@ -52,6 +57,7 @@ export const DashboardProvider: FC<PropsWithChildren> = ({ children }) => {
|
||||
!entitlementsQuery.data ||
|
||||
!appearanceQuery.data ||
|
||||
!experimentsQuery.data ||
|
||||
!buildInfoQuery.data ||
|
||||
!organizationsQuery.data;
|
||||
|
||||
if (isLoading) {
|
||||
@ -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:
|
||||
|
@ -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;
|
||||
|
||||
@ -17,6 +18,8 @@ export const DeploymentSidebar: FC = () => {
|
||||
permissions={permissions}
|
||||
showOrganizations={showOrganizations}
|
||||
hasPremiumLicense={hasPremiumLicense}
|
||||
experiments={experiments}
|
||||
buildInfo={buildInfo}
|
||||
/>
|
||||
);
|
||||
};
|
||||
|
@ -1,3 +1,4 @@
|
||||
import type { BuildInfoResponse, Experiment } from "api/typesGenerated";
|
||||
import {
|
||||
Sidebar as BaseSidebar,
|
||||
SettingsSidebarNavItem as SidebarNavItem,
|
||||
@ -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;
|
||||
}
|
||||
|
||||
/**
|
||||
@ -22,6 +26,8 @@ export const DeploymentSidebarView: FC<DeploymentSidebarViewProps> = ({
|
||||
permissions,
|
||||
showOrganizations,
|
||||
hasPremiumLicense,
|
||||
experiments,
|
||||
buildInfo,
|
||||
}) => {
|
||||
return (
|
||||
<BaseSidebar>
|
||||
@ -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>
|
||||
)}
|
||||
|
@ -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)}
|
||||
/>
|
||||
|
@ -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;
|
||||
|
||||
@ -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
|
||||
|
@ -13,6 +13,7 @@ import type { FC } from "react";
|
||||
import { type Location, useLocation } from "react-router-dom";
|
||||
import {
|
||||
MockAppearanceConfig,
|
||||
MockBuildInfo,
|
||||
MockDeploymentConfig,
|
||||
MockEntitlements,
|
||||
MockFailedWorkspace,
|
||||
@ -554,6 +555,10 @@ describe("WorkspacePage", () => {
|
||||
appearance: MockAppearanceConfig,
|
||||
entitlements: MockEntitlements,
|
||||
experiments: [],
|
||||
buildInfo: {
|
||||
...MockBuildInfo,
|
||||
version: "v0.0.0-test",
|
||||
},
|
||||
organizations: [MockOrganization],
|
||||
showOrganizations: true,
|
||||
canViewOrganizationSettings: true,
|
||||
|
@ -18,6 +18,7 @@ import type { FC } from "react";
|
||||
import { useQueryClient } from "react-query";
|
||||
import {
|
||||
MockAppearanceConfig,
|
||||
MockBuildInfo,
|
||||
MockDefaultOrganization,
|
||||
MockDeploymentConfig,
|
||||
MockEntitlements,
|
||||
@ -56,6 +57,10 @@ export const withDashboardProvider = (
|
||||
entitlements,
|
||||
experiments,
|
||||
appearance: MockAppearanceConfig,
|
||||
buildInfo: {
|
||||
...MockBuildInfo,
|
||||
version: "v0.0.0-test",
|
||||
},
|
||||
organizations,
|
||||
showOrganizations,
|
||||
canViewOrganizationSettings,
|
||||
|
@ -22,3 +22,16 @@ export const getStaticBuildInfo = () => {
|
||||
|
||||
return CACHED_BUILD_INFO;
|
||||
};
|
||||
|
||||
// Check if the current build is a development build.
|
||||
// Development builds have versions containing "-devel" or "v0.0.0".
|
||||
// This matches the backend's buildinfo.IsDev() logic.
|
||||
export const isDevBuild = (input: BuildInfoResponse): boolean => {
|
||||
const version = input.version;
|
||||
if (!version) {
|
||||
return false;
|
||||
}
|
||||
|
||||
// Check for dev version pattern (contains "-devel") or no version (v0.0.0)
|
||||
return version.includes("-devel") || version === "v0.0.0";
|
||||
};
|
||||
|
Reference in New Issue
Block a user