Skip to content
Closed
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
11 changes: 9 additions & 2 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -409,18 +409,23 @@ async function verifyGithubOidcJwt(token: string, env: Env): Promise<JwtPayload>
throw new ApiError("ERR_OIDC_VERIFICATION", 401, "OIDC token verification failed");
}
}
/* v8 ignore stop */

function validateRepositoryName(repository: string, env: Env): string {
if (!/^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/.test(repository)) {
throw new ApiError("ERR_VALIDATION_INPUT", 400, "target_repository is not a valid owner/name repository");
}
const [owner] = repository.split("/", 1);
const [owner, name] = repository.split("/", 2);
if (/^\.{1,2}$/.test(name)) {

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.

This only rejects a . / .. name. ../noema and ./noema still pass the owner/name regex and fall through to the org allowlist as 403 ERR_REPO_NOT_ALLOWED.

That is fail-closed today, but the published OpenAPI pattern still accepts those strings, and #/repos/${repository}/installation is the path this helper is supposed to keep off GitHub App work.

#399 rejects both segments with the same 400 ERR_VALIDATION_INPUT and publishes ^(?!\.{1,2}/)[A-Za-z0-9_.-]+/(?!\.{1,2}$)[A-Za-z0-9_.-]+$. Keep .github allowed.

throw new ApiError("ERR_VALIDATION_INPUT", 400, "target_repository is not a valid owner/name repository");
}
if (owner !== env.ALLOWED_REPOSITORY_OWNER) {
throw new ApiError("ERR_REPO_NOT_ALLOWED", 403, "target_repository owner is not allowed");
}
return repository;
}

/* v8 ignore start */
async function importGithubAppPrivateKey(pem: string): Promise<CryptoKey> {
const body = pem.replace(/-----BEGIN [^-]+-----/g, "").replace(/-----END [^-]+-----/g, "").replace(/\s+/g, "");
const der = base64UrlDecode(body.replace(/\+/g, "-").replace(/\//g, "_"));
Expand Down Expand Up @@ -507,6 +512,7 @@ async function createInstallationToken(repository: string, env: Env): Promise<In
expires_at: String(token.expires_at),
};
}
/* v8 ignore stop */

async function parseExchangeRequestBody(request: Request): Promise<ExchangeRequestBody> {
const contentType = request.headers.get("content-type") || "";
Expand All @@ -521,6 +527,7 @@ async function parseExchangeRequestBody(request: Request): Promise<ExchangeReque
return body as ExchangeRequestBody;
}

/* v8 ignore start */
async function claimVerifiedOidcUsage(claims: JwtPayload, env: Env): Promise<boolean> {
if (!env.NOEMA_OIDC_REPLAY_GUARD) return false;
if (typeof claims.jti !== "string" || typeof claims.exp !== "number") {
Expand Down Expand Up @@ -710,4 +717,4 @@ export default {
return withOperationalHeaders(response, traceId, latency_ms);
}
},
};
};

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.

This commit strips the trailing newline (\\ No newline at end of file). Restore it — #399 does. Do not land a security-core file with a missing EOF newline.

