Skip to content

Commit 9046983

Browse files
etrclaude
andcommitted
TASK-074: gate DEBUG raw-body printing behind explicit env-var opt-in
Replaces the #ifdef DEBUG guard around the raw request-body stdout dump in src/detail/webserver_body_pipeline.cpp with a runtime opt-in keyed on LIBHTTPSERVER_DEBUG_DUMP_REQUEST_BODY. Default behaviour is now silent on RELEASE *and* DEBUG builds alike -- the env var is the only gate, so a debug build accidentally shipped to production cannot leak credentials/PII unless the operator explicitly opted in. When the env var is observed at webserver::start(), the library emits a single SECURITY WARNING line to stderr (and to the user's log_error callback when wired) naming the env var, the credential/PII risk, and the docs pointer. Process-wide std::atomic<bool> flag means multiple webservers in the same process produce exactly one stderr emission. The runtime gate is cached in a function-local static (Magic Static) so getenv() is read at most once per process; subsequent setenv() calls are intentionally ignored (debug-knob semantics). New documentation: - docs/debug-env-vars.md - canonical home for LIBHTTPSERVER_* debug knobs; describes effect, risk, startup signal, disable procedure, and the spot-check that release-with-DDEBUG still behaves correctly. Includes a checklist for future debug knobs. - specs/architecture/10-observability.md cross-references the new doc. New tests (test/integ/): - debug_dump_request_body_unset_test.cpp - asserts silence on both stdout and stderr when the env var is unset; passes identically on DEBUG and RELEASE builds. - debug_dump_request_body_set_test.cpp - asserts the body dump and one-shot SECURITY WARNING fire when the env var is set, and pins the process-wide idempotence with a second webserver. The two binaries are split because the function-local-static cache means the first observation in the process locks in for the rest; splitting gives each scenario a deterministic fresh process. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 1605f1a commit 9046983

9 files changed

Lines changed: 603 additions & 18 deletions

File tree

