Skip to content

http_server: accept empty HTTP header values - #12175

Closed
kimonus wants to merge 3 commits into
fluent:lib-monkey-1.8.9from
kimonus:fix-http-empty-header
Closed

http_server: accept empty HTTP header values#12175
kimonus wants to merge 3 commits into
fluent:lib-monkey-1.8.9from
kimonus:fix-http-empty-header

Conversation

@kimonus

@kimonus kimonus commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Fixes #12174.

Important

This PR depends on #12212, which bundles Monkey 1.8.9. It is temporarily
based on lib-monkey-1.8.9 so its diff contains only Fluent Bit-owned
changes. After #12212 merges, this PR will be rebased and retargeted to
master before merge.

Summary

The Monkey HTTP/1 parser fix was merged upstream in monkey/monkey#444 and is
being imported into Fluent Bit by #12212. This PR therefore no longer changes
any files under lib/monkey.

The remaining Fluent Bit-specific work:

  • propagates Monkey HTTP/1 parser errors to the server provider so invalid
    requests are closed instead of being reset and left pending;
  • adds portable runtime coverage for accepted empty generic headers and a
    rejected empty semantic header;
  • adds real-server in_http integration coverage for the original POST/body
    report and bounded Content-Length parsing regressions.

Testing

GCC 13.3, Ubuntu 24.04 Docker:

cmake -S . -B build-docker -DFLB_TESTS_RUNTIME=On -DFLB_TESTS_INTERNAL=On
cmake --build build-docker -j8
ctest --test-dir build-docker -R 'flb-rt-in_http|flb-it-http_server|in_http_tls_expect' --output-on-failure

Result: build passed; focused CTest passed 3/3.

FLUENT_BIT_BINARY=/workspace/build-docker/bin/fluent-bit \
tests/integration/.venv/bin/python -m pytest \
tests/integration/scenarios/in_http/tests/test_in_http_001.py -q

Result: 32 passed.

FLUENT_BIT_BINARY=/workspace/build-docker/bin/fluent-bit \
VALGRIND=1 VALGRIND_STRICT=1 \
tests/integration/.venv/bin/python -m pytest \
tests/integration/scenarios/in_http/tests/test_in_http_001.py -q

Result: 32 passed with strict Valgrind enabled.

Clang 18.1.3 additional verification:

  • flb-rt-in_http and flb-it-http_server: 2/2 passed;
  • all six new real-server integration cases: 6/6 passed.

The CI-style commit-prefix checker passes against lib-monkey-1.8.9.

Compatibility

No configuration, packaging, or bundled-library changes are included in this
PR. The runtime socket regression uses Fluent Bit's portable socket types so it
can compile on supported Windows runtime-test targets. Native Windows and macOS
execution remain delegated to CI.

@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The HTTP parser now accepts empty and whitespace-only header values, finalizes them normally, and continues header processing. The runtime HTTP test sends requests containing both forms and asserts that the server returns HTTP 201.

Changes

Empty header values

Layer / File(s) Summary
Accept empty header values
lib/monkey/mk_server/mk_http_parser.c
MK_ST_HEADER_VALUE finalizes zero-length values on \r and trims both spaces and tabs before non-empty values.
Exercise empty header requests
tests/runtime/in_http.c
The runtime test sends empty and whitespace-only headers and asserts a successful HTTP 201 response.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant HTTPClient
  participant HTTPServer
  participant mk_http_parser
  HTTPClient->>HTTPServer: Send request with empty header values
  HTTPServer->>mk_http_parser: Parse header value
  mk_http_parser-->>HTTPServer: Finalize empty value and continue
  HTTPServer-->>HTTPClient: Return HTTP 201 response
Loading

