fix(cypress): wait for RBAC propagation before asserting project access#7381
fix(cypress): wait for RBAC propagation before asserting project access#7381kanishka-commits wants to merge 2 commits intoopendatahub-io:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughFour Cypress files changed. Two E2E test specs now wait for RBAC propagation by invoking a new helper before re-login and UI assertions. A new exported utility Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Actionable Issues
CVE references are not directly applicable to these changes; referenced CWEs above indicate classes of issues to remediate. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/cypress/cypress/utils/oc_commands/project.ts (1)
111-124: Unquoted interpolation into shell command — harden against CWE-78.
projectanduserare spliced straight into thecy.execstring. Today the callers pass UUID-generated project names and env-sourced usernames, so it's not exploitable, but nothing on the function signature prevents a future caller from passing a value containing shell metacharacters (;,`,$(...), spaces). Since this helper sits alongside other project helpers that will likely be reused, a bit of defensive hardening is cheap.🛡️ Suggested defensive hardening
export const waitForUserProjectAccess = ( project: string, user: string, attempts = 15, interval = 2000, -): Cypress.Chainable<Cypress.Exec> => - pollUntilSuccess( - `oc get project ${project} --as=${user} -o name`, - `${user} access to ${project}`, - { - maxAttempts: attempts, - pollIntervalMs: interval, - }, - ); +): Cypress.Chainable<Cypress.Exec> => { + const safe = /^[A-Za-z0-9._@-]+$/; + if (!safe.test(project) || !safe.test(user)) { + throw new Error(`Invalid project or user identifier passed to waitForUserProjectAccess`); + } + return pollUntilSuccess( + `oc get project '${project}' --as='${user}' -o name`, + `${user} access to ${project}`, + { maxAttempts: attempts, pollIntervalMs: interval }, + ); +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cypress/cypress/utils/oc_commands/project.ts` around lines 111 - 124, The command in waitForUserProjectAccess injects unescaped project and user into a shell string (CWE-78); fix by escaping shell metacharacters and wrapping each interpolation in single quotes before passing to pollUntilSuccess (e.g., implement a small helper like escapeShellArg that safely escapes any single quotes in its input and returns the value wrapped in single quotes), then call pollUntilSuccess with the sanitized `'${project}'` and `'${user}'` values in the `oc get project ... --as=...` string so no raw user-controlled characters reach the shell.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/cypress/cypress/utils/oc_commands/project.ts`:
- Around line 111-124: The command in waitForUserProjectAccess injects unescaped
project and user into a shell string (CWE-78); fix by escaping shell
metacharacters and wrapping each interpolation in single quotes before passing
to pollUntilSuccess (e.g., implement a small helper like escapeShellArg that
safely escapes any single quotes in its input and returns the value wrapped in
single quotes), then call pollUntilSuccess with the sanitized `'${project}'` and
`'${user}'` values in the `oc get project ... --as=...` string so no raw
user-controlled characters reach the shell.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1df5c5b4-9279-468d-9a2f-5e7c84ddf953
📒 Files selected for processing (3)
packages/cypress/cypress/tests/e2e/modelTraining/rayJobs/testRayJobProjectAccessPermissions.cy.tspackages/cypress/cypress/tests/e2e/modelTraining/trainJobs/testProjectAccessPermissions.cy.tspackages/cypress/cypress/utils/oc_commands/project.ts
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7381 +/- ##
==========================================
- Coverage 65.04% 63.99% -1.06%
==========================================
Files 2458 2519 +61
Lines 76443 78049 +1606
Branches 19289 19853 +564
==========================================
+ Hits 49726 49948 +222
- Misses 26717 28101 +1384 see 100 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cypress/cypress/utils/oc_commands/distributedWorkloads.ts`:
- Line 71: The command construction used by cy.exec is vulnerable to shell
injection because localQueueName, clusterQueueName, resourceFlavor, and
projectName are interpolated directly; update the code that builds commands (the
cy.exec calls constructing strings like `oc delete LocalQueue ${localQueueName}
-n ${projectName} --wait=false${ignoreFlag}`) to either validate each identifier
against an RFC1123 label regex (e.g., /^[a-z0-9-]+$/) and reject or throw on
mismatch, or stop using direct string interpolation and instead delete by safe
means (use oc delete with --selector or pass a JSON/YAML manifest via stdin or a
file) so untrusted chars never reach a shell; ensure the validation/alternative
logic is applied consistently to localQueueName, clusterQueueName,
resourceFlavor, and projectName before executing cy.exec.
- Around line 70-74: The deleteKueueResources helper builds ocCommand with
--wait=false for all resources, which causes flakiness for cluster-scoped
ClusterQueue and ResourceFlavor when static names are reused; update the
ocCommand construction in distributedWorkloads.ts (the deleteKueueResources call
site building ocCommand) so that ClusterQueue and ResourceFlavor use the default
wait behavior (remove the --wait=false flag or explicitly use --wait=true) or
implement a short poll after deletion to confirm removal before returning;
alternatively, if you prefer fire-and-forget, make callers that use static names
(e.g., testWorkloadMetricsDefaultPageContents.cy.ts using
testData.clusterQueue/testData.resourceFlavour) append a UUID suffix to those
names to guarantee uniqueness.
🪄 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 YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ee723374-f04e-4f98-b172-43322d530ad7
📒 Files selected for processing (1)
packages/cypress/cypress/utils/oc_commands/distributedWorkloads.ts
|
As you have updated the PR can you also update the description? |
|
Ran trainJobs/testProjectAccessPermissions.cy.ts and rayJobs/testRayJobProjectAccessPermissions.cy.ts on RHOAI E2E cluster, all tests green 
|
|
@PR3MM please share id of the jenkins job. TY ! |
|
I tested it locally @ConorOM1 , @kanishka-commits can you please run on jenkins |
3 participants
Description
TrainJob and RayJob access permission tests fail intermittently on first attempt because OpenShift RBAC cache hasn't propagated the new role binding yet. Added
waitForUserProjectAccessutil that polls until the permission is effective before re-logging in.How Has This Been Tested?
Both specs pass with
CY_RETRY=0on a live RHOAI cluster. Lint and type-check pass.Test Impact
Fixes flakiness in existing e2e tests. No new tests needed.
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
mainSummary by CodeRabbit
Tests
Chores