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

SEC-1431-Lintly-parser-for-gitleaks #5

Merged
merged 1 commit into from
Mar 1, 2021

Conversation

nandusekarv10
Copy link

No description provided.

@nandusekarv10 nandusekarv10 force-pushed the lintly-parser-for-gitleaks-SEC-1431 branch 3 times, most recently from c947f18 to ac72e30 Compare February 26, 2021 00:28
@scriptsrc scriptsrc self-requested a review February 26, 2021 16:34
Copy link

@scriptsrc scriptsrc left a comment

Choose a reason for hiding this comment

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

Requesting a couple minor changes. It might be a good idea to run some combo of pylint/flake8/black to lint and format consistently.

deploy.sh Outdated Show resolved Hide resolved
lintly/builds.py Outdated Show resolved Hide resolved
lintly/parsers.py Outdated Show resolved Hide resolved
lintly/parsers.py Outdated Show resolved Hide resolved
lintly/parsers.py Outdated Show resolved Hide resolved
tests/test_parsers.py Outdated Show resolved Hide resolved
lintly/parsers.py Outdated Show resolved Hide resolved
lintly/parsers.py Outdated Show resolved Hide resolved
@nandusekarv10 nandusekarv10 force-pushed the lintly-parser-for-gitleaks-SEC-1431 branch from ac72e30 to b1d21ff Compare February 26, 2021 16:51
@nandusekarv10 nandusekarv10 force-pushed the lintly-parser-for-gitleaks-SEC-1431 branch from c0debdc to 38bb9e7 Compare March 1, 2021 20:59
lintly/parsers.py Outdated Show resolved Hide resolved
@nandusekarv10 nandusekarv10 force-pushed the lintly-parser-for-gitleaks-SEC-1431 branch from 38bb9e7 to be2f6c0 Compare March 1, 2021 21:43
Copy link

@sarahc23 sarahc23 left a comment

Choose a reason for hiding this comment

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

Please also edit the commit message (I dunno, may require a rebase and force push?) to be something more descriptive of the entire commit. (Using something similar to the title of this PR is fine, although it's practice around here to put the ticket number before the rest of the message -- SEC-1431 Added lintly parser for gitleaks. or something like that.)

"tags": "key, AWS"
},
{
"line": " \"line\": \"-----BEGIN PRIVATE KEY-----\",",
Copy link

Choose a reason for hiding this comment

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

I still think it's confusing for a future maintainer of this project to use a false positive test case here.

I'm assuming you got this from running gitleaks on the current file (gitleaks.json), which is a FP, because there's no actual private key material here.

If someone had checked in a real key, the line would start with just -----BEGIN PRIVATE KEY-----. (In other words, this entire line should just be

         "line": "-----BEGIN PRIVATE KEY-----",

).

Copy link
Author

Choose a reason for hiding this comment

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

So you want me to change this line to -----BEGIN PRIVATE KEY----- or add extra file with private key and then write test case around that?

Copy link

Choose a reason for hiding this comment

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

Just change the line. I don't think you need to add extra tests or make any changes to the existing test.

@nandusekarv10 nandusekarv10 changed the title Lintly parser for gitleaks sec 1431 SEC-1431-Lintly-parser-for-gitleaks Mar 1, 2021
@nandusekarv10 nandusekarv10 force-pushed the lintly-parser-for-gitleaks-SEC-1431 branch from be2f6c0 to e809073 Compare March 1, 2021 22:31
@sarahc23
Copy link

sarahc23 commented Mar 1, 2021

lol you didn't need to put dashes or hyphens in a commit message (spaces are okay! commit messages are meant for human consumption), but this is fine.

@sarahc23 sarahc23 closed this Mar 1, 2021
@sarahc23 sarahc23 reopened this Mar 1, 2021
@nandusekarv10 nandusekarv10 merged commit 333b5ce into master Mar 1, 2021
@nandusekarv10 nandusekarv10 deleted the lintly-parser-for-gitleaks-SEC-1431 branch March 1, 2021 22:36
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