Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
28 changes: 15 additions & 13 deletions apps/server/src/assets/AssetAccess.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,12 @@ import * as DateTime from "effect/DateTime";
import * as Effect from "effect/Effect";
import * as FileSystem from "effect/FileSystem";
import * as Layer from "effect/Layer";
import * as Redacted from "effect/Redacted";
import * as Path from "effect/Path";
import * as PlatformError from "effect/PlatformError";
import * as Schema from "effect/Schema";
import * as TestClock from "effect/testing/TestClock";
import { HttpClient, HttpClientResponse, HttpServerResponse } from "effect/http";
import { ChildProcessSpawner } from "effect/process";
import { vi } from "vite-plus/test";

import * as ServerSecretStore from "../auth/ServerSecretStore.ts";
Expand All @@ -35,7 +35,7 @@ import { ASSET_ROUTE_PREFIX, issueAssetUrl, resolveAsset } from "./AssetAccess.t
import * as NativeAppIconResolver from "./NativeAppIconResolver.ts";
import { openMediaFile } from "./MediaFile.ts";
import { symlinksSupported } from "@t3tools/shared/testing/symlinks";
import * as GitHubCli from "../sourceControl/GitHubCli.ts";
import * as GitHubCredentials from "../sourceControl/GitHubCredentials.ts";
import { githubMediaResponse } from "./GitHubMediaFetch.ts";

