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 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
148 changes: 148 additions & 0 deletions frontend/test-puppeteer-csp.ts
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 }) => {
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.

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 });
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.

}

if (resourceType === 'CSPViolationReport' && request.url === cspReportURL) {
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}`);

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/';
Copy link
Member

Choose a reason for hiding this comment

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

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.

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 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?

const browser = await initBrowserInstance();

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

await testPage(
browser,
'http://localhost:9000/dashboards',
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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);
});
Copy link
Member

Choose a reason for hiding this comment

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

lets move closing the browser into finally block

Suggested change
});
finally {
await browser.close();
}

Copy link
Contributor Author

@vojtechszocs vojtechszocs Jan 14, 2025

Choose a reason for hiding this comment

The 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) {
    // ...
  }
})();

Loading