security-hardening

name	security-hardening
description	Run BEFORE writing or reviewing code that touches any of these dangerous surfaces in this repo, and BEFORE responding to a "fix the security issues / CodeQL alerts / secret-scan / code-scanning" request. Fires on: `eval(`/`exec(`/`compile(` on a string; `subprocess`/`os.system`/`os.popen` with a shell or a partial path; building a shell/SQL string with an f-string or `+`; `pickle`/ `yaml.load`/`torch.load`/`tarfile.extractall`/`zipfile.extractall` on untrusted data; `urllib`/ `requests` to a caller-influenced URL or without a timeout; `md5`/`sha1` used for a security decision; `tempfile` via a hardcoded `/tmp/...` path; `0.0.0.0` binds; `os.chmod` 0o777; a literal that looks like a credential; "fix all N security/quality issues", "CodeQL", "bandit", "ruff S". Teaches the SAFE alternative for each, AND the repo's known FALSE-POSITIVE patterns so a session never wastes time on noise or — worse — "fixes" intentional, gated code. No capability claim; `canClaimAGI` stays false.
metadata	{"short-description":"Avoid (and correctly triage) the code-scanning / bandit / ruff-S security findings this repo hits: real fixes + the false-positive catalog + the safe-pattern cheatsheet"}

Security hardening (this repo)

See also ci-artifact-drift (secret-scan / deps-audit are CI gates), sophia-agi (the no-overclaim contract), and skill-author (log a new footgun here when you hit one). This skill is about code security surfaces; leaked secrets (a key pasted into chat, committed .env) are an incident — rotate first, see remote-session-fallbacks / the stage-0 close-out.

0. First rule when asked to "fix all N security issues / CodeQL alerts"

A raw scanner count (CodeQL "92", ruff --select S "20 954") is mostly noise in THIS repo. Triage before you touch anything — a blind mass-fix is how you break a research codebase and "fix" a deliberately-gated verifier. The real, actionable findings are almost always the low-count, high-severity rules, not the 20k assert hits. Method:

