Skip to content

Commit e6b6be2

Browse files
authored
docs(planning): file the two defects deferred from the access-log fix (#163)
Both were found while fixing the Litestar access-log body leak (#162) and kept out of it to leave a security fix unencumbered. - log_stream binds sys.stdout at import, so structlog output ignores a stdout the process rebinds before bootstrap, while the root-logger handler follows it (lightweight lane). - Litestar.from_config() skips the request_max_body_size default that Litestar.__init__ applies, so every body-reading handler returns 500 unless the caller sets the field themselves (full lane). Reported upstream as litestar-org/litestar#4296.
1 parent 54c8ad9 commit e6b6be2

2 files changed

Lines changed: 207 additions & 0 deletions

File tree

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
---
2+
summary: Resolve `_MemoryLoggerFactoryConfig.log_stream` at bootstrap instead of at import, so structlog output follows a stdout the process rebinds after importing `lite_bootstrap` (the root-logger handler already does).
3+
---
4+
5+
# Change: Bind the memory logger's stream at bootstrap, not at import
6+
7+
**Lane:** lightweight — ≲30 LOC net, ≤2 files, no new file, no public-API
8+
change, a single straightforward test.
9+
10+
## Goal
11+
12+
`_MemoryLoggerFactoryConfig.log_stream` (`lite_bootstrap/instruments/logging_factory.py`)
13+
defaults to a bare `sys.stdout`, evaluated once when the module is imported.
14+
Every `MemoryLoggerFactory` handler therefore writes to whatever `sys.stdout`
15+
was at import time, even when the process rebinds `sys.stdout` before
16+
bootstrapping. Resolve it at bootstrap instead.
17+
18+
## Approach
19+
20+
```python
21+
log_stream: typing.Any = dataclasses.field(default_factory=lambda: sys.stdout)
22+
```
23+
24+
The config is constructed in `LoggingInstrument.memory_logger_factory`, so a
25+
`default_factory` moves the lookup to bootstrap time — the same moment
26+
`_configure_foreign_loggers` already binds its root-logger
27+
`logging.StreamHandler(sys.stdout)`. Today the two disagree: the root handler
28+
follows a rebound stdout and the structlog path does not.
29+
30+
Observed with a plain `FreeBootstrapper` (all bootstrappers share
31+
`LoggingInstrument`, so this is not Litestar-specific):
32+
33+
```python
34+
with contextlib.redirect_stdout(buffer):
35+
FreeBootstrapper(bootstrap_config=FreeConfig(service_name="svc", logging_buffer_capacity=0)).bootstrap()
36+
structlog.get_logger("demo").info("hello after redirect")
37+
# buffer is empty; the line went to the real stdout instead
38+
```
39+
40+
Two consequences worth naming. In production, anything that wraps or replaces
41+
`sys.stdout` after import — `contextlib.redirect_stdout`, a supervisor that
42+
re-points the stream, a test harness — is silently bypassed by structlog output
43+
while stdlib output follows along. In this repo's own test suite it is why
44+
neither `capsys` nor `capfd` can observe structlog lines (pytest installs its
45+
capture before collection imports the module), which forced
46+
`tests/test_litestar_bootstrap.py` to record through a handler attached to the
47+
`litestar` logger. That workaround stays either way; it is independent of the
48+
capture mechanism, which is the point of it.
49+
50+
Behavior is unchanged for the ordinary case, where nothing rebinds `sys.stdout`
51+
between import and bootstrap.
52+
53+
## Files
54+
55+
- `lite_bootstrap/instruments/logging_factory.py` — `log_stream` gains a `default_factory`.
56+
- `tests/instruments/test_logging_instrument.py` — test added.
57+
58+
## Verification
59+
60+
- [ ] Failing test first: bootstrap a `FreeBootstrapper` inside
61+
`contextlib.redirect_stdout(io.StringIO())`, log one line, assert it lands
62+
in the buffer. Command: `just test -k "log_stream"`. Expected failure: the
63+
buffer is empty because the line went to the import-time stdout.
64+
- [ ] Apply the change.
65+
- [ ] Test passes — `just test -k "log_stream"`.
66+
- [ ] `just test` — full suite green, coverage still 100%.
67+
- [ ] `just lint` — clean.
68+
69+
## Notes
70+
71+
Found while fixing the Litestar access-log body leak
72+
(`planning/changes/2026-08-10.01-litestar-middleware-logging.md`), which is
73+
where the test-capture consequence is documented. Deliberately left out of that
74+
change to keep a security fix unencumbered.
Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,133 @@
1+
---
2+
summary: Fill `request_max_body_size` with Litestar's own 10 MB default when the bootstrapped `AppConfig` leaves it `Empty`, so body-reading handlers stop returning 500 under `Litestar.from_config()`, and pin the constant against Litestar's signature so an upstream change fails CI.
3+
---
4+
5+
# Design: Apply Litestar's request_max_body_size default when the AppConfig leaves it unset
6+
7+
## Summary
8+
9+
`LitestarBootstrapper` builds its application with `Litestar.from_config()`,
10+
which — unlike `Litestar(...)` — does not apply the 10 MB
11+
`request_max_body_size` default. An `AppConfig` that leaves the field at
12+
`Empty` therefore yields an application where every handler that reads a
13+
request body returns 500. Fill the field in the bootstrapper when, and only
14+
when, it is `Empty`.
15+
16+
## Motivation
17+
18+
Reproduced on litestar 2.24.0:
19+
20+
```python
21+
app = litestar.Litestar(route_handlers=[echo]) # request_max_body_size == 10_000_000
22+
app = litestar.Litestar.from_config(AppConfig(...)) # request_max_body_size is Empty
23+
```
24+
25+
With the second form, a `POST` to a handler taking `data: dict` returns 500:
26+
27+
```
28+
ImproperlyConfiguredException: 500: 'request_max_body_size' set to 'Empty' on all layers.
29+
To omit a limit, set 'request_max_body_size=None'
30+
```
31+
32+
`LitestarConfig.application_config` defaults to a bare `AppConfig()`, and a
33+
caller who supplies their own `AppConfig` hits the same default, so **the
34+
failure is the norm rather than the edge case**: any lite-bootstrap Litestar
35+
service whose handlers accept a body 500s unless the caller happens to know to
36+
set `request_max_body_size` themselves. It is not per-route recoverable either
37+
— the exception is raised while resolving the layered value, so the only fixes
38+
are on the handler, a router, or the app.
39+
40+
Found while fixing the access-log body leak
41+
(`planning/changes/2026-08-10.01-litestar-middleware-logging.md`), whose tests
42+
work around it with `request_max_body_size=1000` on their handlers. That
43+
workaround is what should disappear.
44+
45+
## Design
46+
47+
`LitestarBootstrapper._apply_config` already owns exactly this job — it is the
48+
one place that mutates the caller's `AppConfig` before
49+
`Litestar.from_config()` runs, setting `debug` and appending the teardown hook.
50+
Add the fill there:
51+
52+
```python
53+
# litestar_bootstrapper.py, module level
54+
# Litestar.from_config() skips the default that Litestar.__init__ applies, leaving the
55+
# field Empty and 500-ing every body-reading handler. Pinned by a guard test.
56+
_LITESTAR_DEFAULT_REQUEST_MAX_BODY_SIZE: typing.Final = 10_000_000
57+
```
58+
59+
```python
60+
def _apply_config(self, application_config: "AppConfig") -> None:
61+
application_config.debug = self.bootstrap_config.service_debug
62+
if application_config.request_max_body_size is Empty:
63+
application_config.request_max_body_size = _LITESTAR_DEFAULT_REQUEST_MAX_BODY_SIZE
64+
application_config.on_shutdown.append(self.teardown)
65+
```
66+
67+
`Empty` is an enum member (`litestar.types.Empty`, `_EmptyEnum.EMPTY`), not a
68+
class, so the check is an identity comparison — `isinstance` would raise.
69+
`Empty` joins the existing `if import_checker.is_litestar_installed:` import
70+
block.
71+
72+
The guard is the `is Empty` test: a caller's own value, including an explicit
73+
`None` (Litestar's "no limit"), is left alone. Only the unset case is filled.
74+
75+
**Pinning the constant.** Litestar exposes no public constant for the default;
76+
the value lives only in `Litestar.__init__`'s signature, so hardcoding it can
77+
drift silently on a Litestar bump. A guard test reads the signature default and
78+
asserts it equals our constant, turning a drift into a CI failure rather than a
79+
behavior change nobody notices. Runtime introspection was rejected: it makes
80+
every bootstrap depend on a parameter name Litestar does not publish as API,
81+
and it fails opaquely if that name changes.
82+
83+
**Upstream.** `Litestar.from_config()` diverging from `Litestar(...)` on a
84+
constructor default is already reported as
85+
[litestar-org/litestar#4296](https://github.com/litestar-org/litestar/issues/4296),
86+
which lists five such mismatches; our reproduction and the reason this one is a
87+
hard failure rather than a cosmetic difference are in
88+
[a comment there](https://github.com/litestar-org/litestar/issues/4296#issuecomment-5243196116).
89+
`from_config` passes every `AppConfig` field explicitly
90+
(`cls(**dict(extract_dataclass_items(config)))`), so an `__init__` default can
91+
never apply — and on 2.24.0 `request_max_body_size` is the only field where
92+
`AppConfig()` is `Empty` while `__init__` has a real default. If upstream fixes
93+
it, the `is Empty` branch simply stops firing and the guard test keeps the
94+
constant honest until the fill can be dropped.
95+
96+
## Non-goals
97+
98+
- Exposing `request_max_body_size` as a `LitestarConfig` field. Callers who
99+
want a non-default limit set it on their own `AppConfig`, which is where
100+
every other Litestar app-level knob already lives.
101+
- Auditing the other `AppConfig` fields where `from_config()` may diverge from
102+
`Litestar.__init__`. If more turn up, they get their own change.
103+
104+
## Testing
105+
106+
`just test -k "request_max_body_size"`, in `tests/test_litestar_bootstrap.py`:
107+
108+
- a bootstrapped app with a body-reading handler and no explicit
109+
`request_max_body_size`: `POST` succeeds (today: 500).
110+
- a caller-supplied value survives: `AppConfig(request_max_body_size=42)`
111+
bootstraps to `42`.
112+
- an explicit `None` (Litestar's no-limit form) survives as `None`.
113+
- guard: `inspect.signature(litestar.Litestar.__init__).parameters["request_max_body_size"].default`
114+
equals `_LITESTAR_DEFAULT_REQUEST_MAX_BODY_SIZE`.
115+
116+
Once green, drop the `request_max_body_size=1000` workaround from the
117+
access-logging tests added in `2026-08-10.01`, so the suite stops carrying a
118+
note about a defect that no longer exists.
119+
120+
Then `just lint-ci` and the full `just test`.
121+
122+
## Risk
123+
124+
**The constant drifts from Litestar's default.** Low likelihood, low impact
125+
(the number is a size limit), and the guard test converts it into a failed CI
126+
run on the bump that changes it.
127+
128+
**A caller relying on the 500.** Implausible — it is an
129+
`ImproperlyConfiguredException`, not a documented limit.
130+
131+
**Promotion:** `architecture/bootstrappers.md` records that `_apply_config` now
132+
also fills Litestar's unset body-size default, alongside `debug` and the
133+
teardown hook.

0 commit comments

Comments
 (0)