Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: remove node-fetch dependency, use custom fetch implementation #8486

Merged
merged 2 commits into from Aug 26, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 0 additions & 18 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion package.json
Expand Up @@ -67,7 +67,6 @@
"mime": "^2.4.6",
"minimatch": "^3.0.3",
"ms": "^2.1.2",
"node-fetch": "^2.6.1",
yury-s marked this conversation as resolved.
Show resolved Hide resolved
"pirates": "^4.0.1",
"pixelmatch": "^5.2.1",
"pngjs": "^5.0.0",
Expand Down
1 change: 0 additions & 1 deletion packages/build_package.js
Expand Up @@ -72,7 +72,6 @@ const DEPENDENCIES = [
'https-proxy-agent',
'jpeg-js',
'mime',
'node-fetch',
'pngjs',
'progress',
'proper-lockfile',
Expand Down
164 changes: 120 additions & 44 deletions src/server/fetch.ts
Expand Up @@ -15,8 +15,9 @@
*/

import { HttpsProxyAgent } from 'https-proxy-agent';
import nodeFetch from 'node-fetch';
import * as url from 'url';
import url from 'url';
import * as http from 'http';
import * as https from 'https';
import { BrowserContext } from './browserContext';
import * as types from './types';

Expand Down Expand Up @@ -45,54 +46,130 @@ export async function playwrightFetch(context: BrowserContext, params: types.Fet
}

// TODO(https://github.com/microsoft/playwright/issues/8381): set user agent
const response = await nodeFetch(params.url, {
const {fetchResponse, setCookie} = await sendRequest(new URL(params.url), {
method: params.method,
headers: params.headers,
body: params.postData,
agent
});
const body = await response.buffer();
const setCookies = response.headers.raw()['set-cookie'];
if (setCookies) {
const url = new URL(response.url);
// https://datatracker.ietf.org/doc/html/rfc6265#section-5.1.4
const defaultPath = '/' + url.pathname.split('/').slice(0, -1).join('/');
const cookies: types.SetNetworkCookieParam[] = [];
for (const header of setCookies) {
// Decode cookie value?
const cookie: types.SetNetworkCookieParam | null = parseCookie(header);
if (!cookie)
continue;
if (!cookie.domain)
cookie.domain = url.hostname;
if (!canSetCookie(cookie.domain!, url.hostname))
continue;
// https://datatracker.ietf.org/doc/html/rfc6265#section-5.2.4
if (!cookie.path || !cookie.path.startsWith('/'))
cookie.path = defaultPath;
cookies.push(cookie);
}
if (cookies.length)
await context.addCookies(cookies);
}

const headers: types.HeadersArray = [];
for (const [name, value] of response.headers.entries())
headers.push({ name, value });
return {
fetchResponse: {
url: response.url,
status: response.status,
statusText: response.statusText,
headers,
body
}
};
agent,
maxRedirects: 20
}, params.postData);
if (setCookie)
await updateCookiesFromHeader(context, fetchResponse.url, setCookie);
return { fetchResponse };
} catch (e) {
return { error: String(e) };
}
}

async function updateCookiesFromHeader(context: BrowserContext, responseUrl: string, setCookie: string[]) {
const url = new URL(responseUrl);
// https://datatracker.ietf.org/doc/html/rfc6265#section-5.1.4
const defaultPath = '/' + url.pathname.split('/').slice(0, -1).join('/');
const cookies: types.SetNetworkCookieParam[] = [];
for (const header of setCookie) {
// Decode cookie value?
const cookie: types.SetNetworkCookieParam | null = parseCookie(header);
if (!cookie)
continue;
if (!cookie.domain)
cookie.domain = url.hostname;
if (!canSetCookie(cookie.domain!, url.hostname))
continue;
// https://datatracker.ietf.org/doc/html/rfc6265#section-5.2.4
if (!cookie.path || !cookie.path.startsWith('/'))
cookie.path = defaultPath;
cookies.push(cookie);
}
if (cookies.length)
await context.addCookies(cookies);
}

