From b04c6c202c95e66ef4b4089da44558c27f3930e7 Mon Sep 17 00:00:00 2001 From: fullsend-code <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Fri, 10 Jul 2026 16:39:28 +0000 Subject: [PATCH 1/2] refactor(#249): add caller-callee contract consistency heuristic Add a review heuristic for detecting cross-function data flow inconsistencies in security-relevant struct fields. When a function locally defaults a security-relevant field (allowlist, ACL, permission set) without writing it back to the struct, the caller may pass the same struct to downstream functions that interpret nil/absent differently, creating a fail-open pattern. Changes: - security.md: new "Caller-callee contract consistency" section after "Fail-open / fail-closed evaluation" using fail-open category, with 4-step heuristic and edge-case exclusions - code-review/SKILL.md: corresponding bullet in the Security dimension for standalone reviews Note: pre-commit could not run in the sandbox (shellcheck install blocked by network restrictions). Changes are markdown-only. Closes #249 --- .../fullsend-repo/skills/code-review/SKILL.md | 14 ++++++ .../skills/pr-review/sub-agents/security.md | 49 +++++++++++++++++++ 2 files changed, 63 insertions(+) diff --git a/internal/scaffold/fullsend-repo/skills/code-review/SKILL.md b/internal/scaffold/fullsend-repo/skills/code-review/SKILL.md index f67c35a17..e1a6f3cfd 100644 --- a/internal/scaffold/fullsend-repo/skills/code-review/SKILL.md +++ b/internal/scaffold/fullsend-repo/skills/code-review/SKILL.md @@ -123,6 +123,20 @@ dimension carry over to another — each requires its own scrutiny. control, raise a finding even if the unprotected input appears low-risk — the risk assessment belongs in the finding's severity, not in a decision to omit the finding. +- **Caller-callee contract consistency for security defaults:** When a + function locally defaults a security-relevant struct field (allowlist, + ACL, permission set) without writing the default back to the struct, + trace the struct through the caller to check whether downstream + functions interpret nil/absent differently. If one function treats nil + as "use defaults" (permissive) while a subsequent callee treats nil as + "deny all," flag the inconsistency — the two operations within the + same code path apply contradictory security semantics. Severity is at + least **medium** when the field controls a security boundary; **high** + if the inconsistency creates a fail-open path. Do not stop analysis at + the function boundary — the pattern is only visible by tracing the + struct through the caller's subsequent operations. No finding is needed + if the function writes the default back to the struct, the struct is + not reused after the call, or the field is explicitly empty (not nil). - Content security: does the change affect how user-supplied content is handled or rendered? Are there sandboxing gaps? - **Permission manifest changes:** If the diff modifies any file that diff --git a/internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security.md b/internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security.md index 3380a91e3..20f1b9974 100644 --- a/internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security.md +++ b/internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security.md @@ -104,6 +104,55 @@ Policy thresholds: broader access than when the value is correctly set, the code is fail-open. +## Caller-callee contract consistency + +**Category:** Use `fail-open` for all findings in this section. + +When a function receives a security-relevant field via a struct parameter +(e.g., an allowlist, ACL, permission set, or scope list) and applies a +permissive local default for internal use — such as +`localVar := opts.Allowlist; if localVar == nil { localVar = defaults }` +— without writing the default back to the struct, check whether the +caller passes the same struct to downstream functions that interpret the +field differently. + +This is a cross-function fail-open pattern: the callee behaves as if the +field has a permissive default, but the struct still carries the original +nil/absent value. If a subsequent callee treats nil as deny-all while the +first callee treated nil as use-defaults, the two operations within the +same code path apply contradictory security semantics. + +**Heuristic:** When a function in the diff locally defaults a +security-relevant struct field without writing it back: + +1. **Identify the struct parameter** and the field being defaulted. +2. **Trace the struct through the caller.** Find where the caller + obtained the struct and what other functions receive it after the + current function returns. +3. **Check nil/absent semantics in each downstream callee.** If any + downstream function interprets nil/absent for the same field as + deny-all (or any semantics different from the local default), flag + the inconsistency. +4. **Assess severity.** If the field controls a security boundary + (allowlists, permission sets, access scopes), severity is at least + **medium**. If the inconsistency creates a fail-open path — where + one function permits access while a subsequent function denies it, + or vice versa — severity is **high**. + +**Do not stop analysis at the function boundary.** The pattern is only +visible when you trace the struct through the caller's subsequent +operations. A function that locally defaults a field may appear safe in +isolation but create an asymmetry that breaks downstream invariants. + +Edge cases that do NOT require a finding: + +- The function writes the default back to the struct field (the caller + sees the updated value). +- The struct is not passed to any other function after the call. +- The field is an explicitly empty value (e.g., empty slice, not nil) + rather than absent/nil — this signals intentional "no entries" rather + than "not configured." + ## Permission and role changes **Categories:** `permission-expansion`, `permission-reduction`, From 1d7686a0875a1c1f0932869fbdc3e301664fd090 Mon Sep 17 00:00:00 2001 From: fullsend-fix <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Fri, 10 Jul 2026 17:43:52 +0000 Subject: [PATCH 2/2] fix(#249): distinguish deny-then-permit (fail-open) from permit-then-deny (functional bug) Split step 4 severity guidance in caller-callee heuristic: deny-then-permit direction is genuine fail-open at high severity; permit-then-deny direction is a service-disruption / functional correctness bug at medium severity with category logic-error. Updated both security.md and code-review SKILL.md. Addresses review feedback on #283 --- .../fullsend-repo/skills/code-review/SKILL.md | 4 +++- .../skills/pr-review/sub-agents/security.md | 11 ++++++++--- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/internal/scaffold/fullsend-repo/skills/code-review/SKILL.md b/internal/scaffold/fullsend-repo/skills/code-review/SKILL.md index e1a6f3cfd..b14e67cbf 100644 --- a/internal/scaffold/fullsend-repo/skills/code-review/SKILL.md +++ b/internal/scaffold/fullsend-repo/skills/code-review/SKILL.md @@ -132,7 +132,9 @@ dimension carry over to another — each requires its own scrutiny. "deny all," flag the inconsistency — the two operations within the same code path apply contradictory security semantics. Severity is at least **medium** when the field controls a security boundary; **high** - if the inconsistency creates a fail-open path. Do not stop analysis at + if the deny-then-permit direction creates a genuine fail-open path + (use category `logic-error` at **medium** for the permit-then-deny + direction, which is a functional correctness bug). Do not stop analysis at the function boundary — the pattern is only visible by tracing the struct through the caller's subsequent operations. No finding is needed if the function writes the default back to the struct, the struct is diff --git a/internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security.md b/internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security.md index 20f1b9974..0a13ee21d 100644 --- a/internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security.md +++ b/internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security.md @@ -135,9 +135,14 @@ security-relevant struct field without writing it back: the inconsistency. 4. **Assess severity.** If the field controls a security boundary (allowlists, permission sets, access scopes), severity is at least - **medium**. If the inconsistency creates a fail-open path — where - one function permits access while a subsequent function denies it, - or vice versa — severity is **high**. + **medium**. Distinguish the two directions of inconsistency: + - **Deny-then-permit** (first callee restricts, subsequent callee + defaults to permissive): this is a genuine **fail-open** path — + severity is **high**. + - **Permit-then-deny** (first callee permits, subsequent callee + defaults to deny-all): this is a service-disruption / functional + correctness bug, not a security fail-open. Use category + `logic-error` at **medium** severity. **Do not stop analysis at the function boundary.** The pattern is only visible when you trace the struct through the caller's subsequent