feat: web serial log viewer#140
Conversation
Add LogBuffer ring buffer (4KB) that captures key events via logPrintf and serves them at /logs with a dark-themed auto-scrolling viewer. Instrumented events: tag detection (all formats), tag removal, NTAG variant, tag payload summaries, Spoolman sync/create/archive/errors, MQTT connection status, and all write outcomes. Accessible from landing page card and troubleshooting page link.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 19 minutes and 30 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR introduces a log buffering and web viewer feature that captures serial output in a ring buffer and exposes it via a web interface. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 8
🤖 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/LogBuffer.cpp`:
- Around line 36-40: The current write path uses xSemaphoreTake(mutex_,
pdMS_TO_TICKS(50)) to protect append(temp, len) and silently drops the buffer
write on timeout; update this by increasing the timeout (e.g., change
pdMS_TO_TICKS(50) to pdMS_TO_TICKS(100)) and/or add a fallback warning when
xSemaphoreTake fails so dropped writes are visible (use your existing
logging/Serial facility to report that append(temp, len) could not acquire
mutex_ and the message was not written to the ring buffer). Ensure
xSemaphoreGive(mutex_) remains unchanged and reference the same symbols
(xSemaphoreTake, mutex_, append, temp, len) when applying the change.
- Line 2: Replace the C header with the C++ header in LogBuffer.cpp: change the
include of <stdarg.h> to <cstdarg> so the translation unit uses the C++-style
variadic argument header (ensure any uses like va_list in functions such as
LogBuffer::append or other variadic helpers continue to compile and refer to
std::va_list if necessary).
- Around line 5-8: The constructor LogBuffer::LogBuffer currently assigns mutex_
= xSemaphoreCreateMutex() without checking for NULL; add a null-check
immediately after calling xSemaphoreCreateMutex() and handle failure (e.g., log
an error via your logging facility, assert/abort, or set an internal error flag
so later calls that use mutex_ such as xSemaphoreTake() can skip/avoid using it)
to prevent undefined behavior when memory is exhausted; ensure any code paths
that use mutex_ (e.g., methods calling xSemaphoreTake/xSemaphoreGive) check the
flag or mutex_ != NULL before calling.
- Around line 62-68: Modify LogBuffer::clear to return a bool indicating
success: attempt to take the mutex_ via xSemaphoreTake (pdMS_TO_TICKS(50)) and
if successful set head_ = 0 and count_ = 0, xSemaphoreGive(mutex_) and return
true; if the take times out return false to signal failure. Update the
declaration in LogBuffer.h accordingly and ensure callers (e.g., the API handler
invoking LogBuffer::clear) check the bool and propagate/report failure.
In `@src/LogBuffer.h`:
- Around line 10-33: Add deleted copy/move constructors and assignment operators
to the LogBuffer class to fully enforce the singleton; inside the class
declaration (near the private section with LogBuffer() and append()), add
declarations: LogBuffer(const LogBuffer&) = delete; LogBuffer& operator=(const
LogBuffer&) = delete; LogBuffer(LogBuffer&&) = delete; LogBuffer&
operator=(LogBuffer&&) = delete; so copying or moving the singleton via
copy/move ctor or assignment is a compile-time error.
In `@src/LogViewerHTML.h`:
- Around line 127-158: The polling allows overlapping requests:
setInterval(fetchLogs, 2000) can start a new fetchLogs before the prior one
completes causing stale responses (e.g., after clearLogs()). Fix by serializing
requests in fetchLogs using an "inFlight" flag (or an AbortController) so
fetchLogs returns immediately if a request is already running; update the flag
when the fetch promise settles; ensure clearLogs calls fetchLogs() but relies on
the inFlight guard so it won't race with an ongoing request. Reference:
fetchLogs, setInterval(fetchLogs, 2000), and clearLogs.
In `@src/WebServerManager.cpp`:
- Around line 287-297: The current WebServerManager::handleApiLogs allocates a
4KB buffer and calls _server.send which causes an extra internal copy and spikes
memory; change it to stream the log content instead (like
handleApiSpoolmanSpools does) by using _server.setContentLength(...) and
_server.sendContent/sendChunked with a small fixed-size buffer passed to
LogBuffer::getInstance().getLog to produce chunks, or by supplying a content
provider lambda that reads successive chunks from
LogBuffer::getInstance().getLog until exhausted; remove the large persistent
malloc/free and ensure headers (Access-Control-Allow-Origin) remain set before
starting the streamed response.
In `@src/WebServerManager.h`:
- Around line 76-79: The log endpoints expose sensitive data and a destructive
clear action without auth; add authentication guards in handleApiLogs and
handleApiLogsClear (and apply same guard to handleLogViewer) by validating a
configured API token or Authorization header (reject with 401 on failure),
restrict or remove Access-Control-Allow-Origin: * for those handlers, and ensure
handleApiLogsClear requires elevated creds (separate admin token or stronger
check) and logs auth failures; alternatively, if this behavior is intentional,
add clear inline comments and README documentation near WebServerManager
(referencing handleLogViewer, handleApiLogs, handleApiLogsClear) explaining the
deliberate lack of auth and the network isolation requirements.
🪄 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: 3eec7001-64ff-4193-9e76-7c5aa611009e
📒 Files selected for processing (10)
src/HomeAssistantManager.cppsrc/LandingHTML.hsrc/LogBuffer.cppsrc/LogBuffer.hsrc/LogViewerHTML.hsrc/NFCManager.cppsrc/SpoolmanManager.cppsrc/TroubleshootingHTML.hsrc/WebServerManager.cppsrc/WebServerManager.h
| LogBuffer::LogBuffer() { | ||
| mutex_ = xSemaphoreCreateMutex(); | ||
| memset(buffer_, 0, BUFFER_SIZE); | ||
| } |
There was a problem hiding this comment.
Add null check for mutex creation.
xSemaphoreCreateMutex() can return NULL if the system is out of memory. Subsequent xSemaphoreTake() calls on a null mutex would cause undefined behavior.
🐛 Proposed fix
LogBuffer::LogBuffer() {
mutex_ = xSemaphoreCreateMutex();
+ configASSERT(mutex_ != nullptr); // or handle gracefully
memset(buffer_, 0, BUFFER_SIZE);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| LogBuffer::LogBuffer() { | |
| mutex_ = xSemaphoreCreateMutex(); | |
| memset(buffer_, 0, BUFFER_SIZE); | |
| } | |
| LogBuffer::LogBuffer() { | |
| mutex_ = xSemaphoreCreateMutex(); | |
| configASSERT(mutex_ != nullptr); // or handle gracefully | |
| memset(buffer_, 0, BUFFER_SIZE); | |
| } |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 5-5: constructor does not initialize these fields: buffer_, mutex_
(cppcoreguidelines-pro-type-member-init)
[warning] 5-5: use '= default' to define a trivial default constructor
(modernize-use-equals-default)
🪛 Cppcheck (2.20.0)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[performance] 6-6: Variable 'mutex_' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/LogBuffer.cpp` around lines 5 - 8, The constructor LogBuffer::LogBuffer
currently assigns mutex_ = xSemaphoreCreateMutex() without checking for NULL;
add a null-check immediately after calling xSemaphoreCreateMutex() and handle
failure (e.g., log an error via your logging facility, assert/abort, or set an
internal error flag so later calls that use mutex_ such as xSemaphoreTake() can
skip/avoid using it) to prevent undefined behavior when memory is exhausted;
ensure any code paths that use mutex_ (e.g., methods calling
xSemaphoreTake/xSemaphoreGive) check the flag or mutex_ != NULL before calling.
| // Write to ring buffer under mutex | ||
| if (xSemaphoreTake(mutex_, pdMS_TO_TICKS(50)) == pdTRUE) { | ||
| append(temp, len); | ||
| xSemaphoreGive(mutex_); | ||
| } |
There was a problem hiding this comment.
Log messages are silently dropped from the buffer if mutex acquisition times out.
When the 50ms mutex timeout expires, the message is written to Serial but not to the ring buffer. Under heavy concurrent logging from multiple managers (NFC, Spoolman, HomeAssistant), users viewing /logs may miss events.
Consider either:
- Increasing the timeout (e.g., 100ms)
- Logging a warning when drops occur
- Documenting this behavior for users
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/LogBuffer.cpp` around lines 36 - 40, The current write path uses
xSemaphoreTake(mutex_, pdMS_TO_TICKS(50)) to protect append(temp, len) and
silently drops the buffer write on timeout; update this by increasing the
timeout (e.g., change pdMS_TO_TICKS(50) to pdMS_TO_TICKS(100)) and/or add a
fallback warning when xSemaphoreTake fails so dropped writes are visible (use
your existing logging/Serial facility to report that append(temp, len) could not
acquire mutex_ and the message was not written to the ring buffer). Ensure
xSemaphoreGive(mutex_) remains unchanged and reference the same symbols
(xSemaphoreTake, mutex_, append, temp, len) when applying the change.
| void LogBuffer::clear() { | ||
| if (xSemaphoreTake(mutex_, pdMS_TO_TICKS(50)) == pdTRUE) { | ||
| head_ = 0; | ||
| count_ = 0; | ||
| xSemaphoreGive(mutex_); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider returning success status from clear().
If the mutex acquisition times out, the clear operation silently fails. The web UI's "Clear" button will appear to succeed, but the next poll will show logs still present. Returning a bool would allow the API to report failure.
♻️ Proposed change
In src/LogBuffer.h:
- void clear();
+ bool clear();In src/LogBuffer.cpp:
-void LogBuffer::clear() {
+bool LogBuffer::clear() {
if (xSemaphoreTake(mutex_, pdMS_TO_TICKS(50)) == pdTRUE) {
head_ = 0;
count_ = 0;
xSemaphoreGive(mutex_);
+ return true;
}
+ return false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void LogBuffer::clear() { | |
| if (xSemaphoreTake(mutex_, pdMS_TO_TICKS(50)) == pdTRUE) { | |
| head_ = 0; | |
| count_ = 0; | |
| xSemaphoreGive(mutex_); | |
| } | |
| } | |
| bool LogBuffer::clear() { | |
| if (xSemaphoreTake(mutex_, pdMS_TO_TICKS(50)) == pdTRUE) { | |
| head_ = 0; | |
| count_ = 0; | |
| xSemaphoreGive(mutex_); | |
| return true; | |
| } | |
| return false; | |
| } |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 62-62: method 'clear' can be made static
(readability-convert-member-functions-to-static)
🪛 Cppcheck (2.20.0)
[style] 62-62: The function 'clear' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/LogBuffer.cpp` around lines 62 - 68, Modify LogBuffer::clear to return a
bool indicating success: attempt to take the mutex_ via xSemaphoreTake
(pdMS_TO_TICKS(50)) and if successful set head_ = 0 and count_ = 0,
xSemaphoreGive(mutex_) and return true; if the take times out return false to
signal failure. Update the declaration in LogBuffer.h accordingly and ensure
callers (e.g., the API handler invoking LogBuffer::clear) check the bool and
propagate/report failure.
| class LogBuffer { | ||
| public: | ||
| static LogBuffer& getInstance(); | ||
|
|
||
| // Write formatted output to both Serial and the ring buffer | ||
| void logPrintf(const char* format, ...) __attribute__((format(printf, 2, 3))); | ||
|
|
||
| // Copy current buffer contents into a pre-allocated char array. | ||
| // Returns number of bytes written (excluding null terminator). | ||
| size_t getLog(char* out, size_t outSize); | ||
|
|
||
| void clear(); | ||
|
|
||
| private: | ||
| static constexpr size_t BUFFER_SIZE = 4096; | ||
|
|
||
| char buffer_[BUFFER_SIZE]; | ||
| size_t head_ = 0; | ||
| size_t count_ = 0; | ||
| SemaphoreHandle_t mutex_; | ||
|
|
||
| LogBuffer(); | ||
| void append(const char* data, size_t len); | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Delete copy/move constructors and assignment operators to fully enforce singleton.
The singleton pattern should prevent copying and moving. While the private constructor prevents external instantiation, explicitly deleting these operations provides clearer intent and compile-time enforcement.
♻️ Proposed fix
class LogBuffer {
public:
static LogBuffer& getInstance();
+
+ // Singleton — no copy/move
+ LogBuffer(const LogBuffer&) = delete;
+ LogBuffer& operator=(const LogBuffer&) = delete;
+ LogBuffer(LogBuffer&&) = delete;
+ LogBuffer& operator=(LogBuffer&&) = delete;
// Write formatted output to both Serial and the ring buffer
void logPrintf(const char* format, ...) __attribute__((format(printf, 2, 3)));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class LogBuffer { | |
| public: | |
| static LogBuffer& getInstance(); | |
| // Write formatted output to both Serial and the ring buffer | |
| void logPrintf(const char* format, ...) __attribute__((format(printf, 2, 3))); | |
| // Copy current buffer contents into a pre-allocated char array. | |
| // Returns number of bytes written (excluding null terminator). | |
| size_t getLog(char* out, size_t outSize); | |
| void clear(); | |
| private: | |
| static constexpr size_t BUFFER_SIZE = 4096; | |
| char buffer_[BUFFER_SIZE]; | |
| size_t head_ = 0; | |
| size_t count_ = 0; | |
| SemaphoreHandle_t mutex_; | |
| LogBuffer(); | |
| void append(const char* data, size_t len); | |
| }; | |
| class LogBuffer { | |
| public: | |
| static LogBuffer& getInstance(); | |
| // Singleton — no copy/move | |
| LogBuffer(const LogBuffer&) = delete; | |
| LogBuffer& operator=(const LogBuffer&) = delete; | |
| LogBuffer(LogBuffer&&) = delete; | |
| LogBuffer& operator=(LogBuffer&&) = delete; | |
| // Write formatted output to both Serial and the ring buffer | |
| void logPrintf(const char* format, ...) __attribute__((format(printf, 2, 3))); | |
| // Copy current buffer contents into a pre-allocated char array. | |
| // Returns number of bytes written (excluding null terminator). | |
| size_t getLog(char* out, size_t outSize); | |
| void clear(); | |
| private: | |
| static constexpr size_t BUFFER_SIZE = 4096; | |
| char buffer_[BUFFER_SIZE]; | |
| size_t head_ = 0; | |
| size_t count_ = 0; | |
| SemaphoreHandle_t mutex_; | |
| LogBuffer(); | |
| void append(const char* data, size_t len); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/LogBuffer.h` around lines 10 - 33, Add deleted copy/move constructors and
assignment operators to the LogBuffer class to fully enforce the singleton;
inside the class declaration (near the private section with LogBuffer() and
append()), add declarations: LogBuffer(const LogBuffer&) = delete; LogBuffer&
operator=(const LogBuffer&) = delete; LogBuffer(LogBuffer&&) = delete;
LogBuffer& operator=(LogBuffer&&) = delete; so copying or moving the singleton
via copy/move ctor or assignment is a compile-time error.
| function fetchLogs() { | ||
| fetch('/api/logs') | ||
| .then(function(r) { return r.text(); }) | ||
| .then(function(data) { | ||
| logEl.textContent = data || '(empty)'; | ||
| if (autoEl.checked) logEl.scrollTop = logEl.scrollHeight; | ||
| }) | ||
| .catch(function(e) { logEl.textContent = 'Error: ' + e.message; }); | ||
| } | ||
| function copyLogs() { | ||
| var text = logEl.textContent; | ||
| if (navigator.clipboard) { | ||
| navigator.clipboard.writeText(text).then(function() { | ||
| var btn = document.querySelector('.btn'); | ||
| btn.textContent = 'Copied!'; | ||
| setTimeout(function() { btn.textContent = 'Copy'; }, 1500); | ||
| }); | ||
| } else { | ||
| var ta = document.createElement('textarea'); | ||
| ta.value = text; | ||
| document.body.appendChild(ta); | ||
| ta.select(); | ||
| document.execCommand('copy'); | ||
| document.body.removeChild(ta); | ||
| } | ||
| } | ||
| function clearLogs() { | ||
| fetch('/api/logs/clear', { method: 'POST' }) | ||
| .then(function() { fetchLogs(); }); | ||
| } | ||
| fetchLogs(); | ||
| setInterval(fetchLogs, 2000); |
There was a problem hiding this comment.
Avoid overlapping /api/logs polls.
setInterval(fetchLogs, 2000) can queue a new GET before the previous one finishes. On the ESP32 that can leave multiple open requests to the same endpoint, and an older response can repaint stale content immediately after clearLogs() succeeds.
Suggested fix
- function fetchLogs() {
- fetch('/api/logs')
- .then(function(r) { return r.text(); })
- .then(function(data) {
- logEl.textContent = data || '(empty)';
- if (autoEl.checked) logEl.scrollTop = logEl.scrollHeight;
- })
- .catch(function(e) { logEl.textContent = 'Error: ' + e.message; });
- }
+ var logsRequestInFlight = false;
+ function fetchLogs() {
+ if (logsRequestInFlight) return Promise.resolve();
+ logsRequestInFlight = true;
+ return fetch('/api/logs')
+ .then(function(r) { return r.text(); })
+ .then(function(data) {
+ logEl.textContent = data || '(empty)';
+ if (autoEl.checked) logEl.scrollTop = logEl.scrollHeight;
+ })
+ .catch(function(e) {
+ logEl.textContent = 'Error: ' + e.message;
+ })
+ .finally(function() {
+ logsRequestInFlight = false;
+ });
+ }
function clearLogs() {
fetch('/api/logs/clear', { method: 'POST' })
.then(function() { fetchLogs(); });
}
fetchLogs();
- setInterval(fetchLogs, 2000);
+ (function poll() {
+ fetchLogs().finally(function() {
+ setTimeout(poll, 2000);
+ });
+ })();🧰 Tools
🪛 Clang (14.0.6)
[warning] 138-138: if with identical then and else branches
(bugprone-branch-clone)
[note] 144-144: else branch starts here
(clang)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/LogViewerHTML.h` around lines 127 - 158, The polling allows overlapping
requests: setInterval(fetchLogs, 2000) can start a new fetchLogs before the
prior one completes causing stale responses (e.g., after clearLogs()). Fix by
serializing requests in fetchLogs using an "inFlight" flag (or an
AbortController) so fetchLogs returns immediately if a request is already
running; update the flag when the fetch promise settles; ensure clearLogs calls
fetchLogs() but relies on the inFlight guard so it won't race with an ongoing
request. Reference: fetchLogs, setInterval(fetchLogs, 2000), and clearLogs.
| void WebServerManager::handleApiLogs() { | ||
| _server.sendHeader("Access-Control-Allow-Origin", "*"); | ||
| char* buf = (char*)malloc(4097); | ||
| if (!buf) { | ||
| _server.send(500, "text/plain", "out of memory"); | ||
| return; | ||
| } | ||
| LogBuffer::getInstance().getLog(buf, 4097); | ||
| _server.send(200, "text/plain", buf); | ||
| free(buf); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using chunked/streaming response to reduce peak memory usage.
The handler allocates a 4KB buffer, then _server.send() internally copies it again, briefly doubling memory usage (~8KB). On a memory-constrained ESP32, this could cause allocation failures during high activity.
Consider using setContentLength() + sendContent() for streaming, similar to how handleApiSpoolmanSpools() (lines 501-522) streams large responses.
♻️ Optional streaming approach
void WebServerManager::handleApiLogs() {
_server.sendHeader("Access-Control-Allow-Origin", "*");
char* buf = (char*)malloc(4097);
if (!buf) {
_server.send(500, "text/plain", "out of memory");
return;
}
- LogBuffer::getInstance().getLog(buf, 4097);
- _server.send(200, "text/plain", buf);
+ size_t len = LogBuffer::getInstance().getLog(buf, 4097);
+ _server.setContentLength(len);
+ _server.send(200, "text/plain", "");
+ _server.sendContent(buf, len);
free(buf);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void WebServerManager::handleApiLogs() { | |
| _server.sendHeader("Access-Control-Allow-Origin", "*"); | |
| char* buf = (char*)malloc(4097); | |
| if (!buf) { | |
| _server.send(500, "text/plain", "out of memory"); | |
| return; | |
| } | |
| LogBuffer::getInstance().getLog(buf, 4097); | |
| _server.send(200, "text/plain", buf); | |
| free(buf); | |
| } | |
| void WebServerManager::handleApiLogs() { | |
| _server.sendHeader("Access-Control-Allow-Origin", "*"); | |
| char* buf = (char*)malloc(4097); | |
| if (!buf) { | |
| _server.send(500, "text/plain", "out of memory"); | |
| return; | |
| } | |
| size_t len = LogBuffer::getInstance().getLog(buf, 4097); | |
| _server.setContentLength(len); | |
| _server.send(200, "text/plain", ""); | |
| _server.sendContent(buf, len); | |
| free(buf); | |
| } |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 287-287: method 'handleApiLogs' can be made static
(readability-convert-member-functions-to-static)
[warning] 290-290: implicit conversion 'char *' -> bool
(readability-implicit-bool-conversion)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/WebServerManager.cpp` around lines 287 - 297, The current
WebServerManager::handleApiLogs allocates a 4KB buffer and calls _server.send
which causes an extra internal copy and spikes memory; change it to stream the
log content instead (like handleApiSpoolmanSpools does) by using
_server.setContentLength(...) and _server.sendContent/sendChunked with a small
fixed-size buffer passed to LogBuffer::getInstance().getLog to produce chunks,
or by supplying a content provider lambda that reads successive chunks from
LogBuffer::getInstance().getLog until exhausted; remove the large persistent
malloc/free and ensure headers (Access-Control-Allow-Origin) remain set before
starting the streamed response.
| // Log viewer | ||
| void handleLogViewer(); | ||
| void handleApiLogs(); | ||
| void handleApiLogsClear(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=$(fd -i '^WebServerManager\.cpp$' | head -n1)
echo "Inspecting: ${FILE}"
echo
echo "=== Log route registrations / handlers ==="
rg -n -C3 '"/logs"|"/api/logs"|handleLogViewer|handleApiLogs|handleApiLogsClear' "$FILE"
echo
echo "=== Existing auth / sensitive-route patterns for comparison ==="
rg -n -C3 'authenticate|authorized|auth|login|password|token|session|/config|/update' "$FILE"Repository: SpoolSense/spoolsense_scanner
Length of output: 3808
Add authentication guards to /api/logs and /api/logs/clear handlers, or document the intentional absence of auth.
The log routes lack authentication checks and are accessible to any LAN client. All three handlers (handleLogViewer, handleApiLogs, handleApiLogsClear) set Access-Control-Allow-Origin: * but have no auth guards. While this is consistent with other routes in WebServerManager.cpp (/config, /update, /api/config are also unprotected), the destructive /api/logs/clear endpoint and the exposure of sensitive diagnostics (device IDs, tag UIDs, broker hosts) warrant explicit protection. Either implement auth guards (headers-based, network-level, or token validation) or document that the product intentionally has no auth model and relies on network isolation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/WebServerManager.h` around lines 76 - 79, The log endpoints expose
sensitive data and a destructive clear action without auth; add authentication
guards in handleApiLogs and handleApiLogsClear (and apply same guard to
handleLogViewer) by validating a configured API token or Authorization header
(reject with 401 on failure), restrict or remove Access-Control-Allow-Origin: *
for those handlers, and ensure handleApiLogsClear requires elevated creds
(separate admin token or stronger check) and logs auth failures; alternatively,
if this behavior is intentional, add clear inline comments and README
documentation near WebServerManager (referencing handleLogViewer, handleApiLogs,
handleApiLogsClear) explaining the deliberate lack of auth and the network
isolation requirements.
1 participant
Summary
LogBufferclass — 4KB ring buffer singleton withlogPrintf()that writes to both Serial and the buffer, thread-safe via FreeRTOS mutex/logspage — dark-themed auto-scrolling log viewer matching the SpoolSense UI, polls/api/logsevery 2 secondsGET /api/logsreturns buffer contents as plain text,POST /api/logs/clearclears itInstrumented events
Test plan
Closes #139
Summary by CodeRabbit
Release Notes