Skip to content

fix(release): Validate GCP GetSecret json format#5336

Merged
Skarlso merged 4 commits intoexternal-secrets:mainfrom
Gabrielmadrid73:fix/gcp-json-parse
Sep 27, 2025
Merged

fix(release): Validate GCP GetSecret json format#5336
Skarlso merged 4 commits intoexternal-secrets:mainfrom
Gabrielmadrid73:fix/gcp-json-parse

Conversation

@Gabrielmadrid73
Copy link
Copy Markdown
Contributor

@Gabrielmadrid73 Gabrielmadrid73 commented Sep 19, 2025

Problem Statement

The current method allows user to use a GCP secret manager with a malformed json content. It happens because there's no validation and it can cause problems in the future.

Related Issue

Fixes #4336

Proposed Changes

Add a json validation using the official lib encoding/json to GCP secrets. This change will prevent malformed stored json's to the secrets.

Checklist

  • [ x] I have read the contribution guidelines
  • [ x] All commits are signed with git commit --signoff
  • My changes have reasonable test coverage
  • All tests pass with make test
  • I ensured my PR is ready for review with make reviewable

@github-actions github-actions bot added area/release Issues / Pull Requests related to releases kind/bug Categorizes issue or PR as related to a bug. size/s labels Sep 19, 2025
Skarlso
Skarlso reviewed Sep 20, 2025
View reviewed changes
pkg/provider/gcp/secretmanager/client.go
}

func getDataByProperty(data []byte, property string) gjson.Result {
func getDataByProperty(data []byte, property string) (gjson.Result, error) {
Copy link
Copy Markdown
Contributor

@Skarlso Skarlso Sep 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some test for the new behaviour.

Copy link
Copy Markdown
Contributor Author

@Gabrielmadrid73 Gabrielmadrid73 Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I added the test but it's kind of my first test, so it might be wrong.

Copy link
Copy Markdown
Contributor

@Skarlso Skarlso Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done! :) I'll check it out! :)

@Skarlso
Copy link
Copy Markdown
Contributor

Skarlso commented Sep 27, 2025

@Gabrielmadrid73 Ah can you sign your commit please?

@Gabrielmadrid73
chore/json-parser - Updated GCP GetSecret to validate json format
1f89400 
Signed-off-by: Gabriel Madrid <gabrielmadrid73@gmail.com>
@Gabrielmadrid73
fix/gcp-json-parser - Updated GCP needUpdate according getDataByPrope…
c9598df 
…rty changes

Signed-off-by: Gabriel Madrid <gabrielmadrid73@gmail.com>
@Gabrielmadrid73
fix/gcp-json-parse - Added test to GCP client getDataByProperty
07b4aac 
Signed-off-by: Gabriel Madrid <gabrielmadrid73@gmail.com>
@Gabrielmadrid73
Copy link
Copy Markdown
Contributor Author

Gabrielmadrid73 commented Sep 27, 2025

@Skarlso mb, executed the DCO indicated commands:

git rebase HEAD~3 --signoff
git push --force-with-lease origin fix/gcp-json-parse

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Sep 27, 2025

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@Skarlso Skarlso merged commit 2bc24e9 into external-secrets:main Sep 27, 2025
8 checks passed
@hpedrorodrigues
Copy link
Copy Markdown

hpedrorodrigues commented Oct 7, 2025

Hey 👋🏻
First of all, thanks for fixing this!

Even though this is not so critical, it'd be good to release this in a minor version rather than a patch.

Just found out I had a few "broken" secrets after naively applying this patch. I read the changelog but missed this change.

I'm not sure it's worth changing the release since I think I'm the only one mentioning this, but I'm sharing anyway.

@aaaaahaaaaa aaaaahaaaaa mentioned this pull request Oct 8, 2025
Closed
SamuelMolling pushed a commit to SamuelMolling/external-secrets that referenced this pull request Oct 24, 2025
@Gabrielmadrid73 @Skarlso @SamuelMolling
fix(release): Validate GCP GetSecret json format (external-secrets#5336)
6c5980c 
* chore/json-parser - Updated GCP GetSecret to validate json format

Signed-off-by: Gabriel Madrid <gabrielmadrid73@gmail.com>

* fix/gcp-json-parser - Updated GCP needUpdate according getDataByProperty changes

Signed-off-by: Gabriel Madrid <gabrielmadrid73@gmail.com>

* fix/gcp-json-parse - Added test to GCP client getDataByProperty

Signed-off-by: Gabriel Madrid <gabrielmadrid73@gmail.com>

---------

Signed-off-by: Gabriel Madrid <gabrielmadrid73@gmail.com>
Co-authored-by: Gergely Brautigam <skarlso777@gmail.com>
Signed-off-by: Samuel Molling <samuelmolling@gmail.com>
@vshn-renovate vshn-renovate mentioned this pull request Nov 21, 2025
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Skarlso Skarlso Skarlso approved these changes

Assignees

No one assigned

Labels

area/release Issues / Pull Requests related to releases kind/bug Categorizes issue or PR as related to a bug. size/s

Projects

External Secrets
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

GCP Provider- GetSecret method successfully parses broken JSON - it should fail.

3 participants

@Gabrielmadrid73 @Skarlso @hpedrorodrigues