Tags: SocketDev/socket-python-cli
Tags
Preserve pull request context, full-scan SCM branch pipelines, and ga… …te ignore commands (#302) * feat(output): show patched versions in security findings * feat(ci): preserve pull request context in scan metadata * chore(release): bump version to 2.9.0 2.8.0 and 2.8.1 shipped from main while this branch was open, so the original 2.8.0 bump here is dead. This branch changes the behavior of existing flags rather than only fixing them -- --pr-number gains auto-detection, --scm github|gitlab implies --integration, and SCM branch pipelines switch from diff scans to full scans and stop returning a blocking exit code -- so it takes the minor bump per the repo's semver standard, not a patch. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * refactor: share one git remote parser between Buildkite consumers 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) <[email protected]> * fix(ci): apply the pull request link to an already-compared scan pair 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) <[email protected]> * fix(comments): make per-alert ignores round trip * fix(comments): preserve dependency change types * fix(ci): use full scans outside pull requests * docs: update release notes for comment fixes * fix(ci): restrict SCM diffs to pull requests * fix(scans): keep the package list on full scans 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) <[email protected]> * fix(ci): keep branch pipelines out of pull request handling 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) <[email protected]> * fix(comments): stop reading an npm scope as an ecosystem 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) <[email protected]> * fix(comments): keep the diff badge where artwork exists 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) <[email protected]> * refactor(config): scope the config-file defaults dict to its block normalized_defaults has no reader outside the branch that fills it. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * docs: correct the release notes for branch pipeline scans 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) <[email protected]> * fix(comments): stop legacy comment updates crashing on scoped names process_original_security_comment split the package cell on every "@", so a scoped name carrying its own "@" unpacked into three values and raised an uncaught ValueError. Same bug class this branch already fixed one function over in process_updated_security_comment, just left in its sibling. Split from the right, and pass the ecosystem through as pkg_type rather than pre-concatenating it onto the package name. That makes the two comment formats agree: both now accept an ignore command for a scoped package in either the ecosystem-qualified or the bare form, where the legacy path previously matched only the qualified one. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * fix(comments): require write access to ignore an alert An @SocketSecurity ignore command suppresses a security finding, but the CLI honored one from any commenter. Comment.author_association was carried on the dataclass and never read, so nothing on the path from comment to suppressed alert asked whether the author could push to the repository. A drive-by ignore-all on an open pull request silenced every finding on it. Gate the ignore bucket in check_for_socket_comments, the one place every consumer goes through. A rejected command is logged with its author and is also absent from the ignore telemetry, which should record what was acted on. GitHub returns author_association with every comment, so the check is free and definitive: OWNER, MEMBER and COLLABORATOR only. GitLab notes carry no equivalent, so project membership is read once per run, and only when an ignore command is actually present. members/all is used rather than a per-user lookup because it answers non-membership with a 200 and an absent id -- CliClient collapses every HTTP error into APIFailure without a status code, so a per-user 404, exactly the outsider case, would be indistinguishable from a token that cannot read the endpoint and would have to fail open. When membership genuinely cannot be read -- a CI_JOB_TOKEN typically cannot -- the command is honored and a warning names the author, so this does not silently break pipelines already relying on ignore commands. Documented alongside the token requirement to get enforcement. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * fix(ci): validate CI-supplied server URLs before building a link GITHUB_SERVER_URL and CI_SERVER_URL were composed into the pull request link verbatim, while the sibling repository URLs read from the same environment already went through a scheme/netloc check. The result is sent to the API as a diff scan's external_href, so route all of them through one validator. Standard runners set these themselves, so this is defense in depth rather than a live hole. An unusable value now falls back to github.com for GitHub; GitLab has no public default host, so the link is dropped and the scan keeps its number. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * docs: correct the add_purl_capabilities docstring The loop covers updated_packages as well as new_packages; the docstring still described only the latter. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * docs: record the review fixes in the 2.9.0 release notes Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * docs: rewrite branch comments for the reader, not the author Sweep of every comment this branch adds, against the fourth-wall skill: - A test docstring stated the scan type "(since 2.8.0)", which was already wrong after the renumber to 2.9.0 and would rot again on the next one. Version stamps in comments describe a debut rather than the behavior. - Two docstrings narrated the failure the old parser produced instead of the invariant that makes rsplit correct. A scoped name carrying its own "@" is the whole reason; the traceback it used to raise is not. - The "do NOT use on_duplicate=redirect" landmine was explained twice, in full, at both call sites. Kept at the 409 fallback, where the temptation to add it lives; the create site now just says what update does. - A test section header justified its own design to a reviewer ("swapping the call back ... fails them"). Restated as what the test actually pins. - "out of this branch" in the remote-URL regex reads as a git branch in this repo; it means the regex case. 642 passed, ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * fix(comments): parse legacy alert rows defensively Each row of the legacy comment table was unpacked through four consecutive splits with no bounds checks: five cells, then the markdown link, then the ecosystem, then the version. The row comes back from the provider's API, so a cell carrying an extra "|", a package cell that is not a link, or a name with no version raised out of the comment rewrite and ended the run before it reported status. A scoped package name in Socket's own table reached the same place with nobody doing anything unusual. parse_alert_table_row returns None instead of raising for any row it cannot read, and an unreadable row keeps its alert reported -- the safe direction, since a row that cannot be parsed cannot be evaluated against the ignore commands either. Also pins change-type preservation against the real artifact conversion rather than a stubbed field. The existing test assigned diffType by hand, so it would have passed whether or not the conversion populated it; the new one runs real DiffArtifact objects through both response shapes, and fails if the field is dropped. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * feat(comments): add --ignore-authorization The write-access gate had no escape hatch, and its GitLab behavior when project membership cannot be read -- honor the command with a warning -- was the one deliberate weakness in it. Both are now a choice: enforce (default) require write access; honor with a warning where the provider cannot report it strict reject in that case instead off perform no check enforce closes the hole wherever the provider can answer without breaking a pipeline whose token cannot read membership, which is why it is the default. strict closes it everywhere and will fail those pipelines. off restores the prior behavior for anyone who needs comment-driven ignores from unverified authors. Threaded through the adapter constructors as a keyword argument with a default, so existing call sites keep working. With off the predicate is never handed to check_for_socket_comments at all, so nothing is filtered and no rejection is logged, rather than a gate that silently approves everything. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * fix(comments): escape repository-derived values when rendering comments Manifest paths and sources are file paths inside the scanned repository, so anyone who can open a pull request controls them: a directory named with link or tag syntax, holding a manifest, put that markup into a comment posted by a trusted integration. Alert text comes from the API. Neither is markup the CLI authored, so both are escaped where they are interpolated -- text nodes with html.escape, href and src with quotes escaped too, since an unescaped quote closes the attribute and everything after it reads as more attributes. The alert markers are the exception: they are read back verbatim when a comment is rewritten, so they cannot be escaped without breaking the ignore round trip. They instead lose only the ability to terminate the comment early. plain and raw styles are untouched. Slack, Jira and the console do not render HTML, and escaping there would show entities to a human. Round-trip tests render a comment with each hostile path and feed it back through the parser, because the renderer and the parser are two halves of one loop: an escaping choice the parser cannot read would silently stop ignores working. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * fix(gitlab): make the authentication fallback actually run _get_auth_headers guesses between Bearer and PRIVATE-TOKEN from the shape of the token, and retries once under the other scheme on a 401 so a wrong guess does not fail the run. That retry has never executed. Three things had to line up and none of them did: - The retry caught requests.exceptions.HTTPError, but CliClient translates every requests error into APIFailure before it gets there. - CliClient discarded the HTTP status, so even a caught failure could not be identified as a 401. is_transient_error was equally blind for the same reason. - There are two APIFailure classes -- the CLI's own and the SDK's -- and they were independent Exception subclasses. CliClient raises the CLI's; every handler in socketsecurity.core imports the SDK's. None of those eight handlers has ever caught a CliClient failure. The CLI's APIFailure now subclasses the SDK's, so a handler written against either catches both, and the status code travels with the exception. The two tests covering the fallback were skipped rather than fixed, with a reason that no longer described the failure -- the constructor they blamed is used by the two passing tests in the same file. They now drive the exception the way CliClient actually raises it, and fail if any of the three links above is broken again. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * fix(review): address authorization and full-scan findings * fix(comments): close ignore authorization gaps --------- Co-authored-by: Claude Opus 5 (1M context) <[email protected]>
Bump pinned @coana-tech/cli to 15.10.40 (#352) Co-authored-by: socket-pr-bot[bot] <294242679+socket-pr-bot[bot]@users.noreply.github.com> Co-authored-by: lelia <[email protected]>
Normalize Slack issue severity and bump SDK to `v3.6.0` (#331) * fix(slack): normalize the API's "middle" severity to "medium" Every severity lookup in the Slack reachability formatter is keyed on "medium", but "middle" is what the API sends. A mid-severity finding missed all of them at once: uncounted in the summary, excluded from total_findings so the "and N more" count can go negative, and sorted at the default order of 4 -- below "low" -- so it was truncated out of the message first. Normalized at the point the alert is read rather than by adding a parallel key to each dict, so one canonical spelling flows downstream. The GitLab severity map and the PR comment path already accept both forms; this formatter did not. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * chore(deps): bump socketdev to 3.6.0 Unblocks the pin now that 3.6.0 is on PyPI. SocketPURL_Type gained ten members -- alpm, chrome, clawhub, edge-extension, firefox-extension, qpkg, socket, swid, vscode and vscode-extension -- and removed none, so artifacts of those types stop falling back to "unknown". No other CLI change is needed: none of the SDK's enum types are imported here, and every severity and type lookup already has a default, so the new members cannot reach an unguarded branch. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * ci: only floor the version check at the latest published release The check required the PR version to exceed both main and PyPI. Comparing against main forbids the legitimate case where several PRs ship under one unreleased version: the first bumps main, and the rest ride it without bumping again so they stay under a single changelog header. Every such PR failed, and the only way to green it was a throwaway bump that would strand a changelog header on a version that never ships. PyPI is now the floor, since the real invariant is that a release cannot reuse a published version. Main is still a floor in the one direction that matters: a PR may leave the version alone or move it forwards, never back. Every genuine failure the old check caught -- forgetting to bump, reusing a published version, branching from a stale base -- still fails. Also added this workflow to its own paths filter so a change to the check is exercised by the PR that makes it. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * ci: require pyproject.toml and __init__.py versions to agree The version lives as two hand-maintained literals with nothing deriving one from the other: pyproject.toml is what gets published, and __init__.py is what the CLI reports as its User-Agent. Every comparison in this job read only __init__.py, so bumping that alone passed the check and then published under the old number -- surfacing late, as twine rejecting an existing file, after the merge. Both are now required to match before any other comparison runs. uv.lock carries a third copy, but uv derives it and `uv lock --locked` in python-tests already fails when it drifts, so it needs no check here. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --------- Co-authored-by: Claude Opus 5 (1M context) <[email protected]>
Stop passing --lazy-mode to the coana CLI (#336) The coana CLI no longer accepts --lazy-mode, so a reachability run started with --reach-lazy-mode failed on an unrecognized argument. --reach-lazy-mode is kept as a hidden no-op, matching how --reach-disable-analysis-splitting was deprecated, so pipelines that still pass it keep working rather than failing argument parsing. Nothing is forwarded to coana, and the flag is dropped from the reachability documentation. No version bump: this is not being released on its own.
Fix pull request comment rendering and disable flags (#322) * Fix orphaned tags and empty tables in PR comments A whitespace-only line closes a CommonMark HTML block. Optional sections that rendered as empty left one behind inside the alerts table, so the indented closing tags after it were rendered as a literal code block reading `</blockquote></details>` instead of markup. - Drop blank lines from generated comment markup and keep indentation below the four spaces that start a code block. - Collapse alert descriptions, suggestions and license findings onto a single line so multi-line API text cannot break the table either. - Replace the comment body with a short confirmation when no alerts are left to report, instead of keeping the caution banner above a table with no rows. The comment marker is preserved so the same comment is updated later. - Apply ignore-all to the pre-2.0.55 Markdown table format. The check was made once per ignore command and an ignore-all comment produces none, so no rows were removed. Bumps to 2.6.8. * Make --disable-security-issue and --disable-overview suppress comments Both flags were checked only after testing whether a comment of that type was already on the pull request, so they suppressed the first post and then updated that comment on every later run. --disable-security-issue in particular kept refreshing an existing comment with the full alerts table. The flags now mean the CLI does not manage that comment at all. An existing comment is left untouched rather than rewritten, since a body claiming no alerts would be inaccurate when reporting is merely switched off. Moves the decision into should_write_comment() so it is covered by tests directly; main_code() had no harness for this block. Bumps to 2.7.0 rather than a patch, since these flags change behavior.
PreviousNext