fix(http): decide reject-before-read closes from the framing, not the method
Two framing gaps the re-gate reproduced on the production handlers, both the
same class: something other than the wire was used to decide whether unread
body bytes are queued.
1. api/routes.py — the sidecar provenance 403 gated its close on
`read_request_body`, a static per-method flag. That is wrong in both
directions: a GET carrying `Content-Length`/`Transfer-Encoding` was answered
with keep-alive and its body then parsed as the next request line
(`501 Unsupported method ('{}GET')`), while a body-less DELETE/PUT/PATCH had
a healthy pooled connection closed for nothing.
2. api/upload.py — all four multipart handlers read framing from
`Content-Length` alone, so a chunked upload (which has none) was treated as
length zero, matched no parts and was rejected 400 "No file field in
request" with keep-alive intact. Wire trace before the fix:
HTTP/1.1 400 Bad Request <- the upload rejection
HTTP/1.1 400 Bad request syntax ('54') <- our own chunk-size line
New primitive in api/helpers.py: `request_declares_body()` (Content-Length
present and non-zero, or any Transfer-Encoding other than identity; an
unparseable length counts as present, since it cannot prove absence) plus
`arm_connection_close_if_body_pending()` and `unsupported_transfer_encoding()`.
api/upload.py now routes all four handlers through one preflight,
`_read_multipart_or_reject()`, which refuses an undecodable transfer coding
with 411 before touching rfile — http.server never decodes chunked framing, so
that request can only be rejected — and keeps every previous status and message
(413 oversize, 400 invalid Content-Length/boundary) with its close.
Regressions: chunked reject that closes and a body-less reject that does not,
at all four upload handlers and at the sidecar 403, plus two pipelined
real-socket tests against the running server proving the follow-up request is
never answered off a poisoned socket.
This commit is contained in:
1 parent
304815228a
commit
b3a031a19d
5 files changed
+460
-68
No files matched your search
@@ -220,6 +220,94 @@ def arm_connection_close(handler) -> None:
|
|||||||
pass
|
pass
|
||||||
|
|
||||||
|
|
||||||
|
_UNFRAMED_TRANSFER_CODINGS = frozenset({'', 'identity'})
|
||||||
|
|
||||||
|
|
||||||
|
def _framing_header(handler, name: str) -> str | None:
|
||||||
|
"""Read one framing header, tolerating handler stubs and plain dicts.
|
||||||
|
|
||||||
|
A real handler carries an ``email.message.Message``, whose ``get()`` is
|
||||||
|
case-insensitive 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 None
|
||||||
|
try:
|
||||||
|
value = headers.get(name)
|
||||||
|
if value is None and isinstance(headers, dict):
|
||||||
|
lowered = name.lower()
|
||||||
|
for key, candidate in headers.items():
|
||||||
|
if str(key).lower() == lowered:
|
||||||
|
value = candidate
|
||||||
|
break
|
||||||
|
except Exception:
|
||||||
|
return None
|
||||||
|
return None if value is None else str(value)
|
||||||
|
|
||||||
|
|
||||||
|
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.
|
||||||
|
|
||||||
|
An unparseable ``Content-Length`` counts as a declared body: a value we
|
||||||
|
cannot interpret does not prove the body's absence, and the safe assumption
|
||||||
|
for connection reuse is that bytes are pending.
|
||||||
|
"""
|
||||||
|
coding = _framing_header(handler, 'Transfer-Encoding')
|
||||||
|
if coding is not None and coding.strip().lower() not in _UNFRAMED_TRANSFER_CODINGS:
|
||||||
|
return True
|
||||||
|
raw_length = _framing_header(handler, 'Content-Length')
|
||||||
|
if raw_length is None or not raw_length.strip():
|
||||||
|
return False
|
||||||
|
try:
|
||||||
|
return int(raw_length.strip()) != 0
|
||||||
|
except ValueError:
|
||||||
|
return True
|
||||||
|
|
||||||
|
|
||||||
|
def unsupported_transfer_encoding(handler) -> str | None:
|
||||||
|
"""Return the transfer coding this server cannot decode, or ``None``.
|
||||||
|
|
||||||
|
``BaseHTTPRequestHandler`` never decodes ``Transfer-Encoding``: 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. Such a request can only be rejected -- and only
|
||||||
|
with the connection armed for close, because its chunk bytes are still
|
||||||
|
queued on the socket.
|
||||||
|
"""
|
||||||
|
coding = _framing_header(handler, 'Transfer-Encoding')
|
||||||
|
if coding is None:
|
||||||
|
return None
|
||||||
|
normalized = coding.strip()
|
||||||
|
if normalized.lower() in _UNFRAMED_TRANSFER_CODINGS:
|
||||||
|
return None
|
||||||
|
return normalized
|
||||||
|
|
||||||
|
|
||||||
|
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:
|
def advertise_connection_close(handler) -> None:
|
||||||
"""Serialize ``close_connection`` into exactly one ``Connection: close`` header.
|
"""Serialize ``close_connection`` into exactly one ``Connection: close`` header.
|
||||||
|
|
||||||
|
|||||||
+9
-6
@@ -2975,6 +2975,7 @@ from api.helpers import (
|
|||||||
bad,
|
bad,
|
||||||
safe_resolve,
|
safe_resolve,
|
||||||
arm_connection_close,
|
arm_connection_close,
|
||||||
|
arm_connection_close_if_body_pending,
|
||||||
j,
|
j,
|
||||||
t,
|
t,
|
||||||
read_body,
|
read_body,
|
||||||
@@ -6067,12 +6068,14 @@ def _handle_extension_sidecar_proxy(
|
|||||||
# DELETE fell through the CSRF compatibility path that intentionally admits
|
# DELETE fell through the CSRF compatibility path that intentionally admits
|
||||||
# non-browser clients, giving unsafe methods weaker provenance than GET.
|
# non-browser clients, giving unsafe methods weaker provenance than GET.
|
||||||
if not _check_same_origin_browser_request(handler, require_provenance=True):
|
if not _check_same_origin_browser_request(handler, require_provenance=True):
|
||||||
# Provenance rejection runs before read_body(); when a body is pending,
|
# Provenance rejection runs before read_body(), so close-and-advertise
|
||||||
# close-and-advertise rather than reusing an HTTP/1.1 connection whose
|
# whenever the request DECLARED a body (Content-Length non-zero, or any
|
||||||
# unread body would corrupt the next request (same class as _check_csrf).
|
# Transfer-Encoding): those bytes are still queued in rfile and a reused
|
||||||
# GET (read_request_body=False) has nothing unread — keep-alive survives.
|
# HTTP/1.1 connection would parse them as the next request line (same
|
||||||
if read_request_body:
|
# class as _check_csrf). Gating on read_request_body instead was wrong in
|
||||||
arm_connection_close(handler)
|
# 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)
|
return j(handler, {"error": _csrf_rejection_error(handler)}, status=403)
|
||||||
try:
|
try:
|
||||||
request_body = _read_body_bytes(handler) if read_request_body else None
|
request_body = _read_body_bytes(handler) if read_request_body else None
|
||||||
|
|||||||
+69
-62
@@ -8,7 +8,7 @@ import tempfile
|
|||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
|
|
||||||
from api.config import MAX_UPLOAD_BYTES, STATE_DIR
|
from api.config import MAX_UPLOAD_BYTES, STATE_DIR
|
||||||
from api.helpers import arm_connection_close, j
|
from api.helpers import arm_connection_close, j, unsupported_transfer_encoding
|
||||||
from api.models import get_session
|
from api.models import get_session
|
||||||
from api.profiles import _profiles_match, get_active_profile_name as _get_active_profile_name
|
from api.profiles import _profiles_match, get_active_profile_name as _get_active_profile_name
|
||||||
from api.workspace import (
|
from api.workspace import (
|
||||||
@@ -51,6 +51,62 @@ def _reject_before_read(handler, message, status):
|
|||||||
return j(handler, {'error': message}, status=status)
|
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``:
|
||||||
|
|
||||||
|
* ``Transfer-Encoding: chunked`` carries no 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 chunk byte on the socket
|
||||||
|
to poison the next pooled request. ``http.server`` cannot decode chunked
|
||||||
|
framing at all (``rfile`` is the raw socket), so 411 is the only honest
|
||||||
|
answer.
|
||||||
|
* 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.
|
||||||
|
"""
|
||||||
|
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 require a Content-Length)',
|
||||||
|
411,
|
||||||
|
)
|
||||||
|
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:
|
def parse_multipart(rfile, content_type, content_length) -> tuple:
|
||||||
import re as _re, email.parser as _ep
|
import re as _re, email.parser as _ep
|
||||||
# Imported locally (not just module-level) so the function stays
|
# Imported locally (not just module-level) so the function stays
|
||||||
@@ -212,22 +268,10 @@ def _write_office_upload_sidecar(workspace: Path, dest: Path, file_bytes: bytes)
|
|||||||
def handle_upload(handler):
|
def handle_upload(handler):
|
||||||
import traceback as _tb
|
import traceback as _tb
|
||||||
try:
|
try:
|
||||||
content_type = handler.headers.get('Content-Type', '')
|
|
||||||
try:
|
try:
|
||||||
content_length = int(handler.headers.get('Content-Length', 0) or 0)
|
fields, files = _read_multipart_or_reject(handler)
|
||||||
except (TypeError, ValueError):
|
except _RejectBeforeRead as rejection:
|
||||||
# A non-numeric Content-Length is rejected before the body is read.
|
return _reject_before_read(handler, rejection.message, rejection.status)
|
||||||
arm_connection_close(handler)
|
|
||||||
raise ValueError('Invalid Content-Length') from None
|
|
||||||
if content_length > MAX_UPLOAD_BYTES:
|
|
||||||
return _reject_before_read(handler, f'File too large (max {MAX_UPLOAD_BYTES//1024//1024}MB)', 413)
|
|
||||||
try:
|
|
||||||
fields, files = parse_multipart(handler.rfile, content_type, content_length)
|
|
||||||
except ValueError:
|
|
||||||
# parse_multipart validates Content-Length/boundary before reading;
|
|
||||||
# a ValueError here means the body was never consumed.
|
|
||||||
arm_connection_close(handler)
|
|
||||||
raise
|
|
||||||
session_id = fields.get('session_id', '')
|
session_id = fields.get('session_id', '')
|
||||||
if 'file' not in files:
|
if 'file' not in files:
|
||||||
return j(handler, {'error': 'No file field in request'}, status=400)
|
return j(handler, {'error': 'No file field in request'}, status=400)
|
||||||
@@ -413,22 +457,10 @@ def handle_upload_extract(handler):
|
|||||||
"""Handle archive upload and extraction."""
|
"""Handle archive upload and extraction."""
|
||||||
import traceback as _tb
|
import traceback as _tb
|
||||||
try:
|
try:
|
||||||
content_type = handler.headers.get('Content-Type', '')
|
|
||||||
try:
|
try:
|
||||||
content_length = int(handler.headers.get('Content-Length', 0) or 0)
|
fields, files = _read_multipart_or_reject(handler)
|
||||||
except (TypeError, ValueError):
|
except _RejectBeforeRead as rejection:
|
||||||
# A non-numeric Content-Length is rejected before the body is read.
|
return _reject_before_read(handler, rejection.message, rejection.status)
|
||||||
arm_connection_close(handler)
|
|
||||||
raise ValueError('Invalid Content-Length') from None
|
|
||||||
if content_length > MAX_UPLOAD_BYTES:
|
|
||||||
return _reject_before_read(handler, f'File too large (max {MAX_UPLOAD_BYTES//1024//1024}MB)', 413)
|
|
||||||
try:
|
|
||||||
fields, files = parse_multipart(handler.rfile, content_type, content_length)
|
|
||||||
except ValueError:
|
|
||||||
# parse_multipart validates Content-Length/boundary before reading;
|
|
||||||
# a ValueError here means the body was never consumed.
|
|
||||||
arm_connection_close(handler)
|
|
||||||
raise
|
|
||||||
session_id = fields.get('session_id', '')
|
session_id = fields.get('session_id', '')
|
||||||
if 'file' not in files:
|
if 'file' not in files:
|
||||||
return j(handler, {'error': 'No file field in request'}, status=400)
|
return j(handler, {'error': 'No file field in request'}, status=400)
|
||||||
@@ -456,22 +488,10 @@ def handle_transcribe(handler):
|
|||||||
import traceback as _tb
|
import traceback as _tb
|
||||||
temp_path = None
|
temp_path = None
|
||||||
try:
|
try:
|
||||||
content_type = handler.headers.get('Content-Type', '')
|
|
||||||
try:
|
try:
|
||||||
content_length = int(handler.headers.get('Content-Length', 0) or 0)
|
fields, files = _read_multipart_or_reject(handler)
|
||||||
except (TypeError, ValueError):
|
except _RejectBeforeRead as rejection:
|
||||||
# A non-numeric Content-Length is rejected before the body is read.
|
return _reject_before_read(handler, rejection.message, rejection.status)
|
||||||
arm_connection_close(handler)
|
|
||||||
raise ValueError('Invalid Content-Length') from None
|
|
||||||
if content_length > MAX_UPLOAD_BYTES:
|
|
||||||
return _reject_before_read(handler, f'File too large (max {MAX_UPLOAD_BYTES//1024//1024}MB)', 413)
|
|
||||||
try:
|
|
||||||
fields, files = parse_multipart(handler.rfile, content_type, content_length)
|
|
||||||
except ValueError:
|
|
||||||
# parse_multipart validates Content-Length/boundary before reading;
|
|
||||||
# a ValueError here means the body was never consumed.
|
|
||||||
arm_connection_close(handler)
|
|
||||||
raise
|
|
||||||
if 'file' not in files:
|
if 'file' not in files:
|
||||||
return j(handler, {'error': 'No file field in request'}, status=400)
|
return j(handler, {'error': 'No file field in request'}, status=400)
|
||||||
filename, file_bytes = files['file']
|
filename, file_bytes = files['file']
|
||||||
@@ -651,23 +671,10 @@ def handle_workspace_upload(handler):
|
|||||||
"""
|
"""
|
||||||
import traceback as _tb
|
import traceback as _tb
|
||||||
try:
|
try:
|
||||||
content_type = handler.headers.get('Content-Type', '')
|
|
||||||
try:
|
try:
|
||||||
content_length = int(handler.headers.get('Content-Length', 0) or 0)
|
fields, files = _read_multipart_or_reject(handler)
|
||||||
except (TypeError, ValueError):
|
except _RejectBeforeRead as rejection:
|
||||||
# A non-numeric Content-Length is rejected before the body is read.
|
return _reject_before_read(handler, rejection.message, rejection.status)
|
||||||
arm_connection_close(handler)
|
|
||||||
raise ValueError('Invalid Content-Length') from None
|
|
||||||
if content_length > MAX_UPLOAD_BYTES:
|
|
||||||
return _reject_before_read(handler, f'File too large (max {MAX_UPLOAD_BYTES//1024//1024}MB)', 413)
|
|
||||||
|
|
||||||
try:
|
|
||||||
fields, files = parse_multipart(handler.rfile, content_type, content_length)
|
|
||||||
except ValueError:
|
|
||||||
# parse_multipart validates Content-Length/boundary before reading;
|
|
||||||
# a ValueError here means the body was never consumed.
|
|
||||||
arm_connection_close(handler)
|
|
||||||
raise
|
|
||||||
session_id = fields.get('session_id', '')
|
session_id = fields.get('session_id', '')
|
||||||
subpath = fields.get('path', '')
|
subpath = fields.get('path', '')
|
||||||
|
|
||||||
|
|||||||
@@ -8,6 +8,8 @@ The connection must close after those early rejections.
|
|||||||
from types import SimpleNamespace
|
from types import SimpleNamespace
|
||||||
from typing import Any, cast
|
from typing import Any, cast
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
|
||||||
|
|
||||||
def test_auth_rejected_write_closes_connection(monkeypatch):
|
def test_auth_rejected_write_closes_connection(monkeypatch):
|
||||||
import api.auth as auth
|
import api.auth as auth
|
||||||
@@ -63,6 +65,7 @@ def test_sidecar_provenance_rejected_closes_connection(monkeypatch):
|
|||||||
handler = SimpleNamespace(
|
handler = SimpleNamespace(
|
||||||
path="/api/extensions/ext1/sidecar/proxy",
|
path="/api/extensions/ext1/sidecar/proxy",
|
||||||
command="POST",
|
command="POST",
|
||||||
|
headers={"Content-Length": "15"},
|
||||||
close_connection=False,
|
close_connection=False,
|
||||||
)
|
)
|
||||||
monkeypatch.setattr(routes, "_match_extension_sidecar_proxy_path", lambda _path: ("ext1", "/proxy"))
|
monkeypatch.setattr(routes, "_match_extension_sidecar_proxy_path", lambda _path: ("ext1", "/proxy"))
|
||||||
@@ -87,6 +90,7 @@ def test_sidecar_get_provenance_rejected_keeps_connection(monkeypatch):
|
|||||||
handler = SimpleNamespace(
|
handler = SimpleNamespace(
|
||||||
path="/api/extensions/ext1/sidecar/proxy",
|
path="/api/extensions/ext1/sidecar/proxy",
|
||||||
command="GET",
|
command="GET",
|
||||||
|
headers={},
|
||||||
close_connection=False,
|
close_connection=False,
|
||||||
)
|
)
|
||||||
monkeypatch.setattr(routes, "_match_extension_sidecar_proxy_path", lambda _path: ("ext1", "/proxy"))
|
monkeypatch.setattr(routes, "_match_extension_sidecar_proxy_path", lambda _path: ("ext1", "/proxy"))
|
||||||
@@ -98,6 +102,116 @@ def test_sidecar_get_provenance_rejected_keeps_connection(monkeypatch):
|
|||||||
assert handler.close_connection is False
|
assert handler.close_connection is False
|
||||||
|
|
||||||
|
|
||||||
|
# ── Framing, not the method, decides whether a rejection must close ──────────
|
||||||
|
#
|
||||||
|
# The re-gate found both halves of the same mistake: `read_request_body` is a
|
||||||
|
# static per-method flag, so it neither proves a body is pending (a body-less
|
||||||
|
# DELETE closed a healthy connection) nor proves one is absent (a GET carrying a
|
||||||
|
# declared body kept the connection and poisoned it). Both directions below run
|
||||||
|
# against the production handler.
|
||||||
|
|
||||||
|
|
||||||
|
def _rejected_sidecar_call(monkeypatch, method, headers, **kwargs):
|
||||||
|
"""Drive a provenance-rejected sidecar proxy call; return the handler."""
|
||||||
|
import api.routes as routes
|
||||||
|
|
||||||
|
handler = SimpleNamespace(
|
||||||
|
path="/api/extensions/ext1/sidecar/proxy",
|
||||||
|
command=method,
|
||||||
|
headers=headers,
|
||||||
|
close_connection=False,
|
||||||
|
)
|
||||||
|
monkeypatch.setattr(routes, "_match_extension_sidecar_proxy_path", lambda _path: ("ext1", "/proxy"))
|
||||||
|
monkeypatch.setattr(routes, "_check_same_origin_browser_request", lambda _handler, **_: False)
|
||||||
|
monkeypatch.setattr(routes, "j", lambda _handler, _payload, status=200: None)
|
||||||
|
|
||||||
|
routes._handle_extension_sidecar_proxy(
|
||||||
|
cast(Any, handler), SimpleNamespace(path=handler.path, query=""), method, **kwargs
|
||||||
|
)
|
||||||
|
return handler
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize(
|
||||||
|
"headers",
|
||||||
|
[
|
||||||
|
{"Content-Length": "12"},
|
||||||
|
{"Transfer-Encoding": "chunked"},
|
||||||
|
{"Content-Length": "banana"}, # unreadable framing proves nothing is absent
|
||||||
|
],
|
||||||
|
ids=["content-length", "chunked", "garbage-content-length"],
|
||||||
|
)
|
||||||
|
def test_sidecar_get_with_a_declared_body_closes_connection(headers, monkeypatch):
|
||||||
|
"""A GET may still declare a body — those bytes are unread, so close.
|
||||||
|
|
||||||
|
Reproduced by the gate as a 403 WITHOUT `Connection: close`, followed by
|
||||||
|
`501 Unsupported method ('{}GET')` on the same socket.
|
||||||
|
"""
|
||||||
|
handler = _rejected_sidecar_call(monkeypatch, "GET", headers)
|
||||||
|
|
||||||
|
assert handler.close_connection is True
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize("method", ["POST", "PUT", "PATCH", "DELETE"])
|
||||||
|
@pytest.mark.parametrize(
|
||||||
|
"headers", [{}, {"Content-Length": "0"}], ids=["no-content-length", "zero-content-length"]
|
||||||
|
)
|
||||||
|
def test_bodyless_write_method_rejection_keeps_connection(method, headers, monkeypatch):
|
||||||
|
"""A write method with no declared body has nothing unread — keep-alive.
|
||||||
|
|
||||||
|
`read_request_body=True` is passed for every one of these (that is what the
|
||||||
|
real unsafe-method call site does), so this is exactly the case the old
|
||||||
|
per-method gate got wrong in the over-close direction.
|
||||||
|
"""
|
||||||
|
handler = _rejected_sidecar_call(monkeypatch, method, headers, read_request_body=True)
|
||||||
|
|
||||||
|
assert handler.close_connection is False
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize(
|
||||||
|
("headers", "expected"),
|
||||||
|
[
|
||||||
|
({}, False),
|
||||||
|
({"Content-Length": "0"}, False),
|
||||||
|
({"Content-Length": " 0 "}, False),
|
||||||
|
({"Content-Length": ""}, False),
|
||||||
|
({"Content-Length": "12"}, True),
|
||||||
|
({"Content-Length": "-1"}, True),
|
||||||
|
({"Content-Length": "banana"}, True),
|
||||||
|
({"content-length": "7"}, True),
|
||||||
|
({"Transfer-Encoding": "chunked"}, True),
|
||||||
|
({"transfer-encoding": "Chunked"}, True),
|
||||||
|
({"Transfer-Encoding": "identity"}, False),
|
||||||
|
({"Transfer-Encoding": ""}, False),
|
||||||
|
],
|
||||||
|
)
|
||||||
|
def test_request_declares_body_reads_the_framing_headers(headers, expected):
|
||||||
|
from api.helpers import request_declares_body
|
||||||
|
|
||||||
|
handler = SimpleNamespace(headers=headers)
|
||||||
|
|
||||||
|
assert request_declares_body(cast(Any, handler)) is expected
|
||||||
|
|
||||||
|
|
||||||
|
def test_request_declares_body_is_case_insensitive_on_a_real_message():
|
||||||
|
"""Production handlers carry an email.message.Message, not a dict."""
|
||||||
|
from email.parser import Parser
|
||||||
|
|
||||||
|
from api.helpers import request_declares_body
|
||||||
|
|
||||||
|
message = Parser().parsestr("Host: x\r\ntransfer-encoding: Chunked\r\n\r\n")
|
||||||
|
|
||||||
|
assert request_declares_body(cast(Any, SimpleNamespace(headers=message))) is True
|
||||||
|
|
||||||
|
|
||||||
|
def test_request_declares_body_tolerates_a_handler_without_headers():
|
||||||
|
"""Never raise on a rejection path: an exception there becomes a 500."""
|
||||||
|
from api.helpers import request_declares_body
|
||||||
|
|
||||||
|
assert request_declares_body(cast(Any, SimpleNamespace())) is False
|
||||||
|
assert request_declares_body(cast(Any, SimpleNamespace(headers=None))) is False
|
||||||
|
assert request_declares_body(cast(Any, SimpleNamespace(headers=object()))) is False
|
||||||
|
|
||||||
|
|
||||||
def test_csp_report_rate_limited_closes_connection(monkeypatch):
|
def test_csp_report_rate_limited_closes_connection(monkeypatch):
|
||||||
"""A rate-limited CSP report is dropped before its body is read."""
|
"""A rate-limited CSP report is dropped before its body is read."""
|
||||||
import api.routes as routes
|
import api.routes as routes
|
||||||
@@ -137,6 +251,90 @@ def test_upload_oversize_rejects_before_read(monkeypatch):
|
|||||||
assert handler.close_connection is True
|
assert handler.close_connection is True
|
||||||
|
|
||||||
|
|
||||||
|
# ── Multipart framing preflight: chunked uploads must close, empty ones must not ─
|
||||||
|
#
|
||||||
|
# All four multipart handlers keyed on Content-Length alone, so a chunked upload
|
||||||
|
# (which has none) was read as length 0, matched no parts, and was rejected with
|
||||||
|
# "No file field in request" and keep-alive intact — leaving every chunk byte on
|
||||||
|
# the socket, which is the exact poisoning this PR exists to prevent.
|
||||||
|
# http.server never decodes chunked framing, so the only safe answer is 411 with
|
||||||
|
# the connection armed for close.
|
||||||
|
|
||||||
|
_MULTIPART_HANDLERS = [
|
||||||
|
"handle_upload",
|
||||||
|
"handle_upload_extract",
|
||||||
|
"handle_transcribe",
|
||||||
|
"handle_workspace_upload",
|
||||||
|
]
|
||||||
|
|
||||||
|
|
||||||
|
class _NeverRead:
|
||||||
|
"""An rfile that fails the test if a reject-before-read path reads the body."""
|
||||||
|
|
||||||
|
def read(self, *_args):
|
||||||
|
raise AssertionError("the body must not be read on a reject-before-read path")
|
||||||
|
|
||||||
|
|
||||||
|
def _run_multipart_handler(monkeypatch, handler_name, headers, rfile):
|
||||||
|
import api.upload as upload
|
||||||
|
|
||||||
|
answered: dict[str, Any] = {}
|
||||||
|
|
||||||
|
def _record(_handler, payload, status=200):
|
||||||
|
answered["payload"] = payload
|
||||||
|
answered["status"] = status
|
||||||
|
return True
|
||||||
|
|
||||||
|
monkeypatch.setattr(upload, "j", _record)
|
||||||
|
handler = SimpleNamespace(headers=headers, rfile=rfile, close_connection=False)
|
||||||
|
|
||||||
|
getattr(upload, handler_name)(cast(Any, handler))
|
||||||
|
return handler, answered
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize("handler_name", _MULTIPART_HANDLERS)
|
||||||
|
def test_chunked_multipart_rejected_before_read_closes_connection(handler_name, monkeypatch):
|
||||||
|
handler, answered = _run_multipart_handler(
|
||||||
|
monkeypatch,
|
||||||
|
handler_name,
|
||||||
|
{"Content-Type": "multipart/form-data; boundary=x", "Transfer-Encoding": "chunked"},
|
||||||
|
_NeverRead(),
|
||||||
|
)
|
||||||
|
|
||||||
|
assert answered["status"] == 411, answered
|
||||||
|
assert "Transfer-Encoding" in answered["payload"]["error"], answered
|
||||||
|
assert handler.close_connection is True
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize("handler_name", _MULTIPART_HANDLERS)
|
||||||
|
def test_bodyless_multipart_rejection_keeps_connection(handler_name, monkeypatch):
|
||||||
|
"""A well-framed upload with no body is rejected with nothing left unread."""
|
||||||
|
handler, answered = _run_multipart_handler(
|
||||||
|
monkeypatch,
|
||||||
|
handler_name,
|
||||||
|
{"Content-Type": "multipart/form-data; boundary=x", "Content-Length": "0"},
|
||||||
|
SimpleNamespace(read=lambda *_args: b""),
|
||||||
|
)
|
||||||
|
|
||||||
|
assert answered["status"] == 400, answered
|
||||||
|
assert handler.close_connection is False
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize("handler_name", _MULTIPART_HANDLERS)
|
||||||
|
def test_non_numeric_content_length_still_closes(handler_name, monkeypatch):
|
||||||
|
"""The pre-existing invalid-Content-Length reject keeps its 400 and its close."""
|
||||||
|
handler, answered = _run_multipart_handler(
|
||||||
|
monkeypatch,
|
||||||
|
handler_name,
|
||||||
|
{"Content-Type": "multipart/form-data; boundary=x", "Content-Length": "banana"},
|
||||||
|
_NeverRead(),
|
||||||
|
)
|
||||||
|
|
||||||
|
assert answered["status"] == 400, answered
|
||||||
|
assert answered["payload"]["error"] == "Invalid Content-Length", answered
|
||||||
|
assert handler.close_connection is True
|
||||||
|
|
||||||
|
|
||||||
# ── Header serialization: exactly one Connection: close, never a crash ────────
|
# ── Header serialization: exactly one Connection: close, never a crash ────────
|
||||||
#
|
#
|
||||||
# The advertisement lives in Handler.end_headers() via
|
# The advertisement lives in Handler.end_headers() via
|
||||||
|
|||||||
@@ -19,8 +19,11 @@ stale tab gets 410, not 403).
|
|||||||
"""
|
"""
|
||||||
|
|
||||||
import http.client
|
import http.client
|
||||||
|
import socket
|
||||||
import urllib.parse
|
import urllib.parse
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
|
||||||
from tests._pytest_port import BASE
|
from tests._pytest_port import BASE
|
||||||
|
|
||||||
_PORT = urllib.parse.urlparse(BASE).port
|
_PORT = urllib.parse.urlparse(BASE).port
|
||||||
@@ -61,3 +64,96 @@ def test_rejected_write_advertises_close_and_pooled_followup_succeeds():
|
|||||||
assert follow_status == 200, (
|
assert follow_status == 200, (
|
||||||
f"pooled follow-up after a reject-before-read must succeed; got {follow_status}"
|
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) -> bytes:
|
||||||
|
"""Send *request* and a plain GET in one write; return every byte answered."""
|
||||||
|
sock = socket.create_connection(("127.0.0.1", _PORT), timeout=10)
|
||||||
|
try:
|
||||||
|
sock.sendall(request + _FOLLOWING_GET)
|
||||||
|
received = b""
|
||||||
|
while True:
|
||||||
|
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)
|
||||||
Reference in new issue
Block a user