From 2f8beb7f5d138255cb7ad50e9cb0bb11c6eea5d2 Mon Sep 17 00:00:00 2001 From: Tharindu Dharmarathna Date: Tue, 7 Jul 2026 10:37:21 +0530 Subject: [PATCH] include other claude rules --- .claude/rules/authentication_authorization.md | 347 +++++++++++++++++- .claude/rules/file-access.md | 56 +++ .../rules/js-authentication-authorization.md | 300 +++++++++++++++ .claude/rules/js-file-access.md | 58 +++ .claude/rules/js-output-encoding-xss.md | 158 ++++++++ .claude/rules/js-ssrf-prevention.md | 158 ++++++++ .claude/rules/js-xxe-xml-processing.md | 159 ++++++++ .claude/rules/ssrf-prevention.md | 207 +++++++++++ .claude/rules/xxe-xml-processing.md | 189 ++++++++++ 9 files changed, 1631 insertions(+), 1 deletion(-) create mode 100644 .claude/rules/js-output-encoding-xss.md create mode 100644 .claude/rules/js-ssrf-prevention.md create mode 100644 .claude/rules/js-xxe-xml-processing.md create mode 100644 .claude/rules/ssrf-prevention.md create mode 100644 .claude/rules/xxe-xml-processing.md diff --git a/.claude/rules/authentication_authorization.md b/.claude/rules/authentication_authorization.md index eb4c5d5e4..d12f4a250 100644 --- a/.claude/rules/authentication_authorization.md +++ b/.claude/rules/authentication_authorization.md @@ -314,4 +314,349 @@ method := strings.ToUpper(rdcRoute.Method) > * Is every `string(op.Method)`, `string(m)`, or equivalent extraction from a user-supplied typed method value wrapped in `strings.ToUpper()`? (If no, add normalization at the extraction site.) > * Are route keys generated for lookup and creation using the same normalized method string? (Inconsistent case between build and lookup sites causes silent key misses.) > * Does any Envoy `Exact:` header matcher for `:method` receive a value that may be lowercase? (If yes, apply `strings.ToUpper()` before passing to the matcher.) -> * Are access control deny-list or scope-registry map keys built from normalized strings? (A mixed-case key will silently bypass all deny/allow lookups keyed on the uppercase constant.) \ No newline at end of file +> * Are access control deny-list or scope-registry map keys built from normalized strings? (A mixed-case key will silently bypass all deny/allow lookups keyed on the uppercase constant.) + +--- + +## GO-AUTH-007: Deny-by-Default Authorization on Admin/System/Internal REST APIs + +### Severity + +Critical + +### Description + +Every Admin REST API, System REST API, or internal-only endpoint (e.g. `platform-api` operations backing the `ap` CLI, gateway-controller control-plane endpoints) must perform an explicit, per-endpoint scope/role check before executing. Network placement (bound to a private interface, reachable only from inside a cluster) and JWT algorithm allowlisting (GO-AUTH-002) are necessary but not sufficient — the handler itself must independently verify that the caller's token carries the specific administrative scope that endpoint requires. + +### Rationale + +Broken access control on admin and internal endpoints is one of the most severe and recurring security issues across API management platforms. The common failure is an internal/admin endpoint that assumes it is protected by something *outside* the handler — network topology, a shared upstream gateway, an assumed-trustworthy Key Manager — rather than checking the caller's actual granted scope itself. JWT algorithm confusion, self-registered users obtaining elevated tokens via shared Key Managers, low-privileged tokens accepted by admin APIs, and DCR endpoints issuing tokens without access control are all manifestations of this same root cause. + +### Non-Compliant Code + +```go +// ERROR: Assumes this handler is only reachable internally / by admins because +// it's registered under an "/internal" or "/admin" path prefix — no scope check +// inside the handler itself. A JWT bypass, misrouted proxy, or shared Key Manager +// misconfiguration upstream is enough to reach this code with any valid token. +func RotateApiKeyHandler(w http.ResponseWriter, r *http.Request) { + apiID := r.URL.Query().Get("api_id") + newKey, err := rotateApiKey(apiID) + if err != nil { + http.Error(w, "Internal Error", http.StatusInternalServerError) + return + } + json.NewEncoder(w).Encode(map[string]string{"api_key": newKey}) +} + +// ERROR: Checks that a token exists and is valid, but never checks that it +// carries the specific admin scope this System REST API requires. +func DeleteTenantHandler(w http.ResponseWriter, r *http.Request) { + claims, ok := r.Context().Value(ClaimsContextKey).(*Claims) + if !ok { + http.Error(w, "Unauthorized", http.StatusUnauthorized) + return + } + deleteTenant(r.URL.Query().Get("tenant_id")) // Any authenticated caller can reach this +} +``` + +### Compliant Code + +```go +// CORRECT: Explicit, per-endpoint scope check inside the handler — independent +// of network placement, upstream proxy assumptions, or which Key Manager issued +// the token. Deny-by-default: absence of the required scope is a 403, not a pass-through. +const ScopeAdminApiKeyRotate = "internal_apikey_rotate" + +func RotateApiKeyHandler(w http.ResponseWriter, r *http.Request) { + claims, ok := r.Context().Value(ClaimsContextKey).(*Claims) + if !ok { + writeUnauthorized(w) + return + } + if !claims.HasScope(ScopeAdminApiKeyRotate) { + // Generic 403 — do not reveal which scope was expected (aids scope probing). + http.Error(w, "Forbidden", http.StatusForbidden) + return + } + + apiID := r.URL.Query().Get("api_id") + newKey, err := rotateApiKey(apiID) + if err != nil { + logger.LogInternalError(r.Context(), "api key rotation failed: %v", err) + http.Error(w, "Internal Error", http.StatusInternalServerError) + return + } + json.NewEncoder(w).Encode(map[string]string{"api_key": newKey}) +} + +// CORRECT: A shared middleware that enforces the required scope for every +// route registered under it, so no individual handler can omit the check by +// mistake — but the handler above still re-checks explicitly for defense in depth. +func RequireScope(scope string, next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + claims, ok := r.Context().Value(ClaimsContextKey).(*Claims) + if !ok || !claims.HasScope(scope) { + http.Error(w, "Forbidden", http.StatusForbidden) + return + } + next.ServeHTTP(w, r) + }) +} +``` + +> **Verification Checklist before outputting code:** +> * Does an Admin/System/internal REST API handler check anything beyond "is this token valid" before executing a privileged operation? (If it only checks validity, add an explicit scope/role check for that specific operation.) +> * Is the authorization decision made inside the handler (or a middleware wrapping that specific route), rather than assumed from network placement, path prefix, or which upstream Key Manager/IdP issued the token? (If assumed, add an explicit in-handler check — token issuance source is not an authorization boundary.) +> * Does a Dynamic Client Registration, self-registration, or similarly "low trust" issuance flow ever produce a token capable of reaching a System/Admin REST API? (If yes, that issuance flow needs its own scope ceiling independent of downstream endpoint checks.) + +--- + +## GO-AUTH-008: Parameterized Queries for Administrative Data Access + +### Severity + +Critical + +### Description + +Every SQL query built from request input — most acutely in Admin REST API handlers backed by `sqlx`/`database/sql` over the project's SQLite (`go-sqlite3`) store — must use parameterized placeholders (`?` with `sqlx`/`database/sql`, or `sqlx.Named`/`sqlx.In` for dynamic `IN` clauses). Never build a query by string-concatenating or `fmt.Sprintf`-ing a request value into SQL text. + +### Rationale + +Authenticated SQL injection in Admin REST APIs is an exploitable bug class: an administrator manipulating database queries can exfiltrate data or disrupt availability. Administrative endpoints are not a lower-risk surface for injection just because the caller is already authenticated — an admin-scoped SQLi still crosses a trust boundary (reading/writing data the admin's own scope should not reach, or affecting availability platform-wide). + +### Non-Compliant Code + +```go +// ERROR: Request-supplied value concatenated directly into SQL text. +func SearchTenantsHandler(w http.ResponseWriter, r *http.Request) { + name := r.URL.Query().Get("name") + query := "SELECT id, name, status FROM tenants WHERE name LIKE '%" + name + "%'" + rows, err := db.Query(query) // name = "%' OR '1'='1" defeats the filter entirely + if err != nil { + http.Error(w, "Internal Error", http.StatusInternalServerError) + return + } + defer rows.Close() + // ... +} + +// ERROR: fmt.Sprintf into a dynamic ORDER BY / column list — still injectable +// even though it "looks like" metadata rather than a value. +func ListApisHandler(w http.ResponseWriter, r *http.Request) { + sortCol := r.URL.Query().Get("sort") + query := fmt.Sprintf("SELECT * FROM apis ORDER BY %s", sortCol) + db.Query(query) +} +``` + +### Compliant Code + +```go +// CORRECT: sqlx parameterized placeholder — the driver, not string formatting, +// handles escaping, so injected SQL metacharacters are inert data, never syntax. +func SearchTenantsHandler(w http.ResponseWriter, r *http.Request) { + name := r.URL.Query().Get("name") + var tenants []Tenant + err := db.Select(&tenants, + "SELECT id, name, status FROM tenants WHERE name LIKE ?", + "%"+name+"%", + ) + if err != nil { + logger.LogInternalError(r.Context(), "tenant search failed: %v", err) + http.Error(w, "Internal Error", http.StatusInternalServerError) + return + } + json.NewEncoder(w).Encode(tenants) +} + +// CORRECT: Dynamic sort/column selection resolved against an explicit allowlist, +// never interpolated into the query string — this is the one class of SQL +// injection parameterization cannot fix directly (identifiers aren't values). +var allowedSortColumns = map[string]string{ + "name": "name", + "created_at": "created_at", + "status": "status", +} + +func ListApisHandler(w http.ResponseWriter, r *http.Request) { + sortCol, ok := allowedSortColumns[r.URL.Query().Get("sort")] + if !ok { + sortCol = "created_at" // Safe default when the requested column isn't allowlisted + } + query := fmt.Sprintf("SELECT * FROM apis ORDER BY %s", sortCol) // sortCol is now a fixed, known-safe constant + var apis []Api + if err := db.Select(&apis, query); err != nil { + http.Error(w, "Internal Error", http.StatusInternalServerError) + return + } + json.NewEncoder(w).Encode(apis) +} +``` + +> **Verification Checklist before outputting code:** +> * Is any SQL query built with string concatenation, `+`, or `fmt.Sprintf`/`fmt.Sprintf`-style interpolation of a request-derived value? (If yes, rewrite using `?` placeholders via `sqlx`/`database/sql`.) +> * Does a dynamic `ORDER BY`, table name, or column list come from request input? (Placeholders cannot parameterize identifiers — resolve against an explicit allowlist map instead, never interpolate the raw value even after "validation.") +> * Is this query reachable from an Admin/System REST API handler? (Authenticated-admin-only reachability is not a reason to relax this directive — see GO-AUTH-007, which governs *who* can reach the handler; this directive governs how the handler must build its query regardless of who called it.) + +--- + +## GO-AUTH-009: Token and Session Invalidation on Security-State Change + +### Severity + +High + +### Description + +Whenever a security-relevant state change occurs — logout, account lock, password reset, role/permission change, sub-organization disablement, or user deletion — all previously issued access tokens, refresh tokens, and session-bound tokens tied to that identity must be actively revoked, not merely left to expire naturally. Checking current account status at authentication time is not sufficient if a token issued *before* the state change remains independently valid until its own expiry. + +### Rationale + +Failure to revoke tokens on security-state changes is a frequently recurring vulnerability pattern: session-bound tokens not revoked when a session ends, tokens issued before an account lock remaining valid after it, role removal failing to invalidate previously issued tokens, and stale authorization codes remaining usable after a user is deleted. In every case, the *authentication* check was correct at token-issuance time; the gap is that nothing re-validates or revokes the token when the security state that justified issuing it later changes. + +### Non-Compliant Code + +```go +// ERROR: Locks the account but does nothing to the tokens already issued to it — +// they remain valid (and continue to pass introspection) until natural expiry. +func LockAccountHandler(w http.ResponseWriter, r *http.Request) { + userID := r.URL.Query().Get("user_id") + if err := setAccountStatus(userID, StatusLocked); err != nil { + http.Error(w, "Internal Error", http.StatusInternalServerError) + return + } + w.WriteHeader(http.StatusNoContent) + // Missing: revoke all active tokens/sessions for userID +} + +// ERROR: Role change updates the stored role but issued tokens still carry +// (or are still authorized against) the old role until they expire on their own. +func UpdateUserRoleHandler(w http.ResponseWriter, r *http.Request) { + updateUserRole(r.URL.Query().Get("user_id"), r.URL.Query().Get("role")) + w.WriteHeader(http.StatusNoContent) +} +``` + +### Compliant Code + +```go +// CORRECT: Every security-state-changing operation revokes the affected +// identity's active tokens/sessions as part of the same operation, not as an +// afterthought or a separate manual step. +func LockAccountHandler(w http.ResponseWriter, r *http.Request) { + userID := r.URL.Query().Get("user_id") + if err := setAccountStatus(userID, StatusLocked); err != nil { + http.Error(w, "Internal Error", http.StatusInternalServerError) + return + } + if err := tokenStore.RevokeAllForUser(r.Context(), userID); err != nil { + // Revocation failure must not be silently ignored — the lock is + // incomplete without it. Log loudly and surface to the caller. + logger.LogInternalError(r.Context(), "failed to revoke tokens after lock for %s: %v", userID, err) + http.Error(w, "Internal Error", http.StatusInternalServerError) + return + } + w.WriteHeader(http.StatusNoContent) +} + +func UpdateUserRoleHandler(w http.ResponseWriter, r *http.Request) { + userID := r.URL.Query().Get("user_id") + newRole := r.URL.Query().Get("role") + + if err := updateUserRole(userID, newRole); err != nil { + http.Error(w, "Internal Error", http.StatusInternalServerError) + return + } + // Force re-authentication so the next issued token reflects the new role — + // do not let a token minted under the old role remain authoritative. + if err := tokenStore.RevokeAllForUser(r.Context(), userID); err != nil { + logger.LogInternalError(r.Context(), "failed to revoke tokens after role change for %s: %v", userID, err) + http.Error(w, "Internal Error", http.StatusInternalServerError) + return + } + w.WriteHeader(http.StatusNoContent) +} +``` + +> **Verification Checklist before outputting code:** +> * Does this handler change account status, role/permission, or delete a user/sub-organization, without also revoking that identity's active tokens/sessions? (If yes, add revocation as part of the same transaction/operation.) +> * Is account status checked only at authentication/token-issuance time, with no re-check for tokens already in circulation? (If a token's validity depends solely on its own signature/expiry with no live status check, add either revocation-on-change or a short-lived-token-plus-introspection pattern.) +> * On revocation failure, does the handler still return success to the caller? (Revocation failure must fail the overall operation — a lock/role-change that "succeeds" while old tokens remain valid is a silent authorization gap.) + +--- + +## GO-AUTH-010: Redirect and Callback URL Allowlisting (Open Redirect Prevention) + +### Severity + +Medium + +### Description + +Any server-generated HTTP redirect (`Location` header) whose target is derived, even partially, from request input — a post-login `returnTo`/`redirect_uri` parameter, an OAuth/OIDC callback URL, an SSO logout redirect — must be validated against an explicit allowlist of registered destinations before being used. Never redirect to a URL solely because it "looks like" it belongs to the same host (string-prefix or substring checks are bypassable). + +### Rationale + +Open redirect is a persistently recurring vulnerability class. Weak callback URL validation, unvalidated redirect construction, and open redirects in logout or account-recovery flows are consistently used as phishing primitives: the redirect originates from a trusted domain, which is exactly what makes it effective against users who "check the domain" before entering credentials on the destination page. + +### Non-Compliant Code + +```go +// ERROR: Redirects to whatever the caller supplies, with only a same-host +// substring check — bypassable via "https://trusted.example.com.attacker.com" +// or an open second-parameter trick like "https://trusted.example.com@attacker.com". +func LoginCallbackHandler(w http.ResponseWriter, r *http.Request) { + returnTo := r.URL.Query().Get("returnTo") + if strings.Contains(returnTo, "trusted.example.com") { + http.Redirect(w, r, returnTo, http.StatusFound) + return + } + http.Redirect(w, r, "/", http.StatusFound) +} +``` + +### Compliant Code + +```go +// CORRECT: Validated against an explicit, server-controlled allowlist of exact +// registered redirect targets — the same pattern OAuth/OIDC redirect_uri +// validation already requires, applied uniformly to every server-generated redirect. +var allowedRedirectHosts = map[string]bool{ + "portal.example.com": true, + "console.example.com": true, +} + +func safeRedirectTarget(raw string) (string, bool) { + if raw == "" { + return "/", true + } + u, err := url.Parse(raw) + if err != nil { + return "", false + } + // Relative paths are safe by construction — no host to validate. + if u.Host == "" && !strings.HasPrefix(raw, "//") { + return u.String(), true + } + if u.Scheme != "https" || !allowedRedirectHosts[u.Host] { + return "", false + } + return u.String(), true +} + +func LoginCallbackHandler(w http.ResponseWriter, r *http.Request) { + target, ok := safeRedirectTarget(r.URL.Query().Get("returnTo")) + if !ok { + target = "/" // Fall back to a safe default — never echo the rejected value back + } + http.Redirect(w, r, target, http.StatusFound) +} +``` + +> **Verification Checklist before outputting code:** +> * Is a redirect target validated with a substring/prefix check (`strings.Contains`, `strings.HasPrefix`) rather than a parsed-URL host comparison against an explicit allowlist? (Substring checks are bypassable — replace with `url.Parse` + exact host match.) +> * Does the validated redirect target allow scheme-relative URLs (`//attacker.com`) or a userinfo trick (`https://trusted.com@attacker.com`)? (Both parse as "same host" under naive checks — `safeRedirectTarget`-style explicit host comparison closes both.) +> * On rejection, does the handler redirect to a safe default rather than erroring in a way that reflects the rejected URL back into the response? (Reflecting it back risks reintroducing an XSS surface — see `js-output-encoding-xss.md` for the same principle in JS.) \ No newline at end of file diff --git a/.claude/rules/file-access.md b/.claude/rules/file-access.md index 90d21c183..8e43dd4dc 100644 --- a/.claude/rules/file-access.md +++ b/.claude/rules/file-access.md @@ -38,6 +38,13 @@ Apply this rule whenever writing, refactoring, or reviewing Go (`.go`) code that * **Externalize Limits to Configuration:** The byte ceiling must come from application configuration (environment variable, config file) — never hardcoded. Provide a safe default that is used when the configuration key is absent. * **Return a Meaningful Error on Overflow:** If the limit is hit, return `HTTP 413 Request Entity Too Large` with a generic message. Do not expose the configured limit value in the error response. +### 6. Uploaded Content Type Allowlisting and No Dynamic Code Execution on User Input + +* **Content-Sniff Before Trusting an Extension or Declared MIME Type:** Validate an uploaded file's actual byte content (magic-byte/structure sniffing) against an explicit allowlist of accepted types before storing or processing it — never trust the client-declared `Content-Type` header or the filename extension alone. +* **Never Feed User Input to a Script/Expression/Template Engine Without a Sandbox:** If a feature accepts a script, expression, or template body from a request (a policy mediator, a transformation rule, an "execute this snippet" feature), never `eval`-equivalent it against the full language runtime. Execute it only inside an engine configured with an explicit allowlist of reachable classes/methods/built-ins — a blocklist of "dangerous" symbols is not sufficient, because the reachable surface of a general-purpose runtime is too large to enumerate exhaustively. +* **Allowlist, Not Blocklist, for Reflective/Class Access:** Where a scripting or mediation feature must expose part of the Go runtime or a plugin API to user-supplied code, gate it with an explicit allowlist of permitted symbols. A blocklist approach only stops the specific bypasses already known at the time it was written. +* **Treat "Admin-Only" Script Features the Same as Any Other Untrusted Input:** A feature reachable only by an authenticated administrator is still processing untrusted input from this rule's perspective — the authorization boundary controls *who* can reach the feature, not whether the feature itself is safe to execute arbitrary code. + --- ## Code Examples for Enforcement @@ -73,6 +80,19 @@ func ExtractZip(src, destDir string) { io.Copy(out, rc) // No decompression ratio guard } } + +func AcceptUpload(w http.ResponseWriter, r *http.Request, upload []byte, declaredType string) { + // Trusts the client-declared Content-Type / extension with no byte-level check — + // a ".png" upload containing SVG/HTML/script content is stored and later served as-is. + saveUploadedFile(upload, declaredType) +} + +func RunScriptMediator(userScript string, ctx *MessageContext) error { + // Executes user-supplied script text against the full scripting-engine runtime + // with no allowlist of reachable classes/methods — arbitrary code execution. + engine := scripting.NewEngine() + return engine.Eval(userScript, ctx) +} ``` ### Best Practice (What to Generate) @@ -222,6 +242,40 @@ func ExtractSingleEntry(cfg FileConfig, zipData []byte, entryName, destDir strin } return fmt.Errorf("requested entry not found in archive") } + +// GOOD: Content-sniff the actual bytes against an explicit allowlist — never +// trust the client-declared Content-Type header or filename extension alone. +var allowedUploadTypes = map[string]bool{ + "image/png": true, + "image/jpeg": true, + // "image/svg+xml" deliberately excluded — SVG is XML and can carry + // executable content; see js-output-encoding-xss.md for the JS-side + // handling required if SVG upload is ever added. +} + +func AcceptUpload(w http.ResponseWriter, r *http.Request, upload []byte) error { + sniffed := http.DetectContentType(upload) // Inspects the actual bytes, not the header + if !allowedUploadTypes[sniffed] { + return fmt.Errorf("unsupported upload content type") + } + return saveUploadedFile(upload, sniffed) +} + +// GOOD: A scripting/mediation feature restricted to an explicit allowlist of +// reachable symbols — never the full runtime, and never gated by a blocklist +// of "known dangerous" symbols alone. +var allowedScriptSymbols = map[string]bool{ + "ctx.GetProperty": true, + "ctx.SetProperty": true, + "math.Round": true, + // Anything not explicitly listed here is unreachable from user scripts — + // e.g. no filesystem, network, process, or reflection access. +} + +func RunScriptMediator(userScript string, ctx *MessageContext) error { + engine := scripting.NewSandboxedEngine(allowedScriptSymbols) // Allowlist enforced by the engine itself + return engine.Eval(userScript, ctx) +} ``` --- @@ -232,3 +286,5 @@ func ExtractSingleEntry(cfg FileConfig, zipData []byte, entryName, destDir strin > * Is file processing done via `io.Reader` pipelines without intermediate `os.WriteFile` / `os.TempFile`? (If disk writes exist, remove them unless a third-party library strictly requires a path). > * Are all archive entries validated against the destination root before extraction? (If not, apply `safeJoin` on every entry). > * Is every inbound `io.Reader` wrapped in `io.LimitReader` with a limit sourced from configuration? (If hardcoded or absent, externalize to config with a safe default). +> * Is an uploaded file's actual content sniffed (`http.DetectContentType`) against an allowlist, rather than trusting the declared `Content-Type` header or filename extension? (If trusted as-is, add content-sniffing.) +> * Does any feature evaluate user-supplied script/expression/template text against the full scripting-engine runtime, or against a blocklist of "dangerous" symbols? (Both are insufficient — require an explicit allowlist of reachable classes/methods enforced by the engine itself.) diff --git a/.claude/rules/js-authentication-authorization.md b/.claude/rules/js-authentication-authorization.md index 78839fbad..726ceb51e 100644 --- a/.claude/rules/js-authentication-authorization.md +++ b/.claude/rules/js-authentication-authorization.md @@ -388,6 +388,305 @@ const method = normalizeMethod(userInput.method); // Normalized; safe for all d --- +## JS-AUTH-007: Deny-by-Default Authorization on Admin/Internal Routes + +### Severity + +Critical + +### Description + +Every admin-only or internal-only Express route must perform an explicit role/scope check inside its handler (or a middleware wrapping that specific router), independent of router-group placement (JS-AUTH-004) or JWT signature validity (JS-AUTH-002). Router scoping and valid-token checks establish *authentication*; they do not by themselves establish that this specific caller is *authorized* for this specific privileged operation. + +### Rationale + +The JavaScript counterpart of `authentication_authorization.md` GO-AUTH-007 — same exploit class, same fix, different syntax. This failure mode appears repeatedly at critical severity across API management platforms: JWT algorithm-confusion bypasses, self-registered users obtaining elevated tokens via shared Key Managers, unauthenticated access to System REST APIs, and registration endpoints issuing tokens without access control. In each case, a valid, correctly-signed token was treated as sufficient to reach a privileged operation, when it should only have been treated as sufficient to identify the caller. + +### Non-Compliant Code + +```js +// BAD: Router scoping (JS-AUTH-004) authenticates the caller, but the handler +// never checks that this specific authenticated user holds the admin role +// required for this specific operation. +const adminRouter = express.Router(); +adminRouter.use(authMiddleware); // Only proves the token is valid — not that the role fits +adminRouter.post('/tenants/:id/suspend', async (req, res, next) => { + await Tenant.update({ suspended: true }, { where: { id: req.params.id } }); + res.status(204).send(); +}); +app.use('/admin', adminRouter); +``` + +### Compliant Code + +```js +// GOOD: Explicit, per-route scope check inside the handler (or a small +// middleware factory) — deny-by-default, independent of router placement. +function requireScope(scope) { + return (req, res, next) => { + if (!req.user?.scopes?.includes(scope)) { + // Generic 403 — do not reveal which scope was expected. + return res.status(403).json({ error: 'forbidden' }); + } + next(); + }; +} + +const adminRouter = express.Router(); +adminRouter.use(authMiddleware); +adminRouter.post( + '/tenants/:id/suspend', + requireScope('admin:tenant:suspend'), // Explicit scope required for THIS operation + async (req, res, next) => { + try { + await Tenant.update({ suspended: true }, { where: { id: req.params.id } }); + res.status(204).send(); + } catch (err) { + next(err); + } + } +); +app.use('/admin', adminRouter); +``` + +> **Verification Checklist before outputting code:** +> * Does an admin/internal route rely solely on `authMiddleware`/router scoping (JS-AUTH-004) with no additional scope/role check for the specific operation? (If yes, add `requireScope`-style enforcement in the handler chain.) +> * Does a self-registration, Dynamic Client Registration, or other "low trust" flow ever mint a token whose scopes reach an admin route? (If yes, cap the issuable scopes for that flow independently of downstream route checks.) + +--- + +## JS-AUTH-008: Parameterized Sequelize Queries for Administrative Data Access + +### Severity + +Critical + +### Description + +Every Sequelize query built from request input must use parameter binding — `where` clause objects, `sequelize.escape`, or bound `replacements`/`bind` in `sequelize.query` — never raw template-literal interpolation of a request value into a `sequelize.query` string. + +### Rationale + +The JavaScript counterpart of GO-AUTH-008. Authenticated SQL injection in Admin REST APIs is an exploitable bug class: an administrator manipulating database queries can exfiltrate data or disrupt availability. Sequelize's query builder (`where: {...}`) already parameterizes automatically; the risk is entirely concentrated in `sequelize.query(rawSQL)` calls where a developer reaches for raw SQL (commonly for a dynamic sort/filter feature) and interpolates a value directly into the string. + +### Non-Compliant Code + +```js +// BAD: Request value interpolated directly into a raw SQL string. +async function searchTenantsHandler(req, res, next) { + const { name } = req.query; + const [tenants] = await sequelize.query( + `SELECT id, name, status FROM tenants WHERE name LIKE '%${name}%'` + ); // name = "%' OR '1'='1" defeats the filter entirely + res.json(tenants); +} + +// BAD: Dynamic sort column interpolated directly — still injectable even +// though it "looks like" metadata rather than a value. +async function listApisHandler(req, res, next) { + const sortCol = req.query.sort; + const [apis] = await sequelize.query(`SELECT * FROM apis ORDER BY ${sortCol}`); + res.json(apis); +} +``` + +### Compliant Code + +```js +// GOOD: Sequelize's query-builder `where` clause parameterizes automatically. +async function searchTenantsHandler(req, res, next) { + try { + const { name } = req.query; + const tenants = await Tenant.findAll({ + where: { name: { [Op.like]: `%${name}%` } }, // Bound, not interpolated + attributes: ['id', 'name', 'status'], + }); + res.json(tenants); + } catch (err) { + next(err); + } +} + +// GOOD: If raw SQL is genuinely required, use bound `replacements` — never +// template-literal interpolation of the value itself. +async function searchTenantsRawHandler(req, res, next) { + try { + const [tenants] = await sequelize.query( + 'SELECT id, name, status FROM tenants WHERE name LIKE :name', + { replacements: { name: `%${req.query.name}%` }, type: QueryTypes.SELECT } + ); + res.json(tenants); + } catch (err) { + next(err); + } +} + +// GOOD: Dynamic sort column resolved against an explicit allowlist — binding +// cannot parameterize an identifier, so this is the only safe pattern for it. +const ALLOWED_SORT_COLUMNS = new Set(['name', 'createdAt', 'status']); + +async function listApisHandler(req, res, next) { + try { + const sortCol = ALLOWED_SORT_COLUMNS.has(req.query.sort) ? req.query.sort : 'createdAt'; + const apis = await Api.findAll({ order: [[sortCol, 'ASC']] }); // sortCol is now a known-safe constant + res.json(apis); + } catch (err) { + next(err); + } +} +``` + +> **Verification Checklist before outputting code:** +> * Does any `sequelize.query()` call build its SQL string with a template literal or `+` concatenation of a request-derived value? (If yes, switch to the `where`-clause builder or bound `replacements`.) +> * Does a dynamic sort column, table name, or field list come from request input? (Binding cannot parameterize identifiers — resolve against `ALLOWED_SORT_COLUMNS`-style allowlist instead.) +> * Is this query reachable from an admin/internal route? (Authenticated-admin-only reachability is not a reason to relax this directive — JS-AUTH-007 governs *who* can reach the handler; this directive governs how the handler builds its query regardless of who called it.) + +--- + +## JS-AUTH-009: Token and Session Invalidation on Security-State Change + +### Severity + +High + +### Description + +Whenever a security-relevant state change occurs — logout, account lock, password reset, role change, or user/tenant deletion — all of that identity's active sessions and previously issued tokens must be actively revoked (destroy the Sequelize-backed session via `connect-session-sequelize`, and revoke/blacklist any outstanding JWTs), not merely left to expire naturally. + +### Rationale + +The JavaScript counterpart of GO-AUTH-009. Failure to revoke tokens on security-state changes is a recurring vulnerability pattern: session tokens not revoked when a session ends, tokens issued before a lock remaining valid after it, role removal not invalidating previously issued tokens, and tokens for users not revoked on password reset or disablement. In each case the authentication check was correct at issuance time; nothing revoked the token when the state that justified it later changed. + +### Non-Compliant Code + +```js +// BAD: Locks the account but leaves the session store and any issued JWTs +// untouched — both remain valid until natural expiry. +async function lockAccountHandler(req, res, next) { + try { + await User.update({ status: 'locked' }, { where: { id: req.params.userId } }); + res.status(204).send(); + // Missing: destroy active sessions / revoke issued tokens for this user + } catch (err) { + next(err); + } +} +``` + +### Compliant Code + +```js +// GOOD: Revocation is part of the same operation, not a separate manual step. +// Sessions are stored via connect-session-sequelize — destroy them for this user; +// JWTs are checked against a revocation store (e.g. a `tokenVersion` column bumped +// on every security-state change, checked during verification). +async function lockAccountHandler(req, res, next) { + try { + const user = await User.findByPk(req.params.userId); + if (!user) return res.status(404).json({ error: 'not_found' }); + + await sequelize.transaction(async (t) => { + await user.update({ status: 'locked', tokenVersion: user.tokenVersion + 1 }, { transaction: t }); + await Session.destroy({ where: { userId: user.id }, transaction: t }); // connect-session-sequelize table + }); + + res.status(204).send(); + } catch (err) { + logger.error('Failed to lock account and revoke sessions', { userId: req.params.userId, reason: err.message }); + next(err); + } +} + +// middleware/auth.js — GOOD: JWT verification checks the token's embedded +// tokenVersion against the current value, so a token minted before a lock/role +// change/password reset stops working immediately, not at its own expiry. +async function authMiddleware(req, res, next) { + try { + const { payload } = await jwtVerify(token, JWKS, { algorithms: ['RS256'] }); + const user = await User.findByPk(payload.sub); + if (!user || user.status === 'locked' || user.tokenVersion !== payload.tokenVersion) { + return res.status(401).json({ error: 'unauthorized', message: 'Invalid or expired credentials.' }); + } + req.user = { id: user.id, organizationId: user.organizationId, scopes: payload.scopes ?? [] }; + next(); + } catch (err) { + return res.status(401).json({ error: 'unauthorized', message: 'Invalid or expired credentials.' }); + } +} +``` + +> **Verification Checklist before outputting code:** +> * Does a handler change account status, role, or delete a user/tenant without also destroying that user's sessions and invalidating outstanding tokens (e.g. bumping `tokenVersion`)? (If yes, add revocation as part of the same transaction.) +> * Does `authMiddleware` validate a JWT's signature/expiry only, with no live check against the current account/token state? (If so, add a `tokenVersion`-style check so revocation actually takes effect before natural expiry.) +> * On a revocation/session-destroy failure, does the handler still respond with success? (It must not — treat revocation failure as an overall operation failure.) + +--- + +## JS-AUTH-010: Redirect and Callback URL Allowlisting (Open Redirect Prevention) + +### Severity + +Medium + +### Description + +Any `res.redirect()` call whose target is derived from request input — a post-login `returnTo` query parameter, an OIDC `redirect_uri`, a logout redirect — must be validated against an explicit allowlist of registered destinations before use. Never rely on a substring/prefix check against the request value. + +### Rationale + +The JavaScript counterpart of GO-AUTH-010. Open redirect via weak callback URL validation, unvalidated redirect construction, and open redirects in logout flows are consistently used as phishing primitives precisely because the redirect originates from a trusted, recognizable domain. + +### Non-Compliant Code + +```js +// BAD: Same-host substring check — bypassable via +// "https://portal.example.com.attacker.com" or "https://portal.example.com@attacker.com". +app.get('/auth/callback', (req, res) => { + const returnTo = req.query.returnTo || '/'; + if (returnTo.includes('portal.example.com')) { + return res.redirect(returnTo); + } + res.redirect('/'); +}); +``` + +### Compliant Code + +```js +// GOOD: Parsed-URL host comparison against an explicit allowlist — the same +// pattern OIDC redirect_uri validation already requires, applied uniformly. +const ALLOWED_REDIRECT_HOSTS = new Set(['portal.example.com', 'console.example.com']); + +function safeRedirectTarget(raw) { + if (!raw) return '/'; + let parsed; + try { + parsed = new URL(raw, 'https://portal.example.com'); // Base resolves relative paths safely + } catch { + return '/'; + } + // Reject scheme-relative ("//attacker.com") and userinfo tricks implicitly — + // URL parsing normalizes them, and the host check below still applies. + const isRelative = raw.startsWith('/') && !raw.startsWith('//'); + if (isRelative) return parsed.pathname + parsed.search; + if (parsed.protocol !== 'https:' || !ALLOWED_REDIRECT_HOSTS.has(parsed.host)) { + return '/'; // Fall back to a safe default — never echo the rejected value back + } + return parsed.toString(); +} + +app.get('/auth/callback', (req, res) => { + res.redirect(safeRedirectTarget(req.query.returnTo)); +}); +``` + +> **Verification Checklist before outputting code:** +> * Is a redirect target validated with `.includes()`/`.startsWith()` rather than parsing the URL and comparing `host` against an explicit allowlist? (Substring checks are bypassable — replace with `new URL()` + exact host match.) +> * Does the validated target allow a scheme-relative URL (`//attacker.com`) or userinfo trick (`https://trusted.com@attacker.com`) to pass? (Both must be rejected by the host-comparison logic, not assumed away.) +> * On rejection, does the handler redirect to a safe default rather than reflecting the rejected value back into an error page? (Reflecting it back risks reintroducing the XSS surface covered in `js-output-encoding-xss.md`.) + +--- + > **Verification Checklist before outputting code:** > * Does every authentication error branch have a `return` before `next()` or `res.status()`? (If no, add `return`). > * Does JWT verification include an explicit `algorithms` array containing only asymmetric algorithms? (If no, add the allowlist). @@ -395,3 +694,4 @@ const method = normalizeMethod(userInput.method); // Normalized; safe for all d > * Is route protection applied via Express router group scoping rather than raw `req.url` string matching? (If not, restructure to router groups). > * Does every Sequelize query for tenant-scoped data use `organizationId` from `req.user`, not from `req.query`/`req.body`/`req.params`? (If not, source it from `req.user`). > * Is every HTTP method string from user input normalized with `.toUpperCase()` before comparison, map/Set lookup, or policy registration? (If no, add normalization at the ingestion point.) +> * Does an admin/internal route check a specific scope/role beyond token validity (JS-AUTH-007)? Does every `sequelize.query()` call use bound `replacements` rather than string interpolation (JS-AUTH-008)? Does a lock/role-change/deletion handler revoke sessions and bump `tokenVersion` (JS-AUTH-009)? Is every `res.redirect()` target validated against a host allowlist (JS-AUTH-010)? diff --git a/.claude/rules/js-file-access.md b/.claude/rules/js-file-access.md index b66904be0..e158b6919 100644 --- a/.claude/rules/js-file-access.md +++ b/.claude/rules/js-file-access.md @@ -38,6 +38,13 @@ Apply this rule whenever writing, refactoring, or reviewing JavaScript (`.js`) c * **Externalize to the `DP_*` Config System:** Read the byte ceiling from the YAML config or `DP_MAX_UPLOAD_BYTES` / `DP_MAX_ZIP_ENTRIES` / `DP_MAX_ZIP_RATIO` environment variables. Apply a safe default when the key is absent. * **Return HTTP 413 on Overflow:** If multer or stream limits are exceeded, the error handler must respond with `HTTP 413 Request Entity Too Large` and a generic message. Do not echo the configured limit in the response body. +### 6. Uploaded Content Type Allowlisting and No Dynamic Code Execution on User Input + +* **Content-Sniff Before Trusting `mimetype`:** `req.file.mimetype` is client-declared and trivially spoofable. Validate the actual buffer content (e.g. via `file-type`'s `fileTypeFromBuffer`) against an explicit allowlist before storing or serving an upload — never key any decision solely on `req.file.mimetype` or the filename extension. +* **SVG Is Not "Just an Image":** SVG is XML and can carry `