mirror of
https://github.com/stack-auth/stack.git
synced 2026-06-04 21:04:37 +08:00
fix: address code review bot findings for browser-secret auth
Bug 1: Exhausted confirmation code lockout - Add attempts < MAX_ATTEMPTS check to init guard so a fresh code is generated once the previous one is exhausted. Bug 2: Heartbeat code delivery made idempotent - Rename consumeRemoteDevelopmentEnvironmentBrowserSecretConfirmationCodeForCli to peekRemoteDevelopmentEnvironmentBrowserSecretConfirmationCodeForCli (non- destructive). Always return the code until it expires or is consumed by submit. - CLI deduplicates locally so it only logs each code once. Bug 3: Handle browser-secret redirects in config-update - Catch RemoteDevelopmentEnvironmentBrowserSecretRedirectingError and return 'redirecting' instead of throwing. Bug 4: Guard malformed return_to URL - Wrap new URL() in try-catch in sameOriginReturnTo; fail closed to '/'. Bug 5: Localbound helper one-shot enforcement - Close the helper server after successfully issuing a browser secret. Bug 6: Auth gate before body parse in submit-confirmation-code - Run assertRemoteDevelopmentEnvironmentBrowserSecretSetupRequest before reading/parsing the JSON body. Bug 7: Guard response.json() in CLI heartbeat - Wrap response.json() in try-catch to handle unparseable responses. Also: reset process-global browser-secret state between tests and call vi.resetModules() in afterEach. Co-Authored-By: Konstantin Wohlwend <n2d4xc@gmail.com>
This commit is contained in:
parent
04113b5352
commit
dd0d7559b0
@ -1,4 +1,7 @@
|
||||
import { submitRemoteDevelopmentEnvironmentBrowserSecretConfirmationCode } from "@/lib/remote-development-environment/browser-secret";
|
||||
import {
|
||||
assertRemoteDevelopmentEnvironmentBrowserSecretSetupRequest,
|
||||
submitRemoteDevelopmentEnvironmentBrowserSecretConfirmationCode,
|
||||
} from "@/lib/remote-development-environment/browser-secret";
|
||||
import { readRemoteDevelopmentEnvironmentJsonBody } from "@/lib/remote-development-environment/route-json";
|
||||
import { NextRequest, NextResponse } from "next/server";
|
||||
|
||||
@ -17,6 +20,9 @@ function confirmationCodeFromBody(value: unknown): string | null {
|
||||
}
|
||||
|
||||
export async function POST(req: NextRequest) {
|
||||
const securityResponse = assertRemoteDevelopmentEnvironmentBrowserSecretSetupRequest(req);
|
||||
if (securityResponse != null) return securityResponse;
|
||||
|
||||
const parsedBody = await readRemoteDevelopmentEnvironmentJsonBody(req);
|
||||
if (parsedBody instanceof NextResponse) return parsedBody;
|
||||
const code = confirmationCodeFromBody(parsedBody);
|
||||
|
||||
@ -26,7 +26,12 @@ function parseSubmitResponse(value: unknown): { browserSecret: string } {
|
||||
function sameOriginReturnTo(searchParams: URLSearchParams): string {
|
||||
const returnTo = searchParams.get("return_to");
|
||||
if (returnTo == null) return "/";
|
||||
const parsed = new URL(returnTo, window.location.href);
|
||||
let parsed: URL;
|
||||
try {
|
||||
parsed = new URL(returnTo, window.location.href);
|
||||
} catch {
|
||||
return "/";
|
||||
}
|
||||
return parsed.origin === window.location.origin ? parsed.toString() : "/";
|
||||
}
|
||||
|
||||
|
||||
@ -2,7 +2,7 @@
|
||||
|
||||
import { Link } from "@/components/link";
|
||||
import { ActionDialog } from "@/components/ui/action-dialog";
|
||||
import { fetchWithRemoteDevelopmentEnvironmentBrowserSecret } from "@/app/remote-development-environment-browser-secret-client";
|
||||
import { fetchWithRemoteDevelopmentEnvironmentBrowserSecret, RemoteDevelopmentEnvironmentBrowserSecretRedirectingError } from "@/app/remote-development-environment-browser-secret-client";
|
||||
import { useDashboardInternalUser } from "@/lib/dashboard-user";
|
||||
import { getPublicEnvVar } from "@/lib/env";
|
||||
import type { OAuthConnection, PushedConfigSource, StackAdminApp } from "@hexclave/next";
|
||||
@ -492,21 +492,29 @@ function GithubPushBody({
|
||||
async function updateRemoteDevelopmentEnvironmentConfigFile(
|
||||
adminApp: StackAdminApp<false>,
|
||||
configUpdate: EnvironmentConfigOverrideOverride,
|
||||
): Promise<void> {
|
||||
const response = await fetchWithRemoteDevelopmentEnvironmentBrowserSecret("/api/remote-development-environment/config/apply-update", {
|
||||
method: "POST",
|
||||
headers: {
|
||||
"Content-Type": "application/json",
|
||||
Accept: "application/json",
|
||||
},
|
||||
body: JSON.stringify({
|
||||
project_id: adminApp.projectId,
|
||||
config_update: configUpdate,
|
||||
wait_for_sync: false,
|
||||
}),
|
||||
});
|
||||
if (!response.ok) {
|
||||
throw new Error(`Failed to update local development environment config (${response.status}): ${await response.text()}`);
|
||||
): Promise<"updated" | "redirecting"> {
|
||||
try {
|
||||
const response = await fetchWithRemoteDevelopmentEnvironmentBrowserSecret("/api/remote-development-environment/config/apply-update", {
|
||||
method: "POST",
|
||||
headers: {
|
||||
"Content-Type": "application/json",
|
||||
Accept: "application/json",
|
||||
},
|
||||
body: JSON.stringify({
|
||||
project_id: adminApp.projectId,
|
||||
config_update: configUpdate,
|
||||
wait_for_sync: false,
|
||||
}),
|
||||
});
|
||||
if (!response.ok) {
|
||||
throw new Error(`Failed to update local development environment config (${response.status}): ${await response.text()}`);
|
||||
}
|
||||
return "updated";
|
||||
} catch (error) {
|
||||
if (error instanceof RemoteDevelopmentEnvironmentBrowserSecretRedirectingError) {
|
||||
return "redirecting";
|
||||
}
|
||||
throw error;
|
||||
}
|
||||
}
|
||||
|
||||
@ -571,7 +579,9 @@ export function useUpdateConfig() {
|
||||
}
|
||||
|
||||
const project = await adminApp.getProject();
|
||||
await updateRemoteDevelopmentEnvironmentConfigFile(adminApp, configUpdate);
|
||||
if (await updateRemoteDevelopmentEnvironmentConfigFile(adminApp, configUpdate) === "redirecting") {
|
||||
return false;
|
||||
}
|
||||
// Update the remote project immediately so the dashboard reads the new value before the file sync lands.
|
||||
await project.updatePushedConfig(configUpdate);
|
||||
return true;
|
||||
|
||||
@ -345,7 +345,11 @@ export async function startRemoteDevelopmentEnvironmentBrowserSecretLocalboundSe
|
||||
response.statusCode = 200;
|
||||
response.setHeader("Content-Type", "application/json");
|
||||
response.setHeader("Cache-Control", "no-store");
|
||||
response.end(JSON.stringify({ browser_secret: browserSecret }));
|
||||
response.end(JSON.stringify({ browser_secret: browserSecret }), () => {
|
||||
// One-shot: shut down the helper after successfully issuing a secret.
|
||||
server.close();
|
||||
getGlobals().localboundHelper = undefined;
|
||||
});
|
||||
});
|
||||
|
||||
await new Promise<void>((resolvePromise, reject) => {
|
||||
@ -396,6 +400,7 @@ export function initRemoteDevelopmentEnvironmentBrowserSecretConfirmationCode(re
|
||||
if (
|
||||
existing != null &&
|
||||
unixNowMs() <= existing.expiresAtMs &&
|
||||
existing.attempts < CONFIRMATION_CODE_MAX_ATTEMPTS &&
|
||||
existing.targetHost === targetHost &&
|
||||
existing.targetOrigin === targetOrigin
|
||||
) {
|
||||
@ -447,11 +452,13 @@ export function submitRemoteDevelopmentEnvironmentBrowserSecretConfirmationCode(
|
||||
});
|
||||
}
|
||||
|
||||
export function consumeRemoteDevelopmentEnvironmentBrowserSecretConfirmationCodeForCli(): { code: string, expiresAtMillis: number } | null {
|
||||
export function peekRemoteDevelopmentEnvironmentBrowserSecretConfirmationCodeForCli(): { code: string, expiresAtMillis: number } | null {
|
||||
const confirmationCode = getGlobals().confirmationCode;
|
||||
if (confirmationCode == null || unixNowMs() > confirmationCode.expiresAtMs || confirmationCode.shownByCli) {
|
||||
if (confirmationCode == null || unixNowMs() > confirmationCode.expiresAtMs) {
|
||||
return null;
|
||||
}
|
||||
// Non-destructive: always return the code so retried/timed-out heartbeats
|
||||
// can still deliver it. The CLI deduplicates display locally.
|
||||
confirmationCode.shownByCli = true;
|
||||
return {
|
||||
code: confirmationCode.code,
|
||||
|
||||
@ -11,7 +11,7 @@ import { runAsynchronously } from "@hexclave/shared/dist/utils/promises";
|
||||
import { randomUUID } from "crypto";
|
||||
import { watch, type FSWatcher } from "fs";
|
||||
import { basename, dirname } from "path";
|
||||
import { consumeRemoteDevelopmentEnvironmentBrowserSecretConfirmationCodeForCli } from "./browser-secret";
|
||||
import { peekRemoteDevelopmentEnvironmentBrowserSecretConfirmationCodeForCli } from "./browser-secret";
|
||||
import {
|
||||
ensureConfigFileExists,
|
||||
readConfigFile,
|
||||
@ -569,7 +569,7 @@ export function heartbeatRemoteDevelopmentEnvironmentSession(sessionId: string):
|
||||
|
||||
export function getPendingRemoteDevelopmentEnvironmentBrowserSecretConfirmationCode(): { code: string, expiresAtMillis: number } | null {
|
||||
assertRemoteDevelopmentEnvironmentEnabled();
|
||||
return consumeRemoteDevelopmentEnvironmentBrowserSecretConfirmationCodeForCli();
|
||||
return peekRemoteDevelopmentEnvironmentBrowserSecretConfirmationCodeForCli();
|
||||
}
|
||||
|
||||
export function closeRemoteDevelopmentEnvironmentSession(sessionId: string): void {
|
||||
|
||||
@ -39,6 +39,9 @@ afterEach(() => {
|
||||
rmSync(tempDir, { recursive: true, force: true });
|
||||
tempDir = undefined;
|
||||
}
|
||||
// Reset process-global browser-secret state so tests don't leak into each other.
|
||||
delete (globalThis as Record<string, unknown>).__stackRemoteDevelopmentEnvironmentBrowserSecret;
|
||||
vi.resetModules();
|
||||
});
|
||||
|
||||
describe("remote development environment security", () => {
|
||||
|
||||
@ -521,6 +521,7 @@ async function heartbeatUntilStopped(sessionState: DashboardSessionState, option
|
||||
secret: string,
|
||||
shouldStop: () => boolean,
|
||||
}): Promise<void> {
|
||||
let lastLoggedConfirmationCode: string | null = null;
|
||||
while (!options.shouldStop()) {
|
||||
if (await waitForHeartbeatIntervalOrStop(options.shouldStop)) return;
|
||||
|
||||
@ -566,12 +567,23 @@ async function heartbeatUntilStopped(sessionState: DashboardSessionState, option
|
||||
continue;
|
||||
}
|
||||
|
||||
const heartbeatBody: unknown = await response.json();
|
||||
let heartbeatBody: unknown;
|
||||
try {
|
||||
heartbeatBody = await response.json();
|
||||
} catch {
|
||||
logDev("Development environment heartbeat returned unparseable JSON.");
|
||||
continue;
|
||||
}
|
||||
if (!isHeartbeatResponse(heartbeatBody)) {
|
||||
logDev("Development environment heartbeat returned an invalid response.");
|
||||
continue;
|
||||
}
|
||||
logBrowserSecretConfirmationCode(heartbeatBody);
|
||||
// Deduplicate: only log a confirmation code once per unique code value.
|
||||
if (heartbeatBody.browser_secret_confirmation_code != null &&
|
||||
heartbeatBody.browser_secret_confirmation_code !== lastLoggedConfirmationCode) {
|
||||
logBrowserSecretConfirmationCode(heartbeatBody);
|
||||
lastLoggedConfirmationCode = heartbeatBody.browser_secret_confirmation_code;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Loading…
Reference in New Issue
Block a user