Symfony Security Review

Symfony Security Review

Finds hardening that is missing or wrong, grounded in code. It runs in two modes:

• Review a change (default): a PR, the current branch vs its base, or named files.
• Audit a target: a component path or the whole src/ tree, for one or all families.

Both modes run two passes: first reason about the change's trust boundaries from first principles (to catch novel issues), then check it against the hardening-invariant families catalogued in hardening-families.md. The catalogue is a checklist of known classes, not the search space.

Scope and non-goals

• This skill spots missing hardening. It does not triage an incoming report end to end, assign a CVE, or merge a fix. Those are separate workflows.
• Every finding must point at a real sink (a concrete file and line). No speculative findings. A family with no sink in scope is not in scope; but a sink that matches no family is still in scope (that is what Step 1 is for).
• Respect the decision boundaries in the catalog: several plausible-looking shapes were ruled "not a security issue". Do not raise those.

Progress checklist

• Step 0: Pick mode and resolve the scope
• Step 1: First-principles boundary pass (open-world)
• Step 2: Map to families (known-class checklist)
• Step 3: Run the automated gates
• Step 4: Manual per-family review
• Step 5: Report findings

Confirmation rule

Whenever this skill says "Wait for confirmation", treat anything other than an explicit affirmative as no: stop and ask the user how they want to proceed.

Parallelism (large audits only)

When auditing the whole src/ tree or a large multi-file component, fan Step 1 and Step 4 out to subagents: one per family, or one per file-batch, each returning findings that point at a concrete file and line (no speculative findings leaking in through parallelism). Keep the rest in the main loop: aggregation, de-duplication, the completeness check, and the report are synthesis and must see every finding at once. For a single PR or a small diff, run every step inline; subagents are pure overhead there.

Step 0 — Pick mode and resolve the scope

Review a change. Resolve the diff and the changed non-test files:

# A public PR
gh pr diff <n> --repo symfony/symfony
# The current branch against its base (e.g. 6.4)
git diff <base>...HEAD --stat

Audit a target. Take a component path (e.g. src/Symfony/Component/Mailer) or the whole src/ tree. There is no diff; the "changed files" are the target files.

In both modes, build the file list and drop tests (*/Tests/*). Hardening lives in the implementation; tests are checked separately in Step 3.

State the resolved mode and scope back to the user in one line before continuing.

Step 1 — First-principles boundary pass (open-world)

This pass finds what the catalogue does not list. Do it before mapping to families, and do not let the anchors narrow it. For each file in scope, reason as an attacker, independent of any known family:

1. Inputs: what untrusted data can reach this code? Request (query/body/headers/cookies), uploaded files, environment, a broker/message payload, a stored blob later deserialized, a webhook, a third-party API response, a database value that was originally user-set.
2. Flow: follow each input to where it is used. Does it reach a sink (filesystem, process, SQL, deserialization, object construction, HTML output, a redirect, an XML/URL parser, a comparison of a secret)?
3. Boundary: which trust boundary does it cross, and who is the expected actor (unauthenticated, lower-privileged, a malicious broker)?
4. Worst case: if the attacker fully controls the input, what is the maximum impact? State it concretely (RCE, auth bypass, SSRF, file read/write, XSS, info leak).

Record every input-to-sink path with a non-trivial worst case as a candidate finding, whether or not it matches a catalogued family. An unguarded path from untrusted input to a dangerous sink is a finding even if no anchor names it. This step uses no greps by design; it is meant to see sinks the dictionary misses.

Step 2 — Map to families (known-class checklist)

Now apply the catalogue as a checklist, to confirm no known class slipped past Step 1. Treat each anchor's listed APIs as seed examples of an abstract role (an inbound authenticator, a secret comparison, a deserialization entry, an XML/URL sink, a file or process sink); extend to anything in scope that plays that role, including classes the grep does not name.

For each file in scope, decide which families it touches using the grep anchors in hardening-families.md. Run the anchors against the scope, not the whole tree, when reviewing a change:

# Example: which families does this branch touch?
git diff <base>...HEAD --name-only | grep -v /Tests/ > /tmp/scope.txt
grep -lf <(printf 'extends AbstractRequestParser\nfunction __unserialize\nhash_hmac(\nvalidateOnParse\nloadXML(\nescapeshellarg(\npreg_match') $(cat /tmp/scope.txt) 2>/dev/null

Carry forward both Step 1's boundary findings and the families with a sink in scope; they proceed to Step 4. List them.

Step 3 — Run the automated gates

These encode the already-shipped invariants. Run them first; anything they catch needs no manual argument.

Hardening-test convention (tokenizer only, always runs locally):

php .github/sa-tools/check-hardening-tests.php

Fails if a concrete AbstractRequestParser lacks a RejectWebhookException test, or a class with __unserialize() and a string property lacks a __toString gadget test. Accepted gaps live in its ALLOWLIST const.

Custom PHPStan rules (HardenedComparisonRule, UnserializeToStringTrampolineRule, UnserializeMissingAllowedClassesRule). In CI these run base-vs-PR through phpstan-diff.php, which only fails on errors new to the PR. To reproduce locally you need PHPStan installed (it is not in composer.json; CI installs it ad hoc). The source of truth for a rule's logic is a RuleTestCase, not an ad-hoc phpstan analyse run:

# Only meaningful if phpstan is installed in the project
./vendor/bin/phpstan analyse --error-format=json --no-progress \
  --autoload-file=.github/sa-tools/rules/bootstrap.php

Do not trust raw phpstan analyse counts for these rules: the result cache plus parallel workers make them nondeterministic (a rule can report 0 then N for the same input). See the gotchas.