-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Changes from 1 commit
421d262
1ea7f7d
72c9272
f4e0a49
8de0262
2e1365b
1677a37
074eee1
005bd8b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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 | ||||||||||||||||||||||||||||||
// 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'); | ||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 It would be nice to not include it by default and then consider adding it in the future if there's demand There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
cliFlags.extraCookies = JSON.parse(extraCookiesStr); | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @patrickhulce Could you take a look 074eee1? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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} */ | ||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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-headers/valid.json'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. seems like you mean to use the cookies one instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed by f4e0a49 |
||
// @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'); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
NotJSON |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
[ | ||
{ | ||
"name":"test", | ||
"value":"true", | ||
"url":"http://localhost" | ||
} | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -105,6 +105,20 @@ function requestHandler(request, response) { | |
} | ||
} | ||
|
||
if (params.has('extra_cookie')) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. any reason to keep the changes to this file? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
isn't that going to be kind of hard, @patrickhulce? What would we be testing, exactly? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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')); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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:
I don't think LH is intended to use like E2E test which might need any specific cookie handling between the tests. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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>} | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, definitely, this shouldn't be done a second time :) The TODO even lists how to fix:
There was a problem hiding this comment.
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 🤢There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 andextraCookies
for regular? (or we nix theextra
prefix entirely I think was your last suggestion?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bump on this @brendankenny
There was a problem hiding this comment.
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 theCliFlags
interface) andextraCookies
for the setting (onSharedFlagsSettings
). Or--cookies-string
andcookies
.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 :)
There was a problem hiding this comment.
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! 👍