-
Notifications
You must be signed in to change notification settings - Fork 150
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
Add an option to ignore secrets which are removed by the scanned commit #909
base: main
Are you sure you want to change the base?
Conversation
…he scanned commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor remarks, looks good.
not content_splitlines[line_number - 1].startswith("-") | ||
for match in policy_break.matches | ||
for line_number in range( | ||
cast(int, match.line_start), cast(int, match.line_end) + 1 | ||
) | ||
if match.line_start is not None | ||
and match.line_end is not None | ||
and 0 <= line_number - 1 < len(content_splitlines) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This generator expression is a bit long. Can you move it to a helper function?
@@ -20,18 +20,23 @@ def __init__( | |||
verbose: bool, | |||
output: Optional[Path] = None, | |||
ignore_known_secrets: bool = False, | |||
ignore_removed_secrets: bool = True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you set this to False by default, to be consistent with the default value in SecretConfig?
"--ignore-removed-secrets", | ||
is_flag=True, | ||
default=None, | ||
help="Ignore secrets which are removed by the scanned commit.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
help="Ignore secrets which are removed by the scanned commit.", | |
help="Ignore secrets removed by the scanned commit.", |
|
||
### Added | ||
|
||
- Add option `--ignore-removed-secrets` to ignore secrets which are removed by the scanned commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Add option `--ignore-removed-secrets` to ignore secrets which are removed by the scanned commit. | |
- Add option `--ignore-removed-secrets` to ignore secrets removed by the scanned commit. |
@pytest.mark.parametrize("ignore_removed_secrets", [True, False]) | ||
@pytest.mark.parametrize("file_deleted", [True, False]) | ||
def test_scan_repo_ignore_removed_secrets( | ||
tmp_path_factory: pytest.TempPathFactory, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The leaky_repo()
fixture uses tmp_path_factory
because it's a module-scope fixture, but since this test creates its own repo, it can use tmp_path
.
repo.create_commit() | ||
|
||
if file_deleted: | ||
os.remove(secret_file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
os.remove(secret_file) | |
secret_file.unlink() |
ignore_removed_secrets: bool, | ||
file_deleted: bool, | ||
) -> None: | ||
# GIVEN a repository with a past commits adding and removing a leak |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# GIVEN a repository with a past commits adding and removing a leak | |
# GIVEN a repository with a commit adding a leak | |
# AND another commit removing the leak |
Context
Add an option to ignore secrets which are removed by the scanned commit.
The goal of this PR is to create a way to accept secrets which are being remediated. If a dev is removing a secret from their code, but not rewriting the git history, GGShield will prevent them (pre-commit, pre-push or pre-receive) from committing and pushing the secret remediation.
This optional option will allow users to accept secrets in commits when they are removed, either on a deleted line or in a deleted file.
What has been done
--ignore-removed-secrets
via the decoratoradd_secret_scan_common_options
SecretConfig
namedignore_removed_secrets
Validation
Create a repository, add then remove a secret:
Then verify that scanning the repository raises two incidents and only one with
--ignore-removed-secrets
PR check list
skip-changelog
label has been added to the PR.