From 50585fb7c867a94a3dc718cef847ddb2cceead7f Mon Sep 17 00:00:00 2001 From: Cody Maffucci <46459665+Maffooch@users.noreply.github.com> Date: Thu, 20 Aug 2026 09:01:36 -0600 Subject: [PATCH 1/2] chore(skills): add Claude Code dev/review and parser skills Add two repo-scoped Claude Code skills under .claude/skills/, plus a pointer to them from AGENTS.md: - defectdojo-dev: the local dev/test loop (bring the Docker stack up on localhost:8080, reproduce a bug on the target branch before fixing, write behavioral unit tests, drive the UI via Playwright, fetch an API token) that doubles as an inbound PR reviewer with concern lenses (scalability, performance, memory, DB resourcing, query design, security, DRF serializer exposure) and an infra/Helm review checklist. Helpers: get-api-token.sh, run-tests.sh. - defectdojo-parser: authoring and reviewing scan-report parsers to the project's conventions (factory contract, dedup registration and its impact on existing data, defusedxml/utf-8/Endpoint.from_uri, the 0/1/many test set, sample-file sanitization/size, and the unittests/test_parsers.py meta-test). Helper: new-parser-checklist.sh. Un-ignore .claude/skills/ in .gitignore (same negation pattern already used for .claude/hooks/) so the skills are shared in the repo. Co-Authored-By: Claude Opus 4.8 --- .claude/skills/defectdojo-dev/SKILL.md | 190 ++++++++++++++ .../skills/defectdojo-dev/get-api-token.sh | 50 ++++ .claude/skills/defectdojo-dev/run-tests.sh | 45 ++++ .claude/skills/defectdojo-parser/SKILL.md | 237 ++++++++++++++++++ .../defectdojo-parser/new-parser-checklist.sh | 120 +++++++++ .gitignore | 1 + AGENTS.md | 20 ++ 7 files changed, 663 insertions(+) create mode 100644 .claude/skills/defectdojo-dev/SKILL.md create mode 100755 .claude/skills/defectdojo-dev/get-api-token.sh create mode 100755 .claude/skills/defectdojo-dev/run-tests.sh create mode 100644 .claude/skills/defectdojo-parser/SKILL.md create mode 100755 .claude/skills/defectdojo-parser/new-parser-checklist.sh diff --git a/.claude/skills/defectdojo-dev/SKILL.md b/.claude/skills/defectdojo-dev/SKILL.md new file mode 100644 index 00000000000..4824a4a1fa8 --- /dev/null +++ b/.claude/skills/defectdojo-dev/SKILL.md @@ -0,0 +1,190 @@ +--- +name: defectdojo-dev +description: Develop, test, and validate DefectDojo changes end to end against a local Docker stack — bring the app up on localhost:8080, reproduce a bug on the target branch before fixing it, write behavioral unit tests that fail without the fix, drive the UI with the Playwright MCP, and fetch an API token to exercise the REST API. The same review lenses (scalability, performance, memory, DB resourcing, query design, security, DRF serializer exposure) let it double as an inbound PR reviewer, with a dedicated checklist for infra/Helm/deployment PRs. Use when developing or testing a change, reproducing or fixing a bug, writing a regression test, validating a fix, or reviewing any DefectDojo PR/branch (app, API, or Helm chart). +--- + +# Develop, test, and review DefectDojo changes + +This is the primary workflow for building and verifying a change in this repo, and +secondarily for reviewing someone else's PR. DefectDojo is a Django app (server-rendered +templates, not a SPA) run via Docker Compose, with a Postgres DB and a Valkey broker. Work +the loop below against a **running local stack** — do not reason about behavior from the +code alone when you can exercise it. + +Read `AGENTS.md` first for the branch/release-line policy: bug fixes target `bugfix`, +features target `dev`, and `master` is off-limits without explicit confirmation (the +`.claude/hooks/branch-guard.sh` hook enforces this). Put the work on the right branch +before editing. + +## Inputs + +- **What you're working on** (one of): a change/feature you're building, a bug or issue to + reproduce and fix, or a PR number / branch / URL to review. +- **Focus area** (optional): a specific concern lens to emphasize (e.g. "just the query + performance", "security only"). + +## Helper scripts + +Both live in this skill directory. `chmod +x` them once if needed. + +- **`get-api-token.sh`** — fetches a REST API token so you can test API endpoints. + `POST`s to `/api/v2/api-token-auth/` with a username/password and prints the raw token. + Defaults: user `admin`, password `admin`, base URL `http://localhost:8080` (override with + `DD_USER` / `DD_PASSWORD` / `DD_BASE_URL`). Use the token as `Authorization: Token `. + If token auth is disabled, get one from the UI at `/api/key-v2` instead. +- **`run-tests.sh`** — thin wrapper over the repo's sanctioned `./run-unittest.sh` that tees + output to a log. Pass a fully-qualified test target: + `./run-tests.sh unittests.tools.test_acunetix_parser.TestAcunetixParser`. Requires the dev + stack to be up. Do **not** call `pytest` or `manage.py test` directly — the wrapper is the + supported path (runs `--keepdb -v2`, checks compose first). + +## Steps + +1. **Bring the stack up (dev mode).** Dev mode is what gives you `admin`/`admin` and + hot-reload: + ```bash + ./docker/setEnv.sh dev + docker compose up -d + ``` + The UI is at `http://localhost:8080` (login `/login`). Creds are `admin`/`admin` **in dev + mode only**. If someone ran a plain `docker compose up` instead, the admin password is + random — read it with `docker compose logs initializer | grep "Admin password:"`, or + reset via `docker compose exec uwsgi ./manage.py changepassword admin`. + +2. **Reproduce first (bugs).** Before changing anything, prove the bug exists **on the + target/base branch** (the one the fix will land on). Capture the concrete failure — a + screenshot, a stack trace, a wrong value, a 500. Reproduce through the real surface: + - **UI:** drive it with the Playwright MCP — navigate to `http://localhost:8080/login`, + log in, and walk the exact flow. Expect full-page navigations and CSRF-protected forms + (hidden `csrfmiddlewaretoken` inputs); auth is a session cookie, so there's no + client-side router to wait on. + - **API:** grab a token with `get-api-token.sh` and hit `/api/v2/...` with + `Authorization: Token `. + A bug you cannot reproduce is not yet understood — say so rather than guessing at a fix. + +3. **Develop / apply the change.** Make the fix or feature on the correct branch. The dev + stack bind-mounts the source with autoreload, so edits to Python are picked up live — + re-exercise the same UI/API path from step 2 to confirm the behavior actually changed. + +4. **Write behavioral unit tests.** Every fix gets a test that **fails without the change and + passes with it** — that's what stops the regression from coming back. Prefer asserting on + observable behavior (returned values, DB state, response codes, finding counts/attributes) + over implementation details or line coverage. Tests live under `unittests/` + (`unittests/tools/` for parsers). Run them the sanctioned way: + ```bash + ./run-tests.sh unittests.. + ``` + Confirm the test is real by checking it **fails on the pre-fix code** (stash the fix, run, + see red), then passes with the fix. + + For a **query-count / performance** change, the guard is + `unittests/test_importers_performance.py`, which asserts exact query and task counts. If + your change legitimately shifts those counts, regenerate them with + `python scripts/update_performance_test_counts.py` (add `--verify` to check) and commit the + result — don't hand-edit the expected numbers. A prime way to prove a query fix is to assert + the generated SQL no longer contains the offending `LEFT OUTER JOIN` / `GROUP BY` (see + `build_count_subquery` in `dojo/query_utils.py`, the correlated-subquery pattern the product + list views use to avoid GROUP-BY fan-out). + +5. **Self-check with the concern lenses.** Before calling the change done, run it through + each lens below. These are the axes that matter in this codebase — map each finding to a + concrete line: + - **Scalability / query design:** N+1 queries and missing `select_related` / + `prefetch_related`; unbounded querysets materialized with `list()` or iterated in full; + `.count()` or queries inside loops; filtering/ordering on unindexed columns; work that + grows with the number of findings/products (DefectDojo instances routinely hold millions + of findings). + - **Performance:** synchronous work that belongs in a Celery task; repeated recomputation + that should be cached; expensive serialization on hot paths. + - **Memory:** reading an entire uploaded scan file into memory at once; loading a whole + queryset (or a full serialized payload) into RAM instead of streaming/paginating; large + in-memory dedup structures. + - **DB resourcing / migrations:** schema migrations that lock or rewrite large tables; + missing indexes for new query patterns; data migrations that run row-by-row in the + request/boot path; migrations that aren't reversible. Never edit an existing migration — + add a new one, and commit `dojo/db_migrations/max_migration.txt` (django-linear-migrations). + - **Security:** authorization checks (`user_has_permission` / the authorization decorators) + on every new view/endpoint — DefectDojo is multi-tenant, so an object read/write must be + scoped to the requesting user's products; injection (raw SQL, `format`/f-strings into + queries); SSRF and XML entity expansion (parsers must use `defusedxml`, never `lxml`). + - **Serializer exposure (DRF):** mass-assignment and secret leakage both hide in the + `Meta`. `fields = "__all__"` **and** `exclude = (...)` are equally risky — both + auto-expose any *new* model field, so `exclude` is not automatically safe; check what a + new field would surface. Credential-adjacent fields (API keys, tokens, the linked config + object's secret) must be `write_only=True` so they're never serialized into a GET + response, and related-object querysets on writable fields should be scoped to what the + requesting user may reference. Confirm no secret is echoed back and nothing sensitive + lands in logs. + +6. **Run tests + guards.** Run the affected test module(s), plus the guards CI enforces so a + green local run matches CI: + ```bash + docker compose exec uwsgi python manage.py makemigrations --check --dry-run + docker compose exec uwsgi python manage.py spectacular --fail-on-warn + ``` + (The first fails if you changed a model without a migration; the second fails if an API + change broke the OpenAPI schema.) + +7. **Reviewer mode (secondary).** When the input is someone else's PR rather than your own + change: + - `gh pr view --json title,body,headRefName,files` and `gh pr diff ` to understand + the change and its blast radius; check the target branch matches the release-line policy. + - `gh pr checkout `, bring the stack up, and apply **step 2** (reproduce the bug the PR + claims to fix on the base branch, confirm it's gone on the PR branch) and **step 5** (run + the concern lenses over the diff). Confirm the PR includes a behavioral test per step 4; + if it doesn't, that's a finding. + - **Read CI correctly.** The authoritative signals are the green GitHub Actions checks + (`gh pr checks `) and the real `gh pr diff ` (base...head). Third-party bot comments + (e.g. DryRun "sensitive codepath modified by non-allowed author") are advisory and are + often dismissed by maintainers as false positives — a huge "40+ sensitive files" wall is + usually a **rewritten/recreated branch-history artifact**, not a real change. Verify against + the actual diff; if it's 3 files, review 3 files. Also check that the **heavy suites + actually ran** — if only lightweight jobs (autolabeler, analyzers) executed and Unit Tests + / test-rest-framework never imported the code, a green board doesn't mean the code even + imports (a missing import is invisible until the real suite runs). + - **Report a severity-ranked findings summary to the user first.** Do not post to the PR + automatically. Once the user approves, posting inline PR comments (e.g. via + `/code-review --comment` or `gh`) is an explicit opt-in follow-up. + +## Reviewing infra / Helm / deployment PRs + +A recurring PR category touches the Helm chart (`helm/defectdojo/`), nginx config +(`nginx/`), or Docker entrypoints (`docker/`) rather than Django code. The concern lenses +still apply (security defaults, backward compatibility), but the checks are different: + +- **The branch/release-line policy applies to chart and docker PRs too** — they are not + exempt. A fix still targets `bugfix`, a feature `dev`, never `master`. Defer to `AGENTS.md`. +- **Know the three Helm CI jobs** (`.github/workflows/test-helm-chart.yml`) — each is an + automatic blocker when it fails: + - **`Lint chart (version)`** includes an **`artifacthub.io/changes` annotation check**: it + fails on *any* chart change whose `helm/defectdojo/Chart.yaml` annotation wasn't updated + versus the target branch. A chart PR with no new changelog annotation entry is red until + fixed (this is the single most common chart-PR CI failure). + - **`Update schema`** regenerates `values.schema.json` and fails on diff — the schema must be + **generator-produced, not hand-edited**. A hand-edited schema is an automatic request-change. + - **`Update documentation`** runs `helm-docs` — `README.md` must be regenerated from + `values.yaml`, not written by hand. +- **New features must be opt-in and default-off.** Gate every new resource behind + `{{- if .Values.X.enabled }}` with `enabled: false` by default, so `helm template` on defaults + renders nothing new and existing installs are untouched. A new env/config default that applies + to *all* installs (e.g. forcing `LC_ALL`) is a behavior change — flag it and make it overridable. +- **Template-correctness spot checks:** confirm `backendRefs`/service references point at real + service names and ports; watch list-vs-map YAML rendering from a conditional `-` in the wrong + place; guard `required` secret fields so an enabled-but-unconfigured block fails with a clear + message instead of rendering empty strings. +- **Validate locally without the Django stack:** `helm lint helm/defectdojo`, then + `helm template helm/defectdojo` with and without `--set X.enabled=true` to diff what the new + block renders; run the schema/docs generators before pushing. + +## Notes + +- **Creds caveat:** `admin`/`admin` is a **dev-mode** convenience only. A production-style + `docker compose up` generates a random admin password (see step 1). +- **Playwright expectations:** server-rendered Django, so drive full-page loads and real form + submits; the CSRF token is a hidden input on each form and auth is a session cookie. +- **Never publish without approval:** posting review comments, opening/editing PRs, or any + outward action waits for an explicit yes from the user (see the global action policy). +- **Branch & milestones:** `AGENTS.md` is the source of truth for which release line a change + belongs on and the milestone rules for new PRs — defer to it. +- **Test runner:** always go through `./run-unittest.sh` / `run-tests.sh`; raw `pytest` and + `manage.py test` are not the supported invocation here. diff --git a/.claude/skills/defectdojo-dev/get-api-token.sh b/.claude/skills/defectdojo-dev/get-api-token.sh new file mode 100755 index 00000000000..a3bfec44db5 --- /dev/null +++ b/.claude/skills/defectdojo-dev/get-api-token.sh @@ -0,0 +1,50 @@ +#!/usr/bin/env bash +# Fetch a DefectDojo REST API token so you can test API endpoints. +# +# POSTs to /api/v2/api-token-auth/ and prints the raw token to stdout. +# Use it as an auth header: Authorization: Token +# +# Env overrides (defaults suit local dev mode): +# DD_BASE_URL base URL of the app (default http://localhost:8080) +# DD_USER username (default admin) +# DD_PASSWORD password (default admin) +# +# Examples: +# ./get-api-token.sh +# DD_USER=admin DD_PASSWORD='s3cr3t' DD_BASE_URL=http://localhost:8080 ./get-api-token.sh +# TOKEN=$(./get-api-token.sh) && curl -s -H "Authorization: Token $TOKEN" \ +# "$DD_BASE_URL/api/v2/findings/?limit=1" +# +# If token auth is disabled server-side, the endpoint returns 4xx — get a token +# from the UI instead at: $DD_BASE_URL/api/key-v2 + +set -euo pipefail + +BASE_URL="${DD_BASE_URL:-http://localhost:8080}" +USER="${DD_USER:-admin}" +PASSWORD="${DD_PASSWORD:-admin}" +ENDPOINT="${BASE_URL%/}/api/v2/api-token-auth/" + +# Capture body + HTTP status separately so we can give a useful error. +response="$(curl -sS -w $'\n%{http_code}' \ + -X POST "$ENDPOINT" \ + -H 'Content-Type: application/json' \ + -d "{\"username\": \"${USER}\", \"password\": \"${PASSWORD}\"}")" + +http_code="$(printf '%s' "$response" | tail -n1)" +body="$(printf '%s' "$response" | sed '$d')" + +if [[ "$http_code" != "200" ]]; then + echo "ERROR: token request to ${ENDPOINT} returned HTTP ${http_code}" >&2 + echo "Response: ${body}" >&2 + echo "Hint: is the stack up (docker compose up) and are creds correct?" >&2 + echo " If token auth is disabled, use the UI page ${BASE_URL%/}/api/key-v2" >&2 + exit 1 +fi + +# Prefer jq; fall back to a portable grep/sed extraction of the "token" field. +if command -v jq >/dev/null 2>&1; then + printf '%s\n' "$(printf '%s' "$body" | jq -r '.token')" +else + printf '%s\n' "$(printf '%s' "$body" | sed -n 's/.*"token"[[:space:]]*:[[:space:]]*"\([^"]*\)".*/\1/p')" +fi diff --git a/.claude/skills/defectdojo-dev/run-tests.sh b/.claude/skills/defectdojo-dev/run-tests.sh new file mode 100755 index 00000000000..a68e5f03a3e --- /dev/null +++ b/.claude/skills/defectdojo-dev/run-tests.sh @@ -0,0 +1,45 @@ +#!/usr/bin/env bash +# Thin wrapper around the repo's sanctioned ./run-unittest.sh test runner. +# +# Runs a fully-qualified Django test target inside the uwsgi container +# (python manage.py test --keepdb -v2) and tees the output to a log +# so failures are easy to re-read. The dev stack must already be up +# (./docker/setEnv.sh dev && docker compose up). +# +# Usage: +# ./run-tests.sh [extra run-unittest.sh args] +# +# Examples: +# ./run-tests.sh unittests.tools.test_acunetix_parser.TestAcunetixParser +# ./run-tests.sh unittests.tools.test_acunetix_parser.TestAcunetixParser -f +# +# Do NOT invoke pytest or manage.py test directly — this path (via +# run-unittest.sh) is the supported one and matches how CI runs the suite. + +set -euo pipefail + +if [[ $# -lt 1 || "$1" == "-h" || "$1" == "--help" ]]; then + echo "Usage: ./run-tests.sh [extra args]" >&2 + echo "Example: ./run-tests.sh unittests.tools.test_acunetix_parser.TestAcunetixParser" >&2 + exit 1 +fi + +TEST_TARGET="$1" +shift + +# Locate the repo root (this script lives at .claude/skills/defectdojo-dev/). +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +REPO_ROOT="$(cd "$SCRIPT_DIR/../../.." && pwd)" + +if [[ ! -x "$REPO_ROOT/run-unittest.sh" ]]; then + echo "ERROR: $REPO_ROOT/run-unittest.sh not found or not executable." >&2 + echo "Run this from a checkout of django-DefectDojo with the dev stack up." >&2 + exit 1 +fi + +LOG_DIR="${DD_TEST_LOG_DIR:-/tmp}" +LOG_FILE="${LOG_DIR%/}/dd-test-$(printf '%s' "$TEST_TARGET" | tr './:' '___').log" + +echo "Running ${TEST_TARGET} (log: ${LOG_FILE})" +cd "$REPO_ROOT" +./run-unittest.sh --test-case "$TEST_TARGET" "$@" 2>&1 | tee "$LOG_FILE" diff --git a/.claude/skills/defectdojo-parser/SKILL.md b/.claude/skills/defectdojo-parser/SKILL.md new file mode 100644 index 00000000000..6213637472b --- /dev/null +++ b/.claude/skills/defectdojo-parser/SKILL.md @@ -0,0 +1,237 @@ +--- +name: defectdojo-parser +description: Author and review DefectDojo parsers (scan-report importers under dojo/tools//parser.py) to the project's real conventions — the factory contract, required Finding fields, deduplication registration in settings.dist.py, defusedxml/utf-8/Endpoint.from_uri rules, the 0/1/many unit-test set with attribute-level assertions, sample-file sanitization and size discipline, and the CI meta-test (unittests/test_parsers.py) that enforces the directory/docs layout. Use when writing a new parser, adding a scan type, reviewing a parser PR, or debugging a failing parser/test_parsers test. +--- + +# Write and review DefectDojo parsers + +A parser ingests a security tool's scan report and returns unsaved `Finding` objects. +Parsers live at `dojo/tools//parser.py` and are **auto-discovered** by +`dojo/tools/factory.py` — there is no central list to edit. This skill is both an authoring +guide and a reviewer checklist; the two share the same rules, most of which are enforced by +the meta-test `unittests/test_parsers.py` (so violating them fails CI). + +Canonical references in-repo: +- `docs/content/get_started/contributing/how-to-write-a-parser.md` — the authoritative guide. +- `docs/content/get_started/contributing/parser-documentation-template.md` — docs template. +- `unittests/test_parsers.py` — the meta-test that enforces layout/docs. +- `dojo/tools/factory.py` — discovery/registration. +- `dojo/tools/acunetix/parser.py` — a clean, well-documented reference parser. +- `dojo/tools/picus/parser.py`, `dojo/tools/alertlogic/parser.py` — clean CSV parsers with a + `SEVERITY_MAPPING` that defaults unknown values to `Info`. + +## Inputs + +- **A new parser to write** (the scanner name and a sample report), **or** +- **A parser PR / directory to review** (parser dir name, e.g. `acunetix`). + +## Helper script + +`new-parser-checklist.sh ` (in this skill directory) mirrors the CI meta-test +locally: given a parser directory name it checks that the required files exist — +`dojo/tools//parser.py`, `unittests/tools/test__parser.py`, +`unittests/scans//`, and the docs page (`docs/content/supported_tools/parsers/file/.md`, +or `.../api/.md` for an `api_` dir) — validates the docs front-matter +(`title:`, `toc_hide: true`, and for file parsers `### Sample Scan Data` + the scans link), +and flags common code smells (`lxml` import, `.read()` without utf-8). Run +`./run-unittest.sh --test-case unittests.test_parsers` for the real, authoritative check. + +## Required layout (meta-test enforced) + +| Artifact | Path | +|---|---| +| Package init (empty) | `dojo/tools//__init__.py` | +| Parser code | `dojo/tools//parser.py` | +| Sample scans dir | `unittests/scans//` | +| Unit test (exact name) | `unittests/tools/test__parser.py` | +| Docs (file parser) | `docs/content/supported_tools/parsers/file/.md` | +| Docs (API parser) | `docs/content/supported_tools/parsers/api/.md` (dir is `api_`) | +| Dedup/hashcode config | `dojo/settings/settings.dist.py` | + +## Authoring rules + +1. **Factory contract.** The class name is the directory name with underscores removed + + `Parser` (module `dependency_check` → `DependencyCheckParser`). It must have an **empty + constructor** and implement exactly: + - `get_scan_types(self)` → list of scan-type strings + - `get_label_for_scan_types(self, scan_type)` → short UI label + - `get_description_for_scan_types(self, scan_type)` → long UI description + - `get_findings(self, file, test)` → list of unsaved `Finding` objects + + Add `set_mode(self, mode)` only if the parser exposes more than one scan type (e.g. a + `"... detailed"` variant). **Store no per-scan state on the instance** — the factory reuses + one instance across all imports, so instance/class attributes leak between scans (this has + caused real bugs; reset any state inside `get_findings`). The same leak happens **within a + single call** when loop-local variables (`gem_name`, `severity`, a title, etc.) are only + partially reset between records — a record missing an optional field then inherits the + previous record's value. Reset every per-record variable to `None` at the top of each + iteration, and build the finding only from fields actually present. + +2. **Robust `get_findings`.** Guard every optional field: `data.get("k")`, `if "k" in data`, + and `data.get("list") or []` (guards `null`). An unhandled `KeyError` becomes a 500 on + import. Do **not** fill missing fields with placeholder junk like `"NA"` — leave the + attribute unset. On a garbled or wrong-format file, **raise** `ValueError` with a hint + rather than silently importing zero findings (a legitimately empty report is fine and + should log an INFO line, not raise). + +3. **Severity.** Valid severities are exactly `Info / Low / Medium / High / Critical` + (`Finding.SEVERITIES`). Map vendor values through a dict with a default: + `SEVERITY_MAPPING.get(raw, "Info")` (map synonyms like `Informational → Info`). Two distinct + default cases — keep them straight: + - **A value was present but unmapped** (a synonym you didn't handle fell through) → default + to **`Info`**, and consider it a mapping gap to fix. + - **The report carries no severity at all** for this tool → a documented non-`Info` default + (e.g. `Medium`) can be reasonable, but it must be **called out in the parser's docs page**. + An undocumented `Medium` default is a review finding. + + Do **not** hand-roll a CVSS-score→severity ladder — use `from dojo.utils import parse_cvss_data` + (returns `severity`, `cvssv3`, `cvssv4`, `major_version`) or the `cvss` module. + +4. **XML uses `defusedxml`, never `lxml` or stdlib ElementTree.** PRs with `lxml` are rejected + outright (XXE risk). + +5. **Endpoints / URLs.** Never hand-parse URLs. Use `Endpoint.from_uri(...)` (or the + `hyperlink` module if unavoidable) and assign to `finding.unsaved_endpoints`. + +6. **Encoding.** Any `.read()` in parser code must specify utf-8 within a few lines + (`.read().decode("utf-8")` or `encoding="utf-8"`) — the meta-test fails otherwise. + +## Deduplication (the single most common review miss) + +Register the parser's dedup behavior in `dojo/settings/settings.dist.py`. Forgetting this is +the most frequent maintainer callout on parser PRs — without it the parser silently falls +back to the legacy algorithm. Two blocks must agree: +- **`DEDUPLICATION_ALGORITHM_PER_PARSER`** — map the scan type to one of `DEDUPE_ALGO_HASH_CODE`, + `DEDUPE_ALGO_UNIQUE_ID_FROM_TOOL`, or `DEDUPE_ALGO_UNIQUE_ID_FROM_TOOL_OR_HASH_CODE`. +- **`HASHCODE_FIELDS_PER_SCANNER`** — the list of `Finding` fields the hash is computed from + (required whenever the algorithm uses `HASH_CODE`). + +**Every scan type the parser returns from `get_scan_types()` needs its own dedup entries** — +the config is keyed by scan-type string, so a parser that emits two aliases (e.g. a friendly +name and a legacy one) must register both, or the unregistered alias silently falls back to +legacy dedup. Prefer emitting a single scan type unless there's a real reason for more. + +Rules that recur in review: +- **`unique_id_from_tool` / `vuln_id_from_tool` must come verbatim from the report** — never a + value the parser computes or derives. They must be unique per finding and stable across + scans. If a stable id exists, prefer deduping on it directly rather than hashing it. +- **Keep `severity` out of the hash** unless it is provably stable across scans (it usually + isn't — it drifts as occurrences change). + +### Dedup changes to existing data (close the open loop) + +**Any change that alters the dedup key for an existing scan type is a data-migration +concern, not just a code change — treat it as a release-gating review item.** The dedup key +changes when a PR touches `HASHCODE_FIELDS_PER_SCANNER`, `DEDUPLICATION_ALGORITHM_PER_PARSER`, +how `unique_id_from_tool`/`vuln_id_from_tool` is populated, or any parser field that feeds the +hash (title, component, file_path, line, etc.). Also flag parser output changes that shift +those fields even when the config is untouched. + +Why it matters for customers with existing data: **`hash_code` is computed once at import and +stored on the `Finding` row — it is not recomputed retroactively.** So after the change, old +findings keep their old key and new imports compute a different one. Two concrete failures: +- **Duplicates instead of dedup** — the next scan's findings no longer match the stored ones, + so they import as brand-new findings. +- **The close/reopen loop breaks** — reimport closes findings absent from the new report and + reopens ones that return, by matching on the dedup key. When the key shifts, reimport can + fail to mitigate findings that were actually fixed, or **reopen findings that were already + closed** — noisy and alarming for users. + +What the PR (and your review) must ensure: +- **Call it out explicitly** in the PR description and the release note — this is a behavior + change for existing data, not a silent internal tweak. +- **Provide the recompute path.** Existing rows need `hash_code` recomputed and dedup re-run: + `python manage.py dedupe --parser "" --hash_code_only` (recompute only), then + `--dedupe_only` (re-run dedup), or the full `manage.py dedupe`. Note this is heavy on large + instances (mass update over all findings for the scan type, async by default) — it's ops + guidance for the customer, not something the import path does automatically. +- **Validate the config** with `python manage.py validatededupeconfig`. +- **Prefer additive / opt-in.** Reopening a customer's closed findings on upgrade is a strong + reason to reject or redesign; favor changes that don't retroactively alter the key, or that + ship with a clear recompute runbook. + +## Unit-test strategy + +- **Minimum three sample files: `no_vuln`, `one_vuln`, `many_vulns`** (correct extension), + with matching test methods. Assert the empty file yields `0` findings and the multi file the + exact count. +- **Assert concrete attributes, not just counts** — for representative findings check `title`, + `severity` (`assertIn(finding.severity, Finding.SEVERITIES)`), `active`/`verified`/ + `duplicate`, `unique_id_from_tool`/`vuln_id_from_tool`, `cwe`, CVSS fields, + `vulnerability_ids`, dates, tags. Wrap per-finding checks in `with self.subTest(...)`. +- **Open sample files with the `with ... .open(encoding="utf-8")` pattern**, using + `get_unit_tests_scans_path("") / "one_vuln.json"` from `unittests/dojo_test_case.py`. + Subclass `DojoTestCase`. +- **Endpoint parsers:** call `endpoint.clean()` on every `finding.unsaved_endpoints` in a test + to prove RFC-valid endpoints. +- **API parsers:** also add `unittests/tools/test_api__importer.py` and mock the API with + `unittest.mock.patch`. +- **Regression fixtures:** when fixing a specific bug, add a small targeted sample named + `issue_.` (the repo-wide convention) rather than bloating an existing file. +- Run: `./run-unittest.sh --test-case unittests.tools.test__parser.`. + +## Sample-file rules (sanitization & size) + +- **Sanitize sample scans.** Strip real IPs, hostnames, tokens/credentials, customer names, + and other PII — use `example.com` / obviously fabricated data. (This is a strong convention + the maintainers hold; it is not written as a numeric rule in the repo, so apply judgment and + call it out explicitly in review.) +- **Keep the set minimal.** `no_vuln` / `one_vuln` / `many_vulns` is the baseline; add small, + targeted cases for specific edge conditions instead of committing large real-world dumps. + There is no hard committed-fixture size cap, but the runtime upload limit is + `DD_SCAN_FILE_MAX_SIZE` (default 100 MB) — samples should be far smaller. Prefer the + smallest report that still exercises the behavior. + +## Documentation (meta-test enforced) + +The docs page must exist at the path above and contain front-matter `title:` and +`toc_hide: true`. **File parsers** must also include a `### Sample Scan Data` heading and a +link to `https://github.com/DefectDojo/django-DefectDojo/tree/master/unittests/scans`. Follow +`parser-documentation-template.md` (file types + how to export, field mapping, dedup handling, +default severity, sample link). **Do not put source-code line numbers in the field-mapping +table** — they drift and get rejected in review. + +## Reviewer checklist (highest-frequency issues) + +1. **Dedup registered** in `settings.dist.py` — both the algorithm map and (for hashcode) + `HASHCODE_FIELDS_PER_SCANNER`? +2. **`unique_id_from_tool` taken verbatim** from the report, not computed? +3. **`severity` excluded from the hash** unless provably stable? +4. **Dedup-key change on an existing scan type?** If the PR shifts `HASHCODE_FIELDS_PER_SCANNER`, + the algorithm, `unique_id_from_tool`, or a hashed field, is there a recompute/reimport + runbook (`manage.py dedupe`) and a release note? Existing findings won't recompute + `hash_code` automatically, so imports can duplicate and the close/reopen loop can break. +5. **Unrecognized file raises**, doesn't silently import 0 findings? +6. **0 / 1 / many sample files + attribute-level assertions**, opened with `with`? +7. **Sample files sanitized and minimal** (no real IPs/hosts/tokens/customers; no giant dumps)? +8. **Docs page present** with `title:`, `toc_hide: true`, `### Sample Scan Data` + scans link, + and **no source line numbers**? +9. **Ruff clean; utf-8 after `.read()`; `defusedxml` (no lxml); `Endpoint.from_uri()`**? +10. **No leaked state** — no per-instance attributes across imports, and per-record loop + variables fully reset each iteration (a record missing an optional field must not inherit + the previous record's value)? + +## PR hygiene (parser PRs specifically) + +- **Keep the diff to the parser's own files.** A parser PR should touch `dojo/tools//`, + its test + samples, its docs page, and (for dedup) `settings.dist.py` — nothing else. Flag + unrelated edits riding along, especially `requirements.txt` / dependency bumps; ask for them + to be split out. +- **Screen the PR description.** Community parser PRs sometimes carry vendor marketing or paid- + service links in the body. Flag promotional content for a maintainer — it's not a code defect + but the project cares about it. +- **Trust green Actions + the real diff, not bot noise.** The authoritative signals are the + green GitHub Actions checks and the actual `gh pr diff` (base...head). Third-party bot walls + (e.g. DryRun "sensitive codepath modified") are advisory and are routinely dismissed by + maintainers as false positives — don't treat them as blockers; verify against the real diff. + +## Notes + +- **New parser = a feature → targets `dev`**; a parser bugfix targets `bugfix`. Label the PR + `Import Scans`. Defer to `AGENTS.md` for the branch/milestone policy. +- **New API parsers from the community are currently not accepted** (supportability) — flag + this in review of an inbound API parser. +- A scaffolding template exists: `https://github.com/DefectDojo/cookiecutter-scanner-parser`. +- Common meta-test exemptions (shared/common modules) are hard-coded in + `unittests/test_parsers.py` (e.g. `wizcli_common_parsers`, `sysdig_common`, + `checkmarx_osa`) — a genuinely new parser is not one of these. diff --git a/.claude/skills/defectdojo-parser/new-parser-checklist.sh b/.claude/skills/defectdojo-parser/new-parser-checklist.sh new file mode 100755 index 00000000000..4f2211ae49e --- /dev/null +++ b/.claude/skills/defectdojo-parser/new-parser-checklist.sh @@ -0,0 +1,120 @@ +#!/usr/bin/env bash +# Local mirror of the CI meta-test (unittests/test_parsers.py) for a single parser. +# Checks that a parser directory has all the required sibling files and that its +# docs page carries the required front-matter, plus a couple of common code smells. +# +# This is a fast pre-flight only. The authoritative check is: +# ./run-unittest.sh --test-case unittests.test_parsers +# +# Usage: +# ./new-parser-checklist.sh +# Examples: +# ./new-parser-checklist.sh acunetix +# ./new-parser-checklist.sh api_bugcrowd + +set -uo pipefail + +if [[ $# -ne 1 || "$1" == "-h" || "$1" == "--help" ]]; then + echo "Usage: ./new-parser-checklist.sh (e.g. acunetix)" >&2 + exit 2 +fi + +DIR="$1" + +# Repo root: this script lives at .claude/skills/defectdojo-parser/. +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +REPO_ROOT="$(cd "$SCRIPT_DIR/../../.." && pwd)" +cd "$REPO_ROOT" + +# Docs category + name mirror test_parsers.py: an "api_" prefix maps to the api/ +# docs folder with the prefix stripped; everything else is a file parser. +if [[ "$DIR" == api_* ]]; then + CATEGORY="api" + DOC_NAME="${DIR#api_}" +else + CATEGORY="file" + DOC_NAME="$DIR" +fi + +PARSER_PY="dojo/tools/${DIR}/parser.py" +INIT_PY="dojo/tools/${DIR}/__init__.py" +TEST_PY="unittests/tools/test_${DIR}_parser.py" +SCANS_DIR="unittests/scans/${DIR}" +DOC_MD="docs/content/supported_tools/parsers/${CATEGORY}/${DOC_NAME}.md" + +fail=0 +pass() { printf ' \033[32mOK\033[0m %s\n' "$1"; } +bad() { printf ' \033[31mMISS\033[0m %s\n' "$1"; fail=1; } +warn() { printf ' \033[33mWARN\033[0m %s\n' "$1"; } + +echo "Checking parser '${DIR}' (docs category: ${CATEGORY}) in ${REPO_ROOT}" +echo +echo "Required files:" +[[ -f "$INIT_PY" ]] && pass "$INIT_PY" || bad "$INIT_PY" +[[ -f "$PARSER_PY" ]] && pass "$PARSER_PY" || bad "$PARSER_PY" +[[ -f "$TEST_PY" ]] && pass "$TEST_PY" || bad "$TEST_PY" +[[ -d "$SCANS_DIR" ]] && pass "$SCANS_DIR/" || bad "$SCANS_DIR/ (sample scans directory)" +[[ -f "$DOC_MD" ]] && pass "$DOC_MD" || bad "$DOC_MD" + +echo +echo "Sample scan files (recommended: no_vuln / one_vuln / many_vulns):" +if [[ -d "$SCANS_DIR" ]]; then + count=$(find "$SCANS_DIR" -type f | wc -l | tr -d ' ') + if [[ "$count" -eq 0 ]]; then + warn "scans dir is empty — add at least no_vuln / one_vuln / many_vulns samples" + else + echo " found ${count} sample file(s):" + find "$SCANS_DIR" -type f -exec basename {} \; | sed 's/^/ - /' + # Accept either common naming family: no_vuln/one_vuln/many_vulns or + # no_finding(s)/one_finding/many_findings. Only warn if neither is present. + have_scenario() { # $1 = grep -E pattern of acceptable name stems (optional tool-variant prefix allowed) + find "$SCANS_DIR" -type f | grep -Eiq "/([^/]*_)?($1)\.[^/]+$" + } + have_scenario 'no_vuln|no_finding|no_findings|zero_finding|zero_findings|empty' \ + || warn "no empty-report sample (e.g. no_vuln / zero_finding) for the 0-findings test" + have_scenario 'one_vuln|one_finding' \ + || warn "no single-finding sample (e.g. one_vuln / one_finding)" + have_scenario 'many_vulns|many_findings' \ + || warn "no multi-finding sample (e.g. many_vulns / many_findings)" + fi +fi + +echo +echo "Docs front-matter:" +if [[ -f "$DOC_MD" ]]; then + grep -q "title:" "$DOC_MD" && pass "contains 'title:'" || bad "docs missing 'title:'" + grep -q "toc_hide: true" "$DOC_MD" && pass "contains 'toc_hide: true'" || bad "docs missing 'toc_hide: true'" + if [[ "$CATEGORY" == "file" ]]; then + grep -q "### Sample Scan Data" "$DOC_MD" \ + && pass "contains '### Sample Scan Data'" || bad "docs missing '### Sample Scan Data'" + grep -q "https://github.com/DefectDojo/django-DefectDojo/tree/master/unittests/scans" "$DOC_MD" \ + && pass "contains scans-dir link" || bad "docs missing unittests/scans link" + fi +fi + +echo +echo "Code smells in ${PARSER_PY}:" +if [[ -f "$PARSER_PY" ]]; then + if grep -Eq '(^|[^.])\blxml\b' "$PARSER_PY"; then + warn "references 'lxml' — parsers must use defusedxml (XXE risk); rejected in review" + else + pass "no lxml reference" + fi + # .read() should have utf-8 nearby (mirrors the meta-test's ~4-line window). + if grep -q '\.read()' "$PARSER_PY" && ! grep -A4 '\.read()' "$PARSER_PY" | grep -qi 'utf-8'; then + warn ".read() without a nearby utf-8 encoding — meta-test requires utf-8 after .read()" + else + pass "no unencoded .read()" + fi + grep -q "class .*Parser" "$PARSER_PY" && pass "defines a *Parser class" \ + || warn "no '*Parser' class found — check the factory naming convention" +fi + +echo +if [[ "$fail" -eq 0 ]]; then + echo "All required files/docs present. Now run the real meta-test:" +else + echo "Missing required items above. Fix them, then run the real meta-test:" +fi +echo " ./run-unittest.sh --test-case unittests.test_parsers" +exit "$fail" diff --git a/.gitignore b/.gitignore index ad89e4445fa..4c5a4af3fdf 100644 --- a/.gitignore +++ b/.gitignore @@ -158,6 +158,7 @@ MEMORY.md .claude/* !.claude/settings.json !.claude/hooks/ +!.claude/skills/ .claude/settings.local.json CLAUDE.md CLAUDE.local.md diff --git a/AGENTS.md b/AGENTS.md index 1658e1225ca..46d9601ee29 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -89,6 +89,26 @@ unmilestoned. moves which release it ships in, so re-run the query for the new base and `gh pr edit --milestone` to match. +## Skills + +Repo-scoped skills live under `.claude/skills//SKILL.md` (each with helper scripts +alongside). Invoke the matching one when the task fits: + +- **`defectdojo-dev`** — the primary dev/test loop: bring the local Docker stack up on + `localhost:8080`, reproduce a bug on the target branch before fixing it, write behavioral + unit tests, drive the UI with the Playwright MCP, and fetch an API token to exercise the + REST API. The same review lenses (scalability, performance, memory, DB resourcing, query + design, security, DRF serializer exposure) let it double as an inbound PR reviewer, with a + dedicated checklist for infra/Helm/deployment PRs. Helpers: `get-api-token.sh`, + `run-tests.sh`. Use when developing/testing a change, reproducing or fixing a bug, writing + a regression test, or reviewing any PR (app, API, or Helm chart). +- **`defectdojo-parser`** — author and review scan-report parsers (`dojo/tools//parser.py`) + to the project's real conventions: factory contract, dedup registration in + `settings.dist.py`, `defusedxml`/utf-8/`Endpoint.from_uri` rules, the 0/1/many test set with + attribute-level assertions, sample-file sanitization/size discipline, and the + `unittests/test_parsers.py` meta-test. Helper: `new-parser-checklist.sh`. Use when writing a + new parser, adding a scan type, or reviewing a parser PR. + ## Project Overview DefectDojo is a Django application (`dojo` app) for vulnerability management. The codebase is undergoing a modular reorganization to move from monolithic files toward self-contained domain modules. From e1f92409331e1178df9801dcd8179440a47a2678 Mon Sep 17 00:00:00 2001 From: Cody Maffucci <46459665+Maffooch@users.noreply.github.com> Date: Thu, 20 Aug 2026 09:06:20 -0600 Subject: [PATCH 2/2] chore(skills): satisfy shellcheck in new-parser-checklist.sh Convert the `A && B || C` check chains to explicit if/then/else (SC2015) via check_path/check_grep helpers, and guard the `cd "$REPO_ROOT"` (SC2164). Behavior is unchanged. Co-Authored-By: Claude Opus 4.8 --- .../defectdojo-parser/new-parser-checklist.sh | 40 ++++++++++++------- 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/.claude/skills/defectdojo-parser/new-parser-checklist.sh b/.claude/skills/defectdojo-parser/new-parser-checklist.sh index 4f2211ae49e..636b3b3d601 100755 --- a/.claude/skills/defectdojo-parser/new-parser-checklist.sh +++ b/.claude/skills/defectdojo-parser/new-parser-checklist.sh @@ -24,7 +24,7 @@ DIR="$1" # Repo root: this script lives at .claude/skills/defectdojo-parser/. SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" REPO_ROOT="$(cd "$SCRIPT_DIR/../../.." && pwd)" -cd "$REPO_ROOT" +cd "$REPO_ROOT" || { echo "ERROR: cannot cd to repo root $REPO_ROOT" >&2; exit 1; } # Docs category + name mirror test_parsers.py: an "api_" prefix maps to the api/ # docs folder with the prefix stripped; everything else is a file parser. @@ -47,14 +47,23 @@ pass() { printf ' \033[32mOK\033[0m %s\n' "$1"; } bad() { printf ' \033[31mMISS\033[0m %s\n' "$1"; fail=1; } warn() { printf ' \033[33mWARN\033[0m %s\n' "$1"; } +# check_path <-f|-d> +check_path() { + if test "$1" "$2"; then pass "$3"; else bad "$4"; fi +} +# check_grep +check_grep() { + if grep -q "$1" "$2"; then pass "$3"; else bad "$4"; fi +} + echo "Checking parser '${DIR}' (docs category: ${CATEGORY}) in ${REPO_ROOT}" echo echo "Required files:" -[[ -f "$INIT_PY" ]] && pass "$INIT_PY" || bad "$INIT_PY" -[[ -f "$PARSER_PY" ]] && pass "$PARSER_PY" || bad "$PARSER_PY" -[[ -f "$TEST_PY" ]] && pass "$TEST_PY" || bad "$TEST_PY" -[[ -d "$SCANS_DIR" ]] && pass "$SCANS_DIR/" || bad "$SCANS_DIR/ (sample scans directory)" -[[ -f "$DOC_MD" ]] && pass "$DOC_MD" || bad "$DOC_MD" +check_path -f "$INIT_PY" "$INIT_PY" "$INIT_PY" +check_path -f "$PARSER_PY" "$PARSER_PY" "$PARSER_PY" +check_path -f "$TEST_PY" "$TEST_PY" "$TEST_PY" +check_path -d "$SCANS_DIR" "$SCANS_DIR/" "$SCANS_DIR/ (sample scans directory)" +check_path -f "$DOC_MD" "$DOC_MD" "$DOC_MD" echo echo "Sample scan files (recommended: no_vuln / one_vuln / many_vulns):" @@ -82,13 +91,13 @@ fi echo echo "Docs front-matter:" if [[ -f "$DOC_MD" ]]; then - grep -q "title:" "$DOC_MD" && pass "contains 'title:'" || bad "docs missing 'title:'" - grep -q "toc_hide: true" "$DOC_MD" && pass "contains 'toc_hide: true'" || bad "docs missing 'toc_hide: true'" + check_grep "title:" "$DOC_MD" "contains 'title:'" "docs missing 'title:'" + check_grep "toc_hide: true" "$DOC_MD" "contains 'toc_hide: true'" "docs missing 'toc_hide: true'" if [[ "$CATEGORY" == "file" ]]; then - grep -q "### Sample Scan Data" "$DOC_MD" \ - && pass "contains '### Sample Scan Data'" || bad "docs missing '### Sample Scan Data'" - grep -q "https://github.com/DefectDojo/django-DefectDojo/tree/master/unittests/scans" "$DOC_MD" \ - && pass "contains scans-dir link" || bad "docs missing unittests/scans link" + check_grep "### Sample Scan Data" "$DOC_MD" \ + "contains '### Sample Scan Data'" "docs missing '### Sample Scan Data'" + check_grep "https://github.com/DefectDojo/django-DefectDojo/tree/master/unittests/scans" "$DOC_MD" \ + "contains scans-dir link" "docs missing unittests/scans link" fi fi @@ -106,8 +115,11 @@ if [[ -f "$PARSER_PY" ]]; then else pass "no unencoded .read()" fi - grep -q "class .*Parser" "$PARSER_PY" && pass "defines a *Parser class" \ - || warn "no '*Parser' class found — check the factory naming convention" + if grep -q "class .*Parser" "$PARSER_PY"; then + pass "defines a *Parser class" + else + warn "no '*Parser' class found — check the factory naming convention" + fi fi echo