Skip to content

Commit 809dd64

Browse files
authored
Merge pull request #2038 from fullsend-ai/agent/898-security-subagent-heuristics
feat(#898): add security heuristics to review sub-agent
2 parents 637f693 + 91378a6 commit 809dd64

2 files changed

Lines changed: 96 additions & 1 deletion

File tree

internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -195,7 +195,7 @@ review dimension using category as the key:
195195
| Dimension | Categories |
196196
|----------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
197197
| correctness | `logic-error`, `nil-deref`, `off-by-one`, `edge-case`, `api-contract`, `missing-test`, `test-inadequate`, `pattern-violation`, `test-weakened`, `test-removed`, `mock-loosened`, `assertion-weakened`, `coverage-reduced`, `test-poisoning`, `split-payload`, `stale-reference` |
198-
| security | `auth-bypass`, `rbac-violation`, `data-exposure`, `privilege-escalation`, `injection-vuln`, `sandbox-escape`, `xss`, `ssrf`, `insecure-deserialization`, `prompt-injection`, `unicode-steganography`, `bidi-override`, `homoglyph-attack`, `instruction-smuggling` |
198+
| security | `auth-bypass`, `rbac-violation`, `data-exposure`, `privilege-escalation`, `injection-vuln`, `sandbox-escape`, `xss`, `ssrf`, `insecure-deserialization`, `prompt-injection`, `unicode-steganography`, `bidi-override`, `homoglyph-attack`, `instruction-smuggling`, `fail-open`, `permission-expansion`, `permission-reduction`, `role-escalation`, `workflow-permission`, `secret-exposure` |
199199
| intent-coherence | `scope-exceeded`, `tier-mismatch`, `unauthorized-change`, `scope-creep`, `missing-authorization`, `misleading-label`, `design-direction`, `complexity-ratio`, `misplaced-abstraction`, `architectural-conflict`, `design-smell`, `over-engineering`, `under-engineering` |
200200
| style-conventions | `naming-convention`, `error-handling-idiom`, `api-shape`, `code-organization`, `doc-style`, `pattern-inconsistency` |
201201
| docs-currency | `stale-doc`, `missing-doc`, `incorrect-doc`, `incomplete-doc` |

internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security.md

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,3 +32,98 @@ title parameter is also sanitized).
3232
metadata (PR body, commit messages, PR description)
3333

3434
Inspect the code diff for injection patterns.
35+
36+
## Exploration budget
37+
38+
Calibrate investigation to the diff size and security surface area.
39+
40+
**Low-risk diffs (docs-only, test-only, style-only changes):**
41+
42+
- Scan for secrets, injection patterns, and permission changes in the diff.
43+
- Do not read additional source files unless the diff touches auth,
44+
authorization, or permission-declaring files.
45+
46+
**Security-relevant diffs (auth, permissions, workflows, config):**
47+
48+
- Read the full file for every changed auth/authorization module to
49+
understand the complete control flow — not just the diff lines.
50+
- Read related config files (manifests, IAM policies, workflow files)
51+
to verify permission scope.
52+
- Trace call sites of changed functions to check for fail-open paths.
53+
54+
## Fail-open / fail-closed evaluation
55+
56+
**Category:** Use `fail-open` for all findings in this section.
57+
58+
When reviewing authentication, authorization, or validation code, always
59+
determine what happens when configuration values are absent, empty, or
60+
malformed. The default behavior must deny access, not permit it.
61+
62+
**Checklist — apply to every auth/validation code path in the diff:**
63+
64+
- **Unset variables:** If the code reads an environment variable,
65+
allowlist, config key, or feature flag that controls access, what
66+
happens when that value is unset? If the code permits all requests
67+
when the value is missing, that is a **critical** fail-open finding.
68+
- **Empty values:** An empty string, empty list, or zero-length array
69+
must be treated as "no entries allowed," not "all entries allowed."
70+
Code that skips validation when the list is empty is fail-open.
71+
- **Wildcard entries:** An allowlist containing `"*"` or `"all"` must
72+
be called out. If the wildcard is intentional, it still requires a
73+
finding (info severity) documenting the design choice. If it appears
74+
accidental or unjustified, it is **high** severity.
75+
- **Parse failures:** If config parsing fails (malformed JSON/YAML,
76+
invalid regex, type mismatch), the code must reject access rather
77+
than falling through to a permissive default.
78+
79+
**Rule of thumb:** If you can construct a scenario where removing or
80+
emptying a configuration value causes the system to grant broader access
81+
than when the value is correctly set, the code is fail-open and must be
82+
flagged.
83+
84+
## Permission manifest changes
85+
86+
**Category:** Use `permission-expansion` when permissions are added or
87+
broadened, `permission-reduction` when permissions are removed or narrowed.
88+
89+
If the diff modifies any file that declares or scopes permissions —
90+
GitHub App manifests, token downscoping maps, OAuth scope lists,
91+
IAM/RBAC policies, Kubernetes RBAC, or workflow `permissions:` blocks —
92+
always produce a finding, even if the change appears internally
93+
consistent. Evaluate:
94+
95+
(a) Does the new permission grant capabilities beyond the stated use
96+
case?
97+
(b) Is there a least-privilege alternative that achieves the same goal?
98+
(c) Is there a linked issue or ADR explicitly authorizing the expansion?
99+
100+
A permission expansion without explicit justification must be at least
101+
**high** severity. A reduction in permissions is still a finding (info)
102+
confirming the change is intentional.
103+
104+
Examples of permission-declaring files: GitHub App manifest JSON,
105+
`permissions:` blocks in `.github/workflows/*.yml`, token scoping maps,
106+
IAM policy JSON/YAML, Kubernetes `Role`/`ClusterRole` YAML.
107+
108+
## Workflow permission and role auditing
109+
110+
**Category:** Use `role-escalation` for role or token scope changes,
111+
`workflow-permission` for `permissions:` block changes, `secret-exposure`
112+
for secret handling issues.
113+
114+
When a diff modifies workflow files (`.github/workflows/*.yml`,
115+
reusable workflow definitions):
116+
117+
- **Role changes:** If a job, step, or reusable workflow call changes
118+
its role, token scope, or permission level (e.g., from a read-only
119+
role to a write role), produce a finding. Compare the old and new
120+
values explicitly. A role escalation without linked justification
121+
is **high** severity.
122+
- **`permissions:` blocks:** Any addition, removal, or modification of
123+
a `permissions:` block must be flagged. Evaluate whether the requested
124+
permissions follow the principle of least privilege for the workflow's
125+
stated purpose.
126+
- **`secrets:` blocks:** New secret references or changes to secret
127+
usage patterns must be reviewed. Verify that secrets are not exposed
128+
to untrusted contexts (e.g., pull_request_target workflows running
129+
fork code with access to secrets).

0 commit comments

Comments
 (0)