Skip to content

fix: strip the trailing newline from the yajl error message - #3668

Open
fzipi wants to merge 1 commit into
owasp-modsecurity:v3/masterfrom
fzipi:v3/issue-2278-yajl-error-newline
Open

fzipi wants to merge 1 commit into
owasp-modsecurity:v3/masterfrom
fzipi:v3/issue-2278-yajl-error-newline

Conversation

@fzipi

@fzipi fzipi commented Oct 13, 2026 •

Copy link
Copy Markdown
Collaborator

What

Both yajl_get_error call sites in src/request_body_processor/json.cc now run the message through utils::string::chomp before storing it, so REQBODY_ERROR_MSG no longer ends with the newline that yajl appends.

Why

The newline ended up inside the error log line and the audit log entry. The native writers escape it as \x0a, so the line read

[data "JSON parsing error: parse error: invalid object key (must be a string)\x0a"] [severity "0"]

and connectors or log pipelines that print the message raw got a broken line.

Tests

New test/test-cases/regression/request-body-parser-json-error-message.json (registered in test/test-suite.in): a malformed JSON body, REQBODY_ERROR rule with logdata:'%{REQBODY_ERROR_MSG}', and an error log assertion that the data field closes right after the yajl text. Fails without the fix (the \x0a is in the way), passes with it.

make check passes except the 9 pre-existing removeComments.json failures from the local submodule mismatch.

References

Fixes #2278

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • JSON parsing error messages no longer include a trailing newline, making logged errors cleaner.
  • Tests
    • Added a regression test for malformed JSON request bodies and their error message.

Both yajl_get_error call sites in the JSON request body processor
assigned yajl's message verbatim, and yajl terminates it with a
newline. The newline ended up inside REQBODY_ERROR_MSG and therefore in
the middle of the error log line and the audit log entry, breaking the
single line format of both.

Trailing whitespace is now removed before the message is stored.

Fixes owasp-modsecurity#2278

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0181jBv5JdNQviwDzfnkf1Gy
@coderabbitai

coderabbitai Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 601334be-8ff9-4189-9a27-acd50d8ecc15
📥 Commits

Reviewing files that changed from the base of the PR and between 48e561b and cca33d1.

📒 Files selected for processing (3)
  • src/request_body_processor/json.cc
  • test/test-cases/regression/request-body-parser-json-error-message.json
  • test/test-suite.in

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 0 remain after this review.

📝 Walkthrough

The JSON request-body processor removes trailing newlines from YAJL error text in both JSON::processChunk and JSON::complete. A regression test checks the logged error message for malformed JSON.

Changes: JSON parser error text

Layer / File(s) Summary
Trim YAJL error text and validate the log
src/request_body_processor/json.cc, test/test-cases/regression/request-body-parser-json-error-message.json, test/test-suite.in
Both error-handling paths remove the trailing newline from YAJL error text. The regression test checks the malformed-JSON log message and expects HTTP 403. The test suite now includes this fixture.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to cca33

The JSON error-message change has a regression test that detects the unwanted trailing newline. No issue remains that should delay merging.

Pre-merge checks · Passed 4 · Failed 1

Failed Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files. (2 skipped: 2 … Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks (4 passed)
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: removing the trailing newline from YAJL error messages.
Linked Issues check Passed Issue #2278 requires parser error log data to omit the extra trailing newline. In src/request_body_processor/json.cc, both JSON::processChunk and JSON::complete call utils::string::chomp after…
Out of Scope Changes check Passed The changes are limited to JSON parser error-message handling, its regression test, and test-suite registration. The utility include and comments support the fix. These changes directly support issue …
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files. (2 skipped: 2 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

@sonarqubecloud

Copy link
Copy Markdown

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Log output for JSON/XML error msg has an extra newline in it

1 participant