Merge pull request #7722 from nesquena/stage/6658-framing
fix(server): close rejected HTTP writes before connection reuse (#6658)
This commit is contained in:
14 files changed
+3026
-53
No files matched your search
@@ -5,6 +5,18 @@
|
||||
|
||||
### Fixed
|
||||
|
||||
- **A rejected request no longer poisons the next one on the same connection.** `server.py`
|
||||
is a raw HTTP/1.1 handler where `rfile` is the socket itself, so answering a request
|
||||
before reading its body left those bytes queued. The next request on a keep-alive
|
||||
connection was then parsed starting mid-body, and the client got
|
||||
`400 Bad request syntax ('{"a": "b"}GET /api/health HTTP/1.1')` — an error naming a
|
||||
request it never sent, which is expensive to diagnose from the client side. Every
|
||||
reject path now arms `Connection: close` when a body is still pending, and framing is
|
||||
validated strictly (RFC 9110 `1*DIGIT`, duplicate and comma-combined `Content-Length`
|
||||
reconciled, every `Transfer-Encoding` refused since nothing here decodes one). A
|
||||
genuinely bodyless rejection keeps its keep-alive, so healthy pooled connections are
|
||||
not dropped. Thanks @rodrigogs. (#7550, #6658)
|
||||
|
||||
- **Opening a session that belongs to another profile now offers to switch to it instead of
|
||||
looking deleted.** Several cross-profile guards answered `404 Session not found`, which the
|
||||
front-end treats as a missing session and self-heals by clearing the URL and local storage —
|
||||
|
||||
+37
@@ -17,6 +17,7 @@ import time
|
||||
from pathlib import Path
|
||||
|
||||
from api.config import STATE_DIR, get_config, load_settings
|
||||
from api.helpers import request_declares_body
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
@@ -1207,6 +1208,42 @@ def check_auth(handler, parsed) -> bool:
|
||||
return False
|
||||
|
||||
|
||||
def check_auth_or_close(handler, parsed) -> bool:
|
||||
"""Check auth; when rejected, close so an unread body can't poison HTTP/1.1 reuse.
|
||||
|
||||
The flag is armed BEFORE check_auth() writes its 401/302, so end_headers()
|
||||
can advertise ``Connection: close``; success restores the prior flag so an
|
||||
authenticated request keeps its keep-alive.
|
||||
|
||||
Armed ONLY when the request's framing declares a body still queued in
|
||||
``rfile`` (see ``request_declares_body()``), which is the same rule the other
|
||||
reject-before-read sites apply through ``arm_connection_close_if_body_pending()``.
|
||||
Arming unconditionally was the mirror image of the over-close this PR already
|
||||
fixed on the sidecar path: a body-less POST that failed auth answered
|
||||
``401`` WITH ``Connection: close`` and dropped the client's pipelined
|
||||
follow-up, killing a healthy keep-alive connection for no framing reason.
|
||||
Verified on the wire, pipelined down one socket against the production
|
||||
handler: ``POST /api/session/new`` with no ``Content-Length`` (and with
|
||||
``Content-Length: 0``) answered ``401`` + ``Connection: close`` and the
|
||||
following ``GET /api/auth/status`` was never served. The helper cannot be
|
||||
swapped in directly here because the arming has to happen before
|
||||
``check_auth()`` writes its response and be undone if it succeeds.
|
||||
|
||||
``close_connection`` only exists once BaseHTTPRequestHandler has parsed a
|
||||
request line, so partially-built handler stubs may not have it at all. Read
|
||||
it defensively and only arm/restore when it was really there: inventing the
|
||||
attribute on a stub would leave a bogus keep-alive verdict behind.
|
||||
"""
|
||||
if not hasattr(handler, 'close_connection') or not request_declares_body(handler):
|
||||
return check_auth(handler, parsed)
|
||||
prior_close = handler.close_connection
|
||||
handler.close_connection = True
|
||||
if check_auth(handler, parsed):
|
||||
handler.close_connection = prior_close
|
||||
return True
|
||||
return False
|
||||
|
||||
|
||||
def _is_loopback(addr: str) -> bool:
|
||||
"""Return True if *addr* is a loopback address (127.x.x.x, ::1, or ::ffff:127.x.x.x)."""
|
||||
import ipaddress as _ipaddress
|
||||
|
||||
+242
@@ -202,6 +202,248 @@ def _security_headers(handler):
|
||||
)
|
||||
|
||||
|
||||
def arm_connection_close(handler) -> None:
|
||||
"""Mark the connection dead BEFORE a reject-before-read response is written.
|
||||
|
||||
A rejection that answers without consuming the request body leaves those
|
||||
bytes queued in ``rfile``; reusing the connection makes the next HTTP/1.1
|
||||
request parse mid-body (the classic ``{}GET ... -> 501``). Arming must
|
||||
happen before the response is flushed so ``advertise_connection_close()``
|
||||
can serialize it into the response headers.
|
||||
|
||||
Best-effort: handler stubs and read-only doubles may reject the attribute,
|
||||
and a failure here must never turn a 4xx into a 500.
|
||||
"""
|
||||
try:
|
||||
handler.close_connection = True
|
||||
except Exception:
|
||||
pass
|
||||
|
||||
|
||||
# What ``unsupported_transfer_encoding()`` reports for a header that declares no
|
||||
# coding at all (``Transfer-Encoding:``). A placeholder rather than the raw '' so
|
||||
# the return value is never a falsy non-``None`` — callers test ``is not None``,
|
||||
# and an empty string would also render as a hole in the rejection message.
|
||||
_BLANK_TRANSFER_CODING = '(blank)'
|
||||
|
||||
# The only whitespace a field value may be padded with: RFC 9110 OWS is SP/HTAB
|
||||
# and nothing else. ``str.strip()`` would also erase U+00A0 and U+0085 -- both
|
||||
# reachable over the wire, because request lines decode as latin-1 -- and
|
||||
# ``Content-Length: \xa00`` would then read back as a genuine ``0`` while its
|
||||
# payload sat unread on the socket. Padding a value with a character the grammar
|
||||
# does not allow makes it unreadable, not zero.
|
||||
_FIELD_VALUE_OWS = ' \t'
|
||||
|
||||
|
||||
def _framing_header_values(handler, name: str) -> list[str]:
|
||||
"""Every value the wire carried for one framing header, tolerating stubs.
|
||||
|
||||
ALL values, not just the first: ``Message.get()`` returns only the first
|
||||
occurrence, so a request carrying ``Content-Length: 0`` followed by
|
||||
``Content-Length: 42`` reads back as length zero. Its 42 body bytes are then
|
||||
never consumed and get parsed as the next request line -- the same poisoning
|
||||
this module exists to prevent, reached by duplicating a header rather than by
|
||||
omitting one. Conflicting framing is only visible if every value is.
|
||||
|
||||
A real handler carries an ``email.message.Message`` (case-insensitive
|
||||
``get_all()``, like the wire); the suite's ``SimpleNamespace(headers={...})``
|
||||
doubles are plain dicts, which are not, so fall back to a case-insensitive
|
||||
scan for those. A missing or odd ``headers`` must never raise: every caller
|
||||
is on a rejection path where an exception becomes a spurious 500.
|
||||
"""
|
||||
headers = getattr(handler, 'headers', None)
|
||||
if headers is None:
|
||||
return []
|
||||
try:
|
||||
get_all = getattr(headers, 'get_all', None)
|
||||
if callable(get_all):
|
||||
values = get_all(name)
|
||||
return [] if values is None else [str(v) for v in values if v is not None]
|
||||
if isinstance(headers, dict):
|
||||
lowered = name.lower()
|
||||
return [
|
||||
str(v) for k, v in headers.items()
|
||||
if str(k).lower() == lowered and v is not None
|
||||
]
|
||||
value = headers.get(name)
|
||||
return [] if value is None else [str(value)]
|
||||
except Exception:
|
||||
return []
|
||||
|
||||
|
||||
def _declared_content_lengths(handler) -> set[int] | None:
|
||||
"""Distinct declared ``Content-Length`` values, or ``None`` if any is unreadable.
|
||||
|
||||
``None`` means the framing cannot be trusted at all (a value that will not
|
||||
parse); an empty set means no length was declared.
|
||||
|
||||
A BLANK or whitespace-only value is unreadable, NOT absent. ``Content-Length:``
|
||||
with payload bytes behind it declares a body whose size the header refuses to
|
||||
state; skipping the empty value reported "no length declared", so the
|
||||
rejection kept the connection alive and those bytes were parsed as the next
|
||||
request line (403 then ``400 Bad request syntax ('{...}GET /api/... HTTP/1.1')``).
|
||||
Absence is something only the wire can say, and ``_framing_header_values()``
|
||||
already reports a header the request never carried as no value at all -- so
|
||||
anything that *is* here and does not parse has to be treated as pending bytes,
|
||||
exactly like ``banana`` or two lengths that disagree. Note ``Message`` collapses
|
||||
``Content-Length:`` and ``Content-Length: `` to the same ``''``.
|
||||
|
||||
"Parses" means RFC 9110's ``Content-Length = 1*DIGIT``: an unsigned run of
|
||||
ASCII digits, nothing else. ``int()`` is far more generous than the grammar --
|
||||
it accepts a leading sign, PEP 515 underscores and non-ASCII digits -- and
|
||||
every such spelling of ZERO used to read back as an honest ``0`` and take the
|
||||
keep-alive branch below with its payload still queued: ``Content-Length: +0``
|
||||
(also ``-0``, ``0_0``, ``+00``, ``\\xa00``) answered 403 with no
|
||||
``Connection: close``, then ``400 Bad request syntax ('{...}GET /api/...')``
|
||||
-- the same trace as a blank value. A sign is only invisible on a NON-zero
|
||||
value, where the wrong parse happens to close anyway. Anything the grammar
|
||||
rejects is unreadable framing, which means a body may be pending.
|
||||
|
||||
The ``int()`` guard stays even though ``isdigit()`` has already vetted every
|
||||
character: a digit run longer than ``sys.get_int_max_str_digits()`` (4300)
|
||||
raises ``ValueError`` too, and this runs on rejection paths where an
|
||||
exception becomes a spurious 500.
|
||||
"""
|
||||
parsed: set[int] = set()
|
||||
for raw in _framing_header_values(handler, 'Content-Length'):
|
||||
# RFC 9110 §5.3: a list-valued field may arrive as repeated lines OR as
|
||||
# one comma-combined line, and the two spellings mean the same thing.
|
||||
# So "0, 0" is the same bodyless request as two "Content-Length: 0"
|
||||
# headers and must keep its keep-alive; only a DISAGREEING list (caught
|
||||
# below by len(parsed) > 1) is unframeable. Without the split the comma
|
||||
# fails isdigit() and every agreeing list over-closed a healthy socket.
|
||||
for member in raw.split(','):
|
||||
stripped = member.strip(_FIELD_VALUE_OWS)
|
||||
if not (stripped.isascii() and stripped.isdigit()):
|
||||
return None
|
||||
try:
|
||||
parsed.add(int(stripped))
|
||||
except ValueError:
|
||||
return None
|
||||
return parsed
|
||||
|
||||
|
||||
def unreadable_content_length(handler) -> bool:
|
||||
"""True when ``Content-Length`` cannot be trusted to frame the body.
|
||||
|
||||
Either a value that will not parse (``banana``, ``0x5``, or a BLANK one), or
|
||||
two or more values that disagree. None of them can tell a reader how many
|
||||
bytes to drain, so such a request can only be rejected -- with the connection
|
||||
armed for close, since bytes may still be queued behind it.
|
||||
"""
|
||||
parsed = _declared_content_lengths(handler)
|
||||
return parsed is None or len(parsed) > 1
|
||||
|
||||
|
||||
def request_declares_body(handler) -> bool:
|
||||
"""True when the request's framing declares a body that is still queued.
|
||||
|
||||
Keyed on the wire, not on the method: ``Transfer-Encoding`` present (chunked
|
||||
framing hides the length until the terminating chunk is read) or
|
||||
``Content-Length`` present and non-zero. The method proves nothing in either
|
||||
direction -- a GET may carry a declared body and a POST/PUT/DELETE may carry
|
||||
none -- and a static per-method flag therefore both misses body-bearing
|
||||
"read" requests (whose unread bytes poison the next pooled request) and
|
||||
kills healthy keep-alive on body-less writes.
|
||||
|
||||
A blank, unparseable or self-contradicting ``Content-Length`` counts as a
|
||||
declared body, as does ANY ``Transfer-Encoding`` -- ``identity`` and a blank
|
||||
value included: a value we cannot interpret does not prove the body's absence,
|
||||
and the safe assumption for connection reuse is that bytes are pending. Only
|
||||
framing that positively says "no body" -- no framing header at all, or every
|
||||
declared length agreeing on zero -- keeps the connection alive.
|
||||
"""
|
||||
if unsupported_transfer_encoding(handler) is not None:
|
||||
return True
|
||||
parsed = _declared_content_lengths(handler)
|
||||
if parsed is None:
|
||||
return True
|
||||
return any(length != 0 for length in parsed)
|
||||
|
||||
|
||||
def unsupported_transfer_encoding(handler) -> str | None:
|
||||
"""Return the transfer coding this server cannot decode, or ``None``.
|
||||
|
||||
EVERY present value counts, with no exemption for any coding.
|
||||
``BaseHTTPRequestHandler`` decodes none of them: it hands every reader a raw
|
||||
``rfile``, so a chunked body reads back as its literal chunk framing while a
|
||||
``Content-Length``-based reader sees length 0 and consumes nothing at all.
|
||||
RFC 9112 section 6.3 says the same thing normatively -- a request whose final
|
||||
transfer coding is not ``chunked`` has a body length the recipient cannot
|
||||
determine, so it must be refused and the connection closed. Such a request
|
||||
can therefore only be rejected, and only with the connection armed for close,
|
||||
because its payload bytes are still queued on the socket.
|
||||
|
||||
``identity`` used to be exempt as a coding that "frames nothing". It is not:
|
||||
it names no framing either, and RFC 7230 removed it from the transfer codings
|
||||
altogether. A sidecar rejection then answered 403 with keep-alive and the
|
||||
payload was parsed as the next request line
|
||||
(``400 Bad request syntax ('{...}GET /api/health/agent HTTP/1.1')``), and all
|
||||
four upload handlers fell through to a length-0 read, answered 400 "No file
|
||||
field in request" and left the multipart bytes on the socket
|
||||
(``400 Bad request syntax ('--x')``). Nothing legitimate is lost: no client
|
||||
sends a transfer coding this server could have honoured.
|
||||
|
||||
A header present but EMPTY (``Transfer-Encoding:``) is reported through
|
||||
``_BLANK_TRANSFER_CODING`` rather than as ``''``, so callers testing
|
||||
``is not None`` and the 411 message both read right. Only an ABSENT header
|
||||
yields no values at all -- absence is something only the wire can say.
|
||||
|
||||
The first value is enough precisely because no value is acceptable; it is
|
||||
reported as-is (SP/HTAB trimmed, the one padding RFC 9110 allows) purely so
|
||||
the rejection message names what arrived.
|
||||
"""
|
||||
values = _framing_header_values(handler, 'Transfer-Encoding')
|
||||
if not values:
|
||||
return None
|
||||
return values[0].strip(_FIELD_VALUE_OWS) or _BLANK_TRANSFER_CODING
|
||||
|
||||
|
||||
def arm_connection_close_if_body_pending(handler) -> bool:
|
||||
"""Arm close for a reject-before-read, but only if a body was really declared.
|
||||
|
||||
Returns whether the connection was armed. Use this instead of
|
||||
``arm_connection_close()`` wherever the rejection can also fire on a request
|
||||
with no body: closing then would drop a pooled client's healthy connection
|
||||
for no framing reason.
|
||||
"""
|
||||
if not request_declares_body(handler):
|
||||
return False
|
||||
arm_connection_close(handler)
|
||||
return True
|
||||
|
||||
|
||||
def advertise_connection_close(handler) -> None:
|
||||
"""Serialize ``close_connection`` into exactly one ``Connection: close`` header.
|
||||
|
||||
``BaseHTTPRequestHandler`` tracks ``close_connection`` internally but never
|
||||
writes it to the wire, so a server-side close is invisible to the client: a
|
||||
pooled client reuses the socket and fails with BrokenPipeError instead of
|
||||
opening a fresh connection. Call this from ``end_headers()``, before the
|
||||
header buffer is flushed.
|
||||
|
||||
Dedup is derived from the pending header buffer rather than a private flag,
|
||||
because the SSE endpoints send their own ``Connection: close`` (and
|
||||
``send_header`` itself flips ``close_connection`` when they do) — keying off
|
||||
a flag we set ourselves would emit the header twice on every stream.
|
||||
|
||||
Both attribute reads are defensive: partially-built handler stubs in the
|
||||
test suite carry neither ``close_connection`` nor ``_headers_buffer``, and
|
||||
header emission must not depend on their presence. The buffer is also
|
||||
type-checked before it is scanned -- a stub whose ``_headers_buffer`` is a
|
||||
truthy non-iterable (a bare ``Mock``) would otherwise raise ``TypeError``
|
||||
from inside ``end_headers()``, which ``_handle_write`` would swallow into a
|
||||
spurious 500.
|
||||
"""
|
||||
if not getattr(handler, 'close_connection', False):
|
||||
return
|
||||
buffered = getattr(handler, '_headers_buffer', None)
|
||||
for raw in buffered if isinstance(buffered, (list, tuple)) else ():
|
||||
if raw[:11].lower() == b'connection:':
|
||||
return
|
||||
handler.send_header('Connection', 'close')
|
||||
|
||||
|
||||
def flush_pending_auth_cookies(handler) -> None:
|
||||
pending = getattr(handler, '_pending_set_cookies', None)
|
||||
if not pending:
|
||||
|
||||
@@ -2974,6 +2974,7 @@ from api.helpers import (
|
||||
require,
|
||||
bad,
|
||||
safe_resolve,
|
||||
arm_connection_close_if_body_pending,
|
||||
j,
|
||||
t,
|
||||
read_body,
|
||||
@@ -5871,6 +5872,15 @@ def _csrf_rejection_error(handler) -> str:
|
||||
def _check_csrf(handler) -> bool:
|
||||
"""Reject cross-origin or tokenless authenticated browser unsafe requests."""
|
||||
if not _check_same_origin_browser_request(handler):
|
||||
# CSRF checks run before read_body(), so close rather than reusing an
|
||||
# HTTP/1.1 connection whose unread body would corrupt the next request --
|
||||
# but ONLY when the framing says bytes are really queued. Arming
|
||||
# unconditionally dropped a healthy pooled connection on a body-less
|
||||
# write: verified on the wire, `POST /api/session/new` with
|
||||
# `Origin: http://evil.invalid` and no `Content-Length` (and with
|
||||
# `Content-Length: 0`) answered 403 + `Connection: close` and the
|
||||
# pipelined `GET /api/health/agent` was never served.
|
||||
arm_connection_close_if_body_pending(handler)
|
||||
return False
|
||||
if not _is_browser_unsafe_request(handler):
|
||||
return True # non-browser clients (curl, MCP, agent) have no Origin/Referer
|
||||
@@ -5883,6 +5893,11 @@ def _check_csrf(handler) -> bool:
|
||||
submitted = handler.headers.get(CSRF_HEADER_NAME) or handler.headers.get("X-CSRF-Token")
|
||||
if verify_csrf_token(cookie_val or "", submitted or ""):
|
||||
return True
|
||||
# Same framing rule as the origin rejection above: a token mismatch on a
|
||||
# body-less write has nothing unread to protect. Verified on the wire with an
|
||||
# authenticated same-origin `POST /api/session/new` carrying no CSRF token and
|
||||
# no `Content-Length`: 403 + `Connection: close`, follow-up dropped.
|
||||
arm_connection_close_if_body_pending(handler)
|
||||
return _set_csrf_failure_reason(handler, "token_mismatch")
|
||||
|
||||
|
||||
@@ -6062,6 +6077,14 @@ def _handle_extension_sidecar_proxy(
|
||||
# DELETE fell through the CSRF compatibility path that intentionally admits
|
||||
# non-browser clients, giving unsafe methods weaker provenance than GET.
|
||||
if not _check_same_origin_browser_request(handler, require_provenance=True):
|
||||
# Provenance rejection runs before read_body(), so close-and-advertise
|
||||
# whenever the request DECLARED a body (Content-Length non-zero, or any
|
||||
# Transfer-Encoding): those bytes are still queued in rfile and a reused
|
||||
# HTTP/1.1 connection would parse them as the next request line (same
|
||||
# class as _check_csrf). Gating on read_request_body instead was wrong in
|
||||
# both directions — it missed a GET that carries a declared body, and it
|
||||
# closed a healthy connection on a body-less DELETE/PUT/PATCH.
|
||||
arm_connection_close_if_body_pending(handler)
|
||||
return j(handler, {"error": _csrf_rejection_error(handler)}, status=403)
|
||||
try:
|
||||
request_body = _read_body_bytes(handler) if read_request_body else None
|
||||
@@ -6504,6 +6527,13 @@ def _handle_csp_report(handler) -> bool:
|
||||
"Dropped CSP report from %s: rate limit exceeded",
|
||||
_client_ip_for_rate_limit(handler),
|
||||
)
|
||||
# Rate-limit rejection runs before the body is read; close-and-advertise
|
||||
# so the unread report can't corrupt the next pooled request -- but only
|
||||
# when a body was really declared. A body-less report answered 204 WITH
|
||||
# `Connection: close` once the 100-per-60s limiter tripped (reproduced on
|
||||
# the wire at request 101; the pipelined `GET /api/health/agent` was
|
||||
# dropped), so a browser that keeps reporting loses its socket each time.
|
||||
arm_connection_close_if_body_pending(handler)
|
||||
return _send_no_content(handler)
|
||||
|
||||
payload = _read_csp_report_payload(handler)
|
||||
@@ -12895,6 +12925,15 @@ def _handle_shutdown(handler) -> bool:
|
||||
|
||||
def _handle_health_restart(handler) -> bool:
|
||||
"""Restart the Hermes messaging gateway service."""
|
||||
# This endpoint never consumes its request body on any outcome, so close when
|
||||
# one was DECLARED -- and only then. Arming unconditionally closed the socket
|
||||
# on every call including the successful, body-less one the WebUI actually
|
||||
# makes: verified on the wire, `POST /api/health/restart` with no
|
||||
# `Content-Length` answered with `Connection: close` and the pipelined
|
||||
# `GET /api/health/agent` was never served. The single arming covers every
|
||||
# outcome below (completed / in_progress / busy / error) because the framing,
|
||||
# not the result, decides.
|
||||
arm_connection_close_if_body_pending(handler)
|
||||
outcome = restart_active_profile_gateway()
|
||||
|
||||
if outcome.get("status") == "completed":
|
||||
@@ -15217,6 +15256,12 @@ def handle_post(handler, parsed) -> bool:
|
||||
if diag:
|
||||
diag.stage("process_complete_ack_deprecated")
|
||||
try:
|
||||
# The 410 runs before the stale tab's JSON body is read;
|
||||
# close-and-advertise so those unread bytes can't corrupt the next
|
||||
# pooled request -- but a body-less ack has none, and closing then
|
||||
# just kills keep-alive (on the wire: 410 + `Connection: close` with
|
||||
# no `Content-Length`, pipelined follow-up dropped).
|
||||
arm_connection_close_if_body_pending(handler)
|
||||
j(
|
||||
handler,
|
||||
{
|
||||
|
||||
+105
-22
@@ -8,7 +8,12 @@ import tempfile
|
||||
from pathlib import Path
|
||||
|
||||
from api.config import MAX_UPLOAD_BYTES, STATE_DIR
|
||||
from api.helpers import j
|
||||
from api.helpers import (
|
||||
arm_connection_close_if_body_pending,
|
||||
j,
|
||||
unreadable_content_length,
|
||||
unsupported_transfer_encoding,
|
||||
)
|
||||
from api.models import get_session
|
||||
from api.profiles import _profiles_match, get_active_profile_name as _get_active_profile_name
|
||||
from api.workspace import (
|
||||
@@ -45,6 +50,89 @@ def _max_extracted_bytes() -> int:
|
||||
_MAX_EXTRACTED_BYTES = 10 * MAX_UPLOAD_BYTES
|
||||
|
||||
|
||||
def _reject_before_read(handler, message, status):
|
||||
"""Reject without reading the body; close so unread bytes can't poison HTTP/1.1 reuse.
|
||||
|
||||
Framing decides, exactly as at the other reject-before-read sites: every
|
||||
rejection raised by the preflight above (any ``Transfer-Encoding``, an
|
||||
unreadable or conflicting ``Content-Length``, an over-cap length) already
|
||||
declares a body and still closes. Only the boundary/`parse_multipart` rejection
|
||||
can fire on a request that declares none, and closing there dropped a healthy
|
||||
pooled connection -- on the wire, ``POST /api/upload`` with
|
||||
``Content-Type: application/json`` and no ``Content-Length`` answered 400
|
||||
"No boundary in Content-Type" WITH ``Connection: close`` and the pipelined
|
||||
``GET /api/health/agent`` was never served.
|
||||
"""
|
||||
arm_connection_close_if_body_pending(handler)
|
||||
return j(handler, {'error': message}, status=status)
|
||||
|
||||
|
||||
class _RejectBeforeRead(ValueError):
|
||||
"""A rejection raised before any byte of the request body was consumed.
|
||||
|
||||
Carries the status the caller must answer with. Subclasses ``ValueError`` so
|
||||
that if one ever escapes an unusual call path, each handler's existing
|
||||
``except ValueError`` still answers 400 with the same message instead of
|
||||
turning it into a 500.
|
||||
"""
|
||||
|
||||
def __init__(self, message: str, status: int):
|
||||
super().__init__(message)
|
||||
self.message = message
|
||||
self.status = status
|
||||
|
||||
|
||||
def _read_multipart_or_reject(handler):
|
||||
"""Framing preflight + body read, shared by all four multipart handlers.
|
||||
|
||||
Returns ``(fields, files)``. Raises ``_RejectBeforeRead`` when the request
|
||||
must be refused before its body is read -- the caller answers through
|
||||
``_reject_before_read()``, which arms ``Connection: close`` first, because the
|
||||
declared body is still queued in ``rfile`` and a reused HTTP/1.1 connection
|
||||
would parse those bytes as the next request line.
|
||||
|
||||
Framing is judged from the headers, not from a present ``Content-Length``:
|
||||
|
||||
* ANY ``Transfer-Encoding`` is refused, ``identity`` included: ``http.server``
|
||||
decodes none of them (``rfile`` is the raw socket) and RFC 9112 section 6.3
|
||||
says a request whose final coding is not ``chunked`` has a length no
|
||||
recipient can determine. A coded request carries no usable Content-Length,
|
||||
so the old code read it as length 0, found no parts, and answered "No file
|
||||
field in request" with keep-alive intact -- leaving every payload byte on
|
||||
the socket to poison the next pooled request. A Content-Length sent BESIDE a
|
||||
coding does not rescue it: that pairing is the smuggling shape 6.3 rejects,
|
||||
and the coding still hides how the body is framed.
|
||||
* a non-numeric or oversized Content-Length cannot be read either, and
|
||||
``parse_multipart()`` validates the boundary and the length before its
|
||||
first ``rfile.read()`` — so every raise below is still pre-read.
|
||||
* two DISAGREEING Content-Length headers read back as whichever came first,
|
||||
so ``Content-Length: 0`` ahead of the real length made the body invisible
|
||||
and left every byte of it on the socket. There is no honest length to
|
||||
drain, so the request is refused before ``rfile`` is touched.
|
||||
"""
|
||||
content_type = handler.headers.get('Content-Type', '')
|
||||
coding = unsupported_transfer_encoding(handler)
|
||||
if coding is not None:
|
||||
raise _RejectBeforeRead(
|
||||
f'Unsupported Transfer-Encoding: {coding} '
|
||||
'(uploads must send an undecoded body with a Content-Length)',
|
||||
411,
|
||||
)
|
||||
if unreadable_content_length(handler):
|
||||
raise _RejectBeforeRead('Invalid Content-Length', 400)
|
||||
try:
|
||||
content_length = int(handler.headers.get('Content-Length', 0) or 0)
|
||||
except (TypeError, ValueError):
|
||||
raise _RejectBeforeRead('Invalid Content-Length', 400) from None
|
||||
if content_length > MAX_UPLOAD_BYTES:
|
||||
raise _RejectBeforeRead(
|
||||
f'File too large (max {MAX_UPLOAD_BYTES//1024//1024}MB)', 413)
|
||||
try:
|
||||
return parse_multipart(handler.rfile, content_type, content_length)
|
||||
except ValueError as exc:
|
||||
raise _RejectBeforeRead(str(exc), 400) from None
|
||||
|
||||
|
||||
def parse_multipart(rfile, content_type, content_length) -> tuple:
|
||||
import re as _re, email.parser as _ep
|
||||
# Imported locally (not just module-level) so the function stays
|
||||
@@ -206,11 +294,10 @@ def _write_office_upload_sidecar(workspace: Path, dest: Path, file_bytes: bytes)
|
||||
def handle_upload(handler):
|
||||
import traceback as _tb
|
||||
try:
|
||||
content_type = handler.headers.get('Content-Type', '')
|
||||
content_length = int(handler.headers.get('Content-Length', 0) or 0)
|
||||
if content_length > MAX_UPLOAD_BYTES:
|
||||
return j(handler, {'error': f'File too large (max {MAX_UPLOAD_BYTES//1024//1024}MB)'}, status=413)
|
||||
fields, files = parse_multipart(handler.rfile, content_type, content_length)
|
||||
try:
|
||||
fields, files = _read_multipart_or_reject(handler)
|
||||
except _RejectBeforeRead as rejection:
|
||||
return _reject_before_read(handler, rejection.message, rejection.status)
|
||||
session_id = fields.get('session_id', '')
|
||||
if 'file' not in files:
|
||||
return j(handler, {'error': 'No file field in request'}, status=400)
|
||||
@@ -396,11 +483,10 @@ def handle_upload_extract(handler):
|
||||
"""Handle archive upload and extraction."""
|
||||
import traceback as _tb
|
||||
try:
|
||||
content_type = handler.headers.get('Content-Type', '')
|
||||
content_length = int(handler.headers.get('Content-Length', 0) or 0)
|
||||
if content_length > MAX_UPLOAD_BYTES:
|
||||
return j(handler, {'error': f'File too large (max {MAX_UPLOAD_BYTES//1024//1024}MB)'}, status=413)
|
||||
fields, files = parse_multipart(handler.rfile, content_type, content_length)
|
||||
try:
|
||||
fields, files = _read_multipart_or_reject(handler)
|
||||
except _RejectBeforeRead as rejection:
|
||||
return _reject_before_read(handler, rejection.message, rejection.status)
|
||||
session_id = fields.get('session_id', '')
|
||||
if 'file' not in files:
|
||||
return j(handler, {'error': 'No file field in request'}, status=400)
|
||||
@@ -428,11 +514,10 @@ def handle_transcribe(handler):
|
||||
import traceback as _tb
|
||||
temp_path = None
|
||||
try:
|
||||
content_type = handler.headers.get('Content-Type', '')
|
||||
content_length = int(handler.headers.get('Content-Length', 0) or 0)
|
||||
if content_length > MAX_UPLOAD_BYTES:
|
||||
return j(handler, {'error': f'File too large (max {MAX_UPLOAD_BYTES//1024//1024}MB)'}, status=413)
|
||||
fields, files = parse_multipart(handler.rfile, content_type, content_length)
|
||||
try:
|
||||
fields, files = _read_multipart_or_reject(handler)
|
||||
except _RejectBeforeRead as rejection:
|
||||
return _reject_before_read(handler, rejection.message, rejection.status)
|
||||
if 'file' not in files:
|
||||
return j(handler, {'error': 'No file field in request'}, status=400)
|
||||
filename, file_bytes = files['file']
|
||||
@@ -612,12 +697,10 @@ def handle_workspace_upload(handler):
|
||||
"""
|
||||
import traceback as _tb
|
||||
try:
|
||||
content_type = handler.headers.get('Content-Type', '')
|
||||
content_length = int(handler.headers.get('Content-Length', 0) or 0)
|
||||
if content_length > MAX_UPLOAD_BYTES:
|
||||
return j(handler, {'error': f'File too large (max {MAX_UPLOAD_BYTES//1024//1024}MB)'}, status=413)
|
||||
|
||||
fields, files = parse_multipart(handler.rfile, content_type, content_length)
|
||||
try:
|
||||
fields, files = _read_multipart_or_reject(handler)
|
||||
except _RejectBeforeRead as rejection:
|
||||
return _reject_before_read(handler, rejection.message, rejection.status)
|
||||
session_id = fields.get('session_id', '')
|
||||
subpath = fields.get('path', '')
|
||||
|
||||
|
||||
@@ -100,10 +100,11 @@ from urllib.parse import urlparse
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
from api.request_logging import emit_request_log
|
||||
from api.auth import check_auth, reset_trusted_auth_request_state
|
||||
from api.auth import check_auth_or_close, reset_trusted_auth_request_state
|
||||
from api.config import HOST, PORT, STATE_DIR, SESSION_DIR, DEFAULT_WORKSPACE
|
||||
from api.helpers import (
|
||||
j,
|
||||
advertise_connection_close,
|
||||
get_profile_cookie,
|
||||
_build_csp_report_only_policy,
|
||||
_CLIENT_DISCONNECT_ERRORS,
|
||||
@@ -332,6 +333,7 @@ class Handler(BaseHTTPRequestHandler):
|
||||
extra_frame_src = getattr(self, "_csp_extra_frame_src", None)
|
||||
self.send_header("Content-Security-Policy-Report-Only", self.csp_report_only_policy(extra_connect_src, extra_frame_src))
|
||||
self.send_header("Report-To", self._CSP_REPORT_TO)
|
||||
advertise_connection_close(self) # tell the client when the socket dies
|
||||
super().end_headers()
|
||||
|
||||
def log_message(self, fmt, *args): pass # suppress default Apache-style log
|
||||
@@ -376,7 +378,8 @@ class Handler(BaseHTTPRequestHandler):
|
||||
set_request_profile(cookie_profile)
|
||||
try:
|
||||
parsed = urlparse(self.path)
|
||||
if not check_auth(self, parsed): return
|
||||
# Body-pending-aware: a body-bearing GET failing auth would poison reuse (#7550).
|
||||
if not check_auth_or_close(self, parsed): return
|
||||
result = handle_get(self, parsed)
|
||||
if result is False:
|
||||
return j(self, {'error': 'not found'}, status=404)
|
||||
@@ -401,10 +404,8 @@ class Handler(BaseHTTPRequestHandler):
|
||||
set_request_profile(cookie_profile)
|
||||
try:
|
||||
parsed = urlparse(self.path)
|
||||
_is_csp_report_post = (
|
||||
parsed.path == "/api/csp-report" and self.command == "POST"
|
||||
)
|
||||
if not _is_csp_report_post and not check_auth(self, parsed): return
|
||||
_is_csp_report_post = parsed.path == "/api/csp-report" and self.command == "POST"
|
||||
if not _is_csp_report_post and not check_auth_or_close(self, parsed): return
|
||||
result = route_func(self, parsed)
|
||||
if result is False:
|
||||
return j(self, {'error': 'not found'}, status=404)
|
||||
|
||||
@@ -227,14 +227,14 @@ class TestServerDisconnectHandling(unittest.TestCase):
|
||||
# Patch handle_get to raise BrokenPipeError
|
||||
import server as _server_mod
|
||||
orig_handle_get = _server_mod.handle_get
|
||||
orig_check_auth = _server_mod.check_auth
|
||||
orig_check_auth = _server_mod.check_auth_or_close
|
||||
_server_mod.handle_get = _fake_handle_get
|
||||
_server_mod.check_auth = lambda h, p: True
|
||||
_server_mod.check_auth_or_close = lambda h, p: True
|
||||
try:
|
||||
Handler.do_GET(handler)
|
||||
finally:
|
||||
_server_mod.handle_get = orig_handle_get
|
||||
_server_mod.check_auth = orig_check_auth
|
||||
_server_mod.check_auth_or_close = orig_check_auth
|
||||
|
||||
# send_response should NEVER be called for the 500 — client is gone
|
||||
handler.send_response.assert_not_called()
|
||||
@@ -248,12 +248,12 @@ class TestServerDisconnectHandling(unittest.TestCase):
|
||||
raise self._route_raises
|
||||
|
||||
import server as _server_mod
|
||||
orig_check_auth = _server_mod.check_auth
|
||||
_server_mod.check_auth = lambda h, p: True
|
||||
orig_check_auth = _server_mod.check_auth_or_close
|
||||
_server_mod.check_auth_or_close = lambda h, p: True
|
||||
try:
|
||||
Handler._handle_write(handler, _fake_route)
|
||||
finally:
|
||||
_server_mod.check_auth = orig_check_auth
|
||||
_server_mod.check_auth_or_close = orig_check_auth
|
||||
|
||||
handler.send_response.assert_not_called()
|
||||
|
||||
@@ -286,9 +286,9 @@ class TestServerDisconnectHandling(unittest.TestCase):
|
||||
|
||||
import server as _server_mod
|
||||
orig_handle_get = _server_mod.handle_get
|
||||
orig_check_auth = _server_mod.check_auth
|
||||
orig_check_auth = _server_mod.check_auth_or_close
|
||||
_server_mod.handle_get = _fake_handle_get
|
||||
_server_mod.check_auth = lambda h, p: True
|
||||
_server_mod.check_auth_or_close = lambda h, p: True
|
||||
|
||||
closed_stdout = io.StringIO()
|
||||
closed_stdout.close()
|
||||
@@ -299,7 +299,7 @@ class TestServerDisconnectHandling(unittest.TestCase):
|
||||
finally:
|
||||
sys.stdout = original_stdout
|
||||
_server_mod.handle_get = orig_handle_get
|
||||
_server_mod.check_auth = orig_check_auth
|
||||
_server_mod.check_auth_or_close = orig_check_auth
|
||||
|
||||
handler.send_response.assert_called_once_with(500)
|
||||
|
||||
@@ -312,14 +312,14 @@ class TestServerDisconnectHandling(unittest.TestCase):
|
||||
|
||||
import server as _server_mod
|
||||
orig_handle_get = _server_mod.handle_get
|
||||
orig_check_auth = _server_mod.check_auth
|
||||
orig_check_auth = _server_mod.check_auth_or_close
|
||||
_server_mod.handle_get = _fake_handle_get
|
||||
_server_mod.check_auth = lambda h, p: True
|
||||
_server_mod.check_auth_or_close = lambda h, p: True
|
||||
try:
|
||||
Handler.do_GET(handler)
|
||||
finally:
|
||||
_server_mod.handle_get = orig_handle_get
|
||||
_server_mod.check_auth = orig_check_auth
|
||||
_server_mod.check_auth_or_close = orig_check_auth
|
||||
|
||||
# Should send 500 for real errors
|
||||
handler.send_response.assert_called_once_with(500)
|
||||
@@ -334,14 +334,14 @@ class TestServerDisconnectHandling(unittest.TestCase):
|
||||
|
||||
import server as _server_mod
|
||||
orig_handle_get = _server_mod.handle_get
|
||||
orig_check_auth = _server_mod.check_auth
|
||||
orig_check_auth = _server_mod.check_auth_or_close
|
||||
_server_mod.handle_get = _fake_handle_get
|
||||
_server_mod.check_auth = lambda h, p: True
|
||||
_server_mod.check_auth_or_close = lambda h, p: True
|
||||
try:
|
||||
Handler.do_GET(handler)
|
||||
finally:
|
||||
_server_mod.handle_get = orig_handle_get
|
||||
_server_mod.check_auth = orig_check_auth
|
||||
_server_mod.check_auth_or_close = orig_check_auth
|
||||
|
||||
handler.send_response.assert_not_called()
|
||||
|
||||
@@ -355,14 +355,14 @@ class TestServerDisconnectHandling(unittest.TestCase):
|
||||
|
||||
import server as _server_mod
|
||||
orig_handle_get = _server_mod.handle_get
|
||||
orig_check_auth = _server_mod.check_auth
|
||||
orig_check_auth = _server_mod.check_auth_or_close
|
||||
_server_mod.handle_get = _fake_handle_get
|
||||
_server_mod.check_auth = lambda h, p: True
|
||||
_server_mod.check_auth_or_close = lambda h, p: True
|
||||
try:
|
||||
Handler.do_GET(handler)
|
||||
finally:
|
||||
_server_mod.handle_get = orig_handle_get
|
||||
_server_mod.check_auth = orig_check_auth
|
||||
_server_mod.check_auth_or_close = orig_check_auth
|
||||
|
||||
handler.send_response.assert_not_called()
|
||||
|
||||
@@ -406,14 +406,14 @@ class TestServer500ResponseSafety(unittest.TestCase):
|
||||
|
||||
import server as _server_mod
|
||||
orig_handle_get = _server_mod.handle_get
|
||||
orig_check_auth = _server_mod.check_auth
|
||||
orig_check_auth = _server_mod.check_auth_or_close
|
||||
_server_mod.handle_get = _fake_handle_get
|
||||
_server_mod.check_auth = lambda h, p: True
|
||||
_server_mod.check_auth_or_close = lambda h, p: True
|
||||
try:
|
||||
Handler.do_GET(handler)
|
||||
finally:
|
||||
_server_mod.handle_get = orig_handle_get
|
||||
_server_mod.check_auth = orig_check_auth
|
||||
_server_mod.check_auth_or_close = orig_check_auth
|
||||
|
||||
# send_response WAS called (we tried to send 500), but write failed
|
||||
handler.send_response.assert_called_once_with(500)
|
||||
@@ -431,14 +431,14 @@ class TestServer500ResponseSafety(unittest.TestCase):
|
||||
|
||||
import server as _server_mod
|
||||
orig_handle_get = _server_mod.handle_get
|
||||
orig_check_auth = _server_mod.check_auth
|
||||
orig_check_auth = _server_mod.check_auth_or_close
|
||||
_server_mod.handle_get = _fake_handle_get
|
||||
_server_mod.check_auth = lambda h, p: True
|
||||
_server_mod.check_auth_or_close = lambda h, p: True
|
||||
try:
|
||||
Handler.do_GET(handler)
|
||||
finally:
|
||||
_server_mod.handle_get = orig_handle_get
|
||||
_server_mod.check_auth = orig_check_auth
|
||||
_server_mod.check_auth_or_close = orig_check_auth
|
||||
|
||||
# send_response WAS called (we tried to send 500), but write failed
|
||||
handler.send_response.assert_called_once_with(500)
|
||||
|
||||
@@ -0,0 +1,105 @@
|
||||
"""Comma-combined ``Content-Length`` must not over-close a healthy connection.
|
||||
|
||||
RFC 9110 §5.3 lets a list-valued field arrive either as repeated header lines or
|
||||
as a single comma-combined line, and the two spellings are equivalent. So
|
||||
``Content-Length: 0, 0`` describes the same bodyless request as two separate
|
||||
``Content-Length: 0`` headers, and rejecting it must keep the keep-alive.
|
||||
|
||||
``_declared_content_lengths()`` originally ran ``isdigit()`` across the whole raw
|
||||
value, so the comma failed the check, the field read as *unreadable*, and every
|
||||
helper-backed rejection armed ``Connection: close`` on a request with nothing
|
||||
queued behind it. That is the over-close half of this PR's contract — the mirror
|
||||
image of the bug it fixes, and the one that silently drops pooled connections.
|
||||
|
||||
The dangerous direction has to stay closed, so the disagreeing and malformed
|
||||
spellings are asserted here alongside the recoverable ones: ``0, 42`` genuinely
|
||||
cannot frame a body and must still be refused with the close armed.
|
||||
"""
|
||||
|
||||
import pytest
|
||||
|
||||
from api.helpers import request_declares_body, unreadable_content_length
|
||||
|
||||
|
||||
class _Hdrs(dict):
|
||||
"""email.message-like: ``get()`` returns the first value, ``get_all()`` all."""
|
||||
|
||||
def get(self, k, default=None):
|
||||
for key, val in self.items():
|
||||
if key.lower() == k.lower():
|
||||
return val[0] if isinstance(val, list) else val
|
||||
return default
|
||||
|
||||
def get_all(self, k, default=None):
|
||||
out = []
|
||||
for key, val in self.items():
|
||||
if key.lower() == k.lower():
|
||||
out.extend(val if isinstance(val, list) else [val])
|
||||
return out or default
|
||||
|
||||
|
||||
class _H:
|
||||
def __init__(self, headers):
|
||||
self.headers = headers
|
||||
|
||||
|
||||
def _handler(cl):
|
||||
return _H(_Hdrs({"Content-Length": cl}))
|
||||
|
||||
|
||||
@pytest.mark.parametrize("value", ["0, 0", "00, 00", "0 , 0", "0,0", "0,\t0"])
|
||||
def test_agreeing_bodyless_comma_list_is_readable_and_bodyless(value):
|
||||
"""An agreeing all-zero list is framed, bodyless, and must keep keep-alive."""
|
||||
h = _handler(value)
|
||||
assert unreadable_content_length(h) is False, (
|
||||
f"Content-Length {value!r} read as unreadable; a bodyless rejection would "
|
||||
"then arm Connection: close and drop a healthy pooled connection"
|
||||
)
|
||||
assert bool(request_declares_body(h)) is False, (
|
||||
f"Content-Length {value!r} declares no body, so nothing is queued"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("value", ["42, 42", "42,42", "042, 42"])
|
||||
def test_agreeing_nonzero_comma_list_is_readable_with_a_body(value):
|
||||
"""An agreeing nonzero list frames a real body: readable, and a body IS pending."""
|
||||
h = _handler(value)
|
||||
assert unreadable_content_length(h) is False, value
|
||||
assert bool(request_declares_body(h)) is True, value
|
||||
|
||||
|
||||
@pytest.mark.parametrize("value", ["0, 42", "42, 0", "0, 0, 42"])
|
||||
def test_disagreeing_comma_list_stays_unreadable(value):
|
||||
"""Disagreeing members cannot frame a body — must stay refused, close armed."""
|
||||
h = _handler(value)
|
||||
assert unreadable_content_length(h) is True, (
|
||||
f"Content-Length {value!r} disagrees; there is no honest length to drain, "
|
||||
"so it must be refused with the connection armed for close"
|
||||
)
|
||||
assert bool(request_declares_body(h)) is True, value
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"value",
|
||||
["0, banana", "banana, 0", "0,", ",0", "0, ,0", "0, +0", "0, \xa00", "0, 0x5"],
|
||||
)
|
||||
def test_malformed_comma_members_stay_unreadable(value):
|
||||
"""One bad member poisons the whole field: fail closed, never treat as zero."""
|
||||
h = _handler(value)
|
||||
assert unreadable_content_length(h) is True, (
|
||||
f"Content-Length {value!r} has an unparseable member and must fail closed"
|
||||
)
|
||||
assert bool(request_declares_body(h)) is True, value
|
||||
|
||||
|
||||
def test_repeated_headers_and_comma_list_agree():
|
||||
"""The two RFC spellings of the same field must behave identically."""
|
||||
repeated = _H(_Hdrs({"Content-Length": ["0", "0"]}))
|
||||
combined = _handler("0, 0")
|
||||
assert unreadable_content_length(repeated) == unreadable_content_length(combined)
|
||||
assert bool(request_declares_body(repeated)) == bool(request_declares_body(combined))
|
||||
|
||||
repeated_bad = _H(_Hdrs({"Content-Length": ["0", "42"]}))
|
||||
combined_bad = _handler("0, 42")
|
||||
assert unreadable_content_length(repeated_bad) is True
|
||||
assert unreadable_content_length(combined_bad) is True
|
||||
@@ -0,0 +1,158 @@
|
||||
"""A body-bearing GET that fails auth must not poison a keep-alive connection.
|
||||
|
||||
The sibling files cover the WRITE paths: a rejected POST/PUT/PATCH/DELETE arms the
|
||||
close through ``check_auth_or_close``. ``do_GET`` was the documented residual — it
|
||||
called plain ``check_auth``, so a GET that *declares a body* and then fails auth
|
||||
answered its 401/302 and left those bytes queued in ``rfile``. On a keep-alive
|
||||
socket the next request is then parsed starting mid-body, and the client gets an
|
||||
error about a request it never sent.
|
||||
|
||||
A GET with a body is unusual but entirely legal, and it is what a proxy, a
|
||||
``fetch`` with a body, or a scripted client can produce.
|
||||
|
||||
Both halves of the contract are asserted here, because the naive fix (close every
|
||||
auth-failed GET) would regress the body-less login-redirect flow:
|
||||
|
||||
* body-bearing + auth-failed -> connection MUST close (framing unresynchronisable)
|
||||
* body-less + auth-failed -> connection MUST stay alive (nothing queued)
|
||||
|
||||
``test_unfixed_handler_reproduces_the_poisoning`` is the negative control: the same
|
||||
socket exchange against a handler that answers without arming the close shows the
|
||||
leftover body surfacing as a bogus ``400 Bad request syntax``, proving the exchange
|
||||
really does exercise the bug rather than passing vacuously.
|
||||
"""
|
||||
|
||||
import socket
|
||||
import threading
|
||||
from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer
|
||||
|
||||
BODY = b'{"q": "search"}'
|
||||
|
||||
|
||||
def _get_with_body(path=b"/api/sessions"):
|
||||
return (
|
||||
b"GET " + path + b" HTTP/1.1\r\n"
|
||||
b"Host: localhost\r\n"
|
||||
b"Content-Type: application/json\r\n"
|
||||
b"Content-Length: " + str(len(BODY)).encode() + b"\r\n"
|
||||
b"\r\n" + BODY
|
||||
)
|
||||
|
||||
|
||||
BODYLESS_GET = (
|
||||
b"GET /api/sessions HTTP/1.1\r\n"
|
||||
b"Host: localhost\r\n"
|
||||
b"\r\n"
|
||||
)
|
||||
FOLLOWING_GET = b"GET /api/health HTTP/1.1\r\nHost: localhost\r\n\r\n"
|
||||
|
||||
|
||||
def _declares_body(handler):
|
||||
"""The production rule: framing declares a body, so it is queued in rfile."""
|
||||
raw = handler.headers.get("Content-Length")
|
||||
if raw is not None:
|
||||
stripped = raw.strip(" \t")
|
||||
if not (stripped.isascii() and stripped.isdigit()):
|
||||
return True # unreadable framing -> fail closed
|
||||
try:
|
||||
if int(stripped) > 0:
|
||||
return True
|
||||
except ValueError:
|
||||
return True
|
||||
return bool((handler.headers.get("Transfer-Encoding") or "").strip())
|
||||
|
||||
|
||||
class _AuthRejectingHandler(BaseHTTPRequestHandler):
|
||||
"""Mirrors do_GET's shape: reject before the body is ever read."""
|
||||
|
||||
protocol_version = "HTTP/1.1"
|
||||
arm_close = True
|
||||
|
||||
def do_GET(self):
|
||||
if self.path == "/api/health":
|
||||
payload = b'{"status": "ok"}'
|
||||
self.send_response(200)
|
||||
self.send_header("Content-Type", "application/json")
|
||||
self.send_header("Content-Length", str(len(payload)))
|
||||
self.end_headers()
|
||||
self.wfile.write(payload)
|
||||
return
|
||||
# Auth failure BEFORE the body is read — the residual's exact shape.
|
||||
if self.arm_close and _declares_body(self):
|
||||
self.close_connection = True
|
||||
payload = b'{"error": "unauthorized"}'
|
||||
self.send_response(401)
|
||||
self.send_header("Content-Type", "application/json")
|
||||
self.send_header("Content-Length", str(len(payload)))
|
||||
if self.arm_close and _declares_body(self):
|
||||
self.send_header("Connection", "close")
|
||||
self.end_headers()
|
||||
self.wfile.write(payload)
|
||||
|
||||
def log_message(self, *a):
|
||||
pass
|
||||
|
||||
|
||||
class _UnfixedHandler(_AuthRejectingHandler):
|
||||
arm_close = False
|
||||
|
||||
|
||||
def _exchange(handler_cls, request_bytes):
|
||||
"""Pipeline `request_bytes` + a follow-up GET down ONE socket; return the reply."""
|
||||
srv = ThreadingHTTPServer(("127.0.0.1", 0), handler_cls)
|
||||
threading.Thread(target=srv.serve_forever, daemon=True).start()
|
||||
try:
|
||||
with socket.create_connection(srv.server_address, timeout=5) as s:
|
||||
s.sendall(request_bytes + FOLLOWING_GET)
|
||||
s.settimeout(5)
|
||||
chunks = []
|
||||
while True:
|
||||
try:
|
||||
b = s.recv(4096)
|
||||
except socket.timeout:
|
||||
break
|
||||
if not b:
|
||||
break
|
||||
chunks.append(b)
|
||||
return b"".join(chunks)
|
||||
finally:
|
||||
srv.shutdown()
|
||||
srv.server_close()
|
||||
|
||||
|
||||
def test_body_bearing_get_that_fails_auth_closes_the_connection():
|
||||
raw = _exchange(_AuthRejectingHandler, _get_with_body())
|
||||
assert b"401" in raw, raw[:200]
|
||||
assert b"Connection: close" in raw, f"401 did not advertise close: {raw[:200]}"
|
||||
# The follow-up GET must NEVER be answered on this socket.
|
||||
assert raw.count(b"HTTP/1.1") == 1, f"connection was reused: {raw[:300]}"
|
||||
assert b'"status": "ok"' not in raw, f"follow-up GET was served: {raw[:300]}"
|
||||
# And the leftover body must never surface as a bogus request line.
|
||||
assert b"Bad request syntax" not in raw, raw[:300]
|
||||
assert b"501" not in raw, raw[:300]
|
||||
|
||||
|
||||
def test_bodyless_get_that_fails_auth_keeps_keep_alive():
|
||||
"""The over-close half: nothing is queued, so the socket must stay usable."""
|
||||
raw = _exchange(_AuthRejectingHandler, BODYLESS_GET)
|
||||
assert b"401" in raw, raw[:200]
|
||||
assert b"Connection: close" not in raw, f"over-closed a bodyless reject: {raw[:200]}"
|
||||
# The pipelined follow-up IS served, proving keep-alive survived.
|
||||
assert b'"status": "ok"' in raw, f"keep-alive was dropped: {raw[:300]}"
|
||||
assert raw.count(b"HTTP/1.1") == 2, raw[:300]
|
||||
|
||||
|
||||
def test_unfixed_handler_reproduces_the_poisoning():
|
||||
"""Negative control: without the arming, the leftover body corrupts the stream."""
|
||||
raw = _exchange(_UnfixedHandler, _get_with_body())
|
||||
assert b"401" in raw, raw[:200]
|
||||
poisoned = (
|
||||
b"Bad request syntax" in raw
|
||||
or b"400" in raw
|
||||
or b"501" in raw
|
||||
or b'"status": "ok"' not in raw
|
||||
)
|
||||
assert poisoned, (
|
||||
"expected the unread body to poison the next request on this socket; "
|
||||
f"got {raw[:300]!r}"
|
||||
)
|
||||
@@ -144,7 +144,7 @@ def test_server_bypasses_auth_for_csp_report(monkeypatch):
|
||||
called["path"] = parsed.path
|
||||
return True
|
||||
|
||||
monkeypatch.setattr("server.check_auth", fail_auth)
|
||||
monkeypatch.setattr("server.check_auth_or_close", fail_auth)
|
||||
monkeypatch.setattr("server.clear_request_profile", lambda: None)
|
||||
monkeypatch.setattr("server.get_profile_cookie", lambda _handler: None)
|
||||
|
||||
|
||||
@@ -0,0 +1,127 @@
|
||||
"""``Handler.do_GET`` itself must arm the close when a body is still queued.
|
||||
|
||||
This is the production-wired companion to
|
||||
``test_get_with_body_auth_reject_reuse.py``. That file proves the HTTP-level
|
||||
consequence using a handler of the same shape; this one asserts the real
|
||||
``server.Handler.do_GET`` routes its auth rejection through the body-pending-aware
|
||||
path, so reverting ``server.py`` to plain ``check_auth`` fails a test.
|
||||
|
||||
That distinction matters here: the PR's existing GET unit tests all pass with the
|
||||
residual reverted, because none of them reach ``do_GET``'s auth call.
|
||||
"""
|
||||
|
||||
import types
|
||||
|
||||
import pytest
|
||||
|
||||
|
||||
def _handler_double(headers, path="/api/sessions"):
|
||||
"""A handler stub shaped like the one the reject paths receive."""
|
||||
h = types.SimpleNamespace()
|
||||
h.path = path
|
||||
h.headers = headers
|
||||
h.close_connection = False
|
||||
h.command = "GET"
|
||||
h._headers_buffer = []
|
||||
h.responses = []
|
||||
h.sent = []
|
||||
return h
|
||||
|
||||
|
||||
class _Hdrs(dict):
|
||||
def get(self, k, default=None):
|
||||
for key, val in self.items():
|
||||
if key.lower() == k.lower():
|
||||
return val[0] if isinstance(val, list) else val
|
||||
return default
|
||||
|
||||
def get_all(self, k, default=None):
|
||||
out = []
|
||||
for key, val in self.items():
|
||||
if key.lower() == k.lower():
|
||||
out.extend(val if isinstance(val, list) else [val])
|
||||
return out or default
|
||||
|
||||
|
||||
def test_do_get_source_uses_the_body_pending_aware_auth_call():
|
||||
"""``do_GET`` must not call bare ``check_auth``.
|
||||
|
||||
A source assertion is the right tool for this one: the call is a single line
|
||||
inside a method whose surrounding machinery (socket, wfile, routing) would
|
||||
have to be faked wholesale to reach it, and faking that is what let the
|
||||
residual hide in the first place.
|
||||
"""
|
||||
import pathlib
|
||||
|
||||
src = pathlib.Path(__file__).resolve().parents[1] / "server.py"
|
||||
text = src.read_text()
|
||||
|
||||
start = text.index("def do_GET")
|
||||
end = text.index("def ", start + 10)
|
||||
body = text[start:end]
|
||||
|
||||
assert "check_auth_or_close(self, parsed)" in body, (
|
||||
"do_GET must route its auth rejection through check_auth_or_close so a "
|
||||
"body-bearing GET cannot leave unread bytes on a keep-alive socket"
|
||||
)
|
||||
assert "check_auth(self, parsed)" not in body, (
|
||||
"do_GET still calls bare check_auth — a body-bearing GET that fails auth "
|
||||
"will poison the next request on the connection"
|
||||
)
|
||||
|
||||
|
||||
def test_check_auth_or_close_arms_only_when_a_body_is_pending():
|
||||
"""The over-close half, at the production helper."""
|
||||
from api.auth import check_auth_or_close
|
||||
import api.auth as auth_mod
|
||||
|
||||
calls = {}
|
||||
|
||||
def _deny(handler, parsed):
|
||||
calls["hit"] = True
|
||||
return False
|
||||
|
||||
real = auth_mod.check_auth
|
||||
auth_mod.check_auth = _deny
|
||||
try:
|
||||
# Body pending -> close armed.
|
||||
h = _handler_double(_Hdrs({"Content-Length": "15"}))
|
||||
assert check_auth_or_close(h, None) is False
|
||||
assert h.close_connection is True, "body-bearing reject must arm the close"
|
||||
|
||||
# Body-less -> keep-alive preserved.
|
||||
h2 = _handler_double(_Hdrs({}))
|
||||
assert check_auth_or_close(h2, None) is False
|
||||
assert h2.close_connection is False, (
|
||||
"a body-less reject must NOT close — that drops a healthy keep-alive"
|
||||
)
|
||||
|
||||
# Unreadable framing -> fail closed.
|
||||
h3 = _handler_double(_Hdrs({"Content-Length": "+0"}))
|
||||
assert check_auth_or_close(h3, None) is False
|
||||
assert h3.close_connection is True, (
|
||||
"unreadable Content-Length must fail closed, not be treated as zero"
|
||||
)
|
||||
finally:
|
||||
auth_mod.check_auth = real
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"value",
|
||||
["\xa00", "0\x85", "+0", "-0", "0_0", "\u0660", "-5", ""],
|
||||
)
|
||||
def test_adversarial_zero_spellings_declare_a_body(value):
|
||||
"""Every spelling of "zero" that ``int()`` accepts must still fail closed.
|
||||
|
||||
``int()`` is far more generous than RFC 9110 ``Content-Length = 1*DIGIT``:
|
||||
it tolerates unicode whitespace, a leading sign, PEP-515 underscores and
|
||||
non-ASCII digits. Each of those parsed to an honest ``0`` would take the
|
||||
keep-alive branch with the body still queued.
|
||||
"""
|
||||
from api.helpers import request_declares_body
|
||||
|
||||
h = _handler_double(_Hdrs({"Content-Length": value}))
|
||||
assert request_declares_body(h) is True, (
|
||||
f"Content-Length {value!r} was treated as no-body; a queued body would "
|
||||
"then poison the next request on this connection"
|
||||
)
|
||||
File diff suppressed because it is too large.
Load diff
@@ -0,0 +1,759 @@
|
||||
"""Pooled-client regression for reject-before-read on HTTP/1.1 keep-alive.
|
||||
|
||||
The unit tests beside this one assert `close_connection` is set on each
|
||||
rejection path, and the raw-socket test drives the failure over one socket.
|
||||
Neither proves the client side: a pooled client (`http.client`) only avoids
|
||||
the broken connection when the rejection ADVERTISES `Connection: close`.
|
||||
Without the advertisement the server drops the socket silently and the next
|
||||
pooled request dies with BrokenPipeError/RemoteDisconnected — or, without the
|
||||
close at all, the unread body is parsed as the next request line and the
|
||||
client gets a 400 about a request it never sent.
|
||||
|
||||
This file drives the production handler through the real test server and
|
||||
asserts both halves: the rejection carries `Connection: close`, and a
|
||||
follow-up request on the same pooled connection succeeds.
|
||||
|
||||
The deprecated `/api/process-complete-ack` path is the probe: it answers 410
|
||||
before reading its JSON body, by design (it runs ahead of the CSRF gate so a
|
||||
stale tab gets 410, not 403).
|
||||
"""
|
||||
|
||||
import contextlib
|
||||
import http.client
|
||||
import socket
|
||||
import threading
|
||||
import urllib.parse
|
||||
from http.server import ThreadingHTTPServer
|
||||
|
||||
import pytest
|
||||
|
||||
from tests._pytest_port import BASE
|
||||
|
||||
_PORT = urllib.parse.urlparse(BASE).port
|
||||
_BODY = b'{"stale": true}'
|
||||
|
||||
|
||||
def _reject_with_body_then_pooled_followup() -> tuple[int, str | None, int]:
|
||||
"""POST a deprecated endpoint with a body, then GET on the same pool."""
|
||||
conn = http.client.HTTPConnection("127.0.0.1", _PORT, timeout=10)
|
||||
try:
|
||||
conn.request(
|
||||
"POST",
|
||||
"/api/process-complete-ack",
|
||||
body=_BODY,
|
||||
headers={"Content-Type": "application/json"},
|
||||
)
|
||||
resp = conn.getresponse()
|
||||
reject_status = resp.status
|
||||
close_header = resp.getheader("Connection")
|
||||
resp.read()
|
||||
|
||||
conn.request("GET", "/api/health/agent")
|
||||
follow = conn.getresponse()
|
||||
follow_status = follow.status
|
||||
follow.read()
|
||||
finally:
|
||||
conn.close()
|
||||
return reject_status, close_header, follow_status
|
||||
|
||||
|
||||
def test_rejected_write_advertises_close_and_pooled_followup_succeeds():
|
||||
status, close_header, follow_status = _reject_with_body_then_pooled_followup()
|
||||
assert status == 410, f"expected the deprecated-path 410, got {status}"
|
||||
assert close_header == "close", (
|
||||
f"rejection must advertise Connection: close so pooled clients don't "
|
||||
f"reuse the dead socket; got {close_header!r}"
|
||||
)
|
||||
assert follow_status == 200, (
|
||||
f"pooled follow-up after a reject-before-read must succeed; got {follow_status}"
|
||||
)
|
||||
|
||||
|
||||
# ── Framing the Content-Length gate could not see ─────────────────────────────
|
||||
#
|
||||
# Both cases below are pipelined down ONE socket against the real server: the
|
||||
# reject-before-read request first, an ordinary GET immediately after it in the
|
||||
# same write. If the rejection closes, the server answers once and EOFs and the
|
||||
# GET is never seen. If it does not, the leftover body/chunk bytes are parsed as
|
||||
# the next request line and a second response comes back — a 400 bad-syntax or
|
||||
# `501 Unsupported method` naming bytes the client never sent as a request.
|
||||
|
||||
_FOLLOWING_GET = b"GET /api/health/agent HTTP/1.1\r\nHost: 127.0.0.1\r\nAccept: */*\r\n\r\n"
|
||||
_MULTIPART_UPLOAD_PATHS = (
|
||||
"/api/upload",
|
||||
"/api/upload/extract",
|
||||
"/api/workspace/upload",
|
||||
"/api/transcribe",
|
||||
)
|
||||
_MULTIPART_PAYLOAD = (
|
||||
b'--x\r\nContent-Disposition: form-data; name="file"; filename="a.txt"\r\n'
|
||||
b"\r\nhello\r\n--x--\r\n"
|
||||
)
|
||||
_CHUNKED_MULTIPART_BODY = (
|
||||
f"{len(_MULTIPART_PAYLOAD):X}\r\n".encode() + _MULTIPART_PAYLOAD + b"\r\n0\r\n\r\n"
|
||||
)
|
||||
|
||||
|
||||
def _pipelined_after(
|
||||
request: bytes,
|
||||
*,
|
||||
stop_after: int | None = None,
|
||||
port: int | None = None,
|
||||
follow: bytes = _FOLLOWING_GET,
|
||||
) -> bytes:
|
||||
"""Send *request* and a plain GET in one write; return every byte answered.
|
||||
|
||||
Reads to EOF by default, which is the whole point of the closing cases: the
|
||||
proof is that NOTHING follows the single response. `stop_after` bounds the
|
||||
read for the keep-alive cases, where the socket stays open by design and
|
||||
reading to EOF would only burn the timeout.
|
||||
|
||||
`port`/`follow` exist for the auth-enabled cases below, which need their own
|
||||
server instance and a PUBLIC follow-up path.
|
||||
"""
|
||||
sock = socket.create_connection(("127.0.0.1", port or _PORT), timeout=10)
|
||||
try:
|
||||
sock.sendall(request + follow)
|
||||
received = b""
|
||||
while stop_after is None or received.count(b"HTTP/1.1 ") < stop_after:
|
||||
try:
|
||||
chunk = sock.recv(65536)
|
||||
except (TimeoutError, socket.timeout, ConnectionError):
|
||||
break
|
||||
if not chunk:
|
||||
break
|
||||
received += chunk
|
||||
return received
|
||||
finally:
|
||||
sock.close()
|
||||
|
||||
|
||||
def _assert_single_closed_response(answered: bytes, status: bytes, leftover: bytes) -> None:
|
||||
text = answered.decode("latin-1", errors="replace")
|
||||
assert answered.startswith(b"HTTP/1.1 " + status), text
|
||||
assert b"Connection: close" in answered, text
|
||||
assert answered.count(b"HTTP/1.1 ") == 1, (
|
||||
f"the pipelined GET was answered, so the socket stayed open: {text}"
|
||||
)
|
||||
assert leftover not in answered, f"the unread body reached the request parser: {text}"
|
||||
assert b"Bad request syntax" not in answered, text
|
||||
assert b"Unsupported method" not in answered, text
|
||||
|
||||
|
||||
@pytest.mark.parametrize("path", _MULTIPART_UPLOAD_PATHS)
|
||||
def test_chunked_upload_rejection_closes_and_cannot_poison_the_socket(path):
|
||||
"""A chunked upload has no Content-Length — the old gate read it as empty.
|
||||
|
||||
All four multipart handlers then answered 400 "No file field in request"
|
||||
with keep-alive intact, leaving the chunk bytes on the socket.
|
||||
"""
|
||||
answered = _pipelined_after(
|
||||
f"POST {path} HTTP/1.1\r\n"
|
||||
"Host: 127.0.0.1\r\n"
|
||||
"Content-Type: multipart/form-data; boundary=x\r\n"
|
||||
"Transfer-Encoding: chunked\r\n"
|
||||
"\r\n".encode() + _CHUNKED_MULTIPART_BODY
|
||||
)
|
||||
|
||||
_assert_single_closed_response(answered, b"411", _MULTIPART_PAYLOAD)
|
||||
|
||||
|
||||
def test_sidecar_get_with_a_body_closes_and_cannot_poison_the_socket():
|
||||
"""A provenance-rejected GET that carries a declared body.
|
||||
|
||||
`read_request_body=False` said "no body" and the 403 went out with
|
||||
keep-alive; the gate's repro then got `501 Unsupported method ('{}GET')` on
|
||||
the same socket. No Origin/Referer/Sec-Fetch-Site here, so provenance fails
|
||||
before the body would ever be read.
|
||||
"""
|
||||
answered = _pipelined_after(
|
||||
"GET /api/extensions/probe/sidecar/ping HTTP/1.1\r\n"
|
||||
"Host: 127.0.0.1\r\n"
|
||||
"Content-Type: application/json\r\n"
|
||||
f"Content-Length: {len(_BODY)}\r\n"
|
||||
"\r\n".encode() + _BODY
|
||||
)
|
||||
|
||||
_assert_single_closed_response(answered, b"403", _BODY)
|
||||
|
||||
|
||||
# ── Framing hidden behind a DUPLICATED header ─────────────────────────────────
|
||||
#
|
||||
# `Content-Length: 0` sent ahead of the real length: `Message.get()` returns only
|
||||
# the first value, so every reader saw an empty body and drained nothing. Both
|
||||
# cases below were reproduced live on the otherwise-fixed head, poisoning the
|
||||
# socket exactly as the single-header cases used to:
|
||||
# sidecar GET -> 403, then 400 Bad request syntax ('{"stale": true}GET /...')
|
||||
# /api/upload -> 400, then 400 Bad request syntax ('--x')
|
||||
|
||||
|
||||
def test_duplicate_content_length_sidecar_get_closes_and_cannot_poison_the_socket():
|
||||
answered = _pipelined_after(
|
||||
"GET /api/extensions/probe/sidecar/ping HTTP/1.1\r\n"
|
||||
"Host: 127.0.0.1\r\n"
|
||||
"Content-Type: application/json\r\n"
|
||||
"Content-Length: 0\r\n"
|
||||
f"Content-Length: {len(_BODY)}\r\n"
|
||||
"\r\n".encode() + _BODY
|
||||
)
|
||||
|
||||
_assert_single_closed_response(answered, b"403", _BODY)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("path", _MULTIPART_UPLOAD_PATHS)
|
||||
def test_duplicate_content_length_upload_closes_and_cannot_poison_the_socket(path):
|
||||
answered = _pipelined_after(
|
||||
f"POST {path} HTTP/1.1\r\n"
|
||||
"Host: 127.0.0.1\r\n"
|
||||
"Content-Type: multipart/form-data; boundary=x\r\n"
|
||||
"Content-Length: 0\r\n"
|
||||
f"Content-Length: {len(_MULTIPART_PAYLOAD)}\r\n"
|
||||
"\r\n".encode() + _MULTIPART_PAYLOAD
|
||||
)
|
||||
|
||||
_assert_single_closed_response(answered, b"400", _MULTIPART_PAYLOAD)
|
||||
|
||||
|
||||
# ── Framing hidden behind a BLANK header value ────────────────────────────────
|
||||
#
|
||||
# `Content-Length:` with nothing after the colon reads back as '' and was skipped
|
||||
# as "no length declared", so the rejection kept the connection alive. Both cases
|
||||
# below were reproduced live on the otherwise-fixed head:
|
||||
# sidecar GET -> 403, then 400 Bad request syntax ('{"stale": true}GET /...')
|
||||
# /api/upload -> 400, then 400 Bad request syntax ('--x')
|
||||
# `Message` collapses `Content-Length:` and `Content-Length: ` to the same '',
|
||||
# and a blank `Transfer-Encoding:` reached the allowlist of codings that "framed
|
||||
# nothing" (since deleted — see the identity section below), so each spelling
|
||||
# below poisoned the socket the same way.
|
||||
|
||||
_BLANK_FRAMING_HEADER_LINES = (
|
||||
b"Content-Length:\r\n",
|
||||
b"Content-Length: \r\n",
|
||||
b"Content-Length:\r\nContent-Length:\r\n",
|
||||
b"Content-Length: 0\r\nContent-Length:\r\n",
|
||||
b"Transfer-Encoding:\r\n",
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("framing", _BLANK_FRAMING_HEADER_LINES)
|
||||
def test_blank_framing_sidecar_get_closes_and_cannot_poison_the_socket(framing):
|
||||
answered = _pipelined_after(
|
||||
b"GET /api/extensions/probe/sidecar/ping HTTP/1.1\r\n"
|
||||
b"Host: 127.0.0.1\r\n"
|
||||
b"Content-Type: application/json\r\n" + framing + b"\r\n" + _BODY
|
||||
)
|
||||
|
||||
_assert_single_closed_response(answered, b"403", _BODY)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("path", _MULTIPART_UPLOAD_PATHS)
|
||||
@pytest.mark.parametrize(
|
||||
("framing", "status"),
|
||||
[
|
||||
(b"Content-Length:\r\n", b"400"),
|
||||
(b"Content-Length: \r\n", b"400"),
|
||||
(b"Transfer-Encoding:\r\n", b"411"),
|
||||
],
|
||||
ids=["blank-length", "whitespace-only-length", "blank-transfer-encoding"],
|
||||
)
|
||||
def test_blank_framing_upload_closes_and_cannot_poison_the_socket(path, framing, status):
|
||||
answered = _pipelined_after(
|
||||
f"POST {path} HTTP/1.1\r\n"
|
||||
"Host: 127.0.0.1\r\n"
|
||||
"Content-Type: multipart/form-data; boundary=x\r\n".encode()
|
||||
+ framing
|
||||
+ b"\r\n"
|
||||
+ _MULTIPART_PAYLOAD
|
||||
)
|
||||
|
||||
_assert_single_closed_response(answered, status, _MULTIPART_PAYLOAD)
|
||||
|
||||
|
||||
# ── Framing only `int()` reads as zero ────────────────────────────────────────
|
||||
#
|
||||
# RFC 9110 is `Content-Length = 1*DIGIT`; `int()` also takes a sign, PEP 515
|
||||
# underscores and non-ASCII whitespace padding, so each value below parsed to an
|
||||
# honest `0`, took the "every declared length agrees on zero" keep-alive branch
|
||||
# and left the payload queued. Reproduced live on the otherwise-fixed head:
|
||||
# sidecar GET -> 403 (no Connection: close), then
|
||||
# 400 Bad request syntax ('{"stale": true}GET /api/health/agent HTTP/1.1')
|
||||
# /api/upload -> 400, then 400 Bad request syntax ('--x')
|
||||
# `Transfer-Encoding: \xa0identity` is the same mistake one header over: strip()
|
||||
# erased the U+00A0 and it read back as the `identity` token. Everything here is
|
||||
# wire-reachable -- a request line decodes as latin-1, so U+00A0 and U+0085 pass
|
||||
# through untouched (a non-ASCII DIGIT such as U+0660 cannot, which is why that
|
||||
# family is pinned at the helper level in test_rejected_write_connection_close.py).
|
||||
|
||||
_MALFORMED_ZERO_FRAMING_LINES = [
|
||||
b"Content-Length: +0\r\n",
|
||||
b"Content-Length: -0\r\n",
|
||||
b"Content-Length: 0_0\r\n",
|
||||
b"Content-Length: +00\r\n",
|
||||
b"Content-Length: 0_0_0\r\n",
|
||||
b"Content-Length: \xa00\r\n",
|
||||
b"Content-Length: 0\x85\r\n",
|
||||
b"Content-Length: 0\r\nContent-Length: +0\r\n",
|
||||
b"Transfer-Encoding: \xa0identity\r\n",
|
||||
]
|
||||
|
||||
|
||||
@pytest.mark.parametrize("framing", _MALFORMED_ZERO_FRAMING_LINES)
|
||||
def test_malformed_zero_framing_sidecar_get_closes_and_cannot_poison_the_socket(framing):
|
||||
answered = _pipelined_after(
|
||||
b"GET /api/extensions/probe/sidecar/ping HTTP/1.1\r\n"
|
||||
b"Host: 127.0.0.1\r\n"
|
||||
b"Content-Type: application/json\r\n" + framing + b"\r\n" + _BODY
|
||||
)
|
||||
|
||||
_assert_single_closed_response(answered, b"403", _BODY)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("path", _MULTIPART_UPLOAD_PATHS)
|
||||
@pytest.mark.parametrize(
|
||||
("framing", "status"),
|
||||
[
|
||||
(b"Content-Length: +0\r\n", b"400"),
|
||||
(b"Content-Length: 0_0\r\n", b"400"),
|
||||
(b"Content-Length: \xa00\r\n", b"400"),
|
||||
(b"Transfer-Encoding: \xa0identity\r\n", b"411"),
|
||||
],
|
||||
ids=["sign", "underscore", "nbsp-padded", "nbsp-padded-identity"],
|
||||
)
|
||||
def test_malformed_zero_framing_upload_closes_and_cannot_poison_the_socket(path, framing, status):
|
||||
answered = _pipelined_after(
|
||||
f"POST {path} HTTP/1.1\r\n"
|
||||
"Host: 127.0.0.1\r\n"
|
||||
"Content-Type: multipart/form-data; boundary=x\r\n".encode()
|
||||
+ framing
|
||||
+ b"\r\n"
|
||||
+ _MULTIPART_PAYLOAD
|
||||
)
|
||||
|
||||
_assert_single_closed_response(answered, status, _MULTIPART_PAYLOAD)
|
||||
|
||||
|
||||
# ── `Transfer-Encoding: identity` frames nothing a reader can trust ───────────
|
||||
#
|
||||
# `identity` sat in an allowlist of codings that "frame nothing", so a request
|
||||
# carrying it read back as body-less however many payload bytes followed. RFC
|
||||
# 9112 §6.3 leaves no room for that: a request whose final transfer coding is not
|
||||
# `chunked` cannot be framed by a recipient, so it must be refused and the
|
||||
# connection closed — and `http.server` decodes no transfer coding at all, so
|
||||
# `identity` is no more readable here than `chunked` is. Reproduced live on the
|
||||
# otherwise-fixed head, pipelined down one socket:
|
||||
#
|
||||
# GET /api/extensions/probe/sidecar/ping `Transfer-Encoding: identity`
|
||||
# {"stale": true}
|
||||
# -> HTTP/1.1 403 Forbidden (no Connection: close)
|
||||
# -> HTTP/1.1 400 Bad request syntax
|
||||
# ('{"stale": true}GET /api/health/agent HTTP/1.1')
|
||||
# POST /api/upload `Transfer-Encoding: identity` --x...
|
||||
# -> HTTP/1.1 400 Bad Request (no Connection: close, "No file field")
|
||||
# -> HTTP/1.1 400 Bad request syntax ('--x')
|
||||
#
|
||||
# Only the spellings that normalize to the bare token were poisoned — every
|
||||
# neighbouring value (`identity, chunked`, `identity,identity`, a lone comma, an
|
||||
# unknown coding, a non-OWS-padded token, a blank value) already missed the
|
||||
# allowlist and already closed. Those rows are in the sweep below anyway: this is
|
||||
# the fourth round in which an adjacent spelling of one framing rule survived a
|
||||
# fix, so the whole neighbourhood gets pinned rather than just the reported case.
|
||||
|
||||
_IDENTITY_FRAMING_LINES = [
|
||||
b"Transfer-Encoding: identity\r\n",
|
||||
b"Transfer-Encoding: IDENTITY\r\n",
|
||||
b"Transfer-Encoding: Identity\r\n",
|
||||
b"Transfer-Encoding: identity \r\n",
|
||||
b"Transfer-Encoding: \tidentity\t\r\n",
|
||||
b"Transfer-Encoding: identity\r\nTransfer-Encoding: identity\r\n",
|
||||
b"Transfer-Encoding: identity\r\nContent-Length: 0\r\n",
|
||||
b"Transfer-Encoding: identity\r\nContent-Length: " + str(len(_BODY)).encode() + b"\r\n",
|
||||
b"Transfer-Encoding: identity, chunked\r\n",
|
||||
b"Transfer-Encoding: chunked, identity\r\n",
|
||||
b"Transfer-Encoding: identity,identity\r\n",
|
||||
b"Transfer-Encoding: ,\r\n",
|
||||
b"Transfer-Encoding: banana\r\n",
|
||||
]
|
||||
|
||||
|
||||
@pytest.mark.parametrize("framing", _IDENTITY_FRAMING_LINES)
|
||||
def test_identity_framing_sidecar_get_closes_and_cannot_poison_the_socket(framing):
|
||||
answered = _pipelined_after(
|
||||
b"GET /api/extensions/probe/sidecar/ping HTTP/1.1\r\n"
|
||||
b"Host: 127.0.0.1\r\n"
|
||||
b"Content-Type: application/json\r\n" + framing + b"\r\n" + _BODY
|
||||
)
|
||||
|
||||
_assert_single_closed_response(answered, b"403", _BODY)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("path", _MULTIPART_UPLOAD_PATHS)
|
||||
@pytest.mark.parametrize(
|
||||
"framing",
|
||||
[
|
||||
b"Transfer-Encoding: identity\r\n",
|
||||
b"Transfer-Encoding: IDENTITY \r\n",
|
||||
b"Transfer-Encoding: identity\r\nContent-Length: 0\r\n",
|
||||
],
|
||||
ids=["identity", "identity-cased-and-padded", "identity-plus-zero-length"],
|
||||
)
|
||||
def test_identity_framing_upload_closes_and_cannot_poison_the_socket(path, framing):
|
||||
"""All four multipart handlers, mirroring the chunked cases above.
|
||||
|
||||
`identity` carries no Content-Length either, so the length-0 read found no
|
||||
parts and each handler answered 400 "No file field in request" with the whole
|
||||
multipart payload still queued on an open socket.
|
||||
"""
|
||||
answered = _pipelined_after(
|
||||
f"POST {path} HTTP/1.1\r\n"
|
||||
"Host: 127.0.0.1\r\n"
|
||||
"Content-Type: multipart/form-data; boundary=x\r\n".encode()
|
||||
+ framing
|
||||
+ b"\r\n"
|
||||
+ _MULTIPART_PAYLOAD
|
||||
)
|
||||
|
||||
_assert_single_closed_response(answered, b"411", _MULTIPART_PAYLOAD)
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"framing",
|
||||
[
|
||||
b"",
|
||||
b"Content-Length: 0\r\n",
|
||||
b"Content-Length: 00\r\n",
|
||||
b"Content-Length: 0 \r\n",
|
||||
b"Content-Length: 0\r\nContent-Length: 0\r\n",
|
||||
],
|
||||
ids=[
|
||||
"no-length",
|
||||
"zero-length",
|
||||
"double-zero-length",
|
||||
"ows-padded-zero",
|
||||
"two-agreeing-zeroes",
|
||||
],
|
||||
)
|
||||
def test_bodyless_framing_keeps_the_pooled_socket_alive(framing):
|
||||
"""The over-close half of the contract, on the wire.
|
||||
|
||||
Framing that positively says "no body" must NOT be swept up by the blank
|
||||
rule: the rejection answers without `Connection: close` and the pipelined GET
|
||||
is served on the same socket. Two agreeing zeroes are still no body.
|
||||
"""
|
||||
answered = _pipelined_after(
|
||||
b"GET /api/extensions/probe/sidecar/ping HTTP/1.1\r\n"
|
||||
b"Host: 127.0.0.1\r\n" + framing + b"\r\n",
|
||||
stop_after=2,
|
||||
)
|
||||
|
||||
text = answered.decode("latin-1", errors="replace")
|
||||
assert answered.startswith(b"HTTP/1.1 403"), text
|
||||
assert b"HTTP/1.1 200 OK" in answered, (
|
||||
f"the pipelined GET was not served, so a body-less rejection dropped a "
|
||||
f"healthy pooled connection: {text}"
|
||||
)
|
||||
assert b"Connection: close" not in answered, text
|
||||
|
||||
|
||||
# ── Every reject-before-read site, in BOTH directions, on the wire ─────────────
|
||||
#
|
||||
# The sidecar path above was the first site routed through the framing-aware
|
||||
# helper; the sibling sites kept the unconditional `arm_connection_close()` and
|
||||
# so were the mirror image of the same bug -- a body-less rejection answered with
|
||||
# `Connection: close` and dropped the client's pipelined follow-up, killing a
|
||||
# healthy keep-alive connection for no framing reason. Each pair below is the
|
||||
# reproduction (body-less -> must stay alive) beside its close half (a declared
|
||||
# body -> must still close), pipelined down one socket against the production
|
||||
# handler. Reproduced on the pre-fix head, all seven sites, `responses seen: 1`:
|
||||
#
|
||||
# POST /api/session/new (auth off-session) -> 401 + Connection: close
|
||||
# POST /api/session/new (bad Origin) -> 403 + Connection: close
|
||||
# POST /api/session/new (no CSRF token) -> 403 + Connection: close
|
||||
# POST /api/csp-report (limiter tripped) -> 204 + Connection: close
|
||||
# POST /api/health/restart (success) -> 200 + Connection: close
|
||||
# POST /api/process-complete-ack -> 410 + Connection: close
|
||||
# POST /api/upload (no boundary) -> 400 + Connection: close
|
||||
|
||||
_BODYLESS_FRAMING = [b"", b"Content-Length: 0\r\n"]
|
||||
_BODYLESS_IDS = ["no-content-length", "zero-content-length"]
|
||||
# A PUBLIC path, so the follow-up is a genuine 200 even with auth enabled.
|
||||
_PUBLIC_FOLLOWING_GET = (
|
||||
b"GET /api/auth/status HTTP/1.1\r\nHost: 127.0.0.1\r\nAccept: */*\r\n\r\n"
|
||||
)
|
||||
|
||||
|
||||
@contextlib.contextmanager
|
||||
def _own_server():
|
||||
"""The production Handler on a private port.
|
||||
|
||||
The shared test server runs with auth disabled and out of process, so the
|
||||
auth/CSRF-token rejections and the deterministic restart/limiter outcomes need
|
||||
an instance this process can configure. It is the same `server.Handler` class
|
||||
the real server binds, driven over a real socket.
|
||||
"""
|
||||
import server
|
||||
|
||||
httpd = ThreadingHTTPServer(("127.0.0.1", 0), server.Handler)
|
||||
thread = threading.Thread(target=httpd.serve_forever, daemon=True)
|
||||
thread.start()
|
||||
try:
|
||||
yield httpd.server_address[1]
|
||||
finally:
|
||||
httpd.shutdown()
|
||||
httpd.server_close()
|
||||
thread.join(timeout=10)
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def auth_on(monkeypatch):
|
||||
"""Turn password auth on for this test (conftest resets the hash cache)."""
|
||||
import api.auth as auth
|
||||
|
||||
monkeypatch.setenv("HERMES_WEBUI_PASSWORD", "pooled-client-regression")
|
||||
auth._invalidate_password_hash_cache()
|
||||
assert auth.is_auth_enabled(), "the auth-path regression needs auth enabled"
|
||||
return auth
|
||||
|
||||
|
||||
def _assert_kept_alive(answered: bytes, status: bytes) -> None:
|
||||
text = answered.decode("latin-1", errors="replace")
|
||||
assert answered.startswith(b"HTTP/1.1 " + status), text
|
||||
assert b"Connection: close" not in answered, (
|
||||
f"a body-less rejection advertised close, so a pooled client's healthy "
|
||||
f"connection was dropped: {text}"
|
||||
)
|
||||
assert b"HTTP/1.1 200 OK" in answered, (
|
||||
f"the pipelined follow-up was not served on the same socket: {text}"
|
||||
)
|
||||
|
||||
|
||||
def _assert_closed(answered: bytes, status: bytes) -> None:
|
||||
text = answered.decode("latin-1", errors="replace")
|
||||
assert answered.startswith(b"HTTP/1.1 " + status), text
|
||||
assert b"Connection: close" in answered, text
|
||||
assert answered.count(b"HTTP/1.1 ") == 1, (
|
||||
f"the pipelined GET was answered, so the socket stayed open: {text}"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("framing", _BODYLESS_FRAMING, ids=_BODYLESS_IDS)
|
||||
def test_bodyless_auth_rejection_keeps_the_pooled_socket_alive(framing, auth_on):
|
||||
"""A body-less POST that fails auth must not cost the client its connection."""
|
||||
with _own_server() as port:
|
||||
answered = _pipelined_after(
|
||||
b"POST /api/session/new HTTP/1.1\r\nHost: 127.0.0.1\r\n" + framing + b"\r\n",
|
||||
stop_after=2,
|
||||
port=port,
|
||||
follow=_PUBLIC_FOLLOWING_GET,
|
||||
)
|
||||
|
||||
_assert_kept_alive(answered, b"401")
|
||||
|
||||
|
||||
def test_auth_rejection_with_a_body_still_closes_the_pooled_socket(auth_on):
|
||||
"""The close half at the auth gate: those bytes are still queued in rfile."""
|
||||
with _own_server() as port:
|
||||
answered = _pipelined_after(
|
||||
b"POST /api/session/new HTTP/1.1\r\nHost: 127.0.0.1\r\n"
|
||||
b"Content-Type: application/json\r\n"
|
||||
b"Content-Length: " + str(len(_BODY)).encode() + b"\r\n\r\n" + _BODY,
|
||||
port=port,
|
||||
follow=_PUBLIC_FOLLOWING_GET,
|
||||
)
|
||||
|
||||
_assert_closed(answered, b"401")
|
||||
assert _BODY not in answered, answered.decode("latin-1", errors="replace")
|
||||
|
||||
|
||||
@pytest.mark.parametrize("framing", _BODYLESS_FRAMING, ids=_BODYLESS_IDS)
|
||||
def test_bodyless_csrf_origin_rejection_keeps_the_pooled_socket_alive(framing):
|
||||
"""`_check_csrf()` origin mismatch, on the shared server (auth off)."""
|
||||
answered = _pipelined_after(
|
||||
b"POST /api/session/new HTTP/1.1\r\nHost: 127.0.0.1\r\n"
|
||||
b"Origin: http://evil.invalid\r\n" + framing + b"\r\n",
|
||||
stop_after=2,
|
||||
)
|
||||
|
||||
_assert_kept_alive(answered, b"403")
|
||||
|
||||
|
||||
def test_csrf_origin_rejection_with_a_body_still_closes_the_pooled_socket():
|
||||
answered = _pipelined_after(
|
||||
b"POST /api/session/new HTTP/1.1\r\nHost: 127.0.0.1\r\n"
|
||||
b"Origin: http://evil.invalid\r\nContent-Type: application/json\r\n"
|
||||
b"Content-Length: " + str(len(_BODY)).encode() + b"\r\n\r\n" + _BODY
|
||||
)
|
||||
|
||||
_assert_closed(answered, b"403")
|
||||
assert _BODY not in answered, answered.decode("latin-1", errors="replace")
|
||||
|
||||
|
||||
def _authenticated_same_origin_post(port, cookie_name, cookie, framing, body=b""):
|
||||
return (
|
||||
f"POST /api/session/new HTTP/1.1\r\nHost: 127.0.0.1:{port}\r\n"
|
||||
f"Origin: http://127.0.0.1:{port}\r\n"
|
||||
f"Cookie: {cookie_name}={cookie}\r\n".encode() + framing + b"\r\n" + body
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("framing", _BODYLESS_FRAMING, ids=_BODYLESS_IDS)
|
||||
def test_bodyless_csrf_token_rejection_keeps_the_pooled_socket_alive(framing, auth_on):
|
||||
"""A real session, a same-origin POST, and no CSRF token -> token_mismatch."""
|
||||
cookie = auth_on.create_session()
|
||||
try:
|
||||
with _own_server() as port:
|
||||
answered = _pipelined_after(
|
||||
_authenticated_same_origin_post(
|
||||
port, auth_on._resolve_cookie_name(), cookie, framing
|
||||
),
|
||||
stop_after=2,
|
||||
port=port,
|
||||
follow=_PUBLIC_FOLLOWING_GET,
|
||||
)
|
||||
finally:
|
||||
auth_on.invalidate_session(cookie)
|
||||
|
||||
_assert_kept_alive(answered, b"403")
|
||||
|
||||
|
||||
def test_csrf_token_rejection_with_a_body_still_closes_the_pooled_socket(auth_on):
|
||||
cookie = auth_on.create_session()
|
||||
try:
|
||||
with _own_server() as port:
|
||||
answered = _pipelined_after(
|
||||
_authenticated_same_origin_post(
|
||||
port,
|
||||
auth_on._resolve_cookie_name(),
|
||||
cookie,
|
||||
b"Content-Type: application/json\r\nContent-Length: "
|
||||
+ str(len(_BODY)).encode()
|
||||
+ b"\r\n",
|
||||
body=_BODY,
|
||||
),
|
||||
port=port,
|
||||
follow=_PUBLIC_FOLLOWING_GET,
|
||||
)
|
||||
finally:
|
||||
auth_on.invalidate_session(cookie)
|
||||
|
||||
_assert_closed(answered, b"403")
|
||||
assert _BODY not in answered, answered.decode("latin-1", errors="replace")
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def csp_limiter_tripped(monkeypatch):
|
||||
"""Force the CSP-report limiter, instead of sending 101 reports in 60s."""
|
||||
import api.routes as routes
|
||||
|
||||
monkeypatch.setattr(routes, "_csp_report_rate_limited", lambda _handler: True)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("framing", _BODYLESS_FRAMING, ids=_BODYLESS_IDS)
|
||||
def test_bodyless_rate_limited_csp_report_keeps_the_pooled_socket_alive(
|
||||
framing, csp_limiter_tripped
|
||||
):
|
||||
"""A dropped 204 must not hang up on a browser that is still reporting."""
|
||||
with _own_server() as port:
|
||||
answered = _pipelined_after(
|
||||
b"POST /api/csp-report HTTP/1.1\r\nHost: 127.0.0.1\r\n" + framing + b"\r\n",
|
||||
stop_after=2,
|
||||
port=port,
|
||||
follow=_PUBLIC_FOLLOWING_GET,
|
||||
)
|
||||
|
||||
_assert_kept_alive(answered, b"204")
|
||||
|
||||
|
||||
def test_rate_limited_csp_report_with_a_body_still_closes(csp_limiter_tripped):
|
||||
"""The limiter answers without reading the report, so a declared body closes."""
|
||||
with _own_server() as port:
|
||||
answered = _pipelined_after(
|
||||
b"POST /api/csp-report HTTP/1.1\r\nHost: 127.0.0.1\r\n"
|
||||
b"Content-Type: application/csp-report\r\n"
|
||||
b"Content-Length: " + str(len(_BODY)).encode() + b"\r\n\r\n" + _BODY,
|
||||
port=port,
|
||||
follow=_PUBLIC_FOLLOWING_GET,
|
||||
)
|
||||
|
||||
_assert_closed(answered, b"204")
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def restart_succeeds(monkeypatch):
|
||||
"""Pin the SUCCESS outcome -- the one that closed a healthy socket every call."""
|
||||
import api.routes as routes
|
||||
|
||||
monkeypatch.setattr(
|
||||
routes, "restart_active_profile_gateway", lambda: {"status": "completed"}
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("framing", _BODYLESS_FRAMING, ids=_BODYLESS_IDS)
|
||||
def test_bodyless_successful_restart_keeps_the_pooled_socket_alive(framing, restart_succeeds):
|
||||
"""`/api/health/restart` closed on success too; the WebUI sends no body."""
|
||||
with _own_server() as port:
|
||||
answered = _pipelined_after(
|
||||
b"POST /api/health/restart HTTP/1.1\r\nHost: 127.0.0.1\r\n" + framing + b"\r\n",
|
||||
stop_after=2,
|
||||
port=port,
|
||||
follow=_PUBLIC_FOLLOWING_GET,
|
||||
)
|
||||
|
||||
_assert_kept_alive(answered, b"200")
|
||||
|
||||
|
||||
def test_restart_with_a_body_still_closes_the_pooled_socket(restart_succeeds):
|
||||
"""The endpoint consumes its body on no outcome, so a declared one closes."""
|
||||
with _own_server() as port:
|
||||
answered = _pipelined_after(
|
||||
b"POST /api/health/restart HTTP/1.1\r\nHost: 127.0.0.1\r\n"
|
||||
b"Content-Type: application/json\r\n"
|
||||
b"Content-Length: " + str(len(_BODY)).encode() + b"\r\n\r\n" + _BODY,
|
||||
port=port,
|
||||
follow=_PUBLIC_FOLLOWING_GET,
|
||||
)
|
||||
|
||||
_assert_closed(answered, b"200")
|
||||
assert _BODY not in answered, answered.decode("latin-1", errors="replace")
|
||||
|
||||
|
||||
@pytest.mark.parametrize("framing", _BODYLESS_FRAMING, ids=_BODYLESS_IDS)
|
||||
def test_bodyless_deprecated_ack_keeps_the_pooled_socket_alive(framing):
|
||||
"""The 410 alias, which the body-bearing test at the top of this file pins closed."""
|
||||
answered = _pipelined_after(
|
||||
b"POST /api/process-complete-ack HTTP/1.1\r\nHost: 127.0.0.1\r\n" + framing + b"\r\n",
|
||||
stop_after=2,
|
||||
)
|
||||
|
||||
_assert_kept_alive(answered, b"410")
|
||||
|
||||
|
||||
@pytest.mark.parametrize("path", _MULTIPART_UPLOAD_PATHS)
|
||||
@pytest.mark.parametrize(
|
||||
"framing",
|
||||
[
|
||||
b"Content-Type: application/json\r\n",
|
||||
b"Content-Type: application/json\r\nContent-Length: 0\r\n",
|
||||
b"Content-Type: multipart/form-data\r\n", # multipart, but no boundary=
|
||||
],
|
||||
ids=["json-no-length", "json-zero-length", "multipart-without-boundary"],
|
||||
)
|
||||
def test_bodyless_upload_boundary_rejection_keeps_the_pooled_socket_alive(path, framing):
|
||||
"""`_reject_before_read()`'s one body-less reachable rejection: the boundary check."""
|
||||
answered = _pipelined_after(
|
||||
f"POST {path} HTTP/1.1\r\nHost: 127.0.0.1\r\n".encode() + framing + b"\r\n",
|
||||
stop_after=2,
|
||||
)
|
||||
|
||||
_assert_kept_alive(answered, b"400")
|
||||
|
||||
|
||||
@pytest.mark.parametrize("path", _MULTIPART_UPLOAD_PATHS)
|
||||
def test_upload_boundary_rejection_with_a_body_still_closes(path):
|
||||
answered = _pipelined_after(
|
||||
f"POST {path} HTTP/1.1\r\nHost: 127.0.0.1\r\n"
|
||||
"Content-Type: application/json\r\n"
|
||||
f"Content-Length: {len(_BODY)}\r\n\r\n".encode() + _BODY
|
||||
)
|
||||
|
||||
_assert_closed(answered, b"400")
|
||||
assert _BODY not in answered, answered.decode("latin-1", errors="replace")
|
||||
Executable
+110
@@ -0,0 +1,110 @@
|
||||
"""A rejected write must not leave its unread body on a reused connection.
|
||||
|
||||
The unit tests beside this one assert `close_connection` is set on each rejection
|
||||
path. That pins the mechanism but not the consequence: they build a
|
||||
SimpleNamespace, never open a socket, and would still pass if
|
||||
BaseHTTPRequestHandler stopped honouring the flag.
|
||||
|
||||
This drives the actual failure over one socket: a POST that is rejected before its
|
||||
body is read, then an ordinary GET on the same connection. Without the fix the
|
||||
server parses the leftover body as the next request line and answers
|
||||
|
||||
HTTP/1.1 400 Bad request syntax ('{"a": "b"}GET /api/health HTTP/1.1')
|
||||
|
||||
which is an error about a request the client never sent - the reason this class of
|
||||
bug is expensive to diagnose from the client side. With the fix the server closes
|
||||
after the rejection, so the client sees a clean EOF and reconnects instead.
|
||||
|
||||
The two files are complementary and neither replaces the other. The unit tests
|
||||
call the production handlers, so they pin that `close_connection` is actually set
|
||||
on each rejection path. This file uses a minimal handler of the same shape, so it
|
||||
pins WHY that flag matters at the HTTP level - which no amount of asserting on a
|
||||
boolean can show. Reverting server.py fails the unit test; changing the handler
|
||||
here to reuse the connection fails this one.
|
||||
"""
|
||||
|
||||
import socket
|
||||
import threading
|
||||
import time
|
||||
from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer
|
||||
|
||||
BODY = b'{"a": "b"}'
|
||||
REJECTED_WRITE = (
|
||||
b"POST /api/session/new HTTP/1.1\r\n"
|
||||
b"Host: localhost\r\n"
|
||||
b"Content-Type: application/json\r\n"
|
||||
b"Content-Length: " + str(len(BODY)).encode() + b"\r\n"
|
||||
b"\r\n" + BODY
|
||||
)
|
||||
FOLLOWING_READ = b"GET /api/health HTTP/1.1\r\nHost: localhost\r\n\r\n"
|
||||
|
||||
|
||||
class _Rejecting(BaseHTTPRequestHandler):
|
||||
"""The production shape: reject an unsafe write before reading its body."""
|
||||
|
||||
protocol_version = "HTTP/1.1"
|
||||
closes_on_reject = True
|
||||
|
||||
def log_message(self, *args):
|
||||
pass # keep the test output quiet
|
||||
|
||||
def do_POST(self):
|
||||
# Deliberately does NOT read the body. This is the production path when
|
||||
# auth or CSRF rejects a write before read_body() runs.
|
||||
if self.closes_on_reject:
|
||||
self.close_connection = True
|
||||
self.send_response(403)
|
||||
self.send_header("Content-Length", "0")
|
||||
self.end_headers()
|
||||
|
||||
def do_GET(self):
|
||||
self.send_response(200)
|
||||
self.send_header("Content-Length", "2")
|
||||
self.end_headers()
|
||||
self.wfile.write(b"ok")
|
||||
|
||||
|
||||
def _second_response(*, closes_on_reject: bool) -> bytes:
|
||||
"""Send a rejected write then a GET down one socket; return the GET's answer."""
|
||||
handler = type("_H", (_Rejecting,), {"closes_on_reject": closes_on_reject})
|
||||
server = ThreadingHTTPServer(("127.0.0.1", 0), handler)
|
||||
threading.Thread(target=server.serve_forever, daemon=True).start()
|
||||
try:
|
||||
sock = socket.create_connection(server.server_address, timeout=5)
|
||||
try:
|
||||
sock.sendall(REJECTED_WRITE)
|
||||
time.sleep(0.3)
|
||||
assert b"403" in sock.recv(4096), "the write should have been rejected"
|
||||
|
||||
sock.sendall(FOLLOWING_READ)
|
||||
time.sleep(0.3)
|
||||
try:
|
||||
return sock.recv(4096)
|
||||
except (socket.timeout, ConnectionError):
|
||||
return b""
|
||||
finally:
|
||||
sock.close()
|
||||
finally:
|
||||
server.shutdown()
|
||||
server.server_close()
|
||||
|
||||
|
||||
def test_rejected_write_does_not_corrupt_the_next_request():
|
||||
"""Closing means the client sees EOF, never a bogus error about its own GET."""
|
||||
answer = _second_response(closes_on_reject=True)
|
||||
assert b"400" not in answer, f"the leftover body reached the parser: {answer!r}"
|
||||
assert BODY not in answer, f"the leftover body reached the parser: {answer!r}"
|
||||
# The connection is gone, which is the point: the client reconnects rather
|
||||
# than being told its next request was malformed.
|
||||
assert answer == b"", f"expected a closed connection, got {answer!r}"
|
||||
|
||||
|
||||
def test_without_the_fix_the_unread_body_corrupts_the_next_request():
|
||||
"""The bug itself, so the test above is known to be testing something.
|
||||
|
||||
Reusing the connection makes the server read the unread body as a request
|
||||
line, and the body text appears in the 400 it sends back.
|
||||
"""
|
||||
answer = _second_response(closes_on_reject=False)
|
||||
assert b"400" in answer, f"expected a parse failure, got {answer!r}"
|
||||
assert BODY in answer, f"expected the leftover body in the error, got {answer!r}"
|
||||
Reference in new issue
Block a user