Fix GitLab report links, identifiers and locations, and make diff baselines resilient - #337
Draft
lelia wants to merge 15 commits into
Draft
Fix GitLab report links, identifiers and locations, and make diff baselines resilient#337lelia wants to merge 15 commits into
lelia wants to merge 15 commits into
Conversation
The workspace-scoped head scan lookup treated any failed request as "no
baseline". The SDK logs and returns {} for every non-200, so a transient API
error resolved to None, and create_new_diff answers None by creating an empty
baseline scan -- reporting every dependency in the repository as newly added.
An absent "results" key now raises APIFailure, and resolve_base_full_scan_id
surfaces it the same way a missing --base-commit-sha baseline is surfaced.
Selecting the newest scan on the default branch also reintroduced temporary
scans, which the repository head pointer had excluded. The empty baseline scan
that create_new_diff creates inherits the branch and commit of the run that
created it, so a default-branch run whose real scan fails leaves that empty scan
as the newest one. Both baseline lookups now skip tmp scans.
Also unwrap scan_type before it is URL encoded. FullScanParams types it as a
ScanType enum, and urlencode renders a (str, Enum) member as its repr-style
name, which would filter on a scan type that does not exist.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Issue.props reaches the GitLab formatter from several sources, and core.alert_selection already matches both ghsaId/ghsa_id and cveId/cve_id when deciding reachability. The identifier extractor only read the camelCase spellings, so an alert carrying ghsa_id was selected for the report but emitted with only its socket_alert identifier -- the CVE and GHSA values GitLab dedupes and links on were dropped. Values that are neither a string nor a sequence are now skipped rather than iterated, so a malformed prop cannot raise out of the whole report. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The fixture named a real organization. Public test data should not, so use the reserved com.example namespace instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Socket addresses Maven package pages as groupId:artifactId. The CLI emitted the slash-separated form, so every Maven package link 404'd -- the dashboard's Maven handler rejects the slash form outright with "Maven package must have a colon". Removing the enum leak from these URLs fixed how they looked without fixing where they pointed. The separator now follows the ecosystem, via Package.socket_url, which both the full-scan and diff construction paths call. Previously each built its URL inline and they disagreed on namespace handling, so the same package could produce different links depending on which path ran. Purl strings are deliberately left on the slash form for every ecosystem: that is what the purl spec defines and what Socket's purl API consumes. Only the dashboard URL is ecosystem-dependent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lelia
marked this pull request as draft
September 8, 2026 21:22
The Maven separator rule was explained in three places and the enum-unwrapping rule in two. Each now has one home: the separator at URL_NAMESPACE_SEPARATORS where it is defined, the enum behavior at each helper that depends on it, stated once rather than narrated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit c5fcbc8. Configure here.
An ecosystem with its own URL separator cannot be addressed without both halves of the coordinate. A Maven artifact that arrives with no groupId still gets a link so the finding reports, but that link cannot resolve, and previously it was emitted silently. It now logs a warning naming the package. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reverts the separator introduced two commits ago. It rested on a report that the slash form does not resolve, which has since failed to reproduce: every affected link in that report loads, and the report's own screenshots show a working slash-form link. The defect those links actually exhibit is a namespace and name fused with no separator at all, which yields one path segment that cannot be split back into two. A slash fixes that and matches what the other package construction path has always emitted. The missing-namespace warning is kept and re-aimed: an absent namespace is what produces the unsplittable single segment, so that is the case worth surfacing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
main added structured baseline-selection logging to resolve_base_full_scan_id while this branch added workspace and scan-type scoping plus explicit handling for a failed lookup. Both are kept: the log line now reports the scan chosen by the workspace-scoped lookup, and a lookup that errors still exits rather than resolving to an empty baseline. The CHANGELOG section was renamed to 2.8.2 before merging, since main has since shipped 2.8.1 and a matching header is dropped silently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ations
Two defects in the same location block.
The manifest path fell back to "unknown" whenever a package had no introducing
chain. That happens routinely for a transitive package whose top-level ancestors
are absent from the scan's package set, which a diff-scoped run causes by
construction. The package records its own manifest files regardless, so those
are now used before giving up.
Directness was inferred by looking for " > " in the introducing entry, but no
producer emits that separator -- get_source_data yields either ("direct", files)
or (ancestor_purl, files). Every finding was therefore reported as direct,
including transitive ones. It now comes from the package record.
The dependency chain was also parsed into a local that was never read, and the
docstring advertised a dependency_path key the function never returned. Both are
removed rather than wired up, since the GitLab schema expects dependency
references rather than a name path.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The GitLab dependency-scanning schema types an identifier's url as a string matching ^(https?|ftp)://, so a null fails validation. The socket_alert identifier emitted null whenever an alert carried no url, which invalidates that finding for every consumer that validates the report. Verified against the published schema: a report containing an alert with no url now produces zero validation errors. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…mmit-sha A merge base can have no full scan even when default-branch scanning is configured and running: squash merges and rebases rewrite commits, and a multi-commit push produces one scan for the tip while leaving the commits in between unscanned. Any of those turned every open merge request into a failed pipeline, because a missing baseline was a hard stop with no degraded mode. The requested commit is still preferred. When it has no scan, one listing of recent scans is matched against local first-parent history and the nearest scanned ancestor is used instead, logged at warning with the commit chosen and its distance. Only an unreachable ancestor now fails the run. Both bounds are fixed and neither costs an extra request: the listing is fetched once, and the walk stops at a set depth. Following first parents keeps a merge commit from contributing everything merged into it, and a shallow checkout simply narrows the search rather than breaking it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Package links
Report contents
urlinstead of sendingnull, which fails schema validation for the whole findingunknownDiff baselines
--base-commit-shauses the exact matching scan when available and otherwise falls back to the nearest scanned first-parent ancestor, logging which commit was used and how far back it isVersion bumped to 2.8.2.
Why these belong together
These all sit at the boundary between Socket API data and GitLab-facing output, and they share the same two failure modes: a malformed value reaching the report, or a missing baseline silently becoming an empty one. They also ship in one release, so splitting them would add review cycles without changing what lands.
Testing
pytest -q tests/unit tests/core— 568 passed, 2 skipped. A barepytesthonorstestpaths = tests/unitand skipstests/core; CI runs both directories explicitly.ruff check socketsecurity tests— clean across the full tree.git diff --check— clean.dependency-scanning-report-format.json(v15.2.5): zero errors, including the previously invalid null-url case.Notes for review
location.fileis required withminLength: 1in the GitLab schema, sounknownstays as a last-resort placeholder rather than being omitted.truefor every finding, transitive included, because the inference looked for a" > "separatorget_source_datanever produces. Two tests encoded that inference and now use the package record.Fixes: CE-435, CE-438, CE-453
Refs: CE-441