From a54ac42604add89480527f97849cf6a30cf042ed Mon Sep 17 00:00:00 2001 From: VietKing Date: Mon, 21 Sep 2026 14:16:32 +0700 Subject: [PATCH 1/3] fix(scan): honour .forge/waivers in forge scan and the ship security checkpoint The waiver registry (internal/waiver, DEV-M1-17) was loaded and unit-tested but never called by the scanner, so a repository had no in-tool way to accept a specific finding: the only options were editing the code or a permanently red gate. ApplyWaivers now runs after confidence assignment in `forge scan ` and in the ship checkpoint's security scan. - Matching findings are removed from the result and counted in the new ScanResult.Waived (`waived` in --json, a `waived:` line in text). They do not affect count, status or the exit code. - Fail closed: a waiver missing rationale, approved_by or expires_at is an error, not a silent exemption. - An expired waiver is never honoured; the finding stays and the result note names the lapsed waiver. - IsWaived returned "expired" at the first expired match even when a valid waiver for the same rule and file followed, so a lapsed waiver could not be renewed by adding a new entry. A valid match now wins. Tests cover file-scoped and rule-wide waivers, expiry, renewal, incomplete and malformed waivers, and the CLI exit code flipping only because of the waiver. Mutation-checked: dropping the ApplyWaivers call or reverting the IsWaived fix fails the tests. Co-Authored-By: Claude Sonnet 5 Signed-off-by: VietKing --- internal/cli/cmdscan/scan.go | 11 ++ internal/cli/cmdscan/waivers.go | 130 ++++++++++++++ internal/cli/cmdscan/waivers_test.go | 256 +++++++++++++++++++++++++++ internal/cli/cmdship/ship.go | 7 + internal/waiver/waiver.go | 15 +- internal/waiver/waiver_test.go | 30 ++++ 6 files changed, 446 insertions(+), 3 deletions(-) create mode 100644 internal/cli/cmdscan/waivers.go create mode 100644 internal/cli/cmdscan/waivers_test.go diff --git a/internal/cli/cmdscan/scan.go b/internal/cli/cmdscan/scan.go index ef98f62..cf0517c 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) @@ -1088,6 +1096,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/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) + } +} From f7a37f0aa388e8954646a4a9cbc8b513d6258600 Mon Sep 17 00:00:00 2001 From: VietKing Date: Mon, 21 Sep 2026 14:16:55 +0700 Subject: [PATCH 2/3] fix(scan): stop generic-bearer flagging test fixtures and doc placeholders The built-in generic-bearer heuristic flags any quoted 16+ character literal assigned to a name containing token/secret/password/api-key. On a real Next.js/Supabase repo that was 56 findings, every one a placeholder (test_access_token, whsec_placeholder, mock-refresh-token, sbp_your_token_here), so `forge scan security` was permanently red and real hits could hide in it. A value is now recognised as a placeholder structurally, not by silencing directories: it must be a phrase (2+ segments split on - or _, each a plain lower/UPPER/Capitalised word with up to six trailing digits, or a short number) AND either contain a marker word (test, mock, fake, dummy, example, sample, placeholder, changeme, invalid, your, redacted, xxx, demo, fixture, stub) or sit in test code. Opaque values are never excused, in production or in tests: sk_live_/sk_test_ keys, real whsec_, hex, UUIDs, base62, JWT headers, mixed-case and letter/digit blends, and single long segments. A phrase with no marker is still reported outside test code. Only generic-bearer changed; the AWS, sk-, GitHub-token and private-key rules still fire everywhere, including tests. The finding's Secret field is byte-identical to before. Measured on the repo that produced the findings: 56 -> 3. The 3 left (a documented public verify token and two camelCase fixtures) are what waivers are for, and with three waivers the scan is clean with exit 0. Tests pin the rule to the real values (TC-FP-07/08/09), include the negative cases, and were mutation-checked: disabling the skip fails the placeholder test, and removing the plain-word check makes real secrets slip through and fails the negative tests. Co-Authored-By: Claude Sonnet 5 Signed-off-by: VietKing --- internal/cli/cmdscan/placeholder.go | 104 ++++++++++++ internal/cli/cmdscan/placeholder_test.go | 149 ++++++++++++++++++ internal/cli/cmdscan/scan.go | 12 +- .../cli/cmdscan/scanners_placeholder_test.go | 128 +++++++++++++++ 4 files changed, 391 insertions(+), 2 deletions(-) create mode 100644 internal/cli/cmdscan/placeholder.go create mode 100644 internal/cli/cmdscan/placeholder_test.go create mode 100644 internal/cli/cmdscan/scanners_placeholder_test.go 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 cf0517c..3e5fa3a 100644 --- a/internal/cli/cmdscan/scan.go +++ b/internal/cli/cmdscan/scan.go @@ -777,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]], 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) + } +} From 9c6d9634b1aa0deb729922005e095790f0c1dcb8 Mon Sep 17 00:00:00 2001 From: VietKing Date: Mon, 21 Sep 2026 14:16:56 +0700 Subject: [PATCH 3/3] docs(scan): document generic-bearer placeholder handling and the waiver format CHANGELOG entry for the two scan fixes and the IsWaived renewal fix; docs/verbs/scan.md gains the exact placeholder rule with its limits and the .forge/waivers file format. Co-Authored-By: Claude Sonnet 5 Signed-off-by: VietKing --- CHANGELOG.md | 12 ++++++++++++ docs/verbs/scan.md | 44 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+) 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.