diff --git a/src/review/feature-activation.ts b/src/review/feature-activation.ts index 179ba40785..25a646d0b4 100644 --- a/src/review/feature-activation.ts +++ b/src/review/feature-activation.ts @@ -6,7 +6,11 @@ // (`features:` block). The precedence, highest to lowest: // 1. GLOBAL env flag (GITTENSORY_REVIEW_*) — a MASTER KILL-SWITCH. Off ⇒ the feature never runs anywhere, // regardless of any per-repo override (so an operator keeps one deploy-wide off switch per feature). -// 2. Per-repo `features:` override — `true`/`false` forces the feature on/off for this repo. +// 2. Per-repo `features:` override — `true`/`false` forces the feature on/off for this repo. EXCEPTION: +// `safety` (prompt-injection defanging) is security-critical and `.gittensory.yml` lives in the repo +// itself, writable by a lower-trust actor than the operator — so a repo override may only TIGHTEN +// (force-on) the operator's global enablement, never loosen it. `features.safety: false` is treated as +// "no opinion" (falls through to the allowlist default below) rather than an active force-off (#2269). // 3. `GITTENSORY_REVIEW_REPOS` allowlist — the back-compat DEFAULT when the manifest says nothing, so a repo // that sets no `features:` block behaves exactly as it did before this change. // @@ -32,6 +36,7 @@ const FEATURE_GLOBAL_FLAG: Record boolean> = * Resolve whether a converged feature is active for a repo, given the already-loaded manifest (or null). Pure + * synchronous so it carries no I/O and is the single unit-tested place the precedence lives. Precedence: env * kill-switch (off ⇒ false) → per-repo `features:` override → `GITTENSORY_REVIEW_REPOS` allowlist default. + * `safety` is asymmetric: an override can only force it ON, never force it OFF (#2269) — see the file header. */ export function resolveConvergedFeature( env: Env, @@ -41,6 +46,9 @@ export function resolveConvergedFeature( ): boolean { if (!FEATURE_GLOBAL_FLAG[feature](env)) return false; // master kill-switch const override = manifest?.features?.[feature] ?? null; + // Security-critical: a repo-controlled override must not silently defeat the operator's global enablement. + // `false` is downgraded to "no opinion" so it falls through to the allowlist default instead of forcing off. + if (feature === "safety") return override === true || isConvergenceRepoAllowed(env, repoFullName); if (override !== null) return override; // explicit per-repo on/off return isConvergenceRepoAllowed(env, repoFullName); // back-compat allowlist default } diff --git a/test/unit/feature-activation.test.ts b/test/unit/feature-activation.test.ts index 2f88bffcad..07383c4603 100644 --- a/test/unit/feature-activation.test.ts +++ b/test/unit/feature-activation.test.ts @@ -56,6 +56,29 @@ describe("resolveConvergedFeature — env kill-switch → per-repo override → }); }); +describe("resolveConvergedFeature — safety is force-on-only, never force-off (#2269)", () => { + it("ignores a repo override that tries to force safety OFF, falling through to the allowlist default", () => { + // Operator enabled safety globally AND allowlisted this repo — a repo-controlled override must not defeat it. + const allowlisted = env({ GITTENSORY_REVIEW_SAFETY: "true", GITTENSORY_REVIEW_REPOS: REPO }); + expect(resolveConvergedFeature(allowlisted, manifestWith({ safety: false }), "safety", REPO)).toBe(true); + + // Not allowlisted: the override is still ignored (treated as "no opinion"), so the allowlist default (off) applies. + // This is off for the same reason a bare `manifestWith({})` would be off here — not because the override "worked". + const notAllowlisted = env({ GITTENSORY_REVIEW_SAFETY: "true", GITTENSORY_REVIEW_REPOS: "other/repo" }); + expect(resolveConvergedFeature(notAllowlisted, manifestWith({ safety: false }), "safety", REPO)).toBe(false); + }); + + it("still honors a repo override that forces safety ON, even when the repo is not allowlisted", () => { + const e = env({ GITTENSORY_REVIEW_SAFETY: "true", GITTENSORY_REVIEW_REPOS: "other/repo" }); + expect(resolveConvergedFeature(e, manifestWith({ safety: true }), "safety", REPO)).toBe(true); + }); + + it("still respects the master kill-switch — a true override cannot turn safety on when the global flag is off", () => { + const e = env({ GITTENSORY_REVIEW_REPOS: REPO }); // GITTENSORY_REVIEW_SAFETY unset + expect(resolveConvergedFeature(e, manifestWith({ safety: true }), "safety", REPO)).toBe(false); + }); +}); + describe("convergedFeatureActive — async (loads the cached manifest)", () => { it("short-circuits to false WITHOUT loading the manifest when the env flag is off", async () => { // DB-less env: if it tried to load the manifest it would throw; returning false proves the short-circuit. @@ -73,4 +96,10 @@ describe("convergedFeatureActive — async (loads the cached manifest)", () => { const e = createTestEnv({ GITTENSORY_REVIEW_RAG: "true", GITTENSORY_REVIEW_REPOS: REPO }); expect(await convergedFeatureActive(e, REPO, "rag")).toBe(true); }); + + it("applies the safety force-on-only exception through the async DB-backed path too (#2269)", async () => { + const e = createTestEnv({ GITTENSORY_REVIEW_SAFETY: "true", GITTENSORY_REVIEW_REPOS: REPO }); + await upsertRepoFocusManifest(e, REPO, { features: { safety: false } }); + expect(await convergedFeatureActive(e, REPO, "safety")).toBe(true); // override ignored, allowlist wins + }); });