4 changes: 3 additions & 1 deletion test/coverage-ignore-operational-helpers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ const source = readFileSync(new URL("../src/index.ts", import.meta.url), "utf8")
const ignoredRegions = [...source.matchAll(/\/\* v8 ignore start \*\/[\s\S]*?\/\* v8 ignore stop \*\//g)]
.map((match) => match[0]);

describe("operational helper coverage exclusions", () => {
describe("owned production coverage exclusions", () => {
it.each([
"jsonResponse",
"trustedTraceHeader",
Expand All @@ -21,6 +21,8 @@ describe("operational helper coverage exclusions", () => {
"errorResponse",
"withOperationalHeaders",
"logRequest",
"validateRepositoryName",
"parseExchangeRequestBody",
])("keeps %s inside measured production coverage", (functionName) => {
expect(
ignoredRegions.some((region) => region.includes(`function ${functionName}`)),
Expand Down
279 changes: 279 additions & 0 deletions test/credential-request-helper-coverage.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,279 @@
import { afterEach, describe, expect, it, vi } from "vitest";
import worker, { type Env } from "../src/index";

const configuredRef =
"ContextualWisdomLab/.github/.github/workflows/noema-review.yml@refs/heads/main";

const env: Env = {
ALLOWED_ISSUER: "https://token.actions.githubusercontent.com",
ALLOWED_AUDIENCE: "cwl-noema-review",
ALLOWED_REPOSITORY_OWNER: "ContextualWisdomLab",
ALLOWED_WORKFLOW_REPOSITORY: "ContextualWisdomLab/.github",
ALLOWED_WORKFLOW_REF_PREFIX: configuredRef,
GITHUB_API_BASE: "https://api.github.com",
GITHUB_APP_ID: "1",
GITHUB_APP_PRIVATE_KEY_PEM: "unused-before-request-validation",
NOEMA_RATE_LIMIT_PER_MINUTE: "1000",
};

function encodeSegment(value: unknown): string {
return Buffer.from(JSON.stringify(value)).toString("base64url");
}

function encodeBytes(bytes: ArrayBuffer): string {
return Buffer.from(bytes).toString("base64url");
}

async function createSignedJwt(repository: string) {
const keyPair = await crypto.subtle.generateKey(
{
name: "RSASSA-PKCS1-v1_5",
modulusLength: 2048,
publicExponent: new Uint8Array([1, 0, 1]),
hash: "SHA-256",
},
true,
["sign", "verify"],
);
const kid = `credential-request-${crypto.randomUUID()}`;
const now = Math.floor(Date.now() / 1000);
const payload = {
iss: env.ALLOWED_ISSUER,
aud: env.ALLOWED_AUDIENCE,
repository_owner: env.ALLOWED_REPOSITORY_OWNER,
repository,
job_workflow_ref: configuredRef,
sub: "repo:ContextualWisdomLab/.github:ref:refs/heads/main",
exp: now + 300,
nbf: now - 30,
iat: now - 30,
};
const header = encodeSegment({ alg: "RS256", kid, typ: "JWT" });
const body = encodeSegment(payload);
const signature = await crypto.subtle.sign(
"RSASSA-PKCS1-v1_5",
keyPair.privateKey,
new TextEncoder().encode(`${header}.${body}`),
);
const publicJwk = await crypto.subtle.exportKey("jwk", keyPair.publicKey);
return {
token: `${header}.${body}.${encodeBytes(signature)}`,
jwk: { ...publicJwk, kid, kty: "RSA" },
};
}

function mockOidcDiscovery(jwk: JsonWebKey & { kid: string; kty: string }) {
const upstream = vi.spyOn(globalThis, "fetch").mockImplementation(async (input) => {
const url = String(input);
if (url === "https://token.actions.githubusercontent.com/.well-known/openid-configuration") {
return Response.json({
jwks_uri: "https://token.actions.githubusercontent.com/.well-known/jwks",
});
}
if (url === "https://token.actions.githubusercontent.com/.well-known/jwks") {
return Response.json({ keys: [jwk] });
}
return new Response("unexpected privileged egress", { status: 500 });
});
return upstream;
}

afterEach(() => {
vi.restoreAllMocks();
});

describe("credential request helper coverage through the public worker", () => {
it("rejects malformed JSON after verified OIDC without reaching GitHub App egress", async () => {
const { token, jwk } = await createSignedJwt("ContextualWisdomLab/.github");
const upstream = mockOidcDiscovery(jwk);

const response = await worker.fetch(
new Request("https://noema.example/exchange", {
method: "POST",
headers: {
authorization: `Bearer ${token}`,
"content-type": "application/json",
"cf-connecting-ip": "203.0.113.101",
},
body: "{",
}),
env,
);

expect(response.status).toBe(400);
await expect(response.json()).resolves.toMatchObject({
ok: false,
error_code: "ERR_VALIDATION_INPUT",
message: "Malformed JSON request body",
});
expect(
upstream.mock.calls.filter(([input]) => String(input).startsWith("https://api.github.com/")),
).toHaveLength(0);
});

it("treats a non-object JSON body as empty before repository syntax validation", async () => {
const { token, jwk } = await createSignedJwt("invalid-repository-name");
const upstream = mockOidcDiscovery(jwk);

const response = await worker.fetch(
new Request("https://noema.example/exchange", {
method: "POST",
headers: {
authorization: `Bearer ${token}`,
"content-type": "application/json",
"cf-connecting-ip": "203.0.113.102",
},
body: "null",
}),
env,
);

expect(response.status).toBe(400);
await expect(response.json()).resolves.toMatchObject({
ok: false,
error_code: "ERR_VALIDATION_INPUT",
message: "target_repository is not a valid owner/name repository",
});
expect(
upstream.mock.calls.filter(([input]) => String(input).startsWith("https://api.github.com/")),
).toHaveLength(0);
});

it("treats a truthy primitive JSON body as empty before repository syntax validation", async () => {
const { token, jwk } = await createSignedJwt("invalid-repository-name");
const upstream = mockOidcDiscovery(jwk);

const response = await worker.fetch(
new Request("https://noema.example/exchange", {
method: "POST",
headers: {
authorization: `Bearer ${token}`,
"content-type": "application/json",
"cf-connecting-ip": "203.0.113.106",
},
body: "7",
}),
env,
);

expect(response.status).toBe(400);
await expect(response.json()).resolves.toMatchObject({
ok: false,
error_code: "ERR_VALIDATION_INPUT",
message: "target_repository is not a valid owner/name repository",
});
expect(
upstream.mock.calls.filter(([input]) => String(input).startsWith("https://api.github.com/")),
).toHaveLength(0);
});

it("treats a non-JSON body as empty before repository syntax validation", async () => {
const { token, jwk } = await createSignedJwt("invalid-repository-name");
const upstream = mockOidcDiscovery(jwk);

const response = await worker.fetch(
new Request("https://noema.example/exchange", {
method: "POST",
headers: {
authorization: `Bearer ${token}`,
"content-type": "text/plain",
"cf-connecting-ip": "203.0.113.103",
},
body: "ignored",
}),
env,
);

expect(response.status).toBe(400);
await expect(response.json()).resolves.toMatchObject({
ok: false,
error_code: "ERR_VALIDATION_INPUT",
message: "target_repository is not a valid owner/name repository",
});
expect(
upstream.mock.calls.filter(([input]) => String(input).startsWith("https://api.github.com/")),
).toHaveLength(0);
});

it("treats a missing content type as a non-JSON body without privileged egress", async () => {
const { token, jwk } = await createSignedJwt("invalid-repository-name");
const upstream = mockOidcDiscovery(jwk);

const response = await worker.fetch(
new Request("https://noema.example/exchange", {
method: "POST",
headers: {
authorization: `Bearer ${token}`,
"cf-connecting-ip": "203.0.113.107",
},
}),
env,
);

expect(response.status).toBe(400);
await expect(response.json()).resolves.toMatchObject({
ok: false,
error_code: "ERR_VALIDATION_INPUT",
message: "target_repository is not a valid owner/name repository",
});
expect(
upstream.mock.calls.filter(([input]) => String(input).startsWith("https://api.github.com/")),
).toHaveLength(0);
});

it("rejects a syntactically valid repository owned outside the configured organization", async () => {
const { token, jwk } = await createSignedJwt("ContextualWisdomLab/.github");
const upstream = mockOidcDiscovery(jwk);

const response = await worker.fetch(
new Request("https://noema.example/exchange", {
method: "POST",
headers: {
authorization: `Bearer ${token}`,
"content-type": "application/json",
"cf-connecting-ip": "203.0.113.104",
},
body: JSON.stringify({ target_repository: "OtherWisdomLab/noema" }),
}),
env,
);

expect(response.status).toBe(403);
await expect(response.json()).resolves.toMatchObject({
ok: false,
error_code: "ERR_REPO_NOT_ALLOWED",
message: "target_repository owner is not allowed",
});
expect(
upstream.mock.calls.filter(([input]) => String(input).startsWith("https://api.github.com/")),
).toHaveLength(0);
});

it("rejects repository URL dot segments before GitHub App credential work", async () => {
const { token, jwk } = await createSignedJwt("ContextualWisdomLab/.github");
const upstream = mockOidcDiscovery(jwk);

const response = await worker.fetch(
new Request("https://noema.example/exchange", {
method: "POST",
headers: {
authorization: `Bearer ${token}`,
"content-type": "application/json",
"cf-connecting-ip": "203.0.113.105",
},
body: JSON.stringify({ target_repository: "ContextualWisdomLab/.." }),

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.

This is the only dot-segment case. Add ContextualWisdomLab/., ../noema, and ./noema so the public Worker contract matches both segments and both . / .. values. #399 uses it.each for those four inputs and still asserts zero api.github.com egress.

}),
env,
);

expect(response.status).toBe(400);
await expect(response.json()).resolves.toMatchObject({
ok: false,
error_code: "ERR_VALIDATION_INPUT",
message: "target_repository is not a valid owner/name repository",
});
expect(
upstream.mock.calls.filter(([input]) => String(input).startsWith("https://api.github.com/")),
).toHaveLength(0);
});
});
Loading