Skip to content

Conversation

@acrylJonny
Copy link
Collaborator

The escaping was unnecessary because GraphQL already handles proper deserialization of the input value. Adding JSON escaping at this stage doubles-escapes the value, corrupting it.

Solution

  • Removed the StringEscapeUtils.escapeJson() call from UpdateSecretResolver
  • Removed the unused import org.apache.commons.text.StringEscapeUtils
  • Both createSecret and updateSecret now use the same approach: directly encrypt the input value

Testing

  • Verified that secrets with newlines, forward slashes, and other special characters can now be updated without corruption
  • Confirmed consistency between create and update operations

Impact

  • Secrets with special characters can now be updated correctly
  • Existing corrupted secrets will need to be updated again to fix their values

@github-actions github-actions bot added the product PR or Issue related to the DataHub UI/UX label Nov 28, 2025
@datahub-cyborg datahub-cyborg bot added the needs-review Label for PRs that need review from a maintainer. label Nov 28, 2025
@codecov
Copy link

codecov bot commented Nov 28, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

@sgomezvillamor
Copy link
Contributor

While this looks to me, it concerns me that the escaping was added about 3 months ago in PR #14578

In order to unblock and clarify, what about adding a proper smoke test that:

  • adds a fake bigquery credential private key as secret and
  • reads ok
  • updates it
  • reads ok

WDYT?

@datahub-cyborg datahub-cyborg bot added pending-submitter-response Issue/request has been reviewed but requires a response from the submitter and removed needs-review Label for PRs that need review from a maintainer. labels Nov 29, 2025
@acrylJonny
Copy link
Collaborator Author

While this looks to me, it concerns me that the escaping was added about 3 months ago in PR #14578

In order to unblock and clarify, what about adding a proper smoke test that:

  • adds a fake bigquery credential private key as secret and
  • reads ok
  • updates it
  • reads ok

WDYT?

Yeah, I think that makes sense. It seems strange that the previous change was made only for updates. Let me see if there's something I can alter to still cater for the special characters like doubly/single quotes but I'll apply it across the create and update commands.

But yeah, I think your proposal of that smoke test makes sense. I'll look to add that.

@datahub-cyborg datahub-cyborg bot added needs-review Label for PRs that need review from a maintainer. and removed pending-submitter-response Issue/request has been reviewed but requires a response from the submitter labels Nov 29, 2025
@acrylJonny
Copy link
Collaborator Author

While this looks to me, it concerns me that the escaping was added about 3 months ago in PR #14578
In order to unblock and clarify, what about adding a proper smoke test that:

  • adds a fake bigquery credential private key as secret and
  • reads ok
  • updates it
  • reads ok

WDYT?

Yeah, I think that makes sense. It seems strange that the previous change was made only for updates. Let me see if there's something I can alter to still cater for the special characters like doubly/single quotes but I'll apply it across the create and update commands.

But yeah, I think your proposal of that smoke test makes sense. I'll look to add that.

@sgomezvillamor - smoke test has now been added

@datahub-cyborg datahub-cyborg bot added pending-submitter-merge and removed needs-review Label for PRs that need review from a maintainer. labels Dec 3, 2025
@acrylJonny acrylJonny merged commit 8bea2bd into master Dec 3, 2025
44 checks passed
@acrylJonny acrylJonny deleted the update-secret-bug branch December 3, 2025 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pending-submitter-merge product PR or Issue related to the DataHub UI/UX

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants