Repository navigation
Add automated static security analysis and vulnerability scanning pip… - #1614
Open
Joyful12-tech wants to merge 11 commits into
Open
Joyful12-tech wants to merge 11 commits into
Joyful12-tech wants to merge 11 commits into
Conversation
…eline Payment infrastructure needs supply-chain, secret, and code vulnerabilities caught before merge rather than in a post-incident audit. The existing security workflow covered only dependency audits and CodeQL, and could not have blocked a merge: it had duplicate step IDs (invalidating the cargo-audit cache), a duplicated Rust toolchain block, and no aggregate gate. Rebuild it around a single "Security Gate" required check, and add the two scanners that were missing entirely: - TruffleHog over the full git history, blocking only on secrets it can verify are live, so unverified fixtures do not block every PR. - Semgrep with a curated .semgrep/flowfi.yml ruleset covering Stellar secret seeds, unsafe Prisma raw SQL, shell injection, eval, JWT "none", React dangerouslySetInnerHTML, and unsafe/panicking contract code. Upstream p/default findings are reported to the Security tab but do not block. Also fixes a weekly cron so CVEs published after the last dependency bump are caught without a code change, and runs CodeQL's JS/TS extraction with a resolved dependency tree, which it previously lacked. Blocking is deliberately limited to high-confidence signals: a gate that cries wolf gets ignored. Note that the dependency audit will fail on first run against pre-existing advisories (axios 1.15.0 via a lockfile still pinning @stellar/stellar-sdk 15.1.0, and @hono/node-server via prisma). That lockfile drift needs a separate fix. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v4 |
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Setup Node.js | ||
| uses: actions/setup-node@v4 |
| echo "::endgroup::" | ||
|
|
||
| - name: Setup Rust toolchain | ||
| uses: dtolnay/rust-toolchain@stable |
|
|
||
| - name: Cache cargo-audit | ||
| id: cargo-audit-cache | ||
| uses: actions/cache@v4 |
|
|
||
| - name: Setup Rust toolchain | ||
| if: matrix.language == 'rust' | ||
| uses: dtolnay/rust-toolchain@stable |
| workspace: 'contracts -> target' | ||
|
|
||
| - name: Initialize CodeQL | ||
| uses: github/codeql-action/init@v3 |
|
|
||
| - name: Autobuild | ||
| if: matrix.language != 'rust' | ||
| uses: github/codeql-action/autobuild@v3 |
| uses: github/codeql-action/autobuild@v3 | ||
|
|
||
| - name: Perform CodeQL Analysis | ||
| uses: github/codeql-action/analyze@v3 |
`workspace` is not an input on Swatinem/rust-cache@v2; the input is `workspaces` (plural). The stray `workspace` key was silently ignored, so the Rust leg of CodeQL never cached its target directory and rebuilt the contract workspace from scratch on every run. actionlint now reports the security workflow clean. The same typo exists in ci.yml and deploy-contracts.yml and is left for those workflows to fix, since they are not part of this change. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
`npm ci` fails outright on main:
npm error `npm ci` can only install packages when your package.json and
package-lock.json are in sync.
The lockfile had drifted from package.json: it pinned @stellar/stellar-sdk
15.1.0 against a declared ^17.0.1, vitest 2.1.9 against ^3.2.7, and was
missing @playwright/test and @tanstack/react-virtual entirely. Because every
CI job that installs dependencies runs `npm ci`, this one stale file made the
whole pipeline fail at the install step.
Regenerating it moves axios 1.15.0 -> 1.20.0, which also drops the long list
of high-severity axios prototype-pollution advisories that the security
dependency audit was reporting.
`eslint-config-next` also needs `next` to be resolvable from the root
node_modules, but npm now nests `next` under frontend/node_modules (it
optionally peers on @playwright/test, which lives in the frontend workspace)
while hoisting eslint-config-next to the root. Without a root `next` the
frontend lint step could not even load its config:
Error: Cannot find module 'next/dist/compiled/babel/eslint-parser'
Declaring `next` in the root devDependencies at the same pinned version
restores the hoisted layout and makes the frontend ESLint config loadable.
This is intentionally separate from the security pipeline work: it repairs
repo-wide dependency state rather than adding scanning, and should be merged
and reviewed on its own.
🤖 Generated with Codebuff
Co-Authored-By: Codebuff <noreply@codebuff.com>
|
|
||
| - name: Rust Cache | ||
| if: matrix.language == 'rust' | ||
| uses: Swatinem/rust-cache@v2 |
…ated-security-scanning
The secret scan failed in ~10s, far too fast to have scanned any history, so the failure was in the install step rather than the scan. TruffleHog's install.sh resolves the release tag through an unauthenticated call to the GitHub releases API, which rate-limits on shared runners. This repo has already been bitten by exactly that: ci.yml documents it at the Stellar CLI install step and works around it by authenticating with the runner token. install.sh has no hook for a token, so retrying it would not help. Download the release asset from the CDN instead, which needs no API token, and verify the published SHA-256 before trusting the binary. Verified end-to-end: the checksum for trufflehog 3.97.9 verifies OK and the extracted binary reports the expected version. Also brings in the lockfile repair from fix/lockfile-out-of-sync. Without it the CodeQL javascript/typescript legs cannot get past `npm ci`, so the security workflow could not have run on this branch at all. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
The secret scan was failing in about eight seconds, far too little time to
scan any history, so it was dying during setup rather than finding anything.
The cause is that `--fail-verified` and `--only-verified` are not flags in
TruffleHog v3. They existed in the v2-era launcher documented on the
project's website; the current binary rejects them outright:
error: unknown long flag '--fail-verified', try --help
Because the abort happens before any scanning, every run failed instantly and
the job reported a secret-scan failure that had nothing to do with secrets.
v3 expresses the same intent differently: `--results=verified` selects only
credentials confirmed live against the issuing provider, and `--fail` exits
183 when any are found. Verified against the real 3.97.9 binary:
trufflehog git file://. --results=verified --fail --json
-> exit 0, verified_secrets: 0
The step now handles 0, 183 and unexpected exit codes separately so a scan
error is not reported as a leaked credential. The invalid flags are also
removed from the SECURITY.md runbook and from a stale comment in the job.
Adds the scanner's own JSON/SARIF output to .gitignore: the workflow writes
these into the workspace, and they should never be committed.
🤖 Generated with Codebuff
Co-Authored-By: Codebuff <noreply@codebuff.com>
The crate did not compile: `cargo check` alone reported 15 errors in production code, plus 27 more across test targets. The `Stream` struct grew `cliff_time`, `arbiter`, `dispute_status` and `is_allowance_based`, and the error enum was renumbered and extended, but the construction sites and call sites were never updated. This blocked `cargo check`, `cargo clippy -D warnings` and `cargo build --target wasm32-unknown-unknown`, so CodeQL could not build the Rust leg of the scan at all. Error variants: - `StreamNotActive` / `StreamStillActive` -> `StreamInactive`. Both were "stream is not in the required state" guards and `StreamInactive` is the variant that exists; no discriminant changes. - `ArithmeticOverflow` was referenced by five checked-arithmetic guards but was never present in the enum. Appended as discriminant 34 rather than slotted into the sequence, so every error already observable by clients keeps its meaning. Deleting the guards instead would have silently overflowed stream balances. `cliff_time` is set per schedule rather than blanket-defaulted: the `HybridCliffLinear` constructor carries `Some(cliff_time)`, because that schedule exists to enforce the cliff, while step-tranche, allowance and legacy-upgrade streams get `None`. Two latent bugs fixed while here: - `batch_withdraw` discarded the `Result` from `apply_withdrawal`, so an arithmetic overflow would persist the stream and emit `tokens_withdrawn` even though no tokens moved. Now propagated with `?`. - `create_allowance_stream` passed raw `&Address` where the invoke expects `Val`, and built an unused `token::Client`. Now converted via the inherent `Address::to_val()`; the direct invoke is kept because it surfaces failures as `AllowanceLocked` instead of panicking inside the SDK. `collect_fee` never used its `token_address` argument (the transfer happens in `transfer_fee`); removed rather than silenced. Acceptance tests for `batch_create_streams`, `transfer_recipient` and `extend_stream_ttl` reference contract entry points that do not exist. They are gated behind an opt-in `pending-contract-features` feature so the specification survives without blocking the build; drop each `cfg` as the matching function lands. The two cliff tests were migrated onto the real `create_hybrid_cliff_stream` rather than gated. Verified: `cargo fmt --check`, `cargo clippy --all-targets -D warnings`, `cargo check --workspace --all-targets` and the wasm release build all pass. `cargo test` still has 132 pre-existing failures (93 pass) — those encode outdated payment semantics such as `StreamNotFound` vs `Unauthorized` precedence and `None` vs `Some(0)` claimable amounts, and none of them stem from these changes. They need a domain decision and are left alone. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
`prisma generate` failed outright, which is the first step of the backend
build and of `prisma db push` in CI:
The model "IndexerDeadLetterEvent" cannot be defined because a model with
that name already exists.
The schema carried two definitions of the same model from two eras, with
different columns and different uniqueness rules:
A (old): eventId @unique, ledger, transactionHash, rawPayload
B (new): @@unique([eventId, eventType]), eventType, txHash, ledgerSequence,
cursor, payload
Two live code paths disagreed. `indexerService.ts` writes B and the admin
endpoints order and project on `ledgerSequence`/`payload`, and ADR 0003 names
`indexerService.ts` as the reference implementation and specifies quarantined
events carry "ledger, cursor, raw payload". So B is canonical and A was the
stale copy.
`SorobanEventWorker.deadLetterEvent` still wrote A. It is a private method
with no caller, but rather than delete it, it is migrated onto B so the
behaviour is preserved if it is ever wired up: the upsert keys on
`eventId_eventType` and uses `eventTypeOf` / `serializeDeadLetterPayload`,
both now exported from indexerService.
Verified: `prisma generate` and `prisma validate` pass, and none of the 74
remaining TypeScript errors in the backend are in the lines touched here.
Those 74 are pre-existing fallout from the @stellar/stellar-sdk v15 -> v17
upgrade (bigint/number arguments, `ScVal.sym`, and exports that no longer
exist such as `getPoolMetrics`, `previewReset`, `previewReplay`,
`pollTransactionStatus`), and need their own change.
🤖 Generated with Codebuff
Co-Authored-By: Codebuff <noreply@codebuff.com>
`tsc` reported 74 errors and `npm run build` could not complete: the backend
was still written against the v15 SDK while `@stellar/stellar-sdk` had moved to
17. All 74 are now cleared and the build passes.
stellar-sdk v17 models the XDR types as discriminated unions, so payloads are
plain properties on a concrete subclass rather than accessor methods:
- `ScVal.sym()/u64()/u128()` -> `.sym` / `.u64` / `.u128`, dispatched on the
`type` tag ('scvU64', ...) instead of `switch().value`.
- `TransactionEnvelope.switch()/v1()` -> a union discriminated by `.type`,
with the body on `.v1`; `Transaction.signatures()/operations()` -> properties.
- A contract call moved from `op.func.invokeContract()` to
`op.body.invokeHostFunctionOp.hostFunction.invokeContract`.
- `SorobanResources.instructions()/writeBytes()` -> properties.
Stream IDs are u64 and `lib/stream-id.ts` already returns bigint, so
`getStreamFromChain` now takes a bigint rather than risking a lossy number.
Two split-brain services:
- `services/indexer.service.ts` (dot) had zero importers and duplicated
`getIndexerStatus`/`resetIndexer`/`replayFromLedger` against the live
`indexerService.ts` (camel). Its `previewReset`/`previewReplay` were the ones
actually imported, so they move into the live module and the dead file is
deleted. The stale vitest coverage exclusion is removed with it.
- `resetIndexer` wrote its cursor without taking the worker's mutex. The worker
already implements `runExclusive` for exactly this race (LabsCrypt#1221) and the tests
assert it, but nothing called it — a poll in flight could commit its own
cursor and silently undo a reset. The write is now inside the mutex.
- `replayFromLedger` lost its correlation id, so the replay could not be traced
and the worker poll was not bound to it. Restored to the tested contract:
`(fromLedger, customRequestId?) => Promise<string>`.
Missing pieces that call sites already referenced:
- `getPoolMetrics(pool)` (total/idle/waiting) and an `overrides` parameter on
`createPgPool`, per the pg-pool tests.
- `pollTransactionStatus(hash, timeoutMs, pollIntervalMs)` was called but never
defined, leaving `getTxConfirmationTimeoutMs`/`getTxPollIntervalMs` unused.
It is bounded by default and returns the confirmed response.
- `health` referenced `indexerFailureDegraded`/`indexerLagDegraded` that did not
exist. The worker already computes a 5-minute failure spike via
`getEventCounters().degraded`, so /health now consumes it: lag and failure
spikes both drive readiness, `indexerDegraded` reports the failure signal
alone so a lag-only incident (LabsCrypt#1294) stays distinguishable from a genuinely
failing indexer (LabsCrypt#844).
- `SorobanEventWorker.deadLetterEvent` was private with no caller and failed
`noUnusedLocals`. Removed, along with the orphaned imports. The live
dead-letter path is `indexerService.quarantineEvent`.
Three call sites used a `getServer()` accessor that was never written; they now
go through the existing `executeRpc`, which already falls back to the rpc pool.
Verified: `npm run build` passes, 540 tests pass across 56 files. Test results
went from 524 passing / 32 failing in 5 files to 540 passing / 16 failing in one.
The remaining failures are `tests/integration/admin-dead-letter.test.ts`, which
returns 401 (JWT verification) where the tests expect 200/403. Verified
pre-existing by re-running them with these changes stashed.
🤖 Generated with Codebuff
Co-Authored-By: Codebuff <noreply@codebuff.com>
This branch has not been deployed
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
2 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
Closes #1334
Implemented an automated security analysis and vulnerability scanning pipeline to identify dependency vulnerabilities, exposed secrets, and common code security issues during development.
Changes
.github/workflows/security.ymlsecurity workflow.cargo audit.npm audit --audit-level=high.gitleaks.SECURITY.mdwith security scanning and reporting information.Security
Closes #1334