fix: escape error JSON responses, check write/OTA return values (#77, #78)#89
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughFixes in WebServerManager: check OTA task creation result, enforce NFC write queue backpressure, and escape/serialize error messages safely into JSON to avoid malformed responses. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/WebServerManager.cpp (1)
414-417:⚠️ Potential issue | 🟠 MajorInconsistent: raw JSON concatenation bypasses the escaping fix.
This line constructs JSON by concatenating
errMsg, which includes the raw Spoolmanresponsebody. This is exactly the vulnerability issue#77describes—if the upstream response contains quotes or newlines, the JSON becomes malformed. ThesendErrorhelper should be used here instead.🛠️ Proposed fix
- String errMsg = "Failed to create spool (HTTP " + String(code) + "): " + response; xSemaphoreGive(g_httpMutex); - _server.send(500, "application/json", "{\"error\":\"" + errMsg + "\"}"); + char errMsg[128]; + snprintf(errMsg, sizeof(errMsg), "Failed to create spool (HTTP %d)", code); + sendError(500, errMsg); return;Note: The raw
responsebody is intentionally omitted from the error message since it can be arbitrarily long and contain uncontrolled characters. If the response content is needed for debugging, log it server-side instead of sending it to the client.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WebServerManager.cpp` around lines 414 - 417, The JSON is built by concatenating errMsg (which contains the raw Spoolman response) causing malformed/unsafe JSON; replace the manual _server.send(500, "application/json", "{\"error\":\"" + errMsg + "\"}") call with the existing sendError helper (e.g., call sendError(500, "Failed to create spool (HTTP " + String(code) + ")") or similar) and release the mutex as before with xSemaphoreGive(g_httpMutex); log the full raw response server-side (using your existing logger) rather than including it in the JSON sent to the client so arbitrary characters/length from response are not injected into the JSON body.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/WebServerManager.cpp`:
- Around line 1526-1539: The current manual escaping in
WebServerManager::sendError (variables msg, escaped, body) only handles quotes
and backslashes and fails for newlines/control characters; replace the manual
escape+snprintf logic with ArduinoJson: create a StaticJsonDocument (e.g., 256),
set doc["success"]=false and doc["error"]=msg, serializeJson to a String, call
_server.sendHeader("Access-Control-Allow-Origin","*") and then _server.send with
"application/json" and the serialized body; remove the escaped/body buffers and
loop entirely so ArduinoJson handles all necessary escaping.
---
Outside diff comments:
In `@src/WebServerManager.cpp`:
- Around line 414-417: The JSON is built by concatenating errMsg (which contains
the raw Spoolman response) causing malformed/unsafe JSON; replace the manual
_server.send(500, "application/json", "{\"error\":\"" + errMsg + "\"}") call
with the existing sendError helper (e.g., call sendError(500, "Failed to create
spool (HTTP " + String(code) + ")") or similar) and release the mutex as before
with xSemaphoreGive(g_httpMutex); log the full raw response server-side (using
your existing logger) rather than including it in the JSON sent to the client so
arbitrary characters/length from response are not injected into the JSON body.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3c5c80bd-718b-4be5-9d37-d1717f4b10db
📒 Files selected for processing (1)
src/WebServerManager.cpp
| // Escape quotes and backslashes in msg to produce valid JSON | ||
| char escaped[128]; | ||
| size_t j = 0; | ||
| for (size_t i = 0; msg[i] && j < sizeof(escaped) - 2; i++) { | ||
| if (msg[i] == '"' || msg[i] == '\\') { | ||
| escaped[j++] = '\\'; | ||
| } | ||
| if (j < sizeof(escaped) - 1) { | ||
| escaped[j++] = msg[i]; | ||
| } | ||
| } | ||
| escaped[j] = '\0'; | ||
| char body[192]; | ||
| snprintf(body, sizeof(body), "{\"success\":false,\"error\":\"%s\"}", escaped); |
There was a problem hiding this comment.
Incomplete escaping: newlines and control characters are not handled.
Issue #77 explicitly mentions escaping "quotes, backslashes, newlines" but this implementation only handles quotes and backslashes. If an upstream error contains \n, \r, \t, or other control characters, the JSON will still be malformed.
🛠️ Proposed fix to escape control characters
void WebServerManager::sendError(int code, const char* msg) {
_server.sendHeader("Access-Control-Allow-Origin", "*");
// Escape quotes and backslashes in msg to produce valid JSON
- char escaped[128];
+ char escaped[256];
size_t j = 0;
- for (size_t i = 0; msg[i] && j < sizeof(escaped) - 2; i++) {
- if (msg[i] == '"' || msg[i] == '\\') {
- escaped[j++] = '\\';
- }
- if (j < sizeof(escaped) - 1) {
- escaped[j++] = msg[i];
+ for (size_t i = 0; msg[i] && j < sizeof(escaped) - 6; i++) {
+ char c = msg[i];
+ if (c == '"' || c == '\\') {
+ escaped[j++] = '\\';
+ escaped[j++] = c;
+ } else if (c == '\n') {
+ escaped[j++] = '\\';
+ escaped[j++] = 'n';
+ } else if (c == '\r') {
+ escaped[j++] = '\\';
+ escaped[j++] = 'r';
+ } else if (c == '\t') {
+ escaped[j++] = '\\';
+ escaped[j++] = 't';
+ } else if ((unsigned char)c < 0x20) {
+ // Other control chars: encode as \uXXXX
+ j += snprintf(escaped + j, sizeof(escaped) - j, "\\u%04X", (unsigned char)c);
+ } else {
+ escaped[j++] = c;
}
}
escaped[j] = '\0';
- char body[192];
+ char body[320];
snprintf(body, sizeof(body), "{\"success\":false,\"error\":\"%s\"}", escaped);
_server.send(code, "application/json", body);
}Alternatively, consider using ArduinoJson (already included) to build the error response safely:
void WebServerManager::sendError(int code, const char* msg) {
_server.sendHeader("Access-Control-Allow-Origin", "*");
StaticJsonDocument<256> doc;
doc["success"] = false;
doc["error"] = msg;
String body;
serializeJson(doc, body);
_server.send(code, "application/json", body);
}This leverages ArduinoJson's built-in escaping and is consistent with other response handlers in this file.
🧰 Tools
🪛 Clang (14.0.6)
[warning] 1527-1527: do not declare C-style arrays, use std::array<> instead
(modernize-avoid-c-arrays)
[warning] 1528-1528: variable 'j' is not initialized
(cppcoreguidelines-init-variables)
[warning] 1528-1528: variable name 'j' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 1538-1538: do not declare C-style arrays, use std::array<> instead
(modernize-avoid-c-arrays)
[warning] 1539-1539: escaped string literal can be written as a raw string literal
(modernize-raw-string-literal)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/WebServerManager.cpp` around lines 1526 - 1539, The current manual
escaping in WebServerManager::sendError (variables msg, escaped, body) only
handles quotes and backslashes and fails for newlines/control characters;
replace the manual escape+snprintf logic with ArduinoJson: create a
StaticJsonDocument (e.g., 256), set doc["success"]=false and doc["error"]=msg,
serializeJson to a String, call
_server.sendHeader("Access-Control-Allow-Origin","*") and then _server.send with
"application/json" and the serialized body; remove the escaped/body buffers and
loop entirely so ArduinoJson handles all necessary escaping.
1 participant
Summary
sendError()now escapes quotes and backslashes in error messages before embedding in JSON, preventing malformed responses when upstream text contains special charactershandleApiFormatTag()—enqueueWrite()return checked, returns 503 if queue fullhandleApiWriteTigerTag()— samehandleApiUpdateFromUrl()—xTaskCreatePinnedToCore()return checked, returns 500 and resets OTA state if task creation failsTest Plan
pio run -e esp32devcompiles cleanpio run -e esp32s3zerocompiles cleanCloses #77, closes #78
Summary by CodeRabbit