Conversation
…y default, allow switching off either, and listing a custom set of types if needed
Updated list secret scanning alerts to give all default and generic by default, allow switching off either, and listing a custom set of types if needed
aegilops
requested review from
adrienpessu,
Copilot and
felickz
and removed request for
a team
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the secret type filtering system for secret scanning alerts, replacing a single --generic flag with a more granular approach that allows users to independently control default secrets, generic secrets, and specific secret types.
Key Changes:
- Replaced
--genericflag with--no-genericand--no-defaultflags to allow independent control over secret type categories - Added
--include-typesargument to specify custom secret types - Modified alert aggregation to support multiple concurrent queries for different secret type categories
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
list_secret_scanning_alerts.py |
Refactored alert filtering to support multiple secret type categories with new parameters (default, specific) and modified CLI arguments to use exclusion flags (--no-generic, --no-default) plus inclusion flag (--include-types) |
githubapi.py |
Added secret_types parameter to list_secret_scanning_alerts() method to support filtering by specific secret type list |
README.md |
Updated usage documentation to reflect new command-line argument structure for secret type filtering |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| alerts = [] | ||
|
|
||
| if default: | ||
| default_alerts = g.list_secret_scanning_alerts( | ||
| name, state=state, since=since, scope=scope, bypassed=bypassed, generic=False, progress=progress | ||
| ) | ||
|
|
||
| alerts.append(default_alerts) | ||
|
|
||
| if generic: | ||
| generic_alerts = g.list_secret_scanning_alerts( | ||
| name, state=state, since=since, scope=scope, bypassed=bypassed, generic=True, progress=progress | ||
| ) | ||
|
|
||
| alerts.append(generic_alerts) | ||
|
|
||
| if specific: | ||
| specific_alerts = g.list_secret_scanning_alerts( | ||
| name, state=state, since=since, scope=scope, bypassed=bypassed, secret_types=specific, progress=progress | ||
| ) | ||
| alerts.append(specific_alerts) | ||
|
|
There was a problem hiding this comment.
When none of the flags (default, generic, specific) are set, the alerts list will be empty, and itertools.chain.from_iterable([]) will produce no results. This means the function will return no alerts when called with the default argument values (generic=False, default=False, specific=None). This breaks backward compatibility - previously the function would return default (non-generic) alerts when called without flags. Consider adding a check: if not (default or generic or specific), set default = True to maintain backward compatibility.
| if generic: | ||
| query["secret_type"] = GENERIC_SECRET_TYPES | ||
|
|
||
| if secret_types: | ||
| query["secret_type"] = ",".join(secret_types) |
There was a problem hiding this comment.
When both generic=True and secret_types are provided, the secret_types parameter will overwrite the generic parameter value in the query dictionary (line 443 overwrites line 440). This means that if a user specifies both flags, only the specific types will be queried, and the generic types will be silently ignored. Consider either: (1) merging the values when both are provided, or (2) raising an error to indicate these options are mutually exclusive, or (3) documenting this behavior clearly.
| if generic: | |
| query["secret_type"] = GENERIC_SECRET_TYPES | |
| if secret_types: | |
| query["secret_type"] = ",".join(secret_types) | |
| secret_type_list = [] | |
| if generic: | |
| secret_type_list.extend(GENERIC_SECRET_TYPES.split(",")) | |
| if secret_types: | |
| secret_type_list.extend(secret_types) | |
| if secret_type_list: | |
| query["secret_type"] = ",".join(secret_type_list) |
| generic: bool = False, | ||
| default: bool = False, | ||
| specific: list[str] | None = None, |
There was a problem hiding this comment.
This change breaks backward compatibility for other scripts that call list_secret_scanning_alerts(). The files replay_secret_scanning_result_status.py (line 166) and resolve_duplicate_secret_scanning_alerts.py (line 207) call this function without the new default, generic, or specific parameters. Due to the bug mentioned in the previous comment where empty alerts are returned when all flags are False/None, these scripts will now return zero results. Consider either: (1) fixing the backward compatibility issue by defaulting to previous behavior when no flags are set, or (2) updating all calling scripts to explicitly pass the appropriate flags.
1 participant
No description provided.