From 4b1ef708b15de50fd7c9628455673298d72829e8 Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Thu, 30 Apr 2026 15:27:52 +0000 Subject: [PATCH 1/4] fix(security,test): clear 3 SonarCloud vulnerabilities + flaky pipeline test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .github/workflows/scorecard.yml | 8 +++++--- .github/workflows/security.yml | 6 +++++- internal/ingest/pipeline_test.go | 8 +++++--- internal/ui/ui.go | 16 +++++++++++++++- 4 files changed, 30 insertions(+), 8 deletions(-) diff --git a/.github/workflows/scorecard.yml b/.github/workflows/scorecard.yml index 06c25a2..65fadc9 100644 --- a/.github/workflows/scorecard.yml +++ b/.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: diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index 6547cd0..0faec9e 100644 --- a/.github/workflows/security.yml +++ b/.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: diff --git a/internal/ingest/pipeline_test.go b/internal/ingest/pipeline_test.go index 3ce109a..5d5a0e7 100644 --- a/internal/ingest/pipeline_test.go +++ b/internal/ingest/pipeline_test.go @@ -493,15 +493,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 { diff --git a/internal/ui/ui.go b/internal/ui/ui.go index 5960740..b12d98f 100644 --- a/internal/ui/ui.go +++ b/internal/ui/ui.go @@ -5,6 +5,7 @@ import ( "fmt" "io/fs" "net/http" + "path" "strings" "github.com/RandomCodeSpace/otelcontext/internal/graph" @@ -54,8 +55,21 @@ func (s *Server) RegisterRoutes(mux *http.ServeMux) error { } fileServer := http.FileServer(http.FS(distFS)) mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + // Sanitize: clean the URL path and reject anything that would escape + // the dist root. embed.FS already rejects ".." segments, but we + // gate at the boundary so static analyzers don't have to taint-track + // the user-supplied URL through the FS call. + clean := path.Clean("/" + r.URL.Path) + if strings.Contains(clean, "..") { + http.NotFound(w, r) + return + } + rel := strings.TrimPrefix(clean, "/") + if rel == "" { + rel = "index.html" + } // Try the file as-is; if not found, fall back to index.html (SPA routing). - f, openErr := distFS.Open(strings.TrimPrefix(r.URL.Path, "/")) + f, openErr := distFS.Open(rel) if openErr == nil { _ = f.Close() fileServer.ServeHTTP(w, r) From 350bb92a2e66d0880e0c18d2a672489c84ade4fc Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Thu, 30 Apr 2026 15:33:42 +0000 Subject: [PATCH 2/4] fix(ui): replace manual SPA dispatch with spaFS wrapper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- internal/ui/ui.go | 63 +++++++++++++++++++++++++---------------------- 1 file changed, 33 insertions(+), 30 deletions(-) diff --git a/internal/ui/ui.go b/internal/ui/ui.go index b12d98f..b8b813d 100644 --- a/internal/ui/ui.go +++ b/internal/ui/ui.go @@ -2,10 +2,10 @@ package ui import ( "embed" + "errors" "fmt" "io/fs" "net/http" - "path" "strings" "github.com/RandomCodeSpace/otelcontext/internal/graph" @@ -14,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 @@ -47,39 +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) { - // Sanitize: clean the URL path and reject anything that would escape - // the dist root. embed.FS already rejects ".." segments, but we - // gate at the boundary so static analyzers don't have to taint-track - // the user-supplied URL through the FS call. - clean := path.Clean("/" + r.URL.Path) - if strings.Contains(clean, "..") { - http.NotFound(w, r) - return - } - rel := strings.TrimPrefix(clean, "/") - if rel == "" { - rel = "index.html" - } - // Try the file as-is; if not found, fall back to index.html (SPA routing). - f, openErr := distFS.Open(rel) - 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 } From b51869ba5e93d417bd13e240e4344c806f475753 Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Thu, 30 Apr 2026 15:37:26 +0000 Subject: [PATCH 3/4] test(ingest): bump remaining waitFor timeouts + sync on actual condition MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- internal/ingest/pipeline_test.go | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/internal/ingest/pipeline_test.go b/internal/ingest/pipeline_test.go index 5d5a0e7..22ab028 100644 --- a/internal/ingest/pipeline_test.go +++ b/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,7 +276,7 @@ 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()) } } @@ -293,7 +296,7 @@ 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() @@ -319,7 +322,7 @@ func TestPipeline_FailedTracesAbortsBatch(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() @@ -544,7 +547,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 { From 75c0e99b6afa00023f71d011eddfd08523ed6cbb Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Thu, 30 Apr 2026 15:41:53 +0000 Subject: [PATCH 4/4] test(ingest): extract shared helper for failure-skip pipeline tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- internal/ingest/pipeline_test.go | 47 ++++++++++++++------------------ 1 file changed, 20 insertions(+), 27 deletions(-) diff --git a/internal/ingest/pipeline_test.go b/internal/ingest/pipeline_test.go index 22ab028..1374f13 100644 --- a/internal/ingest/pipeline_test.go +++ b/internal/ingest/pipeline_test.go @@ -281,12 +281,13 @@ func TestPipeline_CallbacksFireAfterPersistence(t *testing.T) { } } -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) @@ -301,36 +302,28 @@ func TestPipeline_FailedSpansSkipsLogs(t *testing.T) { } 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, 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 == "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) {