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
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>
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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
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)
Comment |
![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
| const onDataChanged = (secretsData) => { | ||
| setStringData({ ...secretsData?.stringData }); | ||
| setBase64StringData({ ...secretsData?.base64StringData }); | ||
| // Preserve binary values by merging them with form data | ||
| // This prevents corruption of binary data when editing other fields | ||
| const mergedData = Object.entries(binaryData).reduce( | ||
| (acc, [key, value]) => { | ||
| // If the binary entry still exists in the form data (same key), preserve its value | ||
| if (acc[key]) { | ||
| return { ...acc, [key]: value }; | ||
| } | ||
| return acc; | ||
| }, | ||
| { ...secretsData?.base64StringData }, | ||
| ); | ||
| setBase64StringData(mergedData); |
There was a problem hiding this comment.
Avoid clobbering edited binary entries when merging.
The current merge overwrites any key present in secretsData.base64StringData, which means a user’s updated binary upload (or a conversion to text) will be replaced by the original binary value. That makes binary updates impossible and can lose user edits. Backfill only missing keys instead.
🔧 Suggested fix (only backfill missing keys)
- const mergedData = Object.entries(binaryData).reduce(
- (acc, [key, value]) => {
- // If the binary entry still exists in the form data (same key), preserve its value
- if (acc[key]) {
- return { ...acc, [key]: value };
- }
- return acc;
- },
- { ...secretsData?.base64StringData },
- );
+ const mergedData = { ...(secretsData?.base64StringData ?? {}) };
+ for (const [key, value] of Object.entries(binaryData)) {
+ if (!Object.prototype.hasOwnProperty.call(mergedData, key)) {
+ mergedData[key] = value;
+ }
+ }
setBase64StringData(mergedData);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const onDataChanged = (secretsData) => { | |
| setStringData({ ...secretsData?.stringData }); | |
| setBase64StringData({ ...secretsData?.base64StringData }); | |
| // Preserve binary values by merging them with form data | |
| // This prevents corruption of binary data when editing other fields | |
| const mergedData = Object.entries(binaryData).reduce( | |
| (acc, [key, value]) => { | |
| // If the binary entry still exists in the form data (same key), preserve its value | |
| if (acc[key]) { | |
| return { ...acc, [key]: value }; | |
| } | |
| return acc; | |
| }, | |
| { ...secretsData?.base64StringData }, | |
| ); | |
| setBase64StringData(mergedData); | |
| const onDataChanged = (secretsData) => { | |
| setStringData({ ...secretsData?.stringData }); | |
| // Preserve binary values by merging them with form data | |
| // This prevents corruption of binary data when editing other fields | |
| const mergedData = { ...(secretsData?.base64StringData ?? {}) }; | |
| for (const [key, value] of Object.entries(binaryData)) { | |
| if (!Object.prototype.hasOwnProperty.call(mergedData, key)) { | |
| mergedData[key] = value; | |
| } | |
| } | |
| setBase64StringData(mergedData); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/secrets/create-secret/SecretFormWrapper.tsx`
around lines 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.
|
/retest timed out |
|
@TheRealJon: 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
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