docs/debug-env-vars.md

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
# Debug-only environment variables
2+
3+
This document inventories environment variables that the libhttpserver
4+
runtime reads. **None of these are meant to be set in production.**
5+
All are silent unless explicitly enabled; the library emits a one-shot
6+
`SECURITY WARNING` to stderr (and to the user's `log_error` callback,
7+
when wired) the first time `webserver::start()` observes a set
8+
variable.
9+
10+
---
11+
12+
## `LIBHTTPSERVER_DEBUG_DUMP_REQUEST_BODY`
13+
14+
| Field | Value |
15+
| --- | --- |
16+
| Introduced | TASK-074 (June 2026) |
17+
| Replaces | The prior compile-time `#ifdef DEBUG` guard in `src/detail/webserver_body_pipeline.cpp` |
18+
| Read by | `httpserver::detail::debug_dump_request_body_opted_in()` (cached in a function-local static) |
19+
| Independent of | Compile-time `DEBUG` / `NDEBUG`. The runtime check is the only gate. |
20+
21+
### Effect
22+
23+
When set to any non-empty, non-`"0"` value, every inbound request body
24+
chunk is written verbatim to `stdout`, prefixed by `Writing content: `,
25+
from the body pipeline in `src/detail/webserver_body_pipeline.cpp`.
26+
27+
### Risk
28+
29+
Raw request bodies routinely contain:
30+
- Basic-auth credentials (when sent in the body rather than the header),
31+
- form-posted passwords (`application/x-www-form-urlencoded` with a
32+
`password=` field),
33+
- session cookies,
34+
- CSRF tokens,
35+
- PII (email addresses, names, IDs, addresses).
36+
37+
The `http_request::operator<<` redaction policy introduced in TASK-057
38+
does **not** cover this code path -- bytes are written verbatim. Treat
39+
any process where this variable is set as a credential-leaking
40+
surface (CWE-532: Insertion of Sensitive Information into Log File).
41+
42+
### Startup signal
43+
44+
The library emits a single `SECURITY WARNING` line to stderr at the
45+
first `webserver::start()` call after the variable is observed. The
46+
exact line is:
47+
48+
```
49+
[libhttpserver] SECURITY WARNING: LIBHTTPSERVER_DEBUG_DUMP_REQUEST_BODY is set. Raw request bodies will be written to stdout, including credentials, session cookies, and PII. Unset the variable for production deployments. See docs/debug-env-vars.md.
50+
```
51+
52+
The same line is forwarded to the owning webserver's `log_error`
53+
callback when one is wired via `create_webserver().log_error(...)`. The
54+
forwarding is best-effort: an exception from the callback is swallowed
55+
so a misconfigured logger cannot abort daemon startup.
56+
57+
Multiple `webserver` instances constructed in the same process share a
58+
process-wide print-once flag, so the warning is emitted exactly once
59+
per process.
60+
61+
### How to disable
62+
63+
```sh
64+
unset LIBHTTPSERVER_DEBUG_DUMP_REQUEST_BODY
65+
```
66+
67+
then restart the process. The value is cached in a function-local
68+
static on first request, so a mid-process `unsetenv()` has no effect.
69+
70+
### Spot-check: release build with `-DDEBUG` accidentally set
71+
72+
The runtime check is the only gate; the env var must be explicitly set
73+
for the dump to fire, regardless of the compile-time `DEBUG` /
74+
`NDEBUG` posture. A release binary built with `-DDEBUG` accidentally
75+
defined therefore still does not leak credentials/PII unless the
76+
operator opted in.
77+
78+
---
79+
80+
## Adding new debug env vars
81+
82+
When introducing a new `LIBHTTPSERVER_*` debug environment variable:
83+
84+
1. Read it once via a function-local static (cached, race-free C++
85+
memory model semantics).
86+
2. Add a one-shot stderr warning in `webserver::start()` naming the
87+
risk explicitly.
88+
3. Document the variable, its risk, and its startup signal here.
89+
4. Cross-reference the entry from `specs/architecture/10-observability.md`.

specs/architecture/10-observability.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,5 +4,6 @@ The library is a passive provider; callers wire their own logging:
44
- `log_access` callback (already in `create_webserver`): invoked per request with the URI.
55
- `log_error` callback: invoked on internal errors and uncaught handler exceptions.
66
- No metrics or tracing surface added in v2.0.
7+
- Debug-only environment variables (e.g. `LIBHTTPSERVER_DEBUG_DUMP_REQUEST_BODY`): see [`docs/debug-env-vars.md`](../../docs/debug-env-vars.md). None are read on the production code path; all are silent unless explicitly set, and the library emits a one-shot `SECURITY WARNING` to stderr (and to `log_error` when wired) when any of them is detected at `webserver::start()`.
78

89
---

specs/tasks/M7-v2-cleanup/TASK-074.md

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,10 @@
88
`webserver_body_pipeline.cpp:199-208` contains a `#ifdef DEBUG` block that prints the raw request body (including credentials and PII) to stdout. The inline comment explicitly warns it must never be widened to release builds. The risk is contained but the suppression mechanism is fragile (a developer could ship a debug build to production). Make the print opt-in via an explicit env var so the default behaviour even in `DEBUG` builds is safe.
99

