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

CONSOLE-4430: Content Security Policy E2E testing with Puppeteer & Chrome #14675

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 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
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ cypress-a11y-report.json
/gopath
/Godeps/_workspace/src/github.com/openshift/console
/frontend/.cache-loader
/frontend/.puppeteer
/frontend/.webpack-cycles
/frontend/__coverage__
/frontend/__chrome_browser__
Expand Down
1 change: 1 addition & 0 deletions frontend/.eslintignore
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
.puppeteer
.yarn
__coverage__
**/node_modules
Expand Down
10 changes: 5 additions & 5 deletions frontend/integration-tests/test-cypress.sh
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,17 @@ SCREENSHOTS_DIR=gui_test_screenshots
# https://ci-operator-configresolver-ui-ci.apps.ci.l2s4.p1.openshiftapps.com/help#env
OPENSHIFT_CI=${OPENSHIFT_CI:=false}

if [ "$(basename "$(pwd)")" != "frontend" ]; then
echo "This script must be run from the frontend folder"
exit 1
fi

# 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
Copy link
Member

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?

Copy link
Contributor Author

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

if [ "$OPENSHIFT_CI" = true ]; then
  export NO_COLOR=1
fi

while this is validation

if [ "$(basename "$(pwd)")" != "frontend" ]; then
  echo "This script must be run from the frontend folder"
  exit 1
fi

and I think init/config should logically come before validation.

echo "This script must be run from the frontend folder"
exit 1
fi

if [ ! -d node_modules ]; then
yarn install
fi
Expand Down
2 changes: 2 additions & 0 deletions frontend/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
"test-cypress-topology": "cd packages/topology/integration-tests && yarn run test-cypress",
"test-cypress-topology-headless": "cd packages/topology/integration-tests && yarn run test-cypress-headless",
"test-cypress-topology-nightly": "cd packages/topology/integration-tests && yarn run test-cypress-headless-all",
"test-puppeteer-csp": "yarn ts-node ./test-puppeteer-csp.ts",
"cypress-merge": "mochawesome-merge ./gui_test_screenshots/cypress_report*.json > ./gui_test_screenshots/cypress.json",
"cypress-generate": "marge -o ./gui_test_screenshots/ -f cypress-report -t 'OpenShift Console Cypress Test Results' -p 'OpenShift Cypress Test Results' --showPassed false --assetsDir ./gui_test_screenshots/cypress/assets ./gui_test_screenshots/cypress.json",
"cypress-a11y-report": "echo '\nA11y Test Results:' && mv packages/integration-tests-cypress/cypress-a11y-report.json ./gui_test_screenshots/ && node -e \"console.table(JSON.parse(require('fs').readFileSync(process.argv[1])));\" ./gui_test_screenshots/cypress-a11y-report.json",
Expand Down Expand Up @@ -306,6 +307,7 @@
"monaco-editor": "^0.28.1",
"monaco-editor-webpack-plugin": "^4.2.0",
"prettier": "2.0.5",
"puppeteer-core": "^23.9.0",
"react-refresh": "^0.10.0",
"read-pkg": "5.x",
"redux-mock-store": "^1.5.3",
Expand Down
168 changes: 168 additions & 0 deletions frontend/test-puppeteer-csp.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
/* 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';

const envParameters = {
// 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
noSandbox: process.env.NO_SANDBOX === 'true',

// Base URL of Console web application.
consoleBaseURL: process.env.CONSOLE_BASE_URL || 'http://localhost:9000',

// CSP reporting endpoint to be used for testing Console pages.
cspReportURL: process.env.CSP_REPORT_URL || 'http://localhost:7777',
};

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: envParameters.noSandbox ? ['--no-sandbox'] : [],
});
};

/**
* Use `browser` to test `pageURL` for Content Security Policy (CSP) violations.
*/
const testPage = async (
browser: Browser,
pageURL: URL,
cspReportURL: URL,
pageLoadCallback: (page: Page) => Promise<void>,
errorCallback: VoidFunction,
) => {
const page = await browser.newPage();

// Create a Chrome DevTools Protocol (CDP) 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 }) => {
Copy link
Member

Choose a reason for hiding this comment

The 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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK will do.

// When requesting the web page, add custom 'Test-CSP-Reporting-Endpoint' HTTP header
// in order to instruct Console Bridge server to use the given CSP reporting endpoint.
if (resourceType === 'Document' && request.url === pageURL.href) {
const headers = Object.entries(request.headers).map(([name, value]) => ({ name, value }));

headers.push({ name: 'Test-CSP-Reporting-Endpoint', value: cspReportURL.href });
cdpSession.send('Fetch.continueRequest', { requestId, headers });
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cdpSession.send('Fetch.continueRequest', { requestId, headers });
await cdpSession.send('Fetch.continueRequest', { requestId, headers });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, if we just add await here as per your suggestion, it breaks the syntax:

'await' expressions are only allowed within async functions and at the top levels of modules.ts(1308)

If we really wanted to wait for this operation to finish, we'd have to use the standard Promise API functions.

Copy link
Member

Choose a reason for hiding this comment

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

interesting, Im a bit surprised that its working this way, but I probably need read docs in detail.
Thank you.

}

// The browser will attempt to send any CSP violations to the CSP reporting endpoint.
// When such request occurs, we manually fulfill that request before it is sent over
// the network and therefore avoiding the need to implement that reporting endpoint.
if (resourceType === 'CSPViolationReport' && request.url === cspReportURL.href) {
console.error('CSP violation detected', request.postData);
errorCallback();

cdpSession.send('Fetch.fulfillRequest', { requestId, responseCode: 200 });
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cdpSession.send('Fetch.fulfillRequest', { requestId, responseCode: 200 });
await cdpSession.send('Fetch.fulfillRequest', { requestId, responseCode: 200 });

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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}`);

// At this point, CDP session is already set up.
const httpResponse = await page.goto(pageURL.href);

if (httpResponse.ok()) {
try {
// Wait for the page to finish loading.
await pageLoadCallback(page);
} catch (e) {
console.error(e);
errorCallback();
}
} else {
// Treat non-OK HTTP status code as error.
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 () => {
try {
let errorsDetected = false;

const browser = await initBrowserInstance();

const errorCallback = () => {
errorsDetected = true;
};

await testPage(
browser,
new URL('/dashboards', envParameters.consoleBaseURL),
new URL(envParameters.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);
}
})();
Loading