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

Adding adhoc GitHub script #56

Merged
merged 7 commits into from
Mar 27, 2020
Merged

Adding adhoc GitHub script #56

merged 7 commits into from
Mar 27, 2020

Conversation

domanchi
Copy link
Contributor

@domanchi domanchi commented Mar 25, 2020

Summary

Accidents happen. Sometimes, people write secrets in form fields on Github. However, organizations who subscribe to Github webhooks can scan these fields for secrets, and alert off them appropriately.

This adhoc script enables this functionality, by doing:

from detect_secrets_server.adhoc.github.webhook import scan_for_secrets

def receive_github_event(request):
    try:
        verify_github_webhook(request)
    except Exception as e:
        log_error(e)

    attribution_link = scan_for_secrets(
        request.headers['X-Github-Event'],
        request.json(),
    )
    if attribution_link:
        alert(attribution_link)

I'm uncertain whether this should live in detect-secrets-server or detect-secrets -- I'm open to suggestions for that.

⚠️ Warning! ⚠️

This makes python-crontab an optional dependency. This is because this PR introduces the idea of using detect-secrets-server as a standalone package, that doesn't need to use cron to auto scan repositories.

To address this, I've modified the README to indicate the non-breaking install instruction. However, this will also allow us to install detect-secrets-server with less bloat.

This also depends on Yelp/detect-secrets#287, so I need to perform the appropriate version bumps for necessary tests to pass. However, I thought I'd post the review early on to get comments on it first.

@domanchi domanchi requested a review from KevinHock March 25, 2020 16:00
Copy link
Collaborator

@KevinHock KevinHock left a comment

Choose a reason for hiding this comment

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

looks great so far!! 💯

has_results = any([
line
for line in f.getvalue().splitlines()
if 'True' in line.split(':')[1]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Could add # For e.g. 'SomeDetector : Bool' output



def _parse_comment(body: Dict[str, Any]) -> Tuple[str, str]:
if body.get('action', 'created') == 'deleted':
Copy link
Collaborator

Choose a reason for hiding this comment

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

I could be wrong, but I don't think this does what you want it to.

'created' is the return value of .get if 'action' is not a present key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's accurate. Some comments don't have action on them, and we want to scan the contents as default behavior.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess my feedback is since 'created' == 'deleted' will always be false, it feels weird, but not a strong opinion.

raise KeyError

# NOTE: Explicitly ignoring the issue "title" here, because
# I trust developers enough (hopefully, not famous last words).
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 😄

@KevinHock
Copy link
Collaborator

re: regular detect-secrets vs. d-s-s, I think -server is fitting.

We can add a ref to this additional functionality to the d-s readme though, to add SEO to it.

@domanchi domanchi merged commit c2ccd78 into master Mar 27, 2020
@domanchi domanchi deleted the adding-adhoc-github-script branch March 27, 2020 19:25
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.

None yet

2 participants