type Response = {
fetchResponse: types.FetchResponse,
setCookie?: string[]
};

async function sendRequest(url: URL, options: http.RequestOptions & { maxRedirects: number }, postData?: Buffer): Promise<Response>{
return new Promise<Response>((fulfill, reject) => {
const requestConstructor: ((url: URL, options: http.RequestOptions, callback?: (res: http.IncomingMessage) => void) => http.ClientRequest)
= (url.protocol === 'https:' ? https : http).request;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO for data urls?

Copy link
Member Author

@yury-s yury-s Aug 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we don't need them. Data url can be easily loaded directly in a page. We can always add it later if there is demand.

const request = requestConstructor(url, options, response => {
if (redirectStatus.includes(response.statusCode!)) {
if (!options.maxRedirects) {
reject(new Error('Max redirect count exceeded'));
request.abort();
return;
}
const redirectOptions: http.RequestOptions & { maxRedirects: number } = {
method: options.method,
headers: { ...options.headers },
agent: options.agent,
maxRedirects: options.maxRedirects - 1,
};

// HTTP-redirect fetch step 13 (https://fetch.spec.whatwg.org/#http-redirect-fetch)
const status = response.statusCode!;
const method = redirectOptions.method!;
if ((status === 301 || status === 302) && method === 'POST' ||
status === 303 && !['GET', 'HEAD'].includes(method)) {
redirectOptions.method = 'GET';
postData = undefined;
delete redirectOptions.headers?.[`content-encoding`];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redirectionOptions.headers is coming from the options. Do you ensure there that the user is passing the headers in lower-case?

delete redirectOptions.headers?.[`content-language`];
delete redirectOptions.headers?.[`content-location`];
delete redirectOptions.headers?.[`content-type`];
}

// TODO: set-cookie from response, add cookie from the context.

// HTTP-redirect fetch step 4: If locationURL is null, then return response.
if (response.headers.location) {
const locationURL = new URL(response.headers.location, url);
fulfill(sendRequest(locationURL, redirectOptions, postData));
request.abort();
return;
}
}
const chunks: Buffer[] = [];
response.on('data', chunk => chunks.push(chunk));
response.on('end', () => {
const body = Buffer.concat(chunks);
fulfill({
fetchResponse: {
url: response.url || url.toString(),
status: response.statusCode || 0,
statusText: response.statusMessage || '',
headers: flattenHeaders(response.headers),
body
},
setCookie: response.headers['set-cookie']
});
});
response.on('error',reject);
});
request.on('error', reject);
if (postData)
request.write(postData);
request.end();
});
}

function flattenHeaders(headers: http.IncomingHttpHeaders): types.HeadersArray {
const result: types.HeadersArray = [];
for (const [name, values] of Object.entries(headers)) {
if (values === undefined)
continue;
if (typeof values === 'string') {
result.push({name, value: values as string});
} else {
for (const value of values)
result.push({name, value});
}
}
return result;
}

const redirectStatus = [301, 302, 303, 307, 308];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: please move that before sendRequest, otherwise code navigation is weird.


function canSetCookie(cookieDomain: string, hostname: string) {
// TODO: check public suffix list?
hostname = '.' + hostname;
Expand All @@ -101,7 +178,6 @@ function canSetCookie(cookieDomain: string, hostname: string) {
return hostname.endsWith(cookieDomain);
}


function parseCookie(header: string) {
const pairs = header.split(';').filter(s => s.trim().length > 0).map(p => p.split('=').map(s => s.trim()));
if (!pairs.length)
Expand Down
5 changes: 1 addition & 4 deletions src/server/types.ts
Expand Up @@ -379,9 +379,6 @@ export type FetchResponse = {
url: string,
status: number,
statusText: string,
headers: {
name: string,
value: string,
}[],
headers: HeadersArray,
body: Buffer,
};