-
Notifications
You must be signed in to change notification settings - Fork 615
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
CONSOLE-4430: Content Security Policy E2E testing with Puppeteer & Chrome #14675
base: master
Are you sure you want to change the base?
Changes from all commits
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 |
---|---|---|
@@ -1,3 +1,4 @@ | ||
.puppeteer | ||
.yarn | ||
__coverage__ | ||
**/node_modules | ||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,148 @@ | ||||||||||
/* eslint-env node */ | ||||||||||
/* eslint-disable no-console */ | ||||||||||
|
||||||||||
import * as fs from 'fs'; | ||||||||||
import * as path from 'path'; | ||||||||||
import { | ||||||||||
Browser as BrowserType, | ||||||||||
detectBrowserPlatform, | ||||||||||
getInstalledBrowsers, | ||||||||||
install, | ||||||||||
resolveBuildId, | ||||||||||
} from '@puppeteer/browsers'; | ||||||||||
import { Browser, Page, launch } from 'puppeteer-core'; | ||||||||||
|
||||||||||
// Use 'Chrome for Testing' build of the Chrome web browser. | ||||||||||
// https://googlechromelabs.github.io/chrome-for-testing/ | ||||||||||
const testBrowser = BrowserType.CHROME; | ||||||||||
const testBrowserTag = 'stable'; | ||||||||||
|
||||||||||
// The 'NO_SANDBOX' env. variable can be used to run Chrome under the root user. | ||||||||||
// https://chromium.googlesource.com/chromium/src/+/HEAD/docs/design/sandbox.md | ||||||||||
const noSandbox = process.env.NO_SANDBOX === 'true'; | ||||||||||
|
||||||||||
const baseDir = path.resolve(__dirname, '.puppeteer'); | ||||||||||
const cacheDir = path.resolve(baseDir, 'cache'); | ||||||||||
const userDataDir = path.resolve(baseDir, 'user-data'); | ||||||||||
|
||||||||||
const findInstalledBrowser = async () => { | ||||||||||
const allBrowsers = await getInstalledBrowsers({ cacheDir }); | ||||||||||
return allBrowsers.find((b) => b.browser === testBrowser); | ||||||||||
}; | ||||||||||
|
||||||||||
const initBrowserInstance = async () => { | ||||||||||
let browser = await findInstalledBrowser(); | ||||||||||
|
||||||||||
if (!browser) { | ||||||||||
console.info(`Browser ${testBrowser} not found, installing...`); | ||||||||||
|
||||||||||
browser = await install({ | ||||||||||
browser: testBrowser, | ||||||||||
buildId: await resolveBuildId(testBrowser, detectBrowserPlatform(), testBrowserTag), | ||||||||||
cacheDir, | ||||||||||
}); | ||||||||||
} | ||||||||||
|
||||||||||
console.info( | ||||||||||
`Using browser ${browser.browser} on ${browser.platform} with build ID ${browser.buildId}`, | ||||||||||
); | ||||||||||
|
||||||||||
fs.rmSync(userDataDir, { recursive: true, force: true }); | ||||||||||
|
||||||||||
return launch({ | ||||||||||
headless: true, | ||||||||||
browser: testBrowser, | ||||||||||
executablePath: browser.executablePath, | ||||||||||
userDataDir, | ||||||||||
args: noSandbox ? ['--no-sandbox'] : [], | ||||||||||
}); | ||||||||||
}; | ||||||||||
|
||||||||||
const testPage = async ( | ||||||||||
browser: Browser, | ||||||||||
pageURL: string, | ||||||||||
cspReportURL: string, | ||||||||||
pageLoadCallback: (page: Page) => Promise<void>, | ||||||||||
errorCallback: VoidFunction, | ||||||||||
) => { | ||||||||||
const page = await browser.newPage(); | ||||||||||
|
||||||||||
// Create a Chrome DevTools Protocol session for the page. | ||||||||||
const cdpSession = await page.createCDPSession(); | ||||||||||
|
||||||||||
// This will trigger Fetch.requestPaused events for the matching requests. | ||||||||||
await cdpSession.send('Fetch.enable', { | ||||||||||
patterns: [{ resourceType: 'Document' }, { resourceType: 'CSPViolationReport' }], | ||||||||||
}); | ||||||||||
|
||||||||||
// Handle network requests that get paused through Fetch.enable command. | ||||||||||
cdpSession.on('Fetch.requestPaused', ({ resourceType, request, requestId }) => { | ||||||||||
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. Lets add more comments explaining non-obvious logic, especially for the Chrome DevTools Protocol configuration (Fetch.enable, Fetch.requestPaused). 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. OK will do. |
||||||||||
if (resourceType === 'Document' && request.url === pageURL) { | ||||||||||
const headers = Object.entries(request.headers).map(([name, value]) => ({ name, value })); | ||||||||||
|
||||||||||
headers.push({ name: 'Test-CSP-Reporting-Endpoint', value: cspReportURL }); | ||||||||||
cdpSession.send('Fetch.continueRequest', { requestId, 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.
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 have purposely omitted the CDP session emitted events are async in nature. This line is the last bit of code of an if (resourceType === 'Document' && request.url === pageURL) {
const headers = Object.entries(request.headers).map(([name, value]) => ({ name, value }));
headers.push({ name: 'Test-CSP-Reporting-Endpoint', value: cspReportURL });
cdpSession.send('Fetch.continueRequest', { requestId, headers });
} It makes very little sense to We simply call the command and let it run. We don't need to handle its result, nor do we need to wait for its completion. |
||||||||||
} | ||||||||||
|
||||||||||
if (resourceType === 'CSPViolationReport' && request.url === cspReportURL) { | ||||||||||
console.error('CSP violation detected', request.postData); | ||||||||||
errorCallback(); | ||||||||||
|
||||||||||
cdpSession.send('Fetch.fulfillRequest', { requestId, responseCode: 200 }); | ||||||||||
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. This is effectively the same thing as #14675 (comment) |
||||||||||
} | ||||||||||
}); | ||||||||||
|
||||||||||
console.info(`Loading page ${pageURL}`); | ||||||||||
|
||||||||||
const httpResponse = await page.goto(pageURL); | ||||||||||
|
||||||||||
if (httpResponse.ok()) { | ||||||||||
try { | ||||||||||
await pageLoadCallback(page); | ||||||||||
} catch (e) { | ||||||||||
console.error(e); | ||||||||||
errorCallback(); | ||||||||||
} | ||||||||||
} else { | ||||||||||
console.error(`Non-OK response: status ${httpResponse.status()} ${httpResponse.statusText()}`); | ||||||||||
errorCallback(); | ||||||||||
} | ||||||||||
|
||||||||||
await cdpSession.detach(); | ||||||||||
await page.close(); | ||||||||||
}; | ||||||||||
|
||||||||||
const waitForNetworkIdle = async (page: Page) => { | ||||||||||
await page.waitForNetworkIdle({ idleTime: 2000 }); | ||||||||||
}; | ||||||||||
|
||||||||||
(async () => { | ||||||||||
let errorsDetected = false; | ||||||||||
|
||||||||||
const cspReportURL = 'http://localhost:7777/'; | ||||||||||
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. Avoid hardcoding URLs like http://localhost:9000/ or http://localhost:7777/. 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 change the code to work with env. variables that use localhost fallbacks. The CI job could pass these env. variables, while developers running What do you think? |
||||||||||
const browser = await initBrowserInstance(); | ||||||||||
|
||||||||||
const errorCallback = () => { | ||||||||||
errorsDetected = true; | ||||||||||
}; | ||||||||||
|
||||||||||
await testPage( | ||||||||||
browser, | ||||||||||
'http://localhost:9000/dashboards', | ||||||||||
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. should this param become eventually an array ? 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. It was originally an array of URLs that contains just this one URL. When we come to a point where we want to test multiple pages, we can make it into an array. The The reason for a singular |
||||||||||
cspReportURL, | ||||||||||
waitForNetworkIdle, | ||||||||||
errorCallback, | ||||||||||
); | ||||||||||
|
||||||||||
await browser.close(); | ||||||||||
|
||||||||||
if (errorsDetected) { | ||||||||||
process.exit(1); | ||||||||||
} else { | ||||||||||
console.info('No errors detected'); | ||||||||||
process.exit(0); | ||||||||||
} | ||||||||||
})().catch((e) => { | ||||||||||
console.error(e); | ||||||||||
process.exit(1); | ||||||||||
}); | ||||||||||
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. lets move closing the browser into
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. Top-level Node.js async code should be wrapped in an immediately-invoked async function expression (IIAFE). Are you suggesting we should wrap the whole IIAFE body into a try/catch block? (async () => {
try {
// ...
} catch (e) {
// ...
}
})(); |
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 see that the change is actually not changing the logic, so not sure why the block got moved?
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.
The block got moved so that
OPENSHIFT_CI
env. variable handling is located together.Technically, this is init/config
while this is validation
and I think init/config should logically come before validation.