OCPBUGS-81616, OCPBUGS-79458, OCPBUGS-74469, OCPBUGS-70280, OCPBUGS-67244: fix CVEs by updating vulnerable dependencies#185
Conversation
openshift-ci-robot
added
jira/severity-important
|
@upalatucci: This pull request references Jira Issue OCPBUGS-81616, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: upalatucci 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
WalkthroughUpdated dev dependency version ranges and dependency ordering in Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
upalatucci
changed the title
|
@upalatucci: This pull request references Jira Issue OCPBUGS-81616, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. This pull request references Jira Issue OCPBUGS-79458, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. This pull request references Jira Issue OCPBUGS-74469, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. This pull request references Jira Issue OCPBUGS-70280, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. This pull request references Jira Issue OCPBUGS-67244, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@upalatucci: This pull request references Jira Issue OCPBUGS-81616, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
This pull request references Jira Issue OCPBUGS-79458, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
This pull request references Jira Issue OCPBUGS-74469, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
This pull request references Jira Issue OCPBUGS-70280, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
This pull request references Jira Issue OCPBUGS-67244, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Around line 71-72: The package.json currently mixes Jest 28 with
jest-environment-jsdom@30 which will break installs; update the Jest toolchain
so versions are consistent by bumping "jest", "ts-jest", and "@types/jest" to
the matching v30 range (and keep "jest-environment-jsdom" at v30), then
reinstall dependencies and run the test suite to confirm; update any
Jest/ts-jest config if required for v30 compatibility (look for "jest",
"ts-jest", "@types/jest", and "jest-environment-jsdom" in package.json).
- Around line 97-99: The package.json currently downgrades "mocha" to "^7.2.0"
in dependencies which reintroduces a ReDoS vulnerability; update the "mocha"
entry to "^10.1.0" (or later), move it from "dependencies" into
"devDependencies", and then regenerate the lockfile by running your package
manager (npm install / yarn install) so the lockfile reflects the safe version;
ensure you update the "mocha" key in package.json (not any other package) and
commit both package.json and the updated lockfile.
- Line 59: Cypress v15 requires Node 20+ which conflicts with the project's
current Node 18 baseline (indicated by `@types/node`: ^18.0.0 and CI/dev setups);
resolve by either (A) upgrading runtime and dev environments to Node 20+ (update
CI images, developer setup docs, and bump `@types/node` to ^20) and then keep
"cypress": "^15.13.1", or (B) pinning Cypress to a Node-18-compatible release
(e.g., revert "cypress" in package.json to the latest v12/v14 stable semver) and
ensure package.json and lockfile reflect that; update whichever files reference
"cypress" and "@types/node" and run npm install + CI to verify.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb6e62a8-2a10-4c04-8955-24f4dd751737
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
package.json
| "copy-webpack-plugin": "^14.0.0", | ||
| "css-loader": "^6.7.1", | ||
| "cypress": "^12.17.4", | ||
| "cypress": "^15.13.1", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n package.json | head -70Repository: openshift/nmstate-console-plugin
Length of output: 3636
🏁 Script executed:
fd -type f -name '.nvmrc' -o -name '.node-version' -o -name '.ruby-version'Repository: openshift/nmstate-console-plugin
Length of output: 246
🏁 Script executed:
fd -type f -name '*.yml' -o -name '*.yaml' | grep -E '(workflow|gitlab|circle)' | head -20Repository: openshift/nmstate-console-plugin
Length of output: 246
🏁 Script executed:
find . -name '.github' -type d -exec find {} -name '*.yml' \; 2>/dev/null | head -10Repository: openshift/nmstate-console-plugin
Length of output: 58
🏁 Script executed:
cat package.json | grep -A 5 '"engines"'Repository: openshift/nmstate-console-plugin
Length of output: 58
🏁 Script executed:
find . -path './.git' -prune -o -name '*.yml' -print | grep -v node_modules | head -15Repository: openshift/nmstate-console-plugin
Length of output: 109
🏁 Script executed:
find . -path './.git' -prune -o -type f \( -name '*.yml' -o -name '*.yaml' \) -print | grep -E '(workflow|github|gitlab|circle)' | head -20Repository: openshift/nmstate-console-plugin
Length of output: 58
🏁 Script executed:
ls -la .github/workflows/ 2>/dev/null || echo "No .github/workflows directory"Repository: openshift/nmstate-console-plugin
Length of output: 106
🏁 Script executed:
cat -n package.json | grep -E '(cypress|node|engines)'Repository: openshift/nmstate-console-plugin
Length of output: 709
🏁 Script executed:
tail -20 package.jsonRepository: openshift/nmstate-console-plugin
Length of output: 598
🏁 Script executed:
find . -path './.git' -prune -o -type f -name 'Makefile' -o -name '.travis.yml' -o -name '.circleci' -printRepository: openshift/nmstate-console-plugin
Length of output: 58
🏁 Script executed:
ls -la | grep -E '(Makefile|\.travis|\.circleci|tox\.ini|setup\.cfg)'Repository: openshift/nmstate-console-plugin
Length of output: 58
🌐 Web query:
Cypress 12 Node.js version requirements minimum
💡 Result:
Cypress 12.x requires Node.js 14 or newer to install the cypress npm package (Node.js 12 is not supported for installation). [1]
Source: Cypress docs “Migrating to Cypress 12.0” → “Node.js 14+ support”. [1]
🌐 Web query:
Cypress 15 Node.js version requirements minimum breaking changes
💡 Result:
Cypress 15 Node.js version requirements (installing Cypress)
- Supported Node.js versions: Node.js 20.x, 22.x, and 24+
- No longer supported: Node.js 18.x and 23.x
(Source: Cypress “Migrating to Cypress 15.0” migration guide) [1]
Minimum breaking changes to expect in Cypress 15 (high-signal)
From the Cypress 15 migration guide:
cy.exec()result property renamed:code→exitCode[1]- Linux compatibility: binaries require glibc ≥ 2.31 (older distros won’t work) [1]
- Webpack 4 dropped: Webpack 4 is no longer supported (move to Webpack 5 / supported alternatives) [1]
- Preprocessor change:
@cypress/webpack-batteries-included-preprocessorno longer shims all webpack v4 built-ins (you may need to add fallbacks) [1] - Angular component testing: Angular 17 CT no longer supported; minimum Angular 18.0.0 for CT [1]
- API rename:
Cypress.SelectorPlayground→Cypress.ElementSelector(andonElementremoved fromdefaults) [1]
Also called out in Cypress’ upgrade-path blog post for 14 → 15:
cy.stub()deprecation removal: the 3-argument form is no longer supported; usecallsFake()/returns()etc. [2]
Sources: [1] [2]
Cypress 15 requires Node.js 20+, incompatible with project's Node 18 baseline
Upgrading from Cypress v12 to v15 introduces a breaking Node.js requirement: v12 supported Node 14+, but v15 requires Node 20.x, 22.x, or 24+ and no longer supports Node 18.x. The project's @types/node: ^18.0.0 indicates Node 18 compatibility, making this upgrade incompatible. Contributors and CI environments on Node 18 will fail during npm install and test execution. Either upgrade to Node 20+ across all environments or reconsider this Cypress version.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 59, Cypress v15 requires Node 20+ which conflicts with
the project's current Node 18 baseline (indicated by `@types/node`: ^18.0.0 and
CI/dev setups); resolve by either (A) upgrading runtime and dev environments to
Node 20+ (update CI images, developer setup docs, and bump `@types/node` to ^20)
and then keep "cypress": "^15.13.1", or (B) pinning Cypress to a
Node-18-compatible release (e.g., revert "cypress" in package.json to the latest
v12/v14 stable semver) and ensure package.json and lockfile reflect that; update
whichever files reference "cypress" and "@types/node" and run npm install + CI
to verify.
| "jest": "^28.1.3", | ||
| "jest-environment-jsdom": "^28.1.3", | ||
| "jest-environment-jsdom": "^30.3.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Declared versions:"
jq -r '{
jest: .devDependencies.jest,
jest_environment_jsdom: .devDependencies["jest-environment-jsdom"],
ts_jest: .devDependencies["ts-jest"],
types_jest: .devDependencies["@types/jest"]
}' package.json
echo
echo "Registry metadata (peerDependencies/engines):"
for pkg in "jest-environment-jsdom@30.3.0" "jest@28.1.3" "ts-jest@28.0.7"; do
name="${pkg%@*}"
ver="${pkg#*@}"
echo "== $name@$ver =="
curl -s "https://registry.npmjs.org/$name/$ver" | jq '{name,version,peerDependencies,engines}'
doneRepository: openshift/nmstate-console-plugin
Length of output: 1829
🌐 Web query:
jest-environment-jsdom 30.3.0 peerDependencies jest
💡 Result:
jest-environment-jsdom@30.3.0 does not declare jest as a peerDependency. Its peerDependencies are for canvas only (optional), and it pulls Jest internals via regular dependencies like @jest/environment / @jest/environment-jsdom-abstract (same 30.x line). [1]
So, if you’re seeing an npm “peer dependency” issue involving jest, it’s coming from some other package in your tree—not from jest-environment-jsdom itself. [1]
Sources
[1] https://app.unpkg.com/jest-environment-jsdom%4030.1.1/files/package.json
🌐 Web query:
jest-environment-jsdom 30 jest 28 compatibility
💡 Result:
jest-environment-jsdom@30.x is not meant to be used with jest@28.x.
jest-environment-jsdom@30.1.0depends on Jest 30 internal packages (e.g.@jest/environment@30.1.0,@jest/environment-jsdom-abstract@30.1.0). Mixing that withjest@28typically leads to duplicate/mismatched@jest/*packages and runtime/type issues. It also targets newer Node versions via itsenginesfield. [1]- Jest’s own guidance is to upgrade Jest as a set (the Jest 30 upgrade docs discuss changes around
jest-environment-jsdomas part of the Jest 30 upgrade). [2]
What to do
- If you’re staying on Jest 28, use
jest-environment-jsdom@28.x(match majors). - If you want
jest-environment-jsdom@30, upgrade tojest@30as well.
Sources: jest-environment-jsdom@30.1.0 package metadata (deps/engines) [1], Jest “Upgrading to Jest 30” docs [2].
Jest stack majors are inconsistent and will cause runtime failures
Line 72 upgrades jest-environment-jsdom to v30 while keeping jest, ts-jest, and @types/jest on v28. The jest-environment-jsdom@30.x line depends on internal Jest 30 packages (@jest/environment@30.x, etc.), which will create duplicate and mismatched dependencies when used with Jest 28. This breaks test runtime and install resolution. Upgrade the entire Jest toolchain to v30.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 71 - 72, The package.json currently mixes Jest 28
with jest-environment-jsdom@30 which will break installs; update the Jest
toolchain so versions are consistent by bumping "jest", "ts-jest", and
"@types/jest" to the matching v30 range (and keep "jest-environment-jsdom" at
v30), then reinstall dependencies and run the test suite to confirm; update any
Jest/ts-jest config if required for v30 compatibility (look for "jest",
"ts-jest", "@types/jest", and "jest-environment-jsdom" in package.json).
| "@preact/signals-react": "^3.7.0", | ||
| "esbuild-loader": "^4.4.2", | ||
| "mocha": "^7.2.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd "$(git rev-parse --show-toplevel)" && git log --oneline -10 -- package.jsonRepository: openshift/nmstate-console-plugin
Length of output: 1904
🏁 Script executed:
git show HEAD:package.json | grep -A 2 -B 2 '"mocha"'Repository: openshift/nmstate-console-plugin
Length of output: 200
🏁 Script executed:
git diff HEAD~1 HEAD -- package.json | grep -A 3 -B 3 mochaRepository: openshift/nmstate-console-plugin
Length of output: 104
🏁 Script executed:
git log --all --oneline --follow -- package.json | head -20Repository: openshift/nmstate-console-plugin
Length of output: 1079
🏁 Script executed:
git log --all --pretty=format:"%h %s" -- package.json | head -20Repository: openshift/nmstate-console-plugin
Length of output: 1079
🏁 Script executed:
ls -la | grep -E "package-lock|yarn.lock|pnpm-lock"Repository: openshift/nmstate-console-plugin
Length of output: 145
🏁 Script executed:
cat -n package.json | head -110 | tail -20Repository: openshift/nmstate-console-plugin
Length of output: 738
🏁 Script executed:
git show d38fee2:package.json | grep -A 2 -B 2 '"mocha"'Repository: openshift/nmstate-console-plugin
Length of output: 230
🏁 Script executed:
grep -A 5 '"mocha"' package-lock.json | head -20Repository: openshift/nmstate-console-plugin
Length of output: 676
🌐 Web query:
mocha 7.2.0 security vulnerabilities CVE
💡 Result:
For the npm package mocha version 7.2.0, the currently listed direct security issue is a Regular Expression Denial of Service (ReDoS) in Mocha’s clean function (utils.js) affecting all mocha versions < 10.1.0—which includes 7.2.0. No CVE has been assigned for this issue (“CVE NOT AVAILABLE”); it is tracked as Snyk ID SNYK-JS-MOCHA-2863123. Fix: upgrade to mocha >= 10.1.0. [1] [2]
🌐 Web query:
mocha npm security advisories version 7.2.0 vs 10.x
💡 Result:
- Direct (Mocha package) advisories: The GitLab Advisory Database lists a Mocha ReDoS issue affecting all versions
< 6.0.0, fixed in6.0.0—so Mocha7.2.0and10.xare not in the affected range. [1] - What you’re likely seeing in
npm audit: Mocha maintainers note that most “Mocha vulnerabilities” reported bynpm audit/advisories are usually about transitive dependencies and may not be practically exploitable in typical Mocha usage; they track reports in their “Security Dashboard” issue. [2] - “10.x vs 7.2.0” in practice: Moving from
7.2.0→10.xgenerally helps by pulling in newer dependency versions, which often reducesnpm auditfindings, but that’s about dependency advisories—not a known Mocha-7.2.0vs Mocha-10.xdirect advisory difference. [2][3]
Sources: [1] GitLab Advisory Database (mocha / GMS-2019-7) • [2] Mocha security-vulnerability reports explainer • [3] Snyk vulnerability page for mocha
Mocha downgrade reintroduces ReDoS vulnerability in CVE-remediation PR
Line 99 downgrades mocha from ^10.x to ^7.2.0. This is a security regression: Mocha versions before 10.1.0 contain a Regular Expression Denial of Service (ReDoS) vulnerability (SNYK-JS-MOCHA-2863123). Downgrading in a CVE-remediation PR negates the security objective. Upgrade to ^10.1.0 or later and regenerate the lockfile accordingly.
Additionally, mocha should be in devDependencies, not dependencies, since it is a test framework.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 97 - 99, The package.json currently downgrades
"mocha" to "^7.2.0" in dependencies which reintroduces a ReDoS vulnerability;
update the "mocha" entry to "^10.1.0" (or later), move it from "dependencies"
into "devDependencies", and then regenerate the lockfile by running your package
manager (npm install / yarn install) so the lockfile reflects the safe version;
ensure you update the "mocha" key in package.json (not any other package) and
commit both package.json and the updated lockfile.
|
These coderabbit comments seem important, especially the mocha one. Have you looked at them? |
|
In openshift/networking-console-plugin#359 you downgrade Cypress from v15 to v12 because of prow incompatibility. Is that not an issue here? |
|
@pcbailey i upgraded cypress from 12 to 15 to solve CVEs and incompatibility with node. I noticed that on main we still have the v12 cypress version so i upgraded here . So we have 4.21 and main alligned |
|
/lgtm |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
package.json (2)
71-72:⚠️ Potential issue | 🔴 CriticalJest major versions are inconsistent and can break test runtime.
Line 72 upgrades
jest-environment-jsdomto^30.3.0while Line 71 keepsjestat^28.1.3(ts-jestis also 28.x). Mixed Jest majors are a high-risk test/runtime incompatibility.Verification plan: inspect declared Jest stack versions and package metadata for major coupling.
#!/bin/bash set -euo pipefail echo "Declared Jest stack in package.json:" jq -r '.devDependencies | { jest, "jest-environment-jsdom": ."jest-environment-jsdom", "ts-jest": ."ts-jest", "@types/jest": ."@types/jest" }' package.json echo for pkg in jest-environment-jsdom@30.3.0 jest@28.1.3 ts-jest@28.0.7; do name="${pkg%@*}" ver="${pkg#*@}" echo "== $name@$ver ==" curl -s "https://registry.npmjs.org/$name/$ver" \ | jq '{name, version, engines, peerDependencies, dependencies}' echo doneAs per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 71 - 72, The devDependency versions for Jest are mismatched: "jest" is ^28.1.3 while "jest-environment-jsdom" is ^30.3.0 (and "ts-jest" is 28.x); update package.json to align Jest major versions by either downgrading "jest-environment-jsdom" to a 28.x compatible release or upgrading "jest" and "ts-jest" to 30.x, then run install and the verification script in the review comment to confirm "jest", "jest-environment-jsdom", and "ts-jest" share the same major version.
59-59:⚠️ Potential issue | 🟠 MajorCypress 15 may break installs/tests if CI or dev runtime is still Node 18.
Line 59 upgrades Cypress to
^15.13.1; Cypress 15 requires newer Node than v12 did. This PR should confirm runtime parity (local + CI/prow) before merge, otherwisenpm install/e2e can fail.Verification plan: confirm declared/runtime Node versions in repo and compare with Cypress package engines.
#!/bin/bash set -euo pipefail echo "Declared versions from package.json:" jq -r '{ cypress: .devDependencies.cypress, types_node: .devDependencies["@types/node"], engines_node: (.engines.node // "not-set") }' package.json echo echo "Cypress registry engines:" curl -s https://registry.npmjs.org/cypress/15.13.1 | jq '{name, version, engines}' echo echo "Node/runtime hints in repository config files:" rg -n -C2 --glob '!node_modules/**' --glob '!package-lock.json' \ 'setup-node|node-version|NODE_VERSION|FROM node:|\.nvmrc|\.node-version|cypress'As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 59, The repo upgraded the devDependency "cypress" to ^15.13.1 which may require a newer Node runtime than CI/dev are using; verify the runtime parity by checking package.json.engines.node and any Node hints (.nvmrc, .node-version, workflow steps using setup-node or node-version) against the Cypress package engines, and then either (A) update the repo's declared engines.node and CI workflow (setup-node/node-version entries) to the minimum Node required by Cypress 15, or (B) if you cannot bump runtimes, pin "cypress" to a compatible older version; ensure changes reference the package.json "cypress" entry, the "engines.node" field, and any CI workflow steps that set Node versions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@package.json`:
- Around line 71-72: The devDependency versions for Jest are mismatched: "jest"
is ^28.1.3 while "jest-environment-jsdom" is ^30.3.0 (and "ts-jest" is 28.x);
update package.json to align Jest major versions by either downgrading
"jest-environment-jsdom" to a 28.x compatible release or upgrading "jest" and
"ts-jest" to 30.x, then run install and the verification script in the review
comment to confirm "jest", "jest-environment-jsdom", and "ts-jest" share the
same major version.
- Line 59: The repo upgraded the devDependency "cypress" to ^15.13.1 which may
require a newer Node runtime than CI/dev are using; verify the runtime parity by
checking package.json.engines.node and any Node hints (.nvmrc, .node-version,
workflow steps using setup-node or node-version) against the Cypress package
engines, and then either (A) update the repo's declared engines.node and CI
workflow (setup-node/node-version entries) to the minimum Node required by
Cypress 15, or (B) if you cannot bump runtimes, pin "cypress" to a compatible
older version; ensure changes reference the package.json "cypress" entry, the
"engines.node" field, and any CI workflow steps that set Node versions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24d57ba6-ce8a-409a-9064-135657892b6e
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
package.json
|
/jira refresh The requirements for Jira bugs have changed (Jira issues linked to PRs on main branch need to target different OCP), recalculating validity. |
|
@openshift-bot: This pull request references Jira Issue OCPBUGS-81616, which is valid. 3 validation(s) were run on this bug
This pull request references Jira Issue OCPBUGS-79458, which is valid. 3 validation(s) were run on this bug
This pull request references Jira Issue OCPBUGS-74469, which is valid. 3 validation(s) were run on this bug
This pull request references Jira Issue OCPBUGS-70280, which is valid. 3 validation(s) were run on this bug
This pull request references Jira Issue OCPBUGS-67244, which is valid. 3 validation(s) were run on this bug
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. |
Wrap login form interactions in cy.origin() when redirected to oauth-openshift and refactor login helper to reduce duplication. Made-with: Cursor
|
New changes are detected. LGTM label has been removed. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cypress/support/commands.ts (1)
125-126: Replace the 20s fixed wait with a post-login signal.Line 125 hard-codes a 20-second delay into every login, but it still does not guarantee that the redirect finished before
closeTourPopupIfPresent()runs. The Cypress best-practice documentation recommends avoiding arbitrary waits entirely. Instead, use one of:
- Route aliases:
cy.intercept().as('login')followed bycy.wait('@login')to wait for the actual API response- Assertions:
cy.get('selector').should('be.visible')to wait for a post-login element- Programmatic login: Use
cy.session(),cy.request(), orcy.origin()to bypass UI login and avoid waits altogetherThis approach is faster and more reliable in CI than arbitrary delays.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/support/commands.ts` around lines 125 - 126, Replace the fixed cy.wait(20000) before closeTourPopupIfPresent() with a deterministic post-login signal: remove the hard-coded wait in the login flow and instead set up an intercept() and alias for the login request (use cy.intercept(...).as('login') and cy.wait('@login')), or wait for a stable post-login UI element with cy.get('your-post-login-selector').should('be.visible'), or convert to a programmatic login using cy.session()/cy.request()/cy.origin() to avoid UI waits; update the code around the existing cy.wait call and the closeTourPopupIfPresent() invocation so the popup is closed only after the chosen signal completes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cypress/support/commands.ts`:
- Around line 81-87: The origin check currently reads isOauthOrigin from the
pre-clear URL, causing incorrect branching when a console session exists; move
the clearing of the 'openshift-session-token' cookie to run before the origin
check (i.e., call cy.clearCookie('openshift-session-token') before obtaining
cy.url() / constructing the URL) or, after clearing, re-run cy.url() (or
cy.visit) so isOauthOrigin is computed against the unauthenticated page; update
the logic inside the login() command that computes isOauthOrigin and the branch
that fills the fallback form (the code referencing isOauthOrigin and the
'openshift-session-token' cookie) so the branch decision always reflects the
post-clear state.
- Around line 62-83: The auth check currently calls cy.window() before
determining the page origin, which breaks when redirected to oauth-openshift;
change the flow to call cy.url() first (use cy.url() / URL parsing to detect
oauth-openshift vs console origin), then only invoke cy.window() to read
SERVER_FLAGS?.authDisabled and set authMode when still on the console origin;
otherwise skip to the cy.origin() block for cross-origin login handling (update
references to authMode, cy.window(), cy.url(), and cy.origin() accordingly).
---
Nitpick comments:
In `@cypress/support/commands.ts`:
- Around line 125-126: Replace the fixed cy.wait(20000) before
closeTourPopupIfPresent() with a deterministic post-login signal: remove the
hard-coded wait in the login flow and instead set up an intercept() and alias
for the login request (use cy.intercept(...).as('login') and cy.wait('@login')),
or wait for a stable post-login UI element with
cy.get('your-post-login-selector').should('be.visible'), or convert to a
programmatic login using cy.session()/cy.request()/cy.origin() to avoid UI
waits; update the code around the existing cy.wait call and the
closeTourPopupIfPresent() invocation so the popup is closed only after the
chosen signal completes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 252d1fef-dba0-4cdb-8479-3cd69e5ddd8c
📒 Files selected for processing (2)
cypress.config.tscypress/support/commands.ts
✅ Files skipped from review due to trivial changes (1)
- cypress.config.ts
| cy.visit('/'); // visits baseUrl | ||
|
|
||
| cy.visit(''); // visits baseUrl which is set in plugins.js | ||
| // If auth is disabled (local dev), skip the login flow entirely. | ||
| cy.window().then((win: ConsoleWindowType) => { | ||
| if (win.SERVER_FLAGS?.authDisabled) { | ||
| cy.log('skipping login, console is running with auth disabled'); | ||
|
|
||
| cy.contains('li[data-test="nav"]', 'Networking').click(); | ||
| cy.contains( | ||
| '*[data-test-id="nodenetworkconfigurationpolicy-nav-item"]', | ||
| 'NodeNetworkConfigurationPolicy', | ||
| ).should('be.visible'); | ||
| return; | ||
| return 'authDisabled' as const; | ||
| } | ||
|
|
||
| cy.clearCookie('openshift-session-token'); | ||
|
|
||
| const idp = provider || KUBEADMIN_IDP; | ||
|
|
||
| cy.get('main form').should('be.visible'); | ||
|
|
||
| cy.get('body').then(($body) => { | ||
| if ($body.text().includes(idp)) { | ||
| cy.contains(idp).should('be.visible').click(); | ||
| return 'authEnabled' as const; | ||
| }).then((authMode) => { | ||
| if (authMode === 'authDisabled') return; | ||
|
|
||
| // OpenShift redirects unauthenticated users to oauth-openshift.apps... (different origin than | ||
| // console-openshift-console...). Cypress 12+ requires cy.origin() for commands on that page. | ||
| cy.url().then((currentUrl) => { | ||
| const url = new URL(currentUrl); | ||
| const isOauthOrigin = url.hostname.includes('oauth-openshift'); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Cypress 15, after cy.visit('/') redirects to a different origin, is cy.window() allowed before entering cy.origin()? Please cite the official cross-origin testing documentation.
💡 Result:
No, in Cypress 15, after cy.visit('/') redirects to a different origin, cy.window is not allowed before entering cy.origin. Cypress requires that all commands interacting with the application after navigating to a different origin within the same test must be wrapped in cy.origin for the new origin. Attempting cy.window (or similar window-bound commands like cy.document) outside of cy.origin after the cross-origin redirect will result in a cross-origin error or command timeout, as Cypress enforces the same-origin policy strictly. The official cross-origin testing documentation states: "With the exception of cy.origin, Cypress requires that the URLs navigated to have the same origin for the entirety of a single test. If you attempt to visit two different origins, the cy.origin command must be used to wrap Cypress commands of the second visited domain. Otherwise, Cypress commands will timeout after the navigation and will eventually error." Examples include JavaScript redirects (window.location.href), links, or form submissions leading to different origins, all requiring cy.origin for subsequent commands. This behavior was strengthened in v14.0.0 (no default document.domain injection) and continues in v15. While Cypress 15 includes changes to use automation clients for some window-bound commands (e.g., cy.url, cy.title) to subvert cross-origin boundaries in certain cases (PR #31862), cy.window remains bound to the window object and subject to origin checks, requiring cy.origin for cross-origin contexts.
Citations:
- 1: https://docs.cypress.io/app/guides/cross-origin-testing
- 2: https://on.cypress.io/origin
- 3: https://github.com/cypress-io/cypress-documentation/tree/main/docs/app/guides/cross-origin-testing.mdx
- 4: https://docs.cypress.io/guides/guides/cross-origin-testing
- 5: https://docs.cypress.io/api/commands/origin
- 6: https://on.cypress.io/window
- 7: https://docs.cypress.io/api/commands/window.html
- 8: feat: rework window bound commands to use automation clients cypress-io/cypress#31862
- 9: Feat: certain Cypress commands should not be bound to
windowin order to avoid cross origin issues cypress-io/cypress#31196
Move the cy.window() auth check behind the origin guard.
Line 65 calls cy.window() immediately after cy.visit('/'), but in the auth-enabled path, this redirects to oauth-openshift.apps... (a different origin). Cypress enforces a strict same-origin policy: all commands after a cross-origin redirect must be wrapped in cy.origin(), or they will timeout/error. The current code violates this requirement before ever reaching the cy.origin() block at line 87. Detect the current origin first using cy.url() (which is automation-client-backed in Cypress 15+), and only call cy.window() when still on the console origin.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cypress/support/commands.ts` around lines 62 - 83, The auth check currently
calls cy.window() before determining the page origin, which breaks when
redirected to oauth-openshift; change the flow to call cy.url() first (use
cy.url() / URL parsing to detect oauth-openshift vs console origin), then only
invoke cy.window() to read SERVER_FLAGS?.authDisabled and set authMode when
still on the console origin; otherwise skip to the cy.origin() block for
cross-origin login handling (update references to authMode, cy.window(),
cy.url(), and cy.origin() accordingly).
| cy.url().then((currentUrl) => { | ||
| const url = new URL(currentUrl); | ||
| const isOauthOrigin = url.hostname.includes('oauth-openshift'); | ||
|
|
||
| cy.clearCookie('openshift-session-token'); | ||
|
|
||
| if (isOauthOrigin) { |
There was a problem hiding this comment.
Clear the session before deciding which login path to use.
Lines 81-87 derive isOauthOrigin from the pre-clear URL and only then delete openshift-session-token. If login() starts while a console session is already active, isOauthOrigin stays false and the fallback branch at Lines 115-122 will try to fill a login form that is not on the page. Clear auth state before the initial visit, or revisit after clearing, so the origin check runs against the unauthenticated page.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cypress/support/commands.ts` around lines 81 - 87, The origin check currently
reads isOauthOrigin from the pre-clear URL, causing incorrect branching when a
console session exists; move the clearing of the 'openshift-session-token'
cookie to run before the origin check (i.e., call
cy.clearCookie('openshift-session-token') before obtaining cy.url() /
constructing the URL) or, after clearing, re-run cy.url() (or cy.visit) so
isOauthOrigin is computed against the unauthenticated page; update the logic
inside the login() command that computes isOauthOrigin and the branch that fills
the fallback form (the code referencing isOauthOrigin and the
'openshift-session-token' cookie) so the branch decision always reflects the
post-clear state.
|
@upalatucci: 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. |
openshift-ci
Bot
added
the
needs-rebase
|
PR needs rebase. 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. |
4 participants
Summary
copy-webpack-plugin(12 → 14),cypress(12 → 15),jest-environment-jsdom(28 → 30),mocha(10 → 7.2.0),lodash,qspackage-lock.jsonTest plan
npm installcompletes without errorsnpm auditno longer reports the above CVEsnpm test) and confirm they passSummary by CodeRabbit
Chores
Tests