Skip to content

Commit 17650be

Browse files
fix: harden failure bundle artifact downloads (#60)
* block artifact download redirects * redact artifact download urls --------- Co-authored-by: merlinsantiago982-cmd <merlinsantiago982-cmd@users.noreply.github.com>
1 parent 5ff6399 commit 17650be

2 files changed

Lines changed: 61 additions & 15 deletions

File tree

src/lib/bundle.test.ts

Lines changed: 46 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -641,17 +641,24 @@ describe('streamUrlToFile retry', () => {
641641
calls++;
642642
throw new Error('ENETUNREACH dns lookup failed');
643643
};
644-
await expect(
645-
streamUrlToFile(
646-
'https://example.com/x',
644+
let caught: unknown;
645+
646+
try {
647+
await streamUrlToFile(
648+
'https://example.com/x?X-Amz-Signature=secret-token',
647649
'/tmp/will-not-be-written',
648650
fetchImpl as typeof globalThis.fetch,
649651
{ sleep: noSleep },
650-
),
651-
).rejects.toMatchObject({
652+
);
653+
} catch (err) {
654+
caught = err;
655+
}
656+
657+
expect(caught).toMatchObject({
652658
name: 'TransportError',
653659
message: expect.stringContaining('ENETUNREACH'),
654660
});
661+
expect(caught).not.toMatchObject({ message: expect.stringContaining('secret-token') });
655662
expect(calls).toBe(STREAM_URL_MAX_RETRIES);
656663
});
657664

@@ -661,17 +668,46 @@ describe('streamUrlToFile retry', () => {
661668
calls++;
662669
return new Response('Forbidden', { status: 403 });
663670
};
664-
await expect(
665-
streamUrlToFile(
666-
'https://example.com/x',
671+
let caught: unknown;
672+
const presignedUrl = 'https://example.com/x?X-Amz-Signature=secret-token#download';
673+
674+
try {
675+
await streamUrlToFile(
676+
presignedUrl,
667677
'/tmp/will-not-be-written',
668678
fetchImpl as typeof globalThis.fetch,
669679
{ sleep: noSleep },
670-
),
671-
).rejects.toMatchObject({ code: 'UNAVAILABLE' });
680+
);
681+
} catch (err) {
682+
caught = err;
683+
}
684+
685+
expect(caught).toMatchObject({
686+
code: 'UNAVAILABLE',
687+
details: { status: 403, artifactUrl: 'https://example.com/x' },
688+
});
689+
const details = (caught as { details?: Record<string, unknown> }).details;
690+
expect(details).not.toHaveProperty('url');
691+
expect(JSON.stringify(details)).not.toContain('secret-token');
672692
expect(calls).toBe(1);
673693
});
674694

695+
it('disables automatic redirects so unsafe redirect targets cannot bypass URL validation', async () => {
696+
const dir = mkdtempSync(join(tmpdir(), 'stream-test-'));
697+
const dest = join(dir, 'out.bin');
698+
const redirects: Array<RequestInit['redirect']> = [];
699+
const fetchImpl = async (_url: Parameters<typeof globalThis.fetch>[0], init?: RequestInit) => {
700+
redirects.push(init?.redirect);
701+
return new Response('hello', { status: 200 });
702+
};
703+
704+
await streamUrlToFile('https://example.com/x', dest, fetchImpl as typeof globalThis.fetch, {
705+
sleep: noSleep,
706+
});
707+
708+
expect(redirects).toEqual(['error']);
709+
});
710+
675711
it('sleeps between retries', async () => {
676712
const sleepDelays: number[] = [];
677713
const fetchImpl = async () => {

src/lib/bundle.ts

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -772,17 +772,18 @@ export async function streamUrlToFile(
772772
deps?: { sleep?: (ms: number) => Promise<void> },
773773
): Promise<void> {
774774
const sleepFn = deps?.sleep ?? ((ms: number) => new Promise<void>(r => setTimeout(r, ms)));
775+
const artifactUrl = redactArtifactUrlForDetails(url);
775776
for (let attempt = 1; attempt <= STREAM_URL_MAX_RETRIES; attempt++) {
776777
let response: Response;
777778
try {
778-
response = await fetchImpl(url);
779+
response = await fetchImpl(url, { redirect: 'error' });
779780
} catch (err) {
780781
const message = err instanceof Error ? err.message : String(err);
781782
if (attempt < STREAM_URL_MAX_RETRIES) {
782783
await sleepFn(STREAM_URL_RETRY_DELAY_MS);
783784
continue;
784785
}
785-
throw new TransportError(`Failed to download presigned URL ${url}: ${message}`);
786+
throw new TransportError(`Failed to download presigned URL ${artifactUrl}: ${message}`);
786787
}
787788
if (!response.ok) {
788789
// Non-2xx: the URL itself is bad (expired, unauthorized, not found).
@@ -794,7 +795,7 @@ export async function streamUrlToFile(
794795
nextAction:
795796
'Re-run `testsprite test failure get`. Presigned URLs in the bundle expire after 15 minutes.',
796797
requestId: 'local',
797-
details: { status: response.status, url },
798+
details: { status: response.status, artifactUrl },
798799
},
799800
});
800801
}
@@ -814,7 +815,7 @@ export async function streamUrlToFile(
814815
await sleepFn(STREAM_URL_RETRY_DELAY_MS);
815816
continue;
816817
}
817-
throw new TransportError(`Failed to download presigned URL ${url}: ${message}`);
818+
throw new TransportError(`Failed to download presigned URL ${artifactUrl}: ${message}`);
818819
}
819820
}
820821
await mkdir(dirname(filePath), { recursive: true });
@@ -836,11 +837,20 @@ export async function streamUrlToFile(
836837
await sleepFn(STREAM_URL_RETRY_DELAY_MS);
837838
continue;
838839
}
839-
throw new TransportError(`Failed mid-download of ${url}: ${message}`);
840+
throw new TransportError(`Failed mid-download of ${artifactUrl}: ${message}`);
840841
}
841842
}
842843
}
843844

845+
function redactArtifactUrlForDetails(url: string): string {
846+
try {
847+
const parsed = new URL(url);
848+
return `${parsed.origin}${parsed.pathname}`;
849+
} catch {
850+
return '<invalid-url>';
851+
}
852+
}
853+
844854
function isPresignedUrl(value: string): boolean {
845855
return value.startsWith('https://');
846856
}

0 commit comments

Comments
 (0)