1010
**Action Items:**
11-
- [ ] Replace the `#ifdef DEBUG` guard with a runtime check on `getenv("LIBHTTPSERVER_DEBUG_DUMP_REQUEST_BODY")`. The block compiles always but only fires when the env var is set and the build is `DEBUG` (or unconditionally, since prod builds never set the var). Document the env var in `src/httpserver/detail/README.md` (or wherever debug knobs live).
12-
- [ ] Add a warning log at startup if the env var is set, naming the security risk.
13-
- [ ] Update the inline comment to reference the env var rather than `#ifdef DEBUG`.
14-
- [ ] Spot-check that release builds with `-DDEBUG` accidentally set still do not print without the env var.
11+
- [x] Replace the `#ifdef DEBUG` guard with a runtime check on `getenv("LIBHTTPSERVER_DEBUG_DUMP_REQUEST_BODY")`. The block compiles always but only fires when the env var is set; release builds never set the var, so the gate is a one-line runtime check independent of any compile-time `DEBUG` define. Documented in the new `docs/debug-env-vars.md` (a fresh, dedicated home for `LIBHTTPSERVER_*` debug knobs) with a cross-reference from `specs/architecture/10-observability.md`.
12+
- [x] Add a warning log at startup if the env var is set, naming the security risk. Implemented as `httpserver::detail::maybe_warn_debug_dump_request_body()` called from `webserver::start()`. One-shot stderr write (process-wide `std::atomic<bool>` flag) plus best-effort forwarding to `webserver::get_error_logger()` when wired. Message names the env var, the SECURITY WARNING marker, credentials/cookies/PII at risk, and points to `docs/debug-env-vars.md`.
13+
- [x] Update the inline comment to reference the env var rather than `#ifdef DEBUG`. The comment in `webserver_body_pipeline.cpp` now names `LIBHTTPSERVER_DEBUG_DUMP_REQUEST_BODY`, points to `docs/debug-env-vars.md`, and notes that the TASK-057 `operator<<` redaction does not cover this code path.
14+
- [x] Spot-check that release builds with `-DDEBUG` accidentally set still do not print without the env var. Pinned by `test/integ/debug_dump_request_body_unset_test.cpp`, which builds and runs identically on both `--enable-debug` and stock release configurations; the runtime check is the only gate, so the spot-check resolves by construction.
1515

1616
**Dependencies:**
1717
- Blocked by: None
@@ -27,4 +27,4 @@
2727
**Related Requirements:** PRD §2 security NFR (defense-in-depth)
2828
**Related Decisions:** None new
2929

30-
**Status:** Backlog
30+
**Status:** In Progress

src/detail/webserver_body_pipeline.cpp

Lines changed: 95 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -36,16 +36,18 @@
3636

3737
#include <strings.h>
3838

39+
#include <atomic>
3940
#include <chrono>
4041
#include <cstddef>
4142
#include <cstdint>
43+
#include <cstdio>
44+
#include <cstdlib>
4245
#include <cstring>
43-
#ifdef DEBUG
4446
#include <iostream>
45-
#endif // DEBUG
4647
#include <optional>
4748
#include <span>
4849
#include <string>
50+
#include <string_view>
4951
#include <utility>
5052

5153
#include "httpserver/create_webserver.hpp"
@@ -65,6 +67,35 @@ namespace detail {
6567

6668
namespace {
6769

70+
// TASK-074: gate the raw-body dump in requests_answer_second_step
71+
// behind an explicit, runtime, opt-in environment variable. The check
72+
// is cached in a function-local static so getenv() is called at most
73+
// once per process; subsequent setenv() calls are intentionally
74+
// ignored (debug-knob semantics, matches the cheap race-free choice
75+
// described in DR-009 / TASK-074 plan). C++11 guarantees thread-safe
76+
// init of function-local statics, so the first call from MHD's
77+
// request-handling threads is safe even under concurrent first-touch.
78+
//
79+
// Default behaviour is silent on RELEASE and DEBUG builds alike; the
80+
// env var is the ONLY gate. A debug build accidentally shipped to
81+
// production therefore still does not leak credentials/PII unless the
82+
// operator explicitly opted in. Accepts any non-empty, non-"0" value.
83+
bool debug_dump_request_body_enabled() {
84+
static const bool enabled = [] {
85+
const char* v = std::getenv("LIBHTTPSERVER_DEBUG_DUMP_REQUEST_BODY");
86+
return v != nullptr && v[0] != '\0' && std::string_view(v) != "0";
87+
}();
88+
return enabled;
89+
}
90+
91+
// TASK-074: process-wide print-once flag for the SECURITY WARNING
92+
// emitted at the first webserver::start() that observes the env var.
93+
// std::atomic<bool> + compare_exchange_strong matches the lock-free
94+
// atomic style used elsewhere (e.g. g_arena_fallback_count). The
95+
// idempotence is process-wide -- multiple webservers in the same
96+
// process trigger exactly one stderr warning.
97+
std::atomic<bool> g_debug_dump_warning_emitted{false};
98+
6899
// Wrap the body_chunk firing site so requests_answer_second_step stays a
69100
// flat sequence of small steps. Returns true iff a hook short-circuited
70101
// and the caller should signal MHD that the chunk was consumed (returns
@@ -115,6 +146,55 @@ void run_post_processor_if_attached(modded_request* mr,
115146

116147
} // namespace
117148

149+
// TASK-074: free function in namespace detail, declared in
150+
// webserver_impl.hpp. Reports whether the runtime opt-in env var is
151+
// effective in this process. Used by webserver::start()'s
152+
// security-warning emitter to decide whether to fire the one-shot
153+
// stderr notice.
154+
bool debug_dump_request_body_opted_in() {
155+
return debug_dump_request_body_enabled();
156+
}
157+
158+
// TASK-074: free function in namespace detail, declared in
159+
// webserver_impl.hpp. Called from webserver::start() before
160+
// MHD_start_daemon. Emits a one-shot SECURITY WARNING to stderr when
161+
// the env-var opt-in is observed, and forwards the same line to the
162+
// owning webserver's log_error callback when one is wired. The flag
163+
// is process-wide so multiple webservers do not flood stderr.
164+
//
165+
// Stderr is used (not std::cerr) to match the pre-existing
166+
// fprintf-to-stderr pattern in src/http_request.cpp / src/webserver.cpp
167+
// and to remain visible even when the user has not wired log_error.
168+
//
169+
// Both the stderr path and the log_error forwarding swallow any
170+
// exception so a misconfigured logger cannot abort the daemon
171+
// start sequence.
172+
void maybe_warn_debug_dump_request_body(const webserver* parent) {
173+
if (!debug_dump_request_body_opted_in()) return;
174+
bool expected = false;
175+
if (!g_debug_dump_warning_emitted.compare_exchange_strong(
176+
expected, true)) {
177+
return; // already warned this process
178+
}
179+
constexpr const char* msg =
180+
"[libhttpserver] SECURITY WARNING: "
181+
"LIBHTTPSERVER_DEBUG_DUMP_REQUEST_BODY is set. "
182+
"Raw request bodies will be written to stdout, including "
183+
"credentials, session cookies, and PII. Unset the variable "
184+
"for production deployments. See docs/debug-env-vars.md.";
185+
std::fprintf(stderr, "%s\n", msg);
186+
std::fflush(stderr);
187+
if (parent != nullptr) {
188+
if (auto sink = parent->get_error_logger()) {
189+
try {
190+
sink(msg);
191+
} catch (...) {
192+
// swallow: misconfigured logger must not abort start()
193+
}
194+
}
195+
}
196+
}
197+
118198
MHD_Result webserver_impl::requests_answer_first_step(MHD_Connection* connection, struct detail::modded_request* mr) {
119199
// The http_request constructor calls pick_resource(connection) internally
120200
// to locate the per-connection arena installed by connection_notify via
@@ -196,16 +276,19 @@ MHD_Result webserver_impl::requests_answer_second_step(MHD_Connection* connectio
196276
}
197277
}
198278

199-
#ifdef DEBUG
200-
// WARNING (TASK-057): this emits the raw request body to stdout.
201-
// If a client posts credentials in a form body
202-
// (`application/x-www-form-urlencoded` with a `password=` field) or
203-
// raw Basic-auth bytes, those values will appear verbatim in the
204-
// debug stream. Must remain guarded by `#ifdef DEBUG` and must not
205-
// be widened to release builds; the operator<< redaction policy
206-
// does NOT cover this code path.
207-
std::cout << "Writing content: " << std::string(upload_data, *upload_data_size) << std::endl;
208-
#endif // DEBUG
279+
// TASK-074 (was TASK-057): raw request-body dump, opt-in via the
280+
// env var LIBHTTPSERVER_DEBUG_DUMP_REQUEST_BODY. Default behaviour
281+
// is silent on RELEASE *and* DEBUG builds -- the env var is the
282+
// only gate, so a debug build accidentally shipped to production
283+
// still does not leak credentials/PII unless the operator opted
284+
// in. See docs/debug-env-vars.md for the security warning and
285+
// the one-shot startup notice that fires when the env var is set.
286+
// The operator<< redaction policy (TASK-057) does NOT cover this
287+
// code path: raw bytes are written verbatim.
288+
if (debug_dump_request_body_enabled()) {
289+
std::cout << "Writing content: "
290+
<< std::string(upload_data, *upload_data_size) << std::endl;
291+
}
209292
// The post iterator is only created from the libmicrohttpd for content of type
210293
// multipart/form-data and application/x-www-form-urlencoded
211294
// all other content (which is indicated by mr-pp == nullptr)

src/detail/webserver_setup.cpp

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -296,6 +296,13 @@ bool webserver::start(bool blocking) {
296296
"Cannot specify maximum number of threads when using a thread per connection");
297297
}
298298

299+
// TASK-074: fire the one-shot raw-body-dump opt-in SECURITY
300+
// WARNING before MHD_start_daemon so the operator sees the
301+
// notice on every fresh process invocation that actually starts
302+
// accepting traffic. Defined in webserver_body_pipeline.cpp
303+
// alongside the consumer of the opt-in flag.
304+
detail::maybe_warn_debug_dump_request_body(this);
305+
299306
vector<struct MHD_OptionItem> iov;
300307
impl_->build_mhd_option_array(iov);
301308
const int start_conf = impl_->compose_start_flags();

src/httpserver/detail/webserver_impl.hpp

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,34 @@ class lambda_resource;
100100
// httpserver/detail/connection_state.hpp for documentation and
101101
// rationale.
102102

103+
/**
104+
* @brief Whether the runtime opt-in for raw-body debug dumping is in
105+
* effect for this process (TASK-074).
106+
*
107+
* Reads the env var LIBHTTPSERVER_DEBUG_DUMP_REQUEST_BODY once on
108+
* first call via a function-local static; subsequent setenv() calls
109+
* are intentionally ignored. Returns true iff the variable is set to
110+
* any non-empty, non-`"0"` value. Default behaviour is silent on both
111+
* RELEASE and DEBUG builds.
112+
*/
113+
bool debug_dump_request_body_opted_in();
114+
115+
/**
116+
* @brief Emit a one-shot SECURITY WARNING when raw-body dumping is
117+
* opted in (TASK-074).
118+
*
119+
* Called from webserver::start() before MHD_start_daemon. If the env
120+
* var opt-in is active, writes a single line naming the env var, the
121+
* SECURITY WARNING marker, and the credential / PII risk to stderr,
122+
* and (when wired) forwards the same line to the owning webserver's
123+
* log_error callback. Process-wide idempotent: multiple webservers in
124+
* the same process produce exactly one stderr emission.
125+
*
126+
* @param parent Owning webserver pointer (used to find log_error).
127+
* May be nullptr; the stderr emission still fires.
128+
*/
129+
void maybe_warn_debug_dump_request_body(const webserver* parent);
130+
103131
// webserver_impl: backing object holding all backend-coupled state of
104132
// `webserver` (MHD daemon, mutexes, ban/allowance sets, route table,
105133
// route cache, websocket registry, optional GnuTLS SNI cache) plus the

0 commit comments

Comments
 (0)