CONSOLE-5094: Add pesudo CSP violation detection to Cypress#16048
CONSOLE-5094: Add pesudo CSP violation detection to Cypress#16048logonoff wants to merge 3 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@logonoff: This pull request references CONSOLE-5094 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 sub-task to target the "4.22.0" version, but no target version was set. DetailsIn 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. |
openshift-ci
bot
added
the
kind/cypress
|
@logonoff: This pull request references CONSOLE-5094 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 sub-task to target the "4.22.0" version, but no target version was set. DetailsIn 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. |
📝 WalkthroughWalkthroughThis PR replaces Puppeteer-based CSP testing with a Cypress-based test. It removes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
frontend/packages/integration-tests-cypress/tests/csp/csp-violations.cy.ts (2)
30-32: Consider explicitreq.continue()for intent clarity.While Cypress auto-continues modified requests when no explicit action is taken, calling
req.continue()makes the intent more obvious to future maintainers—especially when the modification is the sole purpose of the intercept.♻️ Suggested improvement
cy.intercept('GET', '/dashboards*', (req) => { req.headers['Test-CSP-Reporting-Endpoint'] = cspReportURL; + req.continue(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/integration-tests-cypress/tests/csp/csp-violations.cy.ts` around lines 30 - 32, The intercept handler modifies the outgoing request headers but omits an explicit req.continue(), making the intent implicit; update the cy.intercept callback used for 'GET', '/dashboards*' so after setting req.headers['Test-CSP-Reporting-Endpoint'] = cspReportURL you call req.continue() to explicitly forward the modified request (keep the header modification in the same callback and then call req.continue()).
60-71: Hardcoded wait may cause flakiness or unnecessary slowdown.The 5-second wait is a common pattern for async-settled assertions, but it's a tradeoff: too short risks flaky tests if resources are slow; too long slows CI unnecessarily. Since CSP violations fire synchronously when blocked resources are encountered, most violations should already be captured by the time
#contentexists.Consider whether you could reduce this or add a comment documenting what specific deferred scenario requires 5 seconds (e.g., lazy-loaded chunks, dynamic
import()). If the/dashboardspage loads additional resources after the initial render, you might alternatively wait for a specific network idle state or known element that appears after all async work completes.That said, if empirical testing shows 5s is the sweet spot, this is acceptable—just be prepared to revisit if the test becomes flaky.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/integration-tests-cypress/tests/csp/csp-violations.cy.ts` around lines 60 - 71, The hardcoded cy.wait(5000) causes flakiness/slow CI; replace it with a deterministic wait for the condition that guarantees CSP violations have been reported instead of a fixed delay. Modify the test around violations, cy.wait(5000) and the subsequent Cypress.log/expect to either wait for a specific post-load signal (e.g., wait for a known element on the /dashboards page, a network idle condition, or a short retry loop that polls violations until stable) and/or reduce the timeout and add a concise comment documenting why any remaining wait is needed (e.g., lazy-loaded chunks). Ensure the unique symbols violations, cy.wait(5000), Cypress.log and the expect(...).to.have.length(0) assertion are updated to use the new deterministic wait strategy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/packages/integration-tests-cypress/tests/csp/csp-violations.cy.ts`:
- Around line 30-32: The intercept handler modifies the outgoing request headers
but omits an explicit req.continue(), making the intent implicit; update the
cy.intercept callback used for 'GET', '/dashboards*' so after setting
req.headers['Test-CSP-Reporting-Endpoint'] = cspReportURL you call
req.continue() to explicitly forward the modified request (keep the header
modification in the same callback and then call req.continue()).
- Around line 60-71: The hardcoded cy.wait(5000) causes flakiness/slow CI;
replace it with a deterministic wait for the condition that guarantees CSP
violations have been reported instead of a fixed delay. Modify the test around
violations, cy.wait(5000) and the subsequent Cypress.log/expect to either wait
for a specific post-load signal (e.g., wait for a known element on the
/dashboards page, a network idle condition, or a short retry loop that polls
violations until stable) and/or reduce the timeout and add a concise comment
documenting why any remaining wait is needed (e.g., lazy-loaded chunks). Ensure
the unique symbols violations, cy.wait(5000), Cypress.log and the
expect(...).to.have.length(0) assertion are updated to use the new deterministic
wait strategy.
|
ran backend with this command: bash -c "source ./contrib/oc-environment.sh && ./bin/bridge -branding openshift"sad caseapplied this diff: $ git diff
diff --git a/frontend/public/components/dashboard/dashboards-page/dashboards.tsx b/frontend/public/components/dashboard/dashboards-page/dashboards.tsx
index af94463581..8bef47a915 100644
--- a/frontend/public/components/dashboard/dashboards-page/dashboards.tsx
+++ b/frontend/public/components/dashboard/dashboards-page/dashboards.tsx
@@ -1,5 +1,5 @@
import type { FC } from 'react';
-import { useMemo } from 'react';
+import { useEffect, useMemo, useState } from 'react';
import { useLocation } from 'react-router-dom-v5-compat';
import { connect } from 'react-redux';
import { Map as ImmutableMap } from 'immutable';
@@ -65,6 +65,15 @@ export const getPluginTabPages = (
const DashboardsPage_: FC<DashboardsPageProps> = ({ kindsInFlight, k8sModels }) => {
const { t } = useTranslation();
const title = t('public~Overview');
+
+ // CSP violation: fetch from an external domain not allowed by connect-src
+ const [motd, setMotd] = useState('');
+ useEffect(() => {
+ fetch('https://motd.logonoff.co')
+ .then((res) => res.text())
+ .then(setMotd)
+ .catch((e) => setMotd(`CSP blocked: ${e.message}`));
+ }, []);
const tabExtensions = useExtensions<DashboardsTab>(isDashboardsTab);
const cardExtensions = useExtensions<DashboardsCard>(isDashboardsCard);
@@ -103,6 +112,7 @@ const DashboardsPage_: FC<DashboardsPageProps> = ({ kindsInFlight, k8sModels })
<>
<PageTitleContext.Provider value={titleProviderValues}>
<PageHeading title={title} badge={badge} />
+ {motd && <pre data-test="csp-motd">{motd}</pre>}
<HorizontalNav pages={allPages} noStatusBox />
</PageTitleContext.Provider>
</>test failed happy casetest passed |
logonoff
force-pushed
the
CONSOLE-5094-csp-tests
branch
from
38f9cb8 to
19e63d1
Compare
|
/label px-approved |
openshift-ci
bot
added
px-approved
logonoff
force-pushed
the
CONSOLE-5094-csp-tests
branch
2 times, most recently
from
a334694 to
f2bc5d3
Compare
|
@coderabbitai review |
|
@logonoff: This pull request references CONSOLE-5094 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 sub-task to target the "4.22.0" version, but no target version was set. DetailsIn 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. |
✅ Actions performedReview triggered.
|
|
@logonoff: This pull request references CONSOLE-5094 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 sub-task to target the "4.22.0" version, but no target version was set. DetailsIn 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. |
logonoff
force-pushed
the
CONSOLE-5094-csp-tests
branch
from
f2bc5d3 to
77072c1
Compare
logonoff
changed the title
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: logonoff The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
logonoff
changed the title
|
@logonoff: This pull request references CONSOLE-5094 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 sub-task to target the "4.22.0" version, but no target version was set. DetailsIn 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. |
logonoff
force-pushed
the
CONSOLE-5094-csp-tests
branch
from
77072c1 to
b743d47
Compare
openshift-ci
bot
added
component/dev-console
openshift-ci
bot
added
component/olm
|
@logonoff: This pull request references CONSOLE-5094 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 sub-task to target the "4.22.0" version, but no target version was set. DetailsIn 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. |
|
@logonoff: This pull request references CONSOLE-5094 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 sub-task to target the "4.22.0" version, but no target version was set. DetailsIn 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. |
logonoff
force-pushed
the
CONSOLE-5094-csp-tests
branch
from
b743d47 to
779ecc5
Compare
logonoff
changed the title
Co-Authored-By: Vojtech Szocs <vojtech.szocs@gmail.com>
|
@logonoff: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
2 participants
A subset of CSP violations that occur during our e2e testing will now fail the e2e test
We cannot remove
test-puppeteer-cspbecause we can't test the entirety of our CSP using cypress alone. Cypressonly allows a certain few CSP directives to be preserved, the others are stripped.
Cypress maintainers will always filter out some of our CSP directives such as
frame-ancestors, and maintainers will probably never add support for this directive as it's fundamentally incompatible with Cypress testing in aniframe.There may also be CSP violations that are produced prior to this hook being registered