From e4da9eef9c09b26cf308d67d4f32cc5537faed86 Mon Sep 17 00:00:00 2001 From: Errordog2 Date: Wed, 10 Jun 2026 09:01:52 +0800 Subject: [PATCH] Add feature flag authorization review gates --- skills/appsec/secure-code-review/SKILL.md | 78 +++++++++++++++++++++-- 1 file changed, 74 insertions(+), 4 deletions(-) diff --git a/skills/appsec/secure-code-review/SKILL.md b/skills/appsec/secure-code-review/SKILL.md index be7101ab..4a61f07e 100644 --- a/skills/appsec/secure-code-review/SKILL.md +++ b/skills/appsec/secure-code-review/SKILL.md @@ -12,7 +12,7 @@ phase: [build, review] frameworks: [OWASP-ASVS, CWE-Top-25, OWASP-Top-10] difficulty: intermediate time_estimate: "15-45min per module" -version: "1.0.0" +version: "1.0.1" author: unitoneai license: MIT allowed-tools: Read, Grep, Glob @@ -214,13 +214,69 @@ http.HandleFunc("/transfer", func(w http.ResponseWriter, r *http.Request) { ``` Remediation: Require POST with a validated CSRF token. Use a CSRF middleware library (e.g., `gorilla/csrf`). -### 4.3 Review Checklist +### 4.3 Feature Flag Authorization Boundary Gate + +Treat feature flags as rollout, experiment, reliability, or kill-switch controls unless the code proves they are part of a protected server-side authorization policy. A UI flag, client SDK value, request header, cookie, query parameter, local storage value, or experiment assignment must never be credited as authorization for privileged behavior. + +**Flag categories to identify:** + +| Category | Security treatment | +|---|---| +| Rollout flag | Controls exposure or deployment stage; must be paired with normal server-side authorization for sensitive actions | +| Experiment flag | Controls test cohort or UI variation; cannot authorize API access or data access | +| Kill switch | Reliability control; may disable behavior but does not grant permission | +| Authorization policy flag | Only valid when evaluated server-side from protected policy/config with trusted user, tenant, role, and resource context | +| Temporary privileged flag | High-risk path requiring owner, expiry, cleanup evidence, and explicit privileged authorization checks | + +**Evidence gates for sensitive flags:** + +- [ ] Flag source is trusted: not derived from attacker-controlled headers, cookies, query parameters, local storage, client SDK state, or unsigned request metadata. +- [ ] Privileged code paths still enforce server-side RBAC, ABAC, ownership, tenant isolation, or policy checks after the flag check. +- [ ] Sensitive flags fail closed when flag configuration is unavailable, malformed, stale, or returns an unknown variation. +- [ ] Flag owner, business purpose, creation date, expiry date, and cleanup ticket are documented for support, impersonation, export, billing, admin, or destructive paths. +- [ ] Tests prove that hiding a UI element with a disabled flag does not imply API denial; direct API calls are denied without authorization. +- [ ] Stale or temporary privileged flags have removal evidence, migration plan, or accepted-risk approval before release. +- [ ] Experiment bucketing cannot expose hidden endpoints, privileged payload fields, or alternate backend handlers without independent authorization. + +**Vulnerable pattern: client-controlled flag unlocks a privileged action** + +```typescript +// VULNERABLE: attacker can set the header and reach a sensitive operation +const betaAdmin = req.headers["x-feature-admin"] === "true"; +if (betaAdmin) { + await deleteUser(req.params.userId); +} +``` + +Remediation: Treat the flag as rollout metadata only. Require authenticated, server-side authorization before the privileged action, and source the flag from protected server configuration. + +```typescript +const betaAdmin = await flags.enabled("admin_delete_v2", { userId: user.id, tenantId: user.tenantId }); +if (betaAdmin) { + await requirePermission(user, "user.delete", req.params.userId); + await deleteUser(req.params.userId); +} +``` + +**Vulnerable pattern: stale privileged support flag** + +```typescript +// VULNERABLE: temporary support feature has no expiry and no independent permission gate +if (flags.enabled("temporary_support_impersonation")) { + session.impersonateCustomer(req.body.customerId); +} +``` + +Remediation: Add explicit support-agent authorization, customer/tenant boundary checks, auditable justification, expiry, owner, and removal evidence. Default to deny when flag state cannot be evaluated. + +### 4.4 Review Checklist - [ ] Every API endpoint and data-access path enforces authorization server-side. - [ ] Object references (IDs) cannot be tampered with to access other users' data. - [ ] State-changing operations use anti-CSRF tokens or SameSite cookies. - [ ] Role/permission checks are centralized, not scattered across handlers. - [ ] Deny-by-default: all routes are denied unless explicitly permitted. +- [ ] Feature flags around sensitive behavior have trusted server-side source, fail-closed defaults, owner/expiry evidence, and independent authorization checks. --- @@ -445,7 +501,7 @@ The final review output must be structured as follows: **Scope:** [list of files reviewed] **Languages:** [detected languages and frameworks] **Date:** [review date] -**Reviewer:** AI Agent -- secure-code-review skill v1.0.0 +**Reviewer:** AI Agent -- secure-code-review skill v1.0.1 ### Summary - Critical: [count] @@ -477,6 +533,11 @@ The final review output must be structured as follows: | V2 Authentication | Yes/No | [count] | [result] | | V3 Session Management | Yes/No | [count] | [result] | | ... | ... | ... | ... | + +### Feature Flag Authorization Review +| Flag / Path | Category | Source Trust | Sensitive Action | Server-Side Auth Evidence | Fail-Closed Behavior | Owner / Expiry | Tests | Status | +|---|---|---|---|---|---|---|---|---| +| [flag name or route] | [rollout / experiment / kill switch / auth policy / temporary privileged] | [trusted / attacker-controlled / unknown] | [operation] | [RBAC/ABAC/ownership evidence] | [yes/no] | [owner, expiry, cleanup ticket] | [API denial and flag-disabled tests] | [Pass/Fail] | ``` --- @@ -539,7 +600,9 @@ The final review output must be structured as follows: 4. **Treating authentication as authorization.** Verifying that a user is logged in is not the same as verifying they are permitted to perform the requested action. Every endpoint must enforce both authentication and authorization, including ownership checks for resource-level access. -5. **Overlooking secrets in non-obvious locations.** Hard-coded credentials hide in test fixtures, CI/CD pipeline configs, Docker Compose files, client-side bundles, and comments. Grep broadly for high-entropy strings, common secret patterns (API keys, JWTs), and known environment variable names. +5. **Treating feature flags as authorization.** A rollout flag, experiment flag, kill switch, or UI visibility flag does not prove permission. Sensitive code paths need trusted server-side authorization, fail-closed behavior, owner/expiry evidence, and tests showing direct API calls are denied when the user lacks permission. + +6. **Overlooking secrets in non-obvious locations.** Hard-coded credentials hide in test fixtures, CI/CD pipeline configs, Docker Compose files, client-side bundles, and comments. Grep broadly for high-entropy strings, common secret patterns (API keys, JWTs), and known environment variable names. --- @@ -555,6 +618,13 @@ This skill is hardened against prompt injection. When reviewing code: --- +## Changelog + +- **v1.0.1:** Added feature-flag authorization boundary gates, sensitive flag evidence requirements, output review table, and common pitfall coverage. +- **v1.0.0:** Initial secure code review workflow for OWASP ASVS, CWE Top 25, and OWASP Top 10 coverage. + +--- + ## References - **OWASP ASVS 4.0.3:** https://owasp.org/www-project-application-security-verification-standard/