sysndd-security-bug-scan

name	sysndd-security-bug-scan
description	Use when reviewing or writing SysNDD code for security vulnerabilities or correctness bugs — authorization/role gates, SQL/expression injection, credential and secret handling, data exposure through public or MCP surfaces, resource-exhaustion/DoS from external calls, error/info leakage, and the repo's known R/Plumber footguns — especially before merging a diff or PR

SysNDD Security & Bug Scan

A focused defect + vulnerability review pass over a diff or PR. Distinct from sysndd-code-quality (maintainability) — this hunts for exploitable and correctness-breaking defects. It complements the generic /security-review and /code-review by encoding SysNDD's specific gates, helpers, and footguns.

Core insight: almost every SysNDD vulnerability is "bypassed a safe helper that already exists." For each finding, prefer the in-repo helper over a hand-rolled fix.

Workflow

1. Scope the diff. For each new/changed endpoint, DB query, external call, or auth path, run the relevant checks below.
2. Flag any hand-rolled code that a listed helper already covers.
3. Run the guard tests for the touched areas (see Verify) — a failing guard test usually means the change is wrong, not the guard.
4. Report findings by severity with file:line and the concrete in-repo fix; separate must-fix from optional.

Security Checks

Authorization & privilege escalation

• Every write / admin / curation endpoint must call require_role(req, res, "<Role>"). Auth is per-endpoint (no global deny filter) — a handler with no gate is reachable unauthenticated.
• direct_approval escalates the gate to Curator server-side; never trust the client flag (re-checked in svc_status_apply_direct_approval / review_apply_direct_approval). The frontend hasMinRole is UX, not a control.
• Attribution is server-set: status_user_id <- req$user_id. Never accept *_user_id from the request body.

Injection (SQL / expression / RCE)

• User filter / sort tokens must pass through validate_query_column() via generate_filter_expressions() / generate_sort_expressions() with allowed_columns_for_view(). Never paste0() or raw input into rlang::parse_exprs() — over a dbplyr tbl that is SQL injection and R-side RCE (local partial-eval runs system(...) etc.).
• Parameterize SQL with ? placeholders + DBI::dbBind(stmt, unname(params)). No string-interpolated values.
• URL-encode external path/query segments: utils::URLencode(x, reserved = TRUE).

Data exposure (public / MCP)

• MCP and public reads are approved-public only: reviews gated is_primary = 1 AND review_approved = 1; records from ndd_entity_view (active only). A query dropping these leaks draft/unapproved curation. See sysndd-mcp-readonly.
• MCP must not write, generate LLM summaries, or call live external providers.

Secrets & credentials

• Auth-sensitive inputs (/auth/signup, /auth/authenticate, password change) are JSON body only — never query string (leaks into access/Traefik logs, browser history).
• Log through sanitize_request() / sanitize_object() (SENSITIVE_FIELDS redaction in core/logging_sanitizer.R); never log tokens, passwords, or secrets.
• Do not bake config.yml / secrets into image layers (no COPY config.yml in api/Dockerfile; use the runtime mount).
• Passwords use Argon2id/sodium (hash_password / verify_password in core/security.R); never plaintext comparison.

Resource exhaustion / DoS

• Every external HTTP call derives its timeout/retry from external_proxy_budget() or make_external_request() — no hardcoded req_timeout(<n>) (enforced by test-unit-external-budget-guard.R). Wrap fetchers in memoise_external_success_only(source = "<provider>").
• Cheap routes (/health, /auth, /statistics) must never call an external fetcher (test-unit-cheap-route-isolation.R).
• Public expensive operations are throttled or cache-only (async submit cap; LLM generation is Curator+ only).

Error / info leakage

• Mount every endpoint sub-router via mount_endpoint() (attaches the RFC 9457 errorHandler + notFoundHandler). A bare plumber::pr_mount(...) leaks opaque 500s with internal detail and drops correct status codes. Throw classed errors (stop_for_bad_request / stop_for_unauthorized / …), not bare stop().

Correctness Footguns (SysNDD-specific)

• DBI::dbBind() with ? placeholders needs unname(params); named lists fail silently.
• dplyr::select / filter are masked (biomaRt/AnnotationDbi) — namespace them explicitly.
• config::get masks base::get in the loaded API/worker env — bare get(x, mode = "function") errors "unused argument (mode)"; use base::get or direct dispatch (test-unit-base-exists-get-guard.R).
• Plumber returns JSON scalars as arrays — unwrap before feeding back into params.
• Use inherits(x, "Date"), not is.Date().
• Worker-executed code is sourced at worker start — a change is not live until the worker restarts.

Verify

Run the guard tests for the touched areas, then make lint-api and the appropriate test lane:

test-unit-security.R, test-unit-filter-column-allowlist.R, test-unit-endpoint-error-handler.R, test-endpoint-auth.R / test-integration-auth.R, test-unit-*-endpoint-guard.R, test-unit-external-budget-guard.R, test-unit-cheap-route-isolation.R.

Lead with findings ordered by severity, each with file:line and the in-repo fix. If nothing is found, say so and list the checks run plus residual risk.