|
| 1 | +# Input-validation pass (design) |
| 2 | + |
| 3 | +- **Date:** 2026-06-03 |
| 4 | +- **Status:** draft, awaiting user review |
| 5 | +- **Scope:** Five small validation/parser fixes accumulated in `planning/deferred-work.md` from the Story-1-2 review. All five are v0-contract gaps in the value-object layer (`Request`, `Response`, `Timeout`, `Limits`, `ClientConfig`). Bundled as one PR because each is small and they share the same review surface (the four frozen dataclasses + one parser function + their existing test files). No CI invariants change. The header/cookie/URL/mapping validation is a strict tightening — code that previously silently accepted garbage now raises; no public API surface change. |
| 6 | +- **Roadmap pointer:** none — this is v0-contract hardening, not an Epic item. Same shape as the project-hygiene-tidy PR that just shipped (`2026-06-02-project-hygiene-tidy-design.md`): one focused PR before Epic 3 (resilience middleware) starts. |
| 7 | + |
| 8 | +## Why |
| 9 | + |
| 10 | +The five items target a single architectural seam: the immutable value objects (`Request`, `Timeout`, `Limits`, `ClientConfig`) accept inputs at construction without enforcing their stated contracts. Today, garbage propagates silently — empty URLs reach the transport, `\r\n` in headers reaches httpx2's wire encoder, negative timeouts get passed through to `httpx2.Timeout` where the error message is less actionable. The fix in every case is the same shape: add `__post_init__` to the frozen dataclass and raise `ValueError`/`TypeError` at the seam. |
| 11 | + |
| 12 | +One item (charset parser robustness) is structurally different — it's a parser bug, not a validation gap — but it lives in the same `response.py` file and ships in the same PR for review cohesion. |
| 13 | + |
| 14 | +Items 2-5 share an architectural decision: validation lives in `__post_init__` on the frozen dataclass, NOT in the `with_*` mutator methods. Two consequences: |
| 15 | + |
| 16 | +1. Single source of truth. Direct construction (`Request(headers={...})`), copy-via-replace (`req.with_header(...)`), and deserialization paths all converge on the same validator. No "which path validates?" question. |
| 17 | +2. `with_*` methods get validation for free with zero per-method code, because every `with_*` uses `dataclasses.replace`, which triggers `__post_init__` on the new instance. |
| 18 | + |
| 19 | +The cost: every `with_*` re-validates the full (post-merge) state, not just the incremental change. For typical Request shapes (≤10 headers, ≤5 query params), this is microseconds. If profiling later shows it matters, optimize then. |
| 20 | + |
| 21 | +### 1. Charset parser robustness (`src/httpware/response.py:20-25`) |
| 22 | + |
| 23 | +Current parser: |
| 24 | + |
| 25 | +```python |
| 26 | +def _parse_charset(content_type: str) -> str | None: |
| 27 | + for raw in content_type.split(";"): |
| 28 | + part = raw.strip() |
| 29 | + if part.lower().startswith(_CHARSET_PREFIX): |
| 30 | + return part[len(_CHARSET_PREFIX) :].strip().strip('"').strip("'") |
| 31 | + return None |
| 32 | +``` |
| 33 | + |
| 34 | +The deferred-work entry listed four concerns. Walking each against the actual code: |
| 35 | + |
| 36 | +- **Inner whitespace inside a quoted value** (`charset=" utf-8 "`) — this IS a bug. After stripping the outer quotes, the inner spaces survive, and `bytes.decode(" utf-8 ")` raises `LookupError`. The `Response.text` property then falls back to UTF-8, which masks the parse failure but produces mojibake if the actual charset isn't UTF-8. `Response.json()` (after the recently-shipped charset fix) inherits this and similarly mis-decodes. |
| 37 | +- **Substring false-positives** (`boundary=charset=foo`) — NOT a bug. The `split(";")` first, then `startswith("charset=")` already prevents this; `boundary=…` doesn't start with `charset=` after the split. |
| 38 | +- **Mismatched quotes** (`charset="utf-8'`) — NOT a bug. Sequential `.strip('"').strip("'")` handles asymmetric quoting. |
| 39 | +- **Multi-charset directives** (`charset=utf-8; charset=iso-8859-1`) — Current code returns the first match. Servers shouldn't send multiple; RFC-compliant behavior is to use the first/canonical value. Keep as-is. |
| 40 | + |
| 41 | +Fix is one extra `.strip()` after the quote-stripping chain to consume any whitespace that was sitting inside the quotes: |
| 42 | + |
| 43 | +```python |
| 44 | +def _parse_charset(content_type: str) -> str | None: |
| 45 | + for raw in content_type.split(";"): |
| 46 | + part = raw.strip() |
| 47 | + if part.lower().startswith(_CHARSET_PREFIX): |
| 48 | + return part[len(_CHARSET_PREFIX) :].strip().strip('"').strip("'").strip() |
| 49 | + return None |
| 50 | +``` |
| 51 | + |
| 52 | +### 2. Header + cookie name/value validation (`src/httpware/request.py`) |
| 53 | + |
| 54 | +Per the "minimal" strictness choice agreed in brainstorming: reject what's actively dangerous or broken, don't enforce full RFC 9110 token grammar (let httpx2 surface the weird-but-technically-allowed cases downstream). |
| 55 | + |
| 56 | +Rules: |
| 57 | + |
| 58 | +- Name and value must both be `str` (catches runtime `None` and other type-system bypasses). |
| 59 | +- Both must be non-empty (HTTP forbids empty header/cookie names; empty values are nonsensical for the values we're shipping). |
| 60 | +- Neither may contain `\r` or `\n` (the injection vector). |
| 61 | + |
| 62 | +Single helper, applied to both header and cookie items: |
| 63 | + |
| 64 | +```python |
| 65 | +def _validate_header_or_cookie(name: str, value: str, *, kind: str) -> None: |
| 66 | + if not isinstance(name, str) or not isinstance(value, str): |
| 67 | + msg = f"{kind} name and value must be str" |
| 68 | + raise TypeError(msg) |
| 69 | + if not name or not value: |
| 70 | + msg = f"{kind} name and value must be non-empty" |
| 71 | + raise ValueError(msg) |
| 72 | + if any(c in name or c in value for c in ("\r", "\n")): |
| 73 | + msg = f"{kind} name and value must not contain CR or LF" |
| 74 | + raise ValueError(msg) |
| 75 | +``` |
| 76 | + |
| 77 | +Called from `Request.__post_init__`: |
| 78 | + |
| 79 | +```python |
| 80 | +for name, value in self.headers.items(): |
| 81 | + _validate_header_or_cookie(name, value, kind="header") |
| 82 | +for name, value in self.cookies.items(): |
| 83 | + _validate_header_or_cookie(name, value, kind="cookie") |
| 84 | +``` |
| 85 | + |
| 86 | +Cookies included alongside headers because the same injection vector exists at the cookie boundary (`with_cookie("name", "value\r\nSet-Cookie: evil=…")`). Same rules, parallel call. |
| 87 | + |
| 88 | +### 3. URL non-empty check + `base_url` normalization |
| 89 | + |
| 90 | +**`Request.url`** — in `Request.__post_init__`: |
| 91 | + |
| 92 | +```python |
| 93 | +if not isinstance(self.url, str): |
| 94 | + msg = "url must be str" |
| 95 | + raise TypeError(msg) |
| 96 | +if not self.url: |
| 97 | + msg = "url must be non-empty" |
| 98 | + raise ValueError(msg) |
| 99 | +``` |
| 100 | + |
| 101 | +URL **format** validation is deliberately not added — httpx2's `InvalidURL` (mapped to our `TransportError`) handles that at send time, and we don't want two parsers disagreeing. The empty-string case is the "you forgot to set it" failure, which deserves to fail at construction. |
| 102 | + |
| 103 | +**`ClientConfig.base_url`** — in `ClientConfig.__post_init__`: |
| 104 | + |
| 105 | +```python |
| 106 | +if self.base_url is not None: |
| 107 | + if not isinstance(self.base_url, str) or not self.base_url: |
| 108 | + msg = "base_url must be a non-empty string or None" |
| 109 | + raise ValueError(msg) |
| 110 | + object.__setattr__(self, "base_url", self.base_url.rstrip("/")) |
| 111 | +``` |
| 112 | + |
| 113 | +(`object.__setattr__` is the standard pattern for mutating a frozen dataclass field inside `__post_init__`.) |
| 114 | + |
| 115 | +After this lands, the stored `base_url` is canonical (no trailing slash). The existing `AsyncClient._resolve_url` at `src/httpware/client.py:122` does `base.rstrip("/")` redundantly — remove it for DRY (one source of truth for "what does the stored base_url look like"). |
| 116 | + |
| 117 | +### 4. Mapping-field validation (`Request.__post_init__`) |
| 118 | + |
| 119 | +`with_query(None)` was the entry-point bug; the underlying gap is broader. None of the four `Mapping` fields on `Request` (`headers`, `params`, `cookies`, `extensions`) currently check that they're actually mappings. A runtime `None` (or a list, or any non-mapping) bypasses type-checking and breaks later at iteration time with a confusing `AttributeError`. |
| 120 | + |
| 121 | +Single loop in `Request.__post_init__`: |
| 122 | + |
| 123 | +```python |
| 124 | +for field_name in ("headers", "params", "cookies", "extensions"): |
| 125 | + field_value = getattr(self, field_name) |
| 126 | + if not isinstance(field_value, Mapping): |
| 127 | + msg = f"{field_name} must be a Mapping (got {type(field_value).__name__})" |
| 128 | + raise TypeError(msg) |
| 129 | +``` |
| 130 | + |
| 131 | +This naturally catches `with_query(None)`, `with_headers(None)`, `with_extensions(None)`, etc., because `dataclasses.replace(self, params=None)` triggers `__post_init__` which then raises. No per-method validation needed. |
| 132 | + |
| 133 | +### 5. `Timeout` / `Limits` negative-value validation (`src/httpware/config.py`) |
| 134 | + |
| 135 | +`Timeout.__post_init__`: |
| 136 | + |
| 137 | +```python |
| 138 | +for attr in ("connect", "read", "write", "pool"): |
| 139 | + value = getattr(self, attr) |
| 140 | + if value < 0: |
| 141 | + msg = f"Timeout.{attr} must be non-negative (got {value})" |
| 142 | + raise ValueError(msg) |
| 143 | +``` |
| 144 | + |
| 145 | +Zero is allowed — valid sentinel for "fail immediately on this phase." |
| 146 | + |
| 147 | +`Limits.__post_init__`: |
| 148 | + |
| 149 | +```python |
| 150 | +if self.max_connections < 0: |
| 151 | + msg = f"max_connections must be non-negative (got {self.max_connections})" |
| 152 | + raise ValueError(msg) |
| 153 | +if self.max_keepalive_connections < 0: |
| 154 | + msg = f"max_keepalive_connections must be non-negative (got {self.max_keepalive_connections})" |
| 155 | + raise ValueError(msg) |
| 156 | +if self.keepalive_expiry < 0: |
| 157 | + msg = f"keepalive_expiry must be non-negative (got {self.keepalive_expiry})" |
| 158 | + raise ValueError(msg) |
| 159 | +``` |
| 160 | + |
| 161 | +httpx2 may have its own opinions about `max_connections=0` (typically interpreted as "no limit"); our contract is just "non-negative" — downstream gets the final say on zero semantics. |
| 162 | + |
| 163 | +## Decisions |
| 164 | + |
| 165 | +| Decision | Choice | |
| 166 | +| --- | --- | |
| 167 | +| Validation placement | `__post_init__` on the frozen dataclass. Single source of truth; `with_*` methods inherit validation via `dataclasses.replace`. | |
| 168 | +| Exception type for bad values | `ValueError`. Matches existing pattern (`raise ValueError(msg)` in `transports/httpx2.py:81`). | |
| 169 | +| Exception type for wrong runtime types | `TypeError`. Distinct from value errors; signals "you bypassed the type system." | |
| 170 | +| Header strictness | Minimal: reject CR/LF, non-`str`, empty. NOT full RFC 9110 token validation — let httpx2 surface the weird-but-technically-allowed cases. | |
| 171 | +| Cookie validation | Same rules as headers, parallel call. Closes the same injection vector. | |
| 172 | +| URL format validation | Deferred to httpx2 (`InvalidURL` → `TransportError` mapping). Only the empty-string case is validated at construction. | |
| 173 | +| `base_url` normalization | Strip trailing slash at `ClientConfig.__post_init__`; remove the now-redundant `rstrip("/")` in `AsyncClient._resolve_url`. | |
| 174 | +| `Timeout` / `Limits` zero values | Allowed. Negative is the bug case. | |
| 175 | +| Charset parser fix | One extra `.strip()` after quote-stripping. The other "concerns" from the deferred entry don't actually fire on the current code; documented in the Why section. | |
| 176 | +| Multi-`charset=` directives | Keep current behavior (return first match). RFC-compliant; not worth changing. | |
| 177 | +| Bundle vs split PR | One PR. All five items touch the value-object layer (`request.py`, `response.py`, `config.py`) and their corresponding test files; splitting adds five rebases for no review-quality gain. | |
| 178 | +| Closing deferred-work entries | Yes — remove the five closed entries from `planning/deferred-work.md` Story 1-2 section in the same PR. | |
| 179 | + |
| 180 | +## File structure |
| 181 | + |
| 182 | +**Modified files:** |
| 183 | + |
| 184 | +- `src/httpware/request.py` — add `__post_init__` to `Request`; add module-private `_validate_header_or_cookie` helper. Validates URL, headers, cookies, and all four Mapping fields. |
| 185 | +- `src/httpware/response.py` — one-line fix in `_parse_charset` (extra `.strip()`). |
| 186 | +- `src/httpware/config.py` — add `__post_init__` to `Timeout`, `Limits`, and `ClientConfig`. `ClientConfig` validates and normalizes `base_url`; the others validate negative values. |
| 187 | +- `src/httpware/client.py` — remove the now-redundant `base.rstrip("/")` in `_resolve_url` (line 122). One-line change. |
| 188 | +- `tests/test_request.py` — add tests for empty URL, non-Mapping fields, bad header/cookie name/value. |
| 189 | +- `tests/test_response.py` — add one test for the charset whitespace-in-quotes case. |
| 190 | +- `tests/test_config.py` — add tests for negative Timeout/Limits fields and `base_url` validation/normalization. |
| 191 | +- `planning/deferred-work.md` — remove the five closed entries from the Story 1-2 section. |
| 192 | + |
| 193 | +**New files:** none. |
| 194 | + |
| 195 | +**Deleted files:** none. |
| 196 | + |
| 197 | +## Verification |
| 198 | + |
| 199 | +- `just lint-ci` exits 0. The `__post_init__` additions don't trigger any new ruff/ty rules (verified mentally; ruff has no opinion about empty `__post_init__` bodies, and ty is fine with `object.__setattr__` inside one). |
| 200 | +- `just test` exits 0. New tests pass; existing tests continue to pass. |
| 201 | +- Specifically: no existing test fixture should construct a `Request` with an empty URL, an empty/CRLF-containing header value, or a non-mapping field. Verify during implementation by running the suite after each `__post_init__` is added; any pre-existing test that now fails should be inspected for whether it was relying on the silent-acceptance behavior (and corrected if so). |
| 202 | +- `Response.text` with `Content-Type: application/json; charset=" utf-8 "` no longer falls back to UTF-8; instead returns the correctly-decoded string. |
| 203 | +- Manual: `grep -E 'Charset parser|Header name/value|URL validation|with_query|Timeout.*Limits.*negative' planning/deferred-work.md` returns empty after the deferred-work cleanup task lands. |
| 204 | + |
| 205 | +## Out of scope |
| 206 | + |
| 207 | +- **Full RFC 9110 header token validation.** Per the brainstorming "minimal" choice. |
| 208 | +- **URL format validation.** Deferred to httpx2's `InvalidURL`. |
| 209 | +- **Header CRLF redaction at transport-side.** That's the `Redactor` middleware (Story 5.3, still deferred). This spec adds construction-side validation; the Redactor adds defense-in-depth at the wire layer. Complementary, not overlapping. |
| 210 | +- **`max_connections=0` semantic check.** Whether 0 means "unlimited" or "no connections" is httpx2's concern. |
| 211 | +- **Other Story 1-2 deferred items not in this bundle:** multi-valued query params (type widening), streaming/async-iterable request bodies (Story 4.1), `@final` to prevent subclassing. Each is a different shape of change. |
| 212 | +- **Per-request `extensions` allowlist.** Deferred to Epic 3 timeout middleware. |
| 213 | + |
| 214 | +## Risks |
| 215 | + |
| 216 | +- **Existing test fixtures may construct `Request` with empty URLs or non-Mapping fields.** Mitigation: run `just test` after each `__post_init__` lands; inspect any pre-existing failures. The likely-affected files are `tests/test_request.py`, `tests/test_client_methods.py`, and `tests/test_middleware.py` (those construct Requests with various shapes). Implementation step verifies before commit. |
| 217 | +- **`object.__setattr__` inside `__post_init__` of a frozen dataclass is mildly unidiomatic.** It's the standard escape hatch for this exact case (frozen + need to normalize a field on construction), and Python's `dataclasses` documentation explicitly sanctions it. No type-checker should flag it; verify ruff/ty stay silent. |
| 218 | +- **Validation cost on every `dataclasses.replace`.** Each `with_*` revalidates the full state. For typical Request shapes (≤10 headers, ≤5 query params, ≤5 cookies), this is microseconds — negligible. Documented in the Why section so a future reader knows the choice was deliberate. |
| 219 | +- **Strictness is a breaking change for buggy callers.** Code that previously silently accepted an empty URL or a `\r\n`-bearing header now raises. Acceptable at v0.1.x; documented as part of the PR description so anyone upgrading sees the contract tightening. |
| 220 | +- **Charset fix may surface a previously-masked bug elsewhere.** If a test or production caller was relying on the mojibake-via-UTF-8-fallback path (intentionally or by accident), the fix changes their observable behavior. Highly unlikely — the fix returns a CORRECT decoded string instead of a wrong one — but worth verifying that `Response.text` and `Response.json()` tests don't depend on the old broken behavior. |
| 221 | + |
| 222 | +## Related work |
| 223 | + |
| 224 | +- `planning/deferred-work.md` — source of all five items; gets edited (five entries removed) as part of this PR. |
| 225 | +- `2026-06-02-project-hygiene-tidy-design.md` — the previous v0-hardening PR. Same shape: bundled config/code fixes that close deferred-work entries; not Epic-tagged. |
| 226 | +- Story 5.3 Redactor middleware — the transport-side complement to item 2's construction-side header CRLF validation. Still deferred; this PR doesn't block it. |
0 commit comments