diff --git a/.github/actions/setup-docker/action.yml b/.github/actions/setup-docker/action.yml index 846efd47..c5ce0fa3 100644 --- a/.github/actions/setup-docker/action.yml +++ b/.github/actions/setup-docker/action.yml @@ -5,6 +5,10 @@ description: >- preview, and stable workflows. inputs: + enable-qemu: + description: "Set up QEMU for multi-platform builds" + required: false + default: "true" dockerhub-username: description: "Docker Hub username (pass from secrets)" required: true @@ -15,9 +19,10 @@ inputs: runs: using: "composite" steps: - - uses: docker/setup-qemu-action@06116385d9baf250c9f4dcb4858b16962ea869c3 # v4.1.0 - - uses: docker/setup-buildx-action@d7f5e7f509e45cec5c76c4d5afdd7de93d0b3df5 # v4.1.0 - - uses: docker/login-action@650006c6eb7dba73a995cc03b0b2d7f5ca915bee # v4.2.0 + - uses: docker/setup-qemu-action@1f40c72289eff860ee54a304f1438e3cff362e0a # v4.3.0 + if: inputs.enable-qemu == 'true' + - uses: docker/setup-buildx-action@37fe631027851001ddb9b187196cc803df7f5f0e # v4.3.0 + - uses: docker/login-action@dbcb813823bdd20940b903addbd779551569679f # v4.6.0 with: username: ${{ inputs.dockerhub-username }} password: ${{ inputs.dockerhub-token }} diff --git a/.github/actions/setup-sfw/action.yml b/.github/actions/setup-sfw/action.yml index 456f90b9..11048d80 100644 --- a/.github/actions/setup-sfw/action.yml +++ b/.github/actions/setup-sfw/action.yml @@ -27,12 +27,12 @@ runs: using: "composite" steps: - if: ${{ inputs.python == 'true' || inputs.uv == 'true' }} - uses: actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 # v6.2.0 + uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 with: python-version: "3.12" - if: ${{ inputs.node == 'true' }} - uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 + uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 with: node-version: "20" diff --git a/.github/workflows/dependency-review.yml b/.github/workflows/dependency-review.yml index a39694f8..fba08631 100644 --- a/.github/workflows/dependency-review.yml +++ b/.github/workflows/dependency-review.yml @@ -48,7 +48,7 @@ jobs: workflow_or_action_changed: ${{ steps.diff.outputs.workflow_or_action_changed }} sfw_mode: ${{ steps.mode.outputs.sfw_mode }} steps: - - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: fetch-depth: 0 persist-credentials: false @@ -135,7 +135,7 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 15 steps: - - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: fetch-depth: 1 persist-credentials: false @@ -226,7 +226,7 @@ jobs: timeout-minutes: 15 environment: socket-firewall steps: - - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: fetch-depth: 1 persist-credentials: false @@ -317,7 +317,7 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 15 steps: - - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: fetch-depth: 1 persist-credentials: false @@ -378,7 +378,7 @@ jobs: timeout-minutes: 15 environment: socket-firewall steps: - - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: fetch-depth: 1 persist-credentials: false @@ -439,7 +439,7 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 15 steps: - - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: fetch-depth: 1 persist-credentials: false @@ -503,7 +503,7 @@ jobs: timeout-minutes: 15 environment: socket-firewall steps: - - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: fetch-depth: 1 persist-credentials: false @@ -565,13 +565,16 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 20 steps: - - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: fetch-depth: 1 persist-credentials: false - name: Build the Dockerfile (no push) - run: docker build --pull -t socket-python-cli:dependabot-smoke . + run: >- + docker build --pull + --build-arg USE_LOCAL_INSTALL=true + -t socket-python-cli:dependabot-smoke . workflow-notice: needs: inspect diff --git a/.github/workflows/docker-stable.yml b/.github/workflows/docker-stable.yml index 934e0d9a..f2a07032 100644 --- a/.github/workflows/docker-stable.yml +++ b/.github/workflows/docker-stable.yml @@ -13,7 +13,7 @@ jobs: stable: runs-on: ubuntu-latest steps: - - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: persist-credentials: false @@ -35,7 +35,7 @@ jobs: dockerhub-token: ${{ secrets.DOCKERHUB_TOKEN }} - name: Build & Push Stable Docker - uses: docker/build-push-action@f9f3042f7e2789586610d6e8b85c8f03e5195baf # v7.2.0 + uses: docker/build-push-action@53b7df96c91f9c12dcc8a07bcb9ccacbed38856a # v7.3.0 with: push: true platforms: linux/amd64,linux/arm64 diff --git a/.github/workflows/e2e-test.yml b/.github/workflows/e2e-test.yml index 1dab8a89..fe763699 100644 --- a/.github/workflows/e2e-test.yml +++ b/.github/workflows/e2e-test.yml @@ -46,6 +46,11 @@ jobs: --enable-debug validate: tests/e2e/validate-reachability.sh setup-node: "true" + # The tier-1 reachability backend intermittently returns empty + # results while the CLI reports success. The probe exits 0 when + # the facts file has alerted components; anything else is + # retried before validation fails the job. + retry-probe: bash tests/e2e/reach-facts-probe.sh tests/e2e/fixtures/simple-npm - name: gitlab args: >- @@ -70,16 +75,16 @@ jobs: name: e2e-${{ matrix.name }} steps: - - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: fetch-depth: 0 persist-credentials: false - - uses: actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 # v6.2.0 + - uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 with: python-version: '3.12' - - uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 + - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 if: matrix.setup-node == 'true' with: node-version: '20' @@ -94,17 +99,59 @@ jobs: run: pip install uv - name: Run Socket CLI + id: run-cli env: SOCKET_SECURITY_API_KEY: ${{ secrets.SOCKET_CLI_API_TOKEN }} + RETRY_PROBE: ${{ matrix.retry-probe }} run: | set -o pipefail - socketcli ${{ matrix.args }} 2>&1 | tee /tmp/e2e-output.log + # Entries with retry-probe get up to 3 attempts: the probe exits 0 + # when the scan output looks complete, and a run that fails it is + # re-run on the assumption of a transient backend failure. A + # persistent incomplete result still reaches validation. Validation + # only treats the explicit zero-project backend signature as + # inconclusive; any other empty result remains a failure. Retries are + # surfaced as warning annotations so flake frequency stays visible. + max_attempts=3 + attempt=1 + while :; do + socketcli ${{ matrix.args }} 2>&1 | tee /tmp/e2e-output.log + [ -z "$RETRY_PROBE" ] && break + if bash -c "$RETRY_PROBE"; then + break + fi + if [ "$attempt" -ge "$max_attempts" ]; then + echo "::warning title=e2e-${{ matrix.name }} incomplete results::output still fails the completeness probe after ${max_attempts} attempts; letting validation classify the result" + echo "diagnostics=true" >> "$GITHUB_OUTPUT" + break + fi + echo "::warning title=e2e-${{ matrix.name }} transient retry::attempt ${attempt} failed the completeness probe (suspected backend transient); retrying" + echo "e2e-${{ matrix.name }}: retry after attempt ${attempt} โ€” completeness probe failed (suspected transient)" >> "$GITHUB_STEP_SUMMARY" + attempt=$((attempt+1)) + sleep 30 + done - name: Validate results env: SOCKET_SECURITY_API_KEY: ${{ secrets.SOCKET_CLI_API_TOKEN }} run: bash ${{ matrix.validate }} + - name: Upload diagnostics on failure + if: failure() || steps.run-cli.outputs.diagnostics == 'true' + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: e2e-${{ matrix.name }}-diagnostics-attempt${{ github.run_attempt }} + path: | + /tmp/e2e-output.log + /tmp/*.sarif + tests/e2e/fixtures/simple-npm/.socket.facts.json + tests/e2e/fixtures/simple-pypi/.socket.facts.json + gl-*.json + license_output.json + if-no-files-found: ignore + include-hidden-files: true + retention-days: 14 + # Branch protection requires the e2e-* checks, but the `e2e` job above is # skipped on PRs that can't access repository secrets -- fork PRs and # Dependabot PRs. A job skipped via a job-level `if` never expands its diff --git a/.github/workflows/package-check.yml b/.github/workflows/package-check.yml new file mode 100644 index 00000000..d71083a4 --- /dev/null +++ b/.github/workflows/package-check.yml @@ -0,0 +1,66 @@ +name: Package Check + +on: + pull_request: + push: + branches: [main] + workflow_dispatch: + +permissions: + contents: read + +concurrency: + group: package-check-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true + +jobs: + package-check: + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + fetch-depth: 1 + persist-credentials: false + + - uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 + with: + python-version: "3.12" + + - name: Install build tooling + uses: ./.github/actions/setup-hatch + + - name: Install distribution validator + run: python -m pip install "twine>=4.0.0" + + - name: Build distributions + run: hatch build + + - name: Validate distributions + run: python -m twine check dist/* + + - name: Install and inspect wheel without resolving dependencies + run: | + python -m venv "$RUNNER_TEMP/package-check" + "$RUNNER_TEMP/package-check/bin/pip" install --no-deps dist/*.whl + "$RUNNER_TEMP/package-check/bin/python" - <<'PY' + import compileall + import importlib.metadata + import pathlib + import sysconfig + + distribution = importlib.metadata.distribution("socketsecurity") + entry_points = {entry_point.name for entry_point in distribution.entry_points} + assert "socketcli" in entry_points + package = pathlib.Path(sysconfig.get_paths()["purelib"]) / "socketsecurity" + assert compileall.compile_dir(package, quiet=1) + print("wheel metadata and bytecode smoke OK", distribution.version) + PY + + - name: Upload distributions + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: socketsecurity-${{ github.sha }} + path: dist/* + if-no-files-found: error + retention-days: 14 diff --git a/.github/workflows/pr-preview.yml b/.github/workflows/pr-preview.yml index eb29ef91..d3ef1c8b 100644 --- a/.github/workflows/pr-preview.yml +++ b/.github/workflows/pr-preview.yml @@ -1,169 +1,314 @@ -name: PR Preview +name: Publish PR Preview + on: pull_request: - types: [opened, synchronize, ready_for_review] + types: [labeled] + workflow_dispatch: + inputs: + pr_number: + description: Pull request number to publish + required: true + type: string + publish_test_pypi: + description: Publish the Python package to TestPyPI + required: true + default: true + type: boolean + publish_docker: + description: Publish socketdev/cli:pr- to Docker Hub + required: true + default: false + type: boolean + sdk_preview_version: + description: Optional exact TestPyPI socketdev prerelease for the Docker image + required: false + type: string -# Cancel an in-flight preview when the PR is pushed again -- previews are slow -# (publish + multi-step Docker build), so superseded runs shouldn't keep going. concurrency: - group: pr-preview-${{ github.event.pull_request.number }} - cancel-in-progress: true + group: publish-pr-preview-${{ github.event.pull_request.number || github.run_id }} + cancel-in-progress: false jobs: - preview: - # Skip on: - # - PRs from forks (no access to publish secrets) - # - Dependabot PRs: preview-publishing a dependency bump to Test PyPI / - # Docker Hub is pointless and fails (no version bump, secret access). + context: if: >- - github.event.pull_request.head.repo.full_name == github.repository && - github.event.pull_request.user.login != 'dependabot[bot]' + github.event_name == 'workflow_dispatch' || + (github.event.label.name == 'publish-preview' || + github.event.label.name == 'publish-docker-preview') && + github.event.pull_request.head.repo.full_name == github.repository runs-on: ubuntu-latest + timeout-minutes: 5 permissions: - id-token: write contents: read - pull-requests: write + pull-requests: read + outputs: + pr_number: ${{ steps.context.outputs.pr_number }} + head_sha: ${{ steps.context.outputs.head_sha }} + steps: + - name: Validate pull request context + id: context + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + env: + DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} + EVENT_PR_NUMBER: ${{ github.event.pull_request.number }} + INPUT_PR_NUMBER: ${{ inputs.pr_number }} + WORKFLOW_REF: ${{ github.ref }} + with: + script: | + const rawPrNumber = context.eventName === 'workflow_dispatch' + ? process.env.INPUT_PR_NUMBER + : process.env.EVENT_PR_NUMBER; + if (!/^[1-9][0-9]*$/.test(rawPrNumber || '')) { + core.setFailed('Pull request number must contain ASCII digits only.'); + return; + } + + if (context.eventName === 'workflow_dispatch') { + const defaultRef = `refs/heads/${process.env.DEFAULT_BRANCH}`; + if (process.env.WORKFLOW_REF !== defaultRef) { + core.setFailed(`Run manual previews from ${defaultRef}.`); + return; + } + } + + const prNumber = Number(rawPrNumber); + if (!Number.isSafeInteger(prNumber)) { + core.setFailed('Pull request number is outside the supported range.'); + return; + } + const {data: pullRequest} = await github.rest.pulls.get({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: prNumber, + }); + if (pullRequest.state !== 'open') { + core.setFailed(`Pull request #${prNumber} is not open.`); + return; + } + if (pullRequest.head.repo?.full_name !== `${context.repo.owner}/${context.repo.repo}`) { + core.setFailed('Preview publication is limited to branches in this repository.'); + return; + } + + core.setOutput('pr_number', String(prNumber)); + core.setOutput('head_sha', pullRequest.head.sha); + + build: + needs: context + runs-on: ubuntu-latest + timeout-minutes: 10 + permissions: + contents: read + outputs: + preview_version: ${{ steps.version.outputs.preview_version }} steps: - - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: + ref: ${{ needs.context.outputs.head_sha }} fetch-depth: 0 persist-credentials: false - - uses: actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 # v6.2.0 + + - uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 with: - python-version: '3.13' + python-version: "3.12" - name: Install build tooling uses: ./.github/actions/setup-hatch - - name: Inject full dynamic version - run: python .hooks/sync_version.py --dev + - name: Install distribution validator + run: python -m pip install "twine>=4.0.0" - - name: Clean previous builds - run: rm -rf dist/ build/ *.egg-info + - name: Inject deterministic preview version + env: + PREVIEW_ID: ${{ github.run_id }} + RUN_ATTEMPT: ${{ github.run_attempt }} + run: | + PREVIEW_ID=$((PREVIEW_ID * 100 + RUN_ATTEMPT)) + python .hooks/sync_version.py --dev --preview-id "$PREVIEW_ID" --skip-lock - - name: Get Hatch version + - name: Read preview version id: version - run: | - VERSION=$(hatch version | cut -d+ -f1) - echo "VERSION=$VERSION" >> $GITHUB_ENV + run: echo "preview_version=$(hatch version)" >> "$GITHUB_OUTPUT" - - name: Check if version already exists on Test PyPI - id: version_check - env: - VERSION: ${{ env.VERSION }} - run: | - if curl -s -f https://test.pypi.org/pypi/socketsecurity/${VERSION}/json > /dev/null; then - echo "exists=true" >> $GITHUB_OUTPUT - else - echo "exists=false" >> $GITHUB_OUTPUT - fi - - - name: Build package - if: steps.version_check.outputs.exists != 'true' + - name: Build and validate distributions run: | hatch build + python -m twine check dist/* + + - name: Install and inspect wheel locally + run: | + python -m venv "$RUNNER_TEMP/preview-check" + "$RUNNER_TEMP/preview-check/bin/pip" install --no-deps dist/*.whl + "$RUNNER_TEMP/preview-check/bin/python" - <<'PY' + import compileall + import importlib.metadata + import pathlib + import sysconfig + + distribution = importlib.metadata.distribution("socketsecurity") + entry_points = {entry_point.name for entry_point in distribution.entry_points} + assert "socketcli" in entry_points + package = pathlib.Path(sysconfig.get_paths()["purelib"]) / "socketsecurity" + assert compileall.compile_dir(package, quiet=1) + print("preview wheel smoke OK", distribution.version) + PY + + - name: Upload preview distributions + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: socketsecurity-preview-${{ github.run_id }}-${{ github.run_attempt }} + path: dist/* + if-no-files-found: error + retention-days: 14 - - name: Publish to Test PyPI - if: steps.version_check.outputs.exists != 'true' - uses: pypa/gh-action-pypi-publish@cef221092ed1bacb1cc03d23a2d87d1d172e277b # v1.14.0 + publish-package: + needs: [context, build] + if: >- + github.event.label.name == 'publish-preview' || + (github.event_name == 'workflow_dispatch' && inputs.publish_test_pypi) + runs-on: ubuntu-latest + timeout-minutes: 10 + permissions: + contents: read + id-token: write + pull-requests: write + steps: + - name: Download preview distributions + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + with: + name: socketsecurity-preview-${{ github.run_id }}-${{ github.run_attempt }} + path: dist + + - name: Publish to TestPyPI + uses: pypa/gh-action-pypi-publish@dc37677b2e1c63e2034f94d8a5b11f265b73ba33 # v1.14.2 with: repository-url: https://test.pypi.org/legacy/ verbose: true - - name: Comment on PR - if: steps.version_check.outputs.exists != 'true' + - name: Comment on pull request uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 env: - VERSION: ${{ env.VERSION }} + PREVIEW_VERSION: ${{ needs.build.outputs.preview_version }} + PR_NUMBER: ${{ needs.context.outputs.pr_number }} with: script: | - const version = process.env.VERSION; - const prNumber = context.payload.pull_request.number; - const owner = context.repo.owner; - const repo = context.repo.repo; - // Find existing bot comments - const comments = await github.rest.issues.listComments({ - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: prNumber, - }); - - const botComment = comments.data.find(comment => - comment.user.type === 'Bot' && - comment.body.includes('๐Ÿš€ Preview package published!') - ); - - const comment = ` - ๐Ÿš€ Preview package published! + const marker = ''; + const prNumber = Number(process.env.PR_NUMBER); + const version = process.env.PREVIEW_VERSION; + const body = `${marker} + ๐Ÿš€ CLI preview published: \`socketsecurity==${version}\` - Install with: \`\`\`bash pip install --index-url https://test.pypi.org/simple/ --extra-index-url https://pypi.org/simple socketsecurity==${version} \`\`\` - Docker image: \`socketdev/cli:pr-${prNumber}\` - `; - - if (botComment) { - // Update existing comment + TestPyPI's package index can take several minutes to expose a newly uploaded version.`; + const {data: comments} = await github.rest.issues.listComments({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + }); + const existing = comments.find(comment => + comment.user.type === 'Bot' && comment.body.includes(marker) + ); + if (existing) { await github.rest.issues.updateComment({ - owner: owner, - repo: repo, - comment_id: botComment.id, - body: comment + owner: context.repo.owner, + repo: context.repo.repo, + comment_id: existing.id, + body, }); } else { - // Create new comment await github.rest.issues.createComment({ - owner: owner, - repo: repo, + owner: context.repo.owner, + repo: context.repo.repo, issue_number: prNumber, - body: comment + body, }); } - - name: Verify package is available - if: steps.version_check.outputs.exists != 'true' - id: verify_package - env: - VERSION: ${{ env.VERSION }} - run: | - for i in {1..30}; do - if pip install --index-url https://test.pypi.org/simple/ --extra-index-url https://pypi.org/simple socketsecurity==${VERSION}; then - echo "Package ${VERSION} is now available and installable on Test PyPI" - pip uninstall -y socketsecurity - echo "success=true" >> $GITHUB_OUTPUT - exit 0 - fi - echo "Attempt $i: Package not yet installable, waiting 20s... (${i}/30)" - sleep 20 - done - echo "success=false" >> $GITHUB_OUTPUT - exit 1 + publish-docker: + needs: [context, build] + if: >- + github.event.label.name == 'publish-docker-preview' || + (github.event_name == 'workflow_dispatch' && inputs.publish_docker) + runs-on: ubuntu-latest + timeout-minutes: 15 + permissions: + contents: read + pull-requests: write + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + # Keep the Dockerfile and credential-handling action on trusted code. + # The pull request enters this job only through the built wheel. + ref: ${{ github.event.repository.default_branch }} + fetch-depth: 1 + persist-credentials: false + + - name: Download preview distributions + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + with: + name: socketsecurity-preview-${{ github.run_id }}-${{ github.run_attempt }} + path: dist - name: Set up Docker publishing - if: steps.verify_package.outputs.success == 'true' uses: ./.github/actions/setup-docker with: + # QEMU is required to build the arm64 layer on an amd64 runner. + enable-qemu: "true" dockerhub-username: ${{ secrets.DOCKERHUB_USERNAME }} dockerhub-token: ${{ secrets.DOCKERHUB_TOKEN }} - - name: Build & Push Docker Preview - if: steps.verify_package.outputs.success == 'true' - uses: docker/build-push-action@f9f3042f7e2789586610d6e8b85c8f03e5195baf # v7.2.0 - env: - VERSION: ${{ env.VERSION }} + - name: Build and push Docker preview + uses: docker/build-push-action@53b7df96c91f9c12dcc8a07bcb9ccacbed38856a # v7.3.0 with: + # Build from the checked-out workspace, not the default Git context. The + # wheel is only present here as a downloaded artifact, and this also keeps + # Dockerfile.preview on the default-branch checkout rather than the PR ref. + context: . + file: Dockerfile.preview push: true - # Preview images are for quick testing -- build amd64 only. arm64 via - # QEMU emulation is the slowest part of the job; release builds keep - # multi-arch. GHA layer cache speeds up repeated preview builds. - platforms: linux/amd64 - cache-from: type=gha - cache-to: type=gha,mode=max - tags: | - socketdev/cli:pr-${{ github.event.pull_request.number }} + pull: true + # Match the arch matrix of the release and stable images so a preview is a + # drop-in replacement for socketdev/cli:latest on arm64 runners too. The + # arm64 layer builds under emulation, so expect roughly double the runtime. + platforms: linux/amd64,linux/arm64 + tags: socketdev/cli:pr-${{ needs.context.outputs.pr_number }} build-args: | - CLI_VERSION=${{ env.VERSION }} - PIP_INDEX_URL=https://test.pypi.org/simple - PIP_EXTRA_INDEX_URL=https://pypi.org/simple + SDK_PREVIEW_VERSION=${{ inputs.sdk_preview_version }} + + - name: Comment on pull request + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + env: + PR_NUMBER: ${{ needs.context.outputs.pr_number }} + with: + script: | + const marker = ''; + const prNumber = Number(process.env.PR_NUMBER); + const body = `${marker} + ๐Ÿณ Docker preview published: \`socketdev/cli:pr-${prNumber}\` + + This mutable tag is only created when a Docker preview is explicitly requested.`; + const {data: comments} = await github.rest.issues.listComments({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + }); + const existing = comments.find(comment => + comment.user.type === 'Bot' && comment.body.includes(marker) + ); + if (existing) { + await github.rest.issues.updateComment({ + owner: context.repo.owner, + repo: context.repo.repo, + comment_id: existing.id, + body, + }); + } else { + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + body, + }); + } diff --git a/.github/workflows/python-tests.yml b/.github/workflows/python-tests.yml index 3247275a..f05b27c9 100644 --- a/.github/workflows/python-tests.yml +++ b/.github/workflows/python-tests.yml @@ -35,12 +35,12 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 20 steps: - - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: fetch-depth: 1 persist-credentials: false - name: ๐Ÿ setup python - uses: actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 # v6.2.0 + uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 with: python-version: ${{ env.PYTHON_VERSION }} - name: ๐Ÿ› ๏ธ install deps @@ -67,16 +67,36 @@ jobs: uv export --no-hashes --no-emit-project --format requirements-txt > /tmp/req-audit.txt uvx pip-audit --strict --progress-spinner off --disable-pip --no-deps -r /tmp/req-audit.txt + ruff: + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + fetch-depth: 1 + persist-credentials: false + - name: ๐Ÿ setup python + uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 + with: + python-version: ${{ env.PYTHON_VERSION }} + - name: ๐Ÿ› ๏ธ install deps + run: | + python -m pip install --upgrade pip + pip install uv + uv sync --extra dev + - name: ๐Ÿงน run ruff + run: uv run ruff check + unsupported-python-install: runs-on: ubuntu-latest timeout-minutes: 10 steps: - - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: fetch-depth: 1 persist-credentials: false - name: ๐Ÿ setup python - uses: actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 # v6.2.0 + uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 with: python-version: "3.10" - name: ๐Ÿšซ verify install is rejected on unsupported python diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 6c41e9c0..62b2f2b7 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -10,11 +10,11 @@ jobs: id-token: write contents: read steps: - - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: fetch-depth: 0 persist-credentials: false - - uses: actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 # v6.2.0 + - uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 with: python-version: '3.13' @@ -66,7 +66,7 @@ jobs: - name: Publish to PyPI if: steps.version_check.outputs.pypi_exists != 'true' - uses: pypa/gh-action-pypi-publish@cef221092ed1bacb1cc03d23a2d87d1d172e277b # v1.14.0 + uses: pypa/gh-action-pypi-publish@dc37677b2e1c63e2034f94d8a5b11f265b73ba33 # v1.14.2 - name: Set up Docker publishing uses: ./.github/actions/setup-docker @@ -79,24 +79,38 @@ jobs: env: VERSION: ${{ env.VERSION }} run: | - for i in {1..30}; do - if pip install socketsecurity==${VERSION}; then + # The first lookup can race PyPI's Simple-index propagation, and a delayed + # CDN purge can leave the index stale well after a successful upload. + # pip caches HTTP responses by default, so without --no-cache-dir every + # retry can reuse that initial stale response instead of checking whether + # the release has appeared. Budget: 30 minutes. + MAX_ATTEMPTS=60 + for i in $(seq 1 "$MAX_ATTEMPTS"); do + if python -m pip install \ + --no-cache-dir \ + --index-url https://pypi.org/simple/ \ + "socketsecurity==${VERSION}"; then echo "Package ${VERSION} is now available and installable on PyPI" - pip uninstall -y socketsecurity - echo "success=true" >> $GITHUB_OUTPUT + python -m pip uninstall -y socketsecurity + echo "success=true" >> "$GITHUB_OUTPUT" exit 0 fi - echo "Attempt $i: Package not yet installable, waiting 20s... (${i}/30)" - sleep 20 + if curl -s -f "https://pypi.org/pypi/socketsecurity/${VERSION}/json" > /dev/null; then + echo "Release ${VERSION} exists on PyPI (JSON API) but is not in the Simple index yet - CDN propagation delay" + fi + if [ "$i" -lt "$MAX_ATTEMPTS" ]; then + echo "Attempt $i: Package not yet installable, waiting 30s... (${i}/${MAX_ATTEMPTS})" + sleep 30 + fi done - echo "success=false" >> $GITHUB_OUTPUT + echo "success=false" >> "$GITHUB_OUTPUT" exit 1 - name: Build & Push Docker if: | steps.verify_package.outputs.success == 'true' && steps.docker_check.outputs.docker_exists != 'true' - uses: docker/build-push-action@f9f3042f7e2789586610d6e8b85c8f03e5195baf # v7.2.0 + uses: docker/build-push-action@53b7df96c91f9c12dcc8a07bcb9ccacbed38856a # v7.3.0 env: VERSION: ${{ env.VERSION }} with: diff --git a/.github/workflows/version-check.yml b/.github/workflows/version-check.yml index a0972080..292e2980 100644 --- a/.github/workflows/version-check.yml +++ b/.github/workflows/version-check.yml @@ -6,6 +6,8 @@ on: - 'socketsecurity/**' - 'pyproject.toml' - 'uv.lock' + # Included so a change to the check itself is exercised by its own PR. + - '.github/workflows/version-check.yml' permissions: contents: read @@ -20,7 +22,7 @@ jobs: if: github.event.pull_request.user.login != 'dependabot[bot]' runs-on: ubuntu-latest steps: - - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: fetch-depth: 0 # Fetch all history for all branches persist-credentials: false @@ -42,16 +44,37 @@ jobs: export PR_VERSION export MAIN_VERSION - # Compare against both main and latest published PyPI release. + # Compare against the latest published PyPI release. python3 <<'PY' import json import os + import tomllib import urllib.request from packaging import version pr_ver = version.parse(os.environ["PR_VERSION"]) main_ver = version.parse(os.environ["MAIN_VERSION"]) + with open("pyproject.toml", "rb") as fh: + pyproject_ver = version.parse(tomllib.load(fh)["project"]["version"]) + + # The version is two hand-maintained literals with nothing deriving one + # from the other: pyproject.toml is what actually gets published, and + # socketsecurity/__init__.py is what the CLI reports as its User-Agent. + # Every comparison below reads only __init__.py, so bumping that alone + # would pass this job and then publish under the old number -- caught + # late, by twine rejecting an existing file, after the merge. Require + # the two to agree before comparing anything. (uv.lock carries a third + # copy, but uv derives it and `uv lock --locked` in python-tests + # already fails when it drifts.) + if pr_ver != pyproject_ver: + print( + f"โŒ Version mismatch inside the PR: pyproject.toml is " + f"{pyproject_ver}, socketsecurity/__init__.py is {pr_ver}. " + f"Bump both." + ) + raise SystemExit(1) + with urllib.request.urlopen("https://pypi.org/pypi/socketsecurity/json") as response: pypi_data = json.load(response) @@ -62,19 +85,37 @@ jobs: published_versions.append(parsed) pypi_ver = max(published_versions) if published_versions else version.parse("0.0.0") - required_floor = max(main_ver, pypi_ver) - if pr_ver <= required_floor: + # The only hard requirement is that the version is ahead of what is + # actually released. Treating main's version as a second floor breaks + # the legitimate case where several PRs share one unreleased release: + # the first bumps main to the new version and the rest ride it without + # bumping again, which is what keeps them under a single changelog + # header. Main is therefore only a floor when this PR moves the + # version -- a change to it must go forwards, never backwards. + if pr_ver <= pypi_ver: print( - f"โŒ Version must be greater than main and PyPI! " - f"Main: {main_ver}, PyPI: {pypi_ver}, PR: {pr_ver}" + f"โŒ Version {pr_ver} is already published on PyPI " + f"(latest release: {pypi_ver}). Bump it." + ) + raise SystemExit(1) + + if pr_ver < main_ver: + print( + f"โŒ Version moves backwards: main is {main_ver}, PR is {pr_ver}." ) raise SystemExit(1) - print( - f"โœ… Version properly incremented. " - f"Main: {main_ver}, PyPI: {pypi_ver}, PR: {pr_ver}" - ) + if pr_ver == main_ver: + print( + f"โœ… Riding main's unreleased {pr_ver} " + f"(latest PyPI release: {pypi_ver})." + ) + else: + print( + f"โœ… Version properly incremented. " + f"Main: {main_ver}, PyPI: {pypi_ver}, PR: {pr_ver}" + ) PY - name: Require uv.lock update when pyproject changes diff --git a/.hooks/sync_version.py b/.hooks/sync_version.py index 57b29d31..51835c3f 100644 --- a/.hooks/sync_version.py +++ b/.hooks/sync_version.py @@ -1,10 +1,10 @@ #!/usr/bin/env python3 -import subprocess +import json import pathlib import re +import subprocess import sys import urllib.request -import json INIT_FILE = pathlib.Path("socketsecurity/__init__.py") PYPROJECT_FILE = pathlib.Path("pyproject.toml") @@ -125,13 +125,45 @@ def run_uv_lock() -> bool: after = UV_LOCK_FILE.read_bytes() if UV_LOCK_FILE.exists() else b"" return before != after + +def read_preview_id(): + if "--preview-id" not in sys.argv: + return None + + option_index = sys.argv.index("--preview-id") + try: + preview_id = sys.argv[option_index + 1] + except IndexError: + print("โŒ `--preview-id` requires a numeric value.") + sys.exit(1) + + if not preview_id.isascii() or not preview_id.isdigit(): + print("โŒ `--preview-id` must contain ASCII digits only.") + sys.exit(1) + return preview_id + + def main(): dev_mode = "--dev" in sys.argv + skip_lock = "--skip-lock" in sys.argv + preview_id = read_preview_id() current_version = read_version_from_init(INIT_FILE) previous_version = read_version_from_git("socketsecurity/__init__.py") print(f"Current: {current_version}, Previous: {previous_version}") + if preview_id is not None: + if not dev_mode: + print("โŒ `--preview-id` can only be used with `--dev`.") + sys.exit(1) + base_version = current_version.split(".dev")[0] + new_version = f"{base_version}.dev{preview_id}" + inject_version(new_version) + if not skip_lock: + run_uv_lock() + print(f"โœ… Prepared deterministic preview version {new_version}.") + sys.exit(0) + if current_version == previous_version: if dev_mode: base_version = current_version.split(".dev")[0] if ".dev" in current_version else current_version diff --git a/CHANGELOG.md b/CHANGELOG.md index 3230afef..5f5d78a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,494 @@ # Changelog +## 2.9.3 + +### Changed: bump pinned @coana-tech/cli to 15.10.45 + +- Bumped the pinned reachability engine (`@coana-tech/cli`) from `15.10.44` to + `15.10.45`. See the + [reachability analysis changelog](https://docs.socket.dev/docs/reachability-analysis-changelog) + for engine changes. + +## 2.9.2 + +### Changed: bump pinned @coana-tech/cli to 15.10.44 + +- Bumped the pinned reachability engine (`@coana-tech/cli`) from `15.10.43` to + `15.10.44`. See the + [reachability analysis changelog](https://docs.socket.dev/docs/reachability-analysis-changelog) + for engine changes. + +## 2.9.1 + +### Changed: bump pinned @coana-tech/cli to 15.10.43 + +- Bumped the pinned reachability engine (`@coana-tech/cli`) from `15.10.41` to + `15.10.43`. See the + [reachability analysis changelog](https://docs.socket.dev/docs/reachability-analysis-changelog) + for engine changes. + +## 2.9.0 + +### Added: patched versions in human-readable security output + +- The native console alert table now includes a `Patched Version` column, + populated from `props.firstPatchedVersionIdentifier` when the API provides it. +- GitHub pull request and GitLab merge request security comments now show the + patched version in each applicable alert's details. + +### Fixed: CLI scans retain pull request context in the Socket Dashboard + +- Pull request numbers are detected from standard GitHub Actions, GitLab CI, + and Azure Pipelines environments when `--pr-number` is not supplied. An + explicitly supplied value, including `0`, remains authoritative. +- The Buildkite workflow and CI/CD guide now forward `BUILDKITE_PULL_REQUEST` + explicitly and document provider selection for Dashboard PR association. With + `--integration github` or `--integration gitlab`, the repository slug and host + for the link are read from `BUILDKITE_REPO`, covering self-hosted installations. +- `--scm github` and `--scm gitlab` now imply the matching scan integration + unless `--integration` is explicitly supplied. +- Diff scans include the detected pull request or merge request URL as their + external link, allowing Dashboard reports to retain their CI change context. + Re-running a comparison over an already-compared scan pair now applies the + link to the existing diff scan instead of leaving that report unassociated. +- A `--pr-number` value that is not a positive integer is now normalized to `0` + before the GitHub adapter reads it, so Buildkite's `false` on a branch build no + longer makes that build look like a pull request event. + +### Changed: GitHub and GitLab branch pipelines create full scans + +- With `--scm github` or `--scm gitlab`, only pull request and merge request + events create diff scans. Every other pipeline, including default-branch + pushes, creates a full scan. The detected event type is authoritative: + `--enable-diff` and `--ignore-commit-files` no longer opt an SCM branch run + into comparison mode. +- Those runs no longer set a blocking exit code. A full scan has no baseline, so + it cannot distinguish newly introduced alerts from pre-existing ones; the CLI + now behaves as if `--disable-blocking` was supplied, matching how it already + treats a run with no supported manifest files. Pull request and merge request + pipelines are unaffected and still block. +- `--generate-license` and `--legal-format fossa` fetch the package list on this + path, so attribution files generated from a branch pipeline are complete rather + than empty. +- Console-only full scans link to the Socket report and state that findings were + not fetched for console output instead of presenting an empty local alert list + as "No issues found." +- License enrichment keeps the package namespace in PURL requests and response + matching, so scoped npm packages and namespaced Maven packages receive their + license details. + +### Changed: `@SocketSecurity ignore` requires write access + +- An ignore command suppresses a security alert, but the CLI honored one from any + commenter, including a drive-by comment from someone with no access to the + repository. Commands are now accepted only from an author with write access. +- On GitHub this is read from the effective repository permission and cached per + commenter for the run. Write, maintain, or admin access is required; relationship + labels such as `MEMBER` and `COLLABORATOR` are not treated as permissions. +- A 404 from GitHub's collaborator-permission endpoint is treated as a definitive + denial rather than an unreadable permission, so the default `enforce` policy does + not honor ignore commands from users outside the repository. +- GitLab notes carry no equivalent field, so project membership is read once per + run (only when an ignore command is present) and Developer or above is required. + If that lookup cannot be answered โ€” a `CI_JOB_TOKEN` generally cannot read the + members API โ€” the command is still honored and a warning names the author, so + enabling this does not silently break pipelines that relied on ignore commands. + Use a `GITLAB_TOKEN` with API read access to get enforcement. +- A rejected command is logged and is also absent from the ignore telemetry, which + records what was acted on. No acknowledgement reaction is added to a comment that + was not honored. +- `--ignore-authorization` selects the policy: `enforce` (default) requires write + access and honors the command with a warning where the provider cannot report it, + `strict` rejects it in that case instead, and `off` performs no check. + +### Fixed: GitLab authentication fallback never ran + +- When a GitLab token's type cannot be inferred from its shape, the CLI guesses + between Bearer and PRIVATE-TOKEN and retries once under the other scheme on a + 401. That retry never happened: the retry caught `requests.exceptions.HTTPError`, + but the HTTP client translates every request error into `APIFailure` first, so a + misclassified token failed the run instead of falling back. +- API failures raised by the CLI's HTTP client now carry their HTTP status code. + Without it a 401 was indistinguishable from any other failure, and + `is_transient_error` could not classify one either. +- The CLI's `APIFailure` now subclasses the SDK exception of the same name. They + were independent types, so an `except APIFailure` importing the SDK's โ€” which is + what every handler in `socketsecurity.core` does โ€” did not catch a failure raised + by the HTTP client. + +### Fixed: pull request and merge request comment accuracy + +- Per-alert ignore instructions now use ecosystem-qualified package names and + accept scoped packages while remaining compatible with older bare-name replies. + A leading npm scope is no longer mistaken for an ecosystem, so + `ignore @types/node@*` no longer also ignores the package named `node`. +- Ignore telemetry uses the same package matcher as alert suppression, so legacy + bare-name commands generate an event for the alert they suppress. +- Dependency overviews preserve added, updated, removed, and replaced package + classifications instead of presenting updates as new dependencies. Added and + updated rows keep their diff badge; removed and replaced, which have no + published badge, use a text label. +- Shared security comment copy no longer describes GitLab merge request output + as Socket for GitHub. +- Updating a security comment in the legacy table format no longer raises on a + malformed row. Each row was unpacked through four consecutive splits with no + bounds checks, so a cell carrying an extra `|`, a package cell that is not a + markdown link, or a name with no version ended the run before it reported + status โ€” and a scoped package name in Socket's own table was enough to trigger + it. Rows are now parsed defensively, and a row that cannot be read keeps its + alert reported. Ignore commands for a scoped package are accepted there in both + the ecosystem-qualified and bare forms. +- Server URLs read from `GITHUB_SERVER_URL` and `CI_SERVER_URL` are validated as + http(s) URLs before being composed into a diff scan's external link, matching + the check already applied to the other repository URLs read from CI. +- Repository-derived values are escaped before they are rendered into a pull + request or merge request comment. Manifest paths and sources are file paths from + the scanned repository, and alert text comes from the API; neither is markup the + CLI authored, so both are now escaped at the point they are interpolated. The + alert markers can no longer be terminated early by a package name. Slack, Jira + and console output are unchanged, since none of them render HTML. +## 2.8.3 + +### Fixed: GitLab report serialization and workspace baselines + +- Full-scan package identities and Socket links now preserve namespaced packages + when the SDK returns enum-backed ecosystem values. +- Namespaced package links separate the namespace from the name instead of + concatenating them, so Maven links no longer fuse groupId and artifactId into a + single unresolvable path segment. A namespaced package whose namespace is + missing now logs a warning rather than emitting a broken link silently. +- GitLab dependency-scanning reports emit CVE and GHSA identifiers from current + API fields while remaining compatible with legacy CVE data. +- GitLab report findings record the manifest they came from when the package's + introducing chain is unavailable, instead of reporting the location as + `unknown`, and report whether a dependency is direct from the package record + rather than inferring it from a dependency-path string that is never produced. +- `--base-commit-sha` degrades to the nearest scanned ancestor of the requested + commit instead of failing the run, and logs which commit was used and how far + back it is. Squash merges, rebases, and multi-commit pushes all leave a merge + base unscanned even when default-branch scanning is configured correctly. The + lookup follows paginated scan history and the run still fails when no scanned + ancestor is reachable or the exact-commit lookup itself fails. +- Implicit diff baselines are selected from the same workspace, scan type, + repository, and default branch, including when no workspace is supplied. A + baseline lookup that fails is reported as an API error instead of resolving to + an empty baseline, and temporary scans are skipped when selecting one. +## 2.8.2 + +### Changed: bump pinned @coana-tech/cli to 15.10.41 + +- Bumped the pinned reachability engine (`@coana-tech/cli`) from `15.10.40` to + `15.10.41`. See the + [reachability analysis changelog](https://docs.socket.dev/docs/reachability-analysis-changelog) + for engine changes. + +## 2.8.1 + +### Changed: bump pinned @coana-tech/cli to 15.10.40 + +- Bumped the pinned reachability engine (`@coana-tech/cli`) from `15.10.39` to + `15.10.40`. See the + [reachability analysis changelog](https://docs.socket.dev/docs/reachability-analysis-changelog) + for engine changes. + +## 2.8.0 + +### Changed: improve monorepo scan diagnostics and guidance + +- Added aggregate scan configuration, manifest-count, baseline-selection, and + fallback diagnostics without listing submitted manifest paths. +- Clarified monorepo scan scoping, workspace flags, CI path filters, and timeout + behavior, with a changed-workspace GitHub Actions example. + +### Changed: bump socketdev to 3.6.0 + +- Bumped the pinned SDK (`socketdev`) from `3.5.0` to `3.6.0`. Its package-type + enum gained ten members โ€” `alpm`, `chrome`, `clawhub`, `edge-extension`, + `firefox-extension`, `qpkg`, `socket`, `swid`, `vscode` and + `vscode-extension` โ€” so artifacts of those types are now reported under their + own type instead of falling back to `unknown`. + +### Fixed: apply configured exit codes to API failures + +- Full-scan and streamed-diff API failures now use the configured infrastructure + error exit code instead of the security-finding exit code. + +### Fixed: mid-severity findings were dropped from the Slack summary + +- The Slack reachability formatter keyed every severity lookup on `medium`, + but the API sends `middle`. A mid-severity finding therefore missed all of + them at once: it was not counted, so the summary always read `Medium: 0`; it + was excluded from `total_findings`, which can drive the "and N more" count + negative; and it sorted at the default order of 4, below `low`, so it was the + first thing truncated when the Slack block limit was reached. +- Severity is now normalized to one spelling when an alert is read, matching + how the GitLab and PR-comment paths already handle both forms. The findings + themselves were always listed; only the counts, ordering and truncation were + wrong. + +## 2.7.2 + +### Changed: bump pinned @coana-tech/cli to 15.10.39 + +- Bumped the pinned reachability engine (`@coana-tech/cli`) from `15.10.36` to + `15.10.39`. See the [Coana changelogs](https://docs.coana.tech/changelogs) for + engine changes. + +## 2.7.1 + +### Changed: bump pinned @coana-tech/cli to 15.10.36 + +- Bumped the pinned reachability engine (`@coana-tech/cli`) from `15.10.32` to + `15.10.36`. See the [Coana changelogs](https://docs.coana.tech/changelogs) for + engine changes. + +## 2.7.0 + +### Fixed: unreadable reachability facts no longer report a blocking package + +- Scans with no supported manifest files uploaded a zero-byte `.socket.facts.json` + placeholder. The API cannot parse that, and answers by adding a + `generic/invalid-socket-facts@1.0.0` artifact to the scan, which the CLI then reported + as a new blocking package with no manifest file and no introducing dependency โ€” + failing the run and, on pull requests, leaving a security comment that could not be + acted on. The placeholder is now an empty but well-formed facts document. +- When the API does report `generic/invalid-socket-facts` (a diagnostic for a facts file + it could not parse, not a real dependency), the CLI now excludes it from scan results + and logs a warning instead. It no longer blocks a run, appears in reports, or triggers + a pull request comment. +- Each placeholder is written to its own temporary directory. Two CLI runs sharing a + temporary directory previously used the same path and could remove each other's + placeholder mid-upload. + +### Fixed: pull request comments no longer show orphaned tags or an empty table + +- Optional sections that rendered as empty, such as the ignore instructions + suppressed by `--disable-ignore`, left a whitespace-only line in the alerts + table. That line closed the surrounding HTML block, and the indented + `` tags after it were rendered as a literal code block. + Generated comment markup now omits blank lines and stays under the indentation + that starts a code block. +- Alert descriptions, suggestions and license findings are collapsed onto a + single line so multi-line API text cannot break the table markup either. +- When a pull request has no alerts left to report, the security comment is + replaced with a short confirmation instead of keeping the "Caution" banner + above a table with no rows. This happens both when a later commit resolves + every alert and when every alert is ignored by comment. The comment marker is + preserved, so a commit that reintroduces an alert updates the same comment + rather than posting a second one. +- `@SocketSecurity ignore-all` now applies to comments written by CLI versions + before 2.0.55, which use the older Markdown alerts table. The check was made + once per ignore command, and an ignore-all comment produces none, so no rows + were removed. + +### Fixed: `--disable-security-issue` and `--disable-overview` now suppress the comment entirely + +- 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 being rewritten, since a body claiming + no alerts would be inaccurate when reporting is merely switched off. +- The decision moved into `should_write_comment()` so it is covered directly by + tests. + +## 2.6.11 + +### Changed: bump pinned @coana-tech/cli to 15.10.32 + +- Bumped the pinned reachability engine (`@coana-tech/cli`) from `15.10.28` to + `15.10.32`. See the [Coana changelogs](https://docs.coana.tech/changelogs) for + engine changes. + +## 2.6.10 + +### Changed: bump pinned @coana-tech/cli to 15.10.28 + +- Bumped the pinned reachability engine (`@coana-tech/cli`) from `15.10.27` to + `15.10.28`. See the [Coana changelogs](https://docs.coana.tech/changelogs) for + engine changes. + +## 2.6.9 + +### Changed: bump pinned @coana-tech/cli to 15.10.27 + +- Bumped the pinned reachability engine (`@coana-tech/cli`) from `15.10.25` to + `15.10.27`. See the [Coana changelogs](https://docs.coana.tech/changelogs) for + engine changes. + +## 2.6.8 + +### Changed: bump pinned @coana-tech/cli to 15.10.25 + +- Bumped the pinned reachability engine (`@coana-tech/cli`) from `15.10.23` to + `15.10.25`. See the [Coana changelogs](https://docs.coana.tech/changelogs) for + engine changes. +## 2.6.7 + +### Changed: bump pinned @coana-tech/cli to 15.10.23 + +- Bumped the pinned reachability engine (`@coana-tech/cli`) from `15.10.16` to + `15.10.23`. See the [Coana changelogs](https://docs.coana.tech/changelogs) for + engine changes. + +## 2.6.6 + +### Changed: faster local scan setup for large repositories + +- Manifest discovery now uses one filesystem walk per scan root and prunes + excluded directories before descent. +- Pull request scans use local Git refs first and fetch only missing history. + Buildkite pull request metadata is now supported directly. +- Supported manifest patterns are cached per invocation, and discovered + manifests are reused during scan creation. +- Added timings for initialization, Git operations, changed-file detection, + pattern lookup, and manifest discovery. + +### Changed: scan comparisons no longer fetch unused artifacts + +- Scan comparisons omit unchanged artifacts unless an enabled output needs them. +- Diff scans poll more frequently and log identifiers and timing details for + easier troubleshooting. +- Documented the `diff-scans:create`, `diff-scans:list` and `full-scans:list` + token scopes required by the optimized comparison path. + +## 2.6.5 + +### Changed: bump pinned @coana-tech/cli to 15.10.16 + +- Bumped the pinned reachability engine (`@coana-tech/cli`) from `15.10.13` to + `15.10.16`. See the [Coana changelogs](https://docs.coana.tech/changelogs) for + engine changes. + +## 2.6.4 + +### Changed: bump pinned @coana-tech/cli to 15.10.13 + +- Bumped the pinned reachability engine (`@coana-tech/cli`) from `15.10.4` to + `15.10.13`. See the [Coana changelogs](https://docs.coana.tech/changelogs) for + engine changes. + +### Changed: bump pinned GitPython to 3.1.59 + +- Bumped `GitPython` from `3.1.57` to `3.1.59`, picking up the security fixes + released in GitPython 3.1.58 and 3.1.59. None of the affected GitPython APIs + are used by this CLI. + +## 2.6.3 + +### Fixed: SBOM fetch failures no longer produce empty reports + +- `Core.get_sbom_data` now raises `APIFailure` when the full-scan stream fetch + fails, so the run exits through the CLI's API-error handling (exit code 3 by + default; `--disable-blocking` still exits 0) instead of writing empty + GitLab dependency-scanning, license, and SARIF reports. +- The underlying stream-parse failure was fixed in `socketdev` 3.4.2 (already + pinned to `3.5.0`): unrecognized purl types such as `generic` now resolve + instead of raising, and individual unparseable artifacts are skipped rather + than failing the whole response. + +### Changed: bump pinned @coana-tech/cli to 15.10.4 + +- Bumped the pinned reachability engine (`@coana-tech/cli`) from `15.10.3` to + `15.10.4`. See the [Coana changelogs](https://docs.coana.tech/changelogs) for + engine changes. + +### Changed: dependency updates and dev/test dependency pinning + +- Bumped pinned runtime dependencies (`beautifulsoup4` `4.15.0`, + `markdown` `3.10.3`). +- Pinned the `test` and `dev` dependency groups to exact versions, extending + the exact-pinning policy introduced in 2.6.0 to all dependency groups. +- Updated GitHub Actions used in CI workflows. No effect on the installed + CLI. + +## 2.6.1 + +### Changed: scan comparison now polls the diff-scans endpoints + +- Diff mode no longer holds a single idle HTTP connection open while the API + computes the scan comparison. The CLI now creates a diff-scan resource + (`POST /orgs/{org}/diff-scans/from-ids`) and polls + `GET /orgs/{org}/diff-scans/{id}?cached=true` with short, bounded requests + until the comparison is ready (HTTP 200 instead of 202). This fixes + intermittent `Connection reset by peer` failures on the final comparison + step when scans take several minutes to compare and network middleboxes + (e.g. Azure NAT gateways, which default to a 4-minute TCP idle timeout) + reap the idle connection. +- Duplicate scan pairs are resolved after an HTTP 409 and then polled through + the same cached endpoint. This avoids automatically following the API's 302 + duplicate redirect with an uncached, potentially long-lived GET request. +- The change is transparent: no flags or workflow changes are needed. If the + org API token is missing the `diff-scans:create`, `diff-scans:list` or + `full-scans:list` scopes โ€” or the new flow fails for any other reason โ€” the + CLI logs a warning and falls back to the legacy streaming comparison. +- Requires the pinned `socketdev==3.5.0` SDK. + +## 2.6.0 + +### Changed: pin all Python dependencies + +- Pinned every runtime dependency in `pyproject.toml` to an exact version; + several were previously unpinned or open ranges. +- Replaced the `bs4` shim package with a direct, pinned `beautifulsoup4` + dependency (the shim provided no version control over the actual library). +- Pinned the bundled `socketdev` SDK to `3.5.0` (previously `>=3.3.0,<4.0.0`). +- Docker images now install Python dependencies from the committed `uv.lock` + with pip hash verification (`--require-hashes`), so image builds no longer + resolve dependency versions from PyPI at build time. `pip check` validates + the environment after install. +- Pinned the `hatchling` build backend and the `uv` binary used in the + Dockerfile. + +### Changed: e2e reachability jobs retry transient empty results + +- Reachability e2e runs that report success with no alerted components in the + facts file are retried up to three times as a suspected transient backend + failure. After retries, only the known zero-project backend signature is + classified as inconclusive โ€” any other empty result still fails โ€” and e2e + jobs upload their logs and reports as diagnostics on failure. + +## 2.5.9 + +### Changed: bump pinned @coana-tech/cli to 15.10.3 + +- Bumped the pinned reachability engine (`@coana-tech/cli`) from `15.10.2` to + `15.10.3`. See the [Coana changelogs](https://docs.coana.tech/changelogs) for + engine changes. + +## 2.5.8 + +### Changed: bump pinned @coana-tech/cli to 15.10.2 + +- Bumped the pinned reachability engine (`@coana-tech/cli`) from `15.10.0` to + `15.10.2`. See the [Coana changelogs](https://docs.coana.tech/changelogs) for + engine changes. + +## 2.5.7 + +### Changed: bump pinned @coana-tech/cli to 15.10.0 + +- Bumped the pinned reachability engine (`@coana-tech/cli`) from `15.9.6` to + `15.10.0`. See the [Coana changelogs](https://docs.coana.tech/changelogs) for + engine changes. + +## 2.5.6 + +### Changed: bump pinned @coana-tech/cli to 15.9.6 + +- Bumped the pinned reachability engine (`@coana-tech/cli`) from `15.9.5` to + `15.9.6`. See the [Coana changelogs](https://docs.coana.tech/changelogs) for + engine changes. + +## 2.5.5 + +### Changed: bump pinned @coana-tech/cli to 15.9.5 + +- Bumped the pinned reachability engine (`@coana-tech/cli`) from `15.9.4` to + `15.9.5`. See the [Coana changelogs](https://docs.coana.tech/changelogs) for + engine changes. + ## 2.5.4 ### Changed: bump pinned @coana-tech/cli to 15.9.4 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 00000000..ed9ee2b0 --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,62 @@ +# Contributing + +## Development setup + +Use Python 3.11 or newer and install +[`uv`](https://docs.astral.sh/uv/getting-started/installation/). From the +repository root, create the environment and install all development +dependencies: + +```bash +uv sync --all-extras +``` + +Before opening a pull request, run: + +```bash +make test +uv run hatch build +uv run python -m twine check dist/* +``` + +To develop against a local SDK checkout, set `SOCKET_SDK_PATH` if it is not at +`../socketdev`, then run `make first-time-local-setup`. + +## Pull request validation + +The `Package Check` workflow runs automatically for pull requests. It builds +and validates the distributions, smoke-tests the wheel, and uploads the +distributions as workflow artifacts. It does not publish a package or Docker +image. + +## Publishing pull request previews + +Preview publication is intentionally opt-in. Only request previews for code +that is trusted to run with the repository's publishing permissions. + +For a pull request from this repository, apply the label for the artifact that +needs testing: + +- `publish-preview` publishes a uniquely versioned `socketsecurity` prerelease + to TestPyPI and adds or updates a pull request comment with the exact version + and installation command. +- `publish-docker-preview` publishes the mutable + `socketdev/cli:pr-` image to Docker Hub and adds or + updates a pull request comment with the image tag. + +Both label-triggered and manually dispatched previews are limited to open pull +requests whose branches belong to this repository. Each label is handled as a +separate event, so applying both labels starts two workflow runs. Use manual +dispatch instead when both artifacts should be published in a single run. + +The workflow reacts when a label is added; pushing another commit while the +label remains on the pull request does not publish a new preview. To publish the +new pull request head or retry a failed publication, remove the relevant label +and apply it again. + +Maintainers can also open **Actions > Publish PR Preview > Run workflow**, run +it from the repository's default branch, enter the pull request number, and +choose whether to publish to TestPyPI, Docker Hub, or both. When testing the CLI +against an SDK preview, enter the exact TestPyPI `socketdev` prerelease in +`sdk_preview_version`; publish the SDK preview first and allow time for +TestPyPI to expose it before starting the CLI Docker preview. diff --git a/Dockerfile b/Dockerfile index 75110780..06e8b0a0 100644 --- a/Dockerfile +++ b/Dockerfile @@ -86,7 +86,7 @@ ENV PATH="/usr/local/go/bin:/usr/lib/go/bin:/root/.cargo/bin:${PATH}" ENV GOPATH="/go" # Install uv -COPY --from=ghcr.io/astral-sh/uv:latest /uv /usr/local/bin/uv +COPY --from=ghcr.io/astral-sh/uv:0.10.4 /uv /usr/local/bin/uv # Install pyenv # pyenv lets us build/install arbitrary Python versions on demand. We install @@ -111,18 +111,39 @@ RUN curl -L https://raw.githubusercontent.com/pyenv/pyenv-installer/master/bin/p ln -s ~/.pyenv/bin/pyenv /bin/pyenv && \ pyenv --version +# Install Python dependencies from the lockfile with hash verification so the +# image never resolves loose versions from PyPI at build time. +COPY pyproject.toml uv.lock /tmp/socket-cli-lock/ +# Index flags are passed explicitly (always production PyPI) so the +# PIP_INDEX_URL/PIP_EXTRA_INDEX_URL ARGs used to point CLI/SDK preview installs +# at TestPyPI don't leak into the locked dependency install via pip's env vars. +RUN uv export --directory /tmp/socket-cli-lock --frozen --no-dev --no-emit-project \ + --format requirements-txt -o /tmp/socket-cli-lock/requirements.txt && \ + pip install --require-hashes --no-deps \ + --index-url https://pypi.org/simple --extra-index-url https://pypi.org/simple \ + -r /tmp/socket-cli-lock/requirements.txt + # Install CLI based on build mode RUN if [ "$USE_LOCAL_INSTALL" = "true" ]; then \ echo "Using local development install"; \ else \ + cli_installed=false; \ for i in $(seq 1 10); do \ echo "Attempt $i/10: Installing socketsecurity==$CLI_VERSION"; \ - if pip install --index-url ${PIP_INDEX_URL} --extra-index-url ${PIP_EXTRA_INDEX_URL} socketsecurity==$CLI_VERSION; then \ + if pip install --no-deps --index-url ${PIP_INDEX_URL} --extra-index-url ${PIP_EXTRA_INDEX_URL} socketsecurity==$CLI_VERSION; then \ + cli_installed=true; \ break; \ fi; \ - echo "Install failed, waiting 30s before retry..."; \ - sleep 30; \ - done && \ + if [ "$i" -lt 10 ]; then \ + echo "Install failed, waiting 30s before retry..."; \ + sleep 30; \ + fi; \ + done; \ + if [ "$cli_installed" != "true" ]; then \ + echo "Failed to install socketsecurity==$CLI_VERSION after 10 attempts"; \ + exit 1; \ + fi; \ + pip check || exit 1; \ if [ ! -z "$SDK_VERSION" ]; then \ pip install --index-url ${PIP_INDEX_URL} --extra-index-url ${PIP_EXTRA_INDEX_URL} socketdev==${SDK_VERSION}; \ fi; \ @@ -132,8 +153,7 @@ RUN if [ "$USE_LOCAL_INSTALL" = "true" ]; then \ COPY . /app WORKDIR /app RUN if [ "$USE_LOCAL_INSTALL" = "true" ]; then \ - pip install --upgrade -e .; \ - pip install --upgrade socketdev; \ + pip install --no-deps -e . && pip check; \ fi # Create workspace directory with proper permissions diff --git a/Dockerfile.preview b/Dockerfile.preview new file mode 100644 index 00000000..62cb3ce7 --- /dev/null +++ b/Dockerfile.preview @@ -0,0 +1,68 @@ +# syntax=docker/dockerfile:1 + +FROM socketdev/cli:latest + +ARG SDK_PREVIEW_VERSION="" +ARG PYPI_INDEX_URL=https://pypi.org/simple/ +ARG TEST_PYPI_INDEX_URL=https://test.pypi.org/simple/ + +COPY dist/socketsecurity-*.whl /tmp/socket-preview/ + +RUN <<'SH' +set -eux + +wheel=$(find /tmp/socket-preview -maxdepth 1 -name 'socketsecurity-*.whl' -print -quit) + +# Resolve every wheel dependency from production PyPI. When an exact SDK +# preview is requested, leave socketdev out so the prerelease can intentionally +# override a final-version floor such as socketdev>=3.4.0. +python - "$wheel" > /tmp/socket-preview/requirements.txt <<'PY' +import email +import os +import sys +import zipfile + +from packaging.requirements import Requirement +from packaging.utils import canonicalize_name + +wheel_path = sys.argv[1] +with zipfile.ZipFile(wheel_path) as archive: + metadata_path = next( + name for name in archive.namelist() if name.endswith(".dist-info/METADATA") + ) + metadata = email.message_from_bytes(archive.read(metadata_path)) + +sdk_preview = os.environ.get("SDK_PREVIEW_VERSION") +for value in metadata.get_all("Requires-Dist", []): + if sdk_preview and canonicalize_name(Requirement(value).name) == "socketdev": + continue + print(value) +PY + +python -m pip install \ + --no-cache-dir \ + --index-url "$PYPI_INDEX_URL" \ + --requirement /tmp/socket-preview/requirements.txt + +if [ -n "$SDK_PREVIEW_VERSION" ]; then + mkdir /tmp/socket-preview/sdk + python -m pip download \ + --no-cache-dir \ + --no-deps \ + --dest /tmp/socket-preview/sdk \ + --index-url "$TEST_PYPI_INDEX_URL" \ + "socketdev==$SDK_PREVIEW_VERSION" + python -m pip install \ + --no-cache-dir \ + --index-url "$PYPI_INDEX_URL" \ + /tmp/socket-preview/sdk/socketdev-*.whl +fi + +python -m pip install \ + --no-cache-dir \ + --no-deps \ + --force-reinstall \ + "$wheel" +socketcli --help >/dev/null +rm -rf /tmp/socket-preview +SH diff --git a/README.md b/README.md index cda9a407..5221daf1 100644 --- a/README.md +++ b/README.md @@ -44,21 +44,22 @@ socketcli --enable-gitlab-security --gitlab-security-file gl-dependency-scanning ### PR scan diffed against the merge base -By default, PR scans are diffed against the repository's latest head scan. To diff against -the exact commit your PR branched from instead, pass the merge base as the baseline: +By default, PR scans are diffed against the repository's latest matching head scan. To +prefer the commit your PR branched from as the baseline, pass the merge base: ```bash BASE_SHA=$(git merge-base origin/main HEAD) socketcli --pr-number 123 --base-commit-sha "$BASE_SHA" ``` -> **Requirement:** `--base-commit-sha` only works if Socket already has a full scan for that -> exact commit. In practice this means your CI must run `socketcli` on **every commit that -> lands on your default branch** โ€” not just some of them. If merges can land without a scan -> (skipped/canceled builds, `[skip ci]`, path-filtered pipelines), the PR scan will fail with -> exit code 3 rather than silently diff against the wrong baseline. See +> The CLI uses the exact commit's newest matching full scan when one exists. Otherwise, it +> searches up to 100 first-parent commits in the local checkout and uses the nearest scanned +> ancestor, with a warning that the diff is wider than the merge base. Run `socketcli` +> regularly on your default branch and ensure PR checkouts contain enough history for that +> walk. The run fails with the configured API-error exit code only when no scanned ancestor +> is reachable (or when the scan lookup itself fails). See > [`docs/cli-reference.md`](https://github.com/SocketDev/socket-python-cli/blob/main/docs/cli-reference.md) -> for the full requirements and a backfill pattern that makes PR jobs self-sufficient. +> for the full behavior and an optional exact-baseline backfill pattern. A specific full scan ID also works: `--base-scan-id `. @@ -229,6 +230,10 @@ value โ€” e.g. a Buildkite code, or `0` to swallow infra errors. Exit `3` is a Socket convention, not an industry standard. +This mapping applies to errors the CLI receives and handles. An external process +supervisor (for example GNU `timeout`) can terminate the CLI before it handles an +error, so the supervisor's exit status (commonly 124 or 137) takes precedence. + ### How these options interact The two flags that affect exit codes can cancel each other out, so the order of diff --git a/benchmarks/manifest_discovery.py b/benchmarks/manifest_discovery.py new file mode 100644 index 00000000..abc0e8bb --- /dev/null +++ b/benchmarks/manifest_discovery.py @@ -0,0 +1,101 @@ +#!/usr/bin/env python3 +"""Compare legacy per-pattern rglob discovery with the single-pass walker. + +This is an opt-in developer benchmark, not a timing assertion in the test +suite. It creates a synthetic monorepo so filesystem or CI-agent changes do not +make regular tests flaky. +""" + +import argparse +import tempfile +import time +from pathlib import Path +from types import SimpleNamespace +from unittest.mock import MagicMock + +from socketsecurity.core import Core +from socketsecurity.core.socket_config import SocketConfig +from socketsecurity.core.utils import socket_globs + + +def seed_tree(root: Path, directories: int, files_per_directory: int) -> None: + for directory_index in range(directories): + directory = root / "packages" / f"package-{directory_index:05d}" + directory.mkdir(parents=True) + (directory / "package.json").write_text("{}\n", encoding="utf-8") + for file_index in range(files_per_directory): + (directory / f"source-{file_index:03d}.txt").write_text( + "not a manifest\n", + encoding="utf-8", + ) + + # These trees model the expensive directories that the new walker prunes + # before descent rather than visiting once for every manifest pattern. + for excluded in (".git/objects", "node_modules/example", ".venv/site-packages"): + directory = root / excluded + directory.mkdir(parents=True) + for index in range(files_per_directory * 10): + (directory / f"object-{index:05d}").write_text("x", encoding="utf-8") + + +def legacy_discover(root: Path) -> set[str]: + results = set() + excluded_dirs = SocketConfig(api_key="benchmark").excluded_dirs + for ecosystem_patterns in socket_globs.values(): + for details in ecosystem_patterns.values(): + for pattern in Core.expand_brace_pattern(details["pattern"]): + insensitive = Core.to_case_insensitive_regex(pattern) + for candidate in root.rglob(insensitive): + if candidate.is_file() and not Core.is_excluded( + str(candidate), + excluded_dirs, + ): + results.add(candidate.as_posix()) + return results + + +def new_core() -> Core: + core = Core.__new__(Core) + core.config = SocketConfig(api_key="benchmark") + core.cli_config = SimpleNamespace(exclude_paths=None) + core.sdk = MagicMock() + core._supported_patterns = socket_globs + return core + + +def timed(function, root: Path) -> tuple[set[str], float]: + start = time.perf_counter() + results = set(function(root)) + return results, time.perf_counter() - start + + +def main() -> None: + parser = argparse.ArgumentParser() + parser.add_argument("--directories", type=int, default=500) + parser.add_argument("--files-per-directory", type=int, default=20) + args = parser.parse_args() + + with tempfile.TemporaryDirectory(prefix="socket-manifest-benchmark-") as temp: + root = Path(temp) + seed_tree(root, args.directories, args.files_per_directory) + legacy_results, legacy_seconds = timed(legacy_discover, root) + new_results, new_seconds = timed( + lambda path: new_core().find_files(str(path)), + root, + ) + + if legacy_results != new_results: + raise SystemExit( + "Manifest result mismatch: " + f"legacy={len(legacy_results)}, single_pass={len(new_results)}" + ) + + speedup = legacy_seconds / new_seconds if new_seconds else float("inf") + print(f"Manifests: {len(new_results)}") + print(f"Legacy per-pattern rglob: {legacy_seconds:.3f}s") + print(f"Single-pass walk: {new_seconds:.3f}s") + print(f"Speedup: {speedup:.1f}x") + + +if __name__ == "__main__": + main() diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 66193f38..253c27ca 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -2,6 +2,10 @@ Use this guide for pipeline-focused CLI usage across platforms. +The shell commands in the recommended patterns are CI-provider neutral. Buildkite +pipeline equivalents and provider-specific considerations are called out alongside +the relevant guidance below. + ## Recommended patterns ### Dashboard-style reachable SARIF @@ -27,6 +31,27 @@ socketcli \ --strict-blocking ``` +### Buildkite: retain SARIF as a build artifact + +Either recommended pattern can run directly in a Buildkite command step. When the +scan writes SARIF, add +[`artifact_paths`](https://buildkite.com/docs/pipelines/configure/artifacts#upload-artifacts-with-a-command-step) +so developers can download the report from the build after the command finishes: + +```yaml +steps: + - label: ":socket: Socket reachable diff" + command: | + socketcli \ + --reach \ + --sarif-file results.sarif \ + --sarif-scope diff \ + --sarif-reachability reachable \ + --strict-blocking + artifact_paths: + - "results.sarif" +``` + ## Config file usage in CI Use `--config .socketcli.toml` or `--config .socketcli.json` to keep pipeline commands small. @@ -60,6 +85,9 @@ Equivalent JSON: } ``` +The Buildkite examples below use the same checked-in `.socketcli.toml` file; no +Buildkite-specific config-file format is required. + ## Platform examples ### GitHub Actions @@ -71,16 +99,282 @@ Equivalent JSON: SOCKET_SECURITY_API_TOKEN: ${{ secrets.SOCKET_SECURITY_API_TOKEN }} ``` +#### GitHub Actions: scan changed monorepo workspaces independently + +GitHub Actions `paths` filters only decide whether a workflow starts. They do not +change `socketcli` discovery or upload scope. For a merge gate, it is usually safer +to start a small selector job on every PR update, then create one scan job per +affected logical workspace. This also avoids a required check remaining pending +when GitHub skips the entire workflow because of a top-level path filter. + +This pattern produces one dashboard entry per logical workspace, which is what +gives each component its own alerts, baseline, and policy. It is also the layout +that grows the dashboard's repository list. See +[Choosing a scan layout](cli-reference.md#choosing-a-scan-layout) for when that +trade-off is worth making. + +Define a repository variable named `SOCKET_MONOREPO_WORKSPACES_JSON`. Its value is +an array with one stable workspace name, one or more scan roots, and the path globs +that should select that workspace. Fill these placeholders with the repository's +real layout. A workspace definition selects directory roots; shared root manifests, +lockfiles, and cross-directory path dependencies outside those roots are not included +automatically. + +```json +[ + { + "name": "", + "sub_paths": [""], + "watch_globs": [""] + } +] +``` + +Each `sub_paths` value must be a directory, not an individual manifest or lockfile. +Using `.` includes the entire target path. Do not use this changed-workspace pattern +until the directory boundaries preserve every shared input needed to resolve each +logical graph. If root workspace metadata governs most or all of the repository, a +smaller coverage-preserving split may not be representable with `--sub-path` alone. + +Also define `SOCKETCLI_VERSION` as the exact package version validated for the +workflow. The workflow below logs that version, uses full Git history for reliable +base/head selection, creates one matrix job (and therefore one graph and baseline) +per selected workspace, and fails closed on CLI/API/timeout failures. It uses API +SCM mode plus `--enable-diff` because parallel `--scm github` jobs can race while +updating the same PR comments; the matrix checks and report links are the gate. + +```yaml +name: Socket Security + +on: + pull_request: + types: [opened, synchronize, reopened] + push: + branches: [main] + +permissions: + contents: read + +jobs: + select-workspaces: + runs-on: ubuntu-latest + outputs: + count: ${{ steps.select.outputs.count }} + matrix: ${{ steps.select.outputs.matrix }} + steps: + - uses: actions/checkout@v5 + with: + fetch-depth: 0 + persist-credentials: false + + - id: select + name: Select changed workspaces + env: + WORKSPACES_JSON: ${{ vars.SOCKET_MONOREPO_WORKSPACES_JSON }} + BASE_SHA: ${{ github.event.pull_request.base.sha || github.event.before }} + HEAD_SHA: ${{ github.event.pull_request.head.sha || github.sha }} + shell: bash + run: | + python - <<'PY' + import fnmatch + import json + import os + import re + import subprocess + + workspaces = json.loads(os.environ["WORKSPACES_JSON"]) + if not isinstance(workspaces, list): + raise SystemExit("SOCKET_MONOREPO_WORKSPACES_JSON must be a JSON array") + + base = os.environ["BASE_SHA"] + head = os.environ["HEAD_SHA"] + if not base or set(base) == {"0"}: + base = subprocess.check_output( + ["git", "rev-parse", f"{head}^"], text=True + ).strip() + changed_output = subprocess.check_output( + ["git", "diff", "--name-only", "-z", base, head] + ) + changed = [ + item.decode("utf-8", "surrogateescape") + for item in changed_output.split(b"\0") + if item + ] + + selected = [] + for workspace in workspaces: + name = workspace.get("name", "") + sub_paths = workspace.get("sub_paths") or [] + watch_globs = workspace.get("watch_globs") or [] + if not re.fullmatch(r"[A-Za-z0-9._-]+", name): + raise SystemExit(f"Invalid workspace name: {name!r}") + if not sub_paths or any( + not isinstance(path, str) + or path.startswith("/") + or ".." in path.split("/") + for path in sub_paths + ): + raise SystemExit(f"Invalid sub_paths for workspace {name!r}") + if not watch_globs: + watch_globs = [ + pattern + for path in sub_paths + for pattern in ( + ["*"] + if path.strip("/") in ("", ".") + else [path.rstrip("/"), f"{path.rstrip('/')}/*"] + ) + ] + if any( + fnmatch.fnmatchcase(path, pattern) + for path in changed + for pattern in watch_globs + ): + selected.append({"name": name, "sub_paths": sub_paths}) + + matrix = json.dumps({"include": selected}, separators=(",", ":")) + with open(os.environ["GITHUB_OUTPUT"], "a", encoding="utf-8") as output: + output.write(f"count={len(selected)}\n") + output.write(f"matrix={matrix}\n") + PY + + scan-workspace: + needs: select-workspaces + if: needs.select-workspaces.outputs.count != '0' + timeout-minutes: 20 + strategy: + fail-fast: false + matrix: ${{ fromJSON(needs.select-workspaces.outputs.matrix) }} + name: Socket scan (${{ matrix.name }}) + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v5 + with: + fetch-depth: 0 + persist-credentials: false + + - uses: actions/setup-python@v6 + with: + python-version: '3.12' + + - name: Install pinned Socket CLI + env: + SOCKETCLI_VERSION: ${{ vars.SOCKETCLI_VERSION }} + run: | + python -m pip install "socketsecurity==$SOCKETCLI_VERSION" + socketcli --version + + - name: Scan workspace + env: + SOCKET_SECURITY_API_KEY: ${{ secrets.SOCKET_SECURITY_API_KEY }} + PR_NUMBER: ${{ github.event.pull_request.number || 0 }} + WORKSPACE_NAME: ${{ matrix.name }} + SUB_PATHS_JSON: ${{ toJSON(matrix.sub_paths) }} + shell: bash + run: | + set +e + args=( + --target-path "$GITHUB_WORKSPACE" + --workspace-name "$WORKSPACE_NAME" + --enable-diff + --pr-number "$PR_NUMBER" + --exit-code-on-api-error 3 + --report-link-file socket-report-link.txt + --summary-file socket-summary.txt + ) + while IFS= read -r sub_path; do + args+=(--sub-path "$sub_path") + done < <(jq -r '.[]' <<<"$SUB_PATHS_JSON") + + socketcli "${args[@]}" 2>&1 | tee socket-output.log + code=${PIPESTATUS[0]} + + { + echo "## Socket scan: $WORKSPACE_NAME" + if [ -s socket-report-link.txt ]; then + echo "[View the report]($(cat socket-report-link.txt))" + fi + if [ -s socket-summary.txt ]; then + echo '```' + cat socket-summary.txt + echo '```' + fi + } >> "$GITHUB_STEP_SUMMARY" + + exit "$code" + + socket-security: + if: always() + needs: [select-workspaces, scan-workspace] + runs-on: ubuntu-latest + steps: + - name: Enforce matrix result + env: + SELECT_RESULT: ${{ needs.select-workspaces.result }} + SCAN_RESULT: ${{ needs.scan-workspace.result }} + run: | + test "$SELECT_RESULT" = success + [[ "$SCAN_RESULT" = success || "$SCAN_RESULT" = skipped ]] +``` + +Each configuration object may intentionally contain several `sub_paths` when +those directories are one logical dependency graph. To split backend resolution, +use separate objects with different `name` values. Add `--workspace ` only +when the Socket organization requires API workspace association; it is not a scan +scope control. Use `--save-submitted-files-list` in a non-required canary to verify +the exact manifests selected before adopting workspace-level scans as a merge gate. + +The job has an explicit 20-minute total budget. Tune that value from observed +workspace-level latency after the split; a five-minute cap can still be too close +to a slow request plus local startup. The CLI's `--timeout` is different: it +defaults to 1,200 seconds **per API request**. If an operator adds GNU `timeout`, +that process supervisor can terminate the CLI before it maps an error through +`--exit-code-on-api-error`; without `--preserve-status`, GNU reports 124 after its +initial timeout signal or 137 if `SIGKILL` is involved. + ### Buildkite +This example assumes a GitHub-hosted repository. Change +`SOCKET_SCM_INTEGRATION` to `gitlab` for a GitLab-hosted repository, or `api` +when provider association is not wanted. The doubled dollar signs defer +Buildkite variable expansion until the command runs on an agent. + ```yaml +env: + SOCKET_SCM_INTEGRATION: "github" + steps: - label: "Socket scan" - command: "socketcli --config .socketcli.toml --target-path ." - env: - SOCKET_SECURITY_API_TOKEN: "${SOCKET_SECURITY_API_TOKEN}" + command: | + socketcli \ + --config .socketcli.toml \ + --target-path . \ + --integration "$${SOCKET_SCM_INTEGRATION:-api}" \ + --pr-number "$${BUILDKITE_PULL_REQUEST:-0}" + secrets: + - SOCKET_SECURITY_API_TOKEN ``` +The `secrets` block expects a +[Buildkite secret](https://buildkite.com/docs/pipelines/security/secrets/buildkite-secrets) +named `SOCKET_SECURITY_API_TOKEN`. If your organization uses an external secrets +plugin or an agent hook instead, remove that block and inject the same environment +variable through your existing mechanism. Do not store the token in pipeline YAML. + +The CLI reads Buildkite's native `BUILDKITE_COMMIT`, `BUILDKITE_BRANCH`, +`BUILDKITE_PULL_REQUEST`, and `BUILDKITE_PULL_REQUEST_BASE_BRANCH` variables. +For pull-request builds, ensure the checkout contains the base branch and the +checked-out head commit. The CLI uses those local refs first and performs a +targeted fetch only when a required ref or its comparison history is missing; +it does not fetch every remote ref and tag during startup. + +When `--scm github` is used from Buildkite, the CLI also posts GitHub PR comments. +It identifies the repository from `BUILDKITE_REPO` and takes the rest of the build +context from `BUILDKITE_BUILD_CHECKOUT_PATH` and the variables above โ€” see +[Buildkite PR context](#buildkite-pr-context). Set `GH_API_TOKEN` to a GitHub token +with the required repository access. GitHub Enterprise users should also set +`GITHUB_API_URL`; GitHub.com defaults to `https://api.github.com`. + #### Merge-base baselines in Buildkite (dynamic pipelines) Notes for using `--base-commit-sha` (see the @@ -101,11 +395,14 @@ generator rather than a static YAML file: BASE_SHA=$(git merge-base "origin/${TARGET}" HEAD) ``` -- **Emit the backfill step conditionally from the generator.** The generator is the - natural place for the "does a baseline scan exist?" check +- **If an exact baseline is required, emit a backfill step conditionally from the + generator.** The generator is the natural place for the "does an exact baseline + scan exist?" check (`GET /orgs/{org}/full-scans?repo=&commit_hash=$BASE_SHA&per_page=1`): only emit the baseline-scan step when it returns nothing. The emitted pipeline then shows - in the UI whether a backfill will run. + in the UI whether a backfill will run. Without a backfill, the CLI automatically + uses the nearest scanned first-parent ancestor within 100 commits and warns that + the diff is wider. - **Keep the backfill inside one command step.** The checkout-base โ†’ scan โ†’ checkout-PR sequence must not be split across steps โ€” steps can land on different @@ -114,17 +411,18 @@ generator rather than a static YAML file: checkout: `git worktree add /tmp/socket-base "$BASE_SHA"` then `socketcli --target-path /tmp/socket-base --branch "$TARGET" --disable-blocking`. -- **Soft-fail infra errors, not findings.** A missing baseline (or any API error) - exits with code 3 (`--exit-code-on-api-error` to change it); real findings exit 1. +- **Soft-fail infra errors, not findings.** No reachable scanned ancestor (or any API + error) exits with code 3 (`--exit-code-on-api-error` to change it); real findings exit 1. [`soft_fail: [{exit_status: 3}]`](https://buildkite.com/docs/pipelines/configure/step-types/command-step) on the PR scan step keeps infra errors from blocking merges while security findings still do. - **["Cancel intermediate builds"](https://buildkite.com/docs/pipelines/configure/canceling-builds#cancel-running-intermediate-builds) - on the default branch is the main source of baseline gaps.** Canceled builds never - scan their commit, so merge-base lookups for PRs based on those commits fail. The - conditional backfill step above is the remedy; there is no per-step exemption from - build cancellation in Buildkite. If you need strict scan-once semantics for + on the default branch is a common source of exact-baseline gaps.** Canceled builds + never scan their commit, so these PRs fall back to an older scanned ancestor. Use + the conditional backfill step above when an exact merge-base comparison is required; + there is no per-step exemption from build cancellation in Buildkite. If you need + strict scan-once semantics for concurrent backfills of the same merge base, serialize the backfill step with a [concurrency group](https://buildkite.com/docs/pipelines/configure/workflows/controlling-concurrency) keyed on the merge-base SHA. @@ -139,6 +437,18 @@ socket_scan: SOCKET_SECURITY_API_TOKEN: $SOCKET_SECURITY_API_TOKEN ``` +### Azure Pipelines + +```yaml +- script: | + socketcli \ + --integration azure \ + --enable-diff \ + --target-path "$(Build.SourcesDirectory)" + env: + SOCKET_SECURITY_API_TOKEN: $(SOCKET_SECURITY_API_TOKEN) +``` + ### Bitbucket Pipelines ```yaml @@ -149,6 +459,69 @@ pipelines: - socketcli --config .socketcli.toml --target-path . ``` +## Scan type by pipeline + +With `--scm github` or `--scm gitlab`, the detected event decides the scan type: + +| Event | Scan | Blocks the build | +|:------|:-----|:-----------------| +| Pull request / merge request | Diff scan against the repository's baseline | Yes, on newly introduced alerts | +| Any other pipeline, including default-branch pushes | Full scan | No | + +A full scan has no baseline, so it cannot tell a newly introduced alert from one +that was already there. Rather than block on a number that would mean something +different depending on which output format was enabled, those runs behave as if +`--disable-blocking` was supplied and report through the Dashboard instead. This +matches how the CLI already treats a run with no supported manifest files. + +The event type is authoritative once `--scm` is set: `--enable-diff` and +`--ignore-commit-files` do not turn a branch pipeline into a comparison. To diff +a branch build, drop `--scm` and use `--enable-diff` with `--integration`, which +runs the comparison without the PR comment adapter. + +`--generate-license` and `--legal-format fossa` work on both paths; a full scan +fetches the package list for them. + +## Pull request and Dashboard association + +The CLI sends the resolved pull request number with each full scan and attaches +the pull request URL to diff scans so the Socket Dashboard can associate the +report with its originating change. If `--pr-number` is supplied, it wins; +passing `--pr-number 0` explicitly disables automatic association. Any value that +is not a positive integer, including Buildkite's `false`, means no pull request. + +Without an explicit value, the CLI recognizes: + +- GitHub Actions: `PR_NUMBER`, then the PR number in `GITHUB_REF`. +- GitLab CI: `CI_MERGE_REQUEST_IID`. +- Azure Pipelines: `SYSTEM_PULLREQUEST_PULLREQUESTNUMBER` for GitHub-hosted + repositories, otherwise `SYSTEM_PULLREQUEST_PULLREQUESTID` for Azure Repos. + +### Buildkite PR context + +Buildkite is SCM-provider neutral, so the CLI does not infer a provider or consume +its PR variable automatically. Pass Buildkite's +[`BUILDKITE_PULL_REQUEST`](https://buildkite.com/docs/pipelines/configure/environment-variables#BUILDKITE_PULL_REQUEST) +value to +`--pr-number` and identify the repository host with `--integration`, as shown in +the Buildkite platform example above. Buildkite sets `BUILDKITE_PULL_REQUEST` to +`false` outside PR builds; the CLI treats that value as no PR. + +Use `--integration github` for GitHub-hosted repositories and `--integration gitlab` +for GitLab-hosted ones. The CLI identifies the repository from +[`BUILDKITE_REPO`](https://buildkite.com/docs/pipelines/configure/environment-variables#BUILDKITE_REPO), +taking both the slug and the host from it, so github.com, GitLab.com, and self-hosted +installations all build a correct pull request or merge request link without extra +configuration. That same value identifies the repository for GitHub PR comments when +`--scm github` is set. `CI_PROJECT_URL` still overrides the derived GitLab project URL. +Keep `--scm api` unless you also intend to configure an existing GitHub or GitLab +comment adapter and its provider token. + +`--scm github` and `--scm gitlab` also imply the matching scan integration for +Dashboard metadata unless `--integration` was explicitly supplied. PR comments +remain limited to the existing GitHub and GitLab SCM adapters; Azure receives +console output and Dashboard association but does not post a PR comment. + ## Workflow templates Prebuilt examples in this repo: @@ -165,3 +538,11 @@ Prebuilt examples in this repo: - `--sarif-grouping alert` currently applies to `--sarif-scope full`. - Diff-based SARIF can validly be empty when there are no matching net-new alerts. - Keep API tokens in secret stores (`SOCKET_SECURITY_API_TOKEN`), not in config files. +- In Buildkite pipeline YAML, follow its + [runtime interpolation](https://buildkite.com/docs/pipelines/configure/environment-variables#runtime-variable-interpolation) + guidance and use `$$` for variables that must expand when the command runs rather + than when the pipeline is uploaded. +- Security findings with `props.firstPatchedVersionIdentifier` show that value in + the console table, including native Buildkite job logs, and in GitHub/GitLab + security comments when that SCM adapter is configured. Findings without a known + patched release leave the console cell blank and omit the comment field. diff --git a/docs/cli-reference.md b/docs/cli-reference.md index c120be34..c5d4f637 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -53,18 +53,67 @@ Pre-configured workflow files are in [`../workflows/`](../workflows/). > **Note:** If you're looking to associate a scan with a named Socket workspace (e.g. because your repo is identified as `org/repo`), see the [`--workspace` flag](#repository) instead. The `--workspace-name` flag described in this section is an unrelated monorepo feature. -The Socket CLI supports scanning specific workspaces within monorepo structures while preserving git context from the repository root. This is useful for organizations that maintain multiple applications or services in a single repository. +The Socket CLI supports scanning selected directories within a monorepo while preserving git context from the repository root. Scan scope is controlled by `--target-path` and `--sub-path`; CI workflow path filters and the CLI's changed-file detection do not narrow the manifests uploaded after a scan starts. ### Key Features -- **Multiple Sub-paths**: Specify multiple `--sub-path` options to scan different directories within your monorepo -- **Combined Workspace**: All sub-paths are scanned together as a single workspace in Socket +- **Target path**: Supplies repository/Git context and is the discovery root when no `--sub-path` is present +- **Multiple Sub-paths**: Restrict discovery to those directories, but combine every repeated `--sub-path` into one upload and one server-side dependency graph - **Git Context Preserved**: Repository metadata (commits, branches, etc.) comes from the main target-path -- **Workspace Naming**: Use `--workspace-name` to differentiate scans from different parts of your monorepo +- **Workspace Naming**: Use a stable, unique `--workspace-name` for each independently scanned logical workspace; it suffixes the repository slug and therefore gives that workspace its own repository head/baseline + +`--workspace` is different: it sends Socket organization workspace context with the full-scan API request. It does not narrow client-side filesystem discovery, split the upload into independent scans, or change the repository suffix. Backend policy/routing for that workspace remains server-owned. + +> **Performance consequence:** If the goal is smaller independently resolvable graphs, run one CLI invocation per logical workspace, with a distinct `--workspace-name`. Adding several unrelated directories to one command with repeated `--sub-path` flags still asks the backend to resolve one combined graph. + +Normal scan logs include the effective repository and Socket workspace context, +repository-relative discovery roots, aggregate manifest count, and selected baseline. +Individual manifest paths remain opt-in through `--save-submitted-files-list`. + +### Choosing a scan layout + +`--sub-path` and `--workspace-name` support two layouts, and picking between them +is a trade-off rather than a preference. There is no third option today. + +**One combined scan** โ€” a single invocation, no `--workspace-name`, with +`--target-path` at the repository root or several repeated `--sub-path` values +sharing one workspace name: + +- One dashboard entry for the repository, named after the repository +- One server-side dependency graph covering everything that was uploaded +- Alerts are **not** broken out by component, so a finding does not tell you which + part of the monorepo introduced it +- Transitive findings can surface without a clear owning component, because the + combined graph has no component boundaries to attribute them to + +**One scan per component** โ€” a separate invocation per component, each with its +own `--sub-path` and a distinct `--workspace-name`: + +- Per-component alerts, baselines, and policy +- Each component gets its own dependency graph, which is also the faster option + (see the performance note above) +- But `--workspace-name` suffixes the repository slug, so *N* components produce + *N* separate entries in the dashboard's repository list + +The second point is what makes this a real choice: a monorepo with a dozen or more +independently scanned components produces a dozen or more repository entries, which +gets hard to navigate as the list grows. A single consolidated entry that still +preserves per-component attribution is a known request and is not available today. + +Rules of thumb: + +- **Few components, or components that share a release cycle** โ€” use one combined + scan and accept coarser attribution. +- **Many components, or components with different owners or policies** โ€” use + per-component scans and accept the extra dashboard entries. Per-component policy + is only possible in this layout. +- **Components that are genuinely one application** โ€” group them under a single + `--workspace-name`, as in the first example below. Grouping is per logical + application, not per directory. ### Usage Examples -**Scan multiple frontend and backend workspaces:** +**Scan several directories that belong to one logical application:** ```bash socketcli --target-path /path/to/monorepo \ --sub-path frontend \ @@ -89,6 +138,19 @@ This will: - Create a repository in Socket named like `my-repo-mobile-web` - Preserve git context (commits, branch info) from the repository root +**Create independent frontend and backend scans:** +```bash +socketcli --target-path /path/to/monorepo \ + --sub-path frontend \ + --workspace-name frontend + +socketcli --target-path /path/to/monorepo \ + --sub-path backend \ + --workspace-name backend +``` + +These are two full-scan uploads, two server-side graphs, and two repository head/baseline sequences. In CI they can run as separate matrix jobs. See [GitHub Actions: scan changed monorepo workspaces independently](ci-cd.md#github-actions-scan-changed-monorepo-workspaces-independently). + **Generate GitLab Security Dashboard report:** ```bash socketcli --enable-gitlab-security \ @@ -138,6 +200,7 @@ This will simultaneously generate: - Both `--sub-path` and `--workspace-name` must be specified together - `--sub-path` can be used multiple times to include multiple directories +- Repeated `--sub-path` values are combined into one scan; they do not create independent workspace scans - All specified sub-paths must exist within the target-path ## Usage @@ -156,7 +219,7 @@ socketcli [-h] [--api-token API_TOKEN] [--repo REPO] [--workspace WORKSPACE] [-- [--reach] [--reach-version REACH_VERSION] [--reach-analysis-timeout REACH_ANALYSIS_TIMEOUT] [--reach-analysis-memory-limit REACH_ANALYSIS_MEMORY_LIMIT] [--reach-concurrency REACH_CONCURRENCY] [--reach-ecosystems REACH_ECOSYSTEMS] [--reach-min-severity ] [--reach-skip-cache] [--reach-disable-analytics] [--reach-enable-analysis-splitting] [--reach-detailed-analysis-log-file] - [--reach-lazy-mode] [--reach-use-only-pregenerated-sboms] [--reach-debug] [--reach-disable-external-tool-checks] + [--reach-use-only-pregenerated-sboms] [--reach-debug] [--reach-disable-external-tool-checks] [--reach-output-file REACH_OUTPUT_FILE] [--only-facts-file] [--version] ```` @@ -175,7 +238,7 @@ If you don't want to provide the Socket API Token every time then you can use th | `--repo` | False | *auto* | Repository name in owner/repo format (auto-detected from git remote) | | `--workspace` | False | | The Socket workspace to associate the scan with (e.g. `my-org` in `my-org/my-repo`). See note below. | | `--repo-is-public` | False | False | If set, flags a new repository creation as public. Defaults to false. | -| `--integration` | False | api | Integration type (api, github, gitlab, azure, bitbucket) | +| `--integration` | False | api | Integration type (api, github, gitlab, azure, bitbucket). When omitted, `--scm github` or `--scm gitlab` implies the matching integration. | | `--owner` | False | | Name of the integration owner, defaults to the socket organization slug | | `--branch` | False | *auto* | Branch name (auto-detected from git) | | `--committers` | False | *auto* | Committer(s) to filter by (auto-detected from git commit) | @@ -189,28 +252,26 @@ If you don't want to provide the Socket API Token every time then you can use th #### Pull Request and Commit | Parameter | Required | Default | Description | |:-----------------|:---------|:--------|:-----------------------------------------------| -| `--pr-number` | False | "0" | Pull request number | +| `--pr-number` | False | *auto* | Pull request number. Auto-detected in GitHub Actions, GitLab CI, and Azure Pipelines; explicitly passing `0` disables detection. | | `--commit-message` | False | *auto* | Commit message (auto-detected from git) | | `--commit-sha` | False | *auto* | Commit SHA (auto-detected from git) | | `--base-scan-id` | False | | Full scan ID to diff against, overriding the repository's head scan as the baseline. Mutually exclusive with `--base-commit-sha` | -| `--base-commit-sha`| False | | Commit SHA to diff against, overriding the repository's head scan as the baseline. The most recent full scan for that commit is used; the CLI errors (exit code 3, or `--exit-code-on-api-error`) if no scan exists for it. Mutually exclusive with `--base-scan-id` | +| `--base-commit-sha`| False | | Commit SHA to prefer as the diff baseline, overriding the repository's head scan. The CLI uses its most recent matching full scan or the nearest scanned first-parent ancestor within 100 local commits. It errors (exit code 3, or `--exit-code-on-api-error`) if no scanned ancestor is reachable. Mutually exclusive with `--base-scan-id` | -> **Diffing against the merge base** โ€” by default, PR scans are diffed against the repository's *latest* head scan, which may include newer default-branch commits than your PR branched from. To diff against the exact commit your PR is based on, compute the merge base and pass it as the baseline: +> **Diffing against the merge base** โ€” by default, PR scans are diffed against the repository's latest matching head scan, which may include newer default-branch commits than your PR branched from. To prefer the commit your PR is based on, compute the merge base and pass it as the baseline: > > ```shell > BASE_SHA=$(git merge-base origin/main HEAD) > socketcli --pr-number 123 --base-commit-sha "$BASE_SHA" > ``` > -> **Requirement: a full scan must already exist for the merge-base commit.** `--base-commit-sha` does not create a scan of that commit; it looks up an existing one. That lookup only succeeds if your CI runs `socketcli` on **every commit that lands on your default branch** โ€” every merge and direct push, not just periodic or latest-only scans. Common ways commits slip through without a scan: +> `--base-commit-sha` does not create a scan of that commit. The CLI first looks for the newest non-temporary scan matching the repository, workspace, scan type, and exact commit. If the exact commit was not scanned, it walks up to 100 first-parent commits from that SHA in the local checkout and uses the nearest matching scanned ancestor. It logs a warning with the selected commit and distance because this produces a wider diff than the merge base. > -> - CI settings that cancel or skip intermediate builds when newer commits land (e.g. Buildkite's ["cancel intermediate builds"](https://buildkite.com/docs/pipelines/configure/canceling-builds#cancel-running-intermediate-builds)) -> - `[skip ci]` commits, path-filtered pipelines, or failed/canceled scan steps -> - merge-base commits that predate your Socket rollout +> Run `socketcli` regularly on the default branch so recent ancestors have scans. PR checkouts must also retain the merge base and enough first-parent history; shallow clones can shorten the search. Gaps are expected when CI cancels intermediate builds, commits use `[skip ci]`, pipelines are path-filtered, or the merge base predates your Socket rollout. > -> If no scan exists for the commit, the CLI **fails** (exit code 3, or your `--exit-code-on-api-error` value; exit 0 with `--disable-blocking`) instead of silently falling back to the head scan โ€” a wrong baseline would misreport which alerts the PR introduces. Don't adopt this flag without default-branch scan coverage in place; you'll fail PR builds on lookup misses. +> If no scanned ancestor is reachable within the local 100-commit walk, the CLI **fails** (exit code 3, or your `--exit-code-on-api-error` value; exit 0 with `--disable-blocking`) instead of silently falling back to the repository head. API or permission failures also fail rather than being treated as a missing exact scan. > -> **Backfill pattern** โ€” if your default-branch coverage has gaps, the PR job can create the missing baseline itself before scanning: +> **Optional exact-baseline backfill** โ€” if the wider ancestor fallback is not acceptable, the PR job can create the missing exact baseline before scanning: > > ```shell > BASE_SHA=$(git merge-base origin/main HEAD) @@ -222,7 +283,7 @@ If you don't want to provide the Socket API Token every time then you can use th > socketcli --pr-number 123 --base-commit-sha "$BASE_SHA" > ``` > -> Run the baseline step with `--disable-blocking` (findings on the default branch must not fail the PR job) and an explicit `--branch`, since branch auto-detection is unreliable at a detached HEAD. +> Run the baseline step with `--disable-blocking` (findings on the default branch must not fail the PR job) and an explicit `--branch`, since branch auto-detection is unreliable at a detached HEAD. Without this step, the CLI automatically uses the nearest scanned ancestor. > > Buildkite users with dynamically generated pipelines: see [Merge-base baselines in Buildkite](ci-cd.md#merge-base-baselines-in-buildkite-dynamic-pipelines) for generation-time vs. step-time guidance. @@ -275,7 +336,7 @@ If you don't want to provide the Socket API Token every time then you can use th | Parameter | Required | Default | Description | |:---------------------------------|:---------|:--------|:---------------------------------------------------------------------------------------------------------------------------| | `--reach` | False | False | Enable reachability analysis to identify which vulnerable functions are actually called by your code. Creates a full application reachability scan (`scan_type=socket_tier1`). | -| `--reach-version` | False | 15.9.4 | Version of @coana-tech/cli to use. Defaults to the pinned version that ships with this CLI release, so the engine only changes when you upgrade the Socket CLI. Pass `latest` to always use the newest published version (opt-in auto-update), or an explicit version (e.g. `1.2.3`) to pin it. | +| `--reach-version` | False | 15.10.45 | Version of @coana-tech/cli to use. Defaults to the pinned version that ships with this CLI release, so the engine only changes when you upgrade the Socket CLI. Pass `latest` to always use the newest published version (opt-in auto-update), or an explicit version (e.g. `1.2.3`) to pin it. | | `--reach-analysis-timeout` | False | 10m | Timeout for each reachability analysis run, e.g. `90s`, `10m` or `1h`. Omitted by default, so coana applies its own default (`10m`). Alias: `--reach-timeout` | | `--reach-analysis-memory-limit` | False | 8GB | Memory limit for each reachability analysis run, e.g. `512MB` or `8GB`. Omitted by default, so coana applies its own default (`8GB`). Alias: `--reach-memory-limit` | | `--reach-concurrency` | False | 1 | Control parallel analysis execution (must be >= 1). Omitted by default, so coana applies its own default. | @@ -286,7 +347,6 @@ If you don't want to provide the Socket API Token every time then you can use th | `--reach-disable-analytics` | False | False | Disable analytics collection during reachability analysis | | `--reach-enable-analysis-splitting` | False | False | Enable analysis splitting/bucketing (a legacy performance feature). Splitting is disabled by default. | | `--reach-detailed-analysis-log-file` | False | False | Write a detailed analysis log file; its path is printed to stdout | -| `--reach-lazy-mode` | False | False | Enable lazy mode (experimental performance feature) | | `--reach-use-only-pregenerated-sboms` | False | False | Build the scan only from pre-generated CycloneDX (CDX) and SPDX files in your project (requires --reach) | | `--reach-debug` | False | False | Enable coana debug output (`--debug`) for the analysis, independent of the global `--enable-debug` | | `--reach-disable-external-tool-checks` | False | False | Disable coana's external tool availability checks (passes `--disable-external-tool-checks`) | @@ -369,11 +429,12 @@ The launcher can be tuned via the `SOCKET_CLI_COANA_LAUNCHER` environment variab |:-------------------------|:---------|:--------|:----------------------------------------------------------------------| | `--ignore-commit-files` | False | False | Ignore commit files | | `--disable-blocking` | False | False | Non-blocking CI mode: the CLI always exits **0**, even when blocking alerts are present (including with `--strict-blocking`). Also exits 0 on uncaught runtime errors and Socket API failures, so the job is treated as successful while findings and errors are still logged. Takes precedence over `--strict-blocking`. | -| `--disable-ignore` | False | False | Disable support for `@SocketSecurity ignore` commands in PR comments. When set, alerts cannot be suppressed via comments and ignore instructions are hidden from comment output. | +| `--disable-ignore` | False | False | Disable support for `@SocketSecurity ignore` commands in PR comments. When set, alerts cannot be suppressed via comments and ignore instructions are hidden from comment output. See [Who can ignore an alert](#who-can-ignore-an-alert). | +| `--ignore-authorization` | False | enforce | Who may suppress alerts with `@SocketSecurity ignore`. `enforce` requires write access and honors the command with a warning when the provider cannot report it; `strict` rejects it in that case; `off` honors any commenter. See [Who can ignore an alert](#who-can-ignore-an-alert). | | `--strict-blocking` | False | False | Fail on ANY security policy violations (blocking severity), not just new ones. Only works in diff mode. See [Strict Blocking Mode](#strict-blocking-mode) for details. | | `--enable-diff` | False | False | Enable diff mode even when using `--integration api` (forces diff mode without SCM integration) | | `--scm` | False | api | Source control management type | -| `--timeout` | False | | Timeout in seconds for API requests | +| `--timeout` | False | 1200 | Timeout in seconds for each API request. This is not a total CLI runtime limit and does not limit local discovery, Git, or reachability analysis. | #### Plugins @@ -628,6 +689,37 @@ The CLI uses intelligent default branch detection with the following priority: Both `--default-branch` and `--pending-head` parameters are automatically synchronized to ensure consistent behavior. +## Who can ignore an alert + +`@SocketSecurity ignore /@` and +`@SocketSecurity ignore-all` suppress security findings, so the CLI honors them +only from a commenter with write access to the repository. A command from anyone +else is skipped, logged with the author's name, and the alerts it named stay +reported. `--disable-ignore` turns the feature off entirely. + +| Provider | How access is determined | If it cannot be determined | +|:---------|:-------------------------|:---------------------------| +| GitHub | Effective repository permission, read once per commenter per run. Write, maintain, or admin access is honored. | The command is honored and a warning is logged. | +| GitLab | Project membership, read once per run when an ignore command is present. Developer (30) or above is honored. | The command is honored and a warning is logged. | + +The GitHub check needs a token that can read repository metadata. GitLab notes +carry no permission field, so that check needs a `GITLAB_TOKEN` that can read +`GET /projects/:id/members/all`. A `CI_JOB_TOKEN` generally cannot. + +`--ignore-authorization` decides what happens when access cannot be determined: + +| Value | Verified write access | Access cannot be determined | +|:------|:----------------------|:----------------------------| +| `enforce` (default) | Honored | Honored, with a warning naming the author | +| `strict` | Honored | Rejected | +| `off` | Honored | Honored, no check performed | + +`enforce` closes the hole wherever the provider can answer, without breaking a +pipeline whose token cannot read membership. `strict` closes it everywhere, at the +cost of failing those pipelines. `off` restores the prior behavior and should be +paired with `--disable-ignore` unless you specifically need comment-driven ignores +from unverified authors. + ## GitLab Token Configuration GitLab token/auth behavior and CI examples are documented in [`ci-cd.md`](ci-cd.md). diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 531099cd..9d6d06df 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -1,5 +1,34 @@ # Troubleshooting +## API token scopes for scan comparisons + +PR/MR runs compare the new scan against the repository's head scan. That comparison +first uses the diff-scans endpoints, which require an organization token with these +scopes in addition to whatever the scan itself needs: + +- `diff-scans:create` +- `diff-scans:list` +- `full-scans:list` + +If the token is missing them the scan still succeeds, so this is easy to miss. The only +signal is a warning, after which the CLI falls back to the older streaming comparison: + +``` +Diff scan comparison failed with APIAccessDenied(Insufficient permissions), falling back to the streaming scan comparison +``` + +Grant the scopes to use the diff-scans path. It polls with short, bounded requests +rather than holding one connection open while the backend computes, which is what lets +large comparisons survive network idle timeouts โ€” notably Azure NAT gateways, which +reap idle connections after four minutes and surface as an intermittent +`ConnectionResetError`. + +The two paths can take noticeably different amounts of time on the same repository, +because cached diff-scan responses always embed per-package license details while the +streaming comparison requests a lean payload. On a large dependency tree, compare the +`Diff scan comparison ready in ...` timing against the `Diff Report Gathered in ...` +total before assuming either path is at fault. + ## Common gotchas - In diff scope, `--strict-blocking` uses a stricter alert set (`new + unchanged`) for blocking checks and diff-based output selection. diff --git a/pyproject.toml b/pyproject.toml index 874bc068..82d3d566 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,26 +1,26 @@ [build-system] requires = [ - "hatchling" + "hatchling==1.32.0" ] build-backend = "hatchling.build" [project] name = "socketsecurity" -version = "2.5.4" +version = "2.9.3" requires-python = ">= 3.11" license = {"file" = "LICENSE"} dependencies = [ - 'requests', - 'mdutils', - 'prettytable', - 'GitPython', - 'packaging', - 'python-dotenv', - "socketdev>=3.3.0,<4.0.0", - "bs4>=0.0.2", - "markdown>=3.10", - "brotli>=1.0.9; platform_python_implementation == 'CPython'", - "brotlicffi>=1.0.9; platform_python_implementation != 'CPython'", + "requests==2.34.2", + "mdutils==1.8.1", + "prettytable==3.18.0", + "GitPython==3.1.59", + "packaging==26.3", + "python-dotenv==1.2.3", + "socketdev==3.6.0", + "beautifulsoup4==4.15.0", + "markdown==3.10.3", + "brotli==1.2.0; platform_python_implementation == 'CPython'", + "brotlicffi==1.2.0.2; platform_python_implementation != 'CPython'", ] readme = "README.md" description = "Socket Security CLI for CI/CD" @@ -40,18 +40,18 @@ classifiers = [ [project.optional-dependencies] test = [ - "pytest>=7.4.0", - "pytest-cov>=4.1.0", - "pytest-mock>=3.12.0", - "pytest-asyncio>=0.23.0", - "pytest-watch >=4.2.0" + "pytest==9.1.1", + "pytest-cov==7.1.0", + "pytest-mock==3.15.1", + "pytest-asyncio==1.4.0", + "pytest-watch==4.2.0" ] dev = [ - "ruff>=0.3.0", - "twine", # for building - "uv>=0.1.0", # for dependency management - "pre-commit", - "hatch" + "ruff==0.16.6", + "twine==7.0.0", # for building + "uv==0.12.9", # for dependency management + "pre-commit==4.6.2", + "hatch==1.18.0" ] [project.scripts] diff --git a/socketsecurity/__init__.py b/socketsecurity/__init__.py index 461becd0..7e432574 100644 --- a/socketsecurity/__init__.py +++ b/socketsecurity/__init__.py @@ -1,3 +1,3 @@ __author__ = 'socket.dev' -__version__ = '2.5.4' +__version__ = '2.9.3' USER_AGENT = f'SocketPythonCLI/{__version__}' diff --git a/socketsecurity/config.py b/socketsecurity/config.py index 26542447..bb2ad009 100644 --- a/socketsecurity/config.py +++ b/socketsecurity/config.py @@ -1,12 +1,14 @@ import argparse +import json import logging import os +import tomllib from dataclasses import asdict, dataclass, field from typing import List, Optional -from socketsecurity import __version__ + from socketdev import INTEGRATION_TYPES, IntegrationType -import json -import tomllib + +from socketsecurity import __version__ def get_plugin_config_from_env(prefix: str) -> dict: @@ -113,6 +115,7 @@ class CliConfig: branch: str = "" committers: Optional[List[str]] = None pr_number: str = "0" + pr_number_explicit: bool = False commit_message: Optional[str] = None default_branch: bool = False target_path: str = "./" @@ -141,6 +144,7 @@ class CliConfig: ignore_commit_files: bool = False disable_blocking: bool = False disable_ignore: bool = False + ignore_authorization: str = "enforce" # Tri-state log-upload preference: True = --upload-logs, False = --no-upload-logs, # None = neither (server-side override decides). upload_logs: Optional[bool] = None @@ -176,7 +180,7 @@ class CliConfig: reach_disable_analysis_splitting: bool = False # Deprecated, kept for backwards compatibility reach_enable_analysis_splitting: bool = False reach_detailed_analysis_log_file: bool = False - reach_lazy_mode: bool = False + reach_lazy_mode: bool = False # Deprecated, kept for backwards compatibility reach_ecosystems: Optional[List[str]] = None reach_exclude_paths: Optional[List[str]] = None reach_skip_cache: bool = False @@ -217,6 +221,17 @@ def from_args(cls, args_list: Optional[List[str]] = None) -> 'CliConfig': parser.set_defaults(**normalized_defaults) args = parser.parse_args(args_list) + integration_explicit = hasattr(args, "integration") + pr_number_explicit = hasattr(args, "pr_number") + + integration_type = getattr(args, "integration", "api") + pr_number = getattr(args, "pr_number", "0") + if ( + not integration_explicit and + integration_type == "api" and + args.scm in ("github", "gitlab") + ): + integration_type = args.scm if args.reach_exclude_paths: logging.warning( @@ -260,7 +275,8 @@ def from_args(cls, args_list: Optional[List[str]] = None) -> 'CliConfig': 'repo': args.repo, 'branch': args.branch, 'committers': args.committers, - 'pr_number': args.pr_number, + 'pr_number': pr_number, + 'pr_number_explicit': pr_number_explicit, 'commit_message': commit_message, 'default_branch': args.default_branch, 'target_path': os.path.expanduser(args.target_path), @@ -290,9 +306,10 @@ def from_args(cls, args_list: Optional[List[str]] = None) -> 'CliConfig': 'ignore_commit_files': args.ignore_commit_files, 'disable_blocking': args.disable_blocking, 'disable_ignore': args.disable_ignore, + 'ignore_authorization': args.ignore_authorization, 'upload_logs': args.upload_logs, 'strict_blocking': args.strict_blocking, - 'integration_type': args.integration, + 'integration_type': integration_type, 'pending_head': args.pending_head, 'timeout': args.timeout, 'exit_code_on_api_error': args.exit_code_on_api_error, @@ -517,8 +534,12 @@ def create_argument_parser() -> argparse.ArgumentParser: "--integration", choices=INTEGRATION_TYPES, metavar="", - help="Integration type of api, github, gitlab, azure, or bitbucket. Defaults to api", - default="api" + help=( + "Integration type of api, github, gitlab, azure, or bitbucket. " + "Defaults to api; --scm github/gitlab implies the matching integration " + "when this option is omitted" + ), + default=argparse.SUPPRESS ) integration_group.add_argument( "--owner", @@ -533,13 +554,17 @@ def create_argument_parser() -> argparse.ArgumentParser: "--pr-number", dest="pr_number", metavar="", - help="Pull request number", - default="0" + help=( + "Pull request number. Auto-detected in supported CI environments when omitted; " + "pass 0 explicitly to disable detection" + ), + default=argparse.SUPPRESS ) pr_group.add_argument( "--pr_number", dest="pr_number", - help=argparse.SUPPRESS + help=argparse.SUPPRESS, + default=argparse.SUPPRESS ) pr_group.add_argument( "--commit-message", @@ -585,9 +610,9 @@ def create_argument_parser() -> argparse.ArgumentParser: metavar="", default=None, help="Commit SHA to diff the new scan against, overriding the repository's head " - "scan as the baseline. The most recent full scan matching this commit (e.g. " - "the merge base from 'git merge-base origin/main HEAD') is used; the CLI " - "errors if no scan exists for it. Mutually exclusive with --base-scan-id." + "scan as the baseline. The CLI uses the most recent matching full scan, or " + "the nearest scanned first-parent ancestor within 100 local commits when " + "the commit itself was not scanned. Mutually exclusive with --base-scan-id." ) # Path and File options @@ -701,6 +726,19 @@ def create_argument_parser() -> argparse.ArgumentParser: action="store_true", help="If true, the new scan will be set as the branch's head scan" ) + config_group.add_argument( + "--ignore-authorization", + dest="ignore_authorization", + choices=["enforce", "strict", "off"], + default="enforce", + help=( + "Who may suppress alerts with @SocketSecurity ignore comments. " + "'enforce' (default) requires write access, and honors the command with " + "a warning when the provider cannot report the commenter's access. " + "'strict' rejects the command in that case instead. " + "'off' honors a command from any commenter." + ) + ) config_group.add_argument( "--pending_head", dest="pending_head", @@ -1091,7 +1129,7 @@ def create_argument_parser() -> argparse.ArgumentParser: "--reach-lazy-mode", dest="reach_lazy_mode", action="store_true", - help="Enable lazy mode for reachability analysis. This is an experimental feature for improving performance" + help=argparse.SUPPRESS # Deprecated, kept for backwards compatibility (no-op) ) reachability_group.add_argument( "--reach-output-file", diff --git a/socketsecurity/core/__init__.py b/socketsecurity/core/__init__.py index a372de4d..fe49bceb 100644 --- a/socketsecurity/core/__init__.py +++ b/socketsecurity/core/__init__.py @@ -1,38 +1,38 @@ +import atexit +import copy +import fnmatch +import importlib +import json import logging import os import random import re +import shutil import sys import tarfile import tempfile import time -import json from dataclasses import asdict -from pathlib import Path, PurePath -from typing import Dict, List, Tuple, Set, TYPE_CHECKING, Optional +from pathlib import PurePath +from typing import TYPE_CHECKING, Dict, Iterator, List, NamedTuple, Optional, Set, Tuple if TYPE_CHECKING: from socketsecurity.config import CliConfig +from git import Repo from socketdev import socketdev from socketdev.exceptions import APIFailure -from socketdev.fullscans import FullScanParams, SocketArtifact +from socketdev.fullscans import DiffArtifacts, FullScanParams, SocketArtifact from socketdev.org import Organization from socketdev.repos import RepositoryInfo -import copy -from socketsecurity import __version__, USER_AGENT -from socketsecurity.core.classes import ( - Alert, - Diff, - FullScan, - Issue, - Package, - Purl -) + +from socketsecurity import USER_AGENT, __version__ +from socketsecurity.core.classes import Alert, Diff, FullScan, Issue, Package, Purl from socketsecurity.core.exceptions import APIResourceNotFound + +from .resource_utils import check_file_count_against_ulimit from .socket_config import SocketConfig from .utils import socket_globs -from .resource_utils import check_file_count_against_ulimit -import importlib + logging_std = importlib.import_module("logging") @@ -52,6 +52,17 @@ _HUMANIZE_BOUNDARY = re.compile(r"(?<=[a-z0-9])(?=[A-Z])|(?<=[A-Z])(?=[A-Z][a-z])") +# How many full scans to request when resolving a diff baseline. The newest scan is +# usually the one we want, but temporary scans have to be skipped (see +# Core.newest_persisted_scan_id), so a single result is not enough. +SCAN_LOOKUP_PAGE_SIZE = 10 + +# Bounds on the search for a scanned ancestor when the requested baseline commit has +# no full scan of its own. Full-scan listing pages are matched against bounded local +# history, so the local walk cannot grow without limit. +ANCESTOR_SCAN_LOOKUP_LIMIT = 100 +ANCESTOR_WALK_MAX_DEPTH = 100 + # Reachability facts-file upload compression. # # The Socket full-scan endpoint transparently brotli-decompresses any multipart part @@ -72,6 +83,16 @@ # Stream the facts file in 1 MiB chunks so large files aren't held fully in memory. SOCKET_FACTS_BROTLI_CHUNK_SIZE = 1024 * 1024 +# Placeholder facts document (see empty_head_scan_file). A zero-byte file does not parse, +# and the API answers an unparseable facts file with the marker artifact below. +SOCKET_FACTS_EMPTY_DOCUMENT = '{"components": []}' + +# Synthetic artifact the API adds when an uploaded ``.socket.facts.json`` could not be +# parsed. A diagnostic, not a dependency, so it is dropped from scan results and reported +# as a warning instead. +INVALID_FACTS_MARKER_TYPE = "generic" +INVALID_FACTS_MARKER_NAME = "invalid-socket-facts" + # Full application reachability finalize retry policy. The finalize call links the reachability # scan to the full scan and can fail transiently (network/API blips); a few backoff retries make it robust. TIER1_FINALIZE_MAX_ATTEMPTS = 3 @@ -92,6 +113,36 @@ FULL_SCAN_UPLOAD_MAX_ATTEMPTS = len(FULL_SCAN_UPLOAD_BACKOFF_SCHEDULE_SECONDS) FULL_SCAN_UPLOAD_BACKOFF_JITTER_SECONDS = 2.0 +# Diff-scan polling policy. The legacy scan comparison (fullscans.stream_diff) holds a +# single HTTP connection open, fully idle, while the backend computes the diff; network +# middleboxes with TCP idle timeouts (notably Azure NAT gateways, which default to +# 4 minutes) kill that connection with a RST, surfacing as an intermittent +# ConnectionResetError on large scans. The diff-scans flow instead creates a +# diff-scan resource and polls its cached endpoint with short bounded requests: the API +# answers 202 while the comparison is still computing and 200 with the result once it is +# ready, so no connection is ever idle long enough to be reaped. +# +# Each poll consumes 1 unit of API quota, so the interval backs off toward +# DIFF_SCAN_POLL_MAX_INTERVAL_SECONDS to stay quota-friendly on comparisons that take +# minutes to compute. The timeout is a backstop against a diff scan that never +# completes; on expiry (or any other failure of this flow) the caller falls back to the +# legacy streaming comparison rather than failing the scan outright. +DIFF_SCAN_POLL_INITIAL_INTERVAL_SECONDS = 5.0 +DIFF_SCAN_POLL_MAX_INTERVAL_SECONDS = 10.0 +DIFF_SCAN_POLL_BACKOFF_MULTIPLIER = 1.5 +DIFF_SCAN_POLL_TIMEOUT_SECONDS = 30 * 60.0 + +# Temp dirs holding placeholder facts files (see Core.empty_head_scan_file). Call sites unlink +# the file itself once the upload finishes; the now-empty directory is removed at process exit +# so a run that raises mid-scan doesn't leak one. +_PLACEHOLDER_FACTS_DIRS: List[str] = [] + + +@atexit.register +def _cleanup_placeholder_facts_dirs() -> None: + for placeholder_dir in _PLACEHOLDER_FACTS_DIRS: + shutil.rmtree(placeholder_dir, ignore_errors=True) + def _humanize_alert_type(alert_type: str) -> str: """Convert a camelCase/PascalCase alert type into a Title-Cased label. @@ -106,6 +157,30 @@ def _humanize_alert_type(alert_type: str) -> str: return " ".join(part[:1].upper() + part[1:] for part in parts if part) +class ManifestPatterns(NamedTuple): + """Manifest patterns prepared once per scan root, case-folded for matching. + + The first three fields are the authoritative matchers used by + Core._matches_manifest_pattern. The candidate_* fields are a prefilter over + basenames alone: a manifest-discovery walk visits every file in the repository + but only a few hundred are manifests, so rejecting a name up front avoids + building a relative path and running a path match for the rest. The prefilter's + globs are pre-compiled into one alternation so the cost per rejected file stays + flat as the API's pattern list grows. + """ + + literal_basenames: Set[str] + basename_globs: List[str] + path_globs: List[str] + candidate_basenames: Set[str] + candidate_basename_regex: Optional["re.Pattern"] + + @property + def is_empty(self) -> bool: + """True when every ecosystem was filtered out, so the walk can be skipped.""" + return not (self.literal_basenames or self.basename_globs or self.path_globs) + + class Core: """Main class for interacting with Socket Security API and processing scan results.""" @@ -127,7 +202,13 @@ def __init__(self, config: SocketConfig, sdk: socketdev, cli_config: Optional['C self.config = config self.sdk = sdk self.cli_config = cli_config + self._supported_patterns: Optional[Dict] = None + org_start_time = time.perf_counter() self.set_org_vars() + log.info( + "Organization initialization completed in " + f"{time.perf_counter() - org_start_time:.2f}s" + ) def set_org_vars(self) -> None: """Sets the main shared configuration variables for organization access.""" @@ -154,18 +235,63 @@ def get_sbom_data(self, full_scan_id: str) -> Dict[str, SocketArtifact]: """Returns SBOM artifacts for a full scan keyed by artifact ID.""" response = self.sdk.fullscans.stream(self.config.org_slug, full_scan_id, use_types=True) if not response.success: - log.debug(f"Failed to get SBOM data for full-scan {full_scan_id}") - log.debug(response.message) - return {} + # Raise instead of returning {} so a failed fetch surfaces as an + # API error (exit code 3 by default) rather than empty reports. + log.error(f"Failed to get SBOM data for full-scan {full_scan_id}") + log.error(response.message) + raise APIFailure( + f"Failed to get SBOM data for full-scan {full_scan_id}: {response.message}" + ) if not hasattr(response, "artifacts") or not response.artifacts: return {} - return response.artifacts + artifacts = { + artifact_id: artifact + for artifact_id, artifact in response.artifacts.items() + if not Core.is_invalid_facts_marker(artifact) + } + Core.warn_if_invalid_facts_marker(len(artifacts) != len(response.artifacts)) + return artifacts def get_sbom_data_list(self, artifacts_dict: Dict[str, SocketArtifact]) -> list[SocketArtifact]: """Converts artifacts dictionary to a list.""" return list(artifacts_dict.values()) + @staticmethod + def is_invalid_facts_marker(artifact) -> bool: + """True for the API's ``generic/invalid-socket-facts`` unparseable-facts marker. + + Treated as a package it becomes a blocking alert with an empty "Introduced by" and + "Manifest File" that no developer can act on, so callers drop it and report the parse + failure through ``warn_if_invalid_facts_marker`` instead. + Matches any version; the API pins it to 1.0.0 but the version carries no meaning. + + Args: + artifact: A ``SocketArtifact`` or diff artifact (anything with ``type``/``name``). + + Returns: + True if the artifact is the marker rather than a real package. + """ + return ( + getattr(artifact, "type", None) == INVALID_FACTS_MARKER_TYPE + and getattr(artifact, "name", None) == INVALID_FACTS_MARKER_NAME + ) + + @staticmethod + def warn_if_invalid_facts_marker(found: bool) -> None: + """Log the parse failure that ``is_invalid_facts_marker`` stands for. + + Dropping the marker silently would hide a real, if non-blocking, problem: the scan ran + without the reachability data it was supposed to carry. + """ + if not found: + return + log.warning( + "Socket could not parse the uploaded .socket.facts.json, so reachability facts " + "were not applied to this scan. Ignoring the " + f"{INVALID_FACTS_MARKER_TYPE}/{INVALID_FACTS_MARKER_NAME} marker returned for it; " + "other scan results are unaffected." + ) def create_sbom_output(self, diff: Diff) -> dict: """Creates CycloneDX output for a given diff.""" @@ -398,6 +524,112 @@ def format_bytes(bytes_value): except Exception as e: log.error(f"Failed to save manifest tar.gz to {output_path}: {e}") + @staticmethod + def _prepare_manifest_patterns( + patterns: Dict, + ecosystems: Optional[List[str]], + excluded_ecosystems: List[str] + ) -> "ManifestPatterns": + """Prepare case-folded manifest patterns for a single filesystem walk. + + Literal basenames are kept in a set for the common fast path. Basename + globs and path-shaped globs are kept separately so the latter retain + pathlib's path-segment-aware matching behavior. The candidate basename + collections are derived here so the walker can reject a file on its name + alone; see ManifestPatterns. + """ + included_ecosystems = set(ecosystems) if ecosystems is not None else None + excluded = set(excluded_ecosystems) + literal_basenames: Set[str] = set() + basename_globs: Set[str] = set() + path_globs: Set[str] = set() + + for ecosystem, ecosystem_patterns in patterns.items(): + if included_ecosystems is not None and ecosystem not in included_ecosystems: + continue + if ecosystem in excluded: + continue + log.debug(f"Scanning ecosystem: {ecosystem}") + for details in ecosystem_patterns.values(): + original_pattern = details["pattern"] + for expanded in Core.expand_brace_pattern(original_pattern): + normalized = expanded.replace("\\", "/").casefold() + if "/" in normalized: + path_globs.add(normalized) + elif any(character in normalized for character in "*?["): + basename_globs.add(normalized) + else: + literal_basenames.add(normalized) + + # PurePath.match compares pattern segments right to left, so a path-shaped glob + # can only match a file whose basename matches the glob's final segment. Folding + # those final segments into the basename prefilter lets the walk skip the path + # match for everything else. A trailing "/" is directory-only under the legacy + # rglob behavior, so its empty final segment intentionally admits no files. + candidate_basenames = set(literal_basenames) + candidate_basename_globs = set(basename_globs) + for pattern in path_globs: + final_segment = pattern.rsplit("/", 1)[-1] + if any(character in final_segment for character in "*?["): + candidate_basename_globs.add(final_segment) + else: + candidate_basenames.add(final_segment) + + return ManifestPatterns( + literal_basenames=literal_basenames, + basename_globs=sorted(basename_globs), + path_globs=sorted(path_globs), + candidate_basenames=candidate_basenames, + candidate_basename_regex=Core._compile_basename_globs(candidate_basename_globs), + ) + + @staticmethod + def _compile_basename_globs(globs: Set[str]) -> Optional["re.Pattern"]: + """Compile basename globs into a single alternation, or None if there are none. + + fnmatch.translate anchors the tail with ``\\Z`` and re.match anchors the head, + so each alternative matches exactly what fnmatch.fnmatchcase would. + """ + if not globs: + return None + return re.compile( + "|".join(f"(?:{fnmatch.translate(glob)})" for glob in sorted(globs)) + ) + + @staticmethod + def _basename_could_match(normalized_name: str, patterns: "ManifestPatterns") -> bool: + """Cheap prefilter: could a file with this basename match any manifest pattern? + + False is authoritative; True still has to be confirmed by + _matches_manifest_pattern against the scan-root-relative path. + """ + if normalized_name in patterns.candidate_basenames: + return True + return ( + patterns.candidate_basename_regex is not None + and patterns.candidate_basename_regex.match(normalized_name) is not None + ) + + @staticmethod + def _matches_manifest_pattern(relative_path: str, patterns: "ManifestPatterns") -> bool: + """Return whether a scan-root-relative path matches a manifest pattern.""" + normalized_path = relative_path.replace("\\", "/").casefold() + basename = normalized_path.rsplit("/", 1)[-1] + if basename in patterns.literal_basenames: + return True + if any(fnmatch.fnmatchcase(basename, pattern) for pattern in patterns.basename_globs): + return True + if not patterns.path_globs: + return False + + candidate = PurePath(normalized_path) + return any(candidate.match(pattern) for pattern in patterns.path_globs) + + @staticmethod + def _matches_excluded_directory(directory_name: str, excluded_dirs: Set[str]) -> bool: + """Match configured directory exclusions, including entries such as ``*.egg-info``.""" + return any(fnmatch.fnmatchcase(directory_name, pattern) for pattern in excluded_dirs) + def find_files(self, path: str, ecosystems: Optional[List[str]] = None) -> List[str]: """ Finds supported manifest files in the given path. @@ -409,8 +641,8 @@ def find_files(self, path: str, ecosystems: Optional[List[str]] = None) -> List[ Returns: List of found manifest file paths. """ - log.debug("Starting Find Files") - start_time = time.time() + log.debug("Starting manifest discovery") + start_time = time.perf_counter() files: Set[str] = set() # Unified --exclude-paths: filter discovered manifests by the same paths/globs that are @@ -424,50 +656,92 @@ def find_files(self, path: str, ecosystems: Optional[List[str]] = None) -> List[ exclude_paths = getattr(self.cli_config, "exclude_paths", None) if self.cli_config else None exclude_regexes = Core.compile_exclude_paths(exclude_paths) if exclude_paths else [] - # Get supported patterns from the API patterns = self.get_supported_patterns() + manifest_patterns = self._prepare_manifest_patterns( + patterns, + ecosystems, + self.config.excluded_ecosystems, + ) - for ecosystem in patterns: - # If ecosystems filter is provided, only include specified ecosystems - if ecosystems is not None and ecosystem not in ecosystems: - continue - if ecosystem in self.config.excluded_ecosystems: - continue - log.debug(f'Scanning ecosystem: {ecosystem}') - ecosystem_patterns = patterns[ecosystem] - for file_name in ecosystem_patterns: - original_pattern = ecosystem_patterns[file_name]["pattern"] - - # Expand brace patterns - expanded_patterns = Core.expand_brace_pattern(original_pattern) - - for pattern in expanded_patterns: - case_insensitive_pattern = Core.to_case_insensitive_regex(pattern) - - log.debug(f"Searching for pattern: {case_insensitive_pattern}") - glob_start = time.time() - - # Use pathlib.Path.rglob() instead of glob.glob() to properly match dotfiles/dotdirs - base_path = Path(path) - glob_files = base_path.rglob(case_insensitive_pattern) - - for glob_file in glob_files: - glob_file_str = str(glob_file) - if not os.path.isfile(glob_file_str): - continue - if Core.is_excluded(glob_file_str, self.config.excluded_dirs): - continue - if exclude_regexes: - rel = os.path.relpath(glob_file_str, path) - if Core.path_matches_exclude_regexes(rel, exclude_regexes): - continue - files.add(glob_file_str.replace("\\", "/")) - - glob_end = time.time() - log.debug(f"Globbing took {glob_end - glob_start:.4f} seconds") + if manifest_patterns.is_empty: + elapsed = time.perf_counter() - start_time + log.info( + "Manifest discovery completed in " + f"{elapsed:.2f}s: root={os.path.abspath(path)}, " + "directories_visited=0, directories_pruned=0, " + "files_visited=0, manifests_found=0" + ) + log.info("Total files found: 0") + return [] + + directories_visited = 0 + directories_pruned = 0 + files_visited = 0 + excluded_dirs = set(self.config.excluded_dirs) + + def handle_walk_error(error: OSError) -> None: + log.debug(f"Unable to inspect path during manifest discovery: {error}") + + for current_root, directory_names, file_names in os.walk( + path, + topdown=True, + followlinks=False, + onerror=handle_walk_error, + ): + directories_visited += 1 + + kept_directories = [] + for directory_name in directory_names: + if directory_name == ".git" or Core._matches_excluded_directory( + directory_name, + excluded_dirs, + ): + directories_pruned += 1 + continue + # Only --exclude-paths needs a scan-root-relative path, so build one + # lazily rather than for every directory in the repository. + if exclude_regexes: + relative_directory = os.path.relpath( + os.path.join(current_root, directory_name), + path, + ) + if Core.path_matches_exclude_regexes(relative_directory, exclude_regexes): + directories_pruned += 1 + continue + kept_directories.append(directory_name) + directory_names[:] = kept_directories + + files_visited += len(file_names) + for file_name in file_names: + # Reject on the basename first: os.walk already hands us the name, so + # non-manifests cost one set lookup instead of a relative path plus a + # path match. Exclusions are then only evaluated for real candidates. + if not Core._basename_could_match(file_name.casefold(), manifest_patterns): + continue + file_path = os.path.join(current_root, file_name) + relative_path = os.path.relpath(file_path, path) + if not Core._matches_manifest_pattern(relative_path, manifest_patterns): + continue + if exclude_regexes and Core.path_matches_exclude_regexes( + relative_path, + exclude_regexes, + ): + continue + if os.path.isfile(file_path): + files.add(file_path.replace("\\", "/")) file_list = sorted(files) file_count = len(file_list) + elapsed = time.perf_counter() - start_time + log.info( + "Manifest discovery completed in " + f"{elapsed:.2f}s: root={os.path.abspath(path)}, " + f"directories_visited={directories_visited}, " + f"directories_pruned={directories_pruned}, " + f"files_visited={files_visited}, manifests_found={file_count}" + ) + # Retain the established count-only message for log consumers while they + # transition to the stage-level timing above. log.info(f"Total files found: {file_count}") # Check if the number of manifest files might exceed ulimit -n @@ -509,19 +783,36 @@ def get_supported_patterns(self) -> Dict: Returns: Dictionary of supported file patterns with 'general' key removed """ + cached_patterns = getattr(self, "_supported_patterns", None) + if cached_patterns is not None: + log.debug("Using cached supported manifest patterns") + return cached_patterns + + start_time = time.perf_counter() response = self.sdk.report.supported() + source = "api" if not response: log.error("Failed to get supported patterns from API") - # Import the old patterns as fallback - from .utils import socket_globs - return socket_globs - - # Remove the 'general' key if it exists - if 'general' in response: - response.pop('general') - - # The response is already in the format we need - return response + response = socket_globs + source = "local-fallback" + + # Do not mutate the SDK response, which may be shared by its own cache. + patterns = { + ecosystem: ecosystem_patterns + for ecosystem, ecosystem_patterns in response.items() + if ecosystem != "general" + } + # Only cache a successful lookup. The local fallback covers far fewer ecosystems + # than the API, so one transient failure must not pin the rest of the run to it โ€” + # has_manifest_files() runs before find_files() and would poison the cache. + if source == "api": + self._supported_patterns = patterns + elapsed = time.perf_counter() - start_time + log.info( + "Supported manifest patterns loaded in " + f"{elapsed:.2f}s: source={source}, ecosystems={len(patterns)}" + ) + return patterns def has_manifest_files(self, files: list) -> bool: """ @@ -594,20 +885,31 @@ def to_case_insensitive_regex(input_string: str) -> str: @staticmethod def empty_head_scan_file() -> List[str]: """ - Creates a temporary empty file for baseline scans when no head scan exists. - + Creates a temporary placeholder manifest for scans with no manifest files. + + Used for baseline scans when a repository has no head scan yet, and for the new scan + when no supported manifest files were found. The API rejects unsupported filenames, so + the placeholder must be named ``.socket.facts.json`` - which means it must also parse + as a facts document. A zero-byte file does not, and the API answers that by adding a + blocking ``generic/invalid-socket-facts@1.0.0`` artifact to the scan. + + Each call gets its own temp directory. The path used to be a fixed + ``$TMPDIR/.socket.facts.json``, so two runs sharing a temp dir could delete or + truncate each other's placeholder mid-upload. + Returns: - List containing path to a temporary empty file + List containing path to a temporary placeholder facts file """ - # Create a temporary directory and then create our specific filename - temp_dir = tempfile.gettempdir() - temp_path = os.path.join(temp_dir, '.socket.facts.json') - - # Create the empty file + # Own directory per call so concurrent runs can't clobber each other's placeholder; + # the basename must stay exactly SOCKET_FACTS_FILENAME to pass the API's validator. + temp_dir = tempfile.mkdtemp(prefix='socket_baseline_') + _PLACEHOLDER_FACTS_DIRS.append(temp_dir) + temp_path = os.path.join(temp_dir, SOCKET_FACTS_FILENAME) + with open(temp_path, 'w') as f: - pass # Creates an empty file - - log.debug(f"Created temporary empty file for baseline scan: {temp_path}") + f.write(SOCKET_FACTS_EMPTY_DOCUMENT) + + log.debug(f"Created temporary placeholder facts file for baseline scan: {temp_path}") return [temp_path] def finalize_tier1_scan(self, full_scan_id: str, facts_file_path: str) -> bool: @@ -744,7 +1046,7 @@ def _compress_facts_files_for_upload(self, files: List[str]) -> Tuple[List[str], exactly ``.socket.facts.json.br``, so compressing here keeps a large facts file under the server's per-file size cap without changing the stored result. Files whose basename is not exactly ``.socket.facts.json`` are left untouched (the server only - matches that exact name), as are empty placeholder files (e.g. baseline scans). + matches that exact name), as are zero-byte files. Compression never blocks an upload: if it fails for any reason (missing optional ``brotli`` dependency, unwritable directory, etc.) the original plain file is used. @@ -864,6 +1166,30 @@ def create_full_scan(self, files: List[str], params: FullScanParams, base_paths: return full_scan + @staticmethod + def _log_scan_configuration( + paths: List[str], + params: FullScanParams, + files: List[str], + manifest_source: str, + base_paths: Optional[List[str]] = None, + ) -> None: + """Log aggregate scan inputs without exposing submitted manifest paths.""" + base_path = base_paths[0] if base_paths else (paths[0] if paths else ".") + absolute_base_path = os.path.abspath(base_path) + relative_roots = [ + os.path.relpath(os.path.abspath(path), absolute_base_path).replace("\\", "/") + for path in paths + ] or ["."] + log.info( + "Scan configuration: " + f"repo={json.dumps(getattr(params, 'repo', None), default=str)} " + f"workspace={json.dumps(getattr(params, 'workspace', None), default=str)} " + f"scan_type={json.dumps(getattr(params, 'scan_type', None), default=str)} " + f"roots={json.dumps(relative_roots, separators=(',', ':'))} " + f"manifests={len(files)} manifest_source={manifest_source}" + ) + def create_full_scan_with_report_url( self, paths: List[str], @@ -906,6 +1232,14 @@ def create_full_scan_with_report_url( for path in paths: files = self.find_files(path) all_files.extend(files) + + self._log_scan_configuration( + paths, + params, + all_files, + manifest_source="provided" if explicit_files is not None else "discovered", + base_paths=base_paths, + ) # Save submitted files list if requested if save_files_list_path and all_files: @@ -957,6 +1291,7 @@ def create_full_scan_with_report_url( diff.report_url = f"{base_socket}/{self.config.org_slug}/sbom/{new_full_scan.id}" diff.diff_url = diff.report_url diff.id = new_full_scan.id + diff.is_full_scan = True needs_alerts = ( self.cli_config is not None @@ -966,30 +1301,43 @@ def create_full_scan_with_report_url( or self.cli_config.enable_sarif ) ) + # --generate-license (and --legal-format fossa, which it gates) enumerates + # diff.packages rather than the alert list, so a full scan has to carry the + # package map even when no alert-bearing output format is enabled. Without + # this, an SCM branch pipeline writes an attribution file with zero packages. + # Keep in sync with _requires_unchanged_artifacts, which lists the same + # consumers for the comparison path. + needs_license_artifacts = ( + self.cli_config is not None and self.cli_config.generate_license + ) - if needs_alerts: - log.info("Output format requires alerts, fetching SBOM data for full scan") + if needs_alerts or needs_license_artifacts: + log.info("Output format requires SBOM data, fetching it for the full scan") sbom_start = time.time() sbom_artifacts_dict = self.get_sbom_data(new_full_scan.id) sbom_artifacts = self.get_sbom_data_list(sbom_artifacts_dict) packages = self._create_packages_dict_without_license_text(sbom_artifacts) + if needs_license_artifacts: + packages = self._add_license_details(packages) diff.packages = packages - all_alerts_collection: Dict[str, List[Issue]] = {} - for package_id, package in packages.items(): - self.add_package_alerts_to_collection( - package=package, - alerts_collection=all_alerts_collection, - packages=packages - ) + if needs_alerts: + all_alerts_collection: Dict[str, List[Issue]] = {} + for package_id, package in packages.items(): + self.add_package_alerts_to_collection( + package=package, + alerts_collection=all_alerts_collection, + packages=packages + ) - consolidated: Set[str] = set() - for alert_key, alerts in all_alerts_collection.items(): - for alert in alerts: - alert_str = f"{alert.purl},{alert.type}" - if (alert.error or alert.warn) and alert_str not in consolidated: - diff.new_alerts.append(alert) - consolidated.add(alert_str) + consolidated: Set[str] = set() + for alert_key, alerts in all_alerts_collection.items(): + for alert in alerts: + alert_str = f"{alert.purl},{alert.type}" + if (alert.error or alert.warn) and alert_str not in consolidated: + diff.new_alerts.append(alert) + consolidated.add(alert_str) + diff.alerts_fetched = True sbom_end = time.time() log.info( @@ -1001,6 +1349,29 @@ def create_full_scan_with_report_url( return diff + def _add_license_details(self, packages: dict[str, Package]) -> dict[str, Package]: + """Populate licenseAttrib/licenseDetails on a full scan's package map. + + get_license_text_via_purl keys off ``ecosystem/name@version`` because that is + what the PURL endpoint echoes back, while a full scan's package map is keyed + by artifact id. Build a purl-keyed view over the same Package objects so the + enrichment lands on the map the caller keeps. + """ + batch_size = self.cli_config.max_purl_batch_size if self.cli_config else 5000 + packages_by_purl = {} + for package in packages.values(): + qualified_name = package.name + if package.namespace: + qualified_name = f"{package.namespace.strip('/')}/{qualified_name}" + packages_by_purl[ + f"{package.type}/{qualified_name}@{package.version}" + ] = package + self.get_license_text_via_purl( + packages_by_purl, + batch_size=batch_size, + ) + return packages + def get_full_scan(self, full_scan_id: str) -> FullScan: """ Get a FullScan object for an existing full scan including sbom_artifacts and packages. @@ -1141,19 +1512,229 @@ def get_repo_info(self, repo_slug: str, default_branch: str = "socket-default-br return response.data - def get_head_scan_for_repo(self, repo_slug: str) -> str: + def get_head_scan_for_repo( + self, + repo_slug: str, + workspace: Optional[str] = None, + scan_type: Optional[str] = None, + ) -> Optional[str]: """ Gets the head scan ID for a repository. + Without a workspace or scan type this is the repository's head scan pointer. + That pointer tracks a single scan for the whole repository rather than one per + workspace or scan type, so scoped runs instead take the newest matching scan + on the default branch. + Args: repo_slug: Repository slug to get head scan for + workspace: Socket workspace the scan belongs to, if any + scan_type: Socket scan type to match, if any Returns: Head scan ID if it exists, None otherwise + + Raises: + APIFailure: If the scoped scan lookup fails. A failed lookup must not + be reported as "no baseline": the caller answers that by creating an + empty baseline scan, which reports every dependency in the repository + as newly added. """ repo_info = self.get_repo_info(repo_slug) + if workspace or scan_type: + query_params = { + "repo": repo_slug, + "branch": repo_info.default_branch, + "sort": "created_at", + "direction": "desc", + "per_page": SCAN_LOOKUP_PAGE_SIZE, + } + if workspace: + query_params["workspace"] = workspace + if scan_type: + query_params["scan_type"] = Core.query_param_value(scan_type) + for results in self._full_scan_result_pages( + query_params, + f"Failed to list matching full scans for repo {repo_slug}", + ): + scan_id = Core.newest_persisted_scan_id(results) + if scan_id: + return scan_id + return None return repo_info.head_full_scan_id if repo_info.head_full_scan_id else None + @staticmethod + def query_param_value(value): + """ + Unwraps an enum member so it survives URL encoding. + + The SDK types several query params as str-backed enums (ScanType, + IntegrationType). urlencode calls str(), which renders a (str, Enum) member + as "ScanType.SOCKET_TIER1" -- a filter value the API does not recognize. + """ + return getattr(value, "value", value) + + @staticmethod + def newest_persisted_scan_id(results: List[dict]) -> Optional[str]: + """ + Returns the newest scan ID from a full scan listing, skipping temporary scans. + + create_new_diff creates an empty ``tmp`` scan when a repository has no baseline + yet, and that scan inherits the branch and commit of the run that created it. + If the real scan then fails, the empty scan is left behind as the newest scan + for that branch/commit; selecting it as a baseline would report every + dependency as newly added. + + Args: + results: Full scan listing results, newest first + + Returns: + Newest non-temporary scan ID, or None if the listing has none + """ + for result in results or []: + if not isinstance(result, dict) or result.get("tmp"): + continue + scan_id = result.get("id") + if scan_id: + return scan_id + return None + + def _full_scan_result_pages( + self, + query_params: dict, + failure_message: str, + ) -> Iterator[List[dict]]: + """Yields successful full-scan listing pages and rejects failed lookups.""" + request_params = dict(query_params) + seen_pages = {str(request_params.get("page", 1))} + per_page = int(request_params.get("per_page", 30)) + + while True: + response = self.sdk.fullscans.get(self.config.org_slug, request_params) + results = response.get("results") if isinstance(response, dict) else None + if results is None: + # The SDK logs and returns {} for any non-200, so a present results + # key is the only signal that the request itself succeeded. + raise APIFailure(failure_message) + yield results + + next_page = response.get("nextPage") + # The API has historically returned nextPage=1 for a short final page. + if len(results) < per_page or next_page in (None, 0, "0", False, ""): + return + + page_key = str(next_page) + if page_key in seen_pages: + raise APIFailure( + f"{failure_message}: full-scan pagination repeated page {next_page}" + ) + seen_pages.add(page_key) + request_params = {**query_params, "page": next_page} + + def first_parent_commits(self, start_commit_sha: str, max_count: int) -> List[str]: + """ + Lists a commit and its first-parent ancestors, newest first. + + Follows only first parents so a merge commit contributes the branch's own + history rather than everything merged into it. A shallow checkout simply + yields fewer commits, which narrows the search rather than failing it. + + Args: + start_commit_sha: Commit to walk back from, included in the result + max_count: Maximum number of commits to return + + Returns: + Commit SHAs, newest first. Empty when the repository or commit is + unavailable locally. + """ + target_path = self.cli_config.target_path if self.cli_config else None + if not target_path: + return [] + try: + repo = Repo(target_path) + output = repo.git.rev_list( + "--first-parent", + f"--max-count={max_count}", + start_commit_sha, + ) + except Exception as error: + log.debug(f"Unable to walk history back from {start_commit_sha}: {error}") + return [] + return [line.strip() for line in output.splitlines() if line.strip()] + + def find_baseline_scan_for_ancestor( + self, + repo_slug: str, + commit_sha: str, + workspace: Optional[str] = None, + scan_type: Optional[str] = None, + ) -> Tuple[Optional[str], Optional[str], int]: + """ + Finds the nearest ancestor of a commit that does have a full scan. + + Used when --base-commit-sha names a commit that was never scanned. Squash + merges and rebases rewrite commits, and a multi-commit push produces one scan + for the tip, so a merge base can be unscanned even when default-branch + scanning is configured correctly. Diffing against a slightly older ancestor + is a wider diff; failing outright is no diff at all. + + Scan listing pages are matched against local first-parent history. All pages + are considered because scans from other branches and reruns can fill newer + pages without covering the nearest candidate ancestors. + + Args: + repo_slug: Repository slug the scan belongs to + commit_sha: Commit that has no full scan of its own + workspace: Socket workspace the scan belongs to, if any + scan_type: Socket scan type to match, if any + + Returns: + (scan_id, ancestor_commit_sha, commits_back), or (None, None, 0) when no + scanned ancestor is reachable. + """ + ancestors = self.first_parent_commits(commit_sha, ANCESTOR_WALK_MAX_DEPTH) + if not ancestors: + return None, None, 0 + + query_params = { + "repo": repo_slug, + "sort": "created_at", + "direction": "desc", + "per_page": ANCESTOR_SCAN_LOOKUP_LIMIT, + } + if workspace: + query_params["workspace"] = workspace + if scan_type: + query_params["scan_type"] = Core.query_param_value(scan_type) + + scans_by_commit = {} + ancestor_set = set(ancestors) + for results in self._full_scan_result_pages( + query_params, + f"Failed to list ancestor full scans for repo {repo_slug}", + ): + for result in results: + if not isinstance(result, dict) or result.get("tmp"): + continue + result_commit = result.get("commit_hash") + scan_id = result.get("id") + # Newest first, so the first scan seen for a commit is the one to keep. + if ( + result_commit in ancestor_set + and scan_id + and result_commit not in scans_by_commit + ): + scans_by_commit[result_commit] = scan_id + + if not scans_by_commit: + return None, None, 0 + + for distance, ancestor in enumerate(ancestors): + scan_id = scans_by_commit.get(ancestor) + if scan_id: + return scan_id, ancestor, distance + return None, None, 0 + def get_full_scan_id_by_commit( self, repo_slug: str, @@ -1182,38 +1763,42 @@ def get_full_scan_id_by_commit( "commit_hash": commit_sha, "sort": "created_at", "direction": "desc", - "per_page": 1, + "per_page": SCAN_LOOKUP_PAGE_SIZE, } if workspace: query_params["workspace"] = workspace if scan_type: - query_params["scan_type"] = scan_type + query_params["scan_type"] = Core.query_param_value(scan_type) - response = self.sdk.fullscans.get( - self.config.org_slug, - query_params, - ) - results = response.get("results") if isinstance(response, dict) else None - if not results: - return None - return results[0].get("id") + for results in self._full_scan_result_pages( + query_params, + f"Failed to list full scans for commit {commit_sha} in repo {repo_slug}", + ): + scan_id = Core.newest_persisted_scan_id(results) + if scan_id: + return scan_id + return None def resolve_base_full_scan_id(self, params: FullScanParams) -> Optional[str]: """ Resolves the baseline full scan ID to diff a new scan against. Priority: --base-scan-id (used verbatim), then --base-commit-sha (newest - full scan for that commit), then the repository's current head scan. A - --base-commit-sha with no matching full scan is a hard error rather than a - silent fallback to the head scan, because diffing against the wrong - baseline silently misreports which alerts a PR introduces. + full scan for that commit, or its nearest scanned first-parent ancestor), + then the repository's current matching head scan. A --base-commit-sha with + no reachable scanned ancestor is a hard error rather than a silent fallback + to the head scan, because diffing against the wrong baseline silently + misreports which alerts a PR introduces. Returns: Full scan ID to use as the diff baseline, or None when the repository has no head scan yet (caller creates an empty baseline scan). """ if self.cli_config and self.cli_config.base_scan_id: - log.info(f"Using full scan {self.cli_config.base_scan_id} as diff baseline (--base-scan-id)") + log.info( + "Baseline selected: source=explicit-scan " + f"scan_id={json.dumps(self.cli_config.base_scan_id)}" + ) return self.cli_config.base_scan_id if self.cli_config and self.cli_config.base_commit_sha: @@ -1224,32 +1809,75 @@ def resolve_base_full_scan_id(self, params: FullScanParams) -> Optional[str]: workspace=params.workspace, scan_type=params.scan_type, ) + baseline_source = "explicit-commit" + baseline_commit = commit_sha if scan_id is None: - log.error( - f"No full scan found for commit {commit_sha} in repo {params.repo} " - "(--base-commit-sha). Ensure a scan was created for that commit " - "(e.g. the CLI runs on default-branch pushes), or pass " - "--base-scan-id instead." + scan_id, ancestor_sha, commits_back = self.find_baseline_scan_for_ancestor( + params.repo, + commit_sha, + workspace=params.workspace, + scan_type=params.scan_type, ) - if self.cli_config.disable_blocking: - sys.exit(0) - sys.exit(self.cli_config.exit_code_on_api_error) - log.info(f"Using full scan {scan_id} (commit {commit_sha}) as diff baseline (--base-commit-sha)") + if scan_id: + baseline_source = "explicit-commit-ancestor" + baseline_commit = ancestor_sha + log.warning( + f"No full scan for commit {commit_sha} (--base-commit-sha). " + f"Diffing against its nearest scanned ancestor {ancestor_sha}, " + f"{commits_back} commit(s) earlier, so the diff is wider than " + "the merge base." + ) + else: + log.error( + f"No full scan found for commit {commit_sha} in repo {params.repo} " + "(--base-commit-sha), and no scanned ancestor within " + f"{ANCESTOR_WALK_MAX_DEPTH} commits of it. Ensure a scan was " + "created for that commit (e.g. the CLI runs on default-branch " + "pushes), or pass --base-scan-id instead." + ) + if self.cli_config.disable_blocking: + sys.exit(0) + sys.exit(self.cli_config.exit_code_on_api_error) + log.info( + f"Baseline selected: source={baseline_source} " + f"scan_id={json.dumps(scan_id)} commit={json.dumps(baseline_commit)}" + ) return scan_id try: - return self.get_head_scan_for_repo(params.repo) + scan_id = self.get_head_scan_for_repo( + params.repo, + workspace=params.workspace, + scan_type=params.scan_type, + ) except APIResourceNotFound: return None + except APIFailure as error: + # Returning None would make the caller create an empty baseline scan, + # reporting every dependency as newly added, so fail loudly like the + # --base-commit-sha path above. + log.error( + f"Failed to resolve the matching head scan for repo {params.repo}: {error}" + ) + if self.cli_config is None: + raise + if self.cli_config.disable_blocking: + sys.exit(0) + sys.exit(self.cli_config.exit_code_on_api_error) + if scan_id: + log.info( + "Baseline selected: source=repository-head " + f"scan_id={json.dumps(scan_id)}" + ) + return scan_id @staticmethod def update_package_values(pkg: Package) -> Package: + pkg.type = Package.normalize_type(pkg.type) pkg.purl = f"{pkg.name}@{pkg.version}" - pkg.url = f"https://socket.dev/{pkg.type}/package" if pkg.namespace: pkg.purl = f"{pkg.namespace}/{pkg.purl}" - pkg.url += f"/{pkg.namespace}" - pkg.url += f"/{pkg.name}/overview/{pkg.version}" + pkg.url = Package.socket_url(pkg.type, pkg.namespace, pkg.name, pkg.version) return pkg def get_license_text_via_purl(self, packages: dict[str, Package], batch_size: int = 5000) -> dict: @@ -1293,6 +1921,9 @@ def get_license_text_via_purl(self, packages: dict[str, Package], batch_size: in for result in results: ecosystem = result["type"] name = result["name"] + namespace = (result.get("namespace") or "").strip("/") + if namespace and not name.startswith(f"{namespace}/"): + name = f"{namespace}/{name}" package_version = result["version"] licenseDetails = result.get("licenseDetails") licenseAttrib = result.get("licenseAttrib") @@ -1303,11 +1934,203 @@ def get_license_text_via_purl(self, packages: dict[str, Package], batch_size: in return packages + def get_diff_scan_artifacts( + self, + head_full_scan_id: str, + new_full_scan_id: str, + external_href: Optional[str] = None + ) -> DiffArtifacts: + """Compare two full scans via the diff-scans endpoints, polling for the result. + + Creates a diff-scan resource from the two full scan IDs, then polls + ``GET /orgs/{org}/diff-scans/{id}?cached=true`` until the API returns the + computed comparison (200) instead of a processing status (202). Unlike the + legacy ``fullscans.stream_diff`` call, no request is ever left idle while + the backend computes, so the comparison survives network idle timeouts. + See the DIFF_SCAN_POLL_* constants for the polling policy. + + Requires an org token with the ``diff-scans:create``, ``diff-scans:list`` + and ``full-scans:list`` scopes; callers are expected to catch failures and + fall back to the legacy streaming comparison. + + Note that cached diff-scan responses always embed per-package license + details (the API ignores ``omit_license_details`` when ``cached=true``), + so unlike the legacy streaming comparison there is no lean-response + option here; see the comment on ``poll_params`` below. + + Args: + head_full_scan_id: The before/base full scan ID + new_full_scan_id: The after/head full scan ID + external_href: Optional pull request or merge request URL to associate + with the diff scan in the Socket Dashboard + + Returns: + DiffArtifacts with the added/removed/unchanged/replaced/updated lists + """ + create_params = { + "before": head_full_scan_id, + "after": new_full_scan_id, + "description": f"Socket Security CLI v{__version__} scan comparison", + } + if external_href: + create_params["external_href"] = external_href + # external_href is only honored while a diff scan is being created, + # so a re-run over an already-compared scan pair needs + # on_duplicate=update to apply the link to the existing resource. It + # answers 200 with the same {"diff_scan": ...} envelope as a create. + create_params["on_duplicate"] = "update" + try: + result = self.sdk.diffscans.create_from_ids(self.config.org_slug, create_params) + diff_scan = result.get("diff_scan") or {} + response_summary = result + except APIFailure as error: + if error.status_code != 409: + raise + + # Reached when there is no pull request context to attach, and on + # deployments that answer 409 regardless. Do NOT switch this to + # on_duplicate=redirect: the SDK follows that 302 automatically with + # a GET that lacks cached=true, which can leave the connection idle + # while an existing diff scan is still computing. Resolve the + # duplicate explicitly so every result fetch continues through the + # bounded cached polling path below. + existing = self.sdk.diffscans.list( + self.config.org_slug, + params={ + "before_full_scan_id": head_full_scan_id, + "after_full_scan_id": new_full_scan_id, + "per_page": 1, + }, + ) + matches = existing.get("results") or [] + diff_scan = matches[0] if matches else {} + response_summary = existing + + diff_scan_id = diff_scan.get("id") + if not diff_scan_id: + raise Exception( + "Error creating or resolving diff scan: " + f"unexpected response: {str(response_summary)[:500]}" + ) + # Logged at INFO, not debug: this is the only identifier that ties a slow or + # failed comparison in a CI log back to a server-side diff scan, and it is + # needed even when the run later falls back to the streaming comparison. + log.info(f"Diff scan created: id={diff_scan_id}") + + # The create and list endpoints are metadata-only. Always fetch artifacts + # through GET below, even if an unexpected/legacy response happens to embed + # them, so omit_unchanged and the bounded cached-polling contract cannot be + # bypassed by an eager response. + artifacts_dict = None + + # cached=true is the polling contract (202 while computing, 200 when + # ready). The API ignores omit_license_details when cached=true - cached + # results always embed license details - so there is no lean-response + # option on this path (unlike stream_diff with + # include_license_details=false, the lean-payload mitigation). If that extra + # payload ever gets a response truncated on a huge dependency tree, + # response.json() fails and the caller falls back to the legacy + # streaming comparison, which still requests the lean payload. + # + # Verified against the live API: passing omit_license_details alongside + # cached=true leaves the license fields in the response, but omit_unchanged + # IS honored and drops the unchanged artifacts entirely (~1.1 KB each), so + # it is requested whenever no enabled output reads them. See + # _requires_unchanged_artifacts. + poll_params = {"cached": "true"} + omit_unchanged = not self._requires_unchanged_artifacts() + if omit_unchanged: + poll_params["omit_unchanged"] = "true" + poll_start = time.monotonic() + deadline = poll_start + DIFF_SCAN_POLL_TIMEOUT_SECONDS + interval = DIFF_SCAN_POLL_INITIAL_INTERVAL_SECONDS + # Tracked so the completion log can separate backend compute time from time the + # result spent ready-but-unpolled: the wait before the final poll bounds the + # latter, which is otherwise invisible in a CI log. + polls = 0 + last_interval = 0.0 + while artifacts_dict is None: + try: + response = self.sdk.diffscans.get(self.config.org_slug, diff_scan_id, params=poll_params) + except APIFailure as error: + if not error.is_transient_error(): + raise + # A dropped/timed-out poll is retryable: the diff scan keeps + # computing server-side regardless of what happens to any one poll. + log.warning( + f"Transient error polling diff scan {diff_scan_id} " + f"({type(error).__name__}), retrying in {interval:.0f}s" + ) + response = {"status": "processing"} + polls += 1 + if response.get("status") != "processing": + scan = response.get("diff_scan") or {} + if scan.get("artifacts") is None: + raise Exception( + f"Error fetching diff scan {diff_scan_id}: unexpected response: {str(response)[:500]}" + ) + artifacts_dict = scan["artifacts"] + log.info( + "Diff scan comparison ready in " + f"{time.monotonic() - poll_start:.2f}s: id={diff_scan_id}, " + f"polls={polls}, wait_before_final_poll={last_interval:.0f}s, " + f"omit_unchanged={str(omit_unchanged).lower()}" + ) + break + if time.monotonic() >= deadline: + raise Exception( + f"Timed out waiting for diff scan {diff_scan_id} after " + f"{DIFF_SCAN_POLL_TIMEOUT_SECONDS:.0f} seconds" + ) + log.debug(f"Diff scan {diff_scan_id} still processing, polling again in {interval:.0f}s") + time.sleep(interval) + last_interval = interval + interval = min(interval * DIFF_SCAN_POLL_BACKOFF_MULTIPLIER, DIFF_SCAN_POLL_MAX_INTERVAL_SECONDS) + + return DiffArtifacts.from_dict({ + key: artifacts_dict.get(key) or [] + for key in ("added", "removed", "unchanged", "replaced", "updated") + }) + + def _requires_unchanged_artifacts(self) -> bool: + """Whether any enabled output reads the unchanged half of a comparison. + + A cached diff-scan response embeds every unchanged artifact at roughly 1 KB + each, so on a large dependency tree they are almost the entire payload + (~11 MB for a tree of ~10k unchanged packages) even though most runs never + look at them. Every consumer is behind an opt-in flag: + + - ``--strict-blocking`` reads ``diff.unchanged_alerts`` to block on + pre-existing issues (socketcli, output, alert_selection, slack plugin). + - ``--enable-gitlab-security`` includes them in the GitLab dependency + scanning report (Messages.create_security_comment_gitlab). + - ``--generate-license`` enumerates ``diff.packages``, which must list every + dependency, not just the changed ones. + - ``--legal-format fossa`` reports all currently-present issues, matching + FOSSA's point-in-time snapshot semantics. + + ``Diff.to_dict`` also serializes them but has no callers. When cli_config is + absent the caller is unknown, so the full payload is kept. + + Keep this in sync with those consumers; test_unchanged_artifacts_gating + pins the list. + """ + config = self.cli_config + if config is None: + return True + return bool( + getattr(config, "strict_blocking", False) + or getattr(config, "enable_gitlab_security", False) + or getattr(config, "generate_license", False) + or getattr(config, "legal_format", "socket") == "fossa" + ) + def get_added_and_removed_packages( self, head_full_scan_id: str, new_full_scan_id: str, - include_license_details: bool = False + include_license_details: bool = False, + external_href: Optional[str] = None ) -> Tuple[Dict[str, Package], Dict[str, Package], Dict[str, Package]]: """ Get packages that were added and removed between scans. @@ -1315,8 +2138,12 @@ def get_added_and_removed_packages( Args: head_full_scan_id: Previous scan (maybe None if first scan) new_full_scan_id: New scan just created - include_license_details: Whether to ask the diff endpoint to embed - per-package license attribution/details in the response. + include_license_details: Whether to ask the *legacy streaming* diff + endpoint to embed per-package license attribution/details in the + response. Only consulted on the fallback path: the primary + diff-scans path always receives embedded license details, since + the API ignores ``omit_license_details`` for cached reads (see + get_diff_scan_artifacts). Defaults to ``False`` on purpose. The diff endpoint exists to compare alerts between two scans; the license fields it can embed @@ -1336,6 +2163,8 @@ def get_added_and_removed_packages( is retained as an explicit override seam, not wired to the ``--exclude-license-details`` user flag (which still governs the human-facing dashboard report URL). + external_href: Optional pull request or merge request URL to associate + with the primary diff-scan resource Returns: Tuple of (added_packages, removed_packages) dictionaries @@ -1343,39 +2172,68 @@ def get_added_and_removed_packages( log.info(f"Comparing scans - Head scan ID: {head_full_scan_id}, New scan ID: {new_full_scan_id}") diff_start = time.time() + diff_artifacts = None try: - diff_report = ( - self.sdk.fullscans.stream_diff( - self.config.org_slug, - head_full_scan_id, - new_full_scan_id, - use_types=True, - include_license_details=str(include_license_details).lower() - ).data + diff_artifacts = self.get_diff_scan_artifacts( + head_full_scan_id, + new_full_scan_id, + external_href=external_href, ) - except APIFailure as e: - log.error(f"API Error: {e}") - if self.cli_config and self.cli_config.disable_blocking: - sys.exit(0) - sys.exit(1) - except Exception as e: - import traceback - log.error(f"Error getting diff report: {str(e)}") - log.error(f"Stack trace:\n{traceback.format_exc()}") - raise + except Exception as error: + # SDK error messages can span many lines (path + response headers); the + # first line carries the status, which is all the warning needs. + error_summary = str(error).strip().splitlines()[0] if str(error).strip() else "" + log.warning( + f"Diff scan comparison failed with {type(error).__name__}({error_summary}), " + "falling back to the streaming scan comparison" + ) + log.info( + "Diff comparison mode: requested=diff-scan effective=streaming " + f"reason={type(error).__name__}" + ) + + if diff_artifacts is None: + try: + diff_artifacts = ( + self.sdk.fullscans.stream_diff( + self.config.org_slug, + head_full_scan_id, + new_full_scan_id, + use_types=True, + include_license_details=str(include_license_details).lower() + ).data.artifacts + ) + except APIFailure as e: + log.error(f"API Error: {e}") + # API failures are mapped to the configured infrastructure exit code by cli(). + raise + except Exception as e: + import traceback + log.error(f"Error getting diff report: {str(e)}") + log.error(f"Stack trace:\n{traceback.format_exc()}") + raise diff_end = time.time() log.info(f"Diff Report Gathered in {diff_end - diff_start:.2f} seconds") + + # Left in, the invalid-socket-facts marker reads as a newly added blocking package. + # Drop it from every bucket before the counts below, which should describe what the + # CLI actually reports on. + marker_found = False + buckets: Dict[str, List] = {} + for name in ("added", "removed", "unchanged", "replaced", "updated"): + bucket = getattr(diff_artifacts, name) + buckets[name] = [a for a in bucket if not Core.is_invalid_facts_marker(a)] + marker_found = marker_found or len(buckets[name]) != len(bucket) + Core.warn_if_invalid_facts_marker(marker_found) + log.info("Diff report artifact counts:") - log.info(f"Added: {len(diff_report.artifacts.added)}") - log.info(f"Removed: {len(diff_report.artifacts.removed)}") - log.info(f"Unchanged: {len(diff_report.artifacts.unchanged)}") - log.info(f"Replaced: {len(diff_report.artifacts.replaced)}") - log.info(f"Updated: {len(diff_report.artifacts.updated)}") + for name, bucket in buckets.items(): + log.info(f"{name.capitalize()}: {len(bucket)}") - added_artifacts = diff_report.artifacts.added + diff_report.artifacts.updated - removed_artifacts = diff_report.artifacts.removed + diff_report.artifacts.replaced - unchanged_artifacts = diff_report.artifacts.unchanged + added_artifacts = buckets["added"] + buckets["updated"] + removed_artifacts = buckets["removed"] + buckets["replaced"] + unchanged_artifacts = buckets["unchanged"] added_packages: Dict[str, Package] = {} removed_packages: Dict[str, Package] = {} @@ -1435,7 +2293,8 @@ def create_new_diff( save_files_list_path: Optional[str] = None, save_manifest_tar_path: Optional[str] = None, base_paths: Optional[List[str]] = None, - explicit_files: Optional[List[str]] = None + explicit_files: Optional[List[str]] = None, + external_href: Optional[str] = None ) -> Diff: """Create a new diff using the Socket SDK. @@ -1447,6 +2306,8 @@ def create_new_diff( save_manifest_tar_path: Optional path to save manifest files tar.gz archive base_paths: List of base paths for the scan (optional) explicit_files: Optional list of explicit files to use instead of discovering files + external_href: Optional pull request or merge request URL to associate + with the diff scan """ log.debug(f"starting create_new_diff with no_change: {no_change}") if no_change: @@ -1461,6 +2322,14 @@ def create_new_diff( for path in paths: files = self.find_files(path) all_files.extend(files) + + self._log_scan_configuration( + paths, + params, + all_files, + manifest_source="provided" if explicit_files is not None else "discovered", + base_paths=base_paths, + ) # Save submitted files list if requested if save_files_list_path and all_files: @@ -1496,7 +2365,10 @@ def create_new_diff( try: head_full_scan = self.create_full_scan(empty_files, tmp_params, base_paths=base_paths) head_full_scan_id = head_full_scan.id - log.debug(f"Created empty baseline scan: {head_full_scan_id}") + log.info( + "Baseline selected: source=empty " + f"scan_id={json.dumps(head_full_scan_id)}" + ) # Clean up the temporary empty file for temp_file in empty_files: @@ -1532,9 +2404,8 @@ def create_new_diff( os.unlink(temp_file) except OSError: pass - if self.cli_config and self.cli_config.disable_blocking: - sys.exit(0) - sys.exit(1) + # API failures are mapped to the configured infrastructure exit code by cli(). + raise except Exception as e: import traceback log.error(f"Error creating new full scan: {str(e)}") @@ -1571,7 +2442,8 @@ def create_new_diff( ) = self.get_added_and_removed_packages( head_full_scan_id, new_full_scan.id, - include_license_details=False + include_license_details=False, + external_href=external_href, ) # Separate unchanged packages from added/removed for --strict-blocking support @@ -1597,6 +2469,10 @@ def create_new_diff( else: diff.diff_url = diff.report_url + # PR/MR runs previously logged only the scan IDs, so a CI log had no link to the + # result. Logged here rather than at each call site so every diff flow gets it. + log.info(f"Diff report URL: {diff.diff_url}") + return diff def create_diff_report( @@ -1631,16 +2507,22 @@ def create_diff_report( alerts_in_removed_packages: Dict[str, List[Issue]] = {} alerts_in_unchanged_packages: Dict[str, List[Issue]] = {} - seen_new_packages = set() - seen_removed_packages = set() + seen_packages = { + "added": set(), + "updated": set(), + "removed": set(), + "replaced": set(), + } for package_id, package in added_packages.items(): purl = self.create_purl(package_id, added_packages) base_purl = f"{purl.ecosystem}/{purl.name}@{purl.version}" - if (not direct_only or package.direct) and base_purl not in seen_new_packages: - diff.new_packages.append(purl) - seen_new_packages.add(base_purl) + change_type = "updated" if package.diffType == "updated" else "added" + target = diff.updated_packages if change_type == "updated" else diff.new_packages + if (not direct_only or package.direct) and base_purl not in seen_packages[change_type]: + target.append(purl) + seen_packages[change_type].add(base_purl) self.add_package_alerts_to_collection( package=package, @@ -1652,9 +2534,11 @@ def create_diff_report( purl = self.create_purl(package_id, removed_packages) base_purl = f"{purl.ecosystem}/{purl.name}@{purl.version}" - if (not direct_only or package.direct) and base_purl not in seen_removed_packages: - diff.removed_packages.append(purl) - seen_removed_packages.add(base_purl) + change_type = "replaced" if package.diffType == "replaced" else "removed" + target = diff.replaced_packages if change_type == "replaced" else diff.removed_packages + if (not direct_only or package.direct) and base_purl not in seen_packages[change_type]: + target.append(purl) + seen_packages[change_type].add(base_purl) self.add_package_alerts_to_collection( package=package, @@ -1779,23 +2663,24 @@ def get_source_data(package: Package, packages: dict) -> list: @staticmethod def add_purl_capabilities(diff: Diff) -> None: """ - Adds capability information to each package in the diff's new_packages list. + Adds capability information to the diff's added and updated packages. + + Both lists are walked because an updated package is still newly present at + its new version, so its capabilities are as relevant as an added one's. Args: diff: Diff object to update with capability information """ - new_packages = [] - for purl in diff.new_packages: - if purl.id in diff.new_capabilities: - new_purl = Purl( - **{**purl.__dict__, - "capabilities": diff.new_capabilities[purl.id]} - ) - new_packages.append(new_purl) - else: - new_packages.append(purl) - - diff.new_packages = new_packages + for attribute in ("new_packages", "updated_packages"): + packages = [] + for purl in getattr(diff, attribute): + if purl.id in diff.new_capabilities: + purl = Purl( + **{**purl.__dict__, + "capabilities": diff.new_capabilities[purl.id]} + ) + packages.append(purl) + setattr(diff, attribute, packages) def add_package_alerts_to_collection(self, package: Package, alerts_collection: dict, packages: dict) -> dict: """ @@ -1848,6 +2733,8 @@ def add_package_alerts_to_collection(self, package: Package, alerts_collection: suggestion=props.suggestion, next_step_title=props.nextStepTitle, introduced_by=introduced_by, + manifest_files=package.manifestFiles or [], + direct=bool(package.direct), purl=package.purl, url=package.url ) diff --git a/socketsecurity/core/alert_selection.py b/socketsecurity/core/alert_selection.py index ae5b4772..132be294 100644 --- a/socketsecurity/core/alert_selection.py +++ b/socketsecurity/core/alert_selection.py @@ -31,7 +31,9 @@ def clone_diff_with_selected_alerts(diff: Diff, selected_alerts: List[Issue]) -> removed_alerts=[], diff_url=getattr(diff, "diff_url", ""), new_packages=getattr(diff, "new_packages", []), + updated_packages=getattr(diff, "updated_packages", []), removed_packages=getattr(diff, "removed_packages", []), + replaced_packages=getattr(diff, "replaced_packages", []), packages=getattr(diff, "packages", {}), ) selected_diff.id = getattr(diff, "id", "") diff --git a/socketsecurity/core/classes.py b/socketsecurity/core/classes.py index db145221..fd8fabcd 100644 --- a/socketsecurity/core/classes.py +++ b/socketsecurity/core/classes.py @@ -1,4 +1,5 @@ import json +import logging from dataclasses import dataclass, field from typing import Dict, List, Optional, TypedDict @@ -11,6 +12,13 @@ SocketScore, ) +log = logging.getLogger("socketdev") + +# Ecosystems whose package pages cannot be addressed by name alone. A Maven +# coordinate is a groupId plus an artifactId; with no namespace the URL collapses to +# one path segment that cannot be split back into two, and the page does not resolve. +NAMESPACE_REQUIRED_TYPES = frozenset({"maven"}) + __all__ = [ "Report", "Score", @@ -142,6 +150,43 @@ class Package(): licenseAttrib: Optional[List] = None + @staticmethod + def normalize_type(package_type) -> str: + """ + Unwraps the SDK's str-backed SocketPURL_Type enum to its value. + + str(SocketPURL_Type.MAVEN) is "SocketPURL_Type.MAVEN", not "maven", so any + enum member reaching an f-string leaks the class name into user-facing output. + """ + return getattr(package_type, "value", package_type) + + @staticmethod + def socket_url(package_type, namespace: Optional[str], name: str, version: str) -> str: + """ + Builds the socket.dev package overview URL for a package. + + Namespace and name are separate path segments, the same form purl strings use. + + Args: + package_type: Ecosystem, as a string or SocketPURL_Type member + namespace: Package namespace (Maven groupId, npm scope), if any + name: Package name + version: Package version + + Returns: + Package overview URL on socket.dev + """ + package_type = Package.normalize_type(package_type) + namespace = (namespace or "").strip("/") + if not namespace and package_type in NAMESPACE_REQUIRED_TYPES: + # The link is still emitted so the finding reports, but it cannot resolve. + log.warning( + f"{package_type} package {name}@{version} has no namespace, so its " + "Socket link collapses to a single path segment and will not resolve" + ) + package_path = "/".join(part for part in (namespace, name) if part) + return f"https://socket.dev/{package_type}/package/{package_path}/overview/{version}" + @classmethod def from_socket_artifact(cls, data: dict) -> "Package": """ @@ -153,18 +198,16 @@ def from_socket_artifact(cls, data: dict) -> "Package": Returns: New Package instance """ - purl = f"{data['type']}/" - namespace = data.get("namespace") - if namespace: - purl += f"{namespace}@" - purl += f"{data['name']}@{data['version']}" - base_url = "https://socket.dev" - url = f"{base_url}/{data['type']}/package/{namespace or ''}{data['name']}/overview/{data['version']}" + package_type = Package.normalize_type(data["type"]) + namespace = (data.get("namespace") or "").strip("/") + package_path = "/".join(part for part in (namespace, data["name"]) if part) + purl = f"{package_type}/{package_path}@{data['version']}" + url = Package.socket_url(package_type, namespace, data["name"], data["version"]) return cls( id=data["id"], name=data["name"], version=data["version"], - type=data["type"], + type=package_type, release=data.get("release"), diffType=data.get("diffType"), score=data["score"], @@ -179,7 +222,7 @@ def from_socket_artifact(cls, data: dict) -> "Package": artifact=data.get("artifact"), purl=purl, url=url, - namespace=namespace + namespace=namespace or None ) @classmethod @@ -274,6 +317,11 @@ class Issue: manifests: str url: str purl: str + # The package's own manifest files, independent of how it was introduced. A + # transitive package whose ancestors are absent from the scan has no + # introduced_by chain, but its manifest is still known. + manifest_files: list + direct: bool def __init__(self, **kwargs): if kwargs: @@ -282,6 +330,10 @@ def __init__(self, **kwargs): if hasattr(self, "created_at"): self.created_at = self.created_at.strip(" (Coordinated Universal Time)") + if not hasattr(self, "manifest_files"): + self.manifest_files = [] + if not hasattr(self, "direct"): + self.direct = False if not hasattr(self, "manifests"): self.manifests = "" if not hasattr(self, "suggestion"): @@ -507,7 +559,9 @@ class Diff: """ new_packages: list[Purl] + updated_packages: list[Purl] removed_packages: list[Purl] + replaced_packages: list[Purl] packages: dict[str, Package] new_capabilities: Dict[str, List[str]] new_alerts: list[Issue] @@ -518,6 +572,8 @@ class Diff: report_url: str diff_url: str new_scan_id: str + is_full_scan: bool + alerts_fetched: bool def __init__(self, **kwargs): if kwargs: @@ -525,8 +581,12 @@ def __init__(self, **kwargs): setattr(self, key, value) if not hasattr(self, "new_packages"): self.new_packages = [] + if not hasattr(self, "updated_packages"): + self.updated_packages = [] if not hasattr(self, "removed_packages"): self.removed_packages = [] + if not hasattr(self, "replaced_packages"): + self.replaced_packages = [] if not hasattr(self, "new_alerts"): self.new_alerts = [] if not hasattr(self, "unchanged_alerts"): @@ -535,6 +595,10 @@ def __init__(self, **kwargs): self.removed_alerts = [] if not hasattr(self, "new_capabilities"): self.new_capabilities = {} + if not hasattr(self, "is_full_scan"): + self.is_full_scan = False + if not hasattr(self, "alerts_fetched"): + self.alerts_fetched = False def __str__(self): return json.dumps(self.__dict__) @@ -548,8 +612,10 @@ def to_dict(self) -> dict: """ return { "new_packages": [p.to_dict() for p in self.new_packages], + "updated_packages": [p.to_dict() for p in self.updated_packages], "new_capabilities": self.new_capabilities, "removed_packages": [p.to_dict() for p in self.removed_packages], + "replaced_packages": [p.to_dict() for p in self.replaced_packages], "new_alerts": [alert.__dict__ for alert in self.new_alerts], "unchanged_alerts": [alert.__dict__ for alert in self.unchanged_alerts] if hasattr(self, "unchanged_alerts") else [], "removed_alerts": [alert.__dict__ for alert in self.removed_alerts] if hasattr(self, "removed_alerts") else [], diff --git a/socketsecurity/core/cli_client.py b/socketsecurity/core/cli_client.py index bfad0d14..405a7443 100644 --- a/socketsecurity/core/cli_client.py +++ b/socketsecurity/core/cli_client.py @@ -6,6 +6,7 @@ import requests from socketsecurity import USER_AGENT + from .exceptions import APIFailure from .socket_config import SocketConfig @@ -55,7 +56,12 @@ def request( except requests.exceptions.RequestException as e: logger.error(f"API request failed: {str(e)}") - raise APIFailure(f"Request failed: {str(e)}") + # Carry the status forward. Callers that need to react to a specific + # code -- the GitLab auth fallback to the other token scheme, and + # APIFailure.is_transient_error -- have no other way to recover it + # once the requests exception has been translated. + status_code = e.response.status_code if e.response is not None else None + raise APIFailure(f"Request failed: {str(e)}", status_code=status_code) from e def post_telemetry_events(self, org_slug: str, events: List[Dict]) -> None: """Post telemetry events one at a time to the v0 telemetry API. Fire-and-forget โ€” logs errors but never raises.""" diff --git a/socketsecurity/core/exceptions.py b/socketsecurity/core/exceptions.py index 03e69b87..b2112ee0 100644 --- a/socketsecurity/core/exceptions.py +++ b/socketsecurity/core/exceptions.py @@ -1,3 +1,5 @@ +from socketdev.exceptions import APIFailure as SdkAPIFailure + __all__ = [ "APIFailure", "APIKeyMissing", @@ -18,8 +20,14 @@ class APIKeyMissing(Exception): pass -class APIFailure(Exception): - """Raised when there is an error using the API""" +class APIFailure(SdkAPIFailure): + """Raised when there is an error using the API. + + Subclasses the SDK's exception of the same name so a handler written against + either one catches both. A separate Exception subclass would bypass an + ``except APIFailure`` importing the SDK's -- which every handler in + socketsecurity.core does -- and would not carry the SDK class's status code. + """ pass @@ -39,4 +47,4 @@ class APIResourceNotFound(Exception): class RequestTimeoutExceeded(Exception): """Raised when access is denied to the API""" - pass \ No newline at end of file + pass diff --git a/socketsecurity/core/git_interface.py b/socketsecurity/core/git_interface.py index da614063..b3c53bdc 100644 --- a/socketsecurity/core/git_interface.py +++ b/socketsecurity/core/git_interface.py @@ -1,6 +1,7 @@ +import os import re +import time import urllib.parse -import os from git import Repo @@ -12,34 +13,33 @@ class Git: path: str def __init__(self, path: str): + initialization_start = time.perf_counter() self.path = path + self._fetched_ref_commits = {} self.ensure_safe_directory(path) self.repo = Repo(path) assert self.repo self.head = self.repo.head - - # Always fetch all remote refs to ensure branches exist for diffing - try: - self.repo.git.fetch('--all') - log.debug("Fetched all remote refs for diffing.") - except Exception as fetch_error: - log.debug(f"Failed to fetch all remote refs: {fetch_error}") # Use CI environment SHA if available, otherwise fall back to current HEAD commit github_sha = os.getenv('GITHUB_SHA') gitlab_sha = os.getenv('CI_COMMIT_SHA') bitbucket_sha = os.getenv('BITBUCKET_COMMIT') - ci_sha = github_sha or gitlab_sha or bitbucket_sha + buildkite_sha = os.getenv('BUILDKITE_COMMIT') + ci_commits = ( + ("BUILDKITE_COMMIT", buildkite_sha), + ("GITHUB_SHA", github_sha), + ("CI_COMMIT_SHA", gitlab_sha), + ("BITBUCKET_COMMIT", bitbucket_sha), + ) + env_source, ci_sha = next( + ((source, sha) for source, sha in ci_commits if sha), + (None, None), + ) if ci_sha: try: self.commit = self.repo.commit(ci_sha) - if github_sha: - env_source = "GITHUB_SHA" - elif gitlab_sha: - env_source = "CI_COMMIT_SHA" - else: - env_source = "BITBUCKET_COMMIT" log.debug(f"Using commit from {env_source}: {ci_sha}") except Exception as error: log.debug(f"Failed to get commit from CI environment: {error}") @@ -82,13 +82,19 @@ def __init__(self, path: str): # Bitbucket Pipelines variables bitbucket_branch = os.getenv('BITBUCKET_BRANCH') + + # Buildkite branch (the source branch for pull-request builds) + buildkite_branch = os.getenv('BUILDKITE_BRANCH') - # Select CI branch with priority: GitLab -> GitHub -> Bitbucket - ci_branch = gitlab_branch or github_branch or bitbucket_branch + # Prefer the native environment when Buildkite is driving the job. This + # also avoids requiring Buildkite users to emulate GitHub Actions vars. + ci_branch = buildkite_branch or gitlab_branch or github_branch or bitbucket_branch if ci_branch: self.branch = ci_branch - if gitlab_branch: + if buildkite_branch: + env_source = "Buildkite" + elif gitlab_branch: env_source = "GitLab CI" elif github_branch: env_source = "GitHub Actions" @@ -141,40 +147,39 @@ def __init__(self, path: str): self.commit_sha = self.commit.binsha self.commit_message = self.commit.message self.committer = self.commit.committer - # Detect changed files in PR/MR context for GitHub, GitLab, Bitbucket; fallback to git show + + # Detect changed files in PR/MR context, using local refs first and + # fetching only a required ref when the checkout does not contain it. + changed_files_start = time.perf_counter() self.show_files = [] detected = False - # GitHub Actions PR context + detection_source = "single-commit" + github_base_ref = os.getenv('GITHUB_BASE_REF') github_head_ref = os.getenv('GITHUB_HEAD_REF') github_event_name = os.getenv('GITHUB_EVENT_NAME') github_before_sha = os.getenv('GITHUB_EVENT_BEFORE') # previous commit for push github_sha = os.getenv('GITHUB_SHA') # current commit - if github_event_name == 'pull_request' and github_base_ref and github_head_ref: - try: - # Fetch both branches individually - self.repo.git.fetch('origin', github_base_ref) - self.repo.git.fetch('origin', github_head_ref) - # Try remote diff first - diff_range = f"origin/{github_base_ref}...origin/{github_head_ref}" - try: - diff_files = self.repo.git.diff('--name-only', diff_range) - self.show_files = diff_files.splitlines() - log.debug(f"Changed files detected via git diff (GitHub PR remote): {self.show_files}") - detected = True - except Exception as remote_error: - log.debug(f"Remote diff failed: {remote_error}") - # Try local branch diff - local_diff_range = f"{github_base_ref}...{github_head_ref}" - try: - diff_files = self.repo.git.diff('--name-only', local_diff_range) - self.show_files = diff_files.splitlines() - log.debug(f"Changed files detected via git diff (GitHub PR local): {self.show_files}") - detected = True - except Exception as local_error: - log.debug(f"Local diff failed: {local_error}") - except Exception as error: - log.debug(f"Failed to fetch branches or diff for GitHub PR: {error}") + + buildkite_pr = os.getenv('BUILDKITE_PULL_REQUEST') + buildkite_base_ref = os.getenv('BUILDKITE_PULL_REQUEST_BASE_BRANCH') + buildkite_head_ref = os.getenv('BUILDKITE_BRANCH') + if self._is_buildkite_pull_request(buildkite_pr) and buildkite_base_ref: + detected = self._detect_pull_request_changes( + provider="Buildkite", + base_ref=buildkite_base_ref, + head_ref=buildkite_head_ref, + ) + if detected: + detection_source = "buildkite-pr" + elif github_event_name == 'pull_request' and github_base_ref: + detected = self._detect_pull_request_changes( + provider="GitHub", + base_ref=github_base_ref, + head_ref=github_head_ref, + ) + if detected: + detection_source = "github-pr" # Commits to default branch (push events) elif github_event_name == 'push' and github_before_sha and github_sha: try: @@ -182,6 +187,7 @@ def __init__(self, path: str): self.show_files = diff_files.splitlines() log.debug(f"Changed files detected via git diff (GitHub push): {self.show_files}") detected = True + detection_source = "github-push" except Exception as error: log.debug(f"Failed to get changed files via git diff (GitHub push): {error}") elif github_event_name == 'push': @@ -189,6 +195,7 @@ def __init__(self, path: str): self.show_files = self.repo.git.show(self.commit, name_only=True, format="%n").splitlines() log.debug(f"Changed files detected via git show (GitHub push fallback): {self.show_files}") detected = True + detection_source = "github-push-fallback" except Exception as error: log.debug(f"Failed to get changed files via git show (GitHub push fallback): {error}") # GitLab CI Merge Request context @@ -196,15 +203,13 @@ def __init__(self, path: str): gitlab_target = os.getenv('CI_MERGE_REQUEST_TARGET_BRANCH_NAME') gitlab_source = os.getenv('CI_MERGE_REQUEST_SOURCE_BRANCH_NAME') if gitlab_target and gitlab_source: - try: - self.repo.git.fetch('origin', gitlab_target, gitlab_source) - diff_range = f"origin/{gitlab_target}...origin/{gitlab_source}" - diff_files = self.repo.git.diff('--name-only', diff_range) - self.show_files = diff_files.splitlines() - log.debug(f"Changed files detected via git diff (GitLab): {self.show_files}") - detected = True - except Exception as error: - log.debug(f"Failed to get changed files via git diff (GitLab): {error}") + detected = self._detect_pull_request_changes( + provider="GitLab", + base_ref=gitlab_target, + head_ref=gitlab_source, + ) + if detected: + detection_source = "gitlab-mr" # Bitbucket Pipelines PR context if not detected: bitbucket_pr_id = os.getenv('BITBUCKET_PR_ID') @@ -212,15 +217,13 @@ def __init__(self, path: str): bitbucket_dest = os.getenv('BITBUCKET_PR_DESTINATION_BRANCH') # BITBUCKET_BRANCH is the source branch in PR builds if bitbucket_pr_id and bitbucket_source and bitbucket_dest: - try: - self.repo.git.fetch('origin', bitbucket_dest, bitbucket_source) - diff_range = f"origin/{bitbucket_dest}...origin/{bitbucket_source}" - diff_files = self.repo.git.diff('--name-only', diff_range) - self.show_files = diff_files.splitlines() - log.debug(f"Changed files detected via git diff (Bitbucket): {self.show_files}") - detected = True - except Exception as error: - log.debug(f"Failed to get changed files via git diff (Bitbucket): {error}") + detected = self._detect_pull_request_changes( + provider="Bitbucket", + base_ref=bitbucket_dest, + head_ref=bitbucket_source, + ) + if detected: + detection_source = "bitbucket-pr" # Fallback to git show for single commit if not detected: # Check if this is a merge commit first @@ -233,20 +236,132 @@ def __init__(self, path: str): self.show_files = self.repo.git.show(self.commit, name_only=True, format="%n").splitlines() log.debug(f"Changed files detected via git show (merge commit fallback): {self.show_files}") detected = True + detection_source = "merge-commit-fallback" + if detected and detection_source == "single-commit": + detection_source = "merge-diff" else: # Regular single commit self.show_files = self.repo.git.show(self.commit, name_only=True, format="%n").splitlines() log.debug(f"Changed files detected via git show: {self.show_files}") detected = True + detection_source = "single-commit" self.changed_files = [] for item in self.show_files: if item != "": # Use relative path for glob matching self.changed_files.append(item) + + log.info( + "Changed-file detection completed in " + f"{time.perf_counter() - changed_files_start:.2f}s: " + f"source={detection_source}, files={len(self.changed_files)}" + ) # Determine if this commit is on the default branch # This considers both GitHub Actions detached HEAD and regular branch situations self.is_default_branch = self._is_commit_and_branch_default() + log.info( + "Git initialization completed in " + f"{time.perf_counter() - initialization_start:.2f}s" + ) + + @staticmethod + def _is_buildkite_pull_request(pull_request: str | None) -> bool: + return bool(pull_request and pull_request.casefold() != "false") + + def _resolve_ref(self, ref: str | None) -> str | None: + """Resolve a branch, tag, or SHA without accessing the network.""" + if not ref: + return None + if ref in self._fetched_ref_commits: + return self._fetched_ref_commits[ref] + + candidates = [ref] + if not ref.startswith("refs/"): + candidates = [f"origin/{ref}", ref] + for candidate in candidates: + try: + return self.repo.commit(candidate).hexsha + except Exception: + continue + return None + + def _fetch_ref(self, ref: str, reason: str) -> str | None: + """Fetch one required ref and return its commit without broadening scope.""" + if ref in self._fetched_ref_commits: + return self._fetched_ref_commits[ref] + + fetch_start = time.perf_counter() + try: + self.repo.git.fetch("origin", ref) + commit_sha = self.repo.commit("FETCH_HEAD").hexsha + self._fetched_ref_commits[ref] = commit_sha + log.info( + "Git fetch completed in " + f"{time.perf_counter() - fetch_start:.2f}s: " + f"remote=origin, ref={ref}, reason={reason}" + ) + return commit_sha + except Exception as error: + log.info( + "Git fetch failed in " + f"{time.perf_counter() - fetch_start:.2f}s: " + f"remote=origin, ref={ref}, reason={reason}" + ) + log.debug(f"Targeted fetch failed for {ref}: {error}") + return None + + def _detect_pull_request_changes( + self, + provider: str, + base_ref: str, + head_ref: str | None, + ) -> bool: + """Detect a full PR range locally, fetching only refs needed to complete it.""" + base_commit = self._resolve_ref(base_ref) + if base_commit is None: + base_commit = self._fetch_ref(base_ref, f"{provider} pull-request base ref missing") + if base_commit is None: + log.debug(f"Unable to resolve {provider} pull-request base ref: {base_ref}") + return False + + head_commit = self.commit.hexsha + diff_range = f"{base_commit}...{head_commit}" + try: + diff_files = self.repo.git.diff("--name-only", diff_range) + self.show_files = diff_files.splitlines() + log.debug( + f"Changed files detected via local git diff ({provider}): {self.show_files}" + ) + return True + except Exception as local_error: + log.debug(f"Local {provider} pull-request diff failed: {local_error}") + + # A shallow checkout can contain both tips but not their merge base. In + # that case refresh only the two relevant branch histories and retry. + base_commit = self._fetch_ref( + base_ref, + f"{provider} pull-request history incomplete", + ) or base_commit + if head_ref: + self._fetch_ref( + head_ref, + f"{provider} pull-request history incomplete", + ) + + try: + diff_files = self.repo.git.diff( + "--name-only", + f"{base_commit}...{head_commit}", + ) + self.show_files = diff_files.splitlines() + log.debug( + f"Changed files detected after targeted fetch ({provider}): {self.show_files}" + ) + return True + except Exception as retry_error: + log.debug(f"Targeted {provider} pull-request diff failed: {retry_error}") + return False def _is_commit_and_branch_default(self) -> bool: """ @@ -268,9 +383,29 @@ def _is_commit_and_branch_default(self) -> bool: gitlab_mr_branch = os.getenv('CI_MERGE_REQUEST_SOURCE_BRANCH_NAME') gitlab_default_branch = os.getenv('CI_DEFAULT_BRANCH', '') bitbucket_branch = os.getenv('BITBUCKET_BRANCH') + buildkite_branch = os.getenv('BUILDKITE_BRANCH') + buildkite_pr = os.getenv('BUILDKITE_PULL_REQUEST') + buildkite_default_branch = os.getenv('BUILDKITE_PIPELINE_DEFAULT_BRANCH') + # Handle Buildkite before GitHub because some Buildkite pipelines + # intentionally provide GitHub-compatible environment variables. + if buildkite_branch: + if self._is_buildkite_pull_request(buildkite_pr): + log.debug( + f"Processing Buildkite pull request from branch: {buildkite_branch}, " + "not default branch" + ) + return False + default_branch_name = buildkite_default_branch or self.get_default_branch_name() + is_default = buildkite_branch == default_branch_name + log.debug( + f"Buildkite branch: {buildkite_branch}, Default: {default_branch_name}, " + f"Is default: {is_default}" + ) + return is_default + # Handle GitHub Actions - if github_ref: + elif github_ref: log.debug(f"GitHub ref: {github_ref}") # Handle pull requests - they're not on the default branch @@ -483,7 +618,7 @@ def get_default_branch_name(self) -> str: if f'origin/{branch_name}' in [str(ref) for ref in self.repo.remotes.origin.refs]: log.debug(f"Using fallback default branch: {branch_name}") return branch_name - except: + except Exception: continue # Last fallback: assume 'main' @@ -505,12 +640,12 @@ def is_commit_on_default_branch(self) -> bool: # Try remote branch first default_branch_ref = self.repo.remotes.origin.refs[default_branch] default_branch_commit = default_branch_ref.commit - except: + except Exception: # Fallback to local branch try: default_branch_ref = self.repo.heads[default_branch] default_branch_commit = default_branch_ref.commit - except: + except Exception: log.debug(f"Could not find default branch '{default_branch}' locally or remotely") return False @@ -572,4 +707,4 @@ def ensure_safe_directory(path: str) -> None: else: log.debug(f"{abs_path} already present in git safe.directory config.") except Exception as safe_error: - log.debug(f"Failed to set safe.directory for git: {safe_error}") \ No newline at end of file + log.debug(f"Failed to set safe.directory for git: {safe_error}") diff --git a/socketsecurity/core/git_remote.py b/socketsecurity/core/git_remote.py new file mode 100644 index 00000000..eb5b9c02 --- /dev/null +++ b/socketsecurity/core/git_remote.py @@ -0,0 +1,43 @@ +"""Parsing for git remote URLs. + +CI systems that are not tied to a single SCM expose the checkout URL rather than +an ``owner/repo`` slug (Buildkite's ``BUILDKITE_REPO``, for example). Both the +GitHub comment adapter and pull request context resolution need to recover the +slug from it, so the parsing lives here rather than in either caller. +""" +import re +from typing import Optional, Tuple +from urllib.parse import urlparse + +# git@host:owner/repo - the scp-like syntax urlparse cannot handle. The negative +# lookahead keeps scheme-prefixed URLs (https://, ssh://) out of this case. +_SCP_LIKE_REMOTE = re.compile(r"^(?:[^@/]+@)?([^:/]+):(?!//)(.+)$") + + +def parse_git_remote(value: Optional[str]) -> Tuple[Optional[str], Optional[str]]: + """Split a git remote URL into its host and its repository path. + + Returns ``(host, path)``, or ``(None, None)`` when the value is not a usable + remote. The path is returned whole rather than as ``owner``/``repo`` because + GitLab projects can be nested under subgroups; callers that only want the + last two segments can split it themselves. ``host`` is ``None`` for a bare + ``owner/repo`` path, which carries no host to report. + """ + if not value: + return None, None + url = value.strip().rstrip("/") + if url.endswith(".git"): + url = url[:-4] + + match = _SCP_LIKE_REMOTE.match(url) + if match: + return match.group(1), match.group(2).strip("/") + + parsed = urlparse(url) + if parsed.scheme in ("http", "https", "ssh", "git") and parsed.hostname: + return parsed.hostname, parsed.path.strip("/") + + # A bare owner/repo path, with no scheme and nothing to infer a host from. + if "/" in url: + return None, url.strip("/") + return None, None diff --git a/socketsecurity/core/helper/__init__.py b/socketsecurity/core/helper/__init__.py index ab7d06f7..224f3cc7 100644 --- a/socketsecurity/core/helper/__init__.py +++ b/socketsecurity/core/helper/__init__.py @@ -1,7 +1,8 @@ +import string + import markdown from bs4 import BeautifulSoup, Tag from bs4.element import NavigableString -import string class Helper: diff --git a/socketsecurity/core/helper/socket_facts_loader.py b/socketsecurity/core/helper/socket_facts_loader.py index fd93b9dd..26c1ae25 100644 --- a/socketsecurity/core/helper/socket_facts_loader.py +++ b/socketsecurity/core/helper/socket_facts_loader.py @@ -2,9 +2,9 @@ import json import logging -from pathlib import Path -from typing import Dict, Any, Optional, List from copy import deepcopy +from pathlib import Path +from typing import Any, Dict, List, Optional logger = logging.getLogger(__name__) @@ -65,7 +65,7 @@ def load_socket_facts(file_path: str = ".socket.facts.json") -> Optional[Dict[st return None if 'components' not in data: - logger.warning(f"Socket facts file missing 'components' key") + logger.warning("Socket facts file missing 'components' key") return data diff --git a/socketsecurity/core/lazy_file_loader.py b/socketsecurity/core/lazy_file_loader.py index 9127652b..a5bfa15a 100644 --- a/socketsecurity/core/lazy_file_loader.py +++ b/socketsecurity/core/lazy_file_loader.py @@ -2,9 +2,7 @@ Lazy file loading utilities for efficient manifest file processing. """ import logging -from typing import List, Tuple, Union, BinaryIO -from io import BytesIO -import os +from typing import List, Tuple log = logging.getLogger("socketdev") diff --git a/socketsecurity/core/messages.py b/socketsecurity/core/messages.py index 319e454b..4017a9f8 100644 --- a/socketsecurity/core/messages.py +++ b/socketsecurity/core/messages.py @@ -4,7 +4,9 @@ import re import uuid from datetime import datetime, timezone +from html import escape from pathlib import Path + from mdutils import MdUtils from prettytable import PrettyTable @@ -14,6 +16,13 @@ class Messages: + @staticmethod + def get_patched_version(alert: Issue) -> str: + """Return the first patched version exposed by an alert, if any.""" + props = getattr(alert, "props", {}) or {} + value = props.get("firstPatchedVersionIdentifier") + return str(value) if value not in (None, "") else "" + @staticmethod def map_severity_to_sarif(severity: str) -> str: """ @@ -647,32 +656,48 @@ def extract_identifiers_gitlab(alert: Issue) -> list: """ identifiers = [] - # Primary identifier: Socket alert type - identifiers.append({ + # Primary identifier: Socket alert type. The GitLab schema types identifier + # url as a string matching ^(https?|ftp)://, so an absent url is omitted + # rather than sent as null, which fails validation for the whole finding. + socket_identifier = { "type": "socket_alert", "name": f"Socket {alert.type}", "value": alert.type, - "url": alert.url if hasattr(alert, 'url') and alert.url else None - }) - - # Extract CVE identifiers from props - if hasattr(alert, 'props') and alert.props: - if 'cve' in alert.props: - cves = alert.props['cve'] - if isinstance(cves, list): - for cve in cves: - identifiers.append({ - "type": "cve", - "name": cve, - "value": cve, - "url": f"https://cve.mitre.org/cgi-bin/cvename.cgi?name={cve}" - }) - elif isinstance(cves, str): + } + alert_url = getattr(alert, "url", None) + if alert_url: + socket_identifier["url"] = alert_url + identifiers.append(socket_identifier) + + props = getattr(alert, "props", None) or {} + # Alerts reach Issue.props from several sources, so both spellings of each + # field are in play; core.alert_selection matches on the same pair. "cve" is + # the older spelling and still appears in some payloads. + identifier_fields = ( + (("cveId", "cve_id", "cve"), "cve", "https://nvd.nist.gov/vuln/detail/"), + (("ghsaId", "ghsa_id"), "ghsa", "https://github.com/advisories/"), + ) + seen = set() + for fields, identifier_type, url_prefix in identifier_fields: + for field in fields: + values = props.get(field) + if isinstance(values, str): + values = [values] + elif not isinstance(values, (list, tuple)): + continue + for value in values: + if not isinstance(value, str) or not value.strip(): + continue + value = value.strip() + identifier_key = (identifier_type, value.upper()) + if identifier_key in seen: + continue + seen.add(identifier_key) identifiers.append({ - "type": "cve", - "name": cves, - "value": cves, - "url": f"https://cve.mitre.org/cgi-bin/cvename.cgi?name={cves}" + "type": identifier_type, + "name": value, + "value": value, + "url": f"{url_prefix}{value}" }) return identifiers @@ -685,37 +710,35 @@ def extract_location_gitlab(alert: Issue) -> dict: GitLab location requires: - file: path to manifest file - dependency: package name and version - - dependency_path (optional): dependency chain """ - # Get manifest file from introduced_by or manifests attribute - manifest_file = "unknown" - dependency_path = [] - is_direct = True - - if hasattr(alert, 'introduced_by') and alert.introduced_by: - if isinstance(alert.introduced_by, list) and len(alert.introduced_by) > 0: - first_entry = alert.introduced_by[0] - if isinstance(first_entry, (list, tuple)) and len(first_entry) >= 2: - dependency_path_str = first_entry[0] - manifest_file = first_entry[1].split(';')[0] if ';' in first_entry[1] else first_entry[1] - - # Parse dependency path - if ' > ' in dependency_path_str: - dependency_path = dependency_path_str.split(' > ') - # If there's a chain, it's transitive (not direct) - is_direct = len(dependency_path) <= 1 - - elif hasattr(alert, 'manifests') and alert.manifests: - manifest_file = alert.manifests.split(';')[0] + manifest_file = "" + + introduced_by = getattr(alert, "introduced_by", None) + if isinstance(introduced_by, list) and introduced_by: + first_entry = introduced_by[0] + if isinstance(first_entry, (list, tuple)) and len(first_entry) >= 2: + manifest_file = (first_entry[1] or "").split(";")[0] + + if not manifest_file: + manifest_file = (getattr(alert, "manifests", "") or "").split(";")[0] + + if not manifest_file: + # A transitive package whose ancestors are not in this scan has no + # introduced_by chain, but the package still records its own manifest. + for entry in getattr(alert, "manifest_files", None) or []: + candidate = entry.get("file") if isinstance(entry, dict) else None + if candidate: + manifest_file = candidate + break location = { - "file": manifest_file, + "file": manifest_file or "unknown", "dependency": { "package": { "name": alert.pkg_name }, "version": alert.pkg_version, - "direct": is_direct + "direct": bool(getattr(alert, "direct", False)) } } @@ -806,6 +829,121 @@ def create_security_comment_gitlab(diff: Diff) -> dict: return gitlab_report + # A blank line terminates a CommonMark HTML block. When that happens inside + # the alerts table the closing tags that follow are no longer treated as + # markup, and because they are indented four or more spaces they render as a + # literal code block containing `` instead. + MAX_HTML_INDENT = 3 + + @staticmethod + def inline_html_text(value) -> str: + """ + Collapses API supplied text onto a single line. + + Alert descriptions and suggestions are interpolated into the comment HTML, + so an embedded newline would otherwise be able to close the surrounding + HTML block early. + + :param value: The value to flatten. ``None`` becomes an empty string. + :return: str - The value with all whitespace runs collapsed to a single space. + """ + if value is None: + return "" + return " ".join(str(value).split()) + + @staticmethod + def html_text(value) -> str: + """Flatten a value onto one line and escape it for an HTML text node. + + Manifest paths and sources come from the customer's repository, so any PR + author controls them: a directory named ``![x](https://host/p.png)`` or + carrying a raw tag would otherwise render as that markup inside a comment + posted by a trusted integration. Alert text comes from the API and is + escaped for the same reason, since neither is markup the CLI authored. + """ + return escape(Messages.inline_html_text(value)) + + @staticmethod + def html_attr(value) -> str: + """Escape a value for an HTML attribute, quotes included. + + Used for href and src, where an unescaped quote closes the attribute and + everything after it is read as more attributes. + """ + return escape(Messages.inline_html_text(value), quote=True) + + @staticmethod + def comment_marker_text(value) -> str: + """Neutralize an HTML comment terminator inside a marker value. + + The alert markers carry the package name so the comment can be rewritten + later, and the parser reads them back verbatim -- so this cannot escape the + value, only stop it ending the comment early. + """ + return str(value or "").replace("-->", "-->").replace("", + "", + "> **โœ… Socket Security** ", + "> No dependency alerts to report. Any alerts previously reported on this " + "pull request have been resolved or ignored.", + ] + if view_report_url: + lines += ["", f"[View full report]({view_report_url})"] + return "\n".join(lines) + "\n" + + @staticmethod + def get_view_report_url(diff: Diff) -> str: + """ + Resolves the report link for a diff, preferring the PR/MR diff view. + + :param diff: Diff - Diff report to pull the URL from. + :return: str - The report URL, or an empty string when neither is set. + """ + if getattr(diff, "diff_url", None): + return diff.diff_url + if getattr(diff, "report_url", None): + return diff.report_url + return "" + @staticmethod def security_comment_template(diff: Diff, config=None) -> str: """ @@ -819,7 +957,7 @@ def security_comment_template(diff: Diff, config=None) -> str: # Group license policy violations by PURL (ecosystem/package@version) license_groups = {} security_alerts = [] - + for alert in diff.new_alerts: if alert.type == "licenseSpdxDisj": purl_key = f"{alert.pkg_type}/{alert.pkg_name}@{alert.pkg_version}" @@ -829,6 +967,13 @@ def security_comment_template(diff: Diff, config=None) -> str: else: security_alerts.append(alert) + view_report_url = Messages.get_view_report_url(diff) + + # Without this the caution banner would sit above a table with no rows, + # which is how a comment looks once every alert it raised is resolved. + if not security_alerts and not license_groups: + return Messages.security_comment_no_alerts_template(view_report_url) + # Start of the comment comment = """ @@ -836,7 +981,7 @@ def security_comment_template(diff: Diff, config=None) -> str: > **Review the following alerts detected in dependencies.** > > According to your organization's policies, you **must** resolve all **"Block"** alerts before proceeding. It's recommended to resolve **"Warn"** alerts too. -> Learn more about [Socket for GitHub](https://socket.dev?utm_medium=gh). +> Learn more about [Socket](https://socket.dev). @@ -857,39 +1002,48 @@ def security_comment_template(diff: Diff, config=None) -> str: severity_icon = Messages.get_severity_icon(alert.severity) action = "Block" if alert.error else "Warn" details_open = "" + patched_version = Messages.get_patched_version(alert) + patched_version_html = ( + "

