chore(ci): add security workflow with dependency audit, CodeQL, and secret scanning#56
Closed
OkeyAmy wants to merge 71 commits into
Closed
chore(ci): add security workflow with dependency audit, CodeQL, and secret scanning#56OkeyAmy wants to merge 71 commits into
OkeyAmy wants to merge 71 commits into
Conversation
The Lint & Format job runs prettier --check over workflow YAML; the aligned trailing comment failed it on every push.
Updated the README with a new link and added a video description.
- prettier-clean the README video block (fixes CI format:check on main) - bump version to 0.1.1 - CHANGELOG: add [0.1.1] docs-only entry Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
NPM_TOKEN secret was never configured, so tag-triggered releases failed with ENEEDAUTH. Auth now uses npm trusted publishing (configured on npmjs.com for this repo + release.yaml): - drop NODE_AUTH_TOKEN env (empty token would shadow OIDC auth) - upgrade npm to >= 11.5.1 (trusted publishing requirement; Node 22 bundles 10.x) - bump checkout/setup-node to v5 ahead of the June 16 Node 20 runner cutoff
ci(release): switch npm publish to OIDC trusted publishing
- Onboarding consolidated into `testsprite setup` (formerly `init`); the granular auth commands remain as hidden, deprecated aliases. - CLI reports its version in the User-Agent header. - README: the launch video no longer renders as a bare URL on npm.
create-batch --run launched its first concurrencyLimit triggers in parallel, but the steady-state loop awaited each subsequent job to fully finish (trigger + full --wait poll) before launching the next one. Effective concurrency dropped to 1 after the initial wave regardless of --max-concurrency. Switch to the launch-then-race pattern already used by the other three fan-outs in this file: launch up to the limit, relaunch on each completion via startNext(), never await a whole job inline. Add a regression test using equal-delay trigger responses so the first wave settles in the same microtask batch, which is the exact condition that exposed the bug.
…urrency fix(test): prevent batch-run scheduler from serializing after first wave
…estSprite#7) test code get --out <path> opened (truncated) the destination file before the network request. If the GET then failed, or hit the "no code generated yet" branch which writes nothing, the user's pre-existing --out file was left emptied with no way to recover it. Write to a sibling temp file instead and rename it onto the real path only after a successful, complete write. Mirrors the atomic rename contract bundle.ts already uses for multi-file bundles. Add regression tests for both failure modes: a failing fetch and the no-code-yet branch. Both reproduce the truncation on the old code and pass on the fix.
…ite#9) Batch-rerun chunks (>50 testIds) were dispatched concurrently via Promise.all. The backend's producer/teardown closure dedup happens per-request, not across requests, so two concurrent chunks sharing a project's producer could each independently decide to trigger it, double-running the producer or teardown. Dispatch chunks sequentially in both the initial and deferred-retry paths, closing the race at the source. Also dedupe the aggregated accepted[] by testId and merge closure.byProject across chunks as a defensive second layer, warning on stderr if a duplicate trigger is detected. Fixed a pre-existing test whose fixture relied on the old double-counting behavior (same accepted entry returned from every retry call).
…RROR (TestSprite#19) A malformed API endpoint produced an opaque or misleading failure instead of a clear config error: --endpoint-url "not a url" -> `Error: Invalid URL` (exit 1, a raw `new URL()` throw with no guidance) --endpoint-url "localhost:3000" (missing scheme, parses as scheme "localhost:") and "ftp://x" (wrong scheme) -> `fetch failed` / Service unavailable, emitted only after a full retry+backoff cycle — looks like a network outage, not a config typo. Add `assertValidEndpointUrl` in client-factory.ts and run it in both the real and dry-run paths of `makeHttpClient`, on the resolved endpoint (so it covers --endpoint-url, TESTSPRITE_API_URL, and the credentials file). A malformed value now throws a typed VALIDATION_ERROR (exit 5) with an actionable message. Crucially, and unlike the `--target-url` SSRF guard, this does NOT reject localhost or private hosts — the API endpoint legitimately points at a self-hosted, local-dev, or mock backend. Only syntactically invalid values (unparseable, or a non-http(s) scheme) are rejected, so existing self-hosted/CI configs and the test suite's localhost mock backend are unaffected. Adds unit coverage for assertValidEndpointUrl and the two makeHttpClient paths, plus subprocess regressions.
runFailureGet now resolves and validates --out via resolveBundleDir and assertOutDirParentExists before calling GET /tests/{id}/failure, matching runArtifactGet and runCodeGet fast-fail behavior.
Adds regression tests asserting zero fetch calls on empty --out and missing parent dir paths.
…TestSprite#23) Co-authored-by: zeshi-du <zeshi@testsprite.com>
…on (TestSprite#21) A profile name (`--profile` / `TESTSPRITE_PROFILE`) is written verbatim as an INI section header (`[name]`) in `~/.testsprite/credentials`, but was never validated. A name containing the characters that break that grammar silently corrupted the file: --profile "prod]" -> serialises to `[prod]]`, which the section regex cannot match, so the api_key/api_url lines that follow are DROPPED on read. `setup` reports success while the credential never persists. --profile $'a\nb' -> the newline splits the header across two lines. --profile " x " -> does not round-trip (the parser trims section names, so it reads back as `x`). Add `assertValidProfileName` in credentials.ts (a conservative allowlist: letters, digits, dot, underscore, hyphen — covering `default`, `prod`, `ci-staging`, `team.qa`) and call it from every profile-keyed entry point (`readProfile`, `writeProfile`, `deleteProfile`). A malformed name now throws a typed VALIDATION_ERROR (exit 5) before any file write, instead of silently corrupting or failing to persist credentials. Adds unit coverage for the guard and the three entry points, plus a subprocess regression. Co-authored-by: Zeshi Du <duke.zeshi@gmail.com>
…t json (TestSprite#22) When a subcommand fired a parse error (unknown command, missing required argument, invalid option), Commander's outputError callback wrote plain text to stderr immediately and the catch block exited 5 with no further output. A machine consumer that always parses stderr as JSON received an unexpected plain-text string and crashed its JSON.parse. Root cause: configureOutput was only applied to the root program, not to subcommands. Each subcommand retained the default outputError that calls write(str) directly. applyExitOverrideDeep now also propagates configureOutput to every leaf so the message is buffered instead of written. In the CommanderError catch block, a resolved output mode is used to either write a VALIDATION_ERROR JSON envelope or the buffered plain-text message. An argv scan fallback handles the edge case where --output json appears after the bad argument and was not yet parsed when the error fired. The renderCommanderError helper is extracted to src/lib/render-error.ts (alongside the existing rephraseUnknownOption helper) so it is unit-testable without a subprocess. Eight unit tests cover JSON/text output, null fallback, message trimming, and rephrased global-flag embedding. Four subprocess regression tests in the [fix-5] block cover missing-arg, unknown subcommand, argv-fallback, and text-mode no-regression paths. Co-authored-by: zeshi-du <zeshi@testsprite.com>
… gitignore Adds a 5-job GitHub Actions security pipeline (dependency audit, dependency review, CodeQL, ESLint security rules, secret scanning via gitleaks). No external API keys required — all jobs use GITHUB_TOKEN or npm public registry. Adds eslint.security.config.mjs for targeted security linting of src/ using eslint-plugin-security. Updates main eslint config and .gitignore to exclude the local sandbox directory.
OkeyAmy
changed the title
New issue form for hackathon submissions — auto-applies the `hackathon` label and captures the submitter's Discord identity for reward payout coordination.
Contributor
|
@coderabbitai review |
✅ Action performedReview finished.
|
WalkthroughAdds a new GitHub Actions "Security" workflow with jobs for dependency audit, PR dependency review, CodeQL analysis, ESLint-based security linting, and secret scanning via gitleaks. Introduces a dedicated ESLint security config, and updates ChangesSecurity CI Workflow Addition
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant GH as GitHub Actions
participant NPM as npm audit
participant CodeQL as CodeQL
participant ESLint as ESLint Security
participant Gitleaks as Gitleaks
Dev->>GH: Open/update pull request or push
GH->>NPM: Run npm audit (prod + all deps)
GH->>GH: Run dependency review (PR only)
GH->>CodeQL: Build project & run CodeQL analysis
GH->>ESLint: Lint src/ with security rules
GH->>Gitleaks: Scan full git history for secrets
GH-->>Dev: Report results/failures
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…rd (TestSprite#37) assertNotLocal lowercased the hostname but did not strip a trailing dot, so http://localhost. (the FQDN form of localhost, RFC 6761) and http://localhost%2e bypassed the host === 'localhost' loopback check. IP literals are already dot-normalized by the WHATWG URL parser, so only named hosts were affected. Strips one trailing dot before the comparison. Adds 4 regression tests (3 blocked variants + 1 public-FQDN no-false-positive).
* fix(skill-nudge): require complete Codex managed section * docs(skill-nudge): document helper contracts --------- Co-authored-by: ahndohun <19940813+ahndohun@users.noreply.github.com>
…Sprite#166) A successful (200) response whose body is not JSON — a misconfigured endpoint, a proxy / captive-portal / login page returning HTML with a success status, or an empty body — caused the OK path to call raw `response.json()`, whose SyntaxError escaped to the top-level handler. That produced an opaque exit 1 and, under --output json, a bare `{"error":"<parse message>"}` that breaks the typed-envelope contract every other error honors (the non-OK path already reads defensively via safeReadJson). Wrap the OK-path parse failure in a typed INTERNAL ApiError (exit 1 unchanged) that carries the requestId, names the likely cause, and points the operator at their endpoint config; details include the HTTP status, content-type, and underlying parse message. Abort/timeout errors mid-read keep their existing classification. Fixes TestSprite#94
TestSprite#33) pollAccepted in runTestRerun hardcoded exitCode:1 on ApiError, causing the auth-escalation find(r => r.error?.exitCode === 3) to always return undefined -- auth failures silently exited 1 instead of 3. - preserve err.exitCode in pollAccepted (mirrors runTestRunAll fix) - add auth escalation block before generic exit-1 throw - bound initial chunk idempotency key to <=256 chars (mirrors retry path)
* feat(agent): add kiro as an install target
Adds kiro as an own-file agent target on the current multi-skill model: AgentTarget union, a pathFor('kiro') case landing at .kiro/skills/<skill>/SKILL.md, and a TARGETS entry (experimental, own-file, wrapSkill frontmatter like claude/antigravity). Kiro installs both default skills (testsprite-verify + testsprite-onboard). Updates the --target help string, README/DOCUMENTATION target lists and counts, unit + command tests (six keys, list 12 rows, five own-file targets, 10 dry-run would-write lines, renderForTarget + content-integrity coverage), and regenerates the agent/setup help snapshots. Rebuilt on current main.
* test(e2e): include kiro in target matrix guards and multi-target install
The Local E2E Tests CI job failed because two matrix-coverage guards hardcoded the target set (claude, antigravity, cursor, cline, codex) and did not include the new kiro target. Add kiro (own-file, between cline and codex to match TARGETS order) to both guards, and add kiro to the multi-target install e2e so the target is actually exercised end-to-end.
…nd test wait (TestSprite#153) Apply the fix in src/commands/ (the compiled CLI path). When the overall --timeout polling deadline is exceeded, emit {runId, status:"running"} to stdout before exit 7 so JSON agents can chain into test wait. Co-authored-by: Contributor <contributor@testsprite.com> Co-authored-by: Cursor <cursoragent@cursor.com>
…ressions (TestSprite#168) * feat(test): add "test diff <run-a> <run-b>" to isolate run-to-run regressions * test(diff): cover runDiff --dry-run branch (offline canned sample)
… files in --out dir (TestSprite#162) commitBundle's stale-file sweep removed EVERY directory entry not part of the fresh bundle, so 'test failure get --out <dir>' / 'test artifact get --out <dir>' pointed at a pre-existing, populated directory silently deleted the user's unrelated files (exit 0, no warning) — on the very first write, not just re-commits. Scope the sweep to entries the bundle format owns (result.json, failure.json, video.mp4, meta.json, steps, .tmp, .partial, code.<ext>). Stale bundle files are still cleaned — an old video.mp4 when the new bundle has no video, a code.py when the new bundle writes code.ts — but foreign files and directories are never touched. Fixes TestSprite#159 Co-authored-by: Kshitij Bhardwaj <tothemoon202154@outlook.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…al without --all (TestSprite#163) Two silent-footgun gaps in test rerun's flag validation: 1. Explicit test IDs combined with --all silently discarded the listed IDs — the --all branch resolves the full project test set and overwrites them — so 'rerun test_abc --all' dispatched a batch rerun of EVERY test in the project, burning rerun/auto-heal credits with no error. Both siblings already guard this exact ambiguity (test run's positional+--all guard, delete-batch's ids+--all guard). 2. --status <list> and --skip-terminal without --all were silently ignored — including INVALID --status values, which were never validated — while the same misuse of rerun's own --filter (and delete-batch's --status) exits 5. All three narrowing filters now share the same guard. Both reject with VALIDATION_ERROR (exit 5) before any network dispatch. Fixes TestSprite#160 Co-authored-by: Kshitij Bhardwaj <tothemoon202154@outlook.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…e#171) The 'testsprite usage' command help text listed three examples but omitted the --debug flag (a global option useful for diagnosing auth and network issues). It also didn't document the exit codes, which matters for CI/CD scripts that gate on the return value. This PR adds: - A --debug example showing what it traces (HTTP method/path, request id, latency) — useful when debugging 'auth error' or 'transport failure' messages. - An explicit exit-codes section (0 success, 3 auth error, 10 network failure) so users scripting against the CLI know what to expect. Pure documentation improvement — no behavioral change, no new deps. Tested: existing unit tests pass (npm test). Co-authored-by: MOAAMN SAYED <moaamnsayed560@gmail.com>
…t pure) (TestSprite#31) Interactive prompts (`prompt.ts` — the API-key prompt during `setup`/`auth configure`, the target prompt during `agent install`) wrote the question and masking to STDOUT, and the "Configuring profile …" prelude defaulted to stdout too. On the interactive path that mixes UI text into stdout — and under `--output json` it breaks the contract that stdout is a single JSON document, so a consumer doing `JSON.parse(stdout)` fails. Default both to stderr: prompts and informational preludes are interactive UI, not result data. stdout now carries only the command's result (the §8.1 stdout-purity principle the repo already enforces elsewhere). stderr is still the user's TTY, so prompts remain visible; the secret is still never echoed. Callers that inject explicit streams are unaffected. Adds regression tests: promptText writes the question to stderr by default, and the configure prelude lands on stderr (not the result stdout).
* block artifact download redirects * redact artifact download urls --------- Co-authored-by: merlinsantiago982-cmd <merlinsantiago982-cmd@users.noreply.github.com>
* fix(test): validate artifact run id default path * fix(test): reject windows dot artifact run ids * fix(test): reject windows dot-suffix artifact run ids * style(test): format artifact run id guard --------- Co-authored-by: Lexiie <28455136+Lexiie@users.noreply.github.com>
…TestSprite#11) * feat(cli): add runtime Node.js version check with clear error message * refactor(version-guard): extract to a documented module tested against the real implementation
Windsurf (Cascade) reads workspace rules from `.windsurf/rules/*.md`. Add it as an own-file agent target so `testsprite agent install --target windsurf` (and `setup --agent windsurf`) installs the TestSprite skills into a Windsurf project. Reworked onto the v0.2.0 multi-skill agent-targets API (pathFor / SKILLS / DEFAULT_SKILLS). Rule files use Cascade frontmatter with `trigger: model_decision` — the equivalent of the Cursor `.mdc` `alwaysApply: false` mode (description shown up front; full body pulled in on relevance). Budget handling: a `.windsurf/rules/*.md` file caps at ~12 K characters and Cascade silently truncates beyond it, which would cut the full ~22 KB verify skill in half. The windsurf target therefore renders the COMPACT body per skill (new `compactBody` flag + `compactBodyFor`): a skill that ships a trimmed codex asset (`testsprite-verify` → ~5 KB) uses it, while a skill whose codex contribution is only a one-liner (`testsprite-onboard`, ~6.5 KB full) keeps its full body — both land well under the cap. `agent.ts` and `renderForTarget` select the same body so installed bytes match the render. Everything else derives from the TARGETS map automatically (agent list, the setup --agent choices, skill-nudge install detection). Updated the hardcoded help strings, the --help snapshot, the agent-targets/agent unit tests (incl. Cascade-frontmatter and per-skill budget tests), the e2e matrix guards / content-integrity (gated on compactBody), and the README/DOCUMENTATION target lists (incl. the --force own-file list).
… CI proxies (TestSprite#169) * feat(cli): honor HTTPS_PROXY/HTTP_PROXY/NO_PROXY behind corporate and CI proxies * fix(proxy): degrade to default dispatcher when proxy agent init fails instead of crashing startup * fix(proxy): pin undici to ^7.16.0 for Node 20 compatibility (8.x requires Node >=22.19)
…m check, opt-out, CI-safe) (TestSprite#181)
…e#132) * feat(cli): add 'test flaky' repeat-run flaky-test detector * fix(flaky): cap --runs at 10 per maintainer scope (TestSprite#115) Rescope the flaky detector's --runs bound from 1-100 to 1-10 as requested in the TestSprite#115 triage: uncapped FE replays amplify free executions. Updates the MAX_FLAKY_RUNS constant (which drives the validation, error message, and --runs help text), docs, changelog, and the runs-bound tests. Regenerates the help snapshot, which also adds the previously-missing 'test flaky' entry. * test(snapshot): refresh flaky help snapshot after rebase onto main Rebasing onto current main (which added the global --request-timeout option, TestSprite#17) changes the 'Global options' line rendered in the test flaky --help output. Regenerate the snapshot so the help snapshot test stays green on CI. * docs(changelog): resolve leftover merge-conflict markers (keep JUnit + flaky 1-10)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
.github/workflows/security.yml (2)
97-98: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAd-hoc install of
eslint-plugin-securityoutside the lockfile.Installing the plugin at runtime with
--no-savemeans the exact version isn't pinned or reproducible across CI runs — a new release could change rule behavior or introduce supply-chain risk without review. Consider adding it as a pinned devDependency instead.🤖 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/security.yml around lines 97 - 98, The security workflow currently installs eslint-plugin-security ad hoc with npm install --no-save, which leaves the version unpinned and non-reproducible. Update the workflow to use the existing package management flow instead by adding eslint-plugin-security as a pinned devDependency in the project manifest and lockfile, and then remove the runtime install step from the security workflow so it relies on the checked-in dependency version.Source: Linters/SAST tools
22-22: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSet
persist-credentials: falseon checkout steps.None of the
actions/checkoutsteps setpersist-credentials: false, leaving theGITHUB_TOKENpersisted in the local git config for the remainder of the job. Flagged by zizmor as credential persistence (artipacked); low-risk here but easy to harden.Example
- uses: actions/checkout@v5 + with: + persist-credentials: falseAlso applies to: 45-45, 61-61, 88-88, 110-112
🤖 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/security.yml at line 22, Update each actions/checkout step in the security workflow to disable credential persistence by adding persist-credentials: false. Apply this to every checkout invocation in the workflow (including the ones referenced by the review) so the GITHUB_TOKEN is not left in the local git config for the rest of the job.Source: Linters/SAST 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.
Inline comments:
In @.github/workflows/security.yml:
- Line 22: The workflow still references third-party actions by mutable tags
instead of full commit SHAs. Update every action use in the security workflow,
including actions/checkout, actions/setup-node,
actions/dependency-review-action, github/codeql-action/init,
github/codeql-action/analyze, and gitleaks/gitleaks-action, so each is pinned to
a 40-character commit SHA rather than a version tag. Ensure the corresponding
action entries in the workflow are replaced consistently wherever these symbols
appear.
- Around line 9-11: Scope the workflow permissions to least privilege by
removing the workflow-wide security-events write grant from the top-level
permissions block in security.yml and applying it only to the codeql job that
uploads SARIF results. Keep contents: read at the workflow level, then add a
job-level permissions block for codeql with security-events: write so audit,
dependency-review, lint-security, and secret-scan do not inherit unnecessary
write access.
In `@eslint.security.config.mjs`:
- Around line 30-36: Update the stale rule comments in
eslint.security.config.mjs so they match the actual severities in the security
rule block: adjust the note above security/detect-non-literal-regexp to reflect
that it is set to warn, and revise the “Disable rules that generate too much
noise” comment near
security/detect-non-literal-require/security/detect-unsafe-regex so it no longer
implies both are off when only detect-non-literal-require is disabled.
---
Nitpick comments:
In @.github/workflows/security.yml:
- Around line 97-98: The security workflow currently installs
eslint-plugin-security ad hoc with npm install --no-save, which leaves the
version unpinned and non-reproducible. Update the workflow to use the existing
package management flow instead by adding eslint-plugin-security as a pinned
devDependency in the project manifest and lockfile, and then remove the runtime
install step from the security workflow so it relies on the checked-in
dependency version.
- Line 22: Update each actions/checkout step in the security workflow to disable
credential persistence by adding persist-credentials: false. Apply this to every
checkout invocation in the workflow (including the ones referenced by the
review) so the GITHUB_TOKEN is not left in the local git config for the rest of
the job.
🪄 Autofix (Beta)
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.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 293a6623-c25e-43b2-8e7b-3d1583eab5fd
📒 Files selected for processing (4)
.github/workflows/security.yml.gitignoreeslint.config.mjseslint.security.config.mjs
Comment on lines
+9
to
+11
| permissions: | ||
| contents: read | ||
| security-events: write # CodeQL needs this to upload SARIF results |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Scope security-events: write to the CodeQL job only.
security-events: write is only required by the codeql job to upload SARIF results, yet it's granted workflow-wide, giving audit, dependency-review, lint-security, and secret-scan unnecessary write access. Static analysis flags this as excessive-permissions.
As per path instructions, "Prefer least-privilege permissions: blocks."
Proposed fix
permissions:
contents: read
- security-events: write # CodeQL needs this to upload SARIF results
jobs:
...
codeql:
name: CodeQL
runs-on: ubuntu-latest
+ permissions:
+ contents: read
+ security-events: write # needed to upload SARIF results
steps:🧰 Tools
🪛 zizmor (1.26.1)
[warning] 11-11: overly broad permissions (excessive-permissions): security-events: write is overly broad at the workflow level
(excessive-permissions)
🤖 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/security.yml around lines 9 - 11, Scope the workflow
permissions to least privilege by removing the workflow-wide security-events
write grant from the top-level permissions block in security.yml and applying it
only to the codeql job that uploads SARIF results. Keep contents: read at the
workflow level, then add a job-level permissions block for codeql with
security-events: write so audit, dependency-review, lint-security, and
secret-scan do not inherit unnecessary write access.
Sources: Path instructions, Linters/SAST tools
| name: Dependency Audit | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v5 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Pin third-party actions to full commit SHAs.
All actions (actions/checkout@v5, actions/setup-node@v5, actions/dependency-review-action@v4, github/codeql-action/init@v3, github/codeql-action/analyze@v3, gitleaks/gitleaks-action@v2) are referenced by tag rather than a pinned 40-character commit SHA. Tags are mutable and can be repointed, allowing a compromised upstream release to run in this pipeline with security-events: write and repo checkout access.
As per path instructions, "All third-party actions must be pinned to a full 40-char commit SHA."
Example pin for checkout
- - uses: actions/checkout@v5
+ - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v5.0.0Also applies to: 45-45, 46-46, 61-61, 63-63, 68-68, 76-76, 88-88, 90-90, 110-115
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 22-22: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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/security.yml at line 22, The workflow still references
third-party actions by mutable tags instead of full commit SHAs. Update every
action use in the security workflow, including actions/checkout,
actions/setup-node, actions/dependency-review-action, github/codeql-action/init,
github/codeql-action/analyze, and gitleaks/gitleaks-action, so each is pinned to
a 40-character commit SHA rather than a version tag. Ensure the corresponding
action entries in the workflow are replaced consistently wherever these symbols
appear.
Source: Path instructions
Comment on lines
+30
to
+36
| // Error on non-literal RegExp (ReDoS) | ||
| 'security/detect-non-literal-regexp': 'warn', | ||
| // Error on child_process with non-literal args | ||
| 'security/detect-child-process': 'error', | ||
| // Disable rules that generate too much noise for a CLI codebase | ||
| 'security/detect-non-literal-require': 'off', | ||
| 'security/detect-unsafe-regex': 'warn', |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Comment/severity mismatches.
The comment on Line 30 says "Error on non-literal RegExp (ReDoS)" but the rule is set to 'warn' on Line 31. Similarly, the Line 34 comment "Disable rules that generate too much noise" precedes detect-unsafe-regex: 'warn' (Line 36), which is enabled, not disabled — only detect-non-literal-require is actually off. These stale comments could mislead future maintainers about actual enforcement.
Proposed fix
- // Error on non-literal RegExp (ReDoS)
+ // Warn on non-literal RegExp (ReDoS)
'security/detect-non-literal-regexp': 'warn',
// Error on child_process with non-literal args
'security/detect-child-process': 'error',
- // Disable rules that generate too much noise for a CLI codebase
+ // Disable/reduce noisy rules for a CLI codebase
'security/detect-non-literal-require': 'off',
'security/detect-unsafe-regex': 'warn',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Error on non-literal RegExp (ReDoS) | |
| 'security/detect-non-literal-regexp': 'warn', | |
| // Error on child_process with non-literal args | |
| 'security/detect-child-process': 'error', | |
| // Disable rules that generate too much noise for a CLI codebase | |
| 'security/detect-non-literal-require': 'off', | |
| 'security/detect-unsafe-regex': 'warn', | |
| // Warn on non-literal RegExp (ReDoS) | |
| 'security/detect-non-literal-regexp': 'warn', | |
| // Error on child_process with non-literal args | |
| 'security/detect-child-process': 'error', | |
| // Disable/reduce noisy rules for a CLI codebase | |
| 'security/detect-non-literal-require': 'off', | |
| 'security/detect-unsafe-regex': 'warn', |
🤖 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 `@eslint.security.config.mjs` around lines 30 - 36, Update the stale rule
comments in eslint.security.config.mjs so they match the actual severities in
the security rule block: adjust the note above
security/detect-non-literal-regexp to reflect that it is set to warn, and revise
the “Disable rules that generate too much noise” comment near
security/detect-non-literal-require/security/detect-unsafe-regex so it no longer
implies both are off when only detect-non-literal-require is disabled.
|
Hi @OkeyAmy — so sorry for the disruption here. 🙏 While we were publishing a new CLI release, a hiccup in our process unintentionally closed this PR. To be clear: your contribution wasn't rejected, and nothing on your end caused this. Your work is completely safe — your branch and every commit are intact and untouched, and Because of how the closure happened, the cleanest path is a fresh PR from your existing branch (rather than reopening this one) — and opening it yourself keeps it under your name, with full credit for your work: 👉 Open a new PR from your branch It should merge cleanly. So your work doesn't get lost, if we don't hear back in the next ~3 days we'll go ahead and open it for you — but opening it yourself keeps it under your name, and either way we're always happy to hand it back to you. Thank you for contributing to TestSprite, and again, we're sorry for the hassle. We really appreciate this work and want to see it merged. 🙌 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a dedicated security CI workflow that runs on every PR and push to
main,dev,stg.Changes
.github/workflows/security.yml— new security pipeline (5 jobs, zero external API keys)eslint.security.config.mjs— security-focused ESLint config usingeslint-plugin-securityeslint.config.mjs— addsandbox/**to ignore listSecurity jobs
npm audit --omit=devsecurity-extended)eslint-plugin-securityNotes
GITHUB_TOKEN(automatic) or run fully offline.eslint.security.config.mjsruns separately from the main lint so security findings surface as their own CI check.Testing
All locally-runnable steps verified clean before this PR:
npm audit --omit=dev --audit-level=high→ 0 vulnerabilitiesnpm run lint→ 0 errorsnpm run typecheck→ cleannpm run build→ cleannpm test→ 1483/1483 passSummary by CodeRabbit