diff --git a/check_rbac.go b/check_rbac.go index c18023a..f8f5636 100644 --- a/check_rbac.go +++ b/check_rbac.go @@ -16,10 +16,25 @@ import ( // finding. const coreSDKModulePath = "git.dev.alexdunmow.com/block/core" -// isDefinitionsOnlySDKTarget reports whether the backend target is the shared -// proto SDK (core): it carries proto but serves nothing, so it has neither an -// RBAC interceptor nor a designated serving package prefix. Such a target is -// out of scope for check 2 — see coreSDKModulePath. +// definitionsOnlyModulePaths are Go modules that carry proto service/rpc +// definitions but serve NONE of them — RBAC is enforced in the repos that +// actually serve the procedures (cms for blockninja.v1, orchestrator for +// orchestrator.v1). Scanning them in isolation would flag every procedure as +// "missing from MethodRoles", a not-applicable result rather than a finding. +// +// - core: the shared proto SDK. +// - cli: the ninja developer CLI (WO-WZ-023). It vendors +// orchestrator/v1/plugin_registry.proto to generate a CLIENT for the +// registry; it has no server and no RBAC interceptor. +var definitionsOnlyModulePaths = map[string]bool{ + coreSDKModulePath: true, + "git.dev.alexdunmow.com/block/cli": true, +} + +// isDefinitionsOnlySDKTarget reports whether the backend target carries proto +// but serves nothing, so it has neither an RBAC interceptor nor a designated +// serving package prefix. Such a target is out of scope for check 2 — see +// definitionsOnlyModulePaths. func isDefinitionsOnlySDKTarget(target backendScanTarget) bool { if len(target.allowedPackagePrefixes) > 0 { return false // a designated serving backend (cms/orchestrator) @@ -31,7 +46,7 @@ func isDefinitionsOnlySDKTarget(target backendScanTarget) bool { if err != nil { return false } - return modPath == coreSDKModulePath + return definitionsOnlyModulePaths[modPath] } func init() { @@ -43,6 +58,12 @@ func init() { protoMethods := make([]string, 0) rbacMethods := make(map[string]string) protoMethodSet := make(map[string]bool) + // protoMethodSetAll holds every declared proto procedure (unfiltered + // by served-surface). Stale detection compares RBAC entries against + // this full set so an RBAC role for a real-but-unmounted proto method + // (e.g. orchestrator.v1.DNSService, declared + roled but not served) + // is not mistaken for a dangling entry. + protoMethodSetAll := make(map[string]bool) requiresRPCDiscovery := false var rbacSummaries []rbacTargetSummary var definitionsOnlySDKLabels []string @@ -70,6 +91,20 @@ func init() { if err != nil { rep.Fatal("failed to parse proto files for %s: %v", target.displayOrRoot(), err) } + // Record the FULL declared surface for stale detection, then + // scope the missing-RBAC check to the procedures this target + // actually SERVES. A serving backend may carry proto for + // services it never mounts (for frontend codegen); those are + // not real RBAC gaps. See extractServedServiceKeys. + summary.protoMethodsAll = append(summary.protoMethodsAll, targetProtoMethods...) + for _, pm := range targetProtoMethods { + protoMethodSetAll[pm] = true + } + servedKeys, err := extractServedServiceKeys(target.root) + if err != nil { + rep.Fatal("failed to scan served handlers for %s: %v", target.displayOrRoot(), err) + } + targetProtoMethods = filterProceduresToServed(targetProtoMethods, servedKeys) summary.protoMethods = append(summary.protoMethods, targetProtoMethods...) for _, pm := range targetProtoMethods { if protoMethodSet[pm] { @@ -99,6 +134,22 @@ func init() { if err != nil { rep.Fatal("failed to parse proto files for %s: %v", target.displayOrRoot(), err) } + // Same served-surface scoping as the interceptor branch above: + // keep only procedures whose service this target actually mounts. + // This is the case the orchestrator hits — its RBAC interceptor + // lives under backend/ (so hasCoreRBAC is false at this root), + // and it carries the full blockninja.v1 + helpdesk.v1 proto while + // serving only orchestrator.v1. The full surface is retained for + // stale detection. + summary.protoMethodsAll = append(summary.protoMethodsAll, targetProtoMethods...) + for _, pm := range targetProtoMethods { + protoMethodSetAll[pm] = true + } + servedKeys, err := extractServedServiceKeys(target.root) + if err != nil { + rep.Fatal("failed to scan served handlers for %s: %v", target.displayOrRoot(), err) + } + targetProtoMethods = filterProceduresToServed(targetProtoMethods, servedKeys) summary.protoMethods = append(summary.protoMethods, targetProtoMethods...) for _, pm := range targetProtoMethods { if protoMethodSet[pm] { @@ -128,8 +179,12 @@ func init() { if err != nil { rep.Fatal("failed to parse plugin proto files for %s: %v", target.display, err) } + // Plugins serve every procedure they declare — no served-surface + // filtering. The full and served sets are identical here. summary.protoMethods = append(summary.protoMethods, pluginProtoMethods...) + summary.protoMethodsAll = append(summary.protoMethodsAll, pluginProtoMethods...) for _, pm := range pluginProtoMethods { + protoMethodSetAll[pm] = true if protoMethodSet[pm] { continue } @@ -168,9 +223,13 @@ func init() { } } + // Stale = an RBAC entry that matches no DECLARED proto method at + // all (compare against the full surface, not the served subset — + // a role for a real-but-unmounted proto method is intentional, not + // dangling). var stale []string for m := range rbacMethods { - if !protoMethodSet[m] { + if !protoMethodSetAll[m] { if isExcludedServiceMethod(m) { continue } @@ -192,7 +251,9 @@ func init() { if isExcludedServiceMethod(method) { continue } - if !containsString(summary.protoMethods, method) { + // Compare against the full declared surface (see the + // global stale note above), not the served subset. + if !containsString(summary.protoMethodsAll, method) { summary.staleCount++ } } diff --git a/colors.go b/colors.go index 6e9199a..cf3d77c 100644 --- a/colors.go +++ b/colors.go @@ -187,6 +187,21 @@ func init() { } func isColorAllowed(relPath string) bool { + // The color allowlist keys are source-relative (e.g. + // "components/settings/theme-card.tsx"), but relPath is computed relative to + // whatever scan root the caller used. That root varies by repo: the CMS + // frontend scan root is "web/src" (relPath = "components/..."), while the + // orchestrator's is "frontend" (relPath = "src/components/..."). Trim any of + // these leading source prefixes so one allowlist entry matches regardless of + // which repo is the primary scan target. Mirrors isAllowedPlaceholderFile's + // web/src trimming. + normalized := filepath.ToSlash(relPath) + normalized = strings.TrimPrefix(normalized, "web/src/") + normalized = strings.TrimPrefix(normalized, "frontend/src/") + normalized = strings.TrimPrefix(normalized, "src/") + if colorAllowedFiles[normalized] { + return true + } if colorAllowedFiles[relPath] { return true } diff --git a/comingsoon.go b/comingsoon.go index 39311b6..4e34632 100644 --- a/comingsoon.go +++ b/comingsoon.go @@ -40,6 +40,15 @@ var placeholderCopyPatterns = []*regexp.Regexp{ var rePlaceholderComment = regexp.MustCompile(`(?i)\bplaceholder\b`) var rePlaceholderStringLiteral = regexp.MustCompile("`[^`]*\\bplaceholder\\b[^`]*`|\"[^\"]*\\bplaceholder\\b[^\"]*\"|'[^']*\\bplaceholder\\b[^']*'") +// isGeneratedProtoOutput reports whether path lives under a `gen/` directory +// segment — the buf codegen output tree (e.g. `.../lib/api/gen/blockninja/v1/`). +// Scoped to the placeholder/"ship the feature now" check only: generated proto +// code is never hand-authored, so its symbols (COMING_SOON enum values, etc.) +// must not be flagged as placeholder debt. +func isGeneratedProtoOutput(path string) bool { + return strings.Contains(filepath.ToSlash(path), "/gen/") +} + func checkComingSoon(webSrcDir string) []comingSoonViolation { var violations []comingSoonViolation @@ -61,6 +70,14 @@ func checkComingSoon(webSrcDir string) []comingSoonViolation { if strings.HasSuffix(path, ".test.ts") || strings.HasSuffix(path, ".test.tsx") { return nil } + // Skip buf codegen output (any `gen/` directory segment, e.g. + // lib/api/gen/blockninja/v1/*_pb.d.ts). These files are regenerated by + // buf and must never be hand-edited; their enum values like + // SYSTEM_PAGE_TYPE_COMING_SOON / SITE_MODE_COMING_SOON are real proto + // symbols, not authored placeholder debt. + if isGeneratedProtoOutput(path) { + return nil + } relPath, _ := filepath.Rel(webSrcDir, path) if relPath == "" { @@ -141,7 +158,8 @@ func checkPlaceholderLanguage(roots []todoScanRoot) []comingSoonViolation { slashPath := filepath.ToSlash(path) if strings.Contains(slashPath, "/generated/") || strings.HasSuffix(slashPath, ".pb.go") || - strings.HasSuffix(slashPath, ".connect.go") { + strings.HasSuffix(slashPath, ".connect.go") || + isGeneratedProtoOutput(path) { return nil } if strings.Contains(filepath.ToSlash(path), "cmd/check-safety/") { diff --git a/frontend_extra.go b/frontend_extra.go index 5fcde0b..9292d2e 100644 --- a/frontend_extra.go +++ b/frontend_extra.go @@ -213,6 +213,17 @@ type lockfileViolation struct { file string } +// allowedLockfiles are non-pnpm lockfiles intentionally tolerated, keyed by +// repo-relative slash path. These live in gitignored tool working dirs (never +// committed) and are regenerated by an external toolchain we don't control, so +// they are not part of the repo's dependency surface. +var allowedLockfiles = map[string]bool{ + // cms/styles is a gitignored tailwind-service working dir; its + // package-lock.json is regenerated by the tailwind tooling and never + // committed (see cms/.gitignore). + "styles/package-lock.json": true, +} + func checkLockfiles(repoRoot string) []lockfileViolation { var violations []lockfileViolation @@ -226,7 +237,11 @@ func checkLockfiles(repoRoot string) []lockfileViolation { base := filepath.Base(path) if base == "package-lock.json" || base == "yarn.lock" { relPath, _ := filepath.Rel(repoRoot, path) - violations = append(violations, lockfileViolation{file: relPath}) + rel := filepath.ToSlash(relPath) + if allowedLockfiles[rel] { + return nil + } + violations = append(violations, lockfileViolation{file: rel}) } return nil }); err != nil { diff --git a/proto_rbac.go b/proto_rbac.go index 7da0316..f0e0a8f 100644 --- a/proto_rbac.go +++ b/proto_rbac.go @@ -711,6 +711,118 @@ func normalizeRoleName(raw string) string { return strings.ToLower(raw) } +// servedHandlerRefPattern matches a PACKAGE-QUALIFIED connect handler +// constructor reference, e.g. `orchestratorv1connect.NewSettingsServiceHandler`. +// +// A connect-go service is served by a repo iff its generated +// New{Service}Handler constructor is referenced (called) in that repo's +// HAND-WRITTEN backend code — that reference is how the handler gets mounted on +// the mux. The generated `*.connect.go` stubs DEFINE that constructor for every +// service the repo carries proto for, so a repo that vendors the whole +// blockninja.v1 surface only for frontend codegen (the orchestrator) still +// SERVES just the subset it actually mounts. +// +// We keep the package selector (`…connect`) rather than only the short service +// name because two proto packages can declare services that share a short name +// (`blockninja.v1.SettingsService` vs `orchestrator.v1.SettingsService`); the +// selector is what disambiguates which one is really mounted. +var servedHandlerRefPattern = regexp.MustCompile(`([A-Za-z0-9_]+connect)\.New([A-Za-z0-9_]+)ServiceHandler\b`) + +// extractServedServiceKeys scans a backend target's HAND-WRITTEN Go source for +// package-qualified connect handler mounts and returns the set of served +// services, keyed as "|" (e.g. +// "orchestratorv1connect|SettingsService"). +// +// Generated `*.connect.go` stubs and `_test.go` files are skipped: the stubs +// only DEFINE the constructors (unqualified `func New…ServiceHandler`) and would +// otherwise mark every carried service as served; tests may reference handlers +// for services that aren't mounted in the real server. +func extractServedServiceKeys(root string) (map[string]bool, error) { + served := make(map[string]bool) + err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } + if info.IsDir() { + switch info.Name() { + case ".git", ".worktrees", "vendor", "node_modules": + return filepath.SkipDir + } + return nil + } + if !strings.HasSuffix(path, ".go") { + return nil + } + if strings.HasSuffix(path, ".connect.go") || strings.HasSuffix(path, "_test.go") { + return nil + } + data, err := os.ReadFile(path) + if err != nil { + return nil // skip unreadable files + } + for _, m := range servedHandlerRefPattern.FindAllSubmatch(data, -1) { + connectPkg := string(m[1]) + service := string(m[2]) + "Service" + served[connectPkg+"|"+service] = true + } + return nil + }) + if err != nil { + return nil, err + } + return served, nil +} + +// procedureServedKey maps a proto procedure ("/pkg.sub.ServiceName/Method") to +// the served-service key produced by extractServedServiceKeys, deriving the +// connect package name from the proto package exactly the way +// protoc-gen-connect-go does: strip the dots and append "connect" +// (blockninja.v1 -> blockninjav1connect, orchestrator.v1 -> +// orchestratorv1connect). Returns false for procedures without a package. +func procedureServedKey(procedure string) (string, bool) { + trimmed := strings.TrimPrefix(procedure, "/") + parts := strings.SplitN(trimmed, "/", 2) + if len(parts) != 2 || parts[0] == "" { + return "", false + } + serviceFull := parts[0] // e.g. orchestrator.v1.SettingsService + lastDot := strings.LastIndex(serviceFull, ".") + if lastDot <= 0 || lastDot == len(serviceFull)-1 { + return "", false + } + protoPkg := serviceFull[:lastDot] // orchestrator.v1 + serviceName := serviceFull[lastDot+1:] // SettingsService + connectPkg := strings.ReplaceAll(protoPkg, ".", "") + "connect" + return connectPkg + "|" + serviceName, true +} + +// filterProceduresToServed keeps only procedures whose service is actually +// mounted (served) by the target, per the served-service key set. This is what +// scopes check 2 to a repo's real serving surface: a repo may CARRY proto for +// procedures it never mounts (the orchestrator carries all of blockninja.v1 and +// helpdesk.v1 for frontend codegen but serves only orchestrator.v1), and those +// unmounted procedures are not real RBAC gaps. +// +// If the served set is empty — no recognised handler mounts, e.g. a target that +// mounts via a pattern we don't detect — the procedures are returned UNCHANGED +// so we never silently hide a genuine missing-RBAC finding. +func filterProceduresToServed(procedures []string, served map[string]bool) []string { + if len(served) == 0 { + return procedures + } + out := make([]string, 0, len(procedures)) + for _, p := range procedures { + key, ok := procedureServedKey(p) + if !ok { + continue + } + if served[key] { + out = append(out, p) + } + } + return out +} + func isExcludedServiceMethod(method string) bool { parts := strings.Split(method, "/") if len(parts) < 3 { diff --git a/targets.go b/targets.go index 4bbb2a2..2f01581 100644 --- a/targets.go +++ b/targets.go @@ -28,12 +28,19 @@ type frontendScanTarget struct { } type rbacTargetSummary struct { - label string + label string + // protoMethods is the SERVED procedure surface (filtered to services the + // target actually mounts) — it drives the missing-RBAC (permission_denied) + // finding. protoMethods []string - roleByMethod map[string]string - missingCount int - staleCount int - requiresRPCs bool + // protoMethodsAll is the FULL declared procedure surface (unfiltered) — it + // drives stale-entry detection, so an RBAC role for a real-but-unmounted + // proto method is not reported as stale. + protoMethodsAll []string + roleByMethod map[string]string + missingCount int + staleCount int + requiresRPCs bool } func (t backendScanTarget) displayOrRoot() string { @@ -62,6 +69,17 @@ func resolveScanRoots(target string) (backendDir string, repoRoot string, err er return filepath.Join(absTarget, "backend"), absTarget, nil } + // Orchestrator layout — same standalone-app shape as the CMS above, but the + // frontend lives at frontend/src (not web/src) and proto/buf sit at the repo + // root. Without this clause the orchestrator root resolves as its own backend + // (backendDir == repoRoot): backend/go.mod is reported missing, and + // frontend/src is treated as a plugin frontend so the plugin-only rules fire + // on a first-class app. + if fileExists(filepath.Join(absTarget, "backend", "go.mod")) && dirExists(filepath.Join(absTarget, "frontend", "src")) && + (fileExists(filepath.Join(absTarget, "buf.yaml")) || dirExists(filepath.Join(absTarget, "proto"))) { + return filepath.Join(absTarget, "backend"), absTarget, nil + } + if filepath.Base(absTarget) == "backend" && fileExists(filepath.Join(absTarget, "go.mod")) { parent := filepath.Clean(filepath.Join(absTarget, "..")) if fileExists(filepath.Join(parent, "buf.yaml")) || dirExists(filepath.Join(parent, "proto")) {