Patched version: " + f"{Messages.html_text(patched_version)}

" + if patched_version else "" + ) # Generate proper manifest URL manifest_url = Messages.get_manifest_file_url(diff, alert.manifests, config) + pkg_label = Messages.html_text(f"{alert.pkg_name}@{alert.pkg_version}") + pkg_marker = Messages.comment_marker_text(f"{alert.pkg_name}@{alert.pkg_version}") # Generate a table row for each alert ignore_html = ( f"

Mark as acceptable risk: To ignore this alert only in this pull request, reply with:
" - f"@SocketSecurity ignore {alert.pkg_name}@{alert.pkg_version}
" + f"@SocketSecurity ignore {Messages.html_text(alert.pkg_type)}/{pkg_label}
" f"Or ignore all future alerts with:
" f"@SocketSecurity ignore-all

" ) if show_ignore else "" comment += f""" - + - + """ # Add license policy violation entries grouped by PURL @@ -900,24 +1054,31 @@ def security_comment_template(diff: Diff, config=None) -> str: # Use orange diamond for license policy violations license_icon = "๐Ÿ”ถ" + license_label = Messages.html_text( + f"{first_alert.pkg_name}@{first_alert.pkg_version}" + ) + license_marker = Messages.comment_marker_text( + f"{first_alert.pkg_name}@{first_alert.pkg_version}" + ) + # Build license findings list license_findings = [] for alert in alerts: license_findings.append(alert.title) comment += f""" - + - + """ # Close table - # Use diff_url for PRs, report_url for non-PR scans - view_report_url = "" - if hasattr(diff, 'diff_url') and diff.diff_url: - view_report_url = diff.diff_url - elif hasattr(diff, 'report_url') and diff.report_url: - view_report_url = diff.report_url - comment += f"""
{action} - {alert.severity} + {Messages.html_attr(alert.severity)}
- {alert.pkg_name}@{alert.pkg_version} - {alert.title} -

