Skip to content

Commit 5810f5c

Browse files
sjinkssjinks
committed
fix: address code review comments
1 parent 19424da commit 5810f5c

11 files changed

Lines changed: 291 additions & 11 deletions

File tree

‎__tests__/lib/client-file-uploader.js‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,12 @@
22
* @format
33
*/
44

5-
import { getFileHash, getFileMeta, getPartBoundaries } from '../../src/lib/client-file-uploader';
5+
import {
6+
getFileHash,
7+
getFileMeta,
8+
getPartBoundaries,
9+
parseEtagHeader,
10+
} from '../../src/lib/client-file-uploader';
611

712
describe( 'client-file-uploader', () => {
813
describe( 'getFileMeta()', () => {
@@ -90,4 +95,18 @@ describe( 'client-file-uploader', () => {
9095
] );
9196
} );
9297
} );
98+
99+
describe( 'parseEtagHeader()', () => {
100+
it( 'should parse a quoted ETag header', () => {
101+
expect( parseEtagHeader( '"abc123"' ) ).toBe( 'abc123' );
102+
} );
103+
104+
it( 'should strip a weak ETag prefix', () => {
105+
expect( parseEtagHeader( 'W/"abc123"' ) ).toBe( 'abc123' );
106+
} );
107+
108+
it( 'should return an unquoted ETag value as-is', () => {
109+
expect( parseEtagHeader( 'abc123' ) ).toBe( 'abc123' );
110+
} );
111+
} );
93112
} );

‎__tests__/lib/http/proxy-dispatcher.js‎

