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

core: added capability to pass cookies as CLI parameter #9170

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions lighthouse-cli/bin.js
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,21 @@ async function begin() {
cliFlags.extraHeaders = JSON.parse(extraHeadersStr);
}

if (cliFlags.extraCookies) {
// TODO: LH.Flags.extraCookies is actually a string at this point, but needs to be
Copy link
Collaborator

Choose a reason for hiding this comment

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

major bummer that we'll have a second one of these before we fix the first, this will also actually run into problems with the config-based CLI flags approach.

maybe we could at least extract this to a function to share between the two?

Copy link
Member

Choose a reason for hiding this comment

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

major bummer that we'll have a second one of these before we fix the first, this will also actually run into problems with the config-based CLI flags approach.

Yes, definitely, this shouldn't be done a second time :) The TODO even lists how to fix:

either the CLI flag or the setting should have a different name.

Copy link
Collaborator

Choose a reason for hiding this comment

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

well that's the partial fix for the path problem, unless I'm misunderstanding?

there's not really an obvious fix I see for the "this thing is a complex object that can't be represented by yargs that we want to JSON.parse from the CLI but just use from node", --cookies-json-string I guess but that feels 🤢

Copy link
Author

Choose a reason for hiding this comment

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

@brendankenny @patrickhulce I am not pretty sure which fix is required here, as I am Java developer. I assume yargs can't provide you correctly typed JSON value out of the box.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're right on that point @RynatSibahatau! The best we can do and what the comment suggests is that the property name should just be different for the yargs string version.

@brendankenny thoughts on extraCookiesJsonString for yargs and extraCookies for regular? (or we nix the extra prefix entirely I think was your last suggestion?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

bump on this @brendankenny

Copy link
Member

Choose a reason for hiding this comment

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

we could do --cookies for the flag (on the CliFlags interface) and extraCookies for the setting (on SharedFlagsSettings). Or --cookies-string and cookies.

Whatever it is, we should absolutely use this pattern. It's awkward, but most folks won't really care what the flag is (shortish is probably better, though), and the TODOs that have propagated from this point can only be removed in the future by doing this, so might as well do it before landing :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I vote --cookies-string and --cookies then! 👍

// copied over to LH.Settings.extraCookies, which is LH.Crdp.Network.Cookies. Force
// the conversion here, but long term either the CLI flag or the setting should have
// a different name.
// @ts-ignore
let extraCookiesStr = /** @type {string} */ (cliFlags.extraCookies);
// If not a JSON array, assume it's a path to a JSON file.
if (extraCookiesStr.substr(0, 1) !== '[') {
extraCookiesStr = fs.readFileSync(extraCookiesStr, 'utf-8');
Copy link
Member

Choose a reason for hiding this comment

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

yeah, I agree with @hoten that the ambiguity in the overloading of the flag is not worth it (can use the new --cli-flags-path if want to read from file!), but I do see we have it for --extra-headers already.

It would be nice to not include it by default and then consider adding it in the future if there's demand

Copy link
Author

Choose a reason for hiding this comment

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

--cli-flags-path approach looks good for me, I would agree we can avoid this dualism here for cookies. Should I remove it for the headers as well as part of the PR?
If there are no other major concerns for merging the PR. I can start implementing this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would love to remove it from headers but that'd be a major breaking change. Added it to our new list for next major version :) #9180

Copy link
Author

Choose a reason for hiding this comment

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

I have removed the capability to load cookie from file with the same parameter
1677a37

}

cliFlags.extraCookies = JSON.parse(extraCookiesStr);
}
Copy link
Author

Choose a reason for hiding this comment

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

Suggested change
}
if (!Array.isArray(cliFlags.extraCookies)) {
throw new Error('Not a valid JSON array');
}
cliFlags.extraCookies.forEach(key => {
const cookie = cliFlags.extraCookies[key];
if (! cookie.url || ! cookie.domain) {
// Default it to the domain value
cookie.url = url;
}
});
}

Copy link
Author

Choose a reason for hiding this comment

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

I can suggest this extra check and defaulting for the URL to avoid no-op

Copy link
Collaborator

Choose a reason for hiding this comment

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

this is a good idea @RynatSibahatau! I think we would want this in lighthouse-core/ though so node users get this defaulting benefit too

