feat(js-pr-validation): add socket.dev supply chain gate - #645
Conversation
Add a Socket supply-chain layer to the JS/TS PR umbrella, covering the gap npm audit, Trivy and CodeQL leave open: malicious install scripts, typosquats and hijacked patch releases. Two independent composites, toggled from the workflow: - src/security/socket-firewall — free tier, no token. Installs Socket Firewall, shims the package manager and runs the dependency install through it. Enabled by default and blocking, since a blocked package is malware, not a finding to triage. Skips with a warning when no lockfile is found in working-dir so monorepos do not go red on a configuration gap. - src/security/socket-scan — paid tier, socketcli. Posts the alert report on the PR and enforces the org policy. Disabled by default and advisory when enabled; skips with a notice when SOCKET_SECURITY_API_KEY is absent, so enabling it early breaks nothing. The socket job sits behind the existing change gate and exposes a stable Socket status check via result-gate, matching Frontend Analysis and Security. All inputs are additive with defaults and the new secret is optional — no caller migration needed.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughThis change adds Socket Firewall, guarded Node setup, Socket App Gate, Socket API reporting, and Socket result reporting. It integrates these actions into frontend analysis and JavaScript PR validation workflows with configurable inputs, result gating, Dependabot grouping, and documentation. ChangesSocket supply-chain pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
lerian-studio
added
size/L
🔍 Lint Analysis
|
🔍 PR Validation Summary✅ PR Mergeable — no blocking failures
|
🛡️ CodeQL Analysis ResultsLanguages analyzed: ✅ No security issues found. 🔍 View full scan logs | 🛡️ Security tab |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/js-pr-validation.yml:
- Around line 280-320: Declare the socket_use_cache boolean input in
.github/workflows/js-pr-validation.yml lines 280-320 with a safe default, pass
it as use-cache in the Socket Firewall configuration at
.github/workflows/js-pr-validation.yml lines 486-496, and document the matching
input-table row in docs/js-pr-validation.md lines 83-92.
In `@docs/js-pr-validation.md`:
- Line 14: Update the Socket supply chain documentation to state that the
pipeline is enabled by default and callers can disable it with run_socket:
false; remove the inaccurate “opt-in” wording.
In `@src/security/socket-firewall/action.yml`:
- Around line 92-99: Update the guarded “Setup Node.js” step to remove the cache
and cache-dependency-path settings, then add a cache-clearing step before the
Socket Firewall package-manager install that clears the selected npm, Yarn, or
pnpm cache based on inputs.package-manager.
- Around line 117-149: In src/security/socket-firewall/action.yml lines 117-149,
remove the unsupported shims input from the SocketDev/action step and prefix
each package-manager command in the install step with sfw while preserving the
existing arguments and exit-code capture. In
src/security/socket-firewall/README.md lines 41-43, update the documentation to
describe explicit sfw invocation rather than package-manager shimming.
In `@src/security/socket-scan/action.yml`:
- Around line 49-51: Update the SARIF output handling in the guard and scan
steps so sarif_file is assigned only after [[ -f "$SARIF_FILE" ]] confirms the
report exists; leave it empty when creation fails or is not requested. Change
the composite action’s sarif-file output mapping to
steps.scan.outputs.sarif_file, and apply the same behavior to the additional
referenced output paths.
- Around line 119-124: Remove the FAIL_ON_FINDINGS-based condition that adds
--disable-blocking to ARGS, so advisory runs preserve Socket CLI API failure
exit codes. Keep --disable-blocking only for the DRY_RUN case if required, and
leave the evaluator’s existing success/failure gate unchanged.
- Around line 117-129: Update the “Run Socket scan” path in the socket-scan
action so the `socketcli` invocation is skipped entirely when `inputs.dry-run`
is true, rather than only appending `--disable-blocking`; gate the step using
the existing `steps.guard` output and the `dry-run` input, and keep the current
dry-run summary output as the only behavior in that mode. Reference the
`socketcli` call and the surrounding shell block that builds `ARGS` so the
normal scan flow remains unchanged when dry-run is false.
In `@src/security/socket-scan/README.md`:
- Around line 58-80: Add a concise rationale section to the README for the
third-party actions used by the Socket Scan workflow: explain that
actions/checkout provides the repository contents and required history for
scanning, and that actions/setup-python supplies the Python runtime needed by
the composite action. Keep the documentation focused on the purpose of each
action.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9644bb57-8b3b-4f69-a2a9-10671cec8952
📒 Files selected for processing (7)
.github/dependabot.yml.github/workflows/js-pr-validation.ymldocs/js-pr-validation.mdsrc/security/socket-firewall/README.mdsrc/security/socket-firewall/action.ymlsrc/security/socket-scan/README.mdsrc/security/socket-scan/action.yml
Address the CodeRabbit review. Two of the findings were real bypasses that made the free tier a no-op reporting success: - The pinned SocketDev/action v1.3.2 exposes no `shims` input — verified against action.yml at ba6de6cc, where the input is absent and dist/main.js contains no shim logic; the README at that SHA documents `sfw npm install`. `shims: 'true'` was therefore silently ignored and bare `npm ci` installed completely uninspected. Every command is now prefixed with `sfw`. - actions/setup-node restored the package-manager cache, and Socket Firewall free can only inspect what crosses the network — per its docs, "if there are no network requests, as is the case when artifacts are cached locally, there is nothing for sfw to block". Dropped `cache:` and purge the cache before the guarded install. Dropping `cache:` also stops this job from writing a post-run cache entry other jobs would restore. Also from the review: - socketcli has no native --dry-run: any invocation authenticates, creates a real scan and comments on the PR. dry-run now skips the CLI entirely. - Stopped passing --disable-blocking in advisory mode. It forces exit 0 over everything, including API failures that surface as exit 3, so advisory mode was indistinguishable from a clean scan. The native exit code now reaches the evaluate step, which was already the only gate. - sarif-file is exported only once the report exists on disk. - Added socket_use_cache so the composite's use-cache is reachable, per the repo guideline that every optional composite feature gets a workflow input. - Documented the rationale for each third-party action in both composite READMEs, and corrected the docs wording that called a default-true run_socket "opt-in".
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/security/socket-firewall/action.yml (1)
105-236: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake
dry-runpreview-only.
dry-run: truestill installs Socket Firewall, clears package-manager caches, and runssfwdependency installation. This can execute dependency lifecycle scripts while the caller expects a preview.
src/security/socket-firewall/action.yml#L105-L236: skip Firewall installation, cache cleanup, guarded installation, and result evaluation wheninputs.dry-run == 'true'. Keep only resolved-configuration notices.src/security/socket-firewall/README.md#L64-L64: state that dry-run does not install tooling, clear caches, or install dependencies.Based on learnings, “when
dry-runistrue, do not install tooling, invoke external services, apply changes, or create pull request side effects.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/security/socket-firewall/action.yml` around lines 105 - 236, Make dry-run preview-only in src/security/socket-firewall/action.yml lines 105-236: guard the Install Socket Firewall, Clear package manager cache, Install dependencies through Socket Firewall, and Evaluate Socket Firewall result steps so they are skipped when inputs.dry-run is true, while retaining resolved-configuration notices. Update src/security/socket-firewall/README.md line 64 to state that dry-run does not install tooling, clear caches, or install dependencies.Source: Learnings
.github/workflows/js-pr-validation.yml (1)
473-501: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftSecurity Misconfiguration (CWE-250)
Exploitability: Moderate
Separate the write-capable Socket scan from the Firewall install.
The
socketjob always grantsissues: writeandpull-requests: write, including whensocket_enable_scanisfalse. Run the Firewall in acontents: readjob. Run the scan in a separate job with only the permissions it requires. Updatesocket-gateto aggregate both results.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/js-pr-validation.yml around lines 473 - 501, Split the current socket job into separate Firewall and scan jobs: keep the Firewall job limited to contents: read, and place the scan in its own job with only the permissions it needs, gated by socket_enable_scan. Update socket-gate to depend on and aggregate both job results, preserving the existing changes and input conditions while ensuring write permissions are not granted to the Firewall path.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/security/socket-scan/action.yml`:
- Around line 79-82: Update the “Install Socket CLI” step in action.yml to
install a specifically reviewed socketsecurity version instead of using the
unconstrained --upgrade option; include package hashes if the workflow’s pip
installation supports hash pinning, while preserving the existing guard
condition and execution flow.
---
Outside diff comments:
In @.github/workflows/js-pr-validation.yml:
- Around line 473-501: Split the current socket job into separate Firewall and
scan jobs: keep the Firewall job limited to contents: read, and place the scan
in its own job with only the permissions it needs, gated by socket_enable_scan.
Update socket-gate to depend on and aggregate both job results, preserving the
existing changes and input conditions while ensuring write permissions are not
granted to the Firewall path.
In `@src/security/socket-firewall/action.yml`:
- Around line 105-236: Make dry-run preview-only in
src/security/socket-firewall/action.yml lines 105-236: guard the Install Socket
Firewall, Clear package manager cache, Install dependencies through Socket
Firewall, and Evaluate Socket Firewall result steps so they are skipped when
inputs.dry-run is true, while retaining resolved-configuration notices. Update
src/security/socket-firewall/README.md line 64 to state that dry-run does not
install tooling, clear caches, or install dependencies.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 060bfe18-c8d5-42f2-ba38-e98731e70e46
📒 Files selected for processing (6)
.github/workflows/js-pr-validation.ymldocs/js-pr-validation.mdsrc/security/socket-firewall/README.mdsrc/security/socket-firewall/action.ymlsrc/security/socket-scan/README.mdsrc/security/socket-scan/action.yml
`pip install socketsecurity --upgrade` resolved whatever PyPI had published most recently and the next step ran it with SOCKET_SECURITY_API_KEY and a GitHub token in its environment — an unreviewed package executing with credentials, which is the exact risk class this composite exists to catch. Pin via a new cli-version input (default 2.5.8, the current release). PyPI forbids re-uploading an existing version, so an exact pin already resolves to a fixed artifact; --require-hashes is not used because it would also demand hashes for every transitive dependency, which is unmaintainable in a shared composite. `cli-version: latest` restores the old behavior and warns. Exposed as socket_cli_version on the umbrella and documented in both places. Bumping is manual, like the action SHAs — Dependabot does not scan src/**.
TEMPORARY, revert before merge. The socket-firewall and socket-scan composites do not exist at the v1 tag yet, so the socket job cannot resolve them and the end-to-end validation on a real caller is impossible without this. src/lint/pinned-actions reports internal refs outside @vN/@develop/@main as a warning, not an error, so the repo's own CI stays green.
Reshape the Socket layer around what the organization already runs. The socket-security GitHub App is installed and posts Socket Security: Project Report and Pull Request Alerts, both observed passing on product-console#682. Remove socket-scan. Running socketcli would re-scan the same dependency graph the App already analyses, post a second competing PR report and consume quota, for no added coverage. Drops the SOCKET_SECURITY_API_KEY secret and its inputs. Add src/setup/setup-node-guarded and use it for all twelve install jobs in frontend-pr-analysis.yml. This closes the real gap: the firewall shim only protects installs in its own job, so guarding one job left eleven running bare npm ci, where a malicious postinstall executes with the runner's tokens. The guarded path drops the package-manager cache by necessity — sfw only inspects what crosses the network — at a measured cost of ~20s per cold install. Add src/security/socket-app-gate. The App analyses but does not enforce: its checks land as success, neutral or skipped, and neither neutral nor skipped blocks a merge. The gate waits for those checks on the PR head SHA and converts them into a verdict we own, with no token and no duplicate scan. inconclusive (neutral/skipped/timeout) blocks by default, because the un-mergeable-PR skip means no diff was analysed at all and must not read as clean; missing (no App installed) only warns, so those repositories stay green on layer 1 alone. Add src/security/socket-reporter, posting one upserted PR comment in the same Stage/Status/Blocking layout as the security scan comment, under its own marker. A separate comment is forced by topology: pr-security-reporter runs inside the security_scan job of pr-security-scan.yml and step outputs do not cross jobs.
lerian-studio
added
size/XL
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/frontend-pr-analysis.yml:
- Around line 249-257: Replace the mutable feat/js-pr-validation-socket
reference in the setup-node-guarded action at
.github/workflows/frontend-pr-analysis.yml ranges 249-257, 288-296, 320-328,
360-368, 558-566, 597-605, 665-673, 705-713, 770-778, 810-818, 861-869, and
983-992 with `@develop` until the composite action is released, then use `@v1` after
publication.
In @.github/workflows/js-pr-validation.yml:
- Around line 315-318: Expose the Socket App polling interval across all
referenced sites: declare socket_app_poll_interval with a default of 15
alongside socket_app_timeout in .github/workflows/js-pr-validation.yml:315-318,
pass it to socket-app-gate as poll-interval-seconds in
.github/workflows/js-pr-validation.yml:530-538, and add the matching documented
input row in docs/js-pr-validation.md:89-94.
In `@docs/frontend-pr-analysis-workflow.md`:
- Around line 143-149: Update the package-manager command table to reflect
guarded installs as the default: prefix the default npm, yarn, and pnpm commands
with sfw, or add distinct guarded and unguarded rows while preserving the
existing unguarded commands for enable_socket_firewall: false.
In `@src/security/socket-app-gate/action.yml`:
- Around line 86-97: The polling loop’s success condition in the Socket App
check flow must not pass when only one check has appeared. Update the condition
around TOTAL and PENDING to require all expected checks, including both Project
Report and Pull Request Alerts, before writing timed_out=false and breaking;
otherwise continue polling until the complete check set is present and no checks
remain pending.
In `@src/security/socket-reporter/action.yml`:
- Around line 114-128: Update the socket-reporter invocation and its blocking
calculation around APP_STATUS so App Gate handling uses the configured
on-inconclusive and on-missing-app policies in addition to app-fail-on-findings.
Propagate the App Gate outcome or both policy inputs into the reporter, ensuring
PR Blocked and gate results remain consistent for all app verdicts.
In `@src/setup/setup-node-guarded/action.yml`:
- Around line 82-85: Update the missing-lockfile branch in the setup action to
set and expose a skipped result, then use that result to bypass cached Node
setup, dependency installation, and final result evaluation. Follow the existing
skip contract in the upstream socket-firewall action, while preserving the
warning and successful early exit.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: eb4799b8-1b20-4bd9-a191-f26d1d71d5a6
📒 Files selected for processing (10)
.github/workflows/frontend-pr-analysis.yml.github/workflows/js-pr-validation.ymldocs/frontend-pr-analysis-workflow.mddocs/js-pr-validation.mdsrc/security/socket-app-gate/README.mdsrc/security/socket-app-gate/action.ymlsrc/security/socket-reporter/README.mdsrc/security/socket-reporter/action.ymlsrc/setup/setup-node-guarded/README.mdsrc/setup/setup-node-guarded/action.yml
Without result-encoding: string, actions/github-script JSON-encodes the
returned string, so the step output arrived as "{\"has_findings\":false}" — a
quoted string — and the jq validation rejected it as missing has_findings.
pr-security-reporter already sets it; the new reporter did not.
Caught by the end-to-end run on product-console#682, where Socket Firewall and
the App gate both passed and only the report step failed.
End-to-end validation on a real callerValidated against LerianStudio/product-console#682, which points its Socket Firewall is genuinely in the path, in every install jobSampled the That is the fix for the gap this PR originally left open: before, only the dedicated job installed under Socket App gateBoth App checks resolved and the gate read them off the PR head SHA:
PR commentPosted under Cost of dropping the package-manager cacheMeasured on that run: One bug the run caught
Still open before merge
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/security/socket-reporter/action.yml (1)
93-96: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPath Traversal (CWE-22): Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal')
Reachability: External · Exploitability: Moderate
Constrain
APP_FINDINGS_FILEto the workspace.
socket-app-gateaccepts a configurablefindings-file, and the pull-request workflow uses the workspace-relative default after checkout.socket-reporterreads that path without canonicalization or symlink validation. Reject absolute paths, paths outside the workspace, and symlinks before reading the file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/security/socket-reporter/action.yml` around lines 93 - 96, Update the findings-file handling around APP_FINDINGS_FILE before fs.readFileSync: resolve the configured path against the workspace, reject absolute paths and any path outside the workspace, and reject symlinks using lstat or equivalent validation before reading. Preserve the existing appEnabled, existence, and JSON parsing flow for validated workspace-local files.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/security/socket-reporter/action.yml`:
- Around line 93-96: Update the findings-file handling around APP_FINDINGS_FILE
before fs.readFileSync: resolve the configured path against the workspace,
reject absolute paths and any path outside the workspace, and reject symlinks
using lstat or equivalent validation before reading. Preserve the existing
appEnabled, existence, and JSON parsing flow for validated workspace-local
files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e60b0eaf-446b-4394-840f-b9198a78b225
📒 Files selected for processing (1)
src/security/socket-reporter/action.yml
Two defects found while auditing what tokens the Socket layer actually needs. The workflow-level permissions block omitted checks: read, which socket-app-gate needs to read the App's check runs. A reusable workflow's permissions are intersected with the caller's, never expanded, so any caller pinning its own permissions block would starve the gate. Added at workflow level and to the docs usage example, which every consuming repo copies verbatim. That failure was also silent: gh errors were folded into an empty result via `|| echo '[]'`, and no checks is classified as `missing`, which only warns. A missing scope therefore looked like "App not installed" and passed. A non-zero gh exit is now a hard error naming the likely cause. The run on product-console#682 passed only because that caller declares no permissions block and inherits the repository default.
No job consumes it: the firewall inspects traffic locally and the App gate reads the GitHub checks API, both token-free. It is declared because a reusable workflow cannot receive a secret it does not declare — not even via `secrets: inherit` — so declaring it now lets an organization secret become usable without cutting a release. Marked in-line and in the docs as intentionally unconsumed, with Socket Firewall enterprise named as the intended consumer, so it is not later removed as dead. Documented the least-privilege scope guidance alongside it.
Splitting the comment into introduced and pre-existing sections wired the remediation list into the introduced one only. On a pull request that introduces nothing — the common case — no fix rendered at all, dropping the most actionable field Socket returns. Extracted into a helper used by both sections. Identical lines are listed once: one advisory reaches several packages, so the raw list repeated the same `npx socket fix --id` several times. The count in the summary reflects all findings with a fix, while the body lists each distinct remedy.
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/security/socket-reporter/action.yml`:
- Line 248: Update the Markdown rendering helpers used by fixBlock and the
existing package and Firewall renderers to apply one shared Markdown encoder to
every report-controlled package, finding type, and fix value, including f.pkg,
f.type, and fixText(f.fix). Ensure the encoder handles backticks, pipes, and
line breaks consistently before values are inserted into PR comments, while
preserving the existing fixBlock behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 567b848a-d9c7-4e94-a3b8-835092f015a5
📒 Files selected for processing (1)
src/security/socket-reporter/action.yml
The design changed repeatedly and the docs did not follow, so they described behaviour that no longer exists. Documentation that lies is worse than documentation that is missing, because the next person trusts it. socket-api-report's README claimed it reads a full scan and reports per package, with an obsolete extracted shape and no mention of diff-scan attribution, include-actions, fail-on-actions or the blame chain. socket-reporter's described the Stage/Status/Blocking layout that was removed, and the old always-post behaviour. The workflow doc listed full-scans:list as the only scope and omitted socket_comment_when, socket_api_include_actions and socket_api_fail_on_actions. Both READMEs and the workflow doc now match the code, and record the measurements the decisions rest on: 4594 of 4636 alerts are ignore, 118 high alerts are still ignored so severity cannot be the filter, the newest target-branch scan shared only 31.8% of the tree, and 1755 of 1897 artifacts carry topLevelAncestors. Two known couplings are written down rather than left as tribal knowledge: the diff scan id comes from the Socket App's own comment, so disabling those comments breaks attribution silently; and gating on pre-existing findings would fail every pull request in a repository carrying debt. Also fixes links to the deleted socket-scan composite, and the intro item that still advertised "the full Socket CLI report".
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/security/socket-api-report/README.md (1)
142-150: 🗄️ Data Integrity & Integration | 🟡 MinorComplete the composite usage examples before release.
The examples reference step outputs that the examples do not create. A caller who copies them can skip the API report or lose pull-request attribution.
src/security/socket-api-report/README.md#L142-L150: Add thediff-scanresolver beforeapi-report, keep the upstream steps failure-tolerant, and show the job withruns-on: blacksmith-4vcpu-ubuntu-2404.src/security/socket-reporter/README.md#L102-L119: Add thesocket-app-gatestep beforeapi-report, pass itsreport-url, keep the upstream steps failure-tolerant, and show the job withruns-on: blacksmith-4vcpu-ubuntu-2404.As per coding guidelines, composite README usage examples must be complete YAML and specify
blacksmith-4vcpu-ubuntu-2404as the runner.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/security/socket-api-report/README.md` around lines 142 - 150, Add the missing diff-scan resolver before api-report in src/security/socket-api-report/README.md lines 142-150, keep upstream steps failure-tolerant, and define the example job with runs-on: blacksmith-4vcpu-ubuntu-2404. In src/security/socket-reporter/README.md lines 102-119, add socket-app-gate before api-report, pass its report-url, keep upstream steps failure-tolerant, and specify the same runner so both examples are complete YAML.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/security/socket-firewall/README.md`:
- Line 10: Update the install-time layer description in the README to state that
it reports blocked packages and findings and enforces the fail-on-block policy,
while clarifying that Socket App verdicts and per-package API findings remain
provided by socket-app-gate and socket-api-report.
---
Duplicate comments:
In `@src/security/socket-api-report/README.md`:
- Around line 142-150: Add the missing diff-scan resolver before api-report in
src/security/socket-api-report/README.md lines 142-150, keep upstream steps
failure-tolerant, and define the example job with runs-on:
blacksmith-4vcpu-ubuntu-2404. In src/security/socket-reporter/README.md lines
102-119, add socket-app-gate before api-report, pass its report-url, keep
upstream steps failure-tolerant, and specify the same runner so both examples
are complete YAML.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7afb1c40-0c2d-4d14-ab9c-0696e37dad22
📒 Files selected for processing (4)
docs/js-pr-validation.mdsrc/security/socket-api-report/README.mdsrc/security/socket-firewall/README.mdsrc/security/socket-reporter/README.md
The blocking test exposed a race, not a bug in the gate. The diff scan id is read from the Socket App's comment, which the App publishes asynchronously, so a job starting before that comment is updated reads the diff for a *previous* push. Observed: abab@2.0.6 was added by the pull request and appears in the scan — 1898 artifacts against 1897, 44 actioned findings against 43 — while the diff it was compared against reported added=0. The package this change introduced was filed as pre-existing debt, and fail-on-actions passed a pull request it was configured to block. Silent wrong attribution is worse than none: the gate reports success either way, so nothing signals that the policy did not apply. The diff scan is now checked against this commit's scan via after_full_scan_id, and a mismatch is reported and discarded rather than used. This makes the failure visible; it does not make attribution reliable. The fix is to stop depending on the App's timing and create the diff scan from ids, using the diff-scans:create scope already granted. The diff scan's top-level keys are logged so that can be built against the real shape.
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/security/socket-api-report/action.yml`:
- Around line 315-317: The credentialed Socket API helper get must reject
redirects rather than allowing urllib.request.urlopen to follow them. Update get
to use a redirect handler that blocks redirects, then handle the resulting
redirect failure at the diff_scan_id call site as an advisory skip while
preserving normal response processing.
- Around line 315-324: Update the diff-scan validation around the `fresh`
assignment to parse `results` as a list of objects and set `fresh` only when at
least one result has an `id` equal to `diff_scan_id`; treat non-object or
non-list responses as empty results, while preserving the existing JSON decode
fallback and subsequent stale-scan handling.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a6f0993c-7204-469e-8088-7db7a1f8b4b0
📒 Files selected for processing (1)
src/security/socket-api-report/action.yml
… App The blocking test never turned the check red, and the reason was not the gate. abab@2.0.6 was added by the pull request and reached the scan — 1898 artifacts against 1897, 44 actioned findings against 43 — but the diff scan read from the Socket App's comment was from a previous push and reported added=0. The package this change introduced was filed as pre-existing debt, and fail-on-actions passed a pull request it was configured to block. That is a race on the normal path, not an edge case: the App publishes its comment asynchronously, so any job that starts first reads a stale diff and attributes nothing, silently. The App's id is now treated as a hint. If Socket already holds a diff keyed on this commit's scan it is used; otherwise one is created from the stale diff's before_full_scan_id through diff-scans/from-ids, with on_duplicate=redirect so reruns stay idempotent. The before side deliberately comes from the diff Socket itself built rather than from the newest scan of the target branch: that one is a guess, and a stale guess inflates the added set, attributing other people's packages to this pull request and blocking it for them. Also stops truncating the introduced set. It is what the pull request answers for, and abab would have been cut anyway — the table held exactly 25 rows while the text claimed 44 findings, so a monitor-level row nobody could see was dropped below the cap. The pre-existing section now states how many packages and findings are not listed instead of "more exist beyond the row limit". Verified against a stub covering the stale path: detects the mismatch, resolves the base, creates the diff, attributes abab as introduced and reports blocking_count=1.
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/security/socket-api-report/action.yml`:
- Around line 305-313: Update the shared HTTP request handling used by post and
get to use a redirect-rejecting urllib opener, preventing credentialed requests
from following redirects. Preserve redirect responses as the existing advisory
skip outcome, and keep the current status/body handling for non-redirect
responses.
- Around line 321-325: Validate decoded JSON and nested result shapes in the
response-handling helpers around the existing JSON parsing at lines 322, 339,
and 362. Normalize only mapping payloads, ensure results are lists of mappings,
and treat invalid payloads or entries such as null as advisory skips returning
the existing empty/None outcome, while preserving skipped=true and
classified-count emission.
- Around line 335-344: Update the known diff-scan lookup in base_scan_from to
fetch the requested scan by ID or paginate through all diff-scan result pages
until known_id is found, preserving the existing before_full_scan_id return
behavior. Ensure missing or invalid responses still return None, and add a
regression case covering an ID beyond the first 30 results.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 43f41fae-1bee-4413-badc-f1e241ac9cfa
📒 Files selected for processing (2)
src/security/socket-api-report/action.ymlsrc/security/socket-reporter/action.yml
The created diff scan persists and is found on rerun, and still reports added=0 while the head scan grew by exactly the package the pull request added (1897 to 1898 artifacts, 43 to 44 actioned findings). It is not a timing problem: the same result comes back fifteen minutes later. Two explanations remain and the logs cannot currently separate them. The before side is inherited from the App's own diff and has never been confirmed to be the target branch rather than an earlier head of this same pull request, which would make a near-empty diff correct. Or the semantics of the added bucket differ from the assumption. Logs both sides of the created diff with their branch and commit, which the create response carries, and adds a probe that reports whether a named package is present in the head scan at all. One run separates the two. TEMPORARY, remove once resolved.
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/js-pr-validation.yml (1)
644-661: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSkip Socket failure enforcement in dry-run mode.
When
inputs.dry_runistrue, the report step is skipped, but this gate still exits with status 1 forFIREWALL_OUTCOME,APP_GATE_OUTCOME, orAPI_BLOCKING.Pass
DRY_RUNinto this step. In dry-run mode, emit a::notice::with the computed result and exit 0. Preserve the current failure behavior when dry-run is false.As per coding guidelines, dry-run mode must use notice annotations and preview behavior instead of normal enforcement.
Proposed fix
env: + DRY_RUN: ${{ inputs.dry_run }} FIREWALL_OUTCOME: ${{ steps.firewall.outcome }} ... if [ -n "$FAILED" ]; then + if [ "$DRY_RUN" = "true" ]; then + echo "::notice::Dry run: Socket supply chain gate would fail:$FAILED" + exit 0 + fi echo "::error::Socket supply chain gate failed:$FAILED. See the Socket comment on the pull request." exit 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/js-pr-validation.yml around lines 644 - 661, Update the “Gate - Fail on Socket findings” step to pass inputs.dry_run as DRY_RUN. When DRY_RUN is true, emit a ::notice:: containing the computed FAILED result and exit successfully without enforcing failures; preserve the existing error annotation and nonzero exit behavior for normal runs.Source: Coding guidelines
♻️ Duplicate comments (4)
.github/workflows/js-pr-validation.yml (4)
555-555: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSecurity Misconfiguration (CWE-829): Inclusion of Functionality from Untrusted Control Sphere
Reachability: Internal
Replace the temporary feature-branch action references before merge.
These steps resolve composite actions from
@feat/js-pr-validation-socket. The workflow can fail when that branch is deleted. The branch reference also selects code that receivesSOCKET_SECURITY_API_KEYorMANAGE_TOKEN.Replace every feature-branch reference with the repository-approved promoted reference before merge.
Based on learnings, feature-branch composite references are temporary for E2E validation and must be replaced before merge.
#!/bin/bash set -euo pipefail if rg -n 'feat/js-pr-validation-socket' .github/workflows/js-pr-validation.yml; then echo "Temporary feature-branch reference remains." exit 1 fiAlso applies to: 570-570, 610-610, 629-629
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/js-pr-validation.yml at line 555, Replace every `@feat/js-pr-validation-socket` reference in the workflow’s composite action steps with the repository-approved promoted reference, including the occurrences near the affected security and validation steps. Ensure no temporary feature-branch references remain in `.github/workflows/js-pr-validation.yml`.Source: Learnings
529-580: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftRun the optional
socket-scanaction or remove the paid-scan contract.The
socketjob invokessocket-firewallandsocket-app-gate, but it does not invokesocket-scan. The workflow therefore does not run the paid scan described by the PR objective.Add the optional scan step and include its advisory result in reporting and gating. Otherwise remove the paid-scan input and documentation.
The PR objective states that
socket-scanis an optional paid scan.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/js-pr-validation.yml around lines 529 - 580, Add the optional socket-scan action to the socket job alongside socket-firewall and socket-app-gate, wiring its inputs and advisory result into the existing reporting and gating behavior. Preserve the PR objective that socket-scan is an optional paid scan; alternatively remove the corresponding paid-scan input and documentation if the scan is intentionally unsupported.
595-598: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftBind the diff-scan ID to the current pull request and head SHA.
gh api --paginate --jq "$JQ"applies the filter per response page. The laterhead -1can select an older page's Socket comment. Even on one page, the last historical bot comment is not proof that it belongs to the current App run.If the App posts no new comment, a stale
diff-scanID reachessocket-api-report. Aggregate all pages before selecting the comment, then validate the scan metadata against the repository, pull request number, and head SHA. Output an empty ID when validation fails.#!/bin/bash set -euo pipefail sed -n '586,604p' .github/workflows/js-pr-validation.yml rg -n -C 6 'diff_scan_id|after_full_scan_id|head_sha|pull_request|repository' \ src/security/socket-api-report/action.yml🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/js-pr-validation.yml around lines 595 - 598, Update the comment-fetching logic around JQ, BODY, and ID to aggregate all paginated Socket Security comments before selecting the latest candidate, then validate its diff-scan metadata against the current repository, pull request number, and head SHA. Only pass the scan ID to socket-api-report when all metadata matches the current App run; otherwise set ID to an empty value. Reuse the existing workflow context and report-input symbols rather than trusting the first matching historical comment.
280-366: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winForward
socket_app_poll_intervalto Socket App Gate.The App Gate step passes
timeout-seconds, but it does not passpoll-interval-seconds. Ifsocket_app_poll_intervalis declared, callers cannot change the polling interval.Pass the input to the composite. Otherwise remove the unused workflow input and documentation.
Based on the Socket App Gate contract in this stack,
poll-interval-secondsis a configurable composite input.#!/bin/bash set -euo pipefail rg -n -A8 -B3 'poll-interval-seconds' src/security/socket-app-gate/action.yml rg -n -C3 'socket_app_poll_interval|poll-interval-seconds' \ .github/workflows/js-pr-validation.yml docs/js-pr-validation.mdAlso applies to: 576-579
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/js-pr-validation.yml around lines 280 - 366, Update the Socket App Gate invocation to pass the declared socket_app_poll_interval input as poll-interval-seconds alongside timeout-seconds, preserving the composite action’s configurable polling behavior. If no invocation can use this input, remove socket_app_poll_interval and its related documentation instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/js-pr-validation.yml:
- Around line 623-625: Remove the temporary probe-package: 'abab' diagnostic
input from the workflow configuration, ensuring the normal API report no longer
performs unrelated probe or quota work. Do not retain it in the production
pull-request path; only preserve it if gated behind a development-only option
defaulting to false.
In `@src/security/socket-api-report/action.yml`:
- Around line 42-45: Remove the temporary probe-package interface end to end:
delete its input declaration, caller usage, environment mapping, and diagnostic
logging, including the related logic near the workflow invocation and report
handling. Keep the action’s supported inputs and normal scan behavior unchanged;
do not document this temporary interface.
---
Outside diff comments:
In @.github/workflows/js-pr-validation.yml:
- Around line 644-661: Update the “Gate - Fail on Socket findings” step to pass
inputs.dry_run as DRY_RUN. When DRY_RUN is true, emit a ::notice:: containing
the computed FAILED result and exit successfully without enforcing failures;
preserve the existing error annotation and nonzero exit behavior for normal
runs.
---
Duplicate comments:
In @.github/workflows/js-pr-validation.yml:
- Line 555: Replace every `@feat/js-pr-validation-socket` reference in the
workflow’s composite action steps with the repository-approved promoted
reference, including the occurrences near the affected security and validation
steps. Ensure no temporary feature-branch references remain in
`.github/workflows/js-pr-validation.yml`.
- Around line 529-580: Add the optional socket-scan action to the socket job
alongside socket-firewall and socket-app-gate, wiring its inputs and advisory
result into the existing reporting and gating behavior. Preserve the PR
objective that socket-scan is an optional paid scan; alternatively remove the
corresponding paid-scan input and documentation if the scan is intentionally
unsupported.
- Around line 595-598: Update the comment-fetching logic around JQ, BODY, and ID
to aggregate all paginated Socket Security comments before selecting the latest
candidate, then validate its diff-scan metadata against the current repository,
pull request number, and head SHA. Only pass the scan ID to socket-api-report
when all metadata matches the current App run; otherwise set ID to an empty
value. Reuse the existing workflow context and report-input symbols rather than
trusting the first matching historical comment.
- Around line 280-366: Update the Socket App Gate invocation to pass the
declared socket_app_poll_interval input as poll-interval-seconds alongside
timeout-seconds, preserving the composite action’s configurable polling
behavior. If no invocation can use this input, remove socket_app_poll_interval
and its related documentation instead.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d12b950f-3f58-4c57-84a0-576cf41a666f
📒 Files selected for processing (2)
.github/workflows/js-pr-validation.ymlsrc/security/socket-api-report/action.yml
The dashboard renders this exact diff populated while the API returned added, updated and replaced all empty. omit_unchanged=true was the only parameter that could shape the payload, and a smaller payload is worth nothing if it arrives empty. The diagnostic now logs the shape of each top-level value rather than just the key names — the previous one confirmed the keys exist and said nothing about whether they are empty arrays or whether the content lives somewhere else in the response.
Root cause of a diff that attributed nothing while the dashboard rendered the same diff correctly: the buckets are nested under diff_scan.artifacts, and the code read them from the top level. Every bucket came back None, so every finding was filed as pre-existing and fail-on-actions could never fire. omit_unchanged=true actively hid this. It returned a flattened response whose added/updated/replaced keys were present and empty, which reads as "nothing changed" rather than "you are looking in the wrong place" — and sent the investigation after the before side of the diff and after asynchronous computation instead. Reads diff_scan.artifacts, falling back to artifacts and then to the top level, so a shape change degrades to a different layout rather than to silence. All three are covered by a shape test.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/security/socket-api-report/action.yml (1)
417-445: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winTreat an invalid diff payload as skipped.
If JSON parsing fails, this code sets
diff = {}and then setshas_baseline = True. It classifies every finding as pre-existing and emitsblocking_count=0.If the response is a JSON array, or
diff_scanorartifactshas the wrong type,diff.items()orbuckets.get()raises before the action emits classification outputs.Validate the decoded object, nested bucket object, and bucket artifact lists. If validation fails, call
bail(..., "warning")instead of publishing a non-blocking classification.Proposed fix
try: diff = json.loads(bd.decode("utf-8", "replace")) except json.JSONDecodeError: - diff = {} + bail("Could not parse the Socket diff scan response.", "warning") + if not isinstance(diff, dict): + bail("Socket diff scan response has an invalid shape.", "warning") - buckets = ((diff.get("diff_scan") or {}).get("artifacts") - or diff.get("artifacts") or diff) + nested = diff.get("diff_scan") + nested_buckets = nested.get("artifacts") if isinstance(nested, dict) else None + buckets = nested_buckets if isinstance(nested_buckets, dict) else ( + diff.get("artifacts") if isinstance(diff.get("artifacts"), dict) else diff + ) counts = {} for bucket in ("added", "updated", "replaced"): arts = buckets.get(bucket) or [] + if not isinstance(arts, list) or any(not isinstance(art, dict) for art in arts): + bail(f"Socket diff scan bucket '{bucket}' has an invalid shape.", "warning")Based on PR objectives, API failures are advisory and must not become successful non-blocking classifications.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/security/socket-api-report/action.yml` around lines 417 - 445, Validate the diff payload before classification: require the decoded value to be a dictionary, ensure diff_scan and artifacts are dictionaries when present, and require the added, updated, and replaced buckets to be lists. In the parsing and validation flow around the shape diagnostic and buckets assignment, call bail with the invalid-payload message and "warning" instead of setting an empty diff or publishing classification outputs; preserve valid nested and fallback artifact bucket handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/security/socket-api-report/action.yml`:
- Around line 417-445: Validate the diff payload before classification: require
the decoded value to be a dictionary, ensure diff_scan and artifacts are
dictionaries when present, and require the added, updated, and replaced buckets
to be lists. In the parsing and validation flow around the shape diagnostic and
buckets assignment, call bail with the invalid-payload message and "warning"
instead of setting an empty diff or publishing classification outputs; preserve
valid nested and fallback artifact bucket handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 53072396-1f62-4b06-84e2-984720923796
📒 Files selected for processing (1)
src/security/socket-api-report/action.yml
The job log named scans and diffs by uuid alone, so reading it meant hand-assembling a dashboard path to see what was described. The Actions log viewer linkifies URLs, so printing them in full makes every scan, diff and probe line clickable — including the created diff's before and after sides, which now show branch and commit alongside their link. This is the same information the pull request comment already links; the gap was that the log, which is where a failure is diagnosed, did not.
The head-side scan_state probe existed to test whether pending/precrawl/resolve mean incomplete. It does not — the head scan reports pending while carrying complete data — and that question is settled, so the call goes: it cost a quota unit on every run, roughly a fifth of the per-run budget, for an answer already recorded in the README. probe-package stays on the composite as a diagnostic input alongside debug-sample, but the hardcoded 'abab' wiring is removed from the workflow. head-sha is no longer used for state logging, only to label the scan link, and its description now says so.
Sixteen references pointed at this feature branch so the new composites would resolve during end-to-end validation: four in js-pr-validation and twelve in frontend-pr-analysis, one per install job. Leaving any behind would pin every consuming repository to a feature branch. They resolve only once the release moves the v1 tag, which is the normal order for this repository: merge, then release. Until then the branch cannot be exercised on a caller, so validation had to finish first — it has.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/frontend-pr-analysis.yml (1)
983-992: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSecurity Misconfiguration (CWE-829): Inclusion of Functionality from Untrusted Control Sphere
Reachability: External · Exploitability: Moderate
Protect Docker image builds with Socket Firewall
docker/build-push-actionbuilds the caller-selected Dockerfile beforesetup-node-guardedruns. If the Dockerfile installs JavaScript dependencies, those downloads and lifecycle scripts run outside the guarded install path.Move the firewall setup before the Docker build, or add an equivalent protected Docker build path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/frontend-pr-analysis.yml around lines 983 - 992, Update the Docker smoke-test workflow so Socket Firewall protection is established before docker/build-push-action executes, rather than only in the later “Setup Node.js and install dependencies” step. Ensure the caller-selected Dockerfile’s dependency downloads and lifecycle scripts run through an equivalent guarded path, while preserving the existing guard, firewall-version, and fail-on-block inputs..github/workflows/js-pr-validation.yml (1)
591-594: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winFlatten paginated comment results before selecting the latest Socket comment.
gh api --paginateemits each page separately unless--slurpis set. The current filter returns one body per page, sohead -1can select an older diff scan ID. This stale ID is passed todiff-scan-idand can break attribution.Proposed pagination fix
- JQ='[.[] | select(.user.login == "socket-security[bot]") | .body] | last // ""' + JQ='[.[].[] | select(.user.login == "socket-security[bot]") | .body] | last // ""' - BODY=$(gh api "repos/$REPO/issues/$PR/comments" --paginate --jq "$JQ") + BODY=$(gh api "repos/$REPO/issues/$PR/comments" --paginate --slurp --jq "$JQ")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/js-pr-validation.yml around lines 591 - 594, Update the BODY query in the pull-request comment retrieval flow to use gh api pagination with slurping, so all comment pages are combined before the JQ filter selects the latest socket-security[bot] comment. Preserve the existing diff-scan ID extraction from BODY.Source: MCP tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/frontend-pr-analysis.yml:
- Around line 983-992: Update the Docker smoke-test workflow so Socket Firewall
protection is established before docker/build-push-action executes, rather than
only in the later “Setup Node.js and install dependencies” step. Ensure the
caller-selected Dockerfile’s dependency downloads and lifecycle scripts run
through an equivalent guarded path, while preserving the existing guard,
firewall-version, and fail-on-block inputs.
In @.github/workflows/js-pr-validation.yml:
- Around line 591-594: Update the BODY query in the pull-request comment
retrieval flow to use gh api pagination with slurping, so all comment pages are
combined before the JQ filter selects the latest socket-security[bot] comment.
Preserve the existing diff-scan ID extraction from BODY.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5ee68f31-f804-4ab3-911b-fb1a16e04213
📒 Files selected for processing (2)
.github/workflows/frontend-pr-analysis.yml.github/workflows/js-pr-validation.yml
Both sides added to the same two places in docs/js-pr-validation.md, so the resolution keeps both rather than picking one: develop's Breaking Change Guard takes position 2 in the pipeline list and Socket supply chain moves to 6, and the branch-protection section requires Frontend Analysis, Security and Socket while keeping develop's note that breaking-change enforcement lives inside Blocking Checks and adds no branch-protection check of its own.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
.github/workflows/js-pr-validation.yml (5)
548-548: 🩺 Stability & Availability | 🟠 MajorReplace temporary feature-branch action references before merge.
The PR objective states that temporary
feat/js-pr-validation-socketreferences remain. Replace every such reference with the published@v1composite reference before merge. A deleted feature branch will make the reusable workflow fail to resolve its actions.Based on learnings, feature-branch references are temporary validation refs and must be replaced before merge.
Also applies to: 563-563, 580-580, 590-590
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/js-pr-validation.yml at line 548, Replace every temporary feat/js-pr-validation-socket action reference in the workflow steps around Checkout code with the published `@v1` composite action reference, including all additionally flagged occurrences. Ensure no feature-branch references remain so reusable workflow action resolution uses the published version.Source: Learnings
612-629: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winForward
SOCKET_SECURITY_API_KEYto the API report action.The workflow declares and inherits the secret, but the Socket API report invocation does not consume it. The composite skips when
socket-api-keyis empty. Therefore, callers that provide the API key still receive no API findings. Passsecrets.SOCKET_SECURITY_API_KEYassocket-api-key. Updatedocs/js-pr-validation.mdLine 134 to describe the actual use.The PR objective states that the key reaches the reusable workflow but is currently consumed by nothing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/js-pr-validation.yml around lines 612 - 629, Update the Socket API Report step to pass secrets.SOCKET_SECURITY_API_KEY through its socket-api-key input so the action runs when callers provide the key. Also update the corresponding documentation entry in js-pr-validation.md to describe that the key is consumed by the API report action.
315-318: 📐 Maintainability & Code Quality | 🟠 MajorExpose the Socket App polling interval.
socket-app-gateexposespoll-interval-seconds, but this reusable workflow does not expose a matching caller input. Addsocket_app_poll_intervalwith default15, pass it to the App Gate, and document it indocs/js-pr-validation.mdLines 92-94.As per coding guidelines, every optional composite feature must have a matching reusable-workflow input.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/js-pr-validation.yml around lines 315 - 318, Add the optional reusable-workflow input socket_app_poll_interval with a default of 15, pass it through to the socket-app-gate action’s poll-interval-seconds setting, and document the new caller input in docs/js-pr-validation.md near the existing Socket App configuration.Source: Coding guidelines
645-667: 🎯 Functional Correctness | 🟠 MajorDo not fail the Socket gate during
dry_run.When
inputs.dry_runistrue, guard the final failure exit for deferred Firewall, App Gate, and API blocking results. Emit a::notice::with the computed result instead. Preserve the existing failure behavior whendry_runisfalse.As per coding guidelines, dry-run mode must use notice output and must not execute the normal enforcement path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/js-pr-validation.yml around lines 645 - 667, Update the “Gate - Fail on Socket findings” step to read the workflow’s dry_run input and, when it is true, emit the computed FAILED result with ::notice:: and skip the enforcement exit. Preserve the existing error output and exit 1 behavior for non-dry-run executions, covering Firewall, App Gate, and API blocking findings.Source: Coding guidelines
10-19: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRemove the
has_*output from this umbrella workflow.
js-pr-validation.ymlis a terminal PR-validation workflow, not a downstream orchestration workflow. Removehas_breaking_changesfromworkflow_call.outputsand fromdocs/js-pr-validation.mdLines 106-114, or verify a caller requires it and document the exception.Based on learnings, PR-validation umbrella workflows should omit
has_*outputs and reserve them for downstream orchestration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/js-pr-validation.yml around lines 10 - 19, Remove the has_breaking_changes output from the workflow_call outputs in js-pr-validation.yml and remove its corresponding documentation entry in docs/js-pr-validation.md. Leave breaking_change_approved and breaking_change_result unchanged unless a verified caller requires the removed output.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/js-pr-validation.yml:
- Line 548: Replace every temporary feat/js-pr-validation-socket action
reference in the workflow steps around Checkout code with the published `@v1`
composite action reference, including all additionally flagged occurrences.
Ensure no feature-branch references remain so reusable workflow action
resolution uses the published version.
- Around line 612-629: Update the Socket API Report step to pass
secrets.SOCKET_SECURITY_API_KEY through its socket-api-key input so the action
runs when callers provide the key. Also update the corresponding documentation
entry in js-pr-validation.md to describe that the key is consumed by the API
report action.
- Around line 315-318: Add the optional reusable-workflow input
socket_app_poll_interval with a default of 15, pass it through to the
socket-app-gate action’s poll-interval-seconds setting, and document the new
caller input in docs/js-pr-validation.md near the existing Socket App
configuration.
- Around line 645-667: Update the “Gate - Fail on Socket findings” step to read
the workflow’s dry_run input and, when it is true, emit the computed FAILED
result with ::notice:: and skip the enforcement exit. Preserve the existing
error output and exit 1 behavior for non-dry-run executions, covering Firewall,
App Gate, and API blocking findings.
- Around line 10-19: Remove the has_breaking_changes output from the
workflow_call outputs in js-pr-validation.yml and remove its corresponding
documentation entry in docs/js-pr-validation.md. Leave breaking_change_approved
and breaking_change_result unchanged unless a verified caller requires the
removed output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6dac4eaa-fcb4-4cbc-a8f4-4aedb74458e6
📒 Files selected for processing (2)
.github/workflows/js-pr-validation.ymldocs/js-pr-validation.md
…s review Credential exposure through redirect (CWE-319/918/200). urllib re-sends the Authorization header on redirect, so a redirect off api.socket.dev would hand the Socket token to another origin, or to plain HTTP. Both get and post now use an opener that refuses any redirect that is not same-host HTTPS. Blanket blocking was not an option: on_duplicate=redirect answers a duplicate diff-scan creation with a 302 back to the existing resource, which is a redirect the design depends on. Markdown and URL injection (CWE-74). Package names, versions, alert types and severities flow from Socket findings — metadata published by the dependency being reported on — straight into comment tables and link URLs. A pipe breaks the table it sits in and a bracket can restructure a link. Registry naming rules make it unlikely, not impossible, and a security report is the wrong place to rely on someone else's validation. Table cells are escaped and URL components encoded. dry-run was preview-only in name. The composite printed "runs but never fails the step" and then installed tooling, fetched packages and exited 1 on a block, and the umbrella gate enforced during dry_run too. Every side-effecting step is now skipped, and the gate prints the verdict it would have applied. job-summary: none silently blinded block detection. The pinned SocketDev action exports SFW_JSON_REPORT_PATH only when job-summary != none, so with none the report is absent, blocked reads false, and a real block is reported as an ordinary install failure. none is coerced to errors. The diff-scan lookup read only the first 30 results of an org-wide, recency-ordered list. A busy organization pushes this pull request's diff off that page, which returned None, classified every finding as pre-existing and zeroed the blocking count. Now paginated with a bounded page count and a warning when exhausted. Also: JSON payloads are normalised before .get, so a valid array or null response is an advisory skip instead of an AttributeError that kills the step before it emits its outputs; max-rows tolerates a non-numeric value; and the tautological GUARDED branches copied from setup-node-guarded are gone from socket-firewall, where the install is always guarded.
CodeRabbit review — analysis and disposition57 unanswered inline comments accumulated across ~40 commits and many review rounds. 31 land on current HEAD; 26 target code that has since been rewritten. Applied in
|
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.github/workflows/js-pr-validation.yml (1)
572-585: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not invoke Socket services during dry run.
Dry run still polls the Socket App, reads PR comments, and runs
socket-api-report. The API report can callPOST /diff-scans/from-ids, so dry run can create a Socket diff scan and consume API quota.Add
!inputs.dry_runto these three step conditions. Keep Lines 671-681 as verbose preview output only.Based on learnings, dry-run is preview-only and must not invoke external services or create side effects.
Also applies to: 592-610, 612-628
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/js-pr-validation.yml around lines 572 - 585, Update the conditions for the Socket App Gate step and the related Socket steps around the symbols at lines 592-610 and 612-628 to require !inputs.dry_run in addition to their existing predicates. Ensure dry runs skip all Socket polling, comment-reading, and socket-api-report activity, while preserving the verbose preview-only output at lines 671-681.Source: Learnings
src/security/socket-api-report/action.yml (1)
197-202: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winEnforce a total HTTP deadline.
Line 197 sets an idle socket timeout. It does not bound
r.read()when the server sends data before each idle timeout. A slow response can hold the job until the workflow timeout.Apply a monotonic deadline around the complete request and publish
skipped=truewhen it expires. Verify with a response that sends one byte before each idle timeout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/security/socket-api-report/action.yml` around lines 197 - 202, Update the request logic surrounding opener.open and r.read to enforce a monotonic total deadline rather than relying only on the 180-second idle timeout. Track the deadline across connection and response reads, abort when it expires, and return the established response shape with skipped=true; preserve HTTPError handling and existing response data for requests completed before the deadline.src/security/socket-firewall/action.yml (1)
92-106: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSkip toolchain setup in dry-run mode.
Set up pnpmandSetup Node.jsstill run wheninputs.dry-runistrue. These actions can download and install tooling. This contradicts Line 120, which states that nothing is installed or fetched.Add
&& inputs.dry-run != 'true'to both step conditions.Based on learnings: “when
dry-runistrue, do not install tooling, invoke external services, apply changes, or create pull request side effects.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/security/socket-firewall/action.yml` around lines 92 - 106, Add the dry-run guard to the `if` conditions for both `Set up pnpm` and `Setup Node.js`, requiring `inputs.dry-run != 'true'` alongside the existing conditions. Ensure neither toolchain setup action runs in dry-run mode while preserving their current behavior otherwise.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/security/socket-api-report/action.yml`:
- Around line 416-418: Validate obj in the diff-scan extraction flow before
reading its fields, and require both the selected diff_scan and its artifacts
value to be mappings before reading buckets or setting has_baseline. Treat
invalid or non-object payloads as unattributed or skipped, preserving blocking
classification rather than accepting fallback {} as a valid baseline; apply the
same mapping guard around obj in the shown extraction logic.
In `@src/security/socket-reporter/action.yml`:
- Around line 116-120: Update the cell formatter in the action’s
report-rendering logic to escape Markdown link and HTML delimiters by handling
[, ], <, and > in addition to the existing pipe, backslash, backtick, and
newline escaping. Ensure all report-controlled values, including f.fix rendered
at the remediation output, pass through this updated cell function.
---
Outside diff comments:
In @.github/workflows/js-pr-validation.yml:
- Around line 572-585: Update the conditions for the Socket App Gate step and
the related Socket steps around the symbols at lines 592-610 and 612-628 to
require !inputs.dry_run in addition to their existing predicates. Ensure dry
runs skip all Socket polling, comment-reading, and socket-api-report activity,
while preserving the verbose preview-only output at lines 671-681.
In `@src/security/socket-api-report/action.yml`:
- Around line 197-202: Update the request logic surrounding opener.open and
r.read to enforce a monotonic total deadline rather than relying only on the
180-second idle timeout. Track the deadline across connection and response
reads, abort when it expires, and return the established response shape with
skipped=true; preserve HTTPError handling and existing response data for
requests completed before the deadline.
In `@src/security/socket-firewall/action.yml`:
- Around line 92-106: Add the dry-run guard to the `if` conditions for both `Set
up pnpm` and `Setup Node.js`, requiring `inputs.dry-run != 'true'` alongside the
existing conditions. Ensure neither toolchain setup action runs in dry-run mode
while preserving their current behavior otherwise.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a3fd79a2-cfe6-4bd9-8600-cd7eb9ef4831
📒 Files selected for processing (4)
.github/workflows/js-pr-validation.ymlsrc/security/socket-api-report/action.ymlsrc/security/socket-firewall/action.ymlsrc/security/socket-reporter/action.yml
typos reads `anc` as a misspelling of `and` and exits 2, which failed the Spelling Check and produced the nine warnings alongside it — one cause, reported twice. Renamed to `ancestors` in the reporter and `with_ancestors` in the API report. Both read better than the abbreviation did, so nothing is lost to appeasing the linter. Verified with typos v1.44.0, the same version CI pins, over the same 15 changed files: exit 0.
Every one of these fails the same way: an error or unreadable state produces a
verdict indistinguishable from clean, so the gate passes a pull request it was
configured to block.
A malformed diff payload decoded to {} and fell through the bucket fallbacks to
the whole object, yielding empty buckets and has_baseline=true — nothing
attributed, blocking count zero, gate green. A payload now only counts as
readable when it carries an actual added/updated/replaced list, and an unreadable
one is reported and attributes nothing.
The App gate finished on the first snapshot where everything it could see was
complete. The App creates its checks independently, so Project Report can finish
before Pull Request Alerts exists and the gate would pass having never seen the
adverse one. The set must now be complete and unchanged across two consecutive
polls, which avoids hardcoding check names.
setup-node-guarded documented "skip with a warning" for a missing lockfile but
only stopped guarding: the cached setup still received a cache-dependency-path
pointing at a file that does not exist, and npm ci needs a lockfile anyway, so the
action failed instead of skipping. A skipped output now bypasses setup, install
and evaluation.
cell() escaped pipes, backticks and newlines but not brackets or angle brackets.
`fix` is free text from Socket, so "[approved fix](https://attacker.example)"
rendered as a link in the remediation list, and raw HTML renders in a comment.
cbaa29e ran `git add -A && rm -f typos`, so the staging happened before the removal and an 8 MB executable landed in the tree. The Spelling Check then read its own binary as text, which is where the "Pn should be On" warnings came from — the failure was self-inflicted, not a real typo. Added to .gitignore so fetching it for local verification cannot repeat this.
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/security/socket-api-report/action.yml`:
- Around line 483-503: Harden buckets_of and the attribution loop to validate
diff_scan is a mapping, each selected added/updated/replaced/removed bucket is a
list, and every artifact entry is a mapping before accessing fields or setting
has_baseline. Treat any invalid structure as no attribution, using the existing
warning path, while preserving skipped and classified count emission instead of
allowing exceptions to terminate the process.
In `@src/security/socket-app-gate/action.yml`:
- Around line 104-114: Update the polling loop around the stable-set branch to
validate DEADLINE immediately after reading each snapshot and before evaluating
TOTAL, PENDING, or SIG stability. Ensure expiration exits with the existing
timeout behavior, preventing confirmation sleeps or continuously changing
completed snapshots from bypassing timeout-seconds.
In `@src/setup/setup-node-guarded/action.yml`:
- Around line 43-45: Update the Outputs table in the setup-node-guarded README
to document the public skipped output from the action metadata, stating that it
is true when the guarded path finds no lockfile and false for the unguarded
path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3393c473-2e01-4bcc-88db-2fa52a49a4c1
📒 Files selected for processing (5)
src/security/socket-api-report/action.ymlsrc/security/socket-app-gate/action.ymlsrc/security/socket-reporter/action.ymlsrc/setup/setup-node-guarded/action.ymltypos
| def buckets_of(d): | ||
| for cand in ((d.get("diff_scan") or {}).get("artifacts"), | ||
| d.get("artifacts"), d): | ||
| if isinstance(cand, dict) and any( | ||
| isinstance(cand.get(b), list) | ||
| for b in ("added", "updated", "replaced", "removed")): | ||
| return cand | ||
| return None | ||
|
|
||
| buckets = buckets_of(diff) if isinstance(diff, dict) else None | ||
| if buckets is None: | ||
| print("::warning::The diff scan payload carried no readable " | ||
| "added/updated/replaced buckets; nothing is attributed to " | ||
| "this pull request.") | ||
| diff_scan_id = None | ||
| counts = {} | ||
| for bucket in ("added", "updated", "replaced") if buckets else (): | ||
| arts = buckets.get(bucket) or [] | ||
| counts[bucket] = len(arts) | ||
| for art in arts: | ||
| introduced_pkgs.add(f"{pkg_name(art)}@{art.get('version')}") |
There was a problem hiding this comment.
Validate every selected diff bucket before attribution.
buckets_of() accepts a payload when only one bucket is a list. It then calls .get() on an unvalidated diff_scan value and assumes every selected bucket contains mapping entries. A 200 payload with diff_scan: "invalid" fails at Line 484. A payload with added: [] and updated: "invalid" fails at Line 503.
The Python process exits before it emits skipped and classified counts. The final workflow defaults a missing API blocking count to zero, so this failure can look clean.
Validate nested diff_scan, every present bucket, and every artifact entry before setting has_baseline. Route invalid structures through the existing no-attribution warning path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/security/socket-api-report/action.yml` around lines 483 - 503, Harden
buckets_of and the attribution loop to validate diff_scan is a mapping, each
selected added/updated/replaced/removed bucket is a list, and every artifact
entry is a mapping before accessing fields or setting has_baseline. Treat any
invalid structure as no attribution, using the existing warning path, while
preserving skipped and classified count emission instead of allowing exceptions
to terminate the process.
| if [ "$TOTAL" -gt 0 ] && [ "$PENDING" -eq 0 ]; then | ||
| if [ "$SIG" = "${PREV_SIG:-}" ]; then | ||
| echo "timed_out=false" >> "$GITHUB_OUTPUT" | ||
| break | ||
| fi | ||
| echo "All $TOTAL check(s) complete; confirming the set is stable before deciding." | ||
| PREV_SIG="$SIG" | ||
| sleep "$INTERVAL" | ||
| continue | ||
| fi | ||
| PREV_SIG="" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Enforce the polling deadline before the stable-set branch.
Every completed snapshot enters this branch and bypasses the deadline check at Line 115. The action can pass after timeout-seconds when the confirmation sleep crosses the deadline. It can also loop until the workflow timeout when the App continuously adds completed checks and changes SIG each poll.
Check DEADLINE immediately after reading the snapshot and before the stability logic.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/security/socket-app-gate/action.yml` around lines 104 - 114, Update the
polling loop around the stable-set branch to validate DEADLINE immediately after
reading each snapshot and before evaluating TOTAL, PENDING, or SIG stability.
Ensure expiration exits with the existing timeout behavior, preventing
confirmation sleeps or continuously changing completed snapshots from bypassing
timeout-seconds.
| skipped: | ||
| description: 'true when no lockfile was found in working-dir, so nothing was installed or inspected' | ||
| value: ${{ steps.resolve.outputs.skipped }} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the new skipped output.
The changed action metadata adds skipped, but src/setup/setup-node-guarded/README.md does not list it in the Outputs table. Document that it is true for the guarded missing-lockfile path and false for the unguarded path.
Based on src/setup/setup-node-guarded/README.md, the documented output contract omits this new public output.
Suggested README update
## Outputs
+| `skipped` | `true` when guarded mode finds no lockfile and skips setup and installation; otherwise `false` |🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/setup/setup-node-guarded/action.yml` around lines 43 - 45, Update the
Outputs table in the setup-node-guarded README to document the public skipped
output from the action metadata, stating that it is true when the guarded path
finds no lockfile and false for the unguarded path.
2 participants
GitHub Actions Shared Workflows
Description
Adds a Socket (socket.dev) supply-chain layer to the JS/TS PR umbrella.
npm audit, Trivy and CodeQL find known CVEs and insecure code. None of them find a supply-chain attack — a package with a malicious install script, a typosquat, a dependency hijacked in a patch release. That is the vector behind the recent npm ecosystem incidents, and it is what Socket covers through behavioral analysis of the package itself.Wired as two independent layers so the free tier can ship now and the paid tier is ready to switch on later.
Affected workflows
.github/workflows/js-pr-validation.ymlsocketandsocket-gatejobssrc/security/socket-firewall/src/security/socket-scan/docs/js-pr-validation.md.github/dependabot.ymlSocketDev/actionadded to thesecurity-scannersgroupFree tier —
src/security/socket-firewall(on by default)Wraps
SocketDev/actioninfirewall-freemode (pinned by SHA,v1.3.2): installs Socket Firewall, which shimsnpm/yarn/pnpm, then runs the project's dependency install through it. A malicious package makes the install exit non-zero and theSocketcheck goes red. No token, no account, no cost.Blocking by default (
socket_fail_on_block: true) — a blocked package is malware, not a finding to triage.falsedowngrades confirmed blocks to::warning::; an install that fails for an ordinary reason always fails, so the toggle cannot hide a real breakage.The shim only protects installs in the same job, so this does a clean install of the same lockfile in a dedicated job rather than patching the 12 install steps inside
frontend-pr-analysis.yml. Same detection, one maintenance point.Paid tier —
src/security/socket-scan(off by default)Runs
socketcli(pip install socketsecurity, the vendor-documented CI path), which posts the full alert report on the PR and enforces the organization's Socket policy. Advisory when enabled (socket_fail_on_findings: false→--disable-blocking), following the same discipline asfail_on_coverage_thresholdand the pre-release gate.Without the
SOCKET_SECURITY_API_KEYsecret it skips with a::notice::and stays green, so it can be wired in before the account exists. Socket API errors (exit 3) and unmet reachability prerequisites (exit 5) are always advisory — they say nothing about the dependencies under review.Type of Change
feat: New workflow or new input/output/step in an existing workflowfix: Bug fix in a workflow (incorrect behavior, broken step, wrong condition)perf: Performance improvement (e.g. caching, parallelism, reduced steps)refactor: Internal restructuring with no behavior changedocs: Documentation only (README, docs/, inline comments)ci: Changes to self-CI (workflows under.github/workflows/that run on this repo)chore: Dependency bumps, config updates, maintenancetest: Adding or updating testsBREAKING CHANGE: Callers must update their configuration after this PRBreaking Changes
None. Every new input has a default, no existing input or default changed, and
SOCKET_SECURITY_API_KEYisrequired: false. No caller migration needed.Worth flagging before merge:
socket_enable_firewalldefaults totrue, so the first caller pinning the new tag gains a job that runsnpm ci. Two failure modes were considered:package-managerinworking-dirand skips with a::warning::pointing atsocket_working_dirinstead of failing..npmrc, so a repo whose install needs a private registry token must setrun_socket: falseuntil that is addressed. Note this is the same limitationfrontend-pr-analysis.ymlalready has (its 12 install steps also configure no.npmrc), so no repo that works today regresses — but a repo relying on a preinstalled runner-level.npmrcshould verify.Opt-out is a single input:
run_socket: false.Testing
@this-branchor the beta tagValidated locally:
yamllint -c .yamllint.ymlon all four changed/new YAML files — no warning beyond the repo-wide baseline (SHA-pin comment spacing, and two pre-existing longfilter_pathsdescriptions).bash -non everyrun:block in the two new composites and the umbrella.inputs.*reference is undeclared;socket/socket-gatewired to the change gate exactly likesecurity/security-gate.socketcli; nothing echoes it. Presence is resolved inside the composite because thesecretscontext is unavailable in stepif:.Not yet validated on a real caller — that is the remaining gate before promoting to
main. Plan, perdocs/js-pr-validation.md:@feat/js-pr-validation-socket, open a PR → expectSocket (checks)green with the Socket Firewall job summary.npm ci,::error::,Socketred. Thensocket_fail_on_block: false→::warning::, green.socketskipped,Socketgreen.socket_enable_scan: truewith no secret → skip notice, green.dry_run: true→ notices with the resolved config, job never red.Caller repo / workflow run: pending — will be linked before promotion to
main.Related Issues