Note: {alert.description}

-

Source: Manifest File

+ {pkg_label} - {Messages.html_text(alert.title)} +

Note: {Messages.html_text(alert.description)}

+ {patched_version_html} +

Source: Manifest File

โ„น๏ธ Read more on: - This package | - This alert | + This package | + This alert | What is known malware?

-

Suggestion: {alert.suggestion}

+

Suggestion: {Messages.html_text(alert.suggestion)}

{ignore_html}
{action} {license_icon}
- {first_alert.pkg_name}@{first_alert.pkg_version} has a License Policy Violation. + {license_label} has a License Policy Violation.

License findings:

    """ for finding in license_findings: - comment += f"
  • {finding}
    • \n" + comment += f"
  • {Messages.html_text(finding)}
    • \n" # Generate proper manifest URL for license violations @@ -925,13 +1086,13 @@ def security_comment_template(diff: Diff, config=None) -> str: license_ignore_html = ( f"

    Mark the package as acceptable risk: To ignore this alert only in this pull request, reply with the comment " - f"@SocketSecurity ignore {first_alert.pkg_name}@{first_alert.pkg_version}. " + f"@SocketSecurity ignore {Messages.html_text(first_alert.pkg_type)}/{license_label}. " f"You can also ignore all packages with @SocketSecurity ignore-all. " f"To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.

    " ) if show_ignore else "" comment += f"""
