Skip to content

Commit 1a49c32

Browse files
committed
fix: harden HTTP request security against downgrade and timing attacks
1 parent 5f1522c commit 1a49c32

File tree

5 files changed

+68
-28
lines changed

5 files changed

+68
-28
lines changed

package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -815,7 +815,7 @@
815815
"hosted-git-info": "8.1.0",
816816
"isexe": "3.1.1",
817817
"lru-cache": "11.2.2",
818-
"minimatch": "9.0.5",
818+
"minimatch": "9.0.6",
819819
"minipass": "7.1.3",
820820
"minipass-fetch": "4.0.1",
821821
"minipass-sized": "1.0.3",

pnpm-lock.yaml

Lines changed: 15 additions & 15 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

scripts/test/main.mjs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,7 @@ async function runTests(
198198
const { mode, reason, tests: testsToRun } = testInfo
199199

200200
// No tests needed
201-
if (testsToRun === null) {
201+
if (testsToRun == null) {
202202
logger.substep('No relevant changes detected, skipping tests')
203203
return 0
204204
}

src/dlx/binary.ts

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -566,13 +566,21 @@ export async function downloadBinaryFile(
566566
.digest('base64')
567567
const actualIntegrity = `sha512-${hash}`
568568

569-
// Verify integrity if provided.
570-
if (integrity && actualIntegrity !== integrity) {
571-
// Clean up invalid file.
572-
await safeDelete(destPath)
573-
throw new Error(
574-
`Integrity mismatch: expected ${integrity}, got ${actualIntegrity}`,
575-
)
569+
// Verify integrity if provided (constant-time comparison).
570+
if (integrity) {
571+
const integrityMatch =
572+
actualIntegrity.length === integrity.length &&
573+
crypto.timingSafeEqual(
574+
Buffer.from(actualIntegrity),
575+
Buffer.from(integrity),
576+
)
577+
if (!integrityMatch) {
578+
// Clean up invalid file.
579+
await safeDelete(destPath)
580+
throw new Error(
581+
`Integrity mismatch: expected ${integrity}, got ${actualIntegrity}`,
582+
)
583+
}
576584
}
577585

578586
// Make executable on POSIX systems.

src/http-request.ts

Lines changed: 36 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -776,6 +776,17 @@ async function httpDownloadAttempt(
776776
? res.headers.location
777777
: new URL(res.headers.location, url).toString()
778778

779+
// Reject HTTPS-to-HTTP downgrade redirects.
780+
const redirectParsed = new URL(redirectUrl)
781+
if (isHttps && redirectParsed.protocol !== 'https:') {
782+
reject(
783+
new Error(
784+
`Redirect from HTTPS to HTTP is not allowed: ${redirectUrl}`,
785+
),
786+
)
787+
return
788+
}
789+
779790
resolve(
780791
httpDownloadAttempt(redirectUrl, destPath, {
781792
ca,
@@ -946,6 +957,17 @@ async function httpRequestAttempt(
946957
? res.headers.location
947958
: new URL(res.headers.location, url).toString()
948959

960+
// Reject HTTPS-to-HTTP downgrade redirects.
961+
const redirectParsed = new URL(redirectUrl)
962+
if (isHttps && redirectParsed.protocol !== 'https:') {
963+
reject(
964+
new Error(
965+
`Redirect from HTTPS to HTTP is not allowed: ${redirectUrl}`,
966+
),
967+
)
968+
return
969+
}
970+
949971
resolve(
950972
httpRequestAttempt(redirectUrl, {
951973
body,
@@ -1141,8 +1163,10 @@ export async function httpDownload(
11411163
// Download to a temp file first, then atomically rename to destination.
11421164
// This prevents partial/corrupted files at the destination path if download fails,
11431165
// and preserves the original file (if any) until download succeeds.
1166+
const crypto = getCrypto()
11441167
const fs = getFs()
1145-
const tempPath = `${destPath}.download`
1168+
const tempSuffix = crypto.randomBytes(6).toString('hex')
1169+
const tempPath = `${destPath}.${tempSuffix}.download`
11461170

11471171
// Clean up any stale temp file from a previous failed download.
11481172
if (fs.existsSync(tempPath)) {
@@ -1165,20 +1189,28 @@ export async function httpDownload(
11651189

11661190
// Verify checksum if sha256 hash is provided.
11671191
if (sha256) {
1168-
const crypto = getCrypto()
11691192
// eslint-disable-next-line no-await-in-loop
11701193
const fileContent = await fs.promises.readFile(tempPath)
11711194
const computedHash = crypto
11721195
.createHash('sha256')
11731196
.update(fileContent)
11741197
.digest('hex')
11751198

1176-
if (computedHash !== sha256.toLowerCase()) {
1199+
const expectedHash = sha256.toLowerCase()
1200+
1201+
// Use constant-time comparison to prevent timing attacks.
1202+
if (
1203+
computedHash.length !== expectedHash.length ||
1204+
!crypto.timingSafeEqual(
1205+
Buffer.from(computedHash),
1206+
Buffer.from(expectedHash),
1207+
)
1208+
) {
11771209
// eslint-disable-next-line no-await-in-loop
11781210
await safeDelete(tempPath)
11791211
throw new Error(
11801212
`Checksum verification failed for ${url}\n` +
1181-
`Expected: ${sha256.toLowerCase()}\n` +
1213+
`Expected: ${expectedHash}\n` +
11821214
`Computed: ${computedHash}`,
11831215
)
11841216
}

0 commit comments

Comments
 (0)