--- id: config-validation-and-trivia name: Config robustness + trivia batch — ALPN dedup, empty-domains validation, packaging excludes, https doc line (C-2, C-3, N-6, N-7) status: completed depends_on: [] scope: narrow risk: low impact: component level: implementation tags: [robustness, packaging, docs, review-001, c2, c3, n6, n7] --- ## Description Four small findings in one pass (all verified in the decomposition session): 1. **C-2** — `acme-tls/1` is appended unconditionally (src/server.rs:91); a caller who already includes it in `alpns` gets it twice (probed: `alpn_protocols == [acme-tls/1, acme-tls/1]`). Harmless to rustls-acme's challenge dispatch today, but the duplication leaks into the wire config and the ACME/non-ACME asymmetry is undocumented. Fix: a one-line `if !alpn.contains(&b"acme-tls/1".to_vec())` guard — **or** pin the current shape in a doc note + test. The guard is the better default (idempotent construction), but either way a test pins the chosen shape. 2. **C-3** — `TlsIdentity::Acme` with an **empty** `domains` list constructs successfully (probed) and spawns the order loop that then fails per-order at runtime as logged-only events. A one-line validation (`if domains.is_empty() { return Err(TlsError::AcmeConfig("empty domain list".into())) }`) at construction is additive and cheap. Note: empty `contact` stays legal (RFC 8555 §7.3 allows zero-contact accounts) — do NOT validate that. **Mechanics note (2026-09-12 re-check): the new test must be `#[cfg(feature = "acme")]`-gated like the existing ACME tests — under default features the Acme arm already errors with `AcmeConfig` for the *missing-feature* reason (pinned by `new_acme_identity_without_feature_returns_config_error`, server.rs), so a non-gated empty-domains test would pass vacuously and prove nothing.** 3. **N-6** — `tasks/*.md` and `docs/architecture/**` ship in the published package (`cargo package --list` re-verified). One-line `exclude` addition: `"tasks/`, `"docs/architecture/` per the alktunnels house pattern (internal SDD notes + architecture docs don't belong in the public package). 4. **N-7** — `AcmeDirectory::Custom(String)` accepts any string and the URL goes to rustls-acme verbatim, so an `http://` custom directory silently runs ACME over plaintext (token-bearing). One doc line on the variant ("must be an `https://` ACME directory URL") closes it. Do not add runtime URL validation — the doc line is the fix (a caller pointing at a non-https *test* directory should not be blocked). ## Work 1. C-2: the dedup guard (preferred) + a test pinning idempotence (caller-supplied `acme-tls/1` → single entry). 2. C-3: the empty-domains validation + a test (`TlsServerConfig::new(&Acme{domains: vec![]}, ..)` → `TlsError::AcmeConfig`), feature-gated like the existing ACME tests. 3. N-6: `exclude = [... , "tasks/", "docs/architecture/"]`; re-run `cargo package --list --allow-dirty` + `cargo publish --dry-run --allow-dirty`. 4. N-7: the doc line on `AcmeDirectory::Custom`. ## Verification - [ ] Dedup: caller-supplied `acme-tls/1` yields exactly one entry (test pinned) - [ ] Empty-domains ACME construction errors with `TlsError::AcmeConfig` (test pinned) - [ ] `cargo package --list --allow-dirty` contains neither `tasks/` nor `docs/architecture/`; `cargo publish --dry-run --allow-dirty` passes - [ ] The `Custom` variant's doc states the https requirement - [ ] `cargo test`, `--all-features`, clippy, fmt, doc green ## Acceptance Criteria - [ ] All four findings resolved (or explicitly doc-pinned with a test, per finding) - [ ] No behavior change beyond the three intended ones ## References - docs/reviews/001-implementation-review.md §C-2, §C-3, §N-6, §N-7 - src/server.rs:91 (the ALPN append), src/server.rs:64-145 (`new_acme` — where the domains validation goes), src/identity.rs:65 (`AcmeDirectory::Custom`), Cargo.toml:11 (exclude list) - (line refs re-checked 2026-09-12 against the post-ADR-007/008 tree; the file has shifted since the decomposition — re-grep before editing rather than trusting these absolutely) ## Notes - C-2: implemented the dedup guard (preferred option) in `new_acme` — `if !alpn.contains(&b"acme-tls/1".to_vec())` before the push. Also documented the ACME/non-ACME ALPN asymmetry on `TlsServerConfig::new` (ACME always serves `acme-tls/1`, appended idempotently; non-ACME uses the caller's list verbatim). Pinned by `new_acme_caller_supplied_acme_tls_alpn_is_not_duplicated` (`src/server.rs`, `#[cfg(feature = "acme")]`). - C-3: added the empty-domains validation at the top of `new_acme` (before any `AcmeConfig` construction or task spawn) returning `TlsError::AcmeConfig("TlsIdentity::Acme requires a non-empty domain list")`. `contact` left unvalidated per the finding (RFC 8555 §7.3 zero-contact accounts are legal). Test `new_acme_empty_domains_returns_config_error` is `#[cfg(feature = "acme")]`-gated per the task's mechanics note. - N-6: `exclude` now also lists `"tasks/"` and `"docs/architecture/"`. Verified `cargo package --list --allow-dirty` contains neither path and `cargo publish --dry-run --allow-dirty` passes. - N-7: doc line added on `AcmeDirectory::Custom` (`src/identity.rs`): the URL goes to rustls-acme verbatim and must be `https://` (an `http://` URL would run ACME token-bearing over plaintext); explicitly notes no runtime validation is applied so non-https test directories stay usable. No runtime check added, per the finding. ## Summary All four findings resolved. Changes: 1. `src/server.rs` — `new_acme` dedups the `acme-tls/1` ALPN append (idempotent construction) and rejects an empty `domains` list with `TlsError::AcmeConfig` before spawning the order loop; the ACME/non-ACME ALPN asymmetry is documented on `TlsServerConfig::new`. 2. `src/identity.rs` — `AcmeDirectory::Custom` documents the https-only caller contract (no runtime validation, per finding). 3. `Cargo.toml` — `exclude` gains `"tasks/"` and `"docs/architecture/"`. New tests (both `#[cfg(feature = "acme")]`): caller-supplied `acme-tls/1` yields exactly one entry; empty `domains` constructs to `TlsError::AcmeConfig`. Verification: `cargo test` (default) and `cargo test --all-features` (77 lib tests, both new tests pass), `cargo clippy --all-targets -- -D warnings`, `cargo fmt --check`, `cargo doc --no-deps`, `cargo package --list --allow-dirty` (no `tasks/` / `docs/architecture/` entries), `cargo publish --dry-run --allow-dirty` — all green.