Skip to content
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

Visual Studio cannot print ggshield error messages if secrets are too long #170

Closed
agateau-gg opened this issue Feb 3, 2022 · 5 comments
Labels
status:confirmed This issue has been reviewed and confirmed type:bug Something isn't working

Comments

@agateau-gg
Copy link
Collaborator

agateau-gg commented Feb 3, 2022

Description

When using ggshield as a pre-commit hook in Visual Studio, Visual Studio sometimes cannot print ggshield error message.

This is the error message without the bug:

image

This is the error message with the bug:

image

This happens because when the secret is too long, ggshield clips it, replacing the clipped part with an ellipsis character(). Visual Studio fails to decode it and falls back to its generic error message.

Workaround

This bug can be avoided by defining the PYTHONIOENCODING environment variable to utf-8 before running ggshield. Here is an example of a pre-commit script configured this way:

#!/bin/bash
export PYTHONIOENCODING=utf-8

# If not already set outside, set your API key here
# export GITGUARDIAN_API_KEY=XXXXX

ggshield scan pre-commit

Note that this also affect, in a less annoying way, the command line:

image

This is also fixed by defining the PYTHONIOENCODING environment variable:

image

Possible fixes

I think we can't set the environment variable from within Python, it has to be set before. But it may be possible to change the output encoding of stdout. If this is not possible, the two alternatives are:

  • Set the environment variable in the .git/hooks/pre-commit script we generate. Problem: this won't work when the hook is setup via pre-commit.
  • Replace the ellipsis with .... This is safe but when clipping both sides it would eat 6 characters instead of 2.
  • As found in another project: look at stdout encoding. Something like this:
    if sys.stdout.encoding.lower() in ("utf-8", "utf-16", "utf-32"):
    
    and do not use ellipsis character if the encoding is not "utf-*". I have not tested this, but it is consistent with the fact that the problem can be avoided by defining the PYTHONIOENCODING environment variable.
@agateau-gg agateau-gg added type:bug Something isn't working status:new This issue needs to be reviewed status:confirmed This issue has been reviewed and confirmed and removed status:new This issue needs to be reviewed labels Feb 3, 2022
@mherzberg
Copy link

Are there any updates on this please? The issue still exists, and unfortunately Visual Studio has made the error message worse - it now just complains about character encodings.

The issue especially affects the bundled version of ggshield for Windows due to the use of pyinstaller. Pyinstaller purposefully ignores any environment variables from the user, so the workarounds and possible fixes with users setting PYTHONIOENCODING or other env vars don't work. In addition, I'd imagine just removing ggshield's use of the ellipsis character is not enough as there might be Unicode characters in the actual git diff, too.

To me it seems this issue could be solved nicely with one of the following options:

  1. Add utf8=1 to Pyinstaller when building to enable UTF-8 mode for Python. More information can be found here. Unfortunately I'm not very familiar with Pyinstaller and can't test it, but that seems like it should work.

  2. Add the following lines to the very beginning of ggshield's entry points, e.g. with main in __main__.py:

    sys.stdout.reconfigure(encoding='utf-8')
    sys.stderr.reconfigure(encoding='utf-8')

    This option works without Pyinstaller and seems like it should work via Pyinstaller, too (again, couldn't test unfortunately). This would affect all platforms, but seems like a sensible default anyways.

@mathieubellon
Copy link
Collaborator

mathieubellon commented Nov 27, 2024

Hello @mherzberg,

I have been looking at this issue and here are some feedbacks :

  • I confirm both the issue and the workaround: With Win11/ggshield version 1.33.0 for Windows and the PYTHONIOENCODING=utf-8 it does works (see screenshot). We believe PyInstaller now outputs builds that are aware of the environment and the documentation may not be up to date.

  • We will force UTF-8 encoding in GGShield immediately after today release (so we have some time to dogfood it internally and prevent any issue)

CleanShot 2024-11-27 at 13 57 47

@mherzberg
Copy link

Hi @mathieubellon,

Many thanks for looking into this issue, and great to hear that "force UTF-8 encoding" will be coming soon!

I'm afraid I can't reproduce your fix and screenshot on my end. I tried setting PYTHONIOENCODING=utf-8 back then and just now again, rebooted my machine, but I still receive only an error message about mismatched character encodings. I assume you have double-checked that the pre-commit hook is not using a non-PyInstaller-based version somehow? I'm not sure what could be different to my environment, but given that a fix is hopefully coming soon, I likely won't be able to look into it much more.

We believe PyInstaller now outputs builds that are aware of the environment and the documentation may not be up to date.

Do you have any more information on this? I saw the following in their changelog for version 6.0.0, but nothing about that topic since then:

PyInstaller-frozen applications are not affected by the PYTHONUTF8 environment variable anymore. To permanently enable or disable the UTF8 mode, use the X utf8_mode=1 or X utf_mode=0 run-time option when building the application. (#7847)

@mathieubellon
Copy link
Collaborator

mathieubellon commented Dec 18, 2024

@mherzberg Just to explain the delay in my response. There are many more complexities related to this problem than I expected, and I want to have a solid understanding before answering. Thank you for your patience.

@mathieubellon
Copy link
Collaborator

Friendly ping @mherzberg, in case you are not notified automatically about the recent release with the requested bugfix
Happy new year !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:confirmed This issue has been reviewed and confirmed type:bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants