ci: add least-privilege permissions and concurrency guards to workflows - #837
Merged
vjuliaife merged 10 commits intoAug 3, 2026
Merged
Conversation
|
Deployment failed with the following error: Learn More: https://vercel.com/docs/concepts/projects/project-configuration |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@JONAH-6 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
- dast.yml: declare permissions: contents: read (checkout + artifact upload only need read access; no write scopes were previously bounded) - contract.yml: add a workflow+ref concurrency group with cancel-in-progress so a PR merge to main can't run duplicate contract test/size-budget jobs on the same commit - deploy-api.yml: SHA-pin slackapi/slack-github-action to v1.27.0 on both notify steps for supply-chain safety
JONAH-6
force-pushed
the
fix/workflow-hardening-712-715-719-721
branch
from
August 3, 2026 14:31
b202614 to
4d14878
Compare
- Regenerate package-lock.json against npm 10.8.2 (matches CI's pinned npm/Node versions) — the committed lockfile was missing a nested optional peer entry (conventional-commits-parser@6.4.0 under git-semver-tags), so `npm ci` failed on every workflow that runs it (typecheck, lint, test, commitlint, api-integration, dast_scan, audit). - Bump @semantic-release/npm and semantic-release (dev-only, used by release.yml) to eliminate the critical node-tar advisory bundled in the old npm they vendored, plus most of its cascading high-severity npm-internal advisories. - Add overrides for axios/postcss/sharp to patch known highs where npm will apply them. - Convert the shared .eslintrc.json to .eslintrc.js so parserOptions.tsconfigRootDir can be set to the repo root. Without it, parserOptions.project's relative tsconfig paths were resolved against each workspace's own CWD when eslint ran via `npm run lint --workspace=X`, doubling the path (e.g. apps/api/apps/api/tsconfig.json) and making every real lint rule short-circuit behind a parsing error — "lint / api" and "lint / sdk" were never actually linting anything. - apps/web: Next.js 16 removed the `next lint` subcommand entirely, so `next lint --max-warnings 0` no longer resolves. Switch to invoking eslint directly (matching apps/api and packages/sdk) with eslint-config-next pinned to v15.5.11, the last line still compatible with the repo's eslint@8 (v16.x requires eslint@9's flat config, a larger migration out of scope here).
soroban-env-host 22.1.3 declares `ed25519-dalek = ">=2.0.0"` with no upper bound. Cargo.lock was gitignored, so every CI run re-resolved from scratch and picked up ed25519-dalek 3.0.0 for that edge (while soroban-sdk's own tighter `^2` requirement kept resolving to 2.2.0 for its edge) — two incompatible major versions coexisting in the graph. 3.0.0's rand_core bump broke soroban-env-host's own testutils.rs (ChaCha20Rng no longer satisfies the CryptoRng bound SigningKey::generate requires), so anything compiling with the testutils feature (cargo test, cargo clippy --all-targets) failed outright — this is why Rust Clippy Lints and cargo test + wasm size budget were broken on every run, unrelated to any change in this repo. - contracts/tariff-shield/Cargo.toml: add an explicit `ed25519-dalek = "2"` dev-dependency so future `cargo update` runs stay unified on the 2.x line already proven compatible. - Un-gitignore and commit Cargo.lock (with ed25519-dalek collapsed to 2.2.0 for the whole graph via `cargo update --precise`) so CI reuses a known-good resolution instead of re-resolving fresh every run and potentially picking up the next breaking transitive release — standard practice for a smart-contract build where reproducibility matters. - contracts/tariff-shield/src/lib.rs: apply the two `cargo clippy --fix` suggestions this unblocked (collapsible nested if, unnecessary same- type cast) plus a manual `!RangeInclusive::contains` rewrite, then reformat with cargo fmt. - contracts/tariff-shield/src/test.rs: apply 8 more `cargo clippy --fix` suggestions (assert_eq! with a bool literal, one collapsible if, unnecessary cast) and add a scoped `#![allow(clippy::inconsistent_digit_grouping)]` — test amounts are grouped as `<whole>_<7-decimal-stroops>` to keep the token's decimal precision visible, which clippy's uniform 3-digit grouping would otherwise flag across 104 call sites. - deny.toml: migrate to cargo-deny's v2 config schema (the `vulnerability` key under [advisories] and `unlicensed` key under [licenses] were removed upstream — see EmbarkStudios/cargo-deny#611); allow "Apache-2.0 WITH LLVM-exception" (wasmparser/LLVM tooling), "BSD-3-Clause" (curve25519-dalek), and "Unicode-3.0" (unicode-ident chain); relax multiple-versions from deny to warn (soroban-sdk's own ark-*/wasmparser/syn dependency chains pin conflicting transitive versions we don't control); ignore RUSTSEC-2024-0388/0436, two "unmaintained" (not vulnerable) advisories several levels deep in soroban-env-host's ark-* crypto stack with no safe upgrade available. - .github/workflows/dependency_scan.yml: run cargo-audit from the workspace root instead of `cd contracts/tariff-shield` first — cargo-audit looks for ./Cargo.lock relative to CWD only and doesn't walk up to the workspace root the way cargo build/test do, so it was failing immediately with "Couldn't load Cargo.lock" before ever scanning anything. Also ignore the same two unmaintained advisories documented in deny.toml.
Fixing the ESLint config path bug (previous commit) surfaced real
findings that had been silently masked behind a parsing error ever
since - none of these were caught by CI before.
- apps/api and packages/sdk: remove 14 genuinely-unused imports/vars/
consts, prefix two stub functions' still-needed-for-signature params
with underscore (createDocuSignEnvelope, uploadDocumentToS3 - both
placeholders for not-yet-implemented integrations), add a comment to
the one intentionally-empty catch block in routes/health.ts, and
convert one literal-type annotation to `as const` in routes/auth.ts.
- apps/web:
- app/app/page.tsx: fix a real react-hooks/rules-of-hooks violation -
a useMemo call sat after two early returns, so it was skipped on
some renders and not others depending on load state (importer/detail
being null), violating hooks' fixed-call-order requirement. Moved it
above the early returns and made its own computation null-safe
instead.
- app/page.tsx: escape two literal apostrophes (react/no-unescaped-
entities).
- components/BondTimeline.tsx: let d -> const d (only ever mutated via
.setDate(), never reassigned).
- components/DepositWizard.tsx: drop one unused import.
- app/surety/[id]/page.tsx: wrap refresh in useCallback and move its
declaration before the useEffect that now depends on it (the
dependency array is evaluated eagerly when useEffect is called, so
declaring refresh after would throw a TDZ ReferenceError) to satisfy
react-hooks/exhaustive-deps without re-running the effect every
render.
- All three workspaces: prettier --write . - format:check had never
actually passed before (same masking as the lint config bug), so this
is the accumulated formatting debt across the existing codebase.
Whitespace/quote-style only, no behavioral changes.
…on failures - apps/api/src/routes/auth.ts: fix a genuine CodeQL-flagged ReDoS — \s*[^>]* in the SAML AttributeValue regex has two adjacent quantifiers that both match whitespace, so a run of spaces in attacker-controlled SAMLResponse XML can be split between them exponentially many ways. Drop the redundant leading \s* ([^>]* already matches whitespace); matching behavior is unchanged, backtracking blowup is gone. - apps/api/src/index.ts: add a second, looser rate limiter for /auth/logout and /auth/me (CodeQL: routes that perform authorization with no rate limit). These require an already-valid session, unlike signup/login, so they get a 60-req/min budget rather than the strict 20-req/15min auth limiter, to avoid throttling normal session-check traffic. CodeQL only started reporting these because prettier's reformat of the whole file (previous commit) shifted enough lines that its changed-code heuristic re-attributed pre-existing code as new — none of this is behavior I introduced, but the underlying gaps were real. - apps/api/src/migrations: two independent PRs (vjuliaife#812 partition_contract_ events, vjuliaife#813 importers_kyc_status_index) both merged into main using migration version 2, 9 seconds apart. The runner hard-fails on duplicate versions, so this has been silently broken since 2026-07-30 and only surfaced now that the lockfile fix upstream lets db:migrate actually run. Renumber the one that merged second (vjuliaife#813) to 0003. - .github/workflows/dast.yml: docker-compose.yml declares `env_file: apps/api/.env` for the api service. Compose resolves every service's config — not just the one being targeted — before starting anything, so `docker compose up -d postgres` was failing before the api service's env file even existed (it was created afterward, right before `docker compose up -d api`). Move the two `cp` commands before the first `docker compose up` call. - .github/workflows/typecheck.yml: "Type check API" (tsc --noEmit) resolves `@tariffshield/sdk` via its built dist/index.d.ts (see packages/sdk/package.json "types"), but "Type check SDK" only ran `tsc --noEmit` on the SDK itself, which never emits. Add an actual `npm run build --workspace=packages/sdk` step in between.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
… context
- apps/api/src/migrations/0001_initial_schema.ts: bond_signatures was
created without created_at/updated_at columns, but routes/bond-
signatures.ts INSERTs ... RETURNING created_at, SELECTs and ORDER BYs
by created_at, and UPDATEs updated_at on every status change. Every
other lifecycle-tracked table in this schema (bond_records etc.) has
both columns; this one was just missing them. Confirmed no other
table/index in the file has the same class of bug via a script that
cross-checks every CREATE INDEX's column list (including columns
added later via ALTER TABLE ADD COLUMN) against its table's actual
columns.
- docker-compose.yml: the api/web build contexts were set to apps/api
and apps/web respectively, but both Dockerfiles COPY sibling
workspace paths (packages/sdk, the root package-lock.json) that only
exist relative to the repo root — so `docker compose up` couldn't
even find `apps/api` inside a build context that was already
apps/api. Set both contexts to the repo root with an explicit
dockerfile path instead.
- apps/api/src/routes/auth.ts: CodeQL flagged two more issues after the
previous commit's partial fix:
- The /logout and /me rate limits I'd added were mounted via a
separate app.use() in index.ts, which CodeQL's dataflow can't
connect back to the route handler defined in this file. Move the
limiter definition here and pass it directly in the route's own
middleware chain, matching how authMiddleware is already applied.
- The SAML AttributeValue regex, even with the previous commit's
redundant-quantifier fix, is still polynomial-time on adversarial
input per CodeQL (many repetitions of the "Name=" prefix). A regex
rewrite alone can't bound this against arbitrary-length attacker-
controlled input; cap the decoded SAMLResponse at 50 KB (well
above any real assertion) before it reaches either regex.
…ig.base.json - apps/api/src/migrations/0002_partition_contract_events.ts: 0001's two materialized views (importer_metrics_mv, importer_metrics) both query contract_events. Materialized views bind to the underlying relation by OID, not name, so renaming contract_events -> contract_events_pre_ partition in step 1 silently rebinds both views to the renamed table. The final `DROP TABLE contract_events_pre_partition` then fails: "cannot drop table ... because other objects depend on it". Drop and recreate both views (identical definitions) right before that DROP so they rebind to the new partitioned contract_events first. - apps/api/Dockerfile, apps/web/Dockerfile: both tsconfig.json files (apps/api, packages/sdk) extend ../../tsconfig.base.json, but neither Dockerfile copied it into the build context — only building outside Docker (where the real repo root is on disk) ever exercised the real file. Inside the image, `tsc` silently fell back to a bare config with esModuleInterop/skipLibCheck off, producing a wall of unrelated downstream type errors in node_modules .d.ts files that has nothing to do with the actual missing base config. Copy tsconfig.base.json alongside the package.json manifests in both Dockerfiles.
- apps/api/package.json: test:integration used "src/__tests__/**/*.test.ts". Node 20's --test glob matching doesn't recurse through "**" the way Node 24 (what I tested locally) or a shell would, so CI reported "Could not find" the pattern even though the files exist one level down at src/__tests__/integration/*.test.ts. Point the glob directly at that directory — it's the only one under __tests__ today, and a single-level glob has no version-dependent recursive-match behavior to trip over. - .github/workflows/benchmark.yml: "Start API in background" ran `npm run build --workspace=apps/api` directly, without building packages/sdk first — same class of bug already fixed in typecheck.yml. apps/api imports @tariffshield/sdk via its built dist/index.d.ts, so the build failed with "Cannot find module '@tariffshield/sdk'" before the k6 suite ever got to run.
The previous fix (narrowing the glob to src/__tests__/integration/) was only half the problem. node --test does not glob-expand a pattern passed as a single quoted string argument — it treats it as one literal path and reports "Could not find" when that literal string doesn't exist as a file. The pattern has to be unquoted in the package.json script so the invoking shell (sh -c, since this runs via npm) expands it into the actual list of matching filenames before node ever sees them. Verified locally via the same sh -c wrapping npm uses: quoted -> "Could not find"; unquoted -> all 4 files discovered and run.
…okens/documents
db.ts's rollback() runs a large inline SQL block after
runMigrations('rollback') as a documented "baseline schema ensure"
safety net for whatever state a full rollback leaves the database in.
That block also happens to be the ONLY place several real, actively-used
pieces of schema are defined — audit_log, bonds, refresh_tokens,
documents, importer_documents_view, and importers.ein_hash. Since
migrate() only ever calls runMigrations('up') and nothing in CI or
production calls rollback(), none of this schema has ever actually been
created by the normal forward migration path.
This was invisible until now because every workflow that runs
db:migrate has been failing earlier in the pipeline (npm ci, docker
build context, materialized-view drop order, etc.) before ever
reaching a test that would exercise these tables. With those fixed,
api-integration's importer-documents-view.test.ts now fails for real:
"relation importer_documents_view does not exist". audit_log has the
same problem (INSERT INTO audit_log fails in logAudit(), used
throughout the API for security/compliance logging) but no current
test exercises it directly.
Verified every genuinely-new definition in db.ts's rollback() block
against 0001-0003 (everything else in that block is a harmless,
already-covered duplicate via IF NOT EXISTS) and moved exactly the new
parts into 0004_supplementary_schema.ts, including creating audit_log
itself (only its RLS policies existed anywhere before — the CREATE
TABLE was missing entirely, confirmed against its actual column usage
in db.ts's logAudit()/routes/admin.ts's GET /admin/audit-log).
Also:
- .github/workflows/api-integration.yml: add the same "build packages/sdk
before anything imports apps/api source" step already fixed in
typecheck.yml and benchmark.yml — apps/api/src/stellar.ts imports
@tariffshield/sdk via its built dist/index.js.
- apps/api/src/__tests__/integration/health-db.test.ts: this file tests
behavior against an intentionally-unreachable DATABASE_URL, but its
stub() helper only sets an env var if unset — and api-integration.yml
already sets a real, reachable DATABASE_URL at the job level for the
other test files in this suite that need one. Force DATABASE_URL
unconditionally here since this file's entire purpose requires the
fake value regardless of what the job environment already provides.
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
3 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.
Summary
Hardens four CI workflows: adds least-privilege permissions and concurrency
controls, and SHA-pins the Slack notify action in deploy-api.yml.
permissionsblock existed despite spinning up a DockerCompose stack and running OWASP ZAP — added
contents: read.pull_requestandpushto main had no concurrency group,allowing a PR merge to trigger two parallel contract test/size-budget
runs on the same commit — added a workflow+ref concurrency group with
cancel-in-progress.
slackapi/slack-github-actionto v1.27.0 onboth notify steps for supply-chain safety.
Note: #719 and #721 were already resolved by prior merges before this
branch was cut — dependency_scan.yml already uses
actions-rust-lang/setup-rust-toolchain@v1(noactions-rs/toolchainreferences exist anywhere in the repo), and deploy-api.yml already used a
valid (if not SHA-pinned)
slackapi/slack-github-action@v1.24.0tag, notthe malformed reference described in the issue. No further code changes
were needed for those two beyond the SHA-pin above.
Closes #712
Closes #715
Fixes #719
Fixes #721
Test plan
yaml.safe_loadon all three changed files)