fix: break the TechLog CSRF bootstrap cycle and close the review's fix-round-1 items
C1 (Critical): getStudioSession was stamped with the same TECH_LOG_STUDIO_SESSION auth profile as every other Studio operation, and that profile requires the CSRF header it is getStudioSession's own job to issue -- an unconditional cycle that recursed without bound in HTTP mode. Fixed with a credential-free TECH_LOG_STUDIO_BOOTSTRAP auth profile for getStudioSession alone, a synchronous re-entrancy guard in createCsrfTokenProvider as defense in depth, and a throwing stub in place of the prior `let x!: T` assertion. Added a composition-level regression test that wires the real executor, CSRF provider, and credential-attach function together and proves getStudioSession dispatches exactly once while its token reaches both a JSON operation and the multipart upload. Also: invalidate the cached CSRF token on a 401/403 from the upload path (I2), a throwing useStudioAssetGateway() accessor so Task 11 cannot silently compile a null-gateway UI (I3), and the M1-M5 minors from the review (guard a malformed success body, cover the untested error fallbacks, align aborted uploads with the JSON path's non-retryable CANCELLED mapping, derive the credential header name from one source instead of two, and correct the adapter review doc's operation count). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
c9c832c365
commit
2cab4974b7
@@ -24,14 +24,17 @@ export function createAssetUploadTransport(
|
||||
const doFetch = deps.fetch ?? globalThis.fetch.bind(globalThis);
|
||||
const endpoint = new URL("api/v1/studio/assets", deps.baseUrl).href;
|
||||
|
||||
function unavailable(detail: string): StudioGatewayError {
|
||||
function unavailable(
|
||||
detail: string,
|
||||
options?: Readonly<{ status?: number; retryable?: boolean }>,
|
||||
): StudioGatewayError {
|
||||
return new StudioGatewayError({
|
||||
type: "https://techlog.local/problems/studio-unavailable",
|
||||
title: "STUDIO_UNAVAILABLE",
|
||||
status: 503,
|
||||
status: options?.status ?? 503,
|
||||
detail,
|
||||
code: "STUDIO_UNAVAILABLE",
|
||||
retryable: true,
|
||||
retryable: options?.retryable ?? true,
|
||||
});
|
||||
}
|
||||
|
||||
@@ -61,13 +64,33 @@ export function createAssetUploadTransport(
|
||||
credentials: "include",
|
||||
});
|
||||
} catch (error) {
|
||||
if (signal.aborted) {
|
||||
// M3 (fix round 1). Align with the JSON path's `CANCELLED` mapping
|
||||
// (`toStudioGatewayError` maps it to `STUDIO_UNAVAILABLE` / 499 /
|
||||
// `retryable: false`) — a caller-cancelled or deadline-exceeded
|
||||
// upload is not a retryable outage, so it must not carry
|
||||
// `retryable: true` the way a genuine transport failure does.
|
||||
throw unavailable("Upload was aborted before it completed.", {
|
||||
status: 499,
|
||||
retryable: false,
|
||||
});
|
||||
}
|
||||
throw unavailable(
|
||||
error instanceof Error ? `Upload transport failed: ${error.message}` : "Upload transport failed.",
|
||||
);
|
||||
}
|
||||
|
||||
if (response.status === 201 || response.status === 200) {
|
||||
return (await response.json()) as Asset;
|
||||
try {
|
||||
return (await response.json()) as Asset;
|
||||
} catch {
|
||||
// M1 (fix round 1). This port's contract is `StudioGatewayError`;
|
||||
// a malformed success body must not throw a raw `SyntaxError` out
|
||||
// of it.
|
||||
throw unavailable(
|
||||
`Upload returned status ${response.status} with a body that could not be parsed as JSON.`,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
let problem: ProblemDetails | null;
|
||||
|
||||
@@ -10,6 +10,7 @@ import type {
|
||||
UploadAssetForm,
|
||||
} from "../../application/ports/studio-asset-gateway.ts";
|
||||
import type { IdempotentOptions, RequestOptions } from "../../application/ports/studio-gateway.ts";
|
||||
import { isStudioGatewayError } from "../../application/ports/studio-gateway-error.ts";
|
||||
import { toStudioGatewayError } from "./studio-error-mapping.ts";
|
||||
import { mutationIntent, type StudioOperationExecutor } from "./http-studio-gateway.ts";
|
||||
import type { CsrfTokenProvider } from "./studio-session-csrf.ts";
|
||||
@@ -62,14 +63,28 @@ export function createHttpStudioAssetGateway(
|
||||
deps.csrf.token(options.signal ? { signal: options.signal } : undefined),
|
||||
deps.csrf.headerName(options.signal ? { signal: options.signal } : undefined),
|
||||
]);
|
||||
return deps.upload.upload(
|
||||
form,
|
||||
Object.freeze({
|
||||
[headerName]: token,
|
||||
"Idempotency-Key": options.idempotencyKey,
|
||||
}),
|
||||
options.signal ? { signal: options.signal } : undefined,
|
||||
);
|
||||
try {
|
||||
return await deps.upload.upload(
|
||||
form,
|
||||
Object.freeze({
|
||||
[headerName]: token,
|
||||
"Idempotency-Key": options.idempotencyKey,
|
||||
}),
|
||||
options.signal ? { signal: options.signal } : undefined,
|
||||
);
|
||||
} catch (error) {
|
||||
// Fix round 1 (I2). This multipart path bypasses
|
||||
// `contractOperations.execute` entirely, so its `UNAUTHENTICATED`
|
||||
// branch (the only other place `techLogCsrf.invalidate()` is called)
|
||||
// never sees an upload's 401/403. Without this, a rejected token
|
||||
// stays cached for every other Studio operation until something else
|
||||
// happens to invalidate it. A CSRF-specific rejection usually arrives
|
||||
// as 403 rather than 401, so both are handled here.
|
||||
if (isStudioGatewayError(error) && (error.status === 401 || error.status === 403)) {
|
||||
deps.csrf.invalidate();
|
||||
}
|
||||
throw error;
|
||||
}
|
||||
},
|
||||
updateAssetMetadata: (assetId, cmd: UpdateAssetCommand, options) =>
|
||||
command<Asset>("updateStudioAsset", { assetId, ...cmd }, options),
|
||||
|
||||
@@ -0,0 +1,72 @@
|
||||
import type { CredentialPatchOutcome } from "../../../../adapters/http/http-contract-bridge.ts";
|
||||
import {
|
||||
CREDENTIAL_HEADER_NAMES,
|
||||
type CredentialHeaderName,
|
||||
} from "../../../../contracts/rest-profiles.ts";
|
||||
import type { CsrfTokenProvider } from "./studio-session-csrf.ts";
|
||||
|
||||
/**
|
||||
* Task 7 fix round 1 (C1, I1). `getStudioSession` is the operation that
|
||||
* *issues* the CSRF token every other Studio operation requires, so it must
|
||||
* not require one itself. Giving it the credential-free
|
||||
* {@link TECH_LOG_STUDIO_BOOTSTRAP_AUTH_PROFILE_ID} instead of
|
||||
* {@link TECH_LOG_STUDIO_SESSION_AUTH_PROFILE_ID} is what breaks the cycle;
|
||||
* see `rest-profiles.ts` and `tech-log-studio-contract-contribution.ts`.
|
||||
*/
|
||||
export const TECH_LOG_STUDIO_BOOTSTRAP_AUTH_PROFILE_ID =
|
||||
"TECH_LOG_STUDIO_BOOTSTRAP";
|
||||
|
||||
/** The seventeen other Studio operations — everything but `getStudioSession`. */
|
||||
export const TECH_LOG_STUDIO_SESSION_AUTH_PROFILE_ID = "TECH_LOG_STUDIO_SESSION";
|
||||
|
||||
function isCredentialHeaderName(value: string): value is CredentialHeaderName {
|
||||
return (CREDENTIAL_HEADER_NAMES as readonly string[]).includes(value);
|
||||
}
|
||||
|
||||
/**
|
||||
* The Studio branch of the platform's `attachCredentials` collaborator,
|
||||
* extracted so the composition root (`bootstrap/runtime-adapters.ts`) and its
|
||||
* regression test (`tests/features/tech-log/studio-csrf-composition.test.ts`)
|
||||
* call the exact same code — a duplicated copy in a test would only prove the
|
||||
* test's own understanding of the fix, not the production wiring.
|
||||
*
|
||||
* Returns `null` for any non-Studio `authProfileId` so the caller falls
|
||||
* through to its own (e.g. bearer) credential logic; that fallback is not
|
||||
* this function's concern.
|
||||
*/
|
||||
export async function attachStudioSessionCredentials(
|
||||
authProfileId: string,
|
||||
authContext: Readonly<{ signal?: AbortSignal }>,
|
||||
csrf: CsrfTokenProvider,
|
||||
): Promise<CredentialPatchOutcome | null> {
|
||||
if (authProfileId === TECH_LOG_STUDIO_BOOTSTRAP_AUTH_PROFILE_ID) {
|
||||
return Object.freeze({ kind: "READY" as const, headers: Object.freeze({}) });
|
||||
}
|
||||
if (authProfileId !== TECH_LOG_STUDIO_SESSION_AUTH_PROFILE_ID) {
|
||||
return null;
|
||||
}
|
||||
// Every operation other than `getStudioSession` uses this profile and
|
||||
// requires the header, so a failure to fetch the token fails only this one
|
||||
// request (`UNAVAILABLE`) and is not promoted to a session-level
|
||||
// `UNAUTHENTICATED`, which would trigger a global re-authentication flow
|
||||
// the session itself did not warrant.
|
||||
try {
|
||||
const [token, headerName] = await Promise.all([
|
||||
csrf.token({ signal: authContext.signal }),
|
||||
csrf.headerName({ signal: authContext.signal }),
|
||||
]);
|
||||
// One naming authority: the header name comes from the same provider the
|
||||
// multipart upload transport reads (`http-studio-asset-gateway.ts`'s
|
||||
// `uploadAsset`), not a second, independently hardcoded literal.
|
||||
const normalized = headerName.toLowerCase();
|
||||
if (!isCredentialHeaderName(normalized)) {
|
||||
return Object.freeze({ kind: "UNAVAILABLE" as const });
|
||||
}
|
||||
return Object.freeze({
|
||||
kind: "READY" as const,
|
||||
headers: Object.freeze({ [normalized]: token }),
|
||||
});
|
||||
} catch {
|
||||
return Object.freeze({ kind: "UNAVAILABLE" as const });
|
||||
}
|
||||
}
|
||||
@@ -16,28 +16,58 @@ export type CsrfTokenProvider = Readonly<{
|
||||
/**
|
||||
* CSRF는 전송 관심사다. UI는 토큰을 보지 않으므로 포트로 노출하지 않고
|
||||
* 어댑터 내부에서 캐시한다. 동시 요청은 하나의 in-flight 조회를 공유한다.
|
||||
*
|
||||
* Task 7 fix round 1 (C1). `deps.execute` must not itself require this
|
||||
* provider's token — if the caller wires it to an operation that is stamped
|
||||
* with an auth profile requiring this same CSRF header, `resolve()` re-enters
|
||||
* itself *synchronously*, before `inFlight` is assigned: `inFlight ??=
|
||||
* deps.execute(options).then(...)` evaluates its right-hand side (which calls
|
||||
* back into `resolve()` through the whole executor stack) before the `??=`
|
||||
* assignment completes, so the re-entrant call still sees `inFlight === null`
|
||||
* and recurses without bound (`RangeError: Maximum call stack size
|
||||
* exceeded`), not deduplicates. The `resolving` flag below is a synchronous
|
||||
* guard: it is set before `deps.execute` is invoked, so a re-entrant call
|
||||
* observes it and fails loudly and immediately with a clear diagnostic
|
||||
* instead of overflowing the stack. It does not replace giving the token's
|
||||
* own issuing operation a credential-free auth profile — it is the seatbelt
|
||||
* for the next time someone gets that wiring wrong.
|
||||
*/
|
||||
export function createCsrfTokenProvider(
|
||||
deps: Readonly<{ execute: CsrfSessionExecutor }>,
|
||||
): CsrfTokenProvider {
|
||||
let cached: StudioSessionSnapshot | null = null;
|
||||
let inFlight: Promise<StudioSessionSnapshot> | null = null;
|
||||
let resolving = false;
|
||||
|
||||
async function resolve(
|
||||
options?: Readonly<{ signal?: AbortSignal }>,
|
||||
): Promise<StudioSessionSnapshot> {
|
||||
if (cached) return cached;
|
||||
inFlight ??= deps.execute(options).then(
|
||||
(snapshot) => {
|
||||
cached = snapshot;
|
||||
inFlight = null;
|
||||
return snapshot;
|
||||
},
|
||||
(error: unknown) => {
|
||||
inFlight = null;
|
||||
throw error;
|
||||
},
|
||||
);
|
||||
if (inFlight) return inFlight;
|
||||
if (resolving) {
|
||||
throw new Error(
|
||||
"createCsrfTokenProvider: re-entered while fetching the token — the " +
|
||||
"operation that issues the token must not itself require this " +
|
||||
"provider's token. Give it a bootstrap auth profile with no " +
|
||||
"required credential headers instead of reusing the session profile.",
|
||||
);
|
||||
}
|
||||
resolving = true;
|
||||
try {
|
||||
inFlight = deps.execute(options).then(
|
||||
(snapshot) => {
|
||||
cached = snapshot;
|
||||
inFlight = null;
|
||||
return snapshot;
|
||||
},
|
||||
(error: unknown) => {
|
||||
inFlight = null;
|
||||
throw error;
|
||||
},
|
||||
);
|
||||
} finally {
|
||||
resolving = false;
|
||||
}
|
||||
return inFlight;
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user