Skip to content

Commit 3c7eb9b

Browse files
dahliacodex
andcommitted
Scope private-address default, cleanup exit status, and deterministic tests
Restore CLI document-loader default allowPrivateAddress behavior to keep non-lookup commands unchanged, while lookup remains explicitly hardened via lookup-specific option/config wiring. Make finalizeAndExit return a non-zero status when cleanup fails after a nominal success path, so shutdown I/O errors are not reported as success. Update imagerenderer tests to use literal public IP URLs to avoid DNS lookup dependencies and keep unit tests deterministic. #608 (comment) #608 (comment) #608 (comment) Co-Authored-By: Codex <codex@openai.com>
1 parent 8f1d18b commit 3c7eb9b

3 files changed

Lines changed: 19 additions & 13 deletions

File tree

packages/cli/src/docloader.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ export function getDocumentLoaderCachePrefix(
3030
}
3131

3232
export async function getDocumentLoader(
33-
{ userAgent, allowPrivateAddress = false }: DocumentLoaderOptions = {},
33+
{ userAgent, allowPrivateAddress = true }: DocumentLoaderOptions = {},
3434
): Promise<DocumentLoader> {
3535
const cacheKey = `${userAgent ?? ""}:${allowPrivateAddress}`;
3636
if (documentLoaders[cacheKey]) return documentLoaders[cacheKey];

packages/cli/src/imagerenderer.test.ts

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,9 @@ import path from "node:path";
44
import test from "node:test";
55
import { downloadImage } from "./imagerenderer.ts";
66

7+
const TEST_PUBLIC_IMAGE_URL = "https://198.51.100.10/image.png";
8+
const TEST_PUBLIC_REDIRECT_URL = "https://198.51.100.11/final.png";
9+
710
test("downloadImage - skips private URL without fetching", async () => {
811
let called = false;
912
const originalFetch = globalThis.fetch;
@@ -28,7 +31,7 @@ test("downloadImage - writes file for public URL", async () => {
2831

2932
let result: string | null = null;
3033
try {
31-
result = await downloadImage("https://example.com/image.png");
34+
result = await downloadImage(TEST_PUBLIC_IMAGE_URL);
3235
assert.notEqual(result, null);
3336
const fileStat = await stat(result!);
3437
assert.equal(fileStat.isFile(), true);
@@ -46,7 +49,7 @@ test("downloadImage - rejects redirect to private URL", async () => {
4649
globalThis.fetch = ((input: URL | RequestInfo) => {
4750
calls++;
4851
const target = typeof input === "string" ? input : input.toString();
49-
if (target === "https://example.com/image.png") {
52+
if (target === TEST_PUBLIC_IMAGE_URL) {
5053
return Promise.resolve(
5154
new Response(null, {
5255
status: 302,
@@ -58,7 +61,7 @@ test("downloadImage - rejects redirect to private URL", async () => {
5861
}) as typeof fetch;
5962

6063
try {
61-
const result = await downloadImage("https://example.com/image.png");
64+
const result = await downloadImage(TEST_PUBLIC_IMAGE_URL);
6265
assert.equal(result, null);
6366
assert.equal(calls, 1);
6467
} finally {
@@ -70,11 +73,11 @@ test("downloadImage - follows validated redirects", async () => {
7073
const originalFetch = globalThis.fetch;
7174
globalThis.fetch = ((input: URL | RequestInfo) => {
7275
const target = typeof input === "string" ? input : input.toString();
73-
if (target === "https://example.com/image.png") {
76+
if (target === TEST_PUBLIC_IMAGE_URL) {
7477
return Promise.resolve(
7578
new Response(null, {
7679
status: 302,
77-
headers: { location: "https://cdn.example.com/final.png" },
80+
headers: { location: TEST_PUBLIC_REDIRECT_URL },
7881
}),
7982
);
8083
}
@@ -83,7 +86,7 @@ test("downloadImage - follows validated redirects", async () => {
8386

8487
let result: string | null = null;
8588
try {
86-
result = await downloadImage("https://example.com/image.png");
89+
result = await downloadImage(TEST_PUBLIC_IMAGE_URL);
8790
assert.notEqual(result, null);
8891
const fileStat = await stat(result!);
8992
assert.equal(fileStat.isFile(), true);
@@ -100,7 +103,7 @@ test("downloadImage - cancels redirect response body before following", async ()
100103
const originalFetch = globalThis.fetch;
101104
globalThis.fetch = ((input: URL | RequestInfo) => {
102105
const target = typeof input === "string" ? input : input.toString();
103-
if (target === "https://example.com/image.png") {
106+
if (target === TEST_PUBLIC_IMAGE_URL) {
104107
const body = new ReadableStream<Uint8Array>({
105108
cancel() {
106109
cancelled++;
@@ -109,7 +112,7 @@ test("downloadImage - cancels redirect response body before following", async ()
109112
return Promise.resolve(
110113
new Response(body, {
111114
status: 302,
112-
headers: { location: "https://cdn.example.com/final.png" },
115+
headers: { location: TEST_PUBLIC_REDIRECT_URL },
113116
}),
114117
);
115118
}
@@ -118,7 +121,7 @@ test("downloadImage - cancels redirect response body before following", async ()
118121

119122
let result: string | null = null;
120123
try {
121-
result = await downloadImage("https://example.com/image.png");
124+
result = await downloadImage(TEST_PUBLIC_IMAGE_URL);
122125
assert.notEqual(result, null);
123126
assert.equal(cancelled, 1);
124127
} finally {
@@ -142,7 +145,7 @@ test("downloadImage - cancels redirect body when location is missing", async ()
142145
}) as typeof fetch;
143146

144147
try {
145-
const result = await downloadImage("https://example.com/image.png");
148+
const result = await downloadImage(TEST_PUBLIC_IMAGE_URL);
146149
assert.equal(result, null);
147150
assert.equal(cancelled, 1);
148151
} finally {
@@ -163,7 +166,7 @@ test("downloadImage - cancels body for non-ok terminal response", async () => {
163166
}) as typeof fetch;
164167

165168
try {
166-
const result = await downloadImage("https://example.com/image.png");
169+
const result = await downloadImage(TEST_PUBLIC_IMAGE_URL);
167170
assert.equal(result, null);
168171
assert.equal(cancelled, 1);
169172
} finally {

packages/cli/src/lookup.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -630,24 +630,27 @@ export async function runLookup(
630630
return outputStream;
631631
};
632632
const finalizeAndExit = async (code: number) => {
633+
let cleanupFailed = false;
633634
try {
634635
await closeWriteStream(outputStream);
635636
} catch (error) {
637+
cleanupFailed = true;
636638
logger.error("Failed to close output stream during shutdown: {error}", {
637639
error,
638640
});
639641
}
640642
try {
641643
await server?.close();
642644
} catch (error) {
645+
cleanupFailed = true;
643646
logger.error(
644647
"Failed to close temporary server during shutdown: {error}",
645648
{
646649
error,
647650
},
648651
);
649652
}
650-
process.exit(code);
653+
process.exit(cleanupFailed && code === 0 ? 1 : code);
651654
};
652655

653656
if (command.authorizedFetch) {

0 commit comments

Comments
 (0)