Preserve PR context and fix dependency comment accuracy - #302
Conversation
e3e708b to
695703b
Compare
695703b to
3cd0355
Compare
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The GitHub comment adapter and pull request link construction each parsed BUILDKITE_REPO independently. Consolidate on socketsecurity.core.git_remote, which also reports the remote host (needed for self-hosted GitHub Enterprise and GitLab) and preserves nested GitLab subgroup paths. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
external_href is only honored while a diff scan is being created, so a re-run over the same before/after pair left the Dashboard report with no link back to its pull request. Send on_duplicate=update alongside it, which applies the link to the existing diff scan and answers 200 with the same envelope as a create. The 409-and-resolve path is retained for runs with no pull request context and for deployments that predate on_duplicate=update. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3cd0355 to
0e03a6f
Compare
lelia
changed the title
create_full_scan_with_report_url only fetched SBOM data when an alert-bearing output format was enabled, so --generate-license and --legal-format fossa saw an empty diff.packages and wrote an attribution file with zero packages. That is the list they enumerate, as _requires_unchanged_artifacts already documents for the comparison path. Fetch the SBOM for them too, and enrich it through the PURL endpoint the way the comparison path does. The full scan's package map is keyed by artifact id while get_license_text_via_purl keys off ecosystem/name@version, so pass a purl-keyed view over the same Package objects. Alert consolidation stays behind its own gate, so an alert-only run does not pay for the license lookup and a license-only run does not build an alert list. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two ways an SCM branch build could still be treated like a pull request: Buildkite always sets BUILDKITE_PULL_REQUEST, to the string "false" on a branch build, so the documented --pr-number "$BUILDKITE_PULL_REQUEST" form delivers a truthy non-numeric value. resolve_pull_request_context read it as no PR but only wrote the normalized number back when one was found, so GithubConfig still saw "false", check_event_type returned "diff" for a push, and comment lookups went to issues/false/comments. Canonicalize config.pr_number before any adapter reads it. A branch run creating a full scan then blocked on diff.new_alerts, which a full scan cannot fill meaningfully: empty with no alert-bearing output format enabled, and every alert in the scan rather than the newly introduced ones with one. The exit code therefore depended on which output format was requested. Treat these runs the way a run with no supported manifest files is already treated and skip blocking, leaving pull request pipelines to enforce policy. Move the scan-type decision into create_scm_scan, which returns the diff and whether it came from a comparison, so the branch is exercised by tests rather than only its predicate. Document both the scan-type table and the blocking consequence in the CI/CD guide. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ignore matching strips the ecosystem off a command so an ecosystem-qualified reply still matches the bare package name parsed out of a start-socket-alert marker. It stripped any leading path segment, and a scope sits in the same position, so "ignore @types/node@*" also suppressed alerts for a package named node. Only strip a leading segment that cannot be a scope. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Labelling every dependency overview row with bold text dropped the badge from added rows, which is the only category the overview rendered before. The badge host publishes diff-added.svg and diff-updated.svg but nothing for removed or replaced, so look the badge up per change type and fall back to the text label only where there is no image to render. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
normalized_defaults has no reader outside the branch that fills it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The entry still described the intermediate behavior where explicit diff flags opted a non-PR run into comparison mode; the detected event type has been authoritative since that was reverted. Record the blocking and license consequences alongside it, plus the ignore and overview fixes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
[agent] Reviewed via two independent passes (security-focused and quality-focused) over the diff against main, plus manual verification of the two most significant findings against the actual source.
Requesting changes on one high-severity issue and one adjacent bug this PR doesn't touch but sits right next to.
Ignore-comment authorization. Comment.author_association is defined on the Comment dataclass but never read anywhere in the codebase -- there's no check that the person posting @SocketSecurity ignore <pkg>@<version> actually has write access to the repo. That gap predates this PR, but this PR is what makes it exploitable in practice: bare (non-ecosystem-qualified) ignore commands were actually broken before (the old parser threw on any scoped package name and the error was swallowed), and the new scope-disambiguation logic here makes that path work again. See the inline comment on is_ignore in scm_comments.py.
Adjacent crash bug, not touched by this PR. process_original_security_comment (the legacy comment-table format) still does pkg_name, pkg_version = details.split("@") on a scoped package name, which unpacks into 3 parts and raises an uncaught ValueError -- the exact bug class this PR fixes in process_updated_security_comment via rsplit("@", 1), just left unfixed in its sibling function. socketsecurity/core/scm_comments.py:155. Worth fixing in the same pass since it's the same bug, in the same file, one function over -- couldn't attach this as an inline comment since the line isn't part of this diff.
Also flagging, not blocking, both left as inline comments: _github_url/_gitlab_url in pull_request.py build the dashboard-link host from GITHUB_SERVER_URL/CI_SERVER_URL without the scheme/netloc validation _repository_url already applies to the sibling env vars in the same file.
One tiny doc fix, not on a changed line so no inline comment for it: add_purl_capabilities's docstring in socketsecurity/core/__init__.py:2346 still says this only touches new_packages, but the loop now also processes updated_packages.
Everything else checked out: the diffType classification, the create_scm_scan full-scan/blocking refactor, the PR-context resolution precedence, and the argparse-suppress plumbing were all traced by hand and are well covered by the new tests.
| target_names = {name} | ||
| if not pkg_type and "/" in name and not name.startswith("@"): | ||
| target_names.add(name.split("/", 1)[1]) | ||
| return bool(package_names & target_names) and (pkg_version == version or version == "*") |
There was a problem hiding this comment.
This function has no way to know whether the commenter is authorized to ignore an alert -- Comment.author_association exists on the dataclass but is never checked anywhere in the call path. This PR is what makes the bare-name fallback here actually functional (it used to throw on any @-containing name), so it's the right spot to add the check rather than deferring it.
Also: package_names always includes the bare pkg_name regardless of pkg_type, so a bare ignore command matches across every ecosystem with that name+version, not just the one that was actually alerted.
| repository = env.get("GITHUB_REPOSITORY") or remote_path or repo | ||
| if not repository or "/" not in repository: | ||
| return None | ||
| server = env.get("GITHUB_SERVER_URL") or (f"https://{remote_host}" if remote_host else "") |
There was a problem hiding this comment.
server is taken directly from GITHUB_SERVER_URL with no scheme/netloc validation, unlike _repository_url just above which checks parsed.scheme in ("http", "https") and parsed.netloc for the sibling env vars. Not reachable through standard GitHub Actions (runner-set), but this value becomes external_href sent to the API, so routing it through the same validator would be cheap defense-in-depth.
| if not project_url: | ||
| remote_host, remote_path = parse_git_remote(env.get("BUILDKITE_REPO")) | ||
| project_path = env.get("CI_PROJECT_PATH") or remote_path or repo | ||
| server = env.get("CI_SERVER_URL") or (f"https://{remote_host}" if remote_host else "") |
There was a problem hiding this comment.
Same gap as GITHUB_SERVER_URL above -- CI_SERVER_URL is used raw here too.
2 participants
Summary
Implementation notes
Pull request context is resolved from explicit configuration first, then from standard provider CI variables. An explicitly supplied zero still disables association.
The diff scan link is sent with
on_duplicate=update, allowing reruns of an existing before/after pair to attach the change URL. Deployments that do not support that behavior retain the existing conflict-resolution fallback.Dependency change categories remain distinct in the report model and overview table. Alert comparison still treats updated dependencies as part of the current scan and replaced dependencies as part of the baseline, preserving the existing blocking semantics.
Generated ignore commands now include the ecosystem, while parsing remains compatible with older bare package commands and splits scoped package names from their version at the final
@.With an SCM adapter active, the detected event type is authoritative: only pull request or merge request events create diff scans. API-only diff flags cannot turn an ordinary branch pipeline into a diff scan.
Testing
579 passed, 2 skippedacrosstests/unitandtests/coreRefs: CE-94, CE-215, CE-337, CE-376, CE-424, CE-441