Suggested reviewers: cosmo0920

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The parser change and regression test address #12174 by accepting empty or whitespace-only generic headers and confirming a 201 response for both forms.
Out of Scope Changes check ✅ Passed The changes are limited to the parser and its regression test, with no unrelated code paths or features introduced.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main change: accepting empty HTTP header values.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@lib/monkey/mk_server/mk_http_parser.c`:
- Around line 999-1008: Update the MK_HEADER_HOST handling in header_lookup to
reject zero-length Host values, including values empty after optional
whitespace, while preserving acceptance of non-empty Host headers and generic
empty-header behavior.

In `@tests/runtime/in_http.c`:
- Line 454: Update the test around send_empty_header_request() so the request
uses a distinguishable payload, then assert that the corresponding output or
callback is observed directly. Ensure the later num > 0 check cannot be
satisfied by the preceding ordinary POST alone.
- Around line 379-395: Update the request/response logic around send(),
setsockopt(), and recv() to handle partial TCP writes and reads: loop until all
request bytes are sent, validate setsockopt() success, and accumulate response
data across recv() calls until the header terminator "\r\n" is found or the
buffer is full. Preserve null termination and reject send, socket-option,
receive, and incomplete/invalid response failures.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 2d4fe72f-846a-4a04-9793-54b1aeb8d981

📥 Commits

Reviewing files that changed from the base of the PR and between 117a727 and 56fc47e.

📒 Files selected for processing (2)
  • lib/monkey/mk_server/mk_http_parser.c
  • tests/runtime/in_http.c

Comment thread lib/monkey/mk_server/mk_http_parser.c
Comment thread tests/runtime/in_http.c Outdated
Comment thread tests/runtime/in_http.c

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 56fc47e6b3

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +999 to +1003
if (buffer[p->i] == '\r') {
p->header_val = p->i;
mark_end();

ret = header_lookup(p, buffer);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid semantic scans from empty header values

When an empty known header reaches header_lookup, header->val.len becomes 0; for Transfer-Encoding: that length is passed to mk_string_search_n, whose len <= 0 path falls back to scanning the NUL-terminated remainder of the request. In plugins/in_http/http_conn.c the buffer is NUL-terminated before parsing, so a request like Transfer-Encoding:\r\nContent-Length: ...\r\n\r\n{"msg":"chunked"} is misclassified as chunked and mk_http_parser_ok returns pending instead of processing the Content-Length body. Please either reject empty semantic headers such as Transfer-Encoding or bypass their value parsing when the value length is zero.

Useful? React with 👍 / 👎.

@kimonus kimonus Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correction: commit 2d920dd now bypasses zero-length Transfer-Encoding semantic scanning, preventing mk_string_search_n from reading beyond the field. Empty Host and Content-Length remain rejected.

@kimonus
kimonus force-pushed the fix-http-empty-header branch 3 times, most recently from 2a9298a to 2d920dd Compare July 30, 2026 10:49
@dmytro-tkachenko2

Copy link
Copy Markdown

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@kimonus
kimonus force-pushed the fix-http-empty-header branch from 2d920dd to 1feac14 Compare July 30, 2026 11:11
@edsiper

edsiper commented Jul 30, 2026

Copy link
Copy Markdown
Member

Please submit the fix into http://github.com/monkey/monkey , once we create a new release with the fix on it we update the budle.

@kimonus

kimonus commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Thanks, I created monkey/monkey#444 with the parser fix and upstream integration coverage.

Since this PR also contains Fluent Bit-specific HTTP/1 parser-error handling and an in_http runtime regression test, would you prefer that I keep this PR open until the new Monkey release is bundled, or submit those Fluent Bit changes separately afterward?

kimonus added 3 commits August 4, 2026 11:40
Monkey's parser fix was merged upstream in monkey/monkey#444 and is
being bundled by fluent#12212. This PR was therefore
rebased onto fluent#12212's lib-monkey-1.8.9 branch, removing its direct
modifications to the vendored Monkey sources.

This commit keeps only Fluent Bit's parser-error propagation. The PR
temporarily depends on fluent#12212 and must be rebased onto master again
after fluent#12212 merges and before this PR is merged.

Signed-off-by: kimonus <kimonus@users.noreply.github.com>
Send real HTTP/1 requests containing empty and whitespace-only generic
headers and require the accepted request to reach the input callback.
Also verify that an invalid empty Upgrade field is not ingested.

Use portable socket types, handle partial writes and reads, and reject
timeouts or incomplete HTTP status lines so the regression runs on the
supported runtime-test platforms.

This test is prepared against Monkey 1.8.9 from fluent#12212. The PR remains
stacked on that bundle update until fluent#12212 merges, then it will be
rebased onto master before merge.

Signed-off-by: kimonus <kimonus@users.noreply.github.com>
Add real-server POST coverage matching fluent#12174 for empty and
whitespace-only generic fields, and verify that the request body is
forwarded successfully.

Cover empty Connection and Transfer-Encoding values plus empty and
whitespace-only Content-Length fields followed immediately by numeric
header or body data. Rejected requests must close or return 400, never
hang or forward a payload.

These tests were prepared and validated against Monkey 1.8.9 from
fluent#12212. The PR will be rebased from that temporary branch onto master
after fluent#12212 merges.

Signed-off-by: kimonus <kimonus@users.noreply.github.com>
@edsiper edsiper closed this Aug 4, 2026
@edsiper

edsiper commented Aug 4, 2026

Copy link
Copy Markdown
Member

it seems that merging #12212 autoclosed this one, cannot be reopen :/

@kimonus

kimonus commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

Thanks, Eduardo — no worries. Since #12212 is now merged and this PR cannot
be reopened, I recreated the Fluent Bit-only changes on current master and
opened #12223.

The replacement PR supersedes #12175, keeps the bundled Monkey sources
untouched, and includes the runtime and real-server regression coverage for
#12174. Thank you for merging the Monkey 1.8.9 bundle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

in_http: HTTP/1.1 empty header value can leave keep-alive request without response

3 participants