Fix the cross-frontend defects the JS rebuild exposed, and two gaps the CVE campaign found

.claude/skills/cve-recall/projects.yaml

@@ -32,7 +32,9 @@ projects:
   - {name: directus,        ecosystem: npm,  package: "@directus/api",  repo: https://github.com/directus/directus.git,            lang: ts, subdir: api}
   - {name: strapi,          ecosystem: npm,  package: "@strapi/strapi", repo: https://github.com/strapi/strapi.git,                lang: ts}
   - {name: nocodb,          ecosystem: npm,  package: nocodb,           repo: https://github.com/nocodb/nocodb.git,                lang: ts}
-  - {name: ghost,           ecosystem: npm,  package: ghost,            repo: https://github.com/TryGhost/Ghost.git,               lang: js, subdir: ghost}
+  # No subdir: Ghost is a monorepo; Portal (apps/portal) is a separate React app
+  # carrying its own CVEs, so scoping to `ghost/` hides them.
+  - {name: ghost,           ecosystem: npm,  package: ghost,            repo: https://github.com/TryGhost/Ghost.git,               lang: js}
   - {name: n8n,             ecosystem: npm,  package: n8n,              repo: https://github.com/n8n-io/n8n.git,                   lang: ts, subdir: packages}
   - {name: nextjs,          ecosystem: npm,  package: next,             repo: https://github.com/vercel/next.js.git,               lang: ts, subdir: packages/next/src}
   - {name: verdaccio,       ecosystem: npm,  package: verdaccio,        repo: https://github.com/verdaccio/verdaccio.git,          lang: ts}

cmd/godzilla/main.go

@@ -160,7 +160,15 @@ func runScan(args []string) {
 	buildTimeout := fs.Duration("build-timeout", proc.BuildTimeout(), "deadline for a whole-project build subprocess (only runs with -allow-build)")
 	_ = fs.Parse(args)
 
-	buildpolicy.SetAllowed(*allowBuild)
+	// Only an EXPLICIT -allow-build decides the policy. Calling SetAllowed
+	// unconditionally made the flag's false default UNSET GODZILLA_ALLOW_BUILD, so
+	// the environment variable this flag's own help text offers as the alternative
+	// could never take effect through the CLI.
+	fs.Visit(func(f *flag.Flag) {
+		if f.Name == "allow-build" {
+			buildpolicy.SetAllowed(*allowBuild)
+		}
+	})
 	proc.SetTimeouts(*parseTimeout, *buildTimeout)
 	report.Version = version // stamp the tool version into SARIF/JSON reports
 

converters/go/converter.go

@@ -62,6 +62,11 @@ type Converter struct {
 	// collectRouteHandlers alongside routeHandlers.
 	routeFormParams map[*ssa.Function][]string
 
+	// funcAliases maps a package-level variable that holds a FUNCTION to that
+	// function, so a call through the variable gets a callee name. Populated by
+	// collectFuncAliases.
+	funcAliases map[*ssa.Global]*ssa.Function
+
 	// targetPkgs is the set of user-authored (scanned) package import paths.
 	// Dependency bodies are lowered so taint flows through them, but findings are
 	// scoped back to these packages so a sink reached inside a library is not
@@ -137,6 +142,7 @@ func (c *Converter) ConvertFile(path string) (*ir.Program, error) {
 	// between route-handler detection and the grouping below.
 	allFns := ssautil.AllFunctions(prog)
 	c.routeHandlers, c.routeFormParams = collectRouteHandlers(allFns)
+	c.funcAliases = collectFuncAliases(allFns)
 
 	// Lower only the functions REACHABLE from user (reportable) code. A whole
 	// dependency closure runs to tens of thousands of functions, but the
@@ -459,13 +465,19 @@ func (c *Converter) lowerModules(funcsByPkg map[*ssa.Package][]*ssa.Function, st
 // typeCache/fnNames, so it can lower functions concurrently without locking or
 // racing on the shared caches. targetPkgs is intentionally NOT copied: it is read
 // only via TargetPackages() on the top-level converter, never on the worker path.
+// worker returns a Converter for one lowering goroutine. Every whole-program
+// analysis result computed once in convertProgram has to be carried across here:
+// a worker builds its own maps, so a field left out is silently EMPTY during
+// lowering rather than nil-panicking, and the analysis it represents just stops
+// having an effect.
 func (c *Converter) worker() *Converter {
 	w := NewConverter()
 	w.fset = c.fset
 	w.baseTypes = c.typeCache
 	w.baseNames = c.fnNames
 	w.routeHandlers = c.routeHandlers
 	w.routeFormParams = c.routeFormParams
+	w.funcAliases = c.funcAliases
 	return w
 }
 
@@ -757,6 +769,75 @@ var routingVerbs = map[string]bool{
 // (r.GET("/x", h), mux.HandleFunc(..., h), e.Use(mw), …) — and maps each to the
 // register name of its request/context parameter, plus its bound-form parameter
 // registers (see formParams).
+// collectFuncAliases maps each package-level variable that holds a FUNCTION to
+// that function. A package re-exporting another's function as a variable --
+// `var Params = macaron.Params`, how grafana's pkg/web surfaces macaron -- turns
+// every call through it into an INDIRECT call: SSA loads the global and calls the
+// loaded value, so there is no static callee and convertCall leaves the name
+// empty. An unnamed callee matches no source, sink or propagator glob, so the
+// call is invisible to every rule; grafana CVE-2021-43798 reaches os.Open through
+// exactly that alias.
+//
+// Only a variable with EXACTLY ONE store program-wide is mapped, and only when
+// that store's value is a function. A second store means the variable is
+// reassigned -- a mock swapped in by a test, a hook rebound at init -- and the
+// call site no longer has one answer, so naming either would be a guess. Counting
+// every store rather than only the function-valued ones is what makes the guard
+// hold: a var later set to nil or to a different function is excluded, not
+// silently resolved to its first value.
+func collectFuncAliases(allFns map[*ssa.Function]bool) map[*ssa.Global]*ssa.Function {
+	type record struct {
+		fn     *ssa.Function
+		stores int
+	}
+	seen := map[*ssa.Global]*record{}
+	for fn := range allFns {
+		for _, b := range fn.Blocks {
+			for _, instr := range b.Instrs {
+				st, ok := instr.(*ssa.Store)
+				if !ok {
+					continue
+				}
+				g, ok := st.Addr.(*ssa.Global)
+				if !ok {
+					continue
+				}
+				r := seen[g]
+				if r == nil {
+					r = &record{}
+					seen[g] = r
+				}
+				r.stores++
+				if target, ok := st.Val.(*ssa.Function); ok {
+					r.fn = target
+				}
+			}
+		}
+	}
+	out := map[*ssa.Global]*ssa.Function{}
+	for g, r := range seen {
+		if r.stores == 1 && r.fn != nil {
+			out[g] = r.fn
+		}
+	}
+	return out
+}
+
+// aliasedFunc returns the function an indirect call's callee value resolves to
+// when that value is a load of a function-holding package-level variable, or nil.
+// A load is UnOp(MUL) over the global.
+func (c *Converter) aliasedFunc(v ssa.Value) *ssa.Function {
+	load, ok := v.(*ssa.UnOp)
+	if !ok || load.Op != token.MUL {
+		return nil
+	}
+	g, ok := load.X.(*ssa.Global)
+	if !ok {
+		return nil
+	}
+	return c.funcAliases[g]
+}
+
 func collectRouteHandlers(allFns map[*ssa.Function]bool) (map[*ssa.Function]string, map[*ssa.Function][]string) {
 	handlers := map[*ssa.Function]string{}
 	forms := map[*ssa.Function][]string{}
@@ -1174,6 +1255,15 @@ func (c *Converter) convertCall(call ssa.CallCommon) *ir.CallCommon {
 		}
 	} else if b, ok := call.Value.(*ssa.Builtin); ok {
 		cc.Callee = "builtin." + b.Name()
+	} else if fn := c.aliasedFunc(call.Value); fn != nil {
+		// A call through a function-holding package-level variable. Named with the
+		// function it resolves to rather than the variable, because that is the Go
+		// frontend's convention throughout -- callees are semantic FQNs from SSA,
+		// never the syntax at the call site. See collectFuncAliases.
+		cc.Callee = c.canonicalFunc(fn)
+		if sig := fn.Signature; sig != nil && sig.Recv() != nil {
+			cc.MethodName = fn.Name()
+		}
 	}
 	if n := len(call.Args); n > 0 {
 		cc.Args = make([]*ir.Value, n)

converters/java/converter.go

@@ -217,6 +217,22 @@ func resolveJavaSource(scanPath string, idx map[string]string, source string) st
 	return scanPath
 }
 
+// Usable reports whether the `java` on PATH is one ConvertFile will accept, and
+// the major version it found (0 when the probe could not determine one).
+//
+// It is the SAME predicate ConvertFile enforces, exported so a test can skip on
+// what would otherwise fail. Guarding on mere presence of `java` disagrees: an
+// old JDK passes the guard and then fails the conversion, which reads as a
+// broken frontend rather than a missing toolchain.
+func Usable() (int, bool) {
+	javaExe, err := exec.LookPath("java")
+	if err != nil {
+		return 0, false
+	}
+	major, ok := javaMajorCached(javaExe)
+	return major, !ok || major >= minJDK
+}
+
 // javaMajorCached memoizes javaMajor per launcher path for the process
 // lifetime: the JDK under a fixed path does not change mid-run, and the probe
 // costs a full JVM spawn.

converters/java/converter_test.go

@@ -11,9 +11,19 @@ import (
 	ir "github.com/bytevet/godzilla/pkg/ir/v1"
 )
 
-// requireJava skips when no JDK `java` launcher is on PATH (the frontend runs
-// the embedded JavaDump.java single-file program via it).
-func requireJava(t *testing.T) { testsupport.RequireTool(t, "java") }
+// requireJava skips unless the `java` on PATH is one the frontend can use.
+//
+// Presence alone is the wrong predicate: the JDK 24 floor is enforced inside
+// ConvertFile, so an older JDK passes a presence guard and then fails every
+// test — red that reads as a broken frontend rather than a toolchain this
+// machine does not have.
+func requireJava(t *testing.T) {
+	t.Helper()
+	testsupport.RequireTool(t, "java")
+	if major, ok := Usable(); !ok {
+		t.Skipf("found Java %d; the frontend requires JDK %d+", major, minJDK)
+	}
+}
 
 func eachInstr(prog *ir.Program, visit func(*ir.Instruction)) {
 	for _, fn := range irwalk.Funcs(prog) {

converters/javascript/converter_test.go

@@ -83,20 +83,6 @@ func requireFinding(t *testing.T, findings []analysis.Finding, ruleID string) an
 	return analysis.Finding{}
 }
 
-// requireNoFallbackIntrinsic asserts no instruction lowered to js.unsupported.
-// A fallback marks a construct the lowering does not model, and it is silent:
-// the file still converts, so only this catches it.
-func requireNoFallbackIntrinsic(t *testing.T, prog *ir.Program, what string) {
-	t.Helper()
-	for _, fn := range irwalk.Funcs(prog) {
-		for inst := range irwalk.Instrs(fn) {
-			if inst.Op == ir.OpCode_OP_CODE_INTRINSIC && inst.Intrinsic == "js.unsupported" {
-				t.Errorf("%s: unsupported instruction in %s: %s", what, fn.CanonicalName, inst.Comment)
-			}
-		}
-	}
-}
-
 func TestConvertXSSSample(t *testing.T) {
 	prog := mustConvert(t, "../../test/js/xss/app.js")
 

converters/javascript/decorators_test.go

@@ -0,0 +1,58 @@
+package js_converter
+
+import (
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// decorateCallees returns the __decorate* callees in a file's lowered IR, which
+// is how the TypeScript legacy-decorator rung is OBSERVABLE from outside: the
+// flag rewrites decorators into those calls, so their presence means the file was
+// parsed with it and their absence means it was parsed as written.
+func decorateCallees(t *testing.T, src string) []string {
+	t.Helper()
+	dir := t.TempDir()
+	p := filepath.Join(dir, "m.ts")
+	if err := os.WriteFile(p, []byte(src), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	prog := mustConvert(t, p)
+	var out []string
+	for _, m := range prog.Modules {
+		for _, f := range m.Functions {
+			for _, b := range f.Blocks {
+				for _, in := range b.Instrs {
+					if in.Call != nil && strings.Contains(in.Call.Callee, "__decorate") {
+						out = append(out, in.Call.Callee)
+					}
+				}
+			}
+		}
+	}
+	return out
+}
+
+// A parameter decorator does not parse without TypeScript's legacy decorators,
+// which costs the WHOLE file -- NestJS and Angular are built out of them. The
+// rung exists so those files are analyzed at all.
+func TestParameterDecoratorParses(t *testing.T) {
+	src := "export class C {\n  async list(@Req() req: Request) { eval(req.query.cmd); }\n}\n"
+	if got := decorateCallees(t, src); len(got) == 0 {
+		t.Errorf("expected the decorator rung to lower this file, got no __decorate* calls")
+	}
+}
+
+// The other half, and the reason the flag is a rung rather than a default: it
+// lowers EVERY experimental decorator in the file it is set on. A file whose
+// decorators sit on the class, method and property parses without it, so it must
+// still reach the lowering as WRITTEN -- if this starts reporting __decorate*
+// calls, the rung has stopped being confined to files that need it and every
+// decorated file is paying for the few that do.
+func TestOrdinaryDecoratorsAreNotLowered(t *testing.T) {
+	src := "@Controller('x')\nexport class C {\n  @Get() list(req: any) { eval(req.query.cmd); }\n  @Inject() svc: any;\n}\n"
+	if got := decorateCallees(t, src); len(got) != 0 {
+		t.Errorf("class/method/property decorators were lowered (%v); the rung should not have been reached", got)
+	}
+}

converters/javascript/dialect.go

@@ -80,6 +80,25 @@ func parseSource(path, code string) (*jsast.File, string, error) {
 			firstErr = parseError(errs)
 		}
 	}
+	// Rung: TypeScript's legacy decorators. A decorator on a PARAMETER is a parse
+	// error without them, which loses the whole file -- NestJS and Angular sources
+	// are built out of them. Decorators on classes, methods and properties parse
+	// without the flag, so only the parameter form needs this.
+	//
+	// A rung and not a default, because the flag LOWERS: it rewrites every
+	// experimental decorator in the file into __decorateClass/__decorateParam
+	// calls emitted after the class, and Class.Decorators comes back empty. Set
+	// always, it would trade the as-written tree of every decorated file for the
+	// few that cannot parse otherwise. Reached only on failure, the lowered shape
+	// is confined to files that would contribute nothing at all.
+	for _, m := range ladder {
+		if !m.ts {
+			continue // the flag needs TypeScript; it is ignored otherwise
+		}
+		if f, errs := jsast.Parse(code, jsast.Options{TS: true, JSX: m.jsx, ExperimentalDecorators: true}); len(errs) == 0 {
+			return f, code, nil
+		}
+	}
 	// Last rung: Flow. Its residue past TypeScript has no dialect of its own, so
 	// no rung recovers it -- the source itself has to change (flowstrip.go).
 	// Retrying the WHOLE ladder rather than one rung keeps the dialect question

converters/javascript/dialects_test.go

@@ -2,6 +2,8 @@ package js_converter
 
 import (
 	"os"
+
+	"github.com/bytevet/godzilla/internal/testsupport"
 	"path/filepath"
 	"testing"
 )
@@ -56,7 +58,7 @@ func TestDialectsScanReportsFullCoverage(t *testing.T) {
 		t.Errorf("Skipped() = %d, want 0 — a dialect stopped parsing; see parseLadder", c.Skipped())
 	}
 	// Converting is half of it: a dialect can parse and still lower to a fallback.
-	requireNoFallbackIntrinsic(t, prog, "testdata/dialects")
+	testsupport.RequireNoFallbackIntrinsic(t, prog, "js.unsupported", "testdata/dialects")
 }
 
 // TestConvertCorpusTreeSkipsOnlyBroken pins the skip count over the whole JS
@@ -76,7 +78,7 @@ func TestConvertCorpusTreeSkipsOnlyBroken(t *testing.T) {
 	if c.Skipped() != 1 {
 		t.Errorf("Skipped() = %d, want 1 (only resilience/broken.js)", c.Skipped())
 	}
-	requireNoFallbackIntrinsic(t, prog, "test/js")
+	testsupport.RequireNoFallbackIntrinsic(t, prog, "js.unsupported", "test/js")
 }
 
 // TestLadderOrderKeepsRelationalArgs pins the rung ORDER, which is load-bearing