-

From: Manifest File

-

โ„น๏ธ Read more on: This package | What is a license policy violation?

+

From: Manifest File

+

โ„น๏ธ Read more on: This package | What is a license policy violation?

Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support@socket.dev.

Suggestion: Find a package that does not violate your license policy or adjust your policy to allow this package's license.

@@ -940,17 +1101,10 @@ def security_comment_template(diff: Diff, config=None) -> str:
@@ -959,7 +1113,7 @@ def security_comment_template(diff: Diff, config=None) -> str: [View full report]({view_report_url}?action=error%2Cwarn) """ - return comment + return Messages.normalize_comment_html(comment) @staticmethod def get_severity_icon(severity: str) -> str: @@ -1147,6 +1301,22 @@ def create_remove_line(diff: Diff, md: MdUtils) -> MdUtils: md.new_line(removed_line) return md + # Change types the shared badge host publishes an image for. Removed and + # replaced have no artwork, so they fall back to a bold text label rather than + # rendering a broken image; added and updated render the available badges. + DIFF_BADGES = { + "Added": "diff-added.svg", + "Updated": "diff-updated.svg", + } + + @staticmethod + def get_diff_badge(change: str, package_url: str) -> str: + """Return the Dependency Overview cell marking how a package changed.""" + badge = Messages.DIFF_BADGES.get(change) + if not badge: + return f"**{change}**" + return f"[![{change}](https://github-app-statics.socket.dev/{badge})]({package_url})" + @staticmethod def create_added_table(diff: Diff, md: MdUtils) -> MdUtils: """ @@ -1168,51 +1338,58 @@ def create_added_table(diff: Diff, md: MdUtils) -> MdUtils: num_of_overview_columns = len(overview_table) count = 0 - for added in diff.new_packages: - added: Purl # Ensure `added` has scores and relevant attributes. - - package_url = f"[{added.purl}]({added.url})" - diff_badge = f"[![+](https://github-app-statics.socket.dev/diff-added.svg)]({added.url})" - - # Scores dynamically converted to badge URLs and linked - def score_to_badge(score): - score_percent = int(score * 100) # Convert to integer percentage - return f"[![{score_percent}](https://github-app-statics.socket.dev/score-{score_percent}.svg)]({added.url})" - - def get_score_for_badge(score_name: str) -> float: - scores = getattr(added, "scores", None) - if isinstance(scores, dict): - raw_score = scores.get(score_name) - else: - raw_score = getattr(scores, score_name, None) if scores is not None else None - - if raw_score is None: - return 1.0 - - score = float(raw_score) - if score > 1: - score = score / 100 - return max(0.0, min(score, 1.0)) - - # Generate badges for each score type - supply_chain_risk_badge = score_to_badge(get_score_for_badge("supplyChain")) - vulnerability_badge = score_to_badge(get_score_for_badge("vulnerability")) - quality_badge = score_to_badge(get_score_for_badge("quality")) - maintenance_badge = score_to_badge(get_score_for_badge("maintenance")) - license_badge = score_to_badge(get_score_for_badge("license")) - - # Add the row for this package - row = [ - diff_badge, - package_url, - supply_chain_risk_badge, - vulnerability_badge, - quality_badge, - maintenance_badge, - license_badge - ] - overview_table.extend(row) - count += 1 # Count total packages + changes = ( + ("Added", diff.new_packages), + ("Updated", diff.updated_packages), + ("Removed", diff.removed_packages), + ("Replaced", diff.replaced_packages), + ) + for change, packages in changes: + for package in packages: + package: Purl + + package_url = f"[{package.purl}]({package.url})" + diff_badge = Messages.get_diff_badge(change, package.url) + + # Scores dynamically converted to badge URLs and linked + def score_to_badge(score): + score_percent = int(score * 100) # Convert to integer percentage + return f"[![{score_percent}](https://github-app-statics.socket.dev/score-{score_percent}.svg)]({package.url})" + + def get_score_for_badge(score_name: str) -> float: + scores = getattr(package, "scores", None) + if isinstance(scores, dict): + raw_score = scores.get(score_name) + else: + raw_score = getattr(scores, score_name, None) if scores is not None else None + + if raw_score is None: + return 1.0 + + score = float(raw_score) + if score > 1: + score = score / 100 + return max(0.0, min(score, 1.0)) + + # Generate badges for each score type + supply_chain_risk_badge = score_to_badge(get_score_for_badge("supplyChain")) + vulnerability_badge = score_to_badge(get_score_for_badge("vulnerability")) + quality_badge = score_to_badge(get_score_for_badge("quality")) + maintenance_badge = score_to_badge(get_score_for_badge("maintenance")) + license_badge = score_to_badge(get_score_for_badge("license")) + + # Add the row for this package + row = [ + diff_badge, + package_url, + supply_chain_risk_badge, + vulnerability_badge, + quality_badge, + maintenance_badge, + license_badge + ] + overview_table.extend(row) + count += 1 # Calculate total rows for table num_of_overview_rows = count + 1 # Include header row @@ -1247,6 +1424,7 @@ def create_console_security_alert_table(diff: Diff) -> PrettyTable: [ "Alert", "Package", + "Patched Version", "url", "Introduced by", "Manifest File", @@ -1267,6 +1445,7 @@ def create_console_security_alert_table(diff: Diff) -> PrettyTable: row = [ alert.title, alert.purl, + Messages.get_patched_version(alert), alert.url, source_str, manifest_str, @@ -1282,8 +1461,11 @@ def create_sources(alert: Issue, style="md") -> tuple[str, str]: for source, manifest in alert.introduced_by: if style == "md": - add_str = f"
  • {manifest}
    • " - source_str = f"
  • {source}
    • " + # These land in rendered Markdown, where an unescaped path is read + # as markup. plain and raw are consumed by Slack, Jira and the + # console, which do not render HTML, so they stay verbatim. + add_str = f"
  • {Messages.html_text(manifest)}
    • " + source_str = f"
  • {Messages.html_text(source)}
    • " elif style == "plain": add_str = f"โ€ข {manifest}" source_str = f"โ€ข {source}" diff --git a/socketsecurity/core/pull_request.py b/socketsecurity/core/pull_request.py new file mode 100644 index 00000000..2dcf77f1 --- /dev/null +++ b/socketsecurity/core/pull_request.py @@ -0,0 +1,144 @@ +import re +from dataclasses import dataclass +from typing import Mapping, Optional +from urllib.parse import urlparse + +from socketsecurity.core.git_remote import parse_git_remote + + +@dataclass(frozen=True) +class PullRequestContext: + number: int = 0 + url: Optional[str] = None + + +def parse_pull_request_number(value) -> int: + """Coerce a configured or CI-supplied pull request number to a positive int. + + Anything that is not a positive integer means "no pull request", including the + literal ``false`` that Buildkite puts in ``BUILDKITE_PULL_REQUEST`` on non-PR + builds. Callers that hand the value on to a comment adapter should store this + result rather than the raw string, which is truthy. + """ + try: + parsed = int(value) + except (TypeError, ValueError): + return 0 + return parsed if parsed > 0 else 0 + + +def _http_url(value: Optional[str]) -> Optional[str]: + """Return ``value`` if it is an http(s) URL with a host, else ``None``. + + Every URL fragment read out of the CI environment goes through here before it + is composed into a link, because the result is sent to the API as a diff scan's + ``external_href``. Standard runners set these variables themselves, so this is + defense in depth rather than a live hole. + """ + if not value: + return None + url = value.strip().rstrip("/") + parsed = urlparse(url) + return url if parsed.scheme in ("http", "https") and parsed.netloc else None + + +def _repository_url(value: Optional[str]) -> Optional[str]: + if not value: + return None + url = value.strip().rstrip("/") + if url.endswith(".git"): + url = url[:-4] + return _http_url(url) + + +def _github_number(env: Mapping[str, str]) -> int: + number = parse_pull_request_number(env.get("PR_NUMBER")) + if number: + return number + match = re.match(r"^refs/pull/(\d+)/", env.get("GITHUB_REF", "")) + return parse_pull_request_number(match.group(1)) if match else 0 + + +def _github_url(number: int, repo: Optional[str], env: Mapping[str, str]) -> Optional[str]: + remote_host, remote_path = parse_git_remote(env.get("BUILDKITE_REPO")) + # config.repo is only ever a bare repository name, so it cannot produce a + # slug on its own; it is kept last for callers that pass a full owner/repo. + repository = env.get("GITHUB_REPOSITORY") or remote_path or repo + if not repository or "/" not in repository: + return None + server = ( + _http_url(env.get("GITHUB_SERVER_URL")) + or (_http_url(f"https://{remote_host}") if remote_host else None) + or "https://github.com" + ) + return f"{server}/{repository.strip('/')}/pull/{number}" + + +def _gitlab_url(number: int, repo: Optional[str], env: Mapping[str, str]) -> Optional[str]: + project_url = _repository_url(env.get("CI_PROJECT_URL")) + 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 = ( + _http_url(env.get("CI_SERVER_URL")) + or (_http_url(f"https://{remote_host}") if remote_host else None) + ) + if server and project_path and "/" in project_path: + project_url = f"{server}/{project_path.strip('/')}" + return f"{project_url}/-/merge_requests/{number}" if project_url else None + + +def _azure_url(number: int, env: Mapping[str, str], github_pr: bool) -> Optional[str]: + repository_url = _repository_url( + env.get("BUILD_REPOSITORY_URI") or + env.get("SYSTEM_PULLREQUEST_SOURCEREPOSITORYURI") + ) + if not repository_url: + return None + github_pr = github_pr or "github" in urlparse(repository_url).netloc.lower() + path = "pull" if github_pr else "pullrequest" + return f"{repository_url}/{path}/{number}" + + +def resolve_pull_request_context( + integration_type: str, + configured_number, + repo: Optional[str], + *, + configured_explicit: bool = False, + env: Optional[Mapping[str, str]] = None, +) -> PullRequestContext: + """Resolve PR metadata without making provider API calls. + + Explicit CLI/config values win, including an explicit zero used to disable + association. Otherwise the provider's standard CI environment is used. + """ + environment = env or {} + provider = str(integration_type or "api").lower() + number = parse_pull_request_number(configured_number) + + if not configured_explicit and not number: + if provider == "github": + number = _github_number(environment) + elif provider == "gitlab": + number = parse_pull_request_number(environment.get("CI_MERGE_REQUEST_IID")) + elif provider == "azure": + number = ( + parse_pull_request_number(environment.get("SYSTEM_PULLREQUEST_PULLREQUESTNUMBER")) or + parse_pull_request_number(environment.get("SYSTEM_PULLREQUEST_PULLREQUESTID")) + ) + + if not number: + return PullRequestContext() + + if provider == "github": + url = _github_url(number, repo, environment) + elif provider == "gitlab": + url = _gitlab_url(number, repo, environment) + elif provider == "azure": + github_pr = bool(environment.get("SYSTEM_PULLREQUEST_PULLREQUESTNUMBER")) + url = _azure_url(number, environment, github_pr) + else: + url = None + + return PullRequestContext(number=number, url=url) diff --git a/socketsecurity/core/resource_utils.py b/socketsecurity/core/resource_utils.py index b49cc2e9..dc78c1b8 100644 --- a/socketsecurity/core/resource_utils.py +++ b/socketsecurity/core/resource_utils.py @@ -2,7 +2,6 @@ System resource utilities for the Socket Security CLI. """ import logging -import sys # The resource module is only available on Unix-like systems resource_available = False diff --git a/socketsecurity/core/scm/client.py b/socketsecurity/core/scm/client.py index 05757117..08769f3f 100644 --- a/socketsecurity/core/scm/client.py +++ b/socketsecurity/core/scm/client.py @@ -2,6 +2,7 @@ from typing import Dict from socketsecurity import USER_AGENT + from ..cli_client import CliClient diff --git a/socketsecurity/core/scm/github.py b/socketsecurity/core/scm/github.py index 7d5905d2..dcfa5607 100644 --- a/socketsecurity/core/scm/github.py +++ b/socketsecurity/core/scm/github.py @@ -8,6 +8,8 @@ from socketsecurity import USER_AGENT from socketsecurity.core import log from socketsecurity.core.classes import Comment +from socketsecurity.core.exceptions import APIFailure +from socketsecurity.core.git_remote import parse_git_remote from socketsecurity.core.scm_comments import Comments from socketsecurity.socketcli import CliClient @@ -34,6 +36,19 @@ class GithubConfig: event_action: Optional[str] headers: dict + @staticmethod + def _repository_from_buildkite() -> tuple[str, str]: + """Return ``(owner, repository)`` from Buildkite's Git repository URL.""" + # Comments and statuses belong to the pipeline/base repository, not a + # contributor's fork from BUILDKITE_PULL_REQUEST_REPO. + _, repository_path = parse_git_remote( + os.getenv("BUILDKITE_REPO") or os.getenv("BUILDKITE_PULL_REQUEST_REPO") + ) + parts = repository_path.split("/") if repository_path else [] + if len(parts) < 2: + return "", "" + return parts[-2], parts[-1] + @classmethod def from_env(cls, pr_number: Optional[str] = None) -> 'GithubConfig': """Create config from environment variables with optional overrides""" @@ -42,12 +57,24 @@ def from_env(cls, pr_number: Optional[str] = None) -> 'GithubConfig': log.error("Unable to get Github API Token from GH_API_TOKEN") sys.exit(2) - # Use provided PR number if available, otherwise fall back to env var + is_buildkite = os.getenv("BUILDKITE") == "true" + buildkite_pr = os.getenv("BUILDKITE_PULL_REQUEST") + is_buildkite_pr = bool( + is_buildkite + and buildkite_pr + and buildkite_pr.casefold() != "false" + ) + + # Use explicit/GitHub-compatible values first, then native Buildkite PR context. pr_number = pr_number or os.getenv('PR_NUMBER') + if not pr_number and is_buildkite_pr: + pr_number = buildkite_pr # Add debug logging - sha = os.getenv('GITHUB_SHA', '') - log.debug(f"Loading SHA from GITHUB_SHA: {sha}") + sha = os.getenv('GITHUB_SHA') or ( + os.getenv("BUILDKITE_COMMIT", "") if is_buildkite else "" + ) + log.debug(f"Loading GitHub integration SHA: {sha}") event_action = os.getenv('EVENT_ACTION', None) if not event_action: event_path = os.getenv('GITHUB_EVENT_PATH') @@ -55,29 +82,66 @@ def from_env(cls, pr_number: Optional[str] = None) -> 'GithubConfig': with open(event_path, 'r') as f: event = json.load(f) event_action = event.get('action') + if not event_action and is_buildkite_pr: + # Buildkite provides the current PR state, not the originating + # GitHub webhook action. A running PR build is equivalent to the + # supported synchronize path for comment updates. + event_action = "synchronize" repository = os.getenv('GITHUB_REPOSITORY', '') owner = os.getenv('GITHUB_REPOSITORY_OWNER', '') if '/' in repository: owner = repository.split('/')[0] repository = repository.split('/')[1] + elif is_buildkite: + buildkite_owner, buildkite_repository = cls._repository_from_buildkite() + owner = owner or buildkite_owner + repository = repository or buildkite_repository default_branch_env = os.getenv('DEFAULT_BRANCH') # Consider the variable truthy if it exists and isn't explicitly 'false' - is_default = default_branch_env is not None and default_branch_env.lower() != 'false' + if default_branch_env is not None: + is_default = default_branch_env.lower() != 'false' + elif is_buildkite: + # Require a branch name: comparing two unset variables would otherwise report + # every build as the default branch and overwrite the repository's baseline. + buildkite_branch = os.getenv("BUILDKITE_BRANCH") + is_default = bool( + not is_buildkite_pr + and buildkite_branch + and buildkite_branch == os.getenv("BUILDKITE_PIPELINE_DEFAULT_BRANCH") + ) + else: + is_default = False + + event_name = os.getenv('GITHUB_EVENT_NAME', '') + if not event_name and is_buildkite: + event_name = "pull_request" if is_buildkite_pr else "push" return cls( - sha=os.getenv('GITHUB_SHA', ''), - api_url=os.getenv('GITHUB_API_URL', ''), - ref_type=os.getenv('GITHUB_REF_TYPE', ''), - event_name=os.getenv('GITHUB_EVENT_NAME', ''), - workspace=os.getenv('GITHUB_WORKSPACE', ''), + sha=sha, + api_url=os.getenv('GITHUB_API_URL') or ( + "https://api.github.com" if is_buildkite else "" + ), + ref_type=os.getenv('GITHUB_REF_TYPE') or ( + "branch" if is_buildkite else "" + ), + event_name=event_name, + workspace=os.getenv('GITHUB_WORKSPACE') or ( + os.getenv("BUILDKITE_BUILD_CHECKOUT_PATH", "") if is_buildkite else "" + ), repository=repository, - ref_name=os.getenv('GITHUB_REF_NAME', ''), + ref_name=os.getenv('GITHUB_REF_NAME') or ( + os.getenv("BUILDKITE_BRANCH", "") if is_buildkite else "" + ), default_branch=is_default, is_default_branch=is_default, pr_number=pr_number, pr_name=os.getenv('PR_NAME'), - commit_message=os.getenv('COMMIT_MESSAGE'), - actor=os.getenv('GITHUB_ACTOR', ''), + commit_message=os.getenv('COMMIT_MESSAGE') or ( + os.getenv("BUILDKITE_MESSAGE") if is_buildkite else None + ), + actor=os.getenv('GITHUB_ACTOR') or ( + os.getenv("BUILDKITE_BUILD_CREATOR", "") if is_buildkite else "" + ), env=os.getenv('GITHUB_ENV', ''), token=token, owner=owner, @@ -91,9 +155,21 @@ def from_env(cls, pr_number: Optional[str] = None) -> 'GithubConfig': class Github: - def __init__(self, client: CliClient, config: Optional[GithubConfig] = None): + WRITE_PERMISSIONS = frozenset({"write", "maintain", "admin"}) + + def __init__( + self, + client: CliClient, + config: Optional[GithubConfig] = None, + ignore_authorization: str = "enforce", + ): self.config = config or GithubConfig.from_env() self.client = client + self.ignore_authorization = ignore_authorization + # Permission is stable for the duration of one CLI run. Cache both + # positive and negative answers so several ignore comments by the same + # author do not each make an API request. + self._ignore_permission_cache: dict[str, Optional[bool]] = {} if not self.config.token: log.error("Unable to get Github API Token") @@ -161,7 +237,76 @@ def get_comments_for_pr(self) -> dict: else: log.error(raw_comments) - return Comments.check_for_socket_comments(comments) + gate = None if self.ignore_authorization == "off" else self.is_ignore_authorized + return Comments.check_for_socket_comments(comments, gate) + + def is_ignore_authorized(self, comment: Comment) -> bool: + """Whether a commenter may suppress alerts with @SocketSecurity ignore. + + ``author_association`` describes a social relationship to the repository, + not the author's role: an organization member or outside collaborator can + still have read-only access. Ask GitHub for the effective repository + permission instead, and cache the answer for subsequent comments. + """ + author = Comments.comment_author_name(comment) + if author == "an unknown user": + permission = None + elif author in self._ignore_permission_cache: + permission = self._ignore_permission_cache[author] + else: + path = ( + f"repos/{self.config.owner}/{self.config.repository}/" + f"collaborators/{author}/permission" + ) + try: + response = self.client.request( + path=path, + headers=self.config.headers, + base_url=self.config.api_url, + ) + result = response.json() + if not isinstance(result, dict) or not isinstance( + result.get("permission"), str + ): + log.warning("Unexpected GitHub repository permission response") + permission = None + else: + permission = ( + result["permission"].casefold() in self.WRITE_PERMISSIONS + ) + except APIFailure as error: + if getattr(error, "status_code", None) == 404: + # The repository was readable when its comments were listed, + # so a missing collaborator permission is a definitive denial. + permission = False + else: + log.warning( + "Could not read GitHub repository permission for " + f"{author}: {error}" + ) + permission = None + except Exception as error: + log.warning( + f"Could not read GitHub repository permission for {author}: {error}" + ) + permission = None + self._ignore_permission_cache[author] = permission + + if permission is not None: + return permission + if self.ignore_authorization == "strict": + log.warning( + f"Rejecting @SocketSecurity ignore from {author}: GitHub repository " + "permission could not be read and --ignore-authorization is strict." + ) + return False + log.warning( + f"Honoring @SocketSecurity ignore from {author} without verifying write " + "access: GitHub repository permission could not be read. Use a token " + "with repository metadata access, or --ignore-authorization strict to " + "reject instead." + ) + return True def add_socket_comments( self, diff --git a/socketsecurity/core/scm/gitlab.py b/socketsecurity/core/scm/gitlab.py index b3b3492f..d1fd1119 100644 --- a/socketsecurity/core/scm/gitlab.py +++ b/socketsecurity/core/scm/gitlab.py @@ -2,9 +2,11 @@ import os import sys from dataclasses import dataclass -from typing import Optional, Union +from typing import Optional import requests +from socketdev.exceptions import APIFailure + from socketsecurity import USER_AGENT from socketsecurity.core import log from socketsecurity.core.classes import Comment @@ -125,37 +127,54 @@ def _get_auth_headers(token: str) -> dict: } class Gitlab: - def __init__(self, client: CliClient, config: Optional[GitlabConfig] = None): + # GitLab access levels: 30 Developer, 40 Maintainer, 50 Owner. Reporter (20) + # and Guest (10) cannot push, so they cannot suppress an alert either. + MIN_IGNORE_ACCESS_LEVEL = 30 + # Bounded so a project with a very large membership cannot stall a scan. Past + # the cap the answer is "undetermined", handled the same as a failed lookup. + MEMBER_PAGE_SIZE = 100 + MEMBER_PAGE_LIMIT = 10 + + def __init__( + self, + client: CliClient, + config: Optional[GitlabConfig] = None, + ignore_authorization: str = "enforce", + ): self.config = config or GitlabConfig.from_env() self.client = client + self.ignore_authorization = ignore_authorization + # None until the first ignore comment forces a lookup; stays None when the + # members API cannot be read, which is the "undetermined" state. + self._member_access: Optional[dict] = None + self._member_lookup_attempted = False def _request_with_fallback(self, **kwargs): - """ - Make a request with automatic fallback between Bearer and PRIVATE-TOKEN authentication. - This provides robustness when the initial token type detection is incorrect. + """Request with one retry under the other GitLab auth scheme on a 401. + + _get_auth_headers guesses between Bearer and PRIVATE-TOKEN from the shape of + the token, and the guess can be wrong for tokens that do not match a known + pattern. Rather than fail the run, try the other scheme once. + + Catches APIFailure, not requests.exceptions.HTTPError: CliClient translates + every requests error into APIFailure, which does not inherit from HTTPError, + so catching the latter here never fired and the fallback never ran. """ try: - # Try the initial request with the configured headers return self.client.request(**kwargs) - except requests.exceptions.HTTPError as e: - # Check if this is an authentication error (401) - if e.response and e.response.status_code == 401: - log.debug(f"Authentication failed with initial headers, trying fallback method") - - # Determine the fallback headers - original_headers = kwargs.get('headers', self.config.headers) - fallback_headers = self._get_fallback_headers(original_headers) - - if fallback_headers and fallback_headers != original_headers: - log.debug("Retrying request with fallback authentication method") - kwargs['headers'] = fallback_headers - return self.client.request(**kwargs) - - # Re-raise the original exception if it's not an auth error or fallback failed - raise - except Exception as e: - # Handle other types of exceptions that don't have response attribute - raise + except APIFailure as error: + if error.status_code != 401: + raise + + log.debug("Authentication failed with initial headers, trying fallback method") + original_headers = kwargs.get('headers', self.config.headers) + fallback_headers = self._get_fallback_headers(original_headers) + if not fallback_headers or fallback_headers == original_headers: + raise + + log.debug("Retrying request with fallback authentication method") + kwargs['headers'] = fallback_headers + return self.client.request(**kwargs) def _get_fallback_headers(self, original_headers: dict) -> dict: """ @@ -255,7 +274,88 @@ def get_comments_for_pr(self) -> dict: comment.body_list = comment.body.split("\n") else: log.error(raw_comments) - return Comments.check_for_socket_comments(comments) + gate = None if self.ignore_authorization == "off" else self.is_ignore_authorized + return Comments.check_for_socket_comments(comments, gate) + + def _load_member_access(self) -> Optional[dict]: + """Map project member user id -> access level, or None if unreadable. + + ``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 this guards against -- would be indistinguishable from a + token that cannot read the endpoint, and would have to fail open. + """ + if self._member_lookup_attempted: + return self._member_access + self._member_lookup_attempted = True + if not self.config.mr_project_id: + return None + + access: dict = {} + for page in range(1, Gitlab.MEMBER_PAGE_LIMIT + 1): + path = ( + f"projects/{self.config.mr_project_id}/members/all" + f"?per_page={Gitlab.MEMBER_PAGE_SIZE}&page={page}" + ) + try: + response = self._request_with_fallback( + path=path, + headers=self.config.headers, + base_url=self.config.api_url + ) + members = response.json() + except Exception as error: + log.warning(f"Could not read GitLab project members: {error}") + return None + if not isinstance(members, list): + log.warning("Unexpected GitLab project members response") + return None + for member in members: + if isinstance(member, dict) and member.get("id") is not None: + access[member["id"]] = member.get("access_level") or 0 + if len(members) < Gitlab.MEMBER_PAGE_SIZE: + self._member_access = access + return access + + log.warning( + f"GitLab project has more than {Gitlab.MEMBER_PAGE_SIZE * Gitlab.MEMBER_PAGE_LIMIT} " + "members; cannot confirm ignore-command authorization" + ) + return None + + def is_ignore_authorized(self, comment: Comment) -> bool: + """Whether a commenter may suppress alerts with @SocketSecurity ignore. + + GitLab notes carry no permission field, so this costs one members lookup + per run (cached, and only when an ignore command is actually present). + + When membership can be read the answer is definitive. When it cannot -- a + CI_JOB_TOKEN generally cannot read the members API -- the command is + honored and a warning is logged, preserving compatibility for pipelines + that rely on ignore commands. Set a token with API read access to get + enforcement. + """ + access = self._load_member_access() + if access is None: + author = Comments.comment_author_name(comment) + if self.ignore_authorization == "strict": + log.warning( + f"Rejecting @SocketSecurity ignore from {author}: GitLab project " + "membership could not be read and --ignore-authorization is strict." + ) + return False + log.warning( + f"Honoring @SocketSecurity ignore from {author} without verifying " + "write access: GitLab project membership could not be read. Use a " + "token with API read access, or --ignore-authorization strict to " + "reject instead." + ) + return True + + author = getattr(comment, "author", None) or {} + user_id = author.get("id") + return access.get(user_id, 0) >= Gitlab.MIN_IGNORE_ACCESS_LEVEL def add_socket_comments( self, diff --git a/socketsecurity/core/scm_comments.py b/socketsecurity/core/scm_comments.py index 741578eb..3ef0e3a6 100644 --- a/socketsecurity/core/scm_comments.py +++ b/socketsecurity/core/scm_comments.py @@ -1,12 +1,23 @@ import json +import re +from typing import Callable, Optional from requests import Response from socketsecurity.core import log from socketsecurity.core.classes import Comment, Issue +from socketsecurity.core.messages import Messages class Comments: + VIEW_REPORT_PATTERN = re.compile(r"\[View full report\]\(([^)\s]+)\)") + + @staticmethod + def comment_author_name(comment: Comment) -> str: + """Best-effort display name for a comment author, across providers.""" + user = getattr(comment, "user", None) or getattr(comment, "author", None) or {} + return user.get("login") or user.get("username") or "an unknown user" + @staticmethod def process_response(response: Response) -> dict: output = {} @@ -33,10 +44,10 @@ def remove_alerts(comments: dict, new_alerts: list) -> list: if ignore_all: break else: - full_name = f"{alert.pkg_type}/{alert.pkg_name}" - purl = (full_name, alert.pkg_version) - purl_star = (full_name, "*") - if purl in ignore_commands or purl_star in ignore_commands: + if any( + Comments.is_ignore(alert.pkg_name, alert.pkg_version, name, version, alert.pkg_type) + for name, version in ignore_commands + ): log.info(f"Alerts for {alert.pkg_name}@{alert.pkg_version} ignored") else: log.info(f"Adding alert {alert.type} for {alert.pkg_name}@{alert.pkg_version}") @@ -62,8 +73,10 @@ def get_ignore_options(comments: dict) -> [bool, list]: ignore_all = True else: command = command.lstrip("ignore").strip() - name, version = command.split("@") - data = (name, version) + name, separator, version = command.rpartition("@") + if not separator or not name or not version: + raise ValueError("Expected package@version") + data = (name.strip(), version.strip()) ignore_commands.append(data) except Exception as error: log.error(f"Unable to process ignore command for {comment}") @@ -71,11 +84,30 @@ def get_ignore_options(comments: dict) -> [bool, list]: return ignore_all, ignore_commands @staticmethod - def is_ignore(pkg_name: str, pkg_version: str, name: str, version: str) -> bool: - result = False - if pkg_name == name and (pkg_version == version or version == "*"): - result = True - return result + def is_ignore( + pkg_name: str, pkg_version: str, name: str, version: str, + pkg_type: str = "" + ) -> bool: + """Match an alert's package against one parsed ignore command. + + Generated commands are ecosystem-qualified (``npm/lodash@4.17.21``) but + replies typed by hand, and commands written by older CLI versions, use the + bare package name, so both have to match. + + Callers that parse the package out of a ``start-socket-alert`` marker have no + pkg_type to compare against and instead strip the ecosystem off the command. + An npm scope looks the same as an ecosystem prefix there, so only strip when + the leading segment cannot be one: without the guard, + ``ignore @types/node@*`` would also silently ignore alerts for a package + literally named ``node``. + """ + package_names = {pkg_name} + if pkg_type: + package_names.add(f"{pkg_type}/{pkg_name}") + 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 == "*") @staticmethod def is_heading_line(line) -> bool: @@ -84,6 +116,20 @@ def is_heading_line(line) -> bool: is_heading_line = False return is_heading_line + @staticmethod + def extract_report_url(body: str) -> str: + """ + Pulls the Socket report link out of an existing comment body so it can be + carried over when the comment is rewritten. + + :param body: str - The existing comment body. + :return: str - The report URL without its query string, or "" if absent. + """ + match = Comments.VIEW_REPORT_PATTERN.search(body) + if not match: + return "" + return match.group(1).split("?", 1)[0] + @staticmethod def process_security_comment(comment: Comment, comments) -> str: ignore_all, ignore_commands = Comments.get_ignore_options(comments) @@ -94,6 +140,33 @@ def process_security_comment(comment: Comment, comments) -> str: return new_body + @staticmethod + def parse_alert_table_row(line: str) -> Optional[tuple[str, str, str]]: + """Pull ``(ecosystem, package, version)`` out of a legacy alert table row. + + Returns None for any row that does not have the expected shape rather than + raising. The row comes back from the provider's API, so its contents are + outside this process's control. Malformed cells must not interrupt status + reporting. A row that cannot be read is a row whose alert stays reported. + """ + cells = line.strip().lstrip("|").rstrip("|").split("|") + if len(cells) != 5: + return None + package = cells[1] + if "](" not in package: + return None + details = package.split("](", 1)[0].lstrip("[") + if "/" not in details: + return None + ecosystem, remainder = details.split("/", 1) + if "@" not in remainder: + return None + # Split from the right: a scoped name carries its own "@". + pkg_name, pkg_version = remainder.rsplit("@", 1) + if not pkg_name or not pkg_version: + return None + return ecosystem, pkg_name, pkg_version + @staticmethod def process_original_security_comment( comment: Comment, @@ -102,29 +175,40 @@ def process_original_security_comment( ) -> str: start = False lines = [] + kept_alert = False for line in comment.body_list: line = line.strip() if "start-socket-alerts-table" in line: start = True lines.append(line) elif start and "end-socket-alerts-table" not in line and not Comments.is_heading_line(line) and line != '': - title, package, introduced_by, manifest, ci = line.lstrip("|").rstrip("|").split("|") - details, _ = package.split("](") - ecosystem, details = details.split("/", 1) - ecosystem = ecosystem.lstrip("[") - pkg_name, pkg_version = details.split("@") - pkg_name = f"{ecosystem}/{pkg_name}" - ignore = False - for name, version in ignore_commands: - if ignore_all or Comments.is_ignore(pkg_name, pkg_version, name, version): - ignore = True + parsed = Comments.parse_alert_table_row(line) + # ignore_all has to be checked outside the loop: an ignore-all + # comment produces no ignore_commands, so a loop-internal check + # never runs and every row was kept. + if parsed is None: + # An unparseable row cannot be evaluated against the ignore + # commands, so keep it: leaving an alert reported is the safe + # direction, and the comment body is not ours to discard. + ignore = ignore_all + else: + ecosystem, pkg_name, pkg_version = parsed + ignore = ignore_all or any( + Comments.is_ignore(pkg_name, pkg_version, name, version, ecosystem) + for name, version in ignore_commands + ) if not ignore: + kept_alert = True lines.append(line) elif "end-socket-alerts-table" in line: start = False lines.append(line) else: lines.append(line) + if not kept_alert: + return Messages.security_comment_no_alerts_template( + Comments.extract_report_url("\n".join(comment.body_list)) + ) return "\n".join(lines) @staticmethod @@ -145,18 +229,22 @@ def process_updated_security_comment( """ lines = [] ignore_section = False + kept_alert = False # Whether any alert row survived the ignore commands pkg_name = pkg_version = "" # Track current package and version # Loop through the comment lines for line in comment.body_list: - line = line.strip() + # Match on the stripped line but keep the original, so the markup is + # rewritten with the same indentation it was generated with. + line = line.rstrip("\r") + stripped = line.strip() # Detect the start of an alert section - if line.startswith("\n\n> text") + assert result == "\n\n> text" + + def test_caps_indentation_below_code_block_threshold(self): + result = Messages.normalize_comment_html(" ") + assert result == " " + + def test_preserves_trailing_markdown_line_breaks(self): + result = Messages.normalize_comment_html("> **Caution** ") + assert result == "> **Caution** " + + +# --- inline_html_text --- + +class TestInlineHtmlText: + def test_collapses_newlines(self): + assert Messages.inline_html_text("one\n\ntwo") == "one two" + + def test_handles_none(self): + assert Messages.inline_html_text(None) == "" + + +# --- Generated comment bodies --- + +class TestSecurityCommentTemplateRendering: + def test_security_alert_row_is_render_safe(self): + body = Messages.security_comment_template(_make_diff([_make_alert()]), _FakeConfig()) + assert_html_block_intact(body) + + def test_render_safe_when_ignore_instructions_disabled(self): + """The empty ignore block used to leave a whitespace-only line behind.""" + body = Messages.security_comment_template( + _make_diff([_make_alert()]), _FakeConfig(disable_ignore=True) + ) + assert_html_block_intact(body) + assert "" in body + assert "@SocketSecurity ignore" not in body + + def test_license_row_render_safe_when_ignore_instructions_disabled(self): + body = Messages.security_comment_template( + _make_diff([_make_alert(type="licenseSpdxDisj", title="LGPL-3.0")]), + _FakeConfig(disable_ignore=True), + ) + assert_html_block_intact(body) + assert "License Policy Violation" in body + + def test_multiline_alert_text_is_flattened(self): + body = Messages.security_comment_template( + _make_diff([ + _make_alert(description="Line one.\n\nLine two.", suggestion="Do this.\nThen that.") + ]), + _FakeConfig(), + ) + assert_html_block_intact(body) + assert "

    Note: Line one. Line two.

    " in body + assert "Do this. Then that." in body + + def test_license_finding_text_is_flattened(self): + body = Messages.security_comment_template( + _make_diff([_make_alert(type="licenseSpdxDisj", title="LGPL-3.0\n\nAND MIT")]), + _FakeConfig(), + ) + assert_html_block_intact(body) + assert "
  • LGPL-3.0 AND MIT
    • " in body + + def test_alert_markers_are_preserved(self): + body = Messages.security_comment_template(_make_diff([_make_alert()]), _FakeConfig()) + assert "" in body + assert "" in body + + def test_copy_is_provider_neutral(self): + body = Messages.security_comment_template( + _make_diff([_make_alert()]), _FakeConfig(scm="gitlab") + ) + assert "Socket for GitHub" not in body + assert "Learn more about [Socket]" in body + + +class TestSecurityCommentTemplateWithNoAlerts: + def test_no_alerts_omits_the_empty_table(self): + body = Messages.security_comment_template(_make_diff([]), _FakeConfig()) + assert "" not in body + assert "Caution" not in body + assert "No dependency alerts to report" in body + + def test_no_alerts_keeps_the_comment_discoverable(self): + """The marker has to survive so a later commit updates this comment + instead of posting a second one.""" + body = Messages.security_comment_template(_make_diff([]), _FakeConfig()) + found = Comments.check_for_socket_comments({1: _make_comment(body)}) + assert "security" in found + + def test_no_alerts_keeps_the_report_link(self): + body = Messages.security_comment_template(_make_diff([]), _FakeConfig()) + assert "[View full report](https://socket.dev/report/abc)" in body + + def test_no_alerts_without_report_url_omits_the_link(self): + diff = Diff() + diff.id = "test-scan-id" + diff.new_alerts = [] + body = Messages.security_comment_template(diff, _FakeConfig()) + assert "View full report" not in body + + +# --- Ignore round trip --- + +def _security_comment_with(alerts: list, config=None) -> Comment: + body = Messages.security_comment_template(_make_diff(alerts), config or _FakeConfig()) + return _make_comment(body) + + +class TestProcessUpdatedSecurityComment: + def _two_alert_comment(self) -> Comment: + return _security_comment_with([ + _make_alert(), + _make_alert(pkg_name="express", pkg_version="4.18.2", purl="pkg:npm/express@4.18.2"), + ]) + + def test_partial_ignore_keeps_remaining_alert_render_safe(self): + security = self._two_alert_comment() + ignore = _make_comment("SocketSecurity ignore lodash@4.17.21", comment_id=2) + comments = {"security": security, "ignore": [ignore]} + + new_body = Comments.process_security_comment(security, comments) + + assert_html_block_intact(new_body) + assert "start-socket-alert-express@4.18.2" in new_body + assert "start-socket-alert-lodash@4.17.21" not in new_body + + def test_ignore_all_collapses_to_the_no_alerts_body(self): + security = self._two_alert_comment() + ignore = _make_comment("SocketSecurity ignore-all", comment_id=2) + comments = {"security": security, "ignore": [ignore]} + + new_body = Comments.process_security_comment(security, comments) + + assert "
    " not in new_body + assert "No dependency alerts to report" in new_body + assert "[View full report](https://socket.dev/report/abc)" in new_body + + def test_ignoring_every_alert_individually_collapses_too(self): + security = self._two_alert_comment() + comments = { + "security": security, + "ignore": [ + _make_comment("SocketSecurity ignore lodash@4.17.21", comment_id=2), + _make_comment("SocketSecurity ignore express@4.18.2", comment_id=3), + ], + } + + new_body = Comments.process_security_comment(security, comments) + + assert "No dependency alerts to report" in new_body + + def test_qualified_scoped_package_ignore_matches_comment_marker(self): + security = _security_comment_with([ + _make_alert( + pkg_name="@socketsecurity/example", + purl="pkg:npm/@socketsecurity/example@4.17.21", + ) + ]) + comments = { + "security": security, + "ignore": [_make_comment( + "SocketSecurity ignore npm/@socketsecurity/example@4.17.21", + comment_id=2, + )], + } + + assert "No dependency alerts to report" in Comments.process_security_comment(security, comments) + + def test_no_ignore_commands_leaves_alerts_in_place(self): + security = self._two_alert_comment() + comments = {"security": security, "ignore": []} + + new_body = Comments.process_security_comment(security, comments) + + assert "start-socket-alert-lodash@4.17.21" in new_body + assert "start-socket-alert-express@4.18.2" in new_body + + def test_collapsed_body_is_stable_when_reprocessed(self): + security = self._two_alert_comment() + ignore = _make_comment("SocketSecurity ignore-all", comment_id=2) + comments = {"security": security, "ignore": [ignore]} + + first = Comments.process_security_comment(security, comments) + comments["security"] = _make_comment(first) + second = Comments.process_security_comment(comments["security"], comments) + + assert first == second + + +LEGACY_COMMENT = """ + + +|Alert|Package|Introduced by|Manifest File|CI| +|:---|:---|:---|:---|:---| +|Known Malware|[npm/lodash@4.17.21](https://socket.dev/x)|lodash|package.json|:no_entry_sign:| +|Known Malware|[npm/express@4.18.2](https://socket.dev/y)|express|package.json|:no_entry_sign:| + + +[View full report](https://socket.dev/report/legacy?action=error%2Cwarn) +""" + +SCOPED_LEGACY_COMMENT = """ + + +|Alert|Package|Introduced by|Manifest File|CI| +|:---|:---|:---|:---|:---| +|Known Malware|[npm/@socketsecurity/example@1.0.0](https://socket.dev/z)|example|package.json|:no_entry_sign:| + + +[View full report](https://socket.dev/report/legacy?action=error%2Cwarn) +""" + + +class TestProcessOriginalSecurityComment: + def test_partial_ignore_keeps_remaining_row(self): + security = _make_comment(LEGACY_COMMENT) + comments = { + "security": security, + "ignore": [_make_comment("SocketSecurity ignore npm/lodash@4.17.21", comment_id=2)], + } + + new_body = Comments.process_security_comment(security, comments) + + assert "npm/express@4.18.2" in new_body + assert "npm/lodash@4.17.21" not in new_body + + def test_ignore_all_collapses_to_the_no_alerts_body(self): + security = _make_comment(LEGACY_COMMENT) + comments = { + "security": security, + "ignore": [_make_comment("SocketSecurity ignore-all", comment_id=2)], + } + + new_body = Comments.process_security_comment(security, comments) + + assert "|Alert|Package|" not in new_body + assert "No dependency alerts to report" in new_body + assert "[View full report](https://socket.dev/report/legacy)" in new_body + + def test_scoped_package_row_does_not_raise(self): + """A scoped name carries its own "@", so the split must come from the right.""" + security = _make_comment(SCOPED_LEGACY_COMMENT) + comments = {"security": security, "ignore": []} + + new_body = Comments.process_security_comment(security, comments) + + assert "npm/@socketsecurity/example@1.0.0" in new_body + + def test_scoped_package_row_is_ignorable_both_ways(self): + for command in ( + "SocketSecurity ignore npm/@socketsecurity/example@1.0.0", + "SocketSecurity ignore @socketsecurity/example@1.0.0", + ): + security = _make_comment(SCOPED_LEGACY_COMMENT) + comments = {"security": security, "ignore": [_make_comment(command, comment_id=2)]} + + new_body = Comments.process_security_comment(security, comments) + + assert "No dependency alerts to report" in new_body, command + + +class TestExtractReportUrl: + def test_strips_the_action_filter(self): + url = Comments.extract_report_url( + "[View full report](https://socket.dev/report/abc?action=error%2Cwarn)" + ) + assert url == "https://socket.dev/report/abc" + + def test_returns_empty_when_absent(self): + assert Comments.extract_report_url("no link here") == "" + + +# --- Escaping repo-derived values --------------------------------------------- +# +# Manifest paths and sources are file paths inside the customer's repository, so +# anyone who can open a pull request controls them: a directory named +# `![x](https://host/p.png)` holding a manifest puts that markup into a comment +# posted by a trusted integration. GitHub and GitLab sanitize comment HTML, so the +# exposure is external resource loading, phishing links and content spoofing +# rather than script execution. + + +@dataclass +class _RepoConfig(_FakeConfig): + """A config that reaches the branch which embeds the path verbatim. + + Without repo/branch, get_manifest_file_url returns "" or a percent-encoded + Socket link, and the path never lands in the comment -- so a test using the + bare config asserts nothing. + """ + repo: str = "acme/widgets" + branch: str = "main" + + +HOSTILE_PATHS = { + "image": "![x](https://evil.example/p.png)/package.json", + "link": "[click me](https://evil.example)/package.json", + "raw_tag": "/package.json", + "backtick": "`code`/package.json", + "pipe": "a|b/package.json", + "quote": 'a" onmouseover="x/package.json', + "comment_close": "x-->y/package.json", +} + + +def _rendered_with_path(path: str) -> str: + return Messages.security_comment_template( + _make_diff([_make_alert(manifests=path)]), _RepoConfig() + ) + + +def test_the_hostile_path_actually_reaches_the_comment(): + """Guards the fixture itself: if the path stops being rendered, the escaping + tests below would pass while asserting nothing.""" + body = _rendered_with_path("sentinel-path/package.json") + + assert "sentinel-path" in body + + +@pytest.mark.parametrize("name,path", sorted(HOSTILE_PATHS.items())) +def test_hostile_manifest_path_cannot_introduce_markup(name, path): + """In the rendered comment the path only ever lands inside an href, where + Markdown is inert. The property that matters there is that the value cannot + open a tag or close the attribute -- see create_sources for the context where + Markdown itself is live.""" + body = _rendered_with_path(path) + + rendered = [ln for ln in body.split("\n") if "Manifest File" in ln][0] + value = rendered.split('href="', 1)[1].split('"', 1)[0] + + for char in ("<", ">", '"'): + assert char not in value, f"{char!r} survived into the href: {value!r}" + assert_html_block_intact(body) + + +def test_quote_in_a_path_cannot_escape_the_href(): + body = _rendered_with_path('a" onmouseover="x/package.json') + + assert """ in body + assert 'href="https://github.com/acme/widgets/blob/main/a" ' not in body + + +def test_hostile_package_name_cannot_close_the_alert_marker(): + body = Messages.security_comment_template( + _make_diff([_make_alert(pkg_name="evil-->x")]), _FakeConfig() + ) + + # Exactly the terminator the CLI wrote, and no stray one inside the value. + for line in body.split("\n"): + if "socket-alert-" in line: + assert line.count("-->") == 1, line + + +def test_alert_text_from_the_api_is_escaped(): + body = Messages.security_comment_template( + _make_diff([_make_alert(description="")]), _FakeConfig() + ) + + assert "