vi.mock("node:fs/promises", async (importOriginal) => {
Expand Down Expand Up @@ -126,7 +126,7 @@ const layerTest = Layer.mergeAll(
).pipe(Layer.provideMerge(NodeServices.layer));

describe("AssetAccess", () => {
it.effect("loads private media immediately after login and reuses the found credential", () => {
it.effect("loads private media immediately after login with the GitHub credential", () => {
let lookups = 0;
const authorizations: Array<string | undefined> = [];
return Effect.gen(function* () {
Expand All @@ -138,19 +138,21 @@ describe("AssetAccess", () => {
expect((yield* githubMediaResponse(asset, {})).status).toBe(404);
expect((yield* githubMediaResponse(asset, {})).status).toBe(200);
expect((yield* githubMediaResponse(asset, {})).status).toBe(200);
expect(lookups).toBe(2);
// Caching the token is GitHubCredentials' job; this asks it every time.
expect(lookups).toBe(3);
expect(authorizations).toEqual([undefined, "Bearer signed-in", "Bearer signed-in"]);
}).pipe(
Effect.provide(
Layer.mock(GitHubCli.GitHubCli)({
execute: () =>
Effect.sync(() => ({
exitCode: ChildProcessSpawner.ExitCode(0),
stdout: ++lookups === 1 ? "" : "signed-in",
stderr: "",
stdoutTruncated: false,
stderrTruncated: false,
})),
Layer.mock(GitHubCredentials.GitHubCredentials)({
get: (host) =>
++lookups === 1
? Effect.fail(new GitHubCredentials.GitHubNotSignedInError({ host }))
: Effect.succeed({
host,
token: Redacted.make("signed-in"),
source: "gh" as const,
fingerprint: "fingerprint",
}),
}),
),
Effect.provideService(
Expand Down
51 changes: 9 additions & 42 deletions apps/server/src/assets/GitHubMediaFetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {
type HttpClientResponse,
} from "effect/http";

import * as GitHubCli from "../sourceControl/GitHubCli.ts";
import * as GitHubCredentials from "../sourceControl/GitHubCredentials.ts";

/**
* Exactly the hosts the credential is for. Everything a redirect leads to — the presigned
Expand All @@ -37,8 +37,6 @@ const isCredentialedHost = (url: string) => {
const MAX_REDIRECTS = 3;
/** Following the redirect here, rather than in `fetch`, is what keeps the token on GitHub. */
const MANUAL_REDIRECT: RequestInit = { redirect: "manual" };
const TOKEN_CACHE_TTL_MS = 5 * 60_000;
const TOKEN_CACHE_MAX_ENTRIES = 32;
/** Passed through so a seek in a long video costs one upstream range request, not a full download. */
const FORWARDED_REQUEST_HEADERS = ["range", "if-range"] as const;
const FORWARDED_RESPONSE_HEADERS = [
Expand All @@ -57,45 +55,14 @@ const SVG_CONTENT_TYPE = "image/svg+xml";
const SVG_CONTENT_SECURITY_POLICY = "default-src 'none'; style-src 'unsafe-inline'; sandbox";

/**
* A media request per image and one per video seek, each of which would otherwise spawn `gh`.
* The token is what `gh auth token` would print again on the next call, and it is held no longer
* than a signed asset URL lives.
* The github.com credential, or null without one: a public asset still loads, and a private one
* fails the way it does in a browser that is not signed in.
*/
const tokenCache = new Map<string, { readonly at: number; readonly token: Redacted.Redacted }>();

const githubToken = Effect.fn("GitHubMediaFetch.githubToken")(function* (input: {
readonly cwd: string;
readonly host: string;
}) {
// `gh` stores a token per host, not per repository, so the directory it runs in is not part
// of the answer and must not fragment the cache a client could otherwise churn. This route
// pins no credential; if it ever does, the pin belongs in this key.
const key = input.host;
const now = yield* Clock.currentTimeMillis;
const cached = tokenCache.get(key);
if (cached !== undefined && now - cached.at < TOKEN_CACHE_TTL_MS) return cached.token;
const github = yield* GitHubCli.GitHubCli;
// No credential is a normal state: a public asset still loads, and a private one fails the way
// it does in a browser that is not signed in.
const token = yield* github
.execute({
cwd: input.cwd,
args: ["auth", "token", "--hostname", input.host],
env: { GH_DEBUG: "" },
})
.pipe(
Effect.map((output) => output.stdout.trim()),
Effect.orElseSucceed(() => ""),
);
// A login or recovered CLI failure must take effect on the next media request.
if (token.length === 0) return null;
if (tokenCache.size >= TOKEN_CACHE_MAX_ENTRIES) {
tokenCache.delete(tokenCache.keys().next().value!);
}
const redacted = Redacted.make(token);
tokenCache.set(key, { at: now, token: redacted });
return redacted;
});
const githubToken = GitHubCredentials.GitHubCredentials.pipe(
Effect.flatMap((credentials) => credentials.get("github.com")),
Effect.map((credential) => credential.token),
Effect.orElseSucceed(() => null),
);

/**
* Follows GitHub's redirect to the signed object itself, and never carries the credential off
Expand Down Expand Up @@ -146,7 +113,7 @@ export const githubMediaResponse = Effect.fn("GitHubMediaFetch.githubMediaRespon
requestHeaders: Record<string, string | undefined>,
) {
// Both media hosts are served by github.com's account, which is the host `gh` stores it under.
const token = yield* githubToken({ cwd: asset.cwd, host: "github.com" });
const token = yield* githubToken;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟠 High assets/GitHubMediaFetch.ts:116

After a signed-out request caches the failed credential lookup, githubToken returns null for up to MISSING_TTL (10 seconds), so private media requests made immediately after gh auth login are still sent without authorization and return 404. Invalidate or refresh the cached credential on login, or avoid using the negative cached result for this route.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/assets/GitHubMediaFetch.ts around line 116:

After a signed-out request caches the failed credential lookup, `githubToken` returns `null` for up to `MISSING_TTL` (10 seconds), so private media requests made immediately after `gh auth login` are still sent without authorization and return 404. Invalidate or refresh the cached credential on login, or avoid using the negative cached result for this route.

const forwarded: Record<string, string> = {};
for (const name of FORWARDED_REQUEST_HEADERS) {
const value = requestHeaders[name];
Expand Down
18 changes: 13 additions & 5 deletions apps/server/src/git/GitManager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import {
ThreadId,
} from "@t3tools/contracts";
import * as DateTime from "effect/DateTime";
import * as GitHubApi from "../sourceControl/GitHubApi.ts";
import * as GitHubCli from "../sourceControl/GitHubCli.ts";
import { decodeGitHubPullRequestListJson } from "../sourceControl/gitHubPullRequests.ts";
import * as GitLabCli from "../sourceControl/GitLabCli.ts";
Expand Down Expand Up @@ -383,7 +384,11 @@ function createGitHubCliWithFakeGh(scenario: FakeGhScenario = {}): {
);
const ghCalls: string[] = [];

const execute: GitHubCli.GitHubCli["Service"]["execute"] = (input) => {
// The fake still speaks in gh's command shapes; the service methods below translate to them.
const execute = (input: {
readonly cwd: string;
readonly args: ReadonlyArray<string>;
}): Effect.Effect<VcsProcess.VcsProcessOutput, GitHubCli.GitHubCliError> => {
const args = [...input.args];
ghCalls.push(args.join(" "));

Expand Down Expand Up @@ -518,7 +523,6 @@ function createGitHubCliWithFakeGh(scenario: FakeGhScenario = {}): {

return {
service: {
execute,
// The fake answers the CLI shape, so batched lookups read it the way the fallback does.
listPullRequestsByHead: (input) =>
execute({
Expand Down Expand Up @@ -724,7 +728,12 @@ function makeManager(input?: {
discover: Effect.succeed([]),
}),
),
Effect.provide(Layer.succeed(GitHubCli.GitHubCli, gitHubCli)),
Effect.provide(
Layer.merge(
Layer.succeed(GitHubCli.GitHubCli, gitHubCli),
Layer.mock(GitHubApi.GitHubApi)({}),
),
),
),
);

Expand Down Expand Up @@ -2660,8 +2669,7 @@ it.layer(layerGitManagerTest)("GitManager", (it) => {
provider: "github",
providerOperation: "listChangeRequests",
providerCommand: "gh",
errorDetail:
"GitHub API rate limit exceeded. For the GraphQL quota and reset time, run `gh api graphql -f query='{rateLimit{remaining resetAt}}'`; `gh api rate_limit` reports REST.",
errorDetail: "GitHub API rate limit exceeded. Requests resume when the limit resets.",
});
const loggedText = [
warning?.message ?? "",
Expand Down
3 changes: 2 additions & 1 deletion apps/server/src/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -558,7 +558,8 @@ const layerRuntimeCoreDependenciesBase = Layer.mergeAll(
Layer.provideMerge(Layer.merge(ProjectStore.layer, ThreadSearch.layer)),
Layer.provideMerge(layerServerSettings),
// The asset route uses the registry's GitHub credential for private PR media.
Layer.provideMerge(Layer.mergeAll(layerSourceControlProviderRegistry, GitHubCli.layer)),
Layer.provideMerge(layerSourceControlProviderRegistry),
Layer.provideMerge(GitHubCli.layer),
Layer.provideMerge(layerGit),
Layer.provideMerge(layerVcs),
Layer.provideMerge(Layer.mergeAll(layerTerminal, layerPreview, layerDevice)),
Expand Down
3 changes: 2 additions & 1 deletion apps/server/src/sourceControl/GitHubApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ export interface GitHubGraphQlInput {
readonly query: string;
readonly variables?: Readonly<Record<string, unknown>>;
readonly allowReserve?: boolean;
readonly maxResponseBytes?: number;
}

export class GitHubApi extends Context.Service<
Expand Down Expand Up @@ -567,7 +568,7 @@ export const make = Effect.gen(function* () {
HttpClientRequest.acceptJson,
HttpClientRequest.bodyJsonUnsafe({ query, variables: input.variables ?? {} }),
),
maxResponseBytes: DEFAULT_MAX_RESPONSE_BYTES,
maxResponseBytes: input.maxResponseBytes ?? DEFAULT_MAX_RESPONSE_BYTES,
allowReserve,
acceptNotModified: false,
graphql: true,
Expand Down
Loading
Loading