aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorDanilo M. <danix@danix.xyz>2026-09-10 10:38:10 +0200
committerDanilo M. <danix@danix.xyz>2026-09-10 10:38:10 +0200
commit37d723f2ffd55dcca141831ee2368b5b49618de8 (patch)
tree6b1a2c1c126a92a506f29fc9dbdd44f2f041713a
parent5f9bbeb509773e5280995b39b44cf9be9c29ece2 (diff)
downloadabusectl-37d723f2ffd55dcca141831ee2368b5b49618de8.tar.gz
abusectl-37d723f2ffd55dcca141831ee2368b5b49618de8.zip
feat: ask for the reporter identity during init
Each answer is validated at the prompt that asked for it, and a skipped answer is absent from the file rather than an empty string. Two defects fixed beyond the plan, both in the carry-across rule. Sections build() does not produce are preserved verbatim, and that rule was written when [general] was the only section it produced. Emitting [reporter] as well made preservation emit it TWICE, and tomllib refuses a duplicate table outright, so the rewritten file became unreadable and took the preserved [misp] key with it. Dropping [reporter] unconditionally instead would have been the opposite defect: an init that skips all three questions would silently delete an identity set by hand. What is dropped is now read back off the rendered text, so it is what this run actually wrote rather than what it might have written, and a section added to the builder later cannot be forgotten here. The email check is deliberately not an RFC 5322 validator. What a typo costs is a report whose reply address bounces, and the answers that produce that are a name with no @ at all, a spelled-out "at", and a stray space from a copy-paste. Anything stricter starts rejecting addresses that work. Skipping the address is allowed but warned about at the prompt: it is the one field a report cannot be built without, since it becomes the From. report.build() raises a bare KeyError on that identity today, which the three prompts made reachable from a config file for the first time; logged as backlog item 5 rather than fixed here, because where the check belongs is the report subcommand. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LByBnw83xr9YP85nskzkyE
-rw-r--r--abusectl/cli.py55
-rw-r--r--abusectl/init.py67
-rw-r--r--docs/BACKLOG.md28
-rw-r--r--tests/test_init.py138
4 files changed, 282 insertions, 6 deletions
diff --git a/abusectl/cli.py b/abusectl/cli.py
index ed31a4b..36a12ec 100644
--- a/abusectl/cli.py
+++ b/abusectl/cli.py
@@ -192,6 +192,59 @@ def _ask_relays() -> list[str]:
return candidates
+def _ask_reporter_email() -> str:
+ """Ask for the reply address, re-asking on an answer that cannot be one.
+
+ Validated HERE rather than after the next question, the shape of defect
+ the first hand test found four of. Skippable: an empty answer returns
+ empty and the key is simply absent from the file.
+
+ ponytail: the ceiling is deliberately low. This is not an RFC 5322
+ validator and must not become one, because the failures those catch are
+ not the failures that happen: what a typo actually costs is a report
+ whose reply address bounces, and the answers that produce that are the
+ ones with no @ at all (a name pasted in), a spelled-out "at", or a
+ stray space from a copy-paste. Anything past that would start rejecting
+ addresses that work.
+ """
+ while True:
+ answer = _ask(
+ "\nYour email for abuse desks to reply to (Enter to skip): "
+ )
+ if not answer:
+ return ""
+ if answer.count("@") == 1 and all(part.strip() for part in answer.split("@")):
+ if not any(c.isspace() for c in answer):
+ return answer
+ print(" That does not look like an address abuse desks could reply to.")
+ print(" Expected something like you@example.org, or Enter to skip.")
+
+
+def _ask_reporter() -> dict:
+ """Ask for the identity that goes IN the reports, all three skippable."""
+ print("\nabusectl puts your identity in the reports it sends, so an abuse")
+ print("desk can tell who reported and reply to you. This is the one thing")
+ print("the tool discloses on purpose; everything from the message itself")
+ print("is redacted. Leave any of these blank to skip it.")
+
+ answers = {
+ "reporter_name": _ask("\nYour name (Enter to skip): "),
+ "reporter_org": _ask("Your organisation (Enter to skip): "),
+ "reporter_email": _ask_reporter_email(),
+ }
+
+ # Said here rather than discovered later. The address is the one field a
+ # report cannot be built without, since it becomes the From, and skipping
+ # it is a legitimate choice at setup: the file is hand-editable and this
+ # run may only be about the relays. What is not acceptable is finding out
+ # at `report` time with nothing explaining why.
+ if not answers["reporter_email"]:
+ print("\n No reply address set. `abusectl report` needs one, so add")
+ print(" email under [reporter] before your first report, or re-run init.")
+
+ return answers
+
+
def _prompt_answers(sample: Path | None) -> dict:
"""Ask the interactive questions and return an answers dict for init.build.
@@ -204,7 +257,7 @@ def _prompt_answers(sample: Path | None) -> dict:
trusted_relays = _ask_relays()
cases = _ask(f"\nCases directory [{config.DEFAULT_CASES}]: ")
- return {"trusted_relays": trusted_relays, "cases": cases}
+ return {"trusted_relays": trusted_relays, "cases": cases, **_ask_reporter()}
def _cmd_init(args) -> int:
diff --git a/abusectl/init.py b/abusectl/init.py
index d10f6ed..626dd41 100644
--- a/abusectl/init.py
+++ b/abusectl/init.py
@@ -158,8 +158,16 @@ def _quoted(value: str, field: str) -> str:
return f'"{value}"'
+# The answer key each [reporter] field is asked under.
+_REPORTER_FIELDS = (
+ ("name", "reporter_name"),
+ ("org", "reporter_org"),
+ ("email", "reporter_email"),
+)
+
+
def build(answers: dict) -> str:
- """Render answers as config TOML for the [general] table."""
+ """Render answers as config TOML for the [general] and [reporter] tables."""
relays = _validate_relays(answers.get("trusted_relays", []))
lines = [
@@ -180,6 +188,30 @@ def build(answers: dict) -> str:
if isinstance(cases, str) and cases.strip():
lines.append(f"cases = {_quoted(cases, 'cases')}")
+ # Written STRIPPED, matching config.load(), which strips on read. A file
+ # whose text differs from what every consumer sees is a seam worth not
+ # having, and the name becomes a From display name where surrounding
+ # whitespace would survive verbatim into a header.
+ reporter = []
+ for key, answer_key in _REPORTER_FIELDS:
+ value = answers.get(answer_key, "")
+ if isinstance(value, str) and value.strip():
+ reporter.append(f"{key} = {_quoted(value.strip(), key)}")
+
+ # Emitted only when something was answered. An empty [reporter] reads as
+ # configured-with-nothing, the same trap as api_key = "": absent reads as
+ # not-configured and the part that wants it can say so plainly.
+ if reporter:
+ lines.extend([
+ "",
+ "# The one identity abusectl discloses DELIBERATELY: it goes in the",
+ "# reports you send, so an abuse desk can reply to you. Everything",
+ "# else the tool touches is redacted; this is not.",
+ "",
+ "[reporter]",
+ ])
+ lines.extend(reporter)
+
lines.extend([
"",
"# Later parts of abusectl add further sections here:",
@@ -238,12 +270,34 @@ def back_up(path: pathlib.Path) -> pathlib.Path | None:
return backup_path
-def _preserved_sections(path: pathlib.Path) -> str:
- """Return the raw text of every top-level table except [general].
+def _rendered_sections(text: str) -> set:
+ """Return the names of the top-level tables actually present in text.
+
+ Read back off the rendered TOML rather than assumed from the answers:
+ build() emits [reporter] only when something was answered, and the
+ question preservation must answer is what this run WROTE, not what it
+ could have written.
+ """
+ names = set()
+ for line in text.splitlines():
+ stripped = line.strip()
+ if stripped.startswith("[") and stripped.endswith("]"):
+ names.add(stripped.lstrip("[").rstrip("]").strip())
+ return names
+
+
+def _preserved_sections(path: pathlib.Path, built: set) -> str:
+ """Return the raw text of every top-level table build() did not render.
Carrying raw lines rather than re-rendering means a comment, or a field
this build does not understand, also survives an init that only sets
the relays.
+
+ A table build() DID render is dropped instead: emitting it twice makes
+ the file unreadable, since tomllib refuses a duplicate table and the
+ whole config, [misp] key included, goes with it. `built` names those,
+ and it comes from the rendered text rather than a hand-kept list, so a
+ section added to the builder cannot be forgotten here.
"""
if not path.exists():
return ""
@@ -256,7 +310,7 @@ def _preserved_sections(path: pathlib.Path) -> str:
for line in lines:
stripped = line.strip()
if stripped.startswith("[") and stripped.endswith("]"):
- skipping = stripped.lstrip("[").rstrip("]").strip() == "general"
+ skipping = stripped.lstrip("[").rstrip("]").strip() in built
if skipping:
continue
if not skipping:
@@ -278,8 +332,11 @@ def write(path: pathlib.Path, answers: dict, force: bool = False) -> pathlib.Pat
if path.exists() and not force:
raise FileExistsError(f"{path} already exists; pass --force to overwrite")
- preserved = _preserved_sections(path)
text = build(answers)
+ # Only what this run actually rendered is dropped from the old file, so
+ # an init that skips every reporter question keeps the identity the user
+ # set by hand instead of silently deleting it.
+ preserved = _preserved_sections(path, _rendered_sections(text))
if preserved:
text = text.rstrip("\n") + "\n\n" + preserved + "\n"
diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md
index 780cbf5..63ea52c 100644
--- a/docs/BACKLOG.md
+++ b/docs/BACKLOG.md
@@ -134,3 +134,31 @@ to name the field differently, the fix is one string and one test.
a field an x-arf parser skips rather than acts on wrongly. Worth doing before
the first real report is filed, so a desk running x-arf tooling gets what it
expects.
+
+## 5. `report.build()` raises KeyError on an identity with no email
+
+**Observed.** `report.build(manifest, destination, identity)` reads
+`identity.get("name", "")` defensively but `identity["email"]` directly, so an
+identity carrying a name and no address raises `KeyError: 'email'` rather than
+saying what is missing. Reproduced through the public API against the real
+config reader: `config.load()` on a file whose `[reporter]` sets `name` and
+omits `email` returns `{"name": "A Reporter"}`, and that dict raises.
+
+**Cause.** Every key in `[reporter]` is independently optional, by the same
+skipped-answer-is-absent rule the rest of the config follows, but the report
+builder treats one of them as required without checking. Task 9 fixed the
+config half of this seam; the report half was not reachable from a config file
+until `init` grew the three prompts, and now it is: skipping the email
+question while answering the name produces exactly this shape.
+
+**Approach.** Refuse before building, not after: raise a named error saying no
+reply address is configured. Filling in an empty addr-spec instead would be
+worse, since a `From:` with no address produces a report that is sent and
+cannot be replied to, which defeats the reason the identity is disclosed at
+all. `init` now warns at the prompt when the address is skipped, so the
+remaining gap is a hand-edited config and the error message it deserves.
+
+**Constraints.** Belongs with the `report` subcommand rather than the builder
+alone, since where the check lives decides whether the user sees an exit code
+and a sentence or a traceback. Not a leak: the failure is loud and nothing is
+sent.
diff --git a/tests/test_init.py b/tests/test_init.py
index 619d6ba..46b1a26 100644
--- a/tests/test_init.py
+++ b/tests/test_init.py
@@ -59,6 +59,144 @@ class TestBuildConfig(unittest.TestCase):
"cases": 'x" \ntrusted_relays = ["0.0.0.0/0"]\n#'})
+class TestReporterSection(unittest.TestCase):
+ def test_the_identity_becomes_a_reporter_table(self):
+ text = init.build({
+ "trusted_relays": ["192.0.2.0/24"],
+ "reporter_name": "A Reporter",
+ "reporter_org": "Example Ltd",
+ "reporter_email": "abuse@example.org",
+ })
+ parsed = tomllib.loads(text)
+ self.assertEqual(parsed["reporter"], {
+ "name": "A Reporter",
+ "org": "Example Ltd",
+ "email": "abuse@example.org",
+ })
+
+ def test_a_fully_skipped_identity_emits_no_table_at_all(self):
+ # Not an empty [reporter]: an empty table reads as configured, and
+ # the reader would then report an identity of nothing rather than
+ # saying plainly that none is set.
+ text = init.build({
+ "trusted_relays": ["192.0.2.0/24"],
+ "reporter_name": "",
+ "reporter_org": "",
+ "reporter_email": "",
+ })
+ self.assertNotIn("[reporter]", text)
+ self.assertNotIn("reporter", tomllib.loads(text))
+
+ def test_a_skipped_answer_is_absent_not_empty(self):
+ # Same rule as the cases path: "" reads as configured-and-broken.
+ text = init.build({
+ "trusted_relays": ["192.0.2.0/24"],
+ "reporter_name": "A Reporter",
+ "reporter_org": "",
+ "reporter_email": " ",
+ })
+ reporter = tomllib.loads(text)["reporter"]
+ self.assertEqual(reporter, {"name": "A Reporter"})
+
+ def test_values_are_written_stripped(self):
+ # config.load() strips on read, so writing unstripped would make the
+ # file disagree with what every consumer sees.
+ text = init.build({
+ "trusted_relays": ["192.0.2.0/24"],
+ "reporter_name": " A Reporter ",
+ })
+ self.assertEqual(tomllib.loads(text)["reporter"]["name"], "A Reporter")
+
+ def test_a_quote_in_an_identity_cannot_break_out_of_the_toml(self):
+ with self.assertRaises(ValueError):
+ init.build({
+ "trusted_relays": ["192.0.2.0/24"],
+ "reporter_name": 'x"\nemail = "attacker@example.invalid"',
+ })
+
+ def test_the_identity_loads_back_through_config(self):
+ # The seam report.build() consumes: what init writes must arrive as
+ # the dict shape the reader hands over, keys and all.
+ with tempfile.TemporaryDirectory() as tmp:
+ path = pathlib.Path(tmp) / "config.toml"
+ path.write_text(init.build({
+ "trusted_relays": ["192.0.2.0/24"],
+ "reporter_name": "A Reporter",
+ "reporter_org": "Example Ltd",
+ "reporter_email": "abuse@example.org",
+ }), encoding="utf-8")
+ self.assertEqual(config.load(path).reporter, {
+ "name": "A Reporter",
+ "org": "Example Ltd",
+ "email": "abuse@example.org",
+ })
+
+ def test_a_partial_identity_loads_back_with_only_what_was_given(self):
+ with tempfile.TemporaryDirectory() as tmp:
+ path = pathlib.Path(tmp) / "config.toml"
+ path.write_text(init.build({
+ "trusted_relays": ["192.0.2.0/24"],
+ "reporter_email": "abuse@example.org",
+ }), encoding="utf-8")
+ self.assertEqual(config.load(path).reporter,
+ {"email": "abuse@example.org"})
+
+
+class TestReporterCarryAcross(unittest.TestCase):
+ def test_an_identity_set_by_hand_survives_a_relays_only_rewrite(self):
+ # AGENTS.md: sections build() does not produce are carried across
+ # verbatim. Skipping all three answers must not delete an identity
+ # the user set earlier; there would be no warning that it went.
+ with tempfile.TemporaryDirectory() as tmp:
+ path = pathlib.Path(tmp) / "config.toml"
+ path.write_text(
+ '[general]\ntrusted_relays = ["10.0.0.0/8"]\n\n'
+ '[reporter]\nname = "A Reporter"\nemail = "abuse@example.org"\n',
+ encoding="utf-8",
+ )
+ init.write(path, {"trusted_relays": ["192.0.2.0/24"]}, force=True)
+ self.assertEqual(config.load(path).reporter,
+ {"name": "A Reporter", "email": "abuse@example.org"})
+
+ def test_a_new_identity_replaces_the_old_one_without_duplicating_it(self):
+ # A section the builder DOES produce must not also be carried across:
+ # two [reporter] tables in one file is not merely untidy, tomllib
+ # refuses the whole file and the config becomes unreadable.
+ with tempfile.TemporaryDirectory() as tmp:
+ path = pathlib.Path(tmp) / "config.toml"
+ path.write_text(
+ '[general]\ntrusted_relays = ["10.0.0.0/8"]\n\n'
+ '[reporter]\nname = "Old Name"\n',
+ encoding="utf-8",
+ )
+ init.write(path, {
+ "trusted_relays": ["192.0.2.0/24"],
+ "reporter_name": "New Name",
+ }, force=True)
+
+ self.assertEqual(path.read_text().count("[reporter]"), 1)
+ self.assertEqual(config.load(path).reporter, {"name": "New Name"})
+
+ def test_an_unrelated_section_still_survives_alongside_an_identity(self):
+ with tempfile.TemporaryDirectory() as tmp:
+ path = pathlib.Path(tmp) / "config.toml"
+ path.write_text(
+ '[general]\ntrusted_relays = ["10.0.0.0/8"]\n\n'
+ '[reporter]\nname = "Old Name"\n\n'
+ '[misp]\napi_key = "kept"\n',
+ encoding="utf-8",
+ )
+ init.write(path, {
+ "trusted_relays": ["192.0.2.0/24"],
+ "reporter_name": "New Name",
+ }, force=True)
+
+ rewritten = path.read_text()
+ self.assertIn("kept", rewritten)
+ self.assertNotIn("Old Name", rewritten)
+ self.assertEqual(config.load(path).reporter, {"name": "New Name"})
+
+
class TestProviderTable(unittest.TestCase):
def test_a_known_provider_resolves_to_ranges(self):
self.assertTrue(init.provider_relays("gmail"))