rayan/bruno
1
0
Fork 0
mirror of https://github.com/usebruno/bruno.git synced 2026-06-27 22:54:07 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(bruno-requests): address code review findings for agent caching

Browse Source 
- Fix Buffer hashing bug: properly handle Buffer values in hashValue()
- Add CA array support: new hashCaValue() handles string[] | Buffer[]
- Fix timeline race condition: capture timeline reference in closure
  at createConnection start to isolate concurrent requests
- Fix SSL verify message: check socket.authorized for accurate status
- Fix HTTP/HTTPS agent logic: only set httpsAgent for HTTPS requests
- Add tests for concurrent requests timeline isolation

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
lohit-bruno
2026-01-30 15:50:39 +05:30
parent 14c66bc42f
commit 15c86f8e6b
5 changed files with 186 additions and 32 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
packages/bruno-cli/src/runner/run-single-request.js
View File

@@ -486,8 +486,6 @@ const runSingleRequest = async function (
if (https_proxy?.length && isHttpsRequest) {
new URL(https_proxy);
request.httpsAgent = getOrCreateAgent(PatchedHttpsProxyAgent, tlsOptions, https_proxy);
} else {
request.httpsAgent = getOrCreateAgent(https.Agent, tlsOptions);
}
} catch (error) {
throw new Error('Invalid system https_proxy');

9
packages/bruno-electron/src/utils/proxy-util.js
View File

@@ -158,6 +158,13 @@ function setupProxyAgents({
try {
if (http_proxy?.length && !isHttpsRequest) {
new URL(http_proxy);
if (timeline) {
timeline.push({
timestamp: new Date(),
type: 'info',
message: `Using system proxy: ${http_proxy}`
});
}
requestConfig.httpAgent = getOrCreateHttpAgent(HttpProxyAgent, { keepAlive: true }, http_proxy, timeline);
}
} catch (error) {
@@ -174,8 +181,6 @@ function setupProxyAgents({
});
}
requestConfig.httpsAgent = getOrCreateAgent(PatchedHttpsProxyAgent, tlsOptions, https_proxy, timeline);
} else {
requestConfig.httpsAgent = getOrCreateAgent(https.Agent, tlsOptions, null, timeline);
}
} catch (error) {
throw new Error(`Invalid system https_proxy "${https_proxy}": ${error.message}`);

109
packages/bruno-requests/src/utils/agent-cache.spec.ts
View File

@@ -1,5 +1,6 @@
import https from 'node:https';
import http from 'node:http';
import { EventEmitter } from 'node:events';
import { getOrCreateAgent, clearAgentCache, getAgentCacheSize } from './agent-cache';
describe('Agent Cache', () => {
@@ -166,4 +167,112 @@ describe('Agent Cache', () => {
expect(destroyMock).toHaveBeenCalled();
});
});
describe('concurrent requests timeline isolation', () => {
it('isolates timeline events for concurrent requests using the same cached agent', () => {
const timeline1: any[] = [];
const timeline2: any[] = [];
// Get the same agent twice with different timelines (simulating concurrent requests)
const agent1 = getOrCreateAgent(https.Agent, {}, null, timeline1) as any;
const agent2 = getOrCreateAgent(https.Agent, {}, null, timeline2) as any;
// Both should return the same cached agent
expect(agent1).toBe(agent2);
// Create mock sockets to simulate concurrent connections
const mockSocket1 = new EventEmitter() as any;
mockSocket1.remoteAddress = '1.2.3.4';
mockSocket1.remotePort = 443;
mockSocket1.getProtocol = () => 'TLSv1.3';
mockSocket1.getCipher = () => ({ name: 'AES-256-GCM', version: 'TLSv1.3' });
mockSocket1.alpnProtocol = 'h2';
mockSocket1.getPeerCertificate = () => ({
subject: { CN: 'example.com' },
valid_from: 'Jan 1 00:00:00 2024 GMT',
valid_to: 'Jan 1 00:00:00 2025 GMT'
});
mockSocket1.authorized = true;
const mockSocket2 = new EventEmitter() as any;
mockSocket2.remoteAddress = '5.6.7.8';
mockSocket2.remotePort = 443;
mockSocket2.getProtocol = () => 'TLSv1.3';
mockSocket2.getCipher = () => ({ name: 'AES-256-GCM', version: 'TLSv1.3' });
mockSocket2.alpnProtocol = 'http/1.1';
mockSocket2.getPeerCertificate = () => ({
subject: { CN: 'other.com' },
valid_from: 'Jan 1 00:00:00 2024 GMT',
valid_to: 'Jan 1 00:00:00 2025 GMT'
});
mockSocket2.authorized = true;
// Mock createConnection to return our mock sockets
const originalCreateConnection = Object.getPrototypeOf(Object.getPrototypeOf(agent1)).createConnection;
let callCount = 0;
jest.spyOn(Object.getPrototypeOf(Object.getPrototypeOf(agent1)), 'createConnection').mockImplementation(function (this: any, options: any, callback: any) {
callCount++;
return callCount === 1 ? mockSocket1 : mockSocket2;
});
// Simulate request 1 starting - this captures timeline1 in the closure
agent1.timeline = timeline1;
const socket1 = agent1.createConnection({ host: 'example.com', port: 443 }, () => {});
// Before request 1's events fire, request 2 starts and updates agent.timeline
// This simulates the race condition
agent1.timeline = timeline2;
const socket2 = agent1.createConnection({ host: 'other.com', port: 443 }, () => {});
// Now fire events for both sockets - they should go to their respective timelines
mockSocket1.emit('connect');
mockSocket1.emit('secureConnect');
mockSocket2.emit('connect');
mockSocket2.emit('secureConnect');
// Verify timeline1 only contains events for request 1 (example.com)
const timeline1Messages = timeline1.map((e) => e.message);
expect(timeline1Messages.some((m) => m.includes('example.com'))).toBe(true);
expect(timeline1Messages.some((m) => m.includes('other.com'))).toBe(false);
// Verify timeline2 only contains events for request 2 (other.com)
const timeline2Messages = timeline2.map((e) => e.message);
expect(timeline2Messages.some((m) => m.includes('other.com'))).toBe(true);
expect(timeline2Messages.some((m) => m.includes('example.com'))).toBe(false);
// Restore the original implementation
jest.restoreAllMocks();
});
it('logs events to captured timeline even after agent.timeline is reassigned', () => {
const timeline1: any[] = [];
const timeline2: any[] = [];
const agent = getOrCreateAgent(https.Agent, {}, null, timeline1) as any;
// Create a mock socket
const mockSocket = new EventEmitter() as any;
mockSocket.remoteAddress = '1.2.3.4';
mockSocket.remotePort = 443;
// Mock createConnection
jest.spyOn(Object.getPrototypeOf(Object.getPrototypeOf(agent)), 'createConnection').mockImplementation(() => mockSocket);
// Start creating connection - this captures timeline1
const socket = agent.createConnection({ host: 'test.com', port: 443 }, () => {});
// Reassign agent.timeline (simulating another request coming in)
agent.timeline = timeline2;
// Fire the connect event - this should still go to timeline1 (captured reference)
mockSocket.emit('connect');
// Verify event went to timeline1, not timeline2
expect(timeline1.some((e) => e.message.includes('Connected to test.com'))).toBe(true);
expect(timeline2.some((e) => e.message.includes('Connected to test.com'))).toBe(false);
jest.restoreAllMocks();
});
});
});