Lines changed: 204 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,204 @@
1+
import { ProxyAgent } from 'undici';
2+
3+
import { createProxyDispatcher } from '../../../src/lib/http/proxy-dispatcher';
4+
5+
describe( 'createProxyDispatcher', () => {
6+
let savedEnv;
7+
8+
beforeAll( () => {
9+
savedEnv = { ...process.env };
10+
} );
11+
12+
beforeEach( () => {
13+
// Clear all applicable environment variables before each test so each
14+
// test starts "clean" even if the runner has proxies set in the shell.
15+
const envVarsToClear = [
16+
'VIP_PROXY',
17+
'vip_proxy',
18+
'HTTPS_PROXY',
19+
'https_proxy',
20+
'SOCKS_PROXY',
21+
'socks_proxy',
22+
'NO_PROXY',
23+
'no_proxy',
24+
'VIP_USE_SYSTEM_PROXY',
25+
];
26+
for ( const envVar of envVarsToClear ) {
27+
delete process.env[ envVar ];
28+
}
29+
} );
30+
31+
afterAll( () => {
32+
process.env = { ...savedEnv };
33+
} );
34+
35+
// Tests that expect null (no proxy)
36+
it.each( [
37+
{
38+
label: 'no proxies set',
39+
env: {
40+
VIP_USE_SYSTEM_PROXY: '',
41+
VIP_PROXY: '',
42+
HTTPS_PROXY: '',
43+
SOCKS_PROXY: '',
44+
NO_PROXY: '',
45+
},
46+
url: 'https://wpapi.org/api',
47+
},
48+
{
49+
label: 'HTTPS_PROXY set but VIP_USE_SYSTEM_PROXY not set',
50+
env: {
51+
VIP_USE_SYSTEM_PROXY: '',
52+
VIP_PROXY: '',
53+
HTTPS_PROXY: 'https://myproxy.com',
54+
SOCKS_PROXY: '',
55+
NO_PROXY: '',
56+
},
57+
url: 'https://wpapi.org/api',
58+
},
59+
{
60+
label: 'SOCKS_PROXY set but VIP_USE_SYSTEM_PROXY not set',
61+
env: {
62+
VIP_USE_SYSTEM_PROXY: '',
63+
VIP_PROXY: '',
64+
HTTPS_PROXY: '',
65+
SOCKS_PROXY: 'socks5://myproxy.com:4022',
66+
NO_PROXY: '',
67+
},
68+
url: 'https://wpapi.org/api',
69+
},
70+
{
71+
label: 'VIP_USE_SYSTEM_PROXY set but NO_PROXY covers all hosts',
72+
env: {
73+
VIP_USE_SYSTEM_PROXY: '1',
74+
VIP_PROXY: '',
75+
HTTPS_PROXY: 'https://myproxy.com',
76+
SOCKS_PROXY: '',
77+
NO_PROXY: '*',
78+
},
79+
url: 'https://wpapi.org/api',
80+
},
81+
{
82+
label: 'VIP_USE_SYSTEM_PROXY set but NO_PROXY covers the target host',
83+
env: {
84+
VIP_USE_SYSTEM_PROXY: '1',
85+
VIP_PROXY: '',
86+
HTTPS_PROXY: '',
87+
SOCKS_PROXY: 'socks5://myproxy.com:4022',
88+
NO_PROXY: 'wpapi.org',
89+
},
90+
url: 'https://wpapi.org/api',
91+
},
92+
{
93+
label: 'only NO_PROXY is set',
94+
env: {
95+
VIP_USE_SYSTEM_PROXY: '1',
96+
VIP_PROXY: '',
97+
HTTPS_PROXY: '',
98+
SOCKS_PROXY: '',
99+
NO_PROXY: 'wpapi.org,.lndo.site,foo.bar.org',
100+
},
101+
url: 'https://wpapi.org/api',
102+
},
103+
] )( 'should return null when $label', ( { env, url } ) => {
104+
for ( const [ key, value ] of Object.entries( env ) ) {
105+
process.env[ key ] = value;
106+
}
107+
expect( createProxyDispatcher( url ) ).toBeNull();
108+
} );
109+
110+
// Tests that expect a ProxyAgent
111+
it.each( [
112+
{
113+
label: 'VIP_PROXY set (no feature flag required)',
114+
env: {
115+
VIP_USE_SYSTEM_PROXY: '',
116+
VIP_PROXY: 'http://myproxy.com:8080',
117+
HTTPS_PROXY: '',
118+
SOCKS_PROXY: '',
119+
NO_PROXY: '',
120+
},
121+
url: 'https://wpapi.org/api',
122+
},
123+
{
124+
label: 'VIP_PROXY takes precedence over other proxies',
125+
env: {
126+
VIP_USE_SYSTEM_PROXY: '1',
127+
VIP_PROXY: 'http://myproxy.com:8080',
128+
HTTPS_PROXY: 'https://other.com',
129+
SOCKS_PROXY: '',
130+
NO_PROXY: '*',
131+
},
132+
url: 'https://wpapi.org/api',
133+
},
134+
{
135+
label: 'SOCKS_PROXY checked first when VIP_USE_SYSTEM_PROXY is set',
136+
env: {
137+
VIP_USE_SYSTEM_PROXY: '1',
138+
VIP_PROXY: '',
139+
HTTPS_PROXY: 'https://myproxy.com',
140+
SOCKS_PROXY: 'socks5://myproxy.com:4022',
141+
NO_PROXY: '',
142+
},
143+
url: 'https://wpapi.org/api',
144+
},
145+
{
146+
label: 'HTTPS_PROXY used when VIP_USE_SYSTEM_PROXY is set and no SOCKS_PROXY',
147+
env: {
148+
VIP_USE_SYSTEM_PROXY: '1',
149+
VIP_PROXY: '',
150+
HTTPS_PROXY: 'https://myproxy.com',
151+
SOCKS_PROXY: '',
152+
NO_PROXY: '',
153+
},
154+
url: 'https://wpapi.org/api',
155+
},
156+
{
157+
label: 'proxied when NO_PROXY does not cover the target host',
158+
env: {
159+
VIP_USE_SYSTEM_PROXY: '1',
160+
VIP_PROXY: '',
161+
HTTPS_PROXY: 'https://myproxy.com',
162+
SOCKS_PROXY: '',
163+
NO_PROXY: 'wpapi.org,.lndo.site',
164+
},
165+
url: 'https://wpapi2.org/api',
166+
},
167+
{
168+
label: 'SOCKS proxy still used when NO_PROXY does not match host',
169+
env: {
170+
VIP_USE_SYSTEM_PROXY: '1',
171+
VIP_PROXY: '',
172+
HTTPS_PROXY: '',
173+
SOCKS_PROXY: 'socks5://myproxy.com:4022',
174+
NO_PROXY: 'example.com',
175+
},
176+
url: 'https://wpapi.org/api',
177+
},
178+
{
179+
label: 'port-specific NO_PROXY does not block different ports',
180+
env: {
181+
VIP_USE_SYSTEM_PROXY: '1',
182+
VIP_PROXY: '',
183+
HTTPS_PROXY: 'https://myproxy.com',
184+
SOCKS_PROXY: '',
185+
NO_PROXY: 'wpapi.org:8443',
186+
},
187+
url: 'https://wpapi.org/api',
188+
},
189+
] )( 'should return a ProxyAgent when $label', ( { env, url } ) => {
190+
for ( const [ key, value ] of Object.entries( env ) ) {
191+
process.env[ key ] = value;
192+
}
193+
const dispatcher = createProxyDispatcher( url );
194+
expect( dispatcher ).not.toBeNull();
195+
expect( dispatcher ).toBeInstanceOf( ProxyAgent );
196+
} );
197+
198+
it( 'returns the same ProxyAgent instance for the same proxy URL (caching)', () => {
199+
process.env.VIP_PROXY = 'http://myproxy.com:8080';
200+
const first = createProxyDispatcher( 'https://wpapi.org/api' );
201+
const second = createProxyDispatcher( 'https://wpapi2.org/api' );
202+
expect( first ).toBe( second );
203+
} );
204+
} );

‎npm-shrinkwrap.json‎

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

