fix(security,test): clear 3 SonarCloud vulnerabilities + flaky pipeline test (#74)

* fix(security,test): clear 3 SonarCloud vulnerabilities + flaky pipeline test

Three real findings flagged by SonarCloud's analysis on main, plus a
race-detector flake that surfaced post-merge.

Vulnerabilities
  * BLOCKER gosecurity:S2083 — internal/ui/ui.go:58 path-injection.
    The SPA fallback handler passed r.URL.Path straight to distFS.Open.
    embed.FS already rejects ".." segments, but Sonar's taint tracker
    can't see that. Sanitize at the boundary: path.Clean + reject any
    residual ".." + default empty path to "index.html". Same behavior,
    explicit gate.
  * MAJOR githubactions:S8234 ×2 — .github/workflows/{scorecard,security}.yml.
    Top-level "permissions: read-all" replaced with the minimum
    "permissions: { contents: read }" needed for actions/checkout.
    Each job already declares the narrower scopes it actually needs
    (security-events: write, id-token: write, etc.), so the overly
    permissive default was redundant and tripped Scorecard's
    Token-Permissions guidance.

Flaky test
  * internal/ingest/pipeline_test.go:497,504 —
    TestPipeline_PerTenantCap_ReleasedAfterProcess used 2s waitFor
    deadlines that fail under the race detector on busy CI runners.
    Bumped both to 5s. Test passes locally in milliseconds; the
    timeout is purely a CI-flake margin. Confirmed reproducible-pass
    locally with -race -count=5.

Verification
  * go vet ./... clean
  * go test ./... — 516 pass / 27 packages
  * go build ./... clean

Out of scope (next pass)
  * 147 code-smell issues (cognitive complexity ×55, string-literal
    duplication ×31, TS-specific ×24, etc.). Tracked separately —
    fixing them in this PR would muddle the security-fix scope.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(ui): replace manual SPA dispatch with spaFS wrapper

Sonar's go-security taint engine kept flagging the explicit
distFS.Open(rel) call in the SPA-fallback handler even after path.Clean
+ ".." check. The pattern is structurally safe (embed.FS rejects ".."
on its own) but the engine can't model it.

Restructure: wrap the dist sub-FS with spaFS — when http.FileServer
hits ErrNotExist on an extensionless path, the wrapper transparently
serves index.html so the React router can claim the URL. Asset-shaped
paths (anything with ".") still 404 normally, so a missing favicon
doesn't surprise the browser with an HTML body.

Net result: the user-controlled URL never crosses our own Open() call —
http.FileServer is the only caller, and Sonar trusts that boundary.

Verification
  * go vet ./... clean
  * go test ./... — 516 pass / 27 packages
  * go build ./... clean

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(ingest): bump remaining waitFor timeouts + sync on actual condition

The PR #74 amendment surfaced a second flaky test
(TestPipeline_PreservesInsertionOrder) that fails on CI under the race
detector for the same reason as the first: 2-second deadlines too tight.

Two fixes
  * Bulk-bump all remaining waitFor(t, 2*time.Second, ...) calls in
    pipeline_test.go to 5 seconds (5 sites). Tolerates race-detector
    overhead on slow CI runners; locally these all complete in
    milliseconds.
  * TestPipeline_PreservesInsertionOrder synced on Stats().Processed == 1
    but asserted on snapshotOrder() length. Stats() can bump between
    BatchCreate calls under the race detector, leaving the order
    snapshot partial. Switched the wait to len(w.snapshotOrder()) >= 3 —
    the actual condition the assertion cares about.

Verification
  * go test -race -count=3 ./internal/ingest/... — 114 pass × 3 runs
  * go vet ./... clean
  * locally reproducible-pass under -race

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(ingest): extract shared helper for failure-skip pipeline tests

SonarCloud quality gate failed PR #74 on
new_duplicated_lines_density=3.5% (threshold 3%). The duplication is
between TestPipeline_FailedSpansSkipsLogs and
TestPipeline_FailedTracesAbortsBatch — same boilerplate, same waitFor,
same assertion shape. The waitFor 2s→5s bump made those lines "new
code", so Sonar recounted the existing duplication against the PR's
new-code budget.

Extract runFailureSkipsCheck(t, writer, forbidden...) — single helper
takes the configured fakeWriter and the BatchCreate* names that must
not fire after the seeded upstream failure. Each failing-path test
collapses to a single line.

Verification
  * go test -race -count=3 -run 'TestPipeline_Failed*' — pass × 3
  * go vet ./... clean

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: aksOps

.github/workflows/scorecard.yml

@@ -13,9 +13,11 @@ on:
     - cron: "0 6 * * 1"
   workflow_dispatch:
 
-# Restrict the default GITHUB_TOKEN to read-only; the steps below request the
-# narrow scopes they actually need.
-permissions: read-all
+# Workflow-level default: minimum needed for actions/checkout. The analysis
+# job declares the additional narrow scopes (security-events: write, id-token:
+# write, actions: read) it actually needs.
+permissions:
+  contents: read
 
 jobs:
   analysis:

.github/workflows/security.yml

@@ -17,7 +17,11 @@ on:
   schedule:
     - cron: '21 4 * * 1'  # Mondays 04:21 UTC — catch newly-disclosed CVEs
 
-permissions: read-all
+# Workflow-level default: minimum needed for actions/checkout. Each job
+# declares the additional narrow scopes (security-events: write for SARIF
+# uploaders, contents: write for SBOM artifact upload) it actually needs.
+permissions:
+  contents: read
 
 jobs:
   osv-scanner:

internal/ingest/pipeline_test.go

@@ -239,8 +239,11 @@ func TestPipeline_PreservesInsertionOrder(t *testing.T) {
 	if err := p.Submit(healthyBatch()); err != nil {
 		t.Fatalf("submit: %v", err)
 	}
-	if !waitFor(t, 2*time.Second, func() bool { return p.Stats().Processed == 1 }) {
-		t.Fatalf("worker did not process batch within deadline")
+	// Sync on the assertion target — the per-signal call sequence — rather
+	// than Stats().Processed, which can bump between BatchCreate calls under
+	// the race detector and trip the length check on a partial slice.
+	if !waitFor(t, 5*time.Second, func() bool { return len(w.snapshotOrder()) >= 3 }) {
+		t.Fatalf("worker did not record 3 calls within deadline (got %v)", w.snapshotOrder())
 	}
 
 	got := w.snapshotOrder()
@@ -273,17 +276,18 @@ func TestPipeline_CallbacksFireAfterPersistence(t *testing.T) {
 	if err := p.Submit(b); err != nil {
 		t.Fatalf("submit: %v", err)
 	}
-	if !waitFor(t, 2*time.Second, func() bool { return spanHits.Load() == 1 && logHits.Load() == 1 }) {
+	if !waitFor(t, 5*time.Second, func() bool { return spanHits.Load() == 1 && logHits.Load() == 1 }) {
 		t.Fatalf("callbacks did not fire (span=%d log=%d, want 1/1)", spanHits.Load(), logHits.Load())
 	}
 }
 
-func TestPipeline_FailedSpansSkipsLogs(t *testing.T) {
-	// When BatchCreateSpans fails, BatchCreateLogs must NOT run for that
-	// batch — preserves the invariant that orphan logs aren't persisted
-	// without their span. Mirrors the synchronous path's behavior of
-	// returning the span error before log insert.
-	w := &fakeWriter{spanErr: errors.New("span db down")}
+// runFailureSkipsCheck wires up a 1-worker pipeline with the configured
+// fakeWriter, submits a healthy batch, waits for the failure to surface,
+// then asserts that none of the forbidden BatchCreate* calls fired.
+// Shared by the trace-fails and span-fails skip tests so the boilerplate
+// (pipeline lifecycle + waitFor) lives in one place.
+func runFailureSkipsCheck(t *testing.T, w *fakeWriter, forbidden ...string) {
+	t.Helper()
 	p := NewPipeline(w, nil, PipelineConfig{Capacity: 2, Workers: 1})
 	ctx, cancel := context.WithCancel(context.Background())
 	t.Cleanup(cancel)
@@ -293,41 +297,33 @@ func TestPipeline_FailedSpansSkipsLogs(t *testing.T) {
 	if err := p.Submit(healthyBatch()); err != nil {
 		t.Fatalf("submit: %v", err)
 	}
-	if !waitFor(t, 2*time.Second, func() bool { return p.Stats().ProcessFailures > 0 }) {
+	if !waitFor(t, 5*time.Second, func() bool { return p.Stats().ProcessFailures > 0 }) {
 		t.Fatalf("expected ProcessFailures > 0, got %d", p.Stats().ProcessFailures)
 	}
 	calls := w.snapshotOrder()
 	for _, c := range calls {
-		if c == "logs" {
-			t.Fatalf("BatchCreateLogs ran after spans failed — order=%v", calls)
+		for _, f := range forbidden {
+			if c == f {
+				t.Fatalf("%s ran after upstream failure — order=%v", f, calls)
+			}
 		}
 	}
 }
 
+func TestPipeline_FailedSpansSkipsLogs(t *testing.T) {
+	// When BatchCreateSpans fails, BatchCreateLogs must NOT run for that
+	// batch — preserves the invariant that orphan logs aren't persisted
+	// without their span. Mirrors the synchronous path's behavior of
+	// returning the span error before log insert.
+	runFailureSkipsCheck(t, &fakeWriter{spanErr: errors.New("span db down")}, "logs")
+}
+
 func TestPipeline_FailedTracesAbortsBatch(t *testing.T) {
 	// Trace failures roll the entire batch back — atomic batches are the
 	// fix for orphan FK rows when a worker crashes between BatchCreate*
 	// calls. Spans and logs must NOT be persisted when the trace insert
 	// fails. Counterpart of TestPipeline_FailedSpansSkipsLogs.
-	w := &fakeWriter{traceErr: errors.New("transient")}
-	p := NewPipeline(w, nil, PipelineConfig{Capacity: 2, Workers: 1})
-	ctx, cancel := context.WithCancel(context.Background())
-	t.Cleanup(cancel)
-	p.Start(ctx)
-	t.Cleanup(p.Stop)
-
-	if err := p.Submit(healthyBatch()); err != nil {
-		t.Fatalf("submit: %v", err)
-	}
-	if !waitFor(t, 2*time.Second, func() bool { return p.Stats().ProcessFailures > 0 }) {
-		t.Fatalf("expected ProcessFailures > 0, got %d", p.Stats().ProcessFailures)
-	}
-	calls := w.snapshotOrder()
-	for _, c := range calls {
-		if c == "spans" || c == "logs" {
-			t.Fatalf("spans/logs ran after trace failure — order=%v", calls)
-		}
-	}
+	runFailureSkipsCheck(t, &fakeWriter{traceErr: errors.New("transient")}, "spans", "logs")
 }
 
 func TestPipeline_DrainsOnStop(t *testing.T) {
@@ -493,15 +489,17 @@ func TestPipeline_PerTenantCap_ReleasedAfterProcess(t *testing.T) {
 	if err := p.Submit(mk()); err != nil {
 		t.Fatalf("submit 1: %v", err)
 	}
-	// Wait for the worker to drain it (and release the slot).
-	if !waitFor(t, 2*time.Second, func() bool { return p.Stats().Processed == 1 }) {
+	// Wait for the worker to drain it (and release the slot). 5s tolerates
+	// the race detector's overhead on slow CI runners — the test passes
+	// locally in milliseconds.
+	if !waitFor(t, 5*time.Second, func() bool { return p.Stats().Processed == 1 }) {
 		t.Fatalf("worker did not process first batch")
 	}
 	// Second batch must succeed because the slot was released.
 	if err := p.Submit(mk()); err != nil {
 		t.Fatalf("submit 2 after release: %v", err)
 	}
-	if !waitFor(t, 2*time.Second, func() bool { return p.Stats().Processed == 2 }) {
+	if !waitFor(t, 5*time.Second, func() bool { return p.Stats().Processed == 2 }) {
 		t.Fatalf("worker did not process second batch")
 	}
 	if got := p.TenantDropped(); got != 0 {
@@ -542,7 +540,7 @@ func TestPipeline_PanicInCallbackRecovered(t *testing.T) {
 	if err := p.Submit(good); err != nil {
 		t.Fatalf("submit good: %v", err)
 	}
-	if !waitFor(t, 2*time.Second, func() bool { return p.Stats().Processed >= 2 }) {
+	if !waitFor(t, 5*time.Second, func() bool { return p.Stats().Processed >= 2 }) {
 		t.Fatalf("worker did not survive callback panic — Processed=%d", p.Stats().Processed)
 	}
 	if p.Stats().ProcessFailures == 0 {

internal/ui/ui.go

@@ -2,6 +2,7 @@ package ui
 
 import (
 	"embed"
+	"errors"
 	"fmt"
 	"io/fs"
 	"net/http"
@@ -13,6 +14,33 @@ import (
 	"github.com/RandomCodeSpace/otelcontext/internal/vectordb"
 )
 
+// spaFS wraps an fs.FS so http.FileServer transparently serves index.html
+// for any extensionless path that doesn't resolve to a real file — the
+// usual single-page-app routing where the React router owns client-side
+// URLs. Asset-shaped paths (anything with a ".") still 404 normally so a
+// missing /favicon.ico doesn't surprise the browser with an HTML body.
+//
+// Wrapping the FS — rather than calling Open() against r.URL.Path in our
+// own handler — keeps the user-controlled name behind the stdlib
+// http.FileServer boundary, where path.Clean has already happened.
+type spaFS struct{ fs.FS }
+
+func (s spaFS) Open(name string) (fs.File, error) {
+	f, err := s.FS.Open(name)
+	if err == nil {
+		return f, nil
+	}
+	if !errors.Is(err, fs.ErrNotExist) {
+		return nil, err
+	}
+	// SPA fallback only for extensionless paths (treated as client-side
+	// routes); legitimate asset 404s still propagate.
+	if strings.Contains(name, ".") {
+		return nil, err
+	}
+	return s.FS.Open("index.html")
+}
+
 //go:embed static/* dist
 var content embed.FS
 
@@ -46,26 +74,15 @@ func (s *Server) SetMCPConfig(enabled bool, path string) {
 func (s *Server) RegisterRoutes(mux *http.ServeMux) error {
 	mux.Handle("/static/", http.FileServer(http.FS(content)))
 
-	// Serve React SPA from dist/ for all non-API paths.
-	// API routes are registered before this is called, so they take priority.
+	// Serve React SPA from dist/ for all non-API paths. API routes are
+	// registered before this is called, so they take priority. spaFS
+	// converts extensionless 404s into index.html so the React router
+	// can claim them.
 	distFS, err := fs.Sub(content, "dist")
 	if err != nil {
 		return fmt.Errorf("ui: failed to create dist sub-fs: %w", err)
 	}
-	fileServer := http.FileServer(http.FS(distFS))
-	mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
-		// Try the file as-is; if not found, fall back to index.html (SPA routing).
-		f, openErr := distFS.Open(strings.TrimPrefix(r.URL.Path, "/"))
-		if openErr == nil {
-			_ = f.Close()
-			fileServer.ServeHTTP(w, r)
-			return
-		}
-		// SPA fallback — let the React router handle the path.
-		r2 := r.Clone(r.Context())
-		r2.URL.Path = "/"
-		fileServer.ServeHTTP(w, r2)
-	})
+	mux.Handle("/", http.FileServer(http.FS(spaFS{distFS})))
 
 	return nil
 }