Skip to content

fix(ghas): Also query generic secret alerts#873

Open
zkdev wants to merge 2 commits into
open-component-model:masterfrom
zkdev:fix-query-generic-secret-alerts
Open

fix(ghas): Also query generic secret alerts#873
zkdev wants to merge 2 commits into
open-component-model:masterfrom
zkdev:fix-query-generic-secret-alerts

Conversation

@zkdev

@zkdev zkdev commented Jun 26, 2026

Copy link
Copy Markdown
Member

What this PR does / why we need it:
Currently, the GHAS extension does only filter for "default" secret alerts.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

GitHub Secret Scanner Alert Extension will now also properly tracking "Generic" alerts.

@zkdev zkdev requested a review from a team as a code owner June 26, 2026 10:11
@zkdev zkdev self-assigned this Jun 26, 2026
@zkdev zkdev added kind/bugfix Bug area/ipcei Important Project of Common European Interest labels Jun 26, 2026
@github-project-automation github-project-automation Bot moved this to 🔦 Needs Triage in Open Delivery Gear Jun 26, 2026
@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The PR changes GHAS secret alert collection to query default and secret-type alerts with URL-based deduplication, and moves the existing GHAS finding metadata lookup in scan to later in the metadata assembly flow.

Changes

GHAS alert processing

Layer / File(s) Summary
Secret alert deduplication
src/ghas.py
get_secret_alerts now queries default alerts first, then secret-type filtered alerts, and skips duplicate URLs while updating the alert count and log output.
Existing metadata lookup timing
src/ghas.py
scan moves the GHAS_FINDING metadata query to after new metadata accumulation, before stale metadata comparison.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

A bunny hopped through GHAS at night,
Counted alerts and kept them right.
New paths sniffed, old echoes hid,
Metadata tucked where timing slid.
🐇✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description follows the template and covers purpose and release note, though the issue reference is left blank.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly matches the main change: GHAS now also queries generic secret alerts.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@zkdev zkdev moved this from 🔦 Needs Triage to 🔍 Review in Open Delivery Gear Jun 26, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/ghas.py`:
- Around line 186-212: The final log message in the secret alert collection flow
is misleading because the `count` in `ghas.py`’s alert-fetching logic includes
both default and deduplicated generic alerts. Update the `logger.info` text near
the end of the function to describe the total de-duplicated alert count rather
than “default” alerts, and consider switching `seen_urls` in the same function
to a `set` so the duplicate check stays efficient while preserving the existing
`github_api_request_paginated` and `SecretAlert` behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 78dbf264-e3fe-46d3-af2d-9ecb266aef55

📥 Commits

Reviewing files that changed from the base of the PR and between 08ce125 and 3fe3c2b.

📒 Files selected for processing (1)
  • src/ghas.py

Comment thread src/ghas.py Outdated
zkdev added 2 commits June 26, 2026 12:39
If not specifying secret-type filter, only default secret alerts are
returned. To also fetch generic secret alerts, their secret-types have
to be explicitly requested.

see:
https://docs.github.com/en/rest/secret-scanning/secret-scanning?apiVersion=2022-11-28#list-secret-scanning-alerts-for-an-organization

> A comma-separated list of secret types to return. All default secret patterns are returned. To return generic patterns, pass the token name(s) in the parameter

Signed-off-by: Philipp Heil (zkdev) <philipp.heil@sap.com>
useful for local testing, as one might not need the ODG metadata.

Signed-off-by: Philipp Heil (zkdev) <philipp.heil@sap.com>
@zkdev zkdev force-pushed the fix-query-generic-secret-alerts branch from 3fe3c2b to 4dddb9b Compare June 26, 2026 10:40
@zkdev zkdev changed the title fix: Also query generic secret alerts fix(ghas): Also query generic secret alerts Jun 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/ipcei Important Project of Common European Interest kind/bugfix Bug

Projects

Status: 🔍 Review

Development

Successfully merging this pull request may close these issues.

1 participant