Skip to content

Fix ggshield retrieval with proxy #80

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Apr 30, 2025

Conversation

sevbch
Copy link
Contributor

@sevbch sevbch commented Apr 28, 2025

Context

Leveraging help from #79. (Thank you @GabrielCousin!)

This PR tentatively addresses certificate errors encountered by customers when installing the extension.

Error logs looked like this:

2025-04-23 22:03:04.615 [error] Activating extension gitguardian-secret-security.gitguardian failed due to an error:
2025-04-23 22:03:04.615 [error] Error: unable to get local issuer certificate
...

or

2025-04-22 17:39:09.130 [error] Activating extension gitguardian-secret-security.gitguardian failed due to an error:
2025-04-22 17:39:09.131 [error] Error: self signed certificate in certificate chain
...

What has been done

  • Use axios instead of sync-request lib to make HTTP calls to github API
  • Use param rejectUnauthorized from axios to disable SSL checks when option "allow self-signed certificate" is checked on the extension.
  • Remove getGGShieldLatestVersion as it's not used anymore.
  • Bump prettier version in pre-commit as it was not up-to-date with typescript version in this repo.

Best reviewed commit by commit.

Validation

I tried validating this using mitmproxy. Unfortunately I wasn't able to tweak it to make ggshield-download step fail on SSL certificate grounds.

So I haven't been able to confirm this line will fix encountered issues with proxy.
However, please note that this confirms the fix should work. So I think we can have a good confidence it will work.

Also, I was able to validate locally that changes here don't affect the standard behavior of the extension.

  • checkout this branch
  • check or un-check param "allow self-signed certificate" in the extension (both should work)
  • delete ggshield-internal folder if it exists
  • launch the extension in debug mode
  • You can see in debug console the ggshield installation logs. If you add a secret somewhere it is well detected.

In conclusion,

  • best case: this PR adresses the proxy scenario
  • worst case: it doesn't but we'll have at least updated code to use a more standard lib for https calls.

PR check list

  • As much as possible, the changes include tests
  • If the changes affect the end user (new feature, behavior change, bug fix) then the PR has a changelog entry.

@sevbch sevbch changed the title Severine/fix ggshield retrieval with proxy Fix ggshield retrieval with proxy Apr 28, 2025
@sevbch sevbch force-pushed the severine/fix-ggshield-retrieval-with-proxy branch from e5e7d39 to b377fff Compare April 28, 2025 15:56
@sevbch sevbch force-pushed the severine/fix-ggshield-retrieval-with-proxy branch from b377fff to b4ac271 Compare April 28, 2025 16:08
@sevbch sevbch self-assigned this Apr 28, 2025
@sevbch sevbch marked this pull request as ready for review April 28, 2025 16:11
@sevbch sevbch requested a review from a team as a code owner April 28, 2025 16:11
@sevbch sevbch requested a review from salome-voltz April 28, 2025 16:12
Copy link
Collaborator

@salome-voltz salome-voltz left a comment

Choose a reason for hiding this comment

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

Works for me ! thank you for the changes 🙏

I just have some minor remarks

@sevbch sevbch requested a review from salome-voltz April 30, 2025 07:28
@salome-voltz salome-voltz merged commit 6777104 into main Apr 30, 2025
5 checks passed
@salome-voltz salome-voltz deleted the severine/fix-ggshield-retrieval-with-proxy branch April 30, 2025 09:10
@GabrielCousin GabrielCousin mentioned this pull request Apr 30, 2025
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants