Skip to content

Commit 354b0bf

Browse files
committed
fix: harden SSL_CERT_FILE support with Content-Length, redirects, and broader coverage
- Set Content-Length header for POST bodies in httpsRequest path to avoid chunked transfer encoding divergence from fetch() - Follow 3xx redirects in httpsRequest path to match fetch() behavior - Route all fetch calls through apiFetch (GitHub API, npm registry) - Add debug logging when certificate file read fails
1 parent fe6e36b commit 354b0bf

File tree

6 files changed

+184
-17
lines changed

6 files changed

+184
-17
lines changed

src/commands/scan/create-scan-from-github.mts

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import { confirm, select } from '@socketsecurity/registry/lib/prompts'
1616
import { fetchSupportedScanFileNames } from './fetch-supported-scan-file-names.mts'
1717
import { handleCreateNewScan } from './handle-create-new-scan.mts'
1818
import constants from '../../constants.mts'
19+
import { apiFetch } from '../../utils/api.mts'
1920
import { debugApiRequest, debugApiResponse } from '../../utils/debug.mts'
2021
import { formatErrorWithDetail } from '../../utils/errors.mts'
2122
import { isReportSupportedFile } from '../../utils/glob.mts'
@@ -402,7 +403,7 @@ async function downloadManifestFile({
402403
debugApiRequest('GET', fileUrl)
403404
let downloadUrlResponse: Response
404405
try {
405-
downloadUrlResponse = await fetch(fileUrl, {
406+
downloadUrlResponse = await apiFetch(fileUrl, {
406407
method: 'GET',
407408
headers: {
408409
Authorization: `Bearer ${githubToken}`,
@@ -466,7 +467,7 @@ async function streamDownloadWithFetch(
466467

467468
try {
468469
debugApiRequest('GET', downloadUrl)
469-
response = await fetch(downloadUrl)
470+
response = await apiFetch(downloadUrl)
470471
debugApiResponse('GET', downloadUrl, response.status)
471472

472473
if (!response.ok) {
@@ -567,7 +568,7 @@ async function getLastCommitDetails({
567568
debugApiRequest('GET', commitApiUrl)
568569
let commitResponse: Response
569570
try {
570-
commitResponse = await fetch(commitApiUrl, {
571+
commitResponse = await apiFetch(commitApiUrl, {
571572
headers: {
572573
Authorization: `Bearer ${githubToken}`,
573574
},
@@ -679,7 +680,7 @@ async function getRepoDetails({
679680
let repoDetailsResponse: Response
680681
try {
681682
debugApiRequest('GET', repoApiUrl)
682-
repoDetailsResponse = await fetch(repoApiUrl, {
683+
repoDetailsResponse = await apiFetch(repoApiUrl, {
683684
method: 'GET',
684685
headers: {
685686
Authorization: `Bearer ${githubToken}`,
@@ -743,7 +744,7 @@ async function getRepoBranchTree({
743744
let treeResponse: Response
744745
try {
745746
debugApiRequest('GET', treeApiUrl)
746-
treeResponse = await fetch(treeApiUrl, {
747+
treeResponse = await apiFetch(treeApiUrl, {
747748
method: 'GET',
748749
headers: {
749750
Authorization: `Bearer ${githubToken}`,

src/utils/api.mts

Lines changed: 56 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ import type {
5050
SocketSdkSuccessResult,
5151
} from '@socketsecurity/sdk'
5252

53+
const MAX_REDIRECTS = 20
5354
const NO_ERROR_MESSAGE = 'No error message returned'
5455

5556
// Cached HTTPS agent for extra CA certificate support in direct API calls.
@@ -73,27 +74,61 @@ function getHttpsAgent(): HttpsAgent | undefined {
7374

7475
// Wrapper around fetch that supports extra CA certificates via SSL_CERT_FILE.
7576
// Uses node:https.request with a custom agent when extra CA certs are needed,
76-
// falling back to regular fetch() otherwise.
77-
type ApiFetchInit = {
77+
// falling back to regular fetch() otherwise. Follows redirects like fetch().
78+
export type ApiFetchInit = {
7879
body?: string | undefined
7980
headers?: Record<string, string> | undefined
8081
method?: string | undefined
8182
}
8283

83-
async function apiFetch(url: string, init: ApiFetchInit): Promise<Response> {
84-
const agent = getHttpsAgent()
85-
if (!agent) {
86-
return await fetch(url, init as globalThis.RequestInit)
87-
}
88-
return await new Promise((resolve, reject) => {
84+
// Internal httpsRequest-based fetch with redirect support.
85+
function _httpsRequestFetch(
86+
url: string,
87+
init: ApiFetchInit,
88+
agent: HttpsAgent,
89+
redirectCount: number,
90+
): Promise<Response> {
91+
return new Promise((resolve, reject) => {
92+
const headers: Record<string, string> = { ...init.headers }
93+
// Set Content-Length for request bodies to avoid chunked transfer encoding.
94+
if (init.body) {
95+
headers['content-length'] = String(Buffer.byteLength(init.body))
96+
}
8997
const req = httpsRequest(
9098
url,
9199
{
92100
method: init.method || 'GET',
93-
headers: init.headers,
101+
headers,
94102
agent,
95103
},
96104
res => {
105+
const { statusCode } = res
106+
// Follow redirects to match fetch() behavior.
107+
if (
108+
statusCode &&
109+
statusCode >= 300 &&
110+
statusCode < 400 &&
111+
res.headers['location']
112+
) {
113+
// Consume the response body to free up memory.
114+
res.resume()
115+
if (redirectCount >= MAX_REDIRECTS) {
116+
reject(new Error('Maximum redirect limit reached'))
117+
return
118+
}
119+
const redirectUrl = new URL(res.headers['location'], url).href
120+
// 307 and 308 preserve the original method and body.
121+
const preserveMethod = statusCode === 307 || statusCode === 308
122+
resolve(
123+
_httpsRequestFetch(
124+
redirectUrl,
125+
preserveMethod ? init : { headers: init.headers, method: 'GET' },
126+
agent,
127+
redirectCount + 1,
128+
),
129+
)
130+
return
131+
}
97132
const chunks: Buffer[] = []
98133
res.on('data', (chunk: Buffer) => chunks.push(chunk))
99134
res.on('end', () => {
@@ -110,7 +145,7 @@ async function apiFetch(url: string, init: ApiFetchInit): Promise<Response> {
110145
}
111146
resolve(
112147
new Response(body, {
113-
status: res.statusCode ?? 0,
148+
status: statusCode ?? 0,
114149
statusText: res.statusMessage ?? '',
115150
headers: responseHeaders,
116151
}),
@@ -127,6 +162,17 @@ async function apiFetch(url: string, init: ApiFetchInit): Promise<Response> {
127162
})
128163
}
129164

165+
export async function apiFetch(
166+
url: string,
167+
init: ApiFetchInit = {},
168+
): Promise<Response> {
169+
const agent = getHttpsAgent()
170+
if (!agent) {
171+
return await fetch(url, init as globalThis.RequestInit)
172+
}
173+
return await _httpsRequestFetch(url, init, agent, 0)
174+
}
175+
130176
export type CommandRequirements = {
131177
permissions?: string[] | undefined
132178
quota?: number | undefined

src/utils/api.test.mts

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -337,4 +337,115 @@ describe('apiFetch with extra CA certificates', () => {
337337

338338
expect(result.ok).toBe(true)
339339
})
340+
341+
it('should set Content-Length header for POST requests through https.request', async () => {
342+
const caCerts = ['ROOT_CERT', 'EXTRA_CERT']
343+
mockGetExtraCaCerts.mockReturnValue(caCerts)
344+
345+
const mockReq = {
346+
end: vi.fn(),
347+
on: vi.fn(),
348+
write: vi.fn(),
349+
}
350+
351+
mockHttpsRequest.mockImplementation(
352+
(_url: string, _opts: unknown, callback: RequestCallback) => {
353+
setTimeout(() => {
354+
const mockRes = {
355+
headers: { 'content-type': 'application/json' },
356+
on: vi.fn(),
357+
statusCode: 200,
358+
statusMessage: 'OK',
359+
}
360+
const handlers: Record<string, Function> = {}
361+
mockRes.on.mockImplementation((event: string, handler: Function) => {
362+
handlers[event] = handler
363+
return mockRes
364+
})
365+
callback(mockRes)
366+
handlers['data']?.(Buffer.from('{"result":"ok"}'))
367+
handlers['end']?.()
368+
}, 0)
369+
return mockReq
370+
},
371+
)
372+
373+
const { sendApiRequest } = await import('./api.mts')
374+
await sendApiRequest('test/path', {
375+
body: { key: 'value' },
376+
method: 'POST',
377+
})
378+
379+
// Verify Content-Length header was set in the request options.
380+
const callArgs = mockHttpsRequest.mock.calls[0]
381+
const requestHeaders = callArgs[1].headers
382+
expect(requestHeaders['content-length']).toBe(
383+
String(Buffer.byteLength('{"key":"value"}')),
384+
)
385+
})
386+
387+
it('should follow redirects in https.request path', async () => {
388+
const caCerts = ['ROOT_CERT', 'EXTRA_CERT']
389+
mockGetExtraCaCerts.mockReturnValue(caCerts)
390+
391+
const mockReq = {
392+
end: vi.fn(),
393+
on: vi.fn(),
394+
write: vi.fn(),
395+
}
396+
397+
// First call: return a 302 redirect.
398+
mockHttpsRequest.mockImplementationOnce(
399+
(_url: string, _opts: unknown, callback: RequestCallback) => {
400+
setTimeout(() => {
401+
const mockRes = {
402+
headers: { location: 'https://api.socket.dev/v0/redirected' },
403+
on: vi.fn(),
404+
resume: vi.fn(),
405+
statusCode: 302,
406+
statusMessage: 'Found',
407+
}
408+
callback(mockRes as any)
409+
}, 0)
410+
return mockReq
411+
},
412+
)
413+
414+
// Second call: return the actual response.
415+
mockHttpsRequest.mockImplementationOnce(
416+
(_url: string, _opts: unknown, callback: RequestCallback) => {
417+
setTimeout(() => {
418+
const mockRes = {
419+
headers: { 'content-type': 'text/plain' },
420+
on: vi.fn(),
421+
statusCode: 200,
422+
statusMessage: 'OK',
423+
}
424+
const handlers: Record<string, Function> = {}
425+
mockRes.on.mockImplementation((event: string, handler: Function) => {
426+
handlers[event] = handler
427+
return mockRes
428+
})
429+
callback(mockRes)
430+
handlers['data']?.(Buffer.from('redirected response'))
431+
handlers['end']?.()
432+
}, 0)
433+
return mockReq
434+
},
435+
)
436+
437+
const { queryApiSafeText } = await import('./api.mts')
438+
const result = await queryApiSafeText('test/path', 'test request')
439+
440+
// Should have made two https requests: original and redirect.
441+
expect(mockHttpsRequest).toHaveBeenCalledTimes(2)
442+
// Second call should be to the redirected URL.
443+
expect(mockHttpsRequest.mock.calls[1][0]).toBe(
444+
'https://api.socket.dev/v0/redirected',
445+
)
446+
expect(result.ok).toBe(true)
447+
if (result.ok) {
448+
expect(result.data).toBe('redirected response')
449+
}
450+
})
340451
})

src/utils/dlx-binary.mts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ import { readJson } from '@socketsecurity/registry/lib/fs'
3131
import { spawn } from '@socketsecurity/registry/lib/spawn'
3232

3333
import constants from '../constants.mts'
34+
import { apiFetch } from './api.mts'
3435
import { InputError } from './errors.mts'
3536

3637
import type {
@@ -117,7 +118,7 @@ async function downloadBinary(
117118
destPath: string,
118119
checksum?: string,
119120
): Promise<string> {
120-
const response = await fetch(url)
121+
const response = await apiFetch(url)
121122
if (!response.ok) {
122123
throw new InputError(
123124
`Failed to download binary: ${response.status} ${response.statusText}`,

src/utils/sdk.mts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ import { rootCertificates } from 'node:tls'
3131
import { HttpProxyAgent, HttpsProxyAgent } from 'hpagent'
3232

3333
import isInteractive from '@socketregistry/is-interactive/index.cjs'
34+
import { debugFn } from '@socketsecurity/registry/lib/debug'
3435
import { logger } from '@socketsecurity/registry/lib/logger'
3536
import { password } from '@socketsecurity/registry/lib/prompts'
3637
import { isNonEmptyString } from '@socketsecurity/registry/lib/strings'
@@ -98,7 +99,8 @@ export function getExtraCaCerts(): string[] | undefined {
9899
// in an agent replaces the default trust store, so both must be included.
99100
_extraCaCerts = [...rootCertificates, extraCerts]
100101
return _extraCaCerts
101-
} catch {
102+
} catch (e) {
103+
debugFn('warn', `Failed to read certificate file: ${certPath}`, e)
102104
return undefined
103105
}
104106
}

src/utils/sdk.test.mts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,12 @@ vi.mock('./debug.mts', () => ({
7777
debugApiResponse: mockDebugApiResponse,
7878
}))
7979

80+
// Mock registry debug functions used by getExtraCaCerts.
81+
vi.mock('@socketsecurity/registry/lib/debug', () => ({
82+
debugDir: vi.fn(),
83+
debugFn: vi.fn(),
84+
}))
85+
8086
// Mock config.
8187
const mockGetConfigValueOrUndef = vi.hoisted(() => vi.fn(() => undefined))
8288
vi.mock('./config.mts', () => ({

0 commit comments

Comments
 (0)