‎package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,7 @@
169169
"socks-proxy-agent": "^10.0.0",
170170
"ssh2": "1.17.0",
171171
"tar": "^7.4.0",
172-
"undici": "^8.2.0",
172+
"undici": "^7.0.0",
173173
"update-notifier": "7.3.1",
174174
"xml2js": "^0.6.2"
175175
},

‎src/lib/analytics/clients/client.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import type { Response } from 'undici';
2+
13
export interface AnalyticsClient {
24
trackEvent( name: string, props?: Record< string, unknown > ): Promise< Response | false >;
35
}

‎src/lib/analytics/clients/pendo.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import debugLib from 'debug';
2+
import { type Response } from 'undici';
23

34
import http from '../../../lib/api/http';
45
import { type Env } from '../../env';

‎src/lib/analytics/index.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import debugLib from 'debug';
33
import env from '../env';
44

55
import type { AnalyticsClient } from './clients/client';
6+
import type { Response } from 'undici';
67

78
const debug = debugLib( '@automattic/vip:analytics' );
89

‎src/lib/api/http.ts‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,12 @@
11
import debugLib from 'debug';
2-
import { fetch, Headers, type BodyInit, type HeadersInit, type RequestInit } from 'undici';
2+
import {
3+
fetch,
4+
Headers,
5+
type BodyInit,
6+
type HeadersInit,
7+
type RequestInit,
8+
type Response,
9+
} from 'undici';
310

411
import { API_HOST } from '../../lib/api';
512
import env from '../../lib/env';

‎src/lib/client-file-uploader.ts‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,22 @@ import { createGunzip, createGzip, Gunzip, ZlibOptions } from 'zlib';
1515
import http, { type FetchOptions } from '../lib/api/http';
1616
import { MB_IN_BYTES } from '../lib/constants/file-size';
1717

18+
export function parseEtagHeader( etag: string ): string {
19+
const normalizedEtag = etag.replace( /^W\//u, '' ).trim();
20+
21+
if ( normalizedEtag.startsWith( '"' ) && normalizedEtag.endsWith( '"' ) ) {
22+
try {
23+
return JSON.parse( normalizedEtag ) as string;
24+
} catch ( err ) {
25+
debug(
26+
`Unable to JSON.parse ETag header, falling back to raw value: ${ ( err as Error ).message }`
27+
);
28+
}
29+
}
30+
31+
return normalizedEtag.replace( /^"(.*)"$/u, '$1' );
32+
}
33+
1834
async function fetchWithRetry(
1935
input: string | URL,
2036
init?: RequestInit,
@@ -656,7 +672,7 @@ async function uploadPart( {
656672
throw new Error( 'Unable to upload file part. Missing ETag response header.' );
657673
}
658674

659-
return JSON.parse( etag ) as string;
675+
return parseEtagHeader( etag );
660676
}
661677

662678
const result = await fetchResponse.text();

‎src/lib/http/proxy-dispatcher.ts‎

Lines changed: 31 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,48 @@
11
import debugLib from 'debug';
2-
import { getProxyForUrl } from 'proxy-from-env';
32
import { ProxyAgent, type Dispatcher } from 'undici';
43

54
const debug = debugLib( 'vip:proxy-dispatcher' );
65

76
const proxyDispatchers = new Map< string, Dispatcher >();
87

8+
function normalizeNoProxyToken( token: string ): string {
9+
return token.trim().toLowerCase();
10+
}
11+
12+
function isNoProxyMatch( hostname: string, port: string, token: string ): boolean {
13+
// eslint-disable-next-line security/detect-possible-timing-attacks
14+
if ( token === '*' ) {
15+
return true;
16+
}
17+
18+
const [ tokenHostRaw, tokenPort ] = token.split( ':' );
19+
const tokenHost = tokenHostRaw.replace( /^\./u, '' );
20+
21+
if ( tokenPort && tokenPort !== port ) {
22+
return false;
23+
}
24+
25+
if ( hostname === tokenHost ) {
26+
return true;
27+
}
28+
29+
return hostname.endsWith( `.${ tokenHost }` );
30+
}
31+
932
function isCoveredByNoProxy( url: string ): boolean {
1033
const NO_PROXY = process.env.NO_PROXY || process.env.no_proxy || null; // NOSONAR
1134

1235
if ( ! NO_PROXY ) {
1336
return false;
1437
}
1538

16-
return getProxyForUrl( url ) === '';
39+
const parsedUrl = new URL( url );
40+
const hostname = parsedUrl.hostname.toLowerCase();
41+
const port = parsedUrl.port || ( parsedUrl.protocol === 'http:' ? '80' : '443' );
42+
43+
const noProxyTokens = NO_PROXY.split( ',' ).map( normalizeNoProxyToken ).filter( Boolean );
44+
45+
return noProxyTokens.some( token => isNoProxyMatch( hostname, port, token ) );
1746
}
1847

1948
function resolveProxyUrl( url: string ): string | null {

0 commit comments

Comments
 (0)