20
packages/bruno-requests/src/utils/agent-cache.ts
View File

@@ -54,7 +54,23 @@ function getAgentClassId(AgentClass: any): number {
*/
function hashValue(value: string | Buffer | undefined): string | null {
if (!value) return null;
return crypto.createHash('sha256').update(String(value)).digest('hex').slice(0, 16);
const data = Buffer.isBuffer(value) ? value : String(value);
return crypto.createHash('sha256').update(data).digest('hex').slice(0, 16);
}
/**
* Hash a CA value which can be a single value or an array of certificates.
* Node.js TLS options allow ca to be string | Buffer | (string | Buffer)[].
*/
function hashCaValue(value: string | Buffer | (string | Buffer)[] | undefined): string | null {
if (!value) return null;
if (Array.isArray(value)) {
// Concatenate all values with separator and hash together
const combined = value.map((v) => (Buffer.isBuffer(v) ? v.toString('base64') : String(v))).join('|');
return crypto.createHash('sha256').update(combined).digest('hex').slice(0, 16);
}
const data = Buffer.isBuffer(value) ? value : String(value);
return crypto.createHash('sha256').update(data).digest('hex').slice(0, 16);
}
/**
@@ -68,7 +84,7 @@ function getAgentCacheKey(agentClassId: number, options: AgentOptions, proxyUri:
proxyUri,
rejectUnauthorized: options.rejectUnauthorized,
// Hash certificates and passphrase instead of including full content
ca: hashValue(options.ca as string | Buffer | undefined),
ca: hashCaValue(options.ca),
cert: hashValue(options.cert),
key: hashValue(options.key),
pfx: hashValue(options.pfx),

78
packages/bruno-requests/src/utils/timeline-agent.ts
View File

@@ -112,9 +112,20 @@ function createTimelineAgentClass<T extends ProxyAgentClass | typeof https.Agent
createConnection(options: any, callback: any) {
const { host, port } = options;
// Capture the current timeline reference to avoid race conditions
// when multiple concurrent requests reuse the same cached agent
const timeline = this.timeline;
const log = (type: 'info' | 'tls' | 'error', message: string): void => {
timeline.push({
timestamp: new Date(),
type,
message
});
};
// Log ALPN protocols offered
if (this.alpnProtocols && this.alpnProtocols.length > 0) {
this.log('tls', `ALPN: offers ${this.alpnProtocols.join(', ')}`);
log('tls', `ALPN: offers ${this.alpnProtocols.join(', ')}`);
}
const rootCerts = this.caCertificatesCount.root || 0;
@@ -122,26 +133,26 @@ function createTimelineAgentClass<T extends ProxyAgentClass | typeof https.Agent
const extraCerts = this.caCertificatesCount.extra || 0;
const customCerts = this.caCertificatesCount.custom || 0;
this.log('tls', `CA Certificates: ${rootCerts} root, ${systemCerts} system, ${extraCerts} extra, ${customCerts} custom`);
log('tls', `CA Certificates: ${rootCerts} root, ${systemCerts} system, ${extraCerts} extra, ${customCerts} custom`);
// Log "Trying host:port..."
this.log('info', `Trying ${host}:${port}...`);
log('info', `Trying ${host}:${port}...`);
let socket: any;
try {
socket = super.createConnection(options, callback);
} catch (error: any) {
this.log('error', `Error creating connection: ${error.message}`);
error.timeline = this.timeline;
log('error', `Error creating connection: ${error.message}`);
error.timeline = timeline;
throw error;
}
// Attach event listeners to the socket
socket?.on('lookup', (err: Error | null, address: string, family: number, host: string) => {
if (err) {
this.log('error', `DNS lookup error for ${host}: ${err.message}`);
log('error', `DNS lookup error for ${host}: ${err.message}`);
} else {
this.log('info', `DNS lookup: ${host} -> ${address}`);
log('info', `DNS lookup: ${host} -> ${address}`);
}
});
@@ -149,7 +160,7 @@ function createTimelineAgentClass<T extends ProxyAgentClass | typeof https.Agent
const address = socket.remoteAddress || host;
const remotePort = socket.remotePort || port;
this.log('info', `Connected to ${host} (${address}) port ${remotePort}`);
log('info', `Connected to ${host} (${address}) port ${remotePort}`);
});
socket?.on('secureConnect', () => {
@@ -157,39 +168,43 @@ function createTimelineAgentClass<T extends ProxyAgentClass | typeof https.Agent
const cipher = socket.getCipher?.();
const cipherSuite = cipher ? `${cipher.name} (${cipher.version})` : 'Unknown cipher';
this.log('tls', `SSL connection using ${protocol} / ${cipherSuite}`);
log('tls', `SSL connection using ${protocol} / ${cipherSuite}`);
// ALPN protocol
const alpnProtocol = socket.alpnProtocol || 'None';
this.log('tls', `ALPN: server accepted ${alpnProtocol}`);
log('tls', `ALPN: server accepted ${alpnProtocol}`);
// Server certificate
const cert = socket.getPeerCertificate?.(true);
if (cert) {
this.log('tls', `Server certificate:`);
log('tls', `Server certificate:`);
if (cert.subject) {
this.log('tls', ` subject: ${Object.entries(cert.subject).map(([k, v]) => `${k}=${v}`).join(', ')}`);
log('tls', ` subject: ${Object.entries(cert.subject).map(([k, v]) => `${k}=${v}`).join(', ')}`);
}
if (cert.valid_from) {
this.log('tls', ` start date: ${cert.valid_from}`);
log('tls', ` start date: ${cert.valid_from}`);
}
if (cert.valid_to) {
this.log('tls', ` expire date: ${cert.valid_to}`);
log('tls', ` expire date: ${cert.valid_to}`);
}
if (cert.subjectaltname) {
this.log('tls', ` subjectAltName: ${cert.subjectaltname}`);
log('tls', ` subjectAltName: ${cert.subjectaltname}`);
}
if (cert.issuer) {
this.log('tls', ` issuer: ${Object.entries(cert.issuer).map(([k, v]) => `${k}=${v}`).join(', ')}`);
log('tls', ` issuer: ${Object.entries(cert.issuer).map(([k, v]) => `${k}=${v}`).join(', ')}`);
}
// SSL certificate verify ok
this.log('tls', `SSL certificate verify ok.`);
// SSL certificate verification status
if (socket.authorized !== false) {
log('tls', `SSL certificate verify ok.`);
} else {
log('tls', `SSL certificate verification skipped (rejectUnauthorized: false).`);
}
}
});
socket?.on('error', (err: Error) => {
this.log('error', `Socket error: ${err.message}`);
log('error', `Socket error: ${err.message}`);
});
return socket;
@@ -242,24 +257,35 @@ function createTimelineHttpAgentClass<T extends HttpProxyAgentClass | typeof htt
createConnection(options: any, callback: any) {
const { host, port } = options;
// Capture the current timeline reference to avoid race conditions
// when multiple concurrent requests reuse the same cached agent
const timeline = this.timeline;
const log = (type: 'info' | 'tls' | 'error', message: string): void => {
timeline.push({
timestamp: new Date(),
type,
message
});
};
// Log "Trying host:port..."
this.log('info', `Trying ${host}:${port}...`);
log('info', `Trying ${host}:${port}...`);
let socket: any;
try {
socket = super.createConnection(options, callback);
} catch (error: any) {
this.log('error', `Error creating connection: ${error.message}`);
error.timeline = this.timeline;
log('error', `Error creating connection: ${error.message}`);
error.timeline = timeline;
throw error;
}
// Attach event listeners to the socket
socket?.on('lookup', (err: Error | null, address: string, family: number, host: string) => {
if (err) {
this.log('error', `DNS lookup error for ${host}: ${err.message}`);
log('error', `DNS lookup error for ${host}: ${err.message}`);
} else {
this.log('info', `DNS lookup: ${host} -> ${address}`);
log('info', `DNS lookup: ${host} -> ${address}`);
}
});
@@ -267,11 +293,11 @@ function createTimelineHttpAgentClass<T extends HttpProxyAgentClass | typeof htt
const address = socket.remoteAddress || host;
const remotePort = socket.remotePort || port;
this.log('info', `Connected to ${host} (${address}) port ${remotePort}`);
log('info', `Connected to ${host} (${address}) port ${remotePort}`);
});
socket?.on('error', (err: Error) => {
this.log('error', `Socket error: ${err.message}`);
log('error', `Socket error: ${err.message}`);
});
return socket;