Skip to content

Commit 4ffa455

Browse files
sjinkssjinks
committed
fix: address code review comments
1 parent 19424da commit 4ffa455

14 files changed

Lines changed: 309 additions & 15 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+
} );

‎jest.setup.js‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { MockAgent, setGlobalDispatcher } from 'undici';
1+
import { fetch, MockAgent, setGlobalDispatcher } from 'undici';
22

33
process.env.API_HOST = 'http://localhost:4000';
44

@@ -20,5 +20,6 @@ delete process.env.VIP_USE_SYSTEM_PROXY;
2020
const mockAgent = new MockAgent();
2121
mockAgent.disableNetConnect();
2222
setGlobalDispatcher( mockAgent );
23+
globalThis.fetch = fetch;
2324

2425
global.__UNDICI_MOCK_AGENT__ = mockAgent;

‎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: 9 additions & 2 deletions
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';
@@ -35,7 +42,7 @@ export default async ( path: string, options: FetchOptions = {} ): Promise< Resp
3542

3643
debug( 'running fetch', url );
3744

38-
const headers = new Headers( { ...options.headers } );
45+
const headers = new Headers( options.headers );
3946
if ( ! headers.has( 'Authorization' ) ) {
4047
headers.set( 'Authorization', `Bearer ${ authToken.raw }` );
4148
}

‎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();

0 commit comments

Comments
 (0)