Skip to content

Commit 5f93d78

Browse files
bandrelclaude
andcommitted
Merge nightly into main: CONTRIBUTING.md, nmap -n reverse-DNS fix
Batch integration merge, not a squash: tools/next_version.py computes `git log <last-tag>..HEAD` from whichever branch is tagging, and a squash would collapse nightly's already-released commits into one unreachable from any prior tag. Contents: - fix: pass -n to every nmap invocation so a live host no longer gets a reverse-DNS lookup nothing consumes (#58, closes #57) - docs: add CONTRIBUTING.md -- target nightly rather than main, and a regression test with every bug fix (#59) - chore: the dev/nightly branch rename and its revert (#59, #60), a no-op in aggregate Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 parents eea6e9f + 1ec408c commit 5f93d78

4 files changed

Lines changed: 281 additions & 11 deletions

File tree

‎CLAUDE.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -438,6 +438,7 @@ Internal discovery runs a single masscan sweep (no source-port override) followe
438438
- **When the record is written and dropped**: on **success paths only**, exactly as `_EMPTY_RESULT_XML` is — a record on a killed scan would assert coverage that never happened. A `KeyboardInterrupt`, a missing binary and a non-zero exit all leave none; `_nmap_udp_discovery()` needs an explicit `proc.returncode == 0` guard for the last of those, because unlike `_nmap_port_discovery()` it does not treat a non-zero exit as fatal and a failing nmap can still leave parseable partial XML. Each phase also calls `_discard_coverage_record()` **before it scans**, not only on failure: a run killed outright never reaches the stamp, and the previous run's record would otherwise sit beside output that run had already replaced. The hazard is specifically a record that covered *more* than the output now on disk, since a subset test accepts it — a narrower leftover can only cause a redundant re-scan. Both halves live in one file written by a single `_atomic_write` for the same reason: as two sidecars written in sequence, a `KeyboardInterrupt` between them (a `BaseException`, so `except Exception` missed it) left a fresh target list beside a stale exclusion list and the gate accepted the pair as an exclusion-free scan. Neither an unreadable input nor a failed write raises on its own account — the scan already succeeded, so unwinding would discard real results — but every failure path, including an interrupt, deletes the record first. A cache whose record is absent or malformed is rejected, so output from before this change re-scans once on the first resume after upgrading; that is the deliberate direction, since a redundant re-scan is visible and an under-scan is not. The `.coverage` suffix is invisible to every result consumer: `masscan_results/` is aggregated by listing the directory, and `_parse_result_xml()` drops anything not ending in `.xml`, the same guard that hides `portN.xml.failed`. `_delete_previous_results()` removes it with the directory, so `--cleanup` and `[d]elete` need no special handling.
439439
- **config.json validation**: `_load_config()` refuses to start a scan it cannot run correctly. Missing required keys are reported all at once and exit. `target_scan` goes through `_config_target_scan()`, which accepts any case/whitespace spelling of `Internal`/`External` (normalising to the exact literal the ~25 comparison sites use) and exits on anything else — an unvalidated `"internal"` matched neither literal, so the scan ran and looked completely normal while every `target_scan == 'Internal'` gated check was silently skipped. Every numeric goes through `_config_int(key, value, default, minimum=1)`, mirroring `_prompt_int`'s floor: a non-numeric or null value warns and takes the default, and a value below `minimum` is clamped with a warning. `max_rate` is included (defaulting to the interactive prompt's 20000 external / 2000 internal) and only then re-`str()`-ed for Popen.
440440
- **Hostname support**: hostnames in the target file are resolved once at startup; nmap receives the original hostname (for SNI/vhost), masscan receives the resolved IP
441+
- **No reverse DNS from nmap**: every nmap command list passes `-n`, unconditionally, so nmap never performs its default per-host reverse (PTR) lookup. SpooNMAP's only DNS call is the one forward `socket.gethostbyname()` in `resolve_hostname()`; nothing in the module ever reads a `<hostname>`/PTR element out of nmap's XML (`_host_elem_to_dict()` and every other consumer build the host record from the in-memory `ip_to_hostname` map, whose only sources are that forward lookup and `_merge_ssl_cert_hostnames()`). Without `-n`, `-Pn` on the banner/NSE passes makes nmap treat every `-iL` host as up and PTR-resolve it once per port scanned — a burst of queries against the client's own resolver on an internal engagement, for output the tool discards. This is the same unrequested-traffic concern as `check_for_updates`/`honeypot_active_confirm`/`scanner_profile`, so it is off by construction rather than configurable: a future feature that actually wants PTR names should add `-n`-removal at its own site, not make the whole tool pay. `TestNmapNeverDoesReverseDns` pins `-n` on every nmap list literal via the module AST (the same technique as the `_DIR`-relative NSE-path guard), so a refactor can't silently drop it — the failure mode is invisible traffic, exactly what these tests exist to catch. The two `_verify_nmap_overlay()` probes are loopback-only and carry `-n` too, for uniformity. (Issue #57.)
441442
- **TLS certificate hostname discovery**: on External scans, `_extract_ssl_cert_hostnames()` parses the `ssl-cert` NSE output SpooNMAP already collects (commonName off the `Subject:` line, each `DNS:` entry off `Subject Alternative Name:`) and reports every name found as a LOW-severity `TLS Certificate Hostname(s) Identified` finding, wildcards included. Separately, `_merge_ssl_cert_hostnames()` runs right after the NSE script pass (gated on `script_scan`, since that's the only time `nse_results/` exists) and fills gaps in `ip_to_hostname` with the first non-wildcard name per host — never overwriting an operator-supplied hostname from the target file — then rewrites `discovery/ip_hostname_map.json`. It runs before `_aggregate_result_dir()` and `generate_findings()` (which re-reads that file fresh) so `spoonmap_output.*` and the findings report both reflect the merged map for the current run. It does not retroactively change what *this* run's nmap invocations targeted — hostname-based targeting via `create_hostname_target_file()` already happened earlier in the same run using whatever `ip_to_hostname` looked like at that point; the benefit is to this run's reporting. A `--resume` run does not inherit the merged file directly — `main()` calls `preprocess_targets()` unconditionally, including on resume, and it rewrites `discovery/ip_hostname_map.json` from the target file alone with no merge of the existing file's contents — but it re-derives the same cert hostnames from the still-cached `nse_results/*.xml`, since a persisted cert-derived hostname reaching `create_hostname_target_file()` could send nmap after a name-resolved address different from the one actually in scope (a commonName lifted off a shared/CDN certificate can resolve elsewhere entirely). The clobber-on-every-run behavior of `preprocess_targets()` is what accidentally prevents that, and is deliberately left as-is. `'TLS Certificate Hostname(s) Identified'` is also added to `_PER_HOST_DETAIL_TITLES`, since its detail (the actual discovered names) differs per host — without that, `findings.txt` would collapse the group to a single shared description that doesn't exist for this finding.
442443
- **IPv4-only, enforced at the edges**: the tool scans IPv4 exclusively (masscan/nmap invocations, target expansion, and address sorting all assume it). IPv6 is rejected rather than half-supported, in two places. (1) `_build_discovery_target_file()`'s `_parse_ranges()` skips any entry `ipaddress.ip_network()` resolves to a non-v4 network and prints the offending file, line number, and content — previously the v6 bounds were stored silently and only surfaced hundreds of lines later as `AddressValueError: ... (>= 2**32)` from `summarize_address_range()`, and only when an exclusions file happened to be configured. (2) The masscan/discovery XML parsers (`_parse_masscan_ping_xml()`, `_parse_nmap_sn_xml()`, `_run_masscan_batch()`) select `address[@addrtype='ipv4']` instead of the first `<address>` child, matching what the nmap-side parsers already did, so a dual-stacked host's IPv6 or MAC string can't enter `live_ips`/`port_ips` and become a masscan `-iL` target. Address sorting goes through `_ip_sort_key()`, which orders valid IPv4 numerically and sorts anything unparseable last instead of raising — the three former inline `tuple(int(o) for o in x.split('.'))` keys ran *after* a completed sweep, so one odd entry discarded the whole thing.
443444
- **XML result parsing is per-element defensive**: every `etree.parse()` site guards the *walk* as well as the parse. Attributes are read with `.attrib.get(...)` and the element is skipped when the identifier is missing — never a bare `attrib['addr']` or `findall('address')[0]`, both of which raise `KeyError`/`IndexError` that `except etree.ParseError` does not catch. Those exceptions escaped the guard and discarded the results for *every other host* in the file (or, in `_host_elem_to_dict()`, lost `spoonmap_output.xml`/`.json` for the whole run) over one truncated element. `<script>` elements with no `id=` are filtered out of the comprehensions for the same reason. Where a fallback to the first `<address>` child is wanted after `address[@addrtype='ipv4']` misses (`generate_findings()`, `_scan_extra_sql_ports()`), it is a `None`-checked `find('address')`.

‎CONTRIBUTING.md‎

Lines changed: 211 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,211 @@
1+
# Contributing to SpooNMAP
2+
3+
Thanks for your interest. SpooNMAP orchestrates masscan and nmap, so most useful
4+
contributions are fixes to how a scan phase drives one of those tools, new or
5+
corrected NSE scripts under `nse/`, better findings output, or resume/coverage
6+
correctness.
7+
8+
Please read the three short sections below before you start. One is about
9+
engagement data, which matters more here than in most projects. One is about
10+
which branch to target, which is easy to get wrong. One is about regression
11+
tests, which are not optional here.
12+
13+
## Never include real engagement data
14+
15+
SpooNMAP is run from jumpboxes inside client networks on authorized penetration
16+
tests, so the files it reads and writes describe somebody else's infrastructure.
17+
**Do not put any of it in an issue, a pull request, a commit, a test fixture, or
18+
a screenshot.** That includes client IP ranges, hostnames, resolved DNS names,
19+
TLS certificate subjects, service banners, masscan and nmap XML output,
20+
`findings.*`, and anything naming an organization.
21+
22+
When you need example data, invent it. Use documentation ranges
23+
(`192.0.2.0/24`, `198.51.100.0/24`, `203.0.113.0/24`) or RFC1918 addresses and
24+
made-up hostnames, exactly as the existing tests do. When you need to describe a
25+
result, report aggregates that cannot be reversed.
26+
27+
`.gitignore` already covers what SpooNMAP writes: `config.json`,
28+
`discovery/`, `live_hosts/`, `masscan_results/`, `nmap_results/`,
29+
`nse_results/`, `all_live_hosts.txt`, `spoonmap_output.*`, `findings.*`,
30+
`resume.cfg` and more. Do not weaken those patterns to commit something, and do
31+
not `git add -A` past a file that shows up untracked because a local scan
32+
produced it.
33+
34+
`tests/test_nse_integration.py` binds real local ports and shells out to real
35+
`nmap` against `127.0.0.1`. Keep it that way — no test in this repo may send a
36+
packet to an address it does not own.
37+
38+
## Target `nightly`, not `main`
39+
40+
`main` is the default branch, so GitHub will preselect it — **change the base to
41+
`nightly`.** `main` holds the last released state and receives work only
42+
through a batch integration merge from `nightly`. A pull request landing on
43+
`main` directly breaks that and creates cleanup for the maintainers.
44+
45+
```bash
46+
git clone https://github.com/<you>/spoonmap
47+
cd spoonmap
48+
git checkout -b my-change origin/nightly
49+
# ... work ...
50+
git push -u origin my-change # then open a PR with base: nightly
51+
```
52+
53+
Branch names follow a `type/short-description` pattern matching the prefixes
54+
already on `origin`: `feat/`, `fix/`, `docs/`, `test/`, `chore/`, `ci/`.
55+
56+
One exception goes straight to `main`: a security fix that must ship
57+
immediately. Say so in the PR body.
58+
59+
`CLAUDE.md` describes the same flow from the maintainer side under "Release
60+
Versioning", and it is worth reading if you want the reasoning. The mechanic you
61+
do not need to do anything about, but should know exists: maintainers integrate
62+
`nightly` into `main` with a **merge or a fast-forward, never a squash**. CI
63+
derives every release version from the commits themselves, and a squash merge
64+
collapses already-released commits into one that no prior tag can reach, which
65+
silently corrupts every version computed afterward.
66+
67+
## Every bug fix needs a regression test
68+
69+
**A pull request that fixes a bug must add a test that fails before your change
70+
and passes after it.** This is the one rule reviewers will not waive. Say in the
71+
PR body which test it is, and confirm you watched it fail on the unfixed code —
72+
a test written after the fix that has never been seen red is not evidence the
73+
fix does anything.
74+
75+
This matters more here than the phrase usually implies, because almost every bug
76+
this project has shipped failed *quietly*. A resume gate that accepts a stale
77+
cache, a coverage record written on a killed scan, a `task_done()` called twice,
78+
a masscan sweep aimed at the whole range instead of the live hosts — none of
79+
them raised, none of them printed anything wrong, and each produced a scan that
80+
reported itself complete while missing hosts. An operator cannot see that. The
81+
test suite is the only thing that can, so assert on the *specific* wrong
82+
behaviour, not merely that the function returns.
83+
84+
Practical guidance:
85+
86+
- Name the test after the behaviour, not the function, and put the reasoning in
87+
the docstring. Match what is already in `tests/test_spoonmap.py` — the
88+
docstrings there explain why the old behaviour was wrong, which is the part a
89+
future reader needs.
90+
- Prove the failure mode, not the code path. If the bug under-scanned, assert on
91+
the target set actually handed to masscan or nmap. If it lost results, assert
92+
the retained host is still present in the written file.
93+
- New features need tests too, including the failure and interrupt paths.
94+
`KeyboardInterrupt` is a `BaseException`, and getting that wrong has already
95+
cost this repo a correctness bug.
96+
- If a bug is real but you are not fixing it in this PR, open an issue rather
97+
than leaving it undocumented.
98+
99+
Coverage is floored at 95% by pytest's `addopts`, so every job inherits it. Do
100+
not lower the floor to land a change.
101+
102+
## Setting up
103+
104+
Requires **Python 3.8 or newer**, [uv](https://docs.astral.sh/uv/), and both
105+
`masscan` and `nmap` installed as system binaries. `spoonmap.py` itself is
106+
dependency-free stdlib; everything in `uv.lock` is test tooling.
107+
108+
```bash
109+
git clone https://github.com/<you>/spoonmap
110+
cd spoonmap
111+
uv sync
112+
```
113+
114+
`uv.lock` is deliberately resolved for Python 3.10+ only, so it can hold a
115+
pytest patched for CVE-2025-71176. `requires-python` stays `>=3.8` because the
116+
tool itself runs there — if you are developing on 3.8 or 3.9, resolve the test
117+
tooling outside the project the way CI's `test-legacy` job does:
118+
119+
```bash
120+
uv run --isolated --no-project --python 3.8 \
121+
--with pytest --with pytest-cov --with pyyaml --with packaging \
122+
pytest tests/ -v -rs
123+
```
124+
125+
## Before you open a pull request
126+
127+
Run everything below. These are the same gates CI runs, so running them locally
128+
is faster than waiting on a red check.
129+
130+
```bash
131+
# Tests (the 95% coverage floor comes from pyproject.toml's addopts)
132+
uv run --frozen pytest -rs tests/
133+
134+
# Lint
135+
uv run --frozen ruff check spoonmap.py tests/ tools/
136+
137+
# Security
138+
uv run --frozen bandit -r spoonmap.py -c pyproject.toml -b .bandit-baseline.json
139+
140+
# Lock file freshness (only if you touched pyproject.toml)
141+
uv lock --check
142+
143+
# Workflow files (only if you touched .github/workflows/)
144+
uvx --from "actionlint-py==1.7.12.24" actionlint
145+
uvx zizmor==1.29.0 --persona=regular .github/workflows/
146+
```
147+
148+
Notes on the gates:
149+
150+
- **`ruff format` is deliberately not adopted.** Do not run it. Reformatting the
151+
module and the test file would bury every future diff.
152+
- **`.bandit-baseline.json` is committed**, so only a *new* finding fails the
153+
build. Do not add inline `# nosec` suppressions — if your change introduces a
154+
finding that is genuinely fine, regenerate the baseline deliberately and
155+
justify the addition in the commit message. The same goes for a type or lint
156+
error: fix it, do not suppress it.
157+
- **Pass `-rs`.** Several NSE integration tests skip when the port they need is
158+
occupied or when you are not root, and a bare pass count hides a test that
159+
started skipping for the wrong reason. CI's `nse-root` job exists precisely
160+
because a skip is not a failure.
161+
- Some NSE tests run only as root (the `-sU` path). You are not expected to run
162+
those locally; CI does.
163+
164+
## Commit messages
165+
166+
Commits follow [Conventional Commits](https://www.conventionalcommits.org/),
167+
because the release version is derived from them by `tools/next_version.py`: any
168+
`feat` in a batch moves the minor version, while `fix`, `docs`, `chore` and
169+
friends move the patch. A `!` or a `BREAKING CHANGE:` footer counts as a
170+
feature — the major version is only ever bumped by hand.
171+
172+
This makes your subject line load-bearing. **A feature typo'd as `fix:` ships as
173+
a patch release**, and there is no file to correct it in afterward: versions are
174+
tags cut by CI, not a string in the repository.
175+
176+
Keep commits atomic — one logical change each. Reference an issue with `(#NNN)`
177+
at the end of the subject when one exists.
178+
179+
## Adding or changing an NSE script
180+
181+
Scripts under `nse/` ship inside the built wheel and sdist, and the `build` CI
182+
job asserts that both contain *exactly* the set of files in that directory —
183+
not merely a superset. So a script you leave there mid-engagement would ship to
184+
every install, and the build will tell you. Add only scripts intended for
185+
release.
186+
187+
Bundled scripts that probe HTTP must send a `User-Agent` and must target
188+
`host.targetname` rather than `host.ip`, reading
189+
`stdnse.get_script_args('http.useragent')` first so the `scanner_profile`
190+
setting reaches them. See the "Scanner Signature Reduction" section of
191+
`CLAUDE.md` for why both are correctness requirements and not style points.
192+
193+
## Reporting a bug
194+
195+
Useful reports say what you ran, what happened, and what you expected — plus
196+
your OS, your Python version, and the versions of `masscan` and `nmap`
197+
(`masscan --version`, `nmap --version`). Include your `config.json` with the
198+
target paths and any client-identifying values removed, and say which scan type
199+
and `target_scan` you used.
200+
201+
If the bug involves a crash, the traceback is the most valuable thing you can
202+
include. Read it for client hostnames and paths before you paste it.
203+
204+
If the bug is a scan that reported itself complete but missed something, say how
205+
you know — that class of bug is the hardest to reproduce and the most important
206+
to fix.
207+
208+
## Questions
209+
210+
Open an issue. If you are unsure whether an idea fits, asking first is welcome
211+
and cheaper than building it.

0 commit comments

Comments
 (0)