Always send lowlevel_error response to client (#2731)

[MitchLewis930]

User description

PR_052

---

PR Type

Enhancement, Bug fix

---

Description

• Refactor error handling to always send lowlevel_error responses to clients

• Extract response writing logic into separate write_response method

• Ensure lowlevel_error_handler returns proper status, headers, body tuple

• Update client_error to use new response writing flow

• Add test for custom lowlevel error handler responses

---

Diagram Walkthrough

flowchart LR
  A["Request handling"] -->|Exception caught| B["lowlevel_error handler"]
  B -->|Returns status, headers, body| C["write_response method"]
  C -->|Writes to client| D["Client receives error response"]
  E["client_error method"] -->|Calls lowlevel_error| B

Loading

File Walkthrough

	Relevant files
Refactoring	request.rb Refactor request handling and extract response writing      lib/puma/request.rb Simplified variable assignments by removing intermediate body and head variables Restructured exception handling to catch ThreadPool::ForceShutdown and generic exceptions separately Extracted response writing logic into new write_response method with comprehensive documentation Moved response body closing logic to write_response method Fixed indentation in chunked response writing section +47/-29
Error handling	server.rb Enhance error handling with proper response formatting      lib/puma/server.rb Updated client_error method signature to accept buffer and requests parameters Modified client_error to call write_response instead of client.write_error Enhanced lowlevel_error to handle handler return values and ensure proper tuple format Added support for handlers with different arities (1, 2, or 3 parameters) Ensured lowlevel_error always returns [status, headers, body] tuple +11/-9
Tests	test_puma_server.rb Add test for custom lowlevel error handler                              test/test_puma_server.rb Added new test test_lowlevel_error_handler_custom_response to verify custom error handler responses Test validates that custom lowlevel error handler can return custom response body Test ensures response includes proper status, headers, and custom error page content +11/-0    test_response_header.rb Fix HTTP version in test assertions                                            test/test_response_header.rb Updated HTTP version in assertions from HTTP/1.1 to HTTP/1.0 in two test cases Changes align with actual response protocol version being tested +2/-2

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Sensitive error disclosure Description: client_error now always generates and writes a lowlevel_error response body to the client for parser/IO errors (e.g., HttpParserError and other StandardErrors), which can inadvertently expose sensitive internal details (exception messages/backtraces) if @leak_stack_on_error is enabled or a custom :lowlevel_error_handler returns detailed diagnostics. server.rb [521-548] Referred Code status, headers, res_body = lowlevel_error(e, client.env, 400) write_response(status, headers, res_body, buffer, requests, client) @events.parse_error e, client else status, headers, res_body = lowlevel_error(e, client.env) write_response(status, headers, res_body, buffer, requests, client) @events.unknown_error e, nil, "Read" end end # A fallback rack response if +@app+ raises as exception. # def lowlevel_error(e, env, status = 500) if handler = @options[:lowlevel_error_handler] if handler.arity == 1 handler_status, headers, res_body = handler.call(e) elsif handler.arity == 2 handler_status, headers, res_body = handler.call(e, env) else handler_status, headers, res_body = handler.call(e, env, status) end ... (clipped 7 lines)
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Nil env risk: write_response assumes client.env is non-nil (e.g., uses env[REQUEST_METHOD]) and may raise during error paths (e.g., parse/early I/O errors) where client.env might not be initialized, preventing graceful error responses. Referred Code def write_response(status, headers, res_body, lines, requests, client) env = client.env io = client.io return false if closed_socket?(io) lines.clear head = env[REQUEST_METHOD] == HEAD after_reply = env[RACK_AFTER_REPLY] || [] Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Handler output exposure: lowlevel_error now always forwards custom lowlevel_error_handler status/headers/body to the client without validating/sanitizing content, which could expose sensitive internal details depending on handler implementation. Referred Code def lowlevel_error(e, env, status = 500) if handler = @options[:lowlevel_error_handler] if handler.arity == 1 handler_status, headers, res_body = handler.call(e) elsif handler.arity == 2 handler_status, headers, res_body = handler.call(e, env) else handler_status, headers, res_body = handler.call(e, env, status) end return [handler_status || status, headers || {}, res_body || []] end Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Safe unpack lowlevel_error handler return Wrap the handler.call result in Array(...) to ensure it can be safely destructured, preventing potential crashes from non-array return values. lib/puma/server.rb [534-543] if handler = @options[:lowlevel_error_handler] if handler.arity == 1 - handler_status, headers, res_body = handler.call(e) + handler_status, headers, res_body = Array(handler.call(e)) elsif handler.arity == 2 - handler_status, headers, res_body = handler.call(e, env) + handler_status, headers, res_body = Array(handler.call(e, env)) else - handler_status, headers, res_body = handler.call(e, env, status) + handler_status, headers, res_body = Array(handler.call(e, env, status)) end return [handler_status || status, headers || {}, res_body || []] end Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential TypeError if the lowlevel_error_handler returns a non-array value and provides a robust, idiomatic Ruby solution using Array() to prevent the server from crashing.	Medium
More