Copy link
Author

Choose a reason for hiding this comment

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

@patrickhulce Could you take a look 074eee1?

Copy link
Collaborator

Choose a reason for hiding this comment

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

that's exactly what I was hoping for! thanks!


if (cliFlags.precomputedLanternDataPath) {
const lanternDataStr = fs.readFileSync(cliFlags.precomputedLanternDataPath, 'utf8');
/** @type {LH.PrecomputedLanternData} */
Expand Down
10 changes: 9 additions & 1 deletion lighthouse-cli/cli-flags.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,17 @@ function getFlags(manualArgv) {
'lighthouse <url> --quiet --chrome-flags="--headless"',
'Launch Headless Chrome, turn off logging')
.example(
'lighthouse <url> --extra-headers "{\\"Cookie\\":\\"monster=blue\\", \\"x-men\\":\\"wolverine\\"}"',
'lighthouse <url> --extra-headers "{\\"x-men\\":\\"wolverine\\"}"',
'Stringify\'d JSON HTTP Header key/value pairs to send in requests')
.example(
'lighthouse <url> --extra-headers=./path/to/file.json',
'Path to JSON file of HTTP Header key/value pairs to send in requests')
.example(
'lighthouse <url> --extra-cookies "[{\\"name\\":\\"session_id\\",\\"value\\":\\"x-men\\" }]"',
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
'Stringify\'d JSON array of HTTP Cookies to send in requests')
.example(
'lighthouse <url> --extra-cookies=./path/to/file.json',
'Path to JSON file of HTTP Cookies to send in requests')
.example(
'lighthouse <url> --only-categories=performance,pwa',
'Only run specific categories.')
Expand Down Expand Up @@ -126,6 +132,7 @@ function getFlags(manualArgv) {
'max-wait-for-load':
'The timeout (in milliseconds) to wait before the page is considered done loading and the run should continue. WARNING: Very high values can lead to large traces and instability',
'extra-headers': 'Set extra HTTP Headers to pass with request',
'extra-cookies': 'Set extra HTTP Cookies to pass with request',
'precomputed-lantern-data-path': 'Path to the file where lantern simulation data should be read from, overwriting the lantern observed estimates for RTT and server latency.',
'lantern-data-output-path': 'Path to the file where lantern simulation data should be written to, can be used in a future run with the `precomputed-lantern-data-path` flag.',
'only-audits': 'Only run the specified audits',
Expand Down Expand Up @@ -166,6 +173,7 @@ function getFlags(manualArgv) {
.array('output')
.array('plugins')
.string('extraHeaders')
.string('extraCookies')
.string('channel')
.string('precomputedLanternDataPath')
.string('lanternDataOutputPath')
Expand Down
2 changes: 2 additions & 0 deletions lighthouse-cli/test/cli/__snapshots__/index-test.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -1237,6 +1237,7 @@ Object {
"channel": "cli",
"disableStorageReset": false,
"emulatedFormFactor": "mobile",
"extraCookies": null,
"extraHeaders": null,
"gatherMode": false,
"locale": "en-US",
Expand Down Expand Up @@ -1371,6 +1372,7 @@ Object {
"channel": "cli",
"disableStorageReset": false,
"emulatedFormFactor": "mobile",
"extraCookies": null,
"extraHeaders": null,
"gatherMode": false,
"locale": "en-US",
Expand Down
20 changes: 20 additions & 0 deletions lighthouse-cli/test/cli/bin-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,26 @@ describe('CLI bin', function() {
});
});

describe('extraCookies', () => {
it('should convert extra cookies to object', async () => {
// @ts-ignore - see TODO: in bin.js
cliFlags = {...cliFlags, extraCookies: '[{"name":"foo", "value": "bar", "url": "http://localhost"}]'};
await bin.begin();

expect(getRunLighthouseArgs()[1]).toHaveProperty('extraCookies', [{'name': 'foo', 'value': 'bar', 'url': 'http://localhost'}]);
});

it('should read extra cookies from file', async () => {
const headersFile = require.resolve('../fixtures/extra-cookies/valid.json');
// @ts-ignore - see TODO: in bin.js
cliFlags = {...cliFlags, extraCookies: headersFile};
await bin.begin();

expect(getRunLighthouseArgs()[1]).toHaveProperty('extraCookies', require(headersFile));
});
});


describe('precomputedLanternData', () => {
it('should read lantern data from file', async () => {
const lanternDataFile = require.resolve('../fixtures/lantern-data.json');
Expand Down
9 changes: 9 additions & 0 deletions lighthouse-cli/test/cli/index-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,15 @@ describe('CLI Tests', function() {
assert.equal(ret.status, 1);
});

it('should exit with a error if the file does not contain valid JSON', () => {
const ret = spawnSync('node', [indexPath, 'https://www.google.com',
'--extra-cookies',
path.resolve(__dirname, '../fixtures/extra-cookies/invalid.txt')], {encoding: 'utf8'});

assert.ok(ret.stderr.includes('Unexpected token'));
assert.equal(ret.status, 1);
});

it('should exit with a error if the passsed in string is not valid JSON', () => {
const ret = spawnSync('node', [indexPath, 'https://www.google.com',
'--extra-headers', '{notjson}'], {encoding: 'utf8'});
Expand Down
1 change: 1 addition & 0 deletions lighthouse-cli/test/fixtures/extra-cookies/invalid.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
NotJSON
7 changes: 7 additions & 0 deletions lighthouse-cli/test/fixtures/extra-cookies/valid.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[
{
"name":"test",
"value":"true",
"url":"http://localhost"
}
]
14 changes: 14 additions & 0 deletions lighthouse-cli/test/fixtures/static-server.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,20 @@ function requestHandler(request, response) {
}
}

if (params.has('extra_cookie')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this file is for our smoketests, we'll want to make sure we add a smoketest that exercises this functionality which is probably going to be tricky...

maybe hold off on this step until we get buy-in from rest of LH team for this feature to work this way?

Copy link
Member

Choose a reason for hiding this comment

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

any reason to keep the changes to this file?

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh @RynatSibahatau think you could add a smoketest exercising this and our cookies logic?

Copy link
Member

Choose a reason for hiding this comment

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

oh @RynatSibahatau think you could add a smoketest exercising this and our cookies logic?

isn't that going to be kind of hard, @patrickhulce? What would we be testing, exactly?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking a page has a link or something that contains the contents of the cookies and it gets picked up by the anchor elements artifact we can assert

const extraCookies = new URLSearchParams(params.get('extra_cookie'));
let cookeString = '';
for (const [cookieName, cookieValue] of extraCookies) {
cookeString += cookieName + '=' + cookieValue + ';';
}

// Extra cookie we allways override possible 'Set-Cookie' header
// which may be already present in request by extra_header
headers['Set-Cookie'] = [];
headers['Set-Cookie'].push(cookeString);
}


if (params.has('gzip')) {
useGzip = Boolean(params.get('gzip'));
}
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/config/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ const defaultSettings = {
blockedUrlPatterns: null,
additionalTraceCategories: null,
extraHeaders: null,
extraCookies: null,
precomputedLanternData: null,
onlyAudits: null,
onlyCategories: null,
Expand Down
13 changes: 13 additions & 0 deletions lighthouse-core/gather/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -1487,6 +1487,19 @@ class Driver {
return this.sendCommand('Network.setExtraHTTPHeaders', {headers});
}

/**
* @param {LH.Crdp.Network.Cookies|null} cookies key/value pairs of HTTP Cookies.
* @return {Promise<void>}
*/
async setCookies(cookies) {
if (!cookies) {
return;
}

return this.sendCommand('Network.setCookies', {cookies});
Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAIK when the domain/url/path isn't set it will just use the existing target, because we're running this before we've navigated to the page I'm guessing the common case won't actually send the headers

Copy link
Member

Choose a reason for hiding this comment

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

good catch.

Are we going to entirely defer to the cookie definition for when they're included, e.g. for redirects to different origins, third-party iframes, etc? That format will definitely need to be documented, and it seems like it's going to be a huge pain trying to debug when something goes wrong, but I guess maybe folks are already used to that with other tools doing similar things?

What does WebPageTest do for setting cookies?

Copy link
Author

Choose a reason for hiding this comment

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

As far as I remember it is executed on "about:blank" page. This is why it is not really set anything, if not specifying either domain or URL. This another +1 for the bigger JSON structure.

Copy link
Author

Choose a reason for hiding this comment

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

Are we going to entirely defer to the cookie definition for when they're included, e.g. for redirects to different origins, third-party iframes, etc? That format will definitely need to be documented, and it seems like it's going to be a huge pain trying to debug when something goes wrong, but I guess maybe folks are already used to that with other tools doing similar things?

What does WebPageTest do for setting cookies?

I would assume we should rely on browser behavior for the session. A cookie is just a can-opener to test page usability under the specific conditions:

  • authorize access to content
  • allow access to CDN images
  • local show locale-specific content
  • perform sticky session
  • canary routing for CI

I don't think LH is intended to use like E2E test which might need any specific cookie handling between the tests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@RynatSibahatau WDYT about asserting that we have either a url or domain on all the cookies here?

}


/**
* @param {string} url
* @return {Promise<void>}
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/gather/gather-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,7 @@ class GatherRunner {
// neccessary at the beginning of the next pass.
await passContext.driver.blockUrlPatterns(blockedUrls);
await passContext.driver.setExtraHTTPHeaders(passContext.settings.extraHeaders);
await passContext.driver.setCookies(passContext.settings.extraCookies);

log.timeEnd(status);
}
Expand Down
29 changes: 28 additions & 1 deletion lighthouse-core/test/gather/driver-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -412,14 +412,41 @@ describe('.setExtraHTTPHeaders', () => {
);
});

it('should Network.setExtraHTTPHeaders when there are extra-headers', async () => {
it('should not Network.setExtraHTTPHeaders when there aren\'t extra-headers', async () => {
connectionStub.sendCommand = createMockSendCommandFn();
await driver.setExtraHTTPHeaders();

expect(connectionStub.sendCommand).not.toHaveBeenCalled();
});
});

describe('.setCookies', () => {
it('should call Network.setCookies when there are extra-cookies', async () => {
connectionStub.sendCommand = createMockSendCommandFn().mockResponse(
'Network.setCookies',
{}
);

await driver.setCookies([{
'name': 'cookie1',
'value': 'monster',
}]);

expect(connectionStub.sendCommand).toHaveBeenCalledWith(
'Network.setCookies',
expect.anything()
);
});

it('should not call Network.setCookies when there aren\'t extra-cookies', async () => {
connectionStub.sendCommand = createMockSendCommandFn();
await driver.setCookies();

expect(connectionStub.sendCommand).not.toHaveBeenCalled();
});
});


describe('.getAppManifest', () => {
it('should return null when no manifest', async () => {
connectionStub.sendCommand = createMockSendCommandFn().mockResponse(
Expand Down
3 changes: 3 additions & 0 deletions lighthouse-core/test/gather/fake-driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,9 @@ function makeFakeDriver({protocolGetVersionResponse}) {
setExtraHTTPHeaders() {
return Promise.resolve();
},
setCookies() {
return Promise.resolve();
},
};
}

Expand Down
28 changes: 27 additions & 1 deletion lighthouse-core/test/gather/gather-runner-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ const fakeDriver = require('./fake-driver.js');
const fakeDriverUsingRealMobileDevice = fakeDriver.fakeDriverUsingRealMobileDevice;

function getMockedEmulationDriver(emulationFn, netThrottleFn, cpuThrottleFn,
blockUrlFn, extraHeadersFn) {
blockUrlFn, extraHeadersFn, extraCookiesFn) {
const Driver = require('../../gather/driver.js');
const Connection = require('../../gather/connections/connection.js');
const EmulationDriver = class extends Driver {
Expand Down Expand Up @@ -81,6 +81,9 @@ function getMockedEmulationDriver(emulationFn, netThrottleFn, cpuThrottleFn,
case 'Network.setExtraHTTPHeaders':
fn = extraHeadersFn;
break;
case 'Network.setCookies':
fn = extraCookiesFn;
break;
default:
fn = null;
break;
Expand Down Expand Up @@ -393,6 +396,7 @@ describe('GatherRunner', function() {
setThrottling: asyncFunc,
blockUrlPatterns: asyncFunc,
setExtraHTTPHeaders: asyncFunc,
setCookies: asyncFunc,
endTrace: asyncFunc,
endDevtoolsLog: () => [],
getBrowserVersion: async () => ({userAgent: ''}),
Expand Down Expand Up @@ -578,6 +582,28 @@ describe('GatherRunner', function() {
));
});

it('tells the driver to set additional cookies when extraCookies flag is given', () => {
let receivedCookies = null;
const driver = getMockedEmulationDriver(null, null, null, null, null, params => {
receivedCookies = params.cookies;
});
const cookies = [{
'name': 'cookie1',
'value': 'monster',
}];

return GatherRunner.setupPassNetwork({
driver,
settings: {
extraCookies: cookies,
},
passConfig: {gatherers: []},
}).then(() => assert.deepStrictEqual(
receivedCookies,
cookies
));
});

it('tells the driver to begin tracing', async () => {
let calledTrace = false;
const driver = {
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/test/results/artifacts/artifacts.json
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
"blockedUrlPatterns": null,
"additionalTraceCategories": null,
"extraHeaders": null,
"extraCookies": null,
"precomputedLanternData": null,
"onlyAudits": null,
"onlyCategories": null,
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/test/results/sample_v2.json
Original file line number Diff line number Diff line change
Expand Up @@ -3327,6 +3327,7 @@
"blockedUrlPatterns": null,
"additionalTraceCategories": null,
"extraHeaders": null,
"extraCookies": null,
"precomputedLanternData": null,
"onlyAudits": null,
"onlyCategories": null,
Expand Down
22 changes: 12 additions & 10 deletions readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -101,18 +101,20 @@ Options:
--throttling.uploadThroughputKbps Controls emulated network upload throughput
--throttling.cpuSlowdownMultiplier Controls simulated + emulated CPU throttling
--extra-headers Set extra HTTP Headers to pass with request [string]
--extra-cookies Set extra HTTP Cookies to pass with request [string]

Examples:
lighthouse <url> --view Opens the HTML report in a browser after the run completes
lighthouse <url> --config-path=./myconfig.js Runs Lighthouse with your own configuration: custom audits, report
generation, etc.
lighthouse <url> --output=json --output-path=./report.json --save-assets Save trace, devtoolslog, and named JSON report.
lighthouse <url> --emulated-form-factor=none Disable device emulation and all throttling.
--throttling-method=provided
lighthouse <url> --chrome-flags="--window-size=412,660" Launch Chrome with a specific window size
lighthouse <url> --quiet --chrome-flags="--headless" Launch Headless Chrome, turn off logging
lighthouse <url> --extra-headers "{\"Cookie\":\"monster=blue\"}" Stringify\'d JSON HTTP Header key/value pairs to send in requests
lighthouse <url> --extra-headers=./path/to/file.json Path to JSON file of HTTP Header key/value pairs to send in requests
lighthouse <url> --view Opens the HTML report in a browser after the run completes
lighthouse <url> --config-path=./myconfig.js Runs Lighthouse with your own configuration: custom audits, report generation, etc.
lighthouse <url> --output=json --output-path=./report.json --save-assets Save trace, screenshots, and named JSON report.
lighthouse <url> --emulated-form-factor=none --throttling-method=provided Disable device emulation and all throttling
lighthouse <url> --chrome-flags="--window-size=412,660" Launch Chrome with a specific window size
lighthouse <url> --quiet --chrome-flags="--headless" Launch Headless Chrome, turn off logging
lighthouse <url> --extra-headers "{\"x-men\":\"wolverine\"}" Stringify'd JSON HTTP Header key/value pairs to send in requests
lighthouse <url> --extra-headers=./path/to/file.json Path to JSON file of HTTP Header key/value pairs to send in requests
lighthouse <url> --extra-cookies "[{\"name\":\"session_id\",\"value\":\"x-men\" }]" Stringify'd JSON array of HTTP Cookies to send in requests
lighthouse <url> --extra-cookies=./path/to/file.json Path to JSON file of HTTP Cookies to send in requests
lighthouse <url> --only-categories=performance,pwa Only run specific categories.
brendankenny marked this conversation as resolved.
Show resolved Hide resolved

For more information on Lighthouse, see https://developers.google.com/web/tools/lighthouse/.
```
Expand Down