OCPBUGS-70273: Prevent binary secret data corruption when editing#16053
OCPBUGS-70273: Prevent binary secret data corruption when editing#16053TheRealJon wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
jira/severity-important
|
@TheRealJon: This pull request references Jira Issue OCPBUGS-70273, 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. |
openshift-ci
Bot
added
component/core
📝 WalkthroughWalkthroughThis change addresses a bug where editing text fields in mixed secrets corrupted binary data. The fix introduces a new 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/packages/integration-tests-cypress/tests/crud/secrets/key-value.cy.ts (1)
161-215: Make mixed-secret cleanup resilient to mid-test failures.Cleanup happens only at the end of the test body, so any earlier failure leaves the mixed secret behind. Consider moving that deletion into
afterEachalongside other secret cleanup.♻️ Suggested refactor
- const tlsSecretName = `key-value-tls-secret-${testName}`; + const tlsSecretName = `key-value-tls-secret-${testName}`; + const mixedSecretName = `key-value-mixed-secret-${testName}`; @@ - cy.exec( - `oc delete secret -n ${testName} ${binarySecretName} ${asciiSecretName} ${unicodeSecretName}`, + cy.exec( + `oc delete secret -n ${testName} ${binarySecretName} ${asciiSecretName} ${unicodeSecretName} ${mixedSecretName}`, { failOnNonZeroExit: false, }, ); @@ - const mixedSecretName = `key-value-mixed-secret-${testName}`; @@ - // Cleanup - cy.exec(`oc delete secret -n ${testName} ${mixedSecretName}`, { - failOnNonZeroExit: false, - });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/integration-tests-cypress/tests/crud/secrets/key-value.cy.ts` around lines 161 - 215, The test currently only deletes mixedSecretName at the end of the test body, leaving the secret if the test fails mid-run; fix by declaring mixedSecretName (and any related keys) in the outer scope (e.g., let mixedSecretName = ``) so it is accessible to hooks, assign it inside the it(...) block, and add an afterEach hook that runs cy.exec(`oc delete secret -n ${testName} ${mixedSecretName}`, { failOnNonZeroExit: false }) to reliably clean up the secret regardless of test outcome; ensure the afterEach uses the same mixedSecretName symbol and keeps failOnNonZeroExit: false for idempotent deletion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/public/components/secrets/create-secret/SecretFormWrapper.tsx`:
- Around line 87-101: The merge in onDataChanged currently overwrites edited
binary entries by replacing keys present in secretsData?.base64StringData with
values from binaryData; change the merge so it only backfills missing keys:
iterate binaryData entries and for each [key,value] if acc[key] is undefined (or
not present) then set acc[key]=value, otherwise keep the existing acc[key]; then
call setBase64StringData with that merged result. Reference onDataChanged,
setStringData, binaryData, secretsData?.base64StringData, mergedData, and
setBase64StringData to locate and update the logic.
---
Nitpick comments:
In
`@frontend/packages/integration-tests-cypress/tests/crud/secrets/key-value.cy.ts`:
- Around line 161-215: The test currently only deletes mixedSecretName at the
end of the test body, leaving the secret if the test fails mid-run; fix by
declaring mixedSecretName (and any related keys) in the outer scope (e.g., let
mixedSecretName = ``) so it is accessible to hooks, assign it inside the it(...)
block, and add an afterEach hook that runs cy.exec(`oc delete secret -n
${testName} ${mixedSecretName}`, { failOnNonZeroExit: false }) to reliably clean
up the secret regardless of test outcome; ensure the afterEach uses the same
mixedSecretName symbol and keeps failOnNonZeroExit: false for idempotent
deletion.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
frontend/packages/integration-tests-cypress/tests/crud/secrets/key-value.cy.tsfrontend/public/components/secrets/create-secret/OpaqueSecretFormEntry.tsxfrontend/public/components/secrets/create-secret/SecretFormWrapper.tsxfrontend/public/components/utils/file-input.tsx
|
/retest timed out |
|
/retest |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@TheRealJon: This pull request references Jira Issue OCPBUGS-70273, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
jhadvig
left a comment
jhadvig
left a comment
There was a problem hiding this comment.
/lgtm
/cherry-pick release-4.21
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhadvig, TheRealJon 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 |
TheRealJon
force-pushed
the
OCPBUGS-70273
branch
from
dbbd532 to
035701c
Compare
|
New changes are detected. LGTM label has been removed. |
TheRealJon
force-pushed
the
OCPBUGS-70273
branch
from
035701c to
d87be41
Compare
|
@TheRealJon: This pull request references Jira Issue OCPBUGS-70273, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
/retest |
1 similar comment
|
/retest |
When editing secrets containing binary data (JAR, JCEKS, etc), the binary values were being corrupted by decoding base64 as UTF-8 text, which inserts replacement characters for non-printable bytes. Fixed by: - Pass base64 data directly to DroppableFileInput for binary entries - Add isBase64Input prop to properly handle base64-encoded input - Preserve original binary values when form updates to prevent corruption - Use useMemo instead of useState for derived binaryData value - Use immutable reducer pattern to merge binary data Added Cypress test to verify editing text fields doesn't corrupt binary data in the same secret. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
TheRealJon
force-pushed
the
OCPBUGS-70273
branch
from
d87be41 to
f0dafd5
Compare
|
@TheRealJon: all tests passed! 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. |
4 participants
Summary
Changes
DroppableFileInputfor binary entries instead of decoding as UTF-8isBase64Inputprop to handle base64-encoded input properlyuseMemofor derivedbinaryDatavalue instead ofuseStateTest plan
yarn run test-cypress-headless --spec packages/integration-tests-cypress/tests/crud/secrets/key-value.cy.ts🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests