Skip to content

Commit d4ad72d

Browse files
etrclaude
andcommitted
unworked validation sweep: close task-057 minor findings
Address 18 of 25 minor findings on the credential-redaction operator<< work (TASK-057); 7 acknowledged-no-action per reviewer guidance: http_request.cpp cleanups - Hoist iequal_ascii out of is_authorization_header_key into a file-scope helper reusing http::http_header_toupper, so the case-fold matches the rule header_view_map orders keys by. - Unify dump_header_map_redacted and dump_cookie_map_redacted onto a shared dump_map_redacted template parameterised by a ValueFor predicate. - Hoist pass_out into std::string_view to drop the redact-path std::string temporary. - Document the HAVE_BAUTH-off / HAVE_DAUTH-off shape in dump_cookie_map_redacted. Tests (http_request_operator_stream_test.cpp) - Add operator_stream_no_credentials edge-case test. - Pin "query args are never redacted" with .arg("page","2") + assertion. - Pin Footer redaction with .footer("Authorization","Bearer footertoken"). - Split the brittle nested-quote Proxy-Authorization assertion into two observable-property checks. Docs and specs - Add @warning block to operator<< Doxygen repeating the query-arg verbatim-emission caveat. - Add WARNING comment above the DEBUG-only body emit in webserver_body_pipeline.cpp calling out the form-body credential risk. - Reinforce development-only intent on create_test_request setter. - Remove digested_pass from RELEASE_NOTES and v2-deferred-backlog-plan (there was never such a field on the operator<< stream). - Add §5.2.1 Diagnostic redaction to 05-cross-cutting.md and an expose_credentials_in_logs paragraph to the create-webserver component spec, mirroring expose_exception_messages. Tested - http_request_operator_stream: 3 tests / 24 checks pass. - http_request_arena: 8 tests / 21 checks pass. - libhttpserver.la rebuilds clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 8e1d00f commit d4ad72d

10 files changed

Lines changed: 187 additions & 71 deletions

File tree

RELEASE_NOTES.md

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -211,14 +211,15 @@ and see the v2 replacement.
211211
they want.
212212
- **`http_request::operator<<` redacts credentials by default.**
213213
v1 (and earlier v2 builds) emitted `pass:"<plaintext>"`,
214-
`digested_pass:"<plaintext>"`, `Authorization`/`Proxy-Authorization`
215-
header values, and cookie values verbatim into diagnostic output,
216-
leaking every credential into any log aggregation pipeline that
217-
captures operator-stream dumps (CWE-312, CWE-532, OWASP A09:2021).
218-
v2.0 replaces those fields with the fixed token `<redacted>` in the
219-
default stream output. To restore the v1 verbose form for local
220-
development, call `.expose_credentials_in_logs(true)` on the
221-
`create_webserver` builder — this flag is intended for development
214+
`Authorization`/`Proxy-Authorization` header values, and cookie values
215+
verbatim into diagnostic output, leaking every credential into any
216+
log aggregation pipeline that captures operator-stream dumps
217+
(CWE-312, CWE-532, OWASP A09:2021). Digest credentials are protected
218+
through the same Authorization-header path (there was never a separate
219+
`digested_pass` emit field). v2.0 replaces those fields with the fixed
220+
token `<redacted>` in the default stream output. To restore the v1
221+
verbose form for local development, call `.expose_credentials_in_logs(true)`
222+
on the `create_webserver` builder — this flag is intended for development
222223
only and must not be set in production deployments.
223224

224225
## Threading

specs/architecture/04-components/create-webserver.md

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,16 @@ removed after the next release. Rationale: completes the
2222
PRD-RSP-REQ-007 value-typed response cutover (DR-009) onto the auth
2323
path, removing the per-authenticated-request control-block allocation.
2424

25+
**`expose_credentials_in_logs` (TASK-057).** Security-opt-in builder
26+
setter that propagates to every `http_request` the webserver dispatches.
27+
Default `false` suppresses credential surfaces in `http_request::operator<<`
28+
(Basic-auth `pass`, `Authorization`/`Proxy-Authorization` headers and
29+
footers, cookie values) by emitting the fixed token `<redacted>`
30+
(CWE-312 / CWE-532). Setting `true` restores the v1 verbose form for
31+
local development; the flag must not be set in production. Shape
32+
mirrors `expose_exception_messages` (see §5.2.1 for the cross-cutting
33+
contract).
34+
2535
**Related requirements:** PRD-CFG-REQ-001..004, PRD-RSP-REQ-007.
2636

2737
---

specs/architecture/05-cross-cutting.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,10 @@
2424
5. `feature_unavailable` is a normal `std::runtime_error`; no special status mapping. Users who care translate it explicitly.
2525
6. There is no throw-as-status idiom. Users wanting 404/400/etc. construct the response by value: `return http_response::empty().with_status(404);`.
2626

27+
#### 5.2.1 Diagnostic redaction (TASK-057)
28+
29+
`http_request::operator<<` redacts credential material by default (CWE-312 / CWE-532): the Basic-auth `pass` field, `Authorization` and `Proxy-Authorization` header/footer values (case-insensitive), and every cookie value are replaced with the fixed token `<redacted>`. The username (REMOTE_USER) and query-string arguments are emitted verbatim. The v1 verbose-everything behaviour is opt-in for development via `create_webserver::expose_credentials_in_logs(true)` — the same security opt-in shape as `expose_exception_messages` above.
30+
2731
### 5.3 Memory and allocation hot paths
2832

2933
| Object | Allocations per instance | Notes |

specs/tasks/v2-deferred-backlog-plan.md

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -305,8 +305,10 @@ aggregation pipeline.
305305

306306
**Action Items:**
307307
- [x] Replace the literal password emission with a fixed redaction token
308-
(`pass:"<redacted>"`). Same treatment for `digested_pass` and any
309-
other authentication secret on the stream.
308+
(`pass:"<redacted>"`). Digest credentials are protected via
309+
case-insensitive redaction of `Authorization` / `Proxy-Authorization`
310+
header values (and footers) — there is no separate `digested_pass`
311+
emit field in the operator stream.
310312
- [x] Add an opt-in `webserver_builder.expose_credentials_in_logs(true)`
311313
flag for the rare developer who needs the verbose form locally.
312314
- [x] Update Doxygen on the operator to call out the redaction policy.

specs/unworked_review_issues/2026-05-30_235001_task-057.md

Lines changed: 52 additions & 25 deletions
Large diffs are not rendered by default.

src/detail/webserver_body_pipeline.cpp

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -197,6 +197,13 @@ MHD_Result webserver_impl::requests_answer_second_step(MHD_Connection* connectio
197197
}
198198

199199
#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.
200207
std::cout << "Writing content: " << std::string(upload_data, *upload_data_size) << std::endl;
201208
#endif // DEBUG
202209
// The post iterator is only created from the libmicrohttpd for content of type

src/http_request.cpp

Lines changed: 45 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -368,61 +368,69 @@ void http_request::set_expose_credentials_in_logs(bool v) {
368368

369369
namespace {
370370

371+
constexpr std::string_view kRedacted = "<redacted>";
372+
373+
// Case-insensitive equality on ASCII bytes, reusing http_header_toupper
374+
// so the casing rule stays in lock-step with http::header_comparator
375+
// (which is what header_view_map orders keys by).
376+
bool iequal_ascii(std::string_view a, std::string_view b) noexcept {
377+
if (a.size() != b.size()) return false;
378+
return std::equal(a.begin(), a.end(), b.begin(),
379+
[](char x, char y) {
380+
return http::http_header_toupper(x) == http::http_header_toupper(y);
381+
});
382+
}
383+
371384
// TASK-057: Authorization-class header names whose values carry
372385
// credential material (Basic / Digest / Bearer payloads). Matched
373386
// case-insensitively against the keys in the Headers / Footers maps
374387
// so that the redaction policy is robust to header-case variation.
375388
bool is_authorization_header_key(std::string_view key) noexcept {
376389
constexpr std::string_view kAuth = "Authorization";
377390
constexpr std::string_view kProxyAuth = "Proxy-Authorization";
378-
if (key.size() != kAuth.size() && key.size() != kProxyAuth.size()) {
379-
return false;
391+
return iequal_ascii(key, kAuth) || iequal_ascii(key, kProxyAuth);
392+
}
393+
394+
// TASK-057: shared redaction-aware dumper for the Headers, Footers, and
395+
// Cookies maps. ValueFor lets the caller decide per-entry whether to emit
396+
// the original value or the fixed redaction token, so the stream-output
397+
// boilerplate (empty-guard, prefix line, key/quote framing) lives in
398+
// exactly one place and cannot drift between sections. The prefix type
399+
// matches http::dump_header_map so all three helpers share one function-
400+
// pointer signature in operator<<.
401+
template <typename ValueFor>
402+
void dump_map_redacted(std::ostream& os, const std::string& prefix,
403+
const http::header_view_map& map,
404+
ValueFor value_for) {
405+
if (map.empty()) return;
406+
os << " " << prefix << " [";
407+
for (const auto& kv : map) {
408+
os << kv.first << ":\"" << value_for(kv) << "\" ";
380409
}
381-
auto ieq = [](std::string_view a, std::string_view b) noexcept {
382-
if (a.size() != b.size()) return false;
383-
for (std::size_t i = 0; i < a.size(); ++i) {
384-
const unsigned char ca = static_cast<unsigned char>(a[i]);
385-
const unsigned char cb = static_cast<unsigned char>(b[i]);
386-
if (std::tolower(ca) != std::tolower(cb)) return false;
387-
}
388-
return true;
389-
};
390-
return ieq(key, kAuth) || ieq(key, kProxyAuth);
410+
os << "]" << std::endl;
391411
}
392412

393-
constexpr std::string_view kRedacted = "<redacted>";
394-
395413
// TASK-057: emit a Headers/Footers map with Authorization-class header
396414
// values replaced by the fixed redaction token. Mirrors the wire shape
397415
// of http::dump_header_map(header_view_map) for non-authorization
398416
// entries so the on-the-wire diagnostic format is unchanged.
399417
void dump_header_map_redacted(std::ostream& os, const std::string& prefix,
400418
const http::header_view_map& map) {
401-
if (map.empty()) return;
402-
os << " " << prefix << " [";
403-
for (const auto& kv : map) {
404-
os << kv.first << ":\"";
405-
if (is_authorization_header_key(kv.first)) {
406-
os << kRedacted;
407-
} else {
408-
os << kv.second;
409-
}
410-
os << "\" ";
411-
}
412-
os << "]" << std::endl;
419+
dump_map_redacted(os, prefix, map, [](const auto& kv) -> std::string_view {
420+
return is_authorization_header_key(kv.first) ? kRedacted : kv.second;
421+
});
413422
}
414423

415424
// TASK-057: cookie values are credential material by default (session
416425
// IDs, CSRF tokens, JWTs); redaction is unconditional on the keys
417-
// (which remain visible for log triage).
426+
// (which remain visible for log triage). On HAVE_BAUTH-off / HAVE_DAUTH-off
427+
// builds the pass and digested-user surfaces are absent by construction,
428+
// so the redaction policy only meaningfully acts on Authorization headers
429+
// and cookies on those configurations.
418430
void dump_cookie_map_redacted(std::ostream& os, const std::string& prefix,
419431
const http::header_view_map& map) {
420-
if (map.empty()) return;
421-
os << " " << prefix << " [";
422-
for (const auto& kv : map) {
423-
os << kv.first << ":\"" << kRedacted << "\" ";
424-
}
425-
os << "]" << std::endl;
432+
dump_map_redacted(os, prefix, map,
433+
[](const auto&) -> std::string_view { return kRedacted; });
426434
}
427435

428436
} // namespace
@@ -437,9 +445,12 @@ std::ostream& operator<< (std::ostream& os, const http_request& r) {
437445
? static_cast<header_dumper>(&http::dump_header_map)
438446
: &dump_cookie_map_redacted;
439447

448+
const std::string_view pass_out = expose
449+
? std::string_view(r.get_pass())
450+
: kRedacted;
440451
os << r.get_method() << " Request ["
441452
<< "user:\"" << r.get_user() << "\" "
442-
<< "pass:\"" << (expose ? r.get_pass() : std::string(kRedacted)) << "\""
453+
<< "pass:\"" << pass_out << "\""
443454
<< "] path:\"" << r.get_path() << "\"" << std::endl;
444455

445456
dump_headers(os, "Headers", r.get_headers());

src/httpserver/create_test_request.hpp

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,11 @@ class create_test_request {
123123
// TASK-057: opt out of the default credential-redaction policy in
124124
// http_request::operator<<. Mirrors the webserver-side builder
125125
// setter @ref create_webserver::expose_credentials_in_logs for the
126-
// unit-test scope (no webserver construction).
126+
// unit-test scope (no webserver construction). The no-argument form
127+
// (`expose_credentials_in_logs()`) is shorthand for `enable=true`
128+
// and must not appear outside test scope — it reinforces the
129+
// development-only intent without forcing callers to type the
130+
// boolean argument.
127131
create_test_request& expose_credentials_in_logs(bool enable = true) {
128132
_expose_credentials_in_logs = enable;
129133
return *this;

src/httpserver/http_request.hpp

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -570,6 +570,13 @@ class http_request {
570570
* header, and every cookie/session token in plaintext to
571571
* anyone with read access to the log store. Guard with
572572
* `#ifndef NDEBUG` or an explicit environment check.
573+
*
574+
* @warning Query-string arguments — including any `?token=...`,
575+
* `?api_key=...`, or other credential-bearing parameters —
576+
* are emitted verbatim regardless of the
577+
* `expose_credentials_in_logs` flag. Avoid routing credential
578+
* material through query strings, or strip the offending
579+
* parameters before the request reaches any logging path.
573580
*/
574581
std::ostream &operator<< (std::ostream &os, const http_request &r);
575582

test/unit/http_request_operator_stream_test.cpp

Lines changed: 44 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,8 +52,10 @@ LT_BEGIN_AUTO_TEST(http_request_operator_stream_suite, operator_stream_redacts_c
5252
.pass("hunter2")
5353
.header("Authorization", "Basic YWRtaW46aHVudGVyMg==")
5454
.header("Proxy-Authorization", "Digest username=\"admin\", response=\"deadbeef\"")
55+
.footer("Authorization", "Bearer footertoken")
5556
.cookie("session", "session-token-cafef00d")
5657
.cookie("csrf", "csrf-token-abad1dea")
58+
.arg("page", "2")
5759
.build();
5860

5961
std::ostringstream oss;
@@ -74,11 +76,19 @@ LT_BEGIN_AUTO_TEST(http_request_operator_stream_suite, operator_stream_redacts_c
7476
LT_CHECK(out.find("YWRtaW46aHVudGVyMg==") == std::string::npos);
7577
LT_CHECK(out.find("deadbeef") == std::string::npos);
7678

79+
// Footer Authorization values share the header redaction path.
80+
LT_CHECK(out.find("footertoken") == std::string::npos);
81+
7782
// Every cookie value must be redacted; cookie keys remain visible.
7883
LT_CHECK(out.find("session:\"<redacted>\"") != std::string::npos);
7984
LT_CHECK(out.find("csrf:\"<redacted>\"") != std::string::npos);
8085
LT_CHECK(out.find("session-token-cafef00d") == std::string::npos);
8186
LT_CHECK(out.find("csrf-token-abad1dea") == std::string::npos);
87+
88+
// Query-string arguments are streamed verbatim regardless of the
89+
// expose flag — guards against accidental over-redaction in the
90+
// Args section if the operator<< body is ever refactored.
91+
LT_CHECK(out.find("page") != std::string::npos);
8292
LT_END_AUTO_TEST(operator_stream_redacts_credentials)
8393

8494
// TASK-057 — Acceptance: opt-in flag (development-only) restores the
@@ -102,7 +112,12 @@ LT_BEGIN_AUTO_TEST(http_request_operator_stream_suite, operator_stream_exposes_c
102112
// Verbose v1 form: every credential surface is streamed plaintext.
103113
LT_CHECK(out.find("pass:\"hunter2\"") != std::string::npos);
104114
LT_CHECK(out.find("Authorization:\"Basic YWRtaW46aHVudGVyMg==\"") != std::string::npos);
105-
LT_CHECK(out.find("Proxy-Authorization:\"Digest username=\"admin\", response=\"deadbeef\"\"") != std::string::npos);
115+
// Asserting on the exact nested-quote serialisation would couple
116+
// the test to the quoting strategy. Check the observable security
117+
// property instead: the key is present and the credential payload
118+
// is streamed verbatim.
119+
LT_CHECK(out.find("Proxy-Authorization:") != std::string::npos);
120+
LT_CHECK(out.find("deadbeef") != std::string::npos);
106121
LT_CHECK(out.find("session:\"session-token-cafef00d\"") != std::string::npos);
107122
LT_CHECK(out.find("csrf:\"csrf-token-abad1dea\"") != std::string::npos);
108123

@@ -111,6 +126,34 @@ LT_BEGIN_AUTO_TEST(http_request_operator_stream_suite, operator_stream_exposes_c
111126
LT_CHECK(out.find("<redacted>") == std::string::npos);
112127
LT_END_AUTO_TEST(operator_stream_exposes_credentials_when_opted_in)
113128

129+
// TASK-057 — Edge case: the no-credentials request still emits the
130+
// `pass:"<redacted>"` token (because the field is unconditional), but
131+
// no Cookies section appears (the redacted dumper short-circuits on
132+
// empty maps) and no header value collides with the redaction token.
133+
LT_BEGIN_AUTO_TEST(http_request_operator_stream_suite, operator_stream_no_credentials)
134+
auto req = create_test_request()
135+
.method("GET")
136+
.path("/health")
137+
.build();
138+
139+
std::ostringstream oss;
140+
oss << req;
141+
const std::string out = oss.str();
142+
143+
// The pass field always emits, even when no Basic-auth password was
144+
// set, so callers can rely on a uniform shape.
145+
LT_CHECK(out.find("pass:\"<redacted>\"") != std::string::npos);
146+
147+
// Empty cookie and footer maps must NOT cause a stray `<redacted>`
148+
// entry to appear in any section.
149+
LT_CHECK(out.find("Cookies") == std::string::npos);
150+
LT_CHECK(out.find("Footers") == std::string::npos);
151+
152+
// Placeholder secrets from the populated-request tests must not
153+
// leak in via shared state or test-runner residue.
154+
LT_CHECK(out.find("hunter2") == std::string::npos);
155+
LT_END_AUTO_TEST(operator_stream_no_credentials)
156+
114157
LT_BEGIN_AUTO_TEST_ENV()
115158
AUTORUN_TESTS()
116159
LT_END_AUTO_TEST_ENV()

0 commit comments

Comments
 (0)