diff --git a/CHANGELOG.md b/CHANGELOG.md index 7d336d9..62b517e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,18 @@ All notable changes to forge will be documented in this file. Format follows [Ke ## [Unreleased] +### Fixed + +- **`forge scan security` was permanently red on real projects because of `generic-bearer` false positives.** The built-in `generic-bearer` heuristic flagged quoted 16+ character literals on test fixtures and documentation placeholders (`test_access_token`, `whsec_placeholder`, `mock-refresh-token`, `sbp_your_token_here`). On a real Next.js/Supabase repo that was 56 findings, every one a placeholder, drowning out real hits. The rule now recognises a placeholder *structurally* — a phrase of plain words that either contains a marker word (`test`, `mock`, `fake`, `dummy`, `example`, `placeholder`, `invalid`, `your`, …) or sits in test code — and stops reporting it. Opaque values (`sk_live_…`, `sk_test_…`, hex, UUIDs, base62, JWT headers, mixed-case or letter/digit blends) are still reported in production code **and** in test files, and phrase-shaped values with no marker are still reported outside tests. Only `generic-bearer` changed; the AWS, `sk-`, GitHub-token and private-key rules are untouched. Measured on the repo that produced the findings: 56 → 3, and the 3 that remain (a documented public verify token and two camelCase fixtures) are exactly the cases a heuristic should not guess. +- **The waiver registry (`.forge/waivers/`, DEV-M1-17) existed but was never consulted by the scanner**, so there was no in-tool way to accept a specific finding. `forge scan ` and the `forge ship` security checkpoint now apply waivers: matching findings are removed from the result and counted in the new `waived` field (JSON) / `waived:` line (text) and do not affect the exit code. A waiver missing `rationale`, `approved_by` or `expires_at` fails the scan instead of silently exempting findings; an expired waiver is never honoured and is named in the result `note`. +- **`waiver.Registry.IsWaived` returned "expired" as soon as it met an expired waiver, even when a valid waiver for the same rule and file followed it.** A lapsed waiver could therefore not be renewed by adding a new entry. A valid match now wins; "expired" is returned only when every matching waiver has lapsed. + +### Added + +- `ScanResult.Waived` (`"waived"` in `--json`) — number of findings suppressed by waivers. +- `docs/verbs/scan.md`: how `generic-bearer` treats placeholders, and the waiver file format. + + ## [1.10.8] — 2026-09-21 — Agent-mode arch debate no longer discards answers, `ship` stops claiming false progress, and the pre-push hook stops inheriting git's `GIT_DIR` All fixes below were found dogfooding `forge ship --agent-mode` through the spec and arch checkpoints of a real feature on a Next.js/Supabase + Python two-repo system, plus a hook bug found while pushing this very change. diff --git a/docs/verbs/scan.md b/docs/verbs/scan.md index 5bb7e91..24b06da 100644 --- a/docs/verbs/scan.md +++ b/docs/verbs/scan.md @@ -24,3 +24,47 @@ forge scan forge scan --only security forge scan --json | jq '.findings' ``` + +## Placeholders in `generic-bearer` + +The built-in `generic-bearer` rule flags a quoted literal of 16+ characters assigned to a name +containing `token`, `secret`, `password` or `api-key`. That is a heuristic, so it also matches test +fixtures (`test_access_token`, `whsec_placeholder`) and documentation (`sbp_your_token_here`). + +Since the rule learned to recognise these, a value is **not** reported when it is a *phrase* — two or +more `-`/`_` separated segments, each a plain word (all lower case, all UPPER case or Capitalised, +with at most six trailing digits) — **and** either + +- it contains a marker word (`test`, `mock`, `fake`, `dummy`, `example`, `sample`, `placeholder`, + `changeme`, `invalid`, `your`, `redacted`, `xxx`, `demo`, `fixture`, `stub`), anywhere in the tree, or +- the file is test code (a `test`/`tests`/`__tests__`/`__mocks__`/`mocks`/`e2e` directory, or a + `*.test.*`, `*.spec.*`, `*_test.*`, `test_*` or `*.mock.*` file). + +Opaque values are never excused: anything that mixes upper and lower case or letters and digits inside +one segment, or is a single long segment (live and test-mode Stripe keys, hex, UUIDs, base62, JWT +headers), is still reported in production code **and** in test files. A phrase-shaped value with no +marker (`correct-horse-battery-staple`) is still reported outside test code. Only `generic-bearer` is +affected: the AWS, `sk-`, GitHub-token and private-key-block rules fire everywhere, including tests. + +## Waivers + +Accept a specific finding with a waiver instead of leaving the gate red. Put YAML files in +`.forge/waivers/` (commit them): + +```yaml +- id: W-001 + rule_id: generic-bearer # the rule name printed next to the finding + file_path: src/lib/webhookConfig.ts # optional; omit to cover the rule in every file + rationale: >- + Public webhook verify token that customers type into their Meta app; documented as not a secret. + approved_by: alice + expires_at: "2027-03-31" # YYYY-MM-DD +``` + +- `rationale`, `approved_by` and `expires_at` are required. A waiver missing any of them makes the scan + **fail** rather than silently exempt findings. +- An expired waiver is never honoured. The finding comes back and the `note` says which waiver lapsed. + To renew, add a new entry; a valid waiver wins over an expired one that also matches. +- `file_path` is matched against the scan-relative, slash-separated path (as printed in the finding). +- Waived findings are removed from `findings`, do not affect `count`, `status` or the exit code, and are + counted in `waived` (JSON) / `waived:` (text). `forge ship`'s security checkpoint honours the same files. diff --git a/internal/cli/cmdscan/placeholder.go b/internal/cli/cmdscan/placeholder.go new file mode 100644 index 0000000..3a774af --- /dev/null +++ b/internal/cli/cmdscan/placeholder.go @@ -0,0 +1,104 @@ +// Copyright 2024 The Forge Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package cmdscan + +import ( + "regexp" + "strings" +) + +// The built-in `generic-bearer` rule is a heuristic: any quoted literal of 16+ +// [A-Za-z0-9_-] characters assigned to a name containing token/secret/password/ +// api-key. On a real project that produces a wall of findings on test fixtures +// ("test_access_token", "whsec_placeholder", "mock-refresh-token") and on +// documentation placeholders ("sbp_your_token_here"), which drowns out real hits +// and makes `forge scan security` permanently red. +// +// isPlaceholderCredential recognises those literals structurally rather than by +// silencing whole directories, so a real secret that happens to sit in a test +// file is still reported: +// +// - A value is a candidate only if it is a *phrase*: two or more segments split +// on '-' or '_', and every segment is a plain word (all lower case, all upper +// case, or Capitalised) optionally followed by up to six digits — or a short +// run of digits. Random credentials are opaque: they mix upper and lower +// case, letters and digits inside one segment, or are a single long segment, +// so they never qualify. That is why live and test-mode Stripe keys, real +// webhook secrets, hex, +// UUIDs, base62 and JWT-ish strings are still flagged. +// - A phrase is then treated as a placeholder only when something says it is +// not real: it contains an explicit marker word (test, mock, fake, dummy, +// example, sample, placeholder, invalid, your, …) OR it sits in a test path. +// A phrase-shaped value in production code with no marker +// ("correct-horse-battery-staple") is still reported. +// +// Provider-specific rules (AWS keys, sk- keys, GitHub tokens, private-key +// blocks) are not affected: they are checked independently of this heuristic. + +// placeholderMarkers are segment words that state, in the value itself, that it +// is not a real credential. +var placeholderMarkers = map[string]struct{}{ + "test": {}, "mock": {}, "fake": {}, "dummy": {}, "example": {}, "sample": {}, + "placeholder": {}, "changeme": {}, "invalid": {}, "your": {}, "redacted": {}, + "xxx": {}, "demo": {}, "fixture": {}, "stub": {}, +} + +// plainSegment matches one word of a phrase: lower, UPPER or Capitalised +// letters with at most six trailing digits, or a bare run of up to six digits. +// Mixed-case blends such as "qWeRtY" and letter/digit blends such as "a1b2c3" +// do not match, which is what keeps opaque credentials out. +var plainSegment = regexp.MustCompile(`^(?:[a-z]+|[A-Z]+|[A-Z][a-z]+)[0-9]{0,6}$|^[0-9]{1,6}$`) + +// testPathDirs are directory names that mark a path as test code. +var testPathDirs = map[string]struct{}{ + "test": {}, "tests": {}, "__tests__": {}, "__mocks__": {}, "mocks": {}, + "e2e": {}, "__fixtures__": {}, +} + +// isTestPath reports whether rel (slash-separated, relative to the scan root) +// is test code, by directory name or by conventional file-name pattern. +func isTestPath(rel string) bool { + parts := strings.Split(strings.ToLower(rel), "/") + for _, dir := range parts[:len(parts)-1] { + if _, ok := testPathDirs[dir]; ok { + return true + } + } + base := parts[len(parts)-1] + return strings.Contains(base, ".test.") || strings.Contains(base, ".spec.") || + strings.Contains(base, "_test.") || strings.HasPrefix(base, "test_") || + strings.Contains(base, ".mock.") +} + +// isPlaceholderCredential reports whether a quoted literal matched by the +// generic-bearer heuristic is recognisably not a real credential. See the +// package comment above for the exact rule and its limits. +func isPlaceholderCredential(rel, value string) bool { + segments := strings.FieldsFunc(value, func(r rune) bool { return r == '-' || r == '_' }) + if len(segments) < 2 { + return false + } + marked := false + for _, seg := range segments { + if !plainSegment.MatchString(seg) { + return false + } + word := strings.ToLower(strings.TrimRight(seg, "0123456789")) + if _, ok := placeholderMarkers[word]; ok { + marked = true + } + } + return marked || isTestPath(rel) +} diff --git a/internal/cli/cmdscan/placeholder_test.go b/internal/cli/cmdscan/placeholder_test.go new file mode 100644 index 0000000..cb169e8 --- /dev/null +++ b/internal/cli/cmdscan/placeholder_test.go @@ -0,0 +1,149 @@ +// Copyright 2024 The Forge Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package cmdscan + +import "testing" + +// Values below are taken from a real project's scan output (56 generic-bearer +// findings, all fixtures/docs) so the rule is pinned to real data, not to +// examples invented to fit it. +func TestIsPlaceholderCredential_RealWorldPlaceholders(t *testing.T) { + t.Parallel() + cases := []struct { + name, rel, value string + }{ + // explicit marker word, anywhere in the tree + {"mock in test", "tests/unit/a.test.ts", "mock-refresh-token"}, + {"mock in setup file", "jest.setup.js", "mock-refresh-token"}, + {"test marker in prod code", "src/lib/oauth/google.ts", "promotiai-test-connection-probe-invalid-token"}, + {"your in docs", "docs/QUICK_APPLY_GUIDE.md", "sbp_your_token_here"}, + {"YOUR upper case in script", "scripts/restart.ps1", "YOUR_ACCESS_TOKEN"}, + {"placeholder word", "src/x.ts", "whsec_placeholder_value"}, + {"invalid marker", "src/x.ts", "invalid_refresh_token"}, + {"do-not-use", "src/webhook.test.ts", "test-secret-do-not-use-in-prod"}, + // no marker, but plain words in test code + {"phrase in tests dir", "tests/integration/oauth.test.js", "duplicate_secret"}, + {"trailing digits are plain", "tests/unit/oauth.test.ts", "invalid_token_12345"}, + {"digit suffix on a word", "tests/unit/tw.test.ts", "tw-oauth1-access-token"}, + {"new_access_token", "src/test/refresh.test.ts", "new_access_token"}, + {"file-name pattern only", "src/lib/refresh.spec.ts", "brand_new_token_abc123"}, + {"__tests__ dir", "src/__tests__/a.ts", "my_super_secret_value_here"}, + } + for _, c := range cases { + c := c + t.Run(c.name, func(t *testing.T) { + t.Parallel() + if !isPlaceholderCredential(c.rel, c.value) { + t.Fatalf("isPlaceholderCredential(%q, %q) = false; want true", c.rel, c.value) + } + }) + } +} + +// The important half: anything that looks like a real credential must keep +// being reported, in production code AND in test files. +func TestIsPlaceholderCredential_RealSecretsStillFlagged(t *testing.T) { + t.Parallel() + // Secret-shaped values are assembled from fragments so no source line contains a + // contiguous provider key: GitHub push protection (rightly) cannot tell a fixture + // from a leak, and blocked the first push of this file. + secrets := map[string]string{ + "stripe live key": "sk_" + "live_51HabcDEFghiJKLmnoPQRstu", + "stripe test key": "sk_" + "test_51HabcDEFghiJKLmnoPQRstu", // test-MODE keys are still secrets + "random base62": "q8Zr3KfL0wXv9TbNc2YpHs7D", + "hex": "3f2a9c1e7b4d40a8b6c5d2e1f0a9b8c7", + "uuid": "3f2a9c1e-7b4d-40a8-b6c5-d2e1f0a9b8c7", + "jwt header": "eyJhbGciOiJIUzI1NiIsInR5cCI", + "real whsec": "whsec_" + "Xk3Lm9Qw2Ert7Yu1Io5PaSd4Fg", + "mixed case blend": "qWeRtY_asDfGh_zXcVbN", + "letter-digit blend": "live-tok-a1b2c3d4e5f6g7h8", + "marker but opaque part": "test_Xk3Lm9Qw2Ert7Yu1Io5PaSd4Fg", + "single long lower word": "abcdefghijklmnopqrstuvwxyz", + "camelCase single token": "xClientSecret123abc456def789ghi", + } + for name, v := range secrets { + name, v := name, v + for _, rel := range []string{"src/lib/auth.ts", "tests/unit/auth.test.ts"} { + rel := rel + t.Run(name+" @ "+rel, func(t *testing.T) { + t.Parallel() + if isPlaceholderCredential(rel, v) { + t.Fatalf("isPlaceholderCredential(%q, %q) = true; a real-looking secret must still be reported", rel, v) + } + }) + } + } +} + +// A phrase-shaped value with no marker is only excused inside test code. The same +// literal in production code is still a plausible hard-coded password. +func TestIsPlaceholderCredential_PhraseInProductionStillFlagged(t *testing.T) { + t.Parallel() + for _, v := range []string{"correct-horse-battery-staple", "my-secret-password-2024", "promotiai-social-inbox-webhook"} { + if isPlaceholderCredential("src/lib/auth.ts", v) { + t.Errorf("%q in production code must still be reported", v) + } + if !isPlaceholderCredential("tests/unit/auth.test.ts", v) { + t.Errorf("%q in a test file should be treated as a fixture", v) + } + } +} + +func TestIsPlaceholderCredential_Boundaries(t *testing.T) { + t.Parallel() + cases := []struct { + value string + want bool + }{ + {"", false}, + {"testtesttesttesttest", false}, // marker, but one segment: not phrase-shaped + {"test_", false}, // one segment after splitting + {"_test_mock_", true}, // leading/trailing separators are ignored + {"test__mock--fake", true}, // repeated separators collapse + {"test_1234567", false}, // 7 digits is not a short number + {"test_123456", true}, // 6 digits is + {"Test_Mock_Token", true}, // Capitalised words + {"TEST_MOCK_TOKEN", true}, // UPPER words + {"tEsT_mock_token", false}, // mixed-case blend + } + for _, c := range cases { + if got := isPlaceholderCredential("src/x.ts", c.value); got != c.want { + t.Errorf("isPlaceholderCredential(src/x.ts, %q) = %v; want %v", c.value, got, c.want) + } + } +} + +func TestIsTestPath(t *testing.T) { + t.Parallel() + yes := []string{ + "tests/a.js", "test/a.js", "src/__tests__/a.ts", "src/__mocks__/x.ts", "e2e/login.ts", + "src/lib/a.test.ts", "src/lib/a.spec.js", "pkg/a_test.go", "scripts/test_helpers.py", "src/api.mock.ts", + "TESTS/A.JS", // case-insensitive + } + no := []string{ + "src/lib/a.ts", "src/contest/a.ts", "src/latest/a.ts", "docs/spec/a.md", "src/attestation.ts", + "src/lib/testify.ts", "a.ts", + } + for _, p := range yes { + if !isTestPath(p) { + t.Errorf("isTestPath(%q) = false; want true", p) + } + } + for _, p := range no { + if isTestPath(p) { + t.Errorf("isTestPath(%q) = true; want false", p) + } + } +} diff --git a/internal/cli/cmdscan/scan.go b/internal/cli/cmdscan/scan.go index ef98f62..3e5fa3a 100644 --- a/internal/cli/cmdscan/scan.go +++ b/internal/cli/cmdscan/scan.go @@ -54,6 +54,9 @@ type ScanResult struct { Count int `json:"count"` Status string `json:"status"` // "clean", "suspicious", "found" Note string `json:"note,omitempty"` + // Waived counts findings suppressed by a valid .forge/waivers entry. They are + // removed from Findings and do not affect Count, Status or the exit code. + Waived int `json:"waived,omitempty"` } func init() { @@ -176,6 +179,11 @@ func New() *cobra.Command { // G-022: assign confidence scores. res.Findings = AssignConfidence(res.Findings) + // DEV-M1-17: drop findings covered by a valid, unexpired .forge/waivers entry. + if err := ApplyWaivers(root, res); err != nil { + return err + } + // G-023: --since diff against baseline. if since != "" { baseline := loadScanBaseline(root, scanner) @@ -769,12 +777,20 @@ func scanWithBuiltinPatterns(root string) []Finding { {"private-key-block", regexp.MustCompile(`-----BEGIN [A-Z ]*PRIVATE KEY-----`)}, // generic-bearer: require the value to be a quoted string literal so that // variable-name references (e.g. token = csrfTokenVar) are not flagged. - {"generic-bearer", regexp.MustCompile(`(?i)(bearer|api[_-]?key|token|secret|password)\s*[:=]\s*["'][A-Za-z0-9_\-]{16,}["']`)}, + // The value is captured (group 2) so recognisable placeholders can be + // dropped — see isPlaceholderCredential. + {"generic-bearer", regexp.MustCompile(`(?i)(bearer|api[_-]?key|token|secret|password)\s*[:=]\s*["']([A-Za-z0-9_\-]{16,})["']`)}, } return scanFiles(root, func(rel string, line int, text string) []Finding { var out []Finding for _, r := range rules { - if loc := r.Pattern.FindStringIndex(text); loc != nil { + if loc := r.Pattern.FindStringSubmatchIndex(text); loc != nil { + // A phrase-shaped literal that says it is not real (marker word) or that + // lives in test code is a fixture/doc placeholder, not a leaked secret. + if r.Name == "generic-bearer" && len(loc) >= 6 && + isPlaceholderCredential(rel, text[loc[4]:loc[5]]) { + continue + } out = append(out, Finding{ File: rel, Line: line, Rule: r.Name, Match: truncate(text, 80), Secret: text[loc[0]:loc[1]], @@ -1088,6 +1104,9 @@ func renderText(cmd *cobra.Command, r *ScanResult) { fmt.Fprintf(w, "forge scan\n") fmt.Fprintf(w, "findings: %d\n", r.Count) fmt.Fprintf(w, "status: %s\n", r.Status) + if r.Waived > 0 { + fmt.Fprintf(w, "waived: %d\n", r.Waived) + } if r.Note != "" { fmt.Fprintf(w, "note: %s\n", r.Note) } diff --git a/internal/cli/cmdscan/scanners_placeholder_test.go b/internal/cli/cmdscan/scanners_placeholder_test.go new file mode 100644 index 0000000..5f79c0b --- /dev/null +++ b/internal/cli/cmdscan/scanners_placeholder_test.go @@ -0,0 +1,128 @@ +// Copyright 2024 The Forge Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package cmdscan + +import ( + "strings" + "testing" +) + +func genericBearerFiles(res *ScanResult) []string { + var out []string + for _, f := range res.Findings { + if f.Rule == "generic-bearer" { + out = append(out, f.File) + } + } + return out +} + +// TC-FP-07: fixture and documentation placeholders must NOT trigger generic-bearer. +// On a real project these were 56 findings that kept `forge scan security` red. +func TestRunSecrets_GenericBearerPlaceholders_NoFinding(t *testing.T) { + t.Parallel() + if hasGitleaks() { + t.Skip("gitleaks installed; built-in patterns not exercised") + } + root := t.TempDir() + writeFile(t, root, "tests/unit/oauth.test.ts", + "const a = { access_token: 'test_access_token' };\n"+ + "process.env.STRIPE_WEBHOOK_SECRET = 'whsec_placeholder';\n"+ + "const b = { password: 'test-password-12345' };\n") + writeFile(t, root, "jest.setup.js", "const x = { refresh_token: 'mock-refresh-token' };\n") + writeFile(t, root, "docs/GUIDE.md", "export SUPABASE_ACCESS_TOKEN='sbp_your_token_here'\n") + writeFile(t, root, "scripts/restart.ps1", `$headers = @{ token = "YOUR_ACCESS_TOKEN" }`+"\n") + writeFile(t, root, "src/lib/probe.ts", "refresh_token: 'promotiai-test-connection-probe-invalid-token',\n") + + res, err := RunSecrets(root) + if err != nil { + t.Fatalf("RunSecrets: %v", err) + } + if got := genericBearerFiles(res); len(got) != 0 { + t.Fatalf("placeholders must not be reported; got findings in: %v (%+v)", got, res.Findings) + } +} + +// TC-FP-08: a real-looking secret is still reported even when it sits in a test +// file, and a phrase-shaped hard-coded value is still reported in production code. +func TestRunSecrets_GenericBearerRealSecretsStillFlagged(t *testing.T) { + t.Parallel() + if hasGitleaks() { + t.Skip("gitleaks installed; built-in patterns not exercised") + } + root := t.TempDir() + writeFile(t, root, "tests/unit/leak.test.ts", `const key = { secret: "q8Zr3KfL0wXv9TbNc2YpHs7D" };`+"\n") + writeFile(t, root, "tests/unit/stripe.test.ts", `const k = { api_key: "sk_`+"test_51HabcDEFghiJKLmnoPQRstu"+`" };`+"\n") + writeFile(t, root, "src/lib/auth.ts", `const cfg = { password: "correct-horse-battery-staple" };`+"\n") + + res, err := RunSecrets(root) + if err != nil { + t.Fatalf("RunSecrets: %v", err) + } + got := strings.Join(genericBearerFiles(res), ",") + for _, want := range []string{"tests/unit/leak.test.ts", "tests/unit/stripe.test.ts", "src/lib/auth.ts"} { + if !strings.Contains(got, want) { + t.Errorf("expected a generic-bearer finding in %s; got: %s", want, got) + } + } +} + +// TC-FP-09: the placeholder heuristic applies to generic-bearer ONLY. Provider-specific +// rules keep firing inside test files. +func TestRunSecrets_ProviderRulesUnaffectedInTestFiles(t *testing.T) { + t.Parallel() + if hasGitleaks() { + t.Skip("gitleaks installed; built-in patterns not exercised") + } + root := t.TempDir() + writeFile(t, root, "tests/unit/aws.test.ts", "const k = 'AKIAIOSFODNN7EXAMPLE';\n") + writeFile(t, root, "tests/unit/pk.test.ts", "-----BEGIN RSA PRIVATE KEY-----\n") + writeFile(t, root, "tests/unit/gh.test.ts", "const t = 'ghp_"+strings.Repeat("a1B2", 8)+"';\n") + + res, err := RunSecrets(root) + if err != nil { + t.Fatalf("RunSecrets: %v", err) + } + rules := map[string]bool{} + for _, f := range res.Findings { + rules[f.Rule] = true + } + for _, want := range []string{"aws-access-key", "private-key-block", "github-token"} { + if !rules[want] { + t.Errorf("rule %s must still fire in test files; findings: %+v", want, res.Findings) + } + } +} + +// The finding's Secret field must be the same text as before the change so +// existing baselines, waivers and JSON consumers keep working. +func TestRunSecrets_GenericBearerSecretFieldUnchanged(t *testing.T) { + t.Parallel() + if hasGitleaks() { + t.Skip("gitleaks installed; built-in patterns not exercised") + } + root := t.TempDir() + writeFile(t, root, "src/config.go", `token = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9_abcXYZ0123"`+"\n") + res, err := RunSecrets(root) + if err != nil { + t.Fatalf("RunSecrets: %v", err) + } + if len(res.Findings) != 1 { + t.Fatalf("want exactly 1 finding, got %+v", res.Findings) + } + if want := `token = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9_abcXYZ0123"`; res.Findings[0].Secret != want { + t.Fatalf("Secret = %q; want the full matched text %q", res.Findings[0].Secret, want) + } +} diff --git a/internal/cli/cmdscan/waivers.go b/internal/cli/cmdscan/waivers.go new file mode 100644 index 0000000..626d7fe --- /dev/null +++ b/internal/cli/cmdscan/waivers.go @@ -0,0 +1,130 @@ +// Copyright 2024 The Forge Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package cmdscan + +import ( + "errors" + "fmt" + "path/filepath" + "sort" + "strings" + + "github.com/teragrid/forge/internal/errcode" + "github.com/teragrid/forge/internal/waiver" +) + +// ApplyWaivers removes from res every finding covered by a valid waiver under +// /.forge/waivers/ and records how many were waived in res.Waived. +// +// Until now the waiver registry (internal/waiver, DEV-M1-17) was loaded and +// tested but never consulted by the scanner, so a repository had no in-tool way +// to accept a specific finding — the only options were to edit the offending +// code or to live with a permanently red gate. +// +// Rules, in the order they are enforced: +// +// 1. A waiver must state why (rationale), who approved it (approved_by) and +// when it lapses (expires_at). A waiver missing any of them is an error, not +// a silent no-op: the scan fails closed so an unreviewable exemption cannot +// hide findings. +// 2. A finding matching an expired waiver is NOT suppressed, and the expiry is +// reported in res.Note so it is visible why the finding came back. +// 3. A waiver with no file_path covers its rule everywhere; with file_path it +// covers that one file (matched on the scan-relative, slash-separated path). +// +// With no waivers directory this is a no-op. +func ApplyWaivers(root string, res *ScanResult) error { + reg, err := waiver.LoadDefault(root) + if err != nil { + return errcode.New(ErrScanFailed, "load waivers", err) + } + if len(reg.All()) == 0 { + return nil + } + if err := validateWaivers(reg.All()); err != nil { + return errcode.New(ErrScanFailed, "invalid waiver", err) + } + + kept := make([]Finding, 0, len(res.Findings)) + expired := map[string]struct{}{} + for _, f := range res.Findings { + ok, werr := reg.IsWaived(f.Rule, filepath.ToSlash(f.File)) + switch { + case werr != nil && errors.Is(werr, waiver.ErrWaiverExpired): + expired[werr.Error()] = struct{}{} + kept = append(kept, f) + case werr != nil: + return errcode.New(ErrScanFailed, "evaluate waiver", werr) + case ok: + res.Waived++ + default: + kept = append(kept, f) + } + } + res.Findings = kept + finalizeStatus(res) + + if res.Waived > 0 { + res.Note = joinNote(res.Note, fmt.Sprintf("%d finding(s) waived by .forge/waivers", res.Waived)) + } + if len(expired) > 0 { + msgs := make([]string, 0, len(expired)) + for m := range expired { + msgs = append(msgs, m) + } + sort.Strings(msgs) + res.Note = joinNote(res.Note, "expired waiver(s) NOT honoured: "+strings.Join(msgs, "; ")) + } + return nil +} + +// validateWaivers rejects waivers that omit the fields the waiver package +// documents as required. It reports every problem at once. +func validateWaivers(specs []waiver.WaiverSpec) error { + var problems []string + for i, w := range specs { + id := w.ID + if id == "" { + id = fmt.Sprintf("#%d", i+1) + } + var missing []string + if w.RuleID == "" { + missing = append(missing, "rule_id") + } + if strings.TrimSpace(w.Rationale) == "" { + missing = append(missing, "rationale") + } + if strings.TrimSpace(w.ApprovedBy) == "" { + missing = append(missing, "approved_by") + } + if strings.TrimSpace(w.ExpiresAt) == "" { + missing = append(missing, "expires_at") + } + if len(missing) > 0 { + problems = append(problems, fmt.Sprintf("waiver %s is missing %s", id, strings.Join(missing, ", "))) + } + } + if len(problems) > 0 { + return errors.New(strings.Join(problems, "; ")) + } + return nil +} + +func joinNote(existing, add string) string { + if existing == "" { + return add + } + return existing + " | " + add +} diff --git a/internal/cli/cmdscan/waivers_test.go b/internal/cli/cmdscan/waivers_test.go new file mode 100644 index 0000000..b4f9aa4 --- /dev/null +++ b/internal/cli/cmdscan/waivers_test.go @@ -0,0 +1,256 @@ +// Copyright 2024 The Forge Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package cmdscan + +import ( + "bytes" + "encoding/json" + "strings" + "testing" + "time" +) + +func future() string { return time.Now().AddDate(0, 1, 0).Format("2006-01-02") } +func past() string { return time.Now().AddDate(0, 0, -3).Format("2006-01-02") } + +func findingsFixture() *ScanResult { + r := &ScanResult{Findings: []Finding{ + {File: "src/lib/webhookConfig.ts", Line: 17, Rule: "generic-bearer"}, + {File: "tests/integration/x.test.js", Line: 4, Rule: "generic-bearer"}, + {File: "src/lib/webhookConfig.ts", Line: 30, Rule: "aws-access-key"}, + }} + finalizeStatus(r) + return r +} + +func TestApplyWaivers_NoDirIsNoop(t *testing.T) { + t.Parallel() + res := findingsFixture() + if err := ApplyWaivers(t.TempDir(), res); err != nil { + t.Fatalf("ApplyWaivers: %v", err) + } + if len(res.Findings) != 3 || res.Waived != 0 || res.Note != "" { + t.Fatalf("no waivers dir must change nothing; got %+v", res) + } +} + +func TestApplyWaivers_FileScopedWaiverRemovesOnlyThatFileAndRule(t *testing.T) { + t.Parallel() + root := t.TempDir() + writeFile(t, root, ".forge/waivers/w.yml", ` +- id: W-001 + rule_id: generic-bearer + file_path: src/lib/webhookConfig.ts + rationale: "Public Meta webhook verify token; documented as not a secret." + approved_by: trung + expires_at: "`+future()+`" +`) + res := findingsFixture() + if err := ApplyWaivers(root, res); err != nil { + t.Fatalf("ApplyWaivers: %v", err) + } + if res.Waived != 1 || len(res.Findings) != 2 || res.Count != 2 { + t.Fatalf("want 1 waived and 2 left; got waived=%d findings=%+v", res.Waived, res.Findings) + } + for _, f := range res.Findings { + if f.File == "src/lib/webhookConfig.ts" && f.Rule == "generic-bearer" { + t.Fatalf("waived finding still present: %+v", f) + } + } + // a DIFFERENT rule in the same file must not be waived + stillThere := false + for _, f := range res.Findings { + if f.Rule == "aws-access-key" { + stillThere = true + } + } + if !stillThere { + t.Fatal("waiver for generic-bearer must not suppress aws-access-key in the same file") + } + if !strings.Contains(res.Note, "1 finding(s) waived") { + t.Fatalf("note should say how many were waived; got %q", res.Note) + } +} + +func TestApplyWaivers_RuleWideWaiver(t *testing.T) { + t.Parallel() + root := t.TempDir() + writeFile(t, root, ".forge/waivers/w.yml", ` +id: W-002 +rule_id: generic-bearer +rationale: "accepted repo-wide while migrating secrets to a vault" +approved_by: trung +expires_at: "`+future()+`" +`) + res := findingsFixture() + if err := ApplyWaivers(root, res); err != nil { + t.Fatalf("ApplyWaivers: %v", err) + } + if res.Waived != 2 || len(res.Findings) != 1 || res.Findings[0].Rule != "aws-access-key" { + t.Fatalf("rule-wide waiver should remove both generic-bearer findings only; got waived=%d %+v", res.Waived, res.Findings) + } + if res.Status != "suspicious" { + t.Fatalf("status should be recomputed for 1 finding; got %q", res.Status) + } +} + +func TestApplyWaivers_ExpiredWaiverIsNotHonoured(t *testing.T) { + t.Parallel() + root := t.TempDir() + writeFile(t, root, ".forge/waivers/w.yml", ` +- id: W-OLD + rule_id: generic-bearer + file_path: src/lib/webhookConfig.ts + rationale: "temporary" + approved_by: trung + expires_at: "`+past()+`" +`) + res := findingsFixture() + if err := ApplyWaivers(root, res); err != nil { + t.Fatalf("ApplyWaivers: %v", err) + } + if res.Waived != 0 || len(res.Findings) != 3 { + t.Fatalf("expired waiver must not suppress anything; got waived=%d findings=%d", res.Waived, len(res.Findings)) + } + if !strings.Contains(res.Note, "expired waiver") || !strings.Contains(res.Note, "W-OLD") { + t.Fatalf("expiry must be visible in the note; got %q", res.Note) + } +} + +// Renewing a waiver by adding a new entry (without deleting the lapsed one) must work. +func TestApplyWaivers_ValidWaiverBeatsExpiredDuplicate(t *testing.T) { + t.Parallel() + root := t.TempDir() + writeFile(t, root, ".forge/waivers/a_old.yml", ` +- id: W-OLD + rule_id: generic-bearer + file_path: src/lib/webhookConfig.ts + rationale: "first approval" + approved_by: trung + expires_at: "`+past()+`" +`) + writeFile(t, root, ".forge/waivers/b_renewed.yml", ` +- id: W-NEW + rule_id: generic-bearer + file_path: src/lib/webhookConfig.ts + rationale: "re-approved after review" + approved_by: trung + expires_at: "`+future()+`" +`) + res := findingsFixture() + if err := ApplyWaivers(root, res); err != nil { + t.Fatalf("ApplyWaivers: %v", err) + } + if res.Waived != 1 || strings.Contains(res.Note, "expired") { + t.Fatalf("renewed waiver should apply cleanly; got waived=%d note=%q", res.Waived, res.Note) + } +} + +// Fail closed: a waiver without a reason, approver or expiry is an error, not a +// silent exemption. +func TestApplyWaivers_IncompleteWaiverFailsClosed(t *testing.T) { + t.Parallel() + cases := map[string]string{ + "no rationale": "id: W-1\nrule_id: generic-bearer\napproved_by: t\nexpires_at: \"" + future() + "\"\n", + "no approved_by": "id: W-2\nrule_id: generic-bearer\nrationale: r\nexpires_at: \"" + future() + "\"\n", + "no expires_at": "id: W-3\nrule_id: generic-bearer\nrationale: r\napproved_by: t\n", + "blank rationale": "id: W-4\nrule_id: generic-bearer\nrationale: \" \"\napproved_by: t\nexpires_at: \"" + future() + "\"\n", + } + for name, body := range cases { + name, body := name, body + t.Run(name, func(t *testing.T) { + t.Parallel() + root := t.TempDir() + writeFile(t, root, ".forge/waivers/w.yml", body) + res := findingsFixture() + err := ApplyWaivers(root, res) + if err == nil { + t.Fatal("expected an error for an incomplete waiver") + } + if len(res.Findings) != 3 || res.Waived != 0 { + t.Fatalf("an invalid waiver must suppress nothing; got waived=%d findings=%d", res.Waived, len(res.Findings)) + } + }) + } +} + +func TestApplyWaivers_MalformedYAMLIsAnError(t *testing.T) { + t.Parallel() + root := t.TempDir() + writeFile(t, root, ".forge/waivers/w.yml", "id: [unterminated\n") + if err := ApplyWaivers(root, findingsFixture()); err == nil { + t.Fatal("expected an error for malformed waiver YAML") + } +} + +// CLI level: what CI actually runs. Same tree, exit code flips only because of the waiver. +func TestScanCommand_WaiverTurnsGateGreen_AndJSONReportsWaived(t *testing.T) { + t.Parallel() + if hasGitleaks() { + t.Skip("gitleaks installed; built-in patterns not exercised") + } + root := t.TempDir() + writeFile(t, root, "src/lib/webhookConfig.ts", + "export const META_WEBHOOK_VERIFY_TOKEN = 'promotiai-social-inbox-webhook';\n") + + run := func() (string, error) { + cmd := New() + var out bytes.Buffer + cmd.SetOut(&out) + cmd.SetErr(&out) + cmd.SetArgs([]string{"secrets", "--root", root, "--json"}) + err := cmd.Execute() + return out.String(), err + } + + if _, err := run(); err == nil { + t.Fatal("precondition: the un-waived finding must fail the gate") + } + + writeFile(t, root, ".forge/waivers/webhook.yml", ` +id: W-WEBHOOK +rule_id: generic-bearer +file_path: src/lib/webhookConfig.ts +rationale: "Public Meta webhook verify token; the file documents it is not a secret." +approved_by: trung +expires_at: "`+future()+`" +`) + out, err := run() + if err != nil { + t.Fatalf("waived finding must not fail the gate: %v\n%s", err, out) + } + var got ScanResult + if jerr := json.Unmarshal([]byte(out), &got); jerr != nil { + t.Fatalf("json: %v\n%s", jerr, out) + } + if got.Waived != 1 || got.Count != 0 || got.Status != "clean" { + t.Fatalf("want waived=1 count=0 clean; got %+v", got) + } +} + +func TestScanCommand_InvalidWaiverFailsTheScan(t *testing.T) { + t.Parallel() + root := t.TempDir() + writeFile(t, root, "src/a.go", "package a\n") + writeFile(t, root, ".forge/waivers/w.yml", "id: W-X\nrule_id: generic-bearer\n") + cmd := New() + var out bytes.Buffer + cmd.SetOut(&out) + cmd.SetErr(&out) + cmd.SetArgs([]string{"secrets", "--root", root}) + if err := cmd.Execute(); err == nil { + t.Fatalf("a scan with an incomplete waiver must fail closed; output: %s", out.String()) + } +} diff --git a/internal/cli/cmdship/ship.go b/internal/cli/cmdship/ship.go index 0b2fdc1..f0e0e2f 100644 --- a/internal/cli/cmdship/ship.go +++ b/internal/cli/cmdship/ship.go @@ -1770,6 +1770,13 @@ func checkVerify(root, description, specName string, pipe *LLMPipe) Checkpoint { return cp } scanRes.Findings = cmdscan.AssignConfidence(scanRes.Findings) + // Honour .forge/waivers here too, so the ship gate and `forge scan security` + // agree about which findings are accepted. + if wErr := cmdscan.ApplyWaivers(root, scanRes); wErr != nil { + cp.Status = "warning" + cp.Detail = fmt.Sprintf("security scan waivers error: %v", wErr) + return cp + } var highFindings []cmdscan.Finding for _, f := range scanRes.Findings { if f.Confidence == string(cmdscan.ConfidenceHigh) { diff --git a/internal/waiver/waiver.go b/internal/waiver/waiver.go index 90ce8e7..f4792e5 100644 --- a/internal/waiver/waiver.go +++ b/internal/waiver/waiver.go @@ -131,9 +131,12 @@ func LoadDefault(root string) (*Registry, error) { // a valid, non-expired waiver. // // filePath may be empty to match any file. -// Returns ErrWaiverExpired if a matching waiver was found but is expired -// (caller should surface this as an error, not silently pass the finding). +// Returns ErrWaiverExpired if matching waivers were found but every one of them +// is expired (caller should surface this as an error, not silently pass the +// finding). A valid waiver wins over an expired one that also matches, so a +// lapsed waiver can be renewed by adding a new entry without deleting the old. func (r *Registry) IsWaived(ruleID, filePath string) (bool, error) { + var expired error for _, w := range r.waivers { if w.RuleID != ruleID { continue @@ -143,10 +146,16 @@ func (r *Registry) IsWaived(ruleID, filePath string) (bool, error) { continue } if w.Expired() { - return false, fmt.Errorf("%w: rule=%s id=%s expires=%s", ErrWaiverExpired, ruleID, w.ID, w.ExpiresAt) + if expired == nil { + expired = fmt.Errorf("%w: rule=%s id=%s expires=%s", ErrWaiverExpired, ruleID, w.ID, w.ExpiresAt) + } + continue } return true, nil } + if expired != nil { + return false, expired + } return false, nil } diff --git a/internal/waiver/waiver_test.go b/internal/waiver/waiver_test.go index f8bfb79..f44e534 100644 --- a/internal/waiver/waiver_test.go +++ b/internal/waiver/waiver_test.go @@ -147,3 +147,33 @@ func TestMissingDirIsOK(t *testing.T) { t.Fatal("expected non-nil registry for missing dir") } } + +// A valid waiver must win over an expired one that also matches, so a lapsed +// waiver can be renewed by adding a new entry. +func TestIsWaived_ValidBeatsExpiredDuplicate(t *testing.T) { + dir := t.TempDir() + yesterday := time.Now().AddDate(0, 0, -2).Format("2006-01-02") + tomorrow := time.Now().AddDate(0, 0, 2).Format("2006-01-02") + writeWaiver(t, dir, "a_old.yml", ` +- id: W-OLD + rule_id: SEC-001 + rationale: "first approval" + approved_by: bob + expires_at: "`+yesterday+`" +`) + writeWaiver(t, dir, "b_new.yml", ` +- id: W-NEW + rule_id: SEC-001 + rationale: "renewed" + approved_by: bob + expires_at: "`+tomorrow+`" +`) + r, err := Load(dir) + if err != nil { + t.Fatal(err) + } + ok, err := r.IsWaived("SEC-001", "") + if err != nil || !ok { + t.Fatalf("valid waiver should win over an expired duplicate: ok=%v err=%v", ok, err) + } +}