1. Enumerate locally: ruff check --select S --statistics . (bandit is the CodeQL analogue; ruff's S/flake8-bandit rules overlap CodeQL's Python queries in root cause — not 1:1 in count).
2. Ignore the intentional-by-design bulk (see §3). Look at the rare rules: S102 exec, S307 eval, S105-107 hardcoded creds, S608 SQL, S103 perms, S324 weak hash, S104 bind-all, S301/S506 pickle/yaml.
3. Read each rare finding IN CONTEXT. Classify: real fix / gate-and-suppress (intentional but dangerous → keep + # noqa:<rule> with a justification) / false positive (suppress or rename). Only then edit.
4. Report honestly: "the N was dominated by false positives / intentional patterns; the genuinely real set was M; fixed M, documented the rest." Never claim "fixed all N" you did not verify.

1. The dangerous surfaces → the SAFE pattern (do this, not that)

Surface	❌ don't	✅ do
Evaluate an expression string	eval(expr) / exec(s) — even from a "trusted" template	a small ast-walk allowlist evaluator (numeric only), or sympy.sympify(expr, rational=True) for math. ast.literal_eval only parses literals — it will NOT evaluate a+b, so it is not a drop-in for arithmetic.
Run a subprocess	subprocess.run(cmd, shell=True), os.system(s), a bare "git" (partial path)	pass an argv list, shell=False (default), full or PATH-resolved exe; quote any interpolated value with shlex.quote. Build shell heredocs so operator values arrive via $ENV, never f-string-interpolated (the repo's pod-launcher pattern).
Build a shell/SQL command	f-string / + with a variable	argv list (shell) or a parameterized query (cur.execute(sql, params)) — never string-format SQL.
Deserialize	pickle.load, yaml.load(...) (full loader), torch.load(untrusted)	json, yaml.safe_load, torch.load(..., weights_only=True); never unpickle data you did not write.
Extract an archive	tarfile.extractall() / zipfile.extractall() on downloaded data	validate each member path stays under the target dir before extracting (path-traversal / zip-slip).
Fetch a URL	urllib.request.urlopen(url) with a caller-influenced url or no timeout	validate the scheme is https, pin/allowlist the host, always pass timeout=.
Hash for a security decision	hashlib.md5(x) / sha1 as an auth/integrity check	hashlib.sha256. (md5/sha1 for non-security content-addressing/dedup is fine — mark it usedforsecurity=False.)
Temp file	hardcoded /tmp/thing.json	tempfile.mkstemp / TemporaryDirectory() (unpredictable, cleaned up).
Bind a server	0.0.0.0 by default	bind 127.0.0.1 unless a remote bind is the deliberate, documented intent.
Permissions	os.chmod(p, 0o777)	least privilege: 0o600 files / 0o700 dirs; 0o755 only for an intentionally-executable script.
Randomness for tokens/secrets	random.random() (S311)	secrets / os.urandom. (random for sampling/seeds/data-gen is fine — that's the common false positive.)
Log/print an object that carries a secret	print(json.dumps(payload)) after "sanitizing" ONE field	build a SEPARATE display object in which the real secret never lands — see §1b. A copy-then-overwrite does not defeat object-level taint, and one-field redaction leaks every other secret.

1b. Clear-text logging of secrets (CodeQL py/clear-text-logging) — the sanitizer trap

The RunPod launchers print their create payload for the operator to eyeball before a paid pod spins up. The payload's env block carries secrets: RUNPOD_API_KEY and — critically — DEEPSEEK_API_KEY / LLMHUB_API_KEY that _build_create_payload forwards from os.environ (the GH-runner Actions secrets) with their REAL values on every call. The original "sanitized" print redacted only RUNPOD_API_KEY, so it silently leaked the forwarded keys. Lessons:

• A one-field redaction is worse than none — it reads as safe while leaking every other secret. Redact by coverage of all secrets in the object, not by the one field you happened to think of.
• Copy-then-overwrite does NOT satisfy CodeQL, and is genuinely fragile. CodeQL taint is object-level: once a real secret value enters an object (even a deepcopy), the sink is flagged even if you overwrite the field afterward. The reliable pattern (real safety and alert-clean) is to build a separate display object in which the real value never lands:
  • Pass a constant placeholder ("***", no data-dependence on the secret), not _redact(secret) — "***(len=N)" still derives from the secret (leaks length) and keeps CodeQL's taint edge alive.
  • Use a SEPARATE, standalone display builder — a redact_secrets=True flag on the SHARED builder does NOT work. CodeQL taint is context-insensitive: it analyses a function once, unioning every caller. If the same builder is called with the real key (for the API POST) and for the printed display payload, CodeQL sees the real-key → env → return → print edge regardless of any if redact_secrets branch inside it — the flag branch does not sever the union. The redact-flag fix was tried and FAILED (PR #851: it left the HIGH alert, and adding the flag to a second launcher took it 1→2 alerts). What works: a dedicated _display_create_payload(args) that never receives the api_key and never reads a secret value — it builds the non-secret shape from args, sets every secret env field to the constant _REDACTED_DISPLAY, and reads os.environ only as a presence boolean (if os.environ.get(_var):) so the log shows which vars forward without their value ever entering the object. In-tree reference: tools/runpod_rlvr.py::_display_create_payload (imported/reused by runpod_ocr_fabrication_bench.py; mirrored in runpod_crosslingual_consensus.py). _build_create_payload stays real-only and is POSTed, never logged.
  • No shared helper on the print path. Even factoring the common dict assembly into _assemble(args, env) re-introduces the alert (the real path taints env, the display path prints the shared return). Duplicate the ~15-line structure and pin it with a test that asserts display/real top-level key parity + the no-leak invariant (tests/test_runpod_display_payload.py) — the parity check is what catches drift from the duplication.
• The forwarding stays real — only the display copy is redacted; the pod still receives the live keys. Verify with a round-trip test: real payload keeps real values, display payload contains no substring of any secret (see the inline test that asserts "REAL…" not in json).
• Same rule for any log line: never print(f"...{api_key}..."); length-only (_redact) is a last resort for a scalar log, never for an object that reaches a taint sink.

2. Legitimate exec/eval — GATE, don't remove

This repo legitimately executes code (RLVR code-verifier, curriculum builder): candidate code is run to decide accepted/rejected. Do not rip that out to satisfy a scanner. Instead:

• Gate it behind SOPHIA_ALLOW_CODE_EXEC (the repo convention — os.environ.get( "SOPHIA_ALLOW_CODE_EXEC","0")); when unset, drop/skip the exec path (fail closed).
• Keep it offline, repo-authored input only — never exec network/model-generated code outside the sandboxed verifier path.
• Suppress the finding with a justification: # noqa: S102 — intentional gated code-verifier …. A bare # noqa with no reason is not acceptable; the reason is the audit trail.
• Example in-tree: tools/generate_math_code_curriculum.py (exec gated by SOPHIA_ALLOW_CODE_EXEC, # noqa: S102 with the rationale).

3. Known FALSE-POSITIVE patterns in THIS repo (do NOT "fix" these)

Editing these to appease a scanner adds churn and can break intent. Recognise them:

• S101 assert (~20k hits). This repo runs tests as python tests/test_*.py (script mode); assert is the test mechanism, and asserts appear in gate logic as invariants. Never strip them. Scope S101 out for tests/** in any lint config.
• S105/106/107 "hardcoded password". Almost all are verdict/label constants literally named PASS, VERDICT_PASS, SECRET_MARKER_TEXT, secret_label, or test fixtures ("secret_key"). Not credentials. If it trips a scanner and you can, prefer a name that doesn't read as a secret (VERDICT_ACCEPTED over VERDICT_PASS); otherwise suppress with a note. A real hardcoded secret is a rotate-now incident, not a code edit.
• S608 "SQL injection" on a shell heredoc. The RunPod pod-launchers build a bash script with an f-string (return f"""set -Eeuo pipefail …"""); ruff/CodeQL sometimes flag it as SQL. It is not SQL, and it is already injection-hardened (operator values arrive as $ENV, never interpolated). False positive — verify the "no f-string interpolation of untrusted values" invariant still holds, then leave it.
• S603/S607 subprocess / partial path in dev tooling that runs git, python, ssh on repo-controlled argv lists — fine; these are not shell=True and take no untrusted input.
• S311 non-crypto random in data generation / sampling / seeding — fine.
• PLR0124 / CodeQL py/comparison-of-identical-expressions (x != x, x == x). The idiomatic NaN test: x != x is True iff x is NaN; x == x is False iff NaN. Real, not a bug. When the operand is a known float, prefer math.isnan(x) — clearer and it clears the alert (different AST). In a test asserting "not NaN", assert not math.isnan(v). Do NOT "fix" it to a constant — that deletes the NaN guard. (Watch for the sibling tautology assert a == a where intent is determinism: assert two independent computations match, e.g. derive(x) == derive(x).)
• CodeQL py/implicit-string-concatenation-in-list / ruff ISC00x. Overwhelmingly a long message split across two source lines for line-length (one logical element inside a [...]/ {...}/call). That is intentional Python idiom, NOT a missing comma. A ruff/AST scan over-counts these into the hundreds; the real missing-comma bug is rare. Read the element: if the two fragments are one sentence/format-string, leave it. Only add a comma when the two fragments are clearly meant to be separate items. (Also: a deliberately-split struct.pack("<III" "Q" …) format string is one argument, not a list — leave it.)
• CodeQL py/empty-except (except Exception: pass). This repo has 200+, virtually all intentional: optional-dependency guards, best-effort cleanup, and fail-open paths that must never break a verified generation (each already carries a one-line reason + often # noqa: BLE001). Do NOT mass-add logging/re-raise — that changes fail-open to fail-closed and can break the safety path. Only touch one if it swallows an error on a load-bearing path with no comment.
• CodeQL py/hardcoded-credentials on a red-team CANARY/decoy (e.g. SECRET_MARKER_TEXT in tools/provision_sophia_fs_layout.py). The literal is a sentinel the airgap/exfil test writes to disk and asserts the sandbox can never read back — it MUST stay in source and MUST contain "SECRET". Not a credential. Recorded as a paths-ignore in .github/codeql/codeql-config.yml.
• CodeQL py/call-to-non-callable on a conditional torch stub (e.g. serving/photonic_complex.py). When a class is defined twice (if _TORCH_AVAILABLE: real nn.Module else a stub), CodeQL unions both definitions; a stub with __init__ but no __call__ makes layer(x) look non-callable at the guarded real call site. Fix in code: give the stub a matching __call__ (unreachable — __init__ already raises — so behaviour is unchanged) so both branches expose the same callable interface.

Where to record a reviewed false positive: .github/codeql/codeql-config.yml — per-file paths-ignore for a whole utility whose core purpose trips a detector (test corpora, the prompt- hygiene scanner, the wiki path-validator, the provision canary), or a repo-wide query-filters: exclude for a low-value recommendation query (e.g. py/cyclic-import). Every entry carries a multi-line justification — the comment IS the audit trail. Prefer a surgical code fix (make the type callable, use math.isnan) over a config exclusion; reach for the config only when the code is correct and the detector simply cannot model it.

4. Make it not recur — the enforceable gate

The skill teaches humans+AI; a lint gate enforces it. Recommended (not yet blocking — land with a grandfathered baseline so CI doesn't flood):

# high-signal security rules only; the intentional bulk scoped out
ruff check --select S102,S103,S104,S105,S106,S107,S307,S324,S608,S301,S506 \
  --per-file-ignores "tests/**:S101,S105,S106,S103" .

Wire this into .github/workflows/security.yml as a report-only step first; promote to blocking only after the real findings are at zero and a baseline is committed (the repo's pip-audit step is the model: "BLOCKING; triaged baseline grandfathered").

5. Rules

• Triage before you edit; never mass---fix a security ruleset on this repo.
• A real leaked secret → rotate first, then remove; never just delete the line and move on.
• Every suppression carries a one-line justification (the # noqa:<rule> — why).
• This is dev-workflow security hygiene: it makes no capability/AGI claim; canClaimAGI stays false.