CONSOLE-4430: Content Security Policy E2E testing with Puppeteer & Chrome #14675
Conversation
vojtechszocs
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@vojtechszocs: This pull request references CONSOLE-4430 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vojtechszocs The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
vojtechszocs
force-pushed
the
csp-e2e-testing-with-puppeteer
branch
from
42563ab to
4f210a5
Compare
vojtechszocs
force-pushed
the
csp-e2e-testing-with-puppeteer
branch
from
4f210a5 to
aa52aeb
Compare
|
/retest |
|
@vojtechszocs: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
There was a problem hiding this comment.
Thanks @vojtechszocs for the PR. Adding couple of comments.
| # Disable color codes in Cypress since they do not render well CI test logs. | ||
| # https://docs.cypress.io/guides/guides/continuous-integration.html#Colors | ||
| if [ "$OPENSHIFT_CI" = true ]; then | ||
| export NO_COLOR=1 | ||
| fi | ||
|
|
||
| if [ "$(basename "$(pwd)")" != "frontend" ]; then |
There was a problem hiding this comment.
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.
The block got moved so that OPENSHIFT_CI env. variable handling is located together.
Technically, this is init/config
if [ "$OPENSHIFT_CI" = true ]; then
export NO_COLOR=1
fiwhile this is validation
if [ "$(basename "$(pwd)")" != "frontend" ]; then
echo "This script must be run from the frontend folder"
exit 1
fiand I think init/config should logically come before validation.
| }); | ||
|
|
||
| // Handle network requests that get paused through Fetch.enable command. | ||
| cdpSession.on('Fetch.requestPaused', ({ resourceType, request, requestId }) => { |
There was a problem hiding this comment.
Lets add more comments explaining non-obvious logic, especially for the Chrome DevTools Protocol configuration (Fetch.enable, Fetch.requestPaused).
| 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.
| cdpSession.send('Fetch.continueRequest', { requestId, headers }); | |
| await cdpSession.send('Fetch.continueRequest', { requestId, headers }); |
There was a problem hiding this comment.
I have purposely omitted the await operator here.
CDP session emitted events are async in nature. This line is the last bit of code of an if block:
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 await the result of Fetch.continueRequest command here.
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.
| console.error('CSP violation detected', request.postData); | ||
| errorCallback(); | ||
|
|
||
| cdpSession.send('Fetch.fulfillRequest', { requestId, responseCode: 200 }); |
There was a problem hiding this comment.
| cdpSession.send('Fetch.fulfillRequest', { requestId, responseCode: 200 }); | |
| await cdpSession.send('Fetch.fulfillRequest', { requestId, responseCode: 200 }); |
There was a problem hiding this comment.
This is effectively the same thing as #14675 (comment)
|
|
||
| await testPage( | ||
| browser, | ||
| 'http://localhost:9000/dashboards', |
There was a problem hiding this comment.
should this param become eventually an array ?
asking so we keep in mind that we reuse the Browser's instance, without launching a new browser instance every time
There was a problem hiding this comment.
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 browser instance should be reused for all testPage function calls, as you mentioned.
The reason for a singular testPage function call is that different pages may need a custom pageLoadCallback beyond the standard waitForNetworkIdle - for example, when testing the dashboard page, wait for "Cluster API address" to be displayed. This is not currently implemented, we have discussed that this could be done as a follow-up to this PR.
| (async () => { | ||
| let errorsDetected = false; | ||
|
|
||
| const cspReportURL = 'http://localhost:7777/'; |
There was a problem hiding this comment.
Avoid hardcoding URLs like http://localhost:9000/ or http://localhost:7777/.
Instead I would pass them as parameters or retrieve them from configuration/environment variables.
There was a problem hiding this comment.
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 yarn test-puppeteer-csp could use fallbacks.
What do you think?
| })().catch((e) => { | ||
| console.error(e); | ||
| process.exit(1); | ||
| }); |
There was a problem hiding this comment.
lets move closing the browser into finally block
| }); | |
| finally { | |
| await browser.close(); | |
| } |
There was a problem hiding this comment.
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) {
// ...
}
})();
This PR adds a script to allow automated testing of Console application for CSP violations.
Console currently implements CSP via Content-Security-Policy-Report-Only HTTP response header, reporting any CSP violations detected in production env. via SecurityPolicyViolationEvent handler to telemetry service.
Summary of changes
frontend/.puppeteerdirectory. Chrome browser is installed only when necessary, i.e. when runningyarn test-puppeteer-cspscript, so the typicalyarn installdependency update flow is not affected.yarn test-puppeteer-cspinfrontenddirectory to launch the CSP test script. This script assumes that Console Bridge server is already running locally and will test http://localhost:9000/dashboards page for CSP violations using Chrome DevTools Protocol (CDP).General test flow
resourceType=Document). Modify the request by adding customTest-CSP-Reporting-Endpointheader, instructing the server to use the given CSP reporting endpoint for testing purposes and then resume the modified request.resourceType=CSPViolationReport). Upon receiving such request, report the error and then fulfill the request with 200 status code (i.e. avoid the need to run a server that actually handles the requests for CSP violation reports).As a follow-up, we should add a CI job that invokes
test-csp.shas the entry point.