Skip to content

Commit 5101c9c

Browse files
dahliacodex
andcommitted
Fix recurse private-address handling in authorized fetch mode
Ensure -p/--allow-private-address is applied consistently in lookup mode, including authorized-fetch paths. In recurse mode, explicit root lookups now use the regular lookup/context loaders (respecting command/config allowPrivateAddress), while recursive follow-up fetches continue to use strict loaders that disallow private addresses. Also consume redirect response bodies in image download redirect handling to avoid keeping redirect bodies alive across manual redirect hops. Add regression tests for redirect-body cancellation in imagerenderer tests. #608 (comment) #608 (comment) #608 (comment) Co-Authored-By: Codex <codex@openai.com>
1 parent ee934f5 commit 5101c9c

3 files changed

Lines changed: 96 additions & 9 deletions

File tree

packages/cli/src/imagerenderer.test.ts

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,3 +94,58 @@ test("downloadImage - follows validated redirects", async () => {
9494
}
9595
}
9696
});
97+
98+
test("downloadImage - cancels redirect response body before following", async () => {
99+
let cancelled = 0;
100+
const originalFetch = globalThis.fetch;
101+
globalThis.fetch = ((input: URL | RequestInfo) => {
102+
const target = typeof input === "string" ? input : input.toString();
103+
if (target === "https://example.com/image.png") {
104+
const body = new ReadableStream<Uint8Array>({
105+
cancel() {
106+
cancelled++;
107+
},
108+
});
109+
return Promise.resolve(
110+
new Response(body, {
111+
status: 302,
112+
headers: { location: "https://cdn.example.com/final.png" },
113+
}),
114+
);
115+
}
116+
return Promise.resolve(new Response(new Uint8Array([1, 2, 3])));
117+
}) as typeof fetch;
118+
119+
let result: string | null = null;
120+
try {
121+
result = await downloadImage("https://example.com/image.png");
122+
assert.notEqual(result, null);
123+
assert.equal(cancelled, 1);
124+
} finally {
125+
globalThis.fetch = originalFetch;
126+
if (result != null) {
127+
await rm(path.dirname(result), { recursive: true, force: true });
128+
}
129+
}
130+
});
131+
132+
test("downloadImage - cancels redirect body when location is missing", async () => {
133+
let cancelled = 0;
134+
const originalFetch = globalThis.fetch;
135+
globalThis.fetch = ((_input: URL | RequestInfo) => {
136+
const body = new ReadableStream<Uint8Array>({
137+
cancel() {
138+
cancelled++;
139+
},
140+
});
141+
return Promise.resolve(new Response(body, { status: 302 }));
142+
}) as typeof fetch;
143+
144+
try {
145+
const result = await downloadImage("https://example.com/image.png");
146+
assert.equal(result, null);
147+
assert.equal(cancelled, 1);
148+
} finally {
149+
globalThis.fetch = originalFetch;
150+
}
151+
});

packages/cli/src/imagerenderer.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,11 @@ export async function downloadImage(url: string): Promise<string | null> {
115115
response.status === 308
116116
) {
117117
const location = response.headers.get("location");
118-
if (location == null) return null;
118+
if (location == null) {
119+
await response.body?.cancel();
120+
return null;
121+
}
122+
await response.body?.cancel();
119123
targetUrl = new URL(location, targetUrl).href;
120124
continue;
121125
}

packages/cli/src/lookup.ts

Lines changed: 36 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -597,6 +597,9 @@ export async function runLookup(
597597
);
598598

599599
let authLoader: DocumentLoader | undefined = undefined;
600+
let authIdentity:
601+
| { keyId: URL; privateKey: CryptoKey }
602+
| undefined = undefined;
600603
let outputStream: WriteStream | undefined;
601604
let outputStreamError: Error | undefined;
602605
const getOutputStream = (): WriteStream | undefined => {
@@ -671,12 +674,15 @@ export async function runLookup(
671674
{ contextLoader },
672675
);
673676
}, { service: command.tunnelService });
677+
authIdentity = {
678+
keyId: new URL("#main-key", server.url),
679+
privateKey: key.privateKey,
680+
};
674681
const baseAuthLoader = getAuthenticatedDocumentLoader(
682+
authIdentity,
675683
{
676-
keyId: new URL("#main-key", server.url),
677-
privateKey: key.privateKey,
678-
},
679-
{
684+
allowPrivateAddress: command.allowPrivateAddress,
685+
userAgent: command.userAgent,
680686
specDeterminer: {
681687
determineSpec() {
682688
return command.firstKnock;
@@ -719,7 +725,29 @@ export async function runLookup(
719725
recursiveBaseContextLoader,
720726
command.timeout,
721727
);
722-
const recursiveLookupDocumentLoader: DocumentLoader = authLoader ??
728+
const recursiveAuthLoader = command.authorizedFetch &&
729+
authIdentity != null
730+
? wrapDocumentLoaderWithTimeout(
731+
getAuthenticatedDocumentLoader(
732+
authIdentity,
733+
{
734+
allowPrivateAddress: false,
735+
userAgent: command.userAgent,
736+
specDeterminer: {
737+
determineSpec() {
738+
return command.firstKnock;
739+
},
740+
rememberSpec() {
741+
},
742+
},
743+
},
744+
),
745+
command.timeout,
746+
)
747+
: undefined;
748+
const initialLookupDocumentLoader: DocumentLoader = authLoader ??
749+
documentLoader;
750+
const recursiveLookupDocumentLoader: DocumentLoader = recursiveAuthLoader ??
723751
recursiveDocumentLoader;
724752
let totalObjects = 0;
725753
const recurseDepth = command.recurseDepth!;
@@ -735,8 +763,8 @@ export async function runLookup(
735763
let current: APObject | null = null;
736764
try {
737765
current = await lookupObject(url, {
738-
documentLoader: recursiveLookupDocumentLoader,
739-
contextLoader: recursiveContextLoader,
766+
documentLoader: initialLookupDocumentLoader,
767+
contextLoader,
740768
userAgent: command.userAgent,
741769
});
742770
} catch (error) {
@@ -772,7 +800,7 @@ export async function runLookup(
772800
current,
773801
command.output,
774802
command.format,
775-
recursiveContextLoader,
803+
contextLoader,
776804
getOutputStream(),
777805
);
778806
} catch (error) {

0 commit comments

Comments
 (0)