Skip to content

don't stop on encoding-errors #393

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.

Sign up for GitHub

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

Closed
a7p wants to merge 5 commits into from
Closed

don't stop on encoding-errors #393

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c13d003
don't stop on encoding-errors
Jul 2, 2019
352113d
trying to fix python-version issue
Jul 2, 2019
5bbf4fd
trying to fix python-version issue 2
Jul 2, 2019
72826ab
trying to fix python-version issue 3
Jul 2, 2019
66af165
removes unused import
Jul 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions pre_commit_hooks/detect_aws_credentials.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
from __future__ import unicode_literals

import argparse
import os
import io
from typing import Dict
from typing import List
from typing import Optional
Expand Down Expand Up @@ -76,8 +76,9 @@ def check_file_for_aws_keys(filenames, keys):
bad_files = []

for filename in filenames:
with open(filename, 'r') as content:
text_body = content.read()
with io.open(filename, 'rb') as content:
Copy link
Member

@asottile asottile Jul 2, 2019

Choose a reason for hiding this comment

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

I'd rather see io.open(filename, encoding='UTF-8') -- errors='ignore' can lead to false negatives

Copy link
Author

@a7p a7p Jul 4, 2019

Choose a reason for hiding this comment

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

providing an encoding does not solve my issue (a file which is not utf-8 encoded).

Using ignore means throwing away the characters which cannot be decoded as UTF-8. Since the AWS-Credentials will be ASCII-Strings (this is me claiming something without having any prove) it may only lead to false positives, which would not be that bad imo. I can try to provide further information on this and tests for various scenarios, but it will take a few weeks.

Copy link
Member

@asottile asottile Jul 4, 2019

Choose a reason for hiding this comment

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

afaik, ignore=... and replace=... can remove bytes surrounding an error (consider a surrogate or continuation byte followed by garbage) so they could in fact remove part of the key leading to no-match

Copy link
Author

@a7p a7p Jul 4, 2019

Choose a reason for hiding this comment

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

Sounds plausible to me, I did not know about this. The question is, what is the expected behavior for detect-aws-credentials on non-utf-8 files? I could live with skipping them or with opening them as utf-8 with ignore or replace - in none of these scenarios credential-detection will not (reliably) work for the file, as you pointed out. I cannot see how to build an encoding agnostic solution. If you've got any idea I'll happy to do try to implement it.

Copy link
Member

@asottile asottile Jul 4, 2019

Choose a reason for hiding this comment

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

another idea would be to scan the files as binary files and use the bytes representation of the credentials -- then you don't worry about the encoding

Copy link
Member

@asottile asottile Jul 7, 2019

Choose a reason for hiding this comment

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

Here's one you can use as an example for that: https://github.com/pre-commit/pre-commit-hooks/blob/master/pre_commit_hooks/detect_private_key.py

Copy link
Author

@a7p a7p Jul 7, 2019

Choose a reason for hiding this comment

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

thank you very much - I'll look into it and try to come up with a PR next weekend.

binary_body = content.read()
text_body = binary_body.decode(errors='ignore')
for key in keys:
# naively match the entire file, low chance of incorrect
# collision
Expand Down