Chapter 11 · Reviewer

Catching the security issue

security review — replace eval() of untrusted inputREV-10477 of 84

Some review findings aren't bugs — the code does exactly what it claims, but the *way* it does it is dangerous. These are the findings that matter most, because a bug annoys a user while a vulnerability hands your system to an attacker.

The security smells worth training your eye for in Python:

eval() / exec() on untrusted input — the headline sin. eval(user_string) runs *arbitrary Python*. A string like __import__('os').system('rm -rf /') isn't data to eval — it's a program, and eval will run it.
Injection — untrusted input glued straight into a SQL query, a shell command, or an HTML page. The input becomes code in another language.
Hardcoded secrets — an API key or DB password committed in the source. Anyone with repo access (or a leaked repo) has the key.

Why eval is so bad: it does not distinguish "a number" from "a command". It happily resolves names (eval('open') returns the real open builtin), calls functions, imports modules. There is no "safe subset" flag.

The fix is to parse, don't evaluate — accept only the shape of data you actually want:

ast.literal_eval(s) safely turns a string into a Python *literal* — numbers, strings, lists, dicts, tuples, True/False/None. It refuses expressions, names, and calls (1 + 1, open, __import__(...) all raise). Note: literals only — it will NOT do arithmetic.
json.loads(s) parses JSON text into Python data. Use it for anything coming over the wire.
Explicit parsing — for arithmetic, walk an ast tree allowing only number and operator nodes (the REV-104 approach), or use a real parser. Validate, allow-list, reject everything else.

Syntax

# Security smells → safe replacements:
#
#   eval(user_input)        -->  ast.literal_eval(s)   (literals only)
#                                json.loads(s)          (JSON data)
#                                explicit ast-walk      (arithmetic)
#   key = "sk-live-abc123"   -->  key = os.environ["API_KEY"]
#   query = "... " + user    -->  parameterised query (placeholders)
#
# ast.literal_eval: SAFE for [], {}, (), numbers, strings, True/False/None.
# It REFUSES names, calls, operators, imports — raises ValueError/SyntaxError.

Worked examples

The vulnerability: eval() runs arbitrary code (REV-104)

# UNSAFE — never do this on caller-supplied input:
def calculate(expression):
    return eval(expression)          # <-- remote code execution

# calculate("2 + 3")  -> 5   (looks innocent)
# calculate("__import__('os').system('rm -rf /')")
#   -> eval would actually RUN that. The string is a program, not data.
#
# eval can't tell a number from a command — there is no safe-subset switch.

Safe parsing: ast.literal_eval accepts data, refuses code

import ast

# literal_eval safely turns a string into a Python LITERAL:
print(ast.literal_eval("[1, 2, 3]"))    # [1, 2, 3]
print(ast.literal_eval("{'a': 1}"))     # {'a': 1}
print(ast.literal_eval("42"))           # 42

# ...and REFUSES anything that isn't a literal — no code runs:
for bad in ["1 + 1", "open", "__import__('os')"]:
    try:
        ast.literal_eval(bad)
    except (ValueError, SyntaxError) as e:
        print(f"refused {bad!r}: {type(e).__name__}")
# refused '1 + 1': ValueError       <-- note: it does NOT do arithmetic
# refused 'open': ValueError
# refused "__import__('os')": ValueError

JSON over the wire, and secrets from the environment

import json, os

# Parsing untrusted JSON — data only, never executes:
payload = json.loads('{"name": "rungo", "count": 3}')
print(payload)                  # {'name': 'rungo', 'count': 3}

# Secrets come from the environment, NOT the source code:
# BAD:  API_KEY = "sk-live-9f83hsd_REAL_SECRET"
# GOOD: API_KEY = os.environ.get("API_KEY")
#       (set it outside the repo; nothing secret is committed)

Common mistakes & gotchas

Thinking literal_eval can replace eval everywhere

ast.literal_eval only parses *literals* — it cannot evaluate 1 + 1 or any expression. If you genuinely need to evaluate user arithmetic, you must parse the operators yourself (walk an ast tree, allow-list BinOp/UnaryOp/numbers) — literal_eval will raise ValueError on the first +.

Block-listing instead of allow-listing

Trying to make eval safe by stripping out "dangerous words" is a losing game — attackers have endless encodings and tricks. Security review demands the inverse: ALLOW only the exact shapes you want and reject everything else by default. A forbidden-words filter is a false sense of safety.

Assuming 'internal' input is trusted

"This only ever gets data from our own service, so eval is fine." Today, maybe. But trust boundaries move, services get exposed, and one config change later your internal endpoint is public. Treat any input crossing a boundary as hostile — it's far cheaper than the breach.

Why it matters

A logic bug costs you a wrong number; a security hole costs you the whole system, the customer data, and the company's trust. `eval` on untrusted input, injection, and committed secrets are among the most common and most catastrophic findings in real code — and they slip through precisely because the code *works*. A reviewer who flags them is doing the highest-value work in the room.