Open
Conversation
Co-authored-by: netmindz <442066+netmindz@users.noreply.github.com>
- Auto-detect TLS: WiFiClientSecure → NetworkClientSecure → BearSSL - Remove old usermod_hue_v2.h (duplicate of PhilipsHueV2.cpp) - Remove accidentally committed generated files (js_iro.h, js_omggif.h) - Update readme with BearSSL and official platform build instructions Co-authored-by: netmindz <442066+netmindz@users.noreply.github.com>
Only change to const.h is the single usermod ID line. Removed all spurious changes from the MM branch that were incorrectly included. usermods_list.cpp is no longer modified. Co-authored-by: netmindz <442066+netmindz@users.noreply.github.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new ESP32-targeted Philips Hue V2 usermod providing CLIP V2 API integration with TLS-capable HTTPS, bridge mDNS discovery, link-button authentication, light discovery/persistence, SSE and polling support, color handling, UI/config hooks, plus a library manifest, README, and new usermod ID constant. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
1 task
Add artifact upload step for firmware binaries.
This reverts commit dd26d44.
… and improve error logging
Member
Author
|
@coderabbitai review - can you pay particular attention to if the code handles self-signed certificates please |
Contributor
🧩 Analysis chain🏁 Script executed: Length of output: 277766 🏁 Script executed: Length of output: 15751
Self-Signed Certificate Handling — SummaryThe code does handle self-signed certificates, but it does so by disabling all TLS certificate verification entirely, for both TLS backends: Native backend (WiFiClientSecure / NetworkClientSecure): client->setInsecure(); // Hue bridge uses self-signed certBearSSL backend: client->setInsecure(BearSSLClient::SNI::Insecure);The author is clearly aware of the self-signed cert issue (the comment acknowledges it), and Issues Found
VerdictFor a local-network-only embedded device, ✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp`:
- Around line 413-417: The pollInterval read via getJsonValue (and used by
loop() and advertised by appendConfigData()) is not clamped, allowing 0 or tiny
values; after loading pollInterval in the config loader (where
getJsonValue(top[F("pollInterval")], pollInterval, 2500UL) is called) enforce
the minimum advertised 1000 ms by checking the pollInterval variable and setting
it to at least 1000UL (e.g., if pollInterval < 1000UL then pollInterval =
1000UL) so loop() cannot spin into back-to-back TLS polls.
- Around line 298-310: The code is marking changed = true whenever newBri !=
lastBri even if the actual WLED local field (bri) wasn't modified; update the
logic in the PhilipsHueV2 brightness handling so that lastBri is updated
separately as a remote-cache update but changed is only set true when the
effective local variable bri is modified by this block (i.e., compare the
previous local bri value to the new effective bri after applying
applyOnOff/applyBri); refer to variables newBri, lastBri, bri, briLast,
applyOnOff, applyBri and ensure you still update lastBri regardless, but only
set changed = true when bri actually changed (so
colorUpdated(CALL_MODE_DIRECT_CHANGE)/stateUpdated() are only triggered on real
local changes).
- Around line 167-186: The read loop currently stops when client->connected()
goes false and can truncate/parse partial HTTP responses; change the logic that
reads the response (the loop using client->connected(), client->available(),
RESPONSE_BUF_SIZE and rawResponse) to keep draining client->available() even
after connected() becomes false, and after each read check if
rawResponse.length() > RESPONSE_BUF_SIZE and immediately fail/return (do not
silently truncate). After the loop ensure you drain any remaining
client->available() bytes before calling client->stop(), and when computing the
body offset use +2 for the "\n\n" fallback (i.e. if bodyStart found via "\n\n"
add 2, not 4) so the body substring is correct.
- Around line 121-123: The timeout passed to the TLS client is currently in
milliseconds (client->setTimeout(4)), which is 4 ms and will truncate reads;
change the call in PhilipsHueV2.cpp to client->setTimeout(4000) so the intended
4-second timeout is used (this affects subsequent readStringUntil('\n') calls
that rely on the stream timeout). Ensure you only update the numeric argument
where client->setTimeout(...) is invoked.
In `@usermods/PhilipsHueV2/readme.md`:
- Around line 18-30: Update the TLS Backend section to explicitly state that the
current implementation calls setInsecure() (skipping certificate validation) for
self-signed Hue Bridges, and clarify that this means HTTPS is used only for
transport framing and is vulnerable to LAN MITM so the hue-application-key
should not be assumed protected; mention the platforms/libraries
(WiFiClientSecure / NetworkClientSecure, ArduinoBearSSL) and add a short note
advising users to enable proper certificate validation when possible or restrict
LAN trust instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b0aa1932-d075-45e5-8845-11392f687f7b
📒 Files selected for processing (4)
usermods/PhilipsHueV2/PhilipsHueV2.cppusermods/PhilipsHueV2/library.jsonusermods/PhilipsHueV2/readme.mdwled00/const.h
Comment on lines
+121
to
+123
| if (!client) { DEBUG_PRINTF("[%s] TLS client alloc failed\n", _name); statusStr = F("Alloc failed"); return ""; } | ||
| client->setInsecure(); // Hue bridge uses self-signed cert | ||
| client->setTimeout(4); // 4 second timeout |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
According to the official Arduino Stream documentation, what units does Stream::setTimeout() use, and do WiFiClientSecure / NetworkClientSecure inherit that behavior?
💡 Result:
- Units:
Stream::setTimeout()uses milliseconds (theStreamAPI stores_timeoutas “number of milliseconds…” andsetTimeout()“sets maximum milliseconds to wait…”). (github.com) - Does
WiFiClientSecure/NetworkClientSecureinherit it? Yes. In Arduino-ESP32,WiFiClientSecureis just a typedef ofNetworkClientSecure. (github.com) AndNetworkClientSecureinheritsNetworkClient, (github.com) which inheritsClient(viaESPLwIPClient), (github.com) andClientinheritsStream. (github.com)
Citations:
- 1: https://github.com/arduino/ArduinoCore-API/blob/master/api/Stream.h
- 2: https://github.com/espressif/arduino-esp32/blob/3.3.1/libraries/NetworkClientSecure/src/WiFiClientSecure.h
- 3: https://github.com/espressif/arduino-esp32/blob/3.3.1/libraries/NetworkClientSecure/src/NetworkClientSecure.h
- 4: https://github.com/espressif/arduino-esp32/blob/3.3.1/libraries/Network/src/NetworkClient.h
- 5: https://github.com/arduino/ArduinoCore-API/blob/master/api/Client.h
Fix timeout units: setTimeout(4) is 4 milliseconds, not 4 seconds.
Stream::setTimeout() (inherited by WiFiClientSecure) takes milliseconds. The readStringUntil('\n') calls below use this timeout, so a 4 ms timeout will truncate header/body reads almost immediately. Change to 4000 for the documented 4-second timeout.
🔧 Fix
- client->setTimeout(4); // 4 second timeout
+ client->setTimeout(4000); // 4 second timeout📝 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.
Suggested change
| if (!client) { DEBUG_PRINTF("[%s] TLS client alloc failed\n", _name); statusStr = F("Alloc failed"); return ""; } | |
| client->setInsecure(); // Hue bridge uses self-signed cert | |
| client->setTimeout(4); // 4 second timeout | |
| if (!client) { DEBUG_PRINTF("[%s] TLS client alloc failed\n", _name); statusStr = F("Alloc failed"); return ""; } | |
| client->setInsecure(); // Hue bridge uses self-signed cert | |
| client->setTimeout(4000); // 4 second timeout |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` around lines 121 - 123, The timeout
passed to the TLS client is currently in milliseconds (client->setTimeout(4)),
which is 4 ms and will truncate reads; change the call in PhilipsHueV2.cpp to
client->setTimeout(4000) so the intended 4-second timeout is used (this affects
subsequent readStringUntil('\n') calls that rely on the stream timeout). Ensure
you only update the numeric argument where client->setTimeout(...) is invoked.
Comment on lines
+167
to
+186
| while (client->connected() && millis() < timeout) { | ||
| if (client->available()) { | ||
| String line = client->readStringUntil('\n'); | ||
| rawResponse += line + "\n"; | ||
| if (rawResponse.length() > RESPONSE_BUF_SIZE) break; | ||
| } | ||
| yield(); | ||
| } | ||
| client->stop(); | ||
|
|
||
| DEBUG_PRINTF("[%s] Response length: %u bytes\n", _name, rawResponse.length()); | ||
|
|
||
| // extract body (skip HTTP headers) | ||
| int bodyStart = rawResponse.indexOf("\r\n\r\n"); | ||
| if (bodyStart < 0) bodyStart = rawResponse.indexOf("\n\n"); | ||
| if (bodyStart < 0) { | ||
| DEBUG_PRINTF("[%s] No HTTP body delimiter found in response\n", _name); | ||
| return ""; | ||
| } | ||
| return rawResponse.substring(bodyStart + 4); |
Contributor
There was a problem hiding this comment.
Don't parse partial HTTP responses.
This loop exits when connected() turns false even if unread bytes are still buffered, then it silently truncates anything past RESPONSE_BUF_SIZE and still parses the result. That will turn valid Hue responses into malformed JSON. Please drain available() after disconnect and fail fast on overflow; the \n\n fallback also needs a 2-byte offset instead of +4.
🛠️ Minimal hardening
- while (client->connected() && millis() < timeout) {
+ while ((client->connected() || client->available()) && millis() < timeout) {
if (client->available()) {
String line = client->readStringUntil('\n');
rawResponse += line + "\n";
- if (rawResponse.length() > RESPONSE_BUF_SIZE) break;
+ if (rawResponse.length() > RESPONSE_BUF_SIZE) {
+ DEBUG_PRINTF("[%s] Response exceeds buffer\n", _name);
+ statusStr = F("Response too large");
+ client->stop();
+ return "";
+ }
}
yield();
}
@@
- int bodyStart = rawResponse.indexOf("\r\n\r\n");
- if (bodyStart < 0) bodyStart = rawResponse.indexOf("\n\n");
+ int bodyStart = rawResponse.indexOf("\r\n\r\n");
+ int delimiterLen = 4;
+ if (bodyStart < 0) {
+ bodyStart = rawResponse.indexOf("\n\n");
+ delimiterLen = 2;
+ }
if (bodyStart < 0) {
DEBUG_PRINTF("[%s] No HTTP body delimiter found in response\n", _name);
return "";
}
- return rawResponse.substring(bodyStart + 4);
+ return rawResponse.substring(bodyStart + delimiterLen);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` around lines 167 - 186, The read loop
currently stops when client->connected() goes false and can truncate/parse
partial HTTP responses; change the logic that reads the response (the loop using
client->connected(), client->available(), RESPONSE_BUF_SIZE and rawResponse) to
keep draining client->available() even after connected() becomes false, and
after each read check if rawResponse.length() > RESPONSE_BUF_SIZE and
immediately fail/return (do not silently truncate). After the loop ensure you
drain any remaining client->available() bytes before calling client->stop(), and
when computing the body offset use +2 for the "\n\n" fallback (i.e. if bodyStart
found via "\n\n" add 2, not 4) so the body substring is correct.
Comment on lines
+298
to
+310
| bool changed = false; | ||
| if (newBri != lastBri) { | ||
| DEBUG_PRINTF("[%s] Brightness changed: %d -> %d\n", _name, lastBri, newBri); | ||
| if (applyOnOff) { | ||
| if (newBri == 0) { bri = 0; } | ||
| else if (bri == 0 && newBri > 0) { bri = briLast; } | ||
| } | ||
| if (applyBri && newBri > 0) { | ||
| bri = newBri; | ||
| } | ||
| lastBri = newBri; | ||
| changed = true; | ||
| } |
Contributor
There was a problem hiding this comment.
Separate remote cache updates from local state changes.
This block sets changed = true whenever Hue brightness differs from lastBri, even if applyOnOff and applyBri leave bri untouched. That later drives colorUpdated(CALL_MODE_DIRECT_CHANGE), which fans out through stateUpdated() in wled00/led.cpp to notify/WebSocket/MQTT. Please only flip changed when a WLED field actually changed, and keep lastBri as a separate cache update.
🔄 One way to gate notifications on real local changes
bool changed = false;
if (newBri != lastBri) {
DEBUG_PRINTF("[%s] Brightness changed: %d -> %d\n", _name, lastBri, newBri);
+ bool briChanged = false;
if (applyOnOff) {
- if (newBri == 0) { bri = 0; }
- else if (bri == 0 && newBri > 0) { bri = briLast; }
+ if (newBri == 0 && bri != 0) {
+ bri = 0;
+ briChanged = true;
+ } else if (bri == 0 && newBri > 0) {
+ bri = briLast;
+ briChanged = true;
+ }
}
- if (applyBri && newBri > 0) {
+ if (applyBri && newBri > 0 && bri != newBri) {
bri = newBri;
+ briChanged = true;
}
lastBri = newBri;
- changed = true;
+ changed |= briChanged;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` around lines 298 - 310, The code is
marking changed = true whenever newBri != lastBri even if the actual WLED local
field (bri) wasn't modified; update the logic in the PhilipsHueV2 brightness
handling so that lastBri is updated separately as a remote-cache update but
changed is only set true when the effective local variable bri is modified by
this block (i.e., compare the previous local bri value to the new effective bri
after applying applyOnOff/applyBri); refer to variables newBri, lastBri, bri,
briLast, applyOnOff, applyBri and ensure you still update lastBri regardless,
but only set changed = true when bri actually changed (so
colorUpdated(CALL_MODE_DIRECT_CHANGE)/stateUpdated() are only triggered on real
local changes).
Comment on lines
+413
to
+417
| configComplete &= getJsonValue(top[F("pollInterval")], pollInterval, 2500UL); | ||
| configComplete &= getJsonValue(top[F("applyOnOff")], applyOnOff, true); | ||
| configComplete &= getJsonValue(top[F("applyBri")], applyBri, true); | ||
| configComplete &= getJsonValue(top[F("applyColor")], applyColor, true); | ||
| configComplete &= getJsonValue(top[F("attemptAuth")], attemptAuth, false); |
Contributor
There was a problem hiding this comment.
Clamp pollInterval when loading config.
appendConfigData() advertises a 1000 ms minimum, but this loader accepts 0 or other tiny values. A bad config edit/import will then turn loop() into back-to-back TLS polls.
⏱️ Clamp on read
configComplete &= getJsonValue(top[F("pollInterval")], pollInterval, 2500UL);
+ if (pollInterval < 1000UL) pollInterval = 1000UL;
configComplete &= getJsonValue(top[F("applyOnOff")], applyOnOff, true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` around lines 413 - 417, The
pollInterval read via getJsonValue (and used by loop() and advertised by
appendConfigData()) is not clamped, allowing 0 or tiny values; after loading
pollInterval in the config loader (where getJsonValue(top[F("pollInterval")],
pollInterval, 2500UL) is called) enforce the minimum advertised 1000 ms by
checking the pollInterval variable and setting it to at least 1000UL (e.g., if
pollInterval < 1000UL then pollInterval = 1000UL) so loop() cannot spin into
back-to-back TLS polls.
Comment on lines
+18
to
+30
| ### TLS Backend | ||
|
|
||
| The Hue V2 API requires HTTPS. The usermod auto-detects the available TLS | ||
| library at compile time: | ||
|
|
||
| | Backend | Library | Platform | | ||
| |---------|---------|----------| | ||
| | Native | `WiFiClientSecure` / `NetworkClientSecure` | Official `espressif32` platform | | ||
| | BearSSL | `ArduinoBearSSL` | Any platform (add as `lib_deps`) | | ||
|
|
||
| The default Tasmota-based platform used by WLED strips `WiFiClientSecure`. | ||
| To get TLS on Tasmota, add **ArduinoBearSSL**: | ||
|
|
Contributor
There was a problem hiding this comment.
Please document that the current TLS path skips certificate validation.
This section reads like normal certificate-validated HTTPS, but the implementation uses setInsecure() for self-signed bridges. Spell that out here so users do not assume the hue-application-key is protected against LAN MITM just because the transport is HTTPS.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/PhilipsHueV2/readme.md` around lines 18 - 30, Update the TLS Backend
section to explicitly state that the current implementation calls setInsecure()
(skipping certificate validation) for self-signed Hue Bridges, and clarify that
this means HTTPS is used only for transport framing and is vulnerable to LAN
MITM so the hue-application-key should not be assumed protected; mention the
platforms/libraries (WiFiClientSecure / NetworkClientSecure, ArduinoBearSSL) and
add a short note advising users to enable proper certificate validation when
possible or restrict LAN trust instead.
…t discovered lights, optimize memory usage, and improve connection backoff logic.
…ve TLS client management, enhance error logging, and update documentation with new features and troubleshooting steps.
Member
Author
|
It appears to be working for me, either if i use HTTP or build with the official ESP Platform (not tasmota as that lacks TLS) |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
usermods/PhilipsHueV2/PhilipsHueV2.cpp (3)
631-632:⚠️ Potential issue | 🟡 MinorClamp
pollIntervalon load.The UI advertises a 1000 ms minimum, but imported or hand-edited config can still set
0here and turnloop()into a tight poller.⏱️ Clamp after read
configComplete &= getJsonValue(top[F("pollInterval")], pollInterval, 2500UL); + if (pollInterval < 1000UL) pollInterval = 1000UL; configComplete &= getJsonValue(top[F("applyOnOff")], applyOnOff, true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` around lines 631 - 632, After loading pollInterval with getJsonValue (the line using getJsonValue(top[F("pollInterval")], pollInterval, 2500UL)), clamp its value to enforce the UI minimum (e.g. set pollInterval = max(pollInterval, 1000UL) or if (pollInterval < 1000UL) pollInterval = 1000UL) so a hand-edited or imported config can't set 0 and make loop() a tight poller; keep this clamp immediately after the read that also updates configComplete (the same block that reads pollInterval and applyOnOff).
282-307:⚠️ Potential issue | 🟠 MajorDon't parse a partially drained response.
This reader still exits on
connected()only, silently truncates onceRESPONSE_BUF_SIZEis hit, and always skips 4 bytes even when the delimiter found was\n\n. That can corrupt otherwise valid Hue JSON or drop buffered tail bytes after disconnect.🔧 Minimal hardening
- while (client->connected()) { + while (client->connected() || client->available()) { if (client->available()) { String line = client->readStringUntil('\n'); rawResponse += line + "\n"; - if (rawResponse.length() > RESPONSE_BUF_SIZE) break; + if (rawResponse.length() > RESPONSE_BUF_SIZE) { + statusStr = F("Response too large"); + client->stop(); + return ""; + } idleStart = millis(); // reset idle timer on data received } else { if (millis() - idleStart > 500) break; // no data for 500ms — done yield(); } } @@ - int bodyStart = rawResponse.indexOf("\r\n\r\n"); - if (bodyStart < 0) bodyStart = rawResponse.indexOf("\n\n"); + int bodyStart = rawResponse.indexOf("\r\n\r\n"); + int delimiterLen = 4; + if (bodyStart < 0) { + bodyStart = rawResponse.indexOf("\n\n"); + delimiterLen = 2; + } if (bodyStart < 0) { DEBUG_PRINTF("[%s] No HTTP body delimiter found in response\n", _name); return ""; } - return rawResponse.substring(bodyStart + 4); + return rawResponse.substring(bodyStart + delimiterLen);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` around lines 282 - 307, The reader must not silently parse a truncated/partially drained response and must respect the actual header delimiter length; update the loop around client->available()/client->connected() so it reads until the connection is closed or a read timeout (use idleStart) and keep draining any remaining available bytes before deciding the response is complete, avoid breaking out and returning a truncated rawResponse when rawResponse.length() > RESPONSE_BUF_SIZE — instead detect overflow and return an error/empty string (or log and abort) so callers don't parse partial data; also fix the bodyStart handling so you record which delimiter matched (check "\r\n\r\n" first, else "\n\n") and advance by 4 or 2 accordingly when calling rawResponse.substring(bodyStart + <delimiter_length>); ensure client->stop() still runs after the final drain/timeout.
500-512:⚠️ Potential issue | 🟠 MajorOnly mark
changedwhen WLED state actually changed.
lastBriis just the remote cache. IfapplyOnOffandapplyBrileavebriuntouched, this still drivescolorUpdated(CALL_MODE_DIRECT_CHANGE), which immediately fans outstateUpdated()notifications.🔄 Gate notifications on real local changes
bool changed = false; if (newBri != lastBri) { DEBUG_PRINTF("[%s] Brightness changed: %d -> %d\n", _name, lastBri, newBri); + bool briChanged = false; if (applyOnOff) { - if (newBri == 0) { bri = 0; } - else if (bri == 0 && newBri > 0) { bri = briLast; } + if (newBri == 0 && bri != 0) { bri = 0; briChanged = true; } + else if (bri == 0 && newBri > 0) { bri = briLast; briChanged = true; } } - if (applyBri && newBri > 0) { + if (applyBri && newBri > 0 && bri != newBri) { bri = newBri; + briChanged = true; } lastBri = newBri; - changed = true; + changed |= briChanged; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` around lines 500 - 512, The code sets changed = true whenever the remote brightness (lastBri) differs, even if local WLED brightness (bri) wasn't modified by the applyOnOff/applyBri logic; this triggers colorUpdated/CALL_MODE_DIRECT_CHANGE unnecessarily. Fix by capturing the original local brightness (e.g., prevBri = bri) before applying applyOnOff/applyBri, perform the same adjustments to bri, then only set changed = true (and update lastBri = newBri) if bri != prevBri; otherwise still update lastBri = newBri but do not mark changed or call color/state update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp`:
- Line 325: The debug logs currently print the full auth HTTP response
(DEBUG_PRINTF("[%s] Auth response: %s\n", _name, response.c_str())), which can
leak the bridge credential stored later in apiKey; change these DEBUG_PRINTF
calls (the one using response and the similar block around lines 348-354) to
avoid dumping the body — log only the response length and HTTP status (e.g.
response.size() and status code/description) or a redacted indicator like
"<REDACTED_AUTH_PAYLOAD>" so the auth payload and success.username are never
written to logs; ensure no other debug prints reference response.c_str() or
apiKey when handling the auth flow.
- Around line 671-677: The code only escapes apostrophes for metadata.name
before embedding it into a JS string; instead fully escape it as a JavaScript
string to prevent injection (escape backslashes, single and double quotes,
newlines, carriage returns, tabs, and the closing "</" sequence). Update the
logic that builds safeName (the variable safeName used in the oappend(...) calls
that produce addOption(dd,'...','id');) to perform these replacements (e.g.,
backslash -> \\ , ' -> \\' , " -> \\" , \n -> \\n , \r -> \\r , \t -> \\t , and
replace "</" with "<\/") so the generated script cannot be broken by control
characters or a </script> sequence.
- Line 207: The code unconditionally disables certificate validation via
tlsClient->setInsecure(); make this explicit and provide a secure alternative:
update the TLS creation path(s) that call tlsClient->setInsecure() to support
certificate pinning (e.g., accept a configured CA cert or fingerprint option)
and only call setInsecure() when an explicit configuration flag (e.g.,
config.allowInsecureTls) is true; if pinning is infeasible due to memory
constraints, document this decision in the README for PhilipsHueV2 and explain
the trade-off and fallback behavior, and add runtime logs that note when
insecure mode is in use so users are aware.
- Around line 196-197: tlsOwner currently holds derived TLS objects via
std::unique_ptr<Client>, which is undefined because Client lacks a virtual
destructor; change tlsOwner to hold concrete types (e.g.
std::unique_ptr<NetworkClientSecure> for native TLS path and
std::unique_ptr<BearSSLClient> for BearSSL path) and keep transportOwner as the
concrete WiFiClient type; when wiring the raw pointer into the polymorphic
client variable, assign client = tlsOwner.get() (or client =
transportOwner.get() where appropriate) so the unique_ptr owns the concrete
object and the correct derived destructor runs (update the assignments where
client is set in the native TLS and BearSSL code paths to use .get()).
---
Duplicate comments:
In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp`:
- Around line 631-632: After loading pollInterval with getJsonValue (the line
using getJsonValue(top[F("pollInterval")], pollInterval, 2500UL)), clamp its
value to enforce the UI minimum (e.g. set pollInterval = max(pollInterval,
1000UL) or if (pollInterval < 1000UL) pollInterval = 1000UL) so a hand-edited or
imported config can't set 0 and make loop() a tight poller; keep this clamp
immediately after the read that also updates configComplete (the same block that
reads pollInterval and applyOnOff).
- Around line 282-307: The reader must not silently parse a truncated/partially
drained response and must respect the actual header delimiter length; update the
loop around client->available()/client->connected() so it reads until the
connection is closed or a read timeout (use idleStart) and keep draining any
remaining available bytes before deciding the response is complete, avoid
breaking out and returning a truncated rawResponse when rawResponse.length() >
RESPONSE_BUF_SIZE — instead detect overflow and return an error/empty string (or
log and abort) so callers don't parse partial data; also fix the bodyStart
handling so you record which delimiter matched (check "\r\n\r\n" first, else
"\n\n") and advance by 4 or 2 accordingly when calling
rawResponse.substring(bodyStart + <delimiter_length>); ensure client->stop()
still runs after the final drain/timeout.
- Around line 500-512: The code sets changed = true whenever the remote
brightness (lastBri) differs, even if local WLED brightness (bri) wasn't
modified by the applyOnOff/applyBri logic; this triggers
colorUpdated/CALL_MODE_DIRECT_CHANGE unnecessarily. Fix by capturing the
original local brightness (e.g., prevBri = bri) before applying
applyOnOff/applyBri, perform the same adjustments to bri, then only set changed
= true (and update lastBri = newBri) if bri != prevBri; otherwise still update
lastBri = newBri but do not mark changed or call color/state update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cb303f1a-f48d-486c-892d-7aaeaa9ce878
📒 Files selected for processing (2)
usermods/PhilipsHueV2/PhilipsHueV2.cppusermods/PhilipsHueV2/readme.md
Comment on lines
+196
to
+197
| std::unique_ptr<Client> tlsOwner; // releases heap TLS wrapper on return | ||
| std::unique_ptr<WiFiClient> transportOwner; // releases heap WiFiClient used by BearSSL |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the relevant code sections
wc -l usermods/PhilipsHueV2/PhilipsHueV2.cppRepository: wled/WLED
Length of output: 95
🏁 Script executed:
# Read lines around 196-197 and 213-214, 228-232 to see the ownership code
sed -n '190,240p' usermods/PhilipsHueV2/PhilipsHueV2.cpp | cat -nRepository: wled/WLED
Length of output: 2773
🏁 Script executed:
# Search for where NetworkClientSecure and BearSSLClient are used/declared in this file
grep -n "NetworkClientSecure\|BearSSLClient\|tlsOwner" usermods/PhilipsHueV2/PhilipsHueV2.cpp | head -30Repository: wled/WLED
Length of output: 949
🏁 Script executed:
# Check if Arduino Client class definition is available locally
find . -name "Client.h" -type f | grep -i arduinoRepository: wled/WLED
Length of output: 35
🏁 Script executed:
# Search for NetworkClientSecure and BearSSLClient class definitions in repo
find . -type f \( -name "*.h" -o -name "*.cpp" \) -exec grep -l "class NetworkClientSecure\|class BearSSLClient" {} \;Repository: wled/WLED
Length of output: 35
🏁 Script executed:
# Search for any local Arduino library definitions or references to Client virtual destructor
grep -r "class Client" --include="*.h" | grep -i "client\|destructor" | head -10Repository: wled/WLED
Length of output: 35
🏁 Script executed:
# Check indentation at lines 196-197, 213-214, 228-232 explicitly
sed -n '196,197p' usermods/PhilipsHueV2/PhilipsHueV2.cpp | cat -A
sed -n '213,214p' usermods/PhilipsHueV2/PhilipsHueV2.cpp | cat -A
sed -n '228,232p' usermods/PhilipsHueV2/PhilipsHueV2.cpp | cat -ARepository: wled/WLED
Length of output: 579
🏁 Script executed:
# Search for destructor definitions in NetworkClientSecure and BearSSLClient
grep -r "~NetworkClientSecure\|~BearSSLClient\|~Client" --include="*.h" --include="*.cpp"Repository: wled/WLED
Length of output: 35
🌐 Web query:
Arduino Client base class virtual destructor
💡 Result:
In the official Arduino API (arduino::Client), the Client base class declares many virtual member functions but does not declare a (virtual) destructor at all. That means it has an implicitly-declared non-virtual destructor. So deleting a derived object (e.g., WiFiClient, EthernetClient) through a Client* is undefined behavior. [1][2]
Implications / what to do instead
- Don’t make
Client*an owning pointer. Store aClient&(orClient*that you neverdelete) and let the caller/library own the concrete client object. - If you truly need ownership, don’t delete via
Client*; instead keep the concrete type in the deleter, e.g.std::unique_ptr<Client, Deleter>whereDeleterknows the real type (or wrap it in a type-erased owner likestd::shared_ptr<void>with a custom deleter that deletes the concrete type).
Sources: ArduinoCore-API Client.h definition (no destructor) [1], C++ rule about deleting via base pointer requiring a virtual base destructor [2].
🌐 Web query:
NetworkClientSecure destructor Arduino ESP32
💡 Result:
In the ESP32 Arduino core, NetworkClientSecure’s destructor does not explicitly call stop(). Instead:
- The TLS/socket context is held in a
std::shared_ptr<sslclient_context>with a custom deleter that callsstop_ssl_socket(sslclient)and thendelete sslclient. When theNetworkClientSecureobject is destroyed, the shared_ptr’s destructor runs and closes/frees the TLS/socket context automatically (as long as no other shared_ptr copies still exist). [1] ~NetworkClientSecure()only frees certificate/key buffers conditionally, based on internal flags:- Frees
_CA_certonly if_ca_cert_free && _CA_cert - Frees
_certonly if_cert_free && _cert - Frees
_private_keyonly if_private_key_free && _private_key[1]
- Frees
Practical implications:
- If you pass cert pointers via
setCACert() / setCertificate() / setPrivateKey(), the destructor generally won’t free them (the “*_free” flags stay false). [1] - If you load them via
loadCACert() / loadCertificate() / loadPrivateKey(), the library allocates buffers and sets the corresponding “*_free” flag, so the destructor will free them. [1]
Sources: NetworkClientSecure.cpp / NetworkClientSecure.h in espressif/arduino-esp32. [1] [2]
Use concrete types for TLS ownership to ensure correct destructor calls.
std::unique_ptr<Client> tlsOwner stores derived pointers (NetworkClientSecure* or BearSSLClient*), but Arduino's Client base class has no virtual destructor. When the unique_ptr destructs, it invokes undefined behavior by calling the non-virtual base destructor instead of the derived destructor, bypassing cleanup logic in NetworkClientSecure::~NetworkClientSecure() and BearSSLClient::~BearSSLClient(). Store concrete types instead:
- std::unique_ptr<Client> tlsOwner; // releases heap TLS wrapper on return
+ `#ifdef` HUE_TLS_BACKEND_NATIVE
+ std::unique_ptr<NetworkClientSecure> nativeTlsOwner;
+ `#endif`
+ `#ifdef` HUE_TLS_BACKEND_BEARSSL
+ std::unique_ptr<BearSSLClient> bearTlsOwner;
+ `#endif`
std::unique_ptr<WiFiClient> transportOwner; // releases heap WiFiClient used by BearSSLUpdate assignment at 213-214 (native) and 228-232 (BearSSL) to use .get() when assigning to client.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` around lines 196 - 197, tlsOwner
currently holds derived TLS objects via std::unique_ptr<Client>, which is
undefined because Client lacks a virtual destructor; change tlsOwner to hold
concrete types (e.g. std::unique_ptr<NetworkClientSecure> for native TLS path
and std::unique_ptr<BearSSLClient> for BearSSL path) and keep transportOwner as
the concrete WiFiClient type; when wiring the raw pointer into the polymorphic
client variable, assign client = tlsOwner.get() (or client =
transportOwner.get() where appropriate) so the unique_ptr owns the concrete
object and the correct derived destructor runs (update the assignments where
client is set in the native TLS and BearSSL code paths to use .get()).
| auto *tlsClient = new WiFiClientSecure; | ||
| #endif | ||
| if (!tlsClient) { DEBUG_PRINTF("[%s] TLS alloc failed\n", _name); statusStr = F("Alloc failed"); return ""; } | ||
| tlsClient->setInsecure(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd PhilipsHueV2 --type fRepository: wled/WLED
Length of output: 91
🏁 Script executed:
cat -n usermods/PhilipsHueV2/PhilipsHueV2.cpp | head -250 | tail -100Repository: wled/WLED
Length of output: 4842
🏁 Script executed:
cat -n usermods/PhilipsHueV2/PhilipsHueV2.cpp | sed -n '190,240p'Repository: wled/WLED
Length of output: 2773
🏁 Script executed:
ls -la usermods/PhilipsHueV2/Repository: wled/WLED
Length of output: 361
🏁 Script executed:
cat usermods/PhilipsHueV2/readme.mdRepository: wled/WLED
Length of output: 5478
🏁 Script executed:
rg -A 5 -B 5 "setCertificate|pinCertificate|fingerprint|setKnownKey" usermods/PhilipsHueV2/Repository: wled/WLED
Length of output: 35
🏁 Script executed:
rg -A 3 -B 3 "setInsecure|certificate" usermods/PhilipsHueV2/PhilipsHueV2.cppRepository: wled/WLED
Length of output: 989
🏁 Script executed:
rg "setCertificate|pinCertificate|fingerprint|setKnownKey" usermods/ --type cpp --type hRepository: wled/WLED
Length of output: 35
🏁 Script executed:
rg "setInsecure" usermods/ --type cpp -B 3 -A 1 | head -50Repository: wled/WLED
Length of output: 1123
Document why certificate validation is disabled for Hue bridge connections, or implement certificate pinning.
Both TLS backends (native and BearSSL) unconditionally call setInsecure() with no alternative for certificate validation. The bridge identity is never authenticated despite every request carrying hue-application-key.
The README should plainly document whether this is a memory constraint limitation (the code shows buffer optimization and heap-allocated TLS clients) or a design choice. If memory prevents certificate pinning, that trade-off must be explicit to users. If it's feasible, add a pinned CA/fingerprint option and leave insecure mode as an explicit fallback only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` at line 207, The code unconditionally
disables certificate validation via tlsClient->setInsecure(); make this explicit
and provide a secure alternative: update the TLS creation path(s) that call
tlsClient->setInsecure() to support certificate pinning (e.g., accept a
configured CA cert or fingerprint option) and only call setInsecure() when an
explicit configuration flag (e.g., config.allowInsecureTls) is true; if pinning
is infeasible due to memory constraints, document this decision in the README
for PhilipsHueV2 and explain the trade-off and fallback behavior, and add
runtime logs that note when insecure mode is in use so users are aware.
| return; | ||
| } | ||
|
|
||
| DEBUG_PRINTF("[%s] Auth response: %s\n", _name, response.c_str()); |
Contributor
There was a problem hiding this comment.
Redact the auth payload.
The next block extracts success.username from this same response and stores it in apiKey, so dumping the full body here leaks the bridge credential into debug logs. Log length/status only.
🔒 Minimal redaction
- DEBUG_PRINTF("[%s] Auth response: %s\n", _name, response.c_str());
+ DEBUG_PRINTF("[%s] Auth response received (%u bytes)\n", _name, response.length());Also applies to: 348-354
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` at line 325, The debug logs currently
print the full auth HTTP response (DEBUG_PRINTF("[%s] Auth response: %s\n",
_name, response.c_str())), which can leak the bridge credential stored later in
apiKey; change these DEBUG_PRINTF calls (the one using response and the similar
block around lines 348-354) to avoid dumping the body — log only the response
length and HTTP status (e.g. response.size() and status code/description) or a
redacted indicator like "<REDACTED_AUTH_PAYLOAD>" so the auth payload and
success.username are never written to logs; ensure no other debug prints
reference response.c_str() or apiKey when handling the auth flow.
Comment on lines
+671
to
+677
| oappend(F("addOption(dd,'")); | ||
| String safeName = String(name ? name : "?"); | ||
| safeName.replace("'", "\\'"); | ||
| oappend(safeName.c_str()); | ||
| oappend(F("','")); | ||
| oappend(id); | ||
| oappend(F("');")); |
Contributor
There was a problem hiding this comment.
Escape light names as JavaScript strings, not just apostrophes.
metadata.name is bridge-controlled input that gets embedded into a generated script. Escaping only ' still leaves backslashes, CR/LF, and </script> able to break the settings page or inject code.
🛡️ Minimal escaping hardening
String safeName = String(name ? name : "?");
+ safeName.replace("\\", "\\\\");
safeName.replace("'", "\\'");
+ safeName.replace("\r", "\\r");
+ safeName.replace("\n", "\\n");
+ safeName.replace("</", "<\\/");
oappend(safeName.c_str());📝 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.
Suggested change
| oappend(F("addOption(dd,'")); | |
| String safeName = String(name ? name : "?"); | |
| safeName.replace("'", "\\'"); | |
| oappend(safeName.c_str()); | |
| oappend(F("','")); | |
| oappend(id); | |
| oappend(F("');")); | |
| oappend(F("addOption(dd,'")); | |
| String safeName = String(name ? name : "?"); | |
| safeName.replace("\\", "\\\\"); | |
| safeName.replace("'", "\\'"); | |
| safeName.replace("\r", "\\r"); | |
| safeName.replace("\n", "\\n"); | |
| safeName.replace("</", "<\\/"); | |
| oappend(safeName.c_str()); | |
| oappend(F("','")); | |
| oappend(id); | |
| oappend(F("');")); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` around lines 671 - 677, The code only
escapes apostrophes for metadata.name before embedding it into a JS string;
instead fully escape it as a JavaScript string to prevent injection (escape
backslashes, single and double quotes, newlines, carriage returns, tabs, and the
closing "</" sequence). Update the logic that builds safeName (the variable
safeName used in the oappend(...) calls that produce addOption(dd,'...','id');)
to perform these replacements (e.g., backslash -> \\ , ' -> \\' , " -> \\" , \n
-> \\n , \r -> \\r , \t -> \\t , and replace "</" with "<\/") so the generated
script cannot be broken by control characters or a </script> sequence.
…pdates, refactor JSON parsing, improve error handling, and enhance configuration management.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (7)
usermods/PhilipsHueV2/PhilipsHueV2.cpp (7)
340-340:⚠️ Potential issue | 🟠 MajorRedact auth payload logs.
Line 340 logs the full auth response, which contains credential material later copied into
apiKey.🔧 Redact response body
- DEBUG_PRINTF("[%s] Auth response: %s\n", _name, response.c_str()); + DEBUG_PRINTF("[%s] Auth response received (%u bytes)\n", _name, response.length());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` at line 340, The DEBUG_PRINTF call in PhilipsHueV2.cpp that prints the full auth response (uses _name and response) leaks credentials later stored in apiKey; change this to avoid printing the response body — either remove the response from the log entirely or log only non-sensitive metadata (e.g., success/failure or masked token) and/or redact sensitive fields before logging. Locate the DEBUG_PRINTF("[%s] Auth response: %s\n", _name, response.c_str()) and replace it with a log that does not include response.c_str() or a masked version of the token.
223-223:⚠️ Potential issue | 🟠 Major
setTimeout(4)/setTimeout(2)are milliseconds, not seconds.Line 223 and Line 253 make
readStringUntil('\n')effectively time out almost immediately inhttpsRequest(), which can truncate headers/body reads.🔧 Fix timeout units
- tlsClient->setTimeout(4); + tlsClient->setTimeout(4000); - tcpClient.setTimeout(2); + tcpClient.setTimeout(2000); - ssePlainClient.setTimeout(2); + ssePlainClient.setTimeout(2000); - tlsClient->setTimeout(4); + tlsClient->setTimeout(4000);Also applies to: 253-253, 545-545, 557-557
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` at line 223, The tlsClient->setTimeout calls are using seconds (e.g. setTimeout(4) / setTimeout(2)) but the API expects milliseconds, causing readStringUntil('\n') in httpsRequest() and other places to time out immediately; update each tlsClient->setTimeout(...) invocation referenced (including calls in httpsRequest() and the other occurrences around lines 223, 253, 545, 557) to use millisecond values (e.g. 4000, 2000, etc.) appropriate for the intended timeout duration so header/body reads complete reliably.
300-323:⚠️ Potential issue | 🟠 MajorDo not parse partial/truncated HTTP responses.
At Line 300, reading stops on
connected()==falseeven if unread bytes remain buffered; at Line 304 truncation is silent; and Line 322 always uses+4even for\n\nfallback.🔧 Minimal hardening
- while (client->connected()) { + while (client->connected() || client->available()) { if (client->available()) { String line = client->readStringUntil('\n'); rawResponse += line + "\n"; - if (rawResponse.length() > RESPONSE_BUF_SIZE) break; + if (rawResponse.length() > RESPONSE_BUF_SIZE) { + DEBUG_PRINTF("[%s] Response exceeds buffer\n", _name); + statusStr = F("Response too large"); + client->stop(); + return ""; + } idleStart = millis(); // reset idle timer on data received } else { if (millis() - idleStart > 500) break; // no data for 500ms — done yield(); } } // extract body (skip HTTP headers) int bodyStart = rawResponse.indexOf("\r\n\r\n"); - if (bodyStart < 0) bodyStart = rawResponse.indexOf("\n\n"); + int delimiterLen = 4; + if (bodyStart < 0) { + bodyStart = rawResponse.indexOf("\n\n"); + delimiterLen = 2; + } if (bodyStart < 0) { DEBUG_PRINTF("[%s] No HTTP body delimiter found in response\n", _name); return ""; } - return rawResponse.substring(bodyStart + 4); + return rawResponse.substring(bodyStart + delimiterLen);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` around lines 300 - 323, The read loop can silently return partial responses and wrongly compute body offset; modify the client read logic (the loop using client, client->available(), rawResponse, idleStart and RESPONSE_BUF_SIZE) to continue draining client->available() bytes even if client->connected() becomes false so buffered data is not lost, and detect when rawResponse exceeds RESPONSE_BUF_SIZE to treat the response as truncated (log with DEBUG_PRINTF and return empty or an error rather than silently truncating). Also fix body extraction: when finding the delimiter in bodyStart, use the correct offset depending on which delimiter matched (if "\r\n\r\n" matched add 4, if "\n\n" matched add 2) instead of always adding 4. Ensure any early-exit paths stop the client (client->stop()) and log the failure.
934-935:⚠️ Potential issue | 🟠 MajorClamp
pollIntervalwhen loading config.Line 934 accepts values below the UI-advertised minimum (1000 ms), which can drive excessive poll frequency.
🔧 Clamp on read
configComplete &= getJsonValue(top[F("pollInterval")], pollInterval, 2500UL); + if (pollInterval < 1000UL) pollInterval = 1000UL;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` around lines 934 - 935, The config loader currently lets pollInterval be set below the UI-advertised minimum; after calling getJsonValue for pollInterval (the line using getJsonValue(top[F("pollInterval")], pollInterval, 2500UL)), clamp the value to a safe range (at minimum 1000 ms, optionally also enforce a sensible upper bound like 60000 ms) and assign the clamped value back to pollInterval before continuing to use it; update the block around pollInterval, configComplete and getJsonValue so that pollInterval is validated and clamped immediately after it is read.
471-483:⚠️ Potential issue | 🟠 MajorOnly mark
changedwhen local WLED state actually changes.At Line 482,
changed = trueis currently set whenever remote brightness cache differs, even ifbriwas not modified byapplyOnOff/applyBri. That causes unnecessarycolorUpdated()fanout.🔧 Gate notifications on real local changes
bool changed = false; if (newBri != lastBri) { DEBUG_PRINTF("[%s] Brightness changed: %d -> %d\n", _name, lastBri, newBri); + bool briChanged = false; if (applyOnOff) { - if (newBri == 0) { bri = 0; } - else if (bri == 0 && newBri > 0) { bri = briLast; } + if (newBri == 0 && bri != 0) { bri = 0; briChanged = true; } + else if (bri == 0 && newBri > 0) { bri = briLast; briChanged = true; } } - if (applyBri && newBri > 0) { - bri = newBri; + if (applyBri && newBri > 0 && bri != newBri) { + bri = newBri; + briChanged = true; } lastBri = newBri; - changed = true; + changed |= briChanged; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` around lines 471 - 483, The handler currently sets changed = true whenever remote newBri != lastBri even if local bri stays the same; capture the prior local value (e.g. prevBri = bri) before applying applyOnOff/applyBri logic and only set changed = true if bri != prevBri, while still updating lastBri = newBri (so the remote cache is kept in sync). Refer to variables prevBri (new), bri, lastBri, newBri, applyOnOff, applyBri, briLast and the colorUpdated() fanout when making this change.
79-80:⚠️ Potential issue | 🔴 CriticalAvoid owning TLS instances through
std::unique_ptr<Client>.At Line 79 and Line 211, owning derived TLS clients via
std::unique_ptr<Client>is unsafe ifClientlacks a virtual destructor in the active core. Keep ownership in concrete typed pointers and expose only non-owningClient*views.🔧 Safer ownership pattern
- std::unique_ptr<Client> sseTlsOwner; // owns heap-allocated TLS wrapper + `#ifdef` HUE_TLS_BACKEND_NATIVE + std::unique_ptr<NetworkClientSecure> sseNativeTlsOwner; + `#endif` + `#ifdef` HUE_TLS_BACKEND_BEARSSL + std::unique_ptr<BearSSLClient> sseBearTlsOwner; + `#endif` ... - std::unique_ptr<Client> tlsOwner; // releases heap TLS wrapper on return + `#ifdef` HUE_TLS_BACKEND_NATIVE + std::unique_ptr<NetworkClientSecure> nativeTlsOwner; + `#endif` + `#ifdef` HUE_TLS_BACKEND_BEARSSL + std::unique_ptr<BearSSLClient> bearTlsOwner; + `#endif`Also applies to: 211-212, 562-563, 621-622
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` around lines 79 - 80, The code currently owns TLS client instances using std::unique_ptr<Client> (e.g., sseTlsOwner and sseTransportOwner) which is unsafe if Client lacks a virtual destructor; change ownership to concrete types (for example std::unique_ptr<WiFiClientSecure> or the specific TLS wrapper class you allocate) and keep any existing non-owning Client* views (the raw pointers used for polymorphic calls) as non-owning only. Locate sseTlsOwner, sseTransportOwner and the other owner pointers flagged (the similar owner variables around lines referenced) and: replace their std::unique_ptr<Client> declarations with std::unique_ptr<ConcreteTLSType> / std::unique_ptr<WiFiClient> as appropriate, update their construction sites to new ConcreteTLSType, and ensure the code still assigns a Client* view variable to the concrete.get() where needed; do the same for the other owner pairs mentioned so ownership is always by concrete type and only non-owning Client* pointers are used for polymorphic access.
979-985:⚠️ Potential issue | 🟠 MajorEscape light names as full JS strings before injecting into script.
At Line 981 only
'is escaped. Bridge-provided names with\, CR/LF, or</can break the generated settings script.🔧 Harden JS escaping
String safeName = String(name ? name : "?"); + safeName.replace("\\", "\\\\"); safeName.replace("'", "\\'"); + safeName.replace("\"", "\\\""); + safeName.replace("\n", "\\n"); + safeName.replace("\r", "\\r"); + safeName.replace("\t", "\\t"); + safeName.replace("</", "<\\/"); oappend(safeName.c_str());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` around lines 979 - 985, The injected light name is not fully escaped for JS single-quoted strings: only single quotes are handled in safeName, leaving backslashes, CR/LF, tabs and the "</" sequence that can break the generated script. Fix by fully escaping the string before oappend: handle backslash first (replace "\" with "\\\\"), then single quote ("'" -> "\\'"), carriage return and newline ("\r" -> "\\r", "\n" -> "\\n"), tab ("\t" -> "\\t"), and escape the closing script sequence by replacing "</" with "<\/"; encapsulate this logic in a small helper (e.g. escapeJSString or similar) and use that for safeName when calling oappend/addOption so the generated call to addOption(dd,'<escapedName>','id'); is always safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp`:
- Line 340: The DEBUG_PRINTF call in PhilipsHueV2.cpp that prints the full auth
response (uses _name and response) leaks credentials later stored in apiKey;
change this to avoid printing the response body — either remove the response
from the log entirely or log only non-sensitive metadata (e.g., success/failure
or masked token) and/or redact sensitive fields before logging. Locate the
DEBUG_PRINTF("[%s] Auth response: %s\n", _name, response.c_str()) and replace it
with a log that does not include response.c_str() or a masked version of the
token.
- Line 223: The tlsClient->setTimeout calls are using seconds (e.g.
setTimeout(4) / setTimeout(2)) but the API expects milliseconds, causing
readStringUntil('\n') in httpsRequest() and other places to time out
immediately; update each tlsClient->setTimeout(...) invocation referenced
(including calls in httpsRequest() and the other occurrences around lines 223,
253, 545, 557) to use millisecond values (e.g. 4000, 2000, etc.) appropriate for
the intended timeout duration so header/body reads complete reliably.
- Around line 300-323: The read loop can silently return partial responses and
wrongly compute body offset; modify the client read logic (the loop using
client, client->available(), rawResponse, idleStart and RESPONSE_BUF_SIZE) to
continue draining client->available() bytes even if client->connected() becomes
false so buffered data is not lost, and detect when rawResponse exceeds
RESPONSE_BUF_SIZE to treat the response as truncated (log with DEBUG_PRINTF and
return empty or an error rather than silently truncating). Also fix body
extraction: when finding the delimiter in bodyStart, use the correct offset
depending on which delimiter matched (if "\r\n\r\n" matched add 4, if "\n\n"
matched add 2) instead of always adding 4. Ensure any early-exit paths stop the
client (client->stop()) and log the failure.
- Around line 934-935: The config loader currently lets pollInterval be set
below the UI-advertised minimum; after calling getJsonValue for pollInterval
(the line using getJsonValue(top[F("pollInterval")], pollInterval, 2500UL)),
clamp the value to a safe range (at minimum 1000 ms, optionally also enforce a
sensible upper bound like 60000 ms) and assign the clamped value back to
pollInterval before continuing to use it; update the block around pollInterval,
configComplete and getJsonValue so that pollInterval is validated and clamped
immediately after it is read.
- Around line 471-483: The handler currently sets changed = true whenever remote
newBri != lastBri even if local bri stays the same; capture the prior local
value (e.g. prevBri = bri) before applying applyOnOff/applyBri logic and only
set changed = true if bri != prevBri, while still updating lastBri = newBri (so
the remote cache is kept in sync). Refer to variables prevBri (new), bri,
lastBri, newBri, applyOnOff, applyBri, briLast and the colorUpdated() fanout
when making this change.
- Around line 79-80: The code currently owns TLS client instances using
std::unique_ptr<Client> (e.g., sseTlsOwner and sseTransportOwner) which is
unsafe if Client lacks a virtual destructor; change ownership to concrete types
(for example std::unique_ptr<WiFiClientSecure> or the specific TLS wrapper class
you allocate) and keep any existing non-owning Client* views (the raw pointers
used for polymorphic calls) as non-owning only. Locate sseTlsOwner,
sseTransportOwner and the other owner pointers flagged (the similar owner
variables around lines referenced) and: replace their std::unique_ptr<Client>
declarations with std::unique_ptr<ConcreteTLSType> / std::unique_ptr<WiFiClient>
as appropriate, update their construction sites to new ConcreteTLSType, and
ensure the code still assigns a Client* view variable to the concrete.get()
where needed; do the same for the other owner pairs mentioned so ownership is
always by concrete type and only non-owning Client* pointers are used for
polymorphic access.
- Around line 979-985: The injected light name is not fully escaped for JS
single-quoted strings: only single quotes are handled in safeName, leaving
backslashes, CR/LF, tabs and the "</" sequence that can break the generated
script. Fix by fully escaping the string before oappend: handle backslash first
(replace "\" with "\\\\"), then single quote ("'" -> "\\'"), carriage return and
newline ("\r" -> "\\r", "\n" -> "\\n"), tab ("\t" -> "\\t"), and escape the
closing script sequence by replacing "</" with "<\/"; encapsulate this logic in
a small helper (e.g. escapeJSString or similar) and use that for safeName when
calling oappend/addOption so the generated call to
addOption(dd,'<escapedName>','id'); is always safe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 391504e2-338f-4856-8fc9-8937464f67e4
📒 Files selected for processing (1)
usermods/PhilipsHueV2/PhilipsHueV2.cpp
…mod, enhance configuration behavior, and improve status reporting.
…mod, enhance configuration behavior, and improve status reporting.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (10)
usermods/PhilipsHueV2/readme.md (1)
43-58:⚠️ Potential issue | 🟡 MinorDocument that TLS certificate validation is disabled.
The TLS section describes HTTPS support but doesn't mention that the code uses
setInsecure()to skip certificate validation for self-signed Hue bridge certificates. Users should understand that:
- The
hue-application-keyis transmitted over an unvalidated TLS channel- A LAN attacker could potentially MITM the connection
- This is a pragmatic trade-off for compatibility with Hue bridge self-signed certs
Consider adding a note like:
Security note: The Hue bridge uses a self-signed certificate, so TLS
certificate validation is disabled (setInsecure()). This provides transport
encryption but does not authenticate the bridge. On a trusted LAN this is
acceptable; on untrusted networks thehue-application-keycould be
intercepted via MITM.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/PhilipsHueV2/readme.md` around lines 43 - 58, Add a clear security note to the TLS Backend section stating that certificate validation is disabled because the code calls setInsecure(), explain that this means the Hue bridge is not authenticated (only encrypted), and explicitly warn that the hue-application-key is transmitted over an unvalidated TLS channel and may be intercepted by a LAN MITM; place the note near the existing TLS Backend text and mention setInsecure() and the trade-off for compatibility with self-signed Hue certificates.usermods/PhilipsHueV2/PhilipsHueV2.cpp (9)
976-976:⚠️ Potential issue | 🟡 MinorClamp
pollIntervalto the documented minimum.
appendConfigData()advertises a 1000 ms minimum, but this loader accepts any value. A misconfigured or malicious import could spinloop()into back-to-back TLS polls.🔧 Fix
configComplete &= getJsonValue(top[F("pollInterval")], pollInterval, 2500UL); + if (pollInterval < 1000UL) pollInterval = 1000UL;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` at line 976, The loader reads pollInterval via getJsonValue(top[F("pollInterval")], pollInterval, 2500UL) but does not enforce the documented 1000 ms minimum; after that call (in PhilipsHueV2.cpp where pollInterval is set), clamp pollInterval to at least 1000UL (e.g., pollInterval = max(pollInterval, 1000UL)) so malformed or malicious configs cannot set it below the advertised minimum; keep configComplete handling as-is and only adjust the value used by the polling logic.
367-367:⚠️ Potential issue | 🟠 MajorRedact the auth payload in debug logs.
This line dumps the full auth response which contains the API key (
success.username), leaking the credential into debug logs.🔧 Fix
- DEBUG_PRINTF("[%s] Auth response: %s\n", _name, response.c_str()); + DEBUG_PRINTF("[%s] Auth response received (%u bytes)\n", _name, response.length());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` at line 367, The DEBUG_PRINTF call printing the full auth response (DEBUG_PRINTF("[%s] Auth response: %s\n", _name, response.c_str())) leaks the API key (success.username); replace this by logging a non-sensitive summary instead: do not print the whole response string, either log only fixed metadata (e.g., connection status or success flag) or parse the JSON in the auth response and redact the success.username value (replace with "<REDACTED>" or omit the field) before passing to DEBUG_PRINTF, ensuring the change targets the DEBUG_PRINTF call and the variables _name and response used there.
327-337:⚠️ Potential issue | 🟠 MajorResponse reading loop may truncate data after disconnect.
The loop exits when
connected()turns false, but buffered bytes may remain unread. Consider drainingavailable()after disconnect.Also, the overflow case silently breaks without failing:
🛠️ Suggested hardening
- while (client->connected()) { + while (client->connected() || client->available()) { if (client->available()) { String line = client->readStringUntil('\n'); rawResponse += line + "\n"; - if (rawResponse.length() > RESPONSE_BUF_SIZE) break; + if (rawResponse.length() > RESPONSE_BUF_SIZE) { + DEBUG_PRINTF("[%s] Response exceeds buffer\n", _name); + statusStr = F("Response too large"); + client->stop(); + return ""; + } idleStart = millis(); // reset idle timer on data received🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` around lines 327 - 337, The response loop in PhilipsHueV2.cpp using client->connected() / client->available() may exit while unread buffered bytes remain and silently break on RESPONSE_BUF_SIZE overflow; modify the loop in the method that builds rawResponse to after the connected() loop run a draining loop that reads remaining client->available() bytes (using client->read() or readStringUntil) into rawResponse until no data remains or RESPONSE_BUF_SIZE is reached, and when RESPONSE_BUF_SIZE is exceeded set/return an explicit error or truncation flag instead of silently breaking; ensure idleStart/millis() logic remains for normal reads and preserve yield() calls while draining.
249-250:⚠️ Potential issue | 🟠 MajorFix timeout units:
setTimeout(4)is 4 milliseconds, not 4 seconds.
Stream::setTimeout()takes milliseconds. ThereadStringUntil('\n')calls use this timeout, so 4 ms will truncate reads almost immediately.🔧 Fix
- tlsClient->setTimeout(4); + tlsClient->setTimeout(4000); // 4 second timeout🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` around lines 249 - 250, The timeout passed to tlsClient->setTimeout(4) is in milliseconds, causing Stream reads (e.g., readStringUntil('\n')) to be truncated; change the call in PhilipsHueV2.cpp to use seconds-to-milliseconds (e.g., setTimeout(4000) or setTimeout(4 * 1000UL)) so the TLS client has a 4-second timeout and Stream::setTimeout behavior is correct.
80-82:⚠️ Potential issue | 🟠 MajorSSE TLS ownership has the same virtual destructor issue.
sseTlsOwnerstores derived TLS objects viastd::unique_ptr<Client>, but Arduino'sClientbase class has no virtual destructor. When the unique_ptr destructs, it invokes undefined behavior. Use concrete types or a custom deleter similar to the fix needed fortlsOwnerinhttpsRequest().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` around lines 80 - 82, sseTlsOwner currently holds derived TLS client objects via std::unique_ptr<Client>, causing undefined behavior because Client lacks a virtual destructor; fix by making sseTlsOwner own the concrete TLS type or use a custom deleter like the httpsRequest() tlsOwner fix: replace std::unique_ptr<Client> sseTlsOwner with a std::unique_ptr<BearSSL::WiFiClientSecure> (or the exact concrete TLS wrapper class used elsewhere) or change it to std::unique_ptr<Client, std::function<void(Client*)>> and provide a deleter that safely deletes the concrete object; ensure sseClient still points to the concrete instance and adjust sseTransportOwner handling accordingly.
343-349:⚠️ Potential issue | 🟠 MajorBody offset is wrong for
\n\ndelimiter.When the delimiter is
\n\n(2 chars) instead of\r\n\r\n(4 chars), using+4will skip into the body content.🔧 Fix
int bodyStart = rawResponse.indexOf("\r\n\r\n"); - if (bodyStart < 0) bodyStart = rawResponse.indexOf("\n\n"); + int delimLen = 4; + if (bodyStart < 0) { + bodyStart = rawResponse.indexOf("\n\n"); + delimLen = 2; + } if (bodyStart < 0) { DEBUG_PRINTF("[%s] No HTTP body delimiter found in response\n", _name); return ""; } - return rawResponse.substring(bodyStart + 4); + return rawResponse.substring(bodyStart + delimLen);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` around lines 343 - 349, The code finds the HTTP body delimiter into rawResponse using either "\r\n\r\n" or "\n\n" but always adds 4 to bodyStart; change the logic so the substring offset matches the matched delimiter length: when you detect "\r\n\r\n" keep +4, and when you detect "\n\n" use +2. Update the bodyStart calculation (or introduce a delimiterLength variable) surrounding the rawResponse.indexOf checks and the final rawResponse.substring(bodyStart + ...) call so the correct number of bytes are skipped before returning the body.
498-510:⚠️ Potential issue | 🟠 Major
changedis set even whenbriwasn't actually modified.The block sets
changed = truewhenevernewBri != lastBri, even ifapplyOnOffandapplyBrileavebriuntouched. This triggers unnecessarycolorUpdated(CALL_MODE_DIRECT_CHANGE)calls.🔄 Suggested fix
bool changed = false; if (newBri != lastBri) { DEBUG_PRINTF("[%s] Brightness changed: %d -> %d\n", _name, lastBri, newBri); + byte prevBri = bri; if (applyOnOff) { if (newBri == 0) { bri = 0; } else if (bri == 0 && newBri > 0) { bri = briLast; } } if (applyBri && newBri > 0) { bri = newBri; } lastBri = newBri; - changed = true; + changed = (bri != prevBri); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` around lines 498 - 510, The code sets changed = true whenever newBri != lastBri even if the actual working brightness variable bri wasn't modified; fix by capturing the current bri (e.g. oldBri = bri) before applying applyOnOff/applyBri logic, run the existing adjustments (the block that may set bri and lastBri), then set changed = true only if bri != oldBri (and still update lastBri = newBri as before). Refer to variables bri, lastBri, newBri and the changed flag in the brightness-handling block to locate and apply the change.
1024-1030:⚠️ Potential issue | 🟠 MajorIncomplete JavaScript string escaping for light names.
Only escaping
'leaves backslashes, CR/LF, and</script>able to break the settings page or inject code. Bridge-controlled input should be fully escaped.🛡️ Suggested hardening
String safeName = String(name ? name : "?"); + safeName.replace("\\", "\\\\"); safeName.replace("'", "\\'"); + safeName.replace("\r", "\\r"); + safeName.replace("\n", "\\n"); + safeName.replace("</", "<\\/"); oappend(safeName.c_str());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` around lines 1024 - 1030, The current manual escaping only handles single quotes and leaves backslashes, CR/LF and sequences like </script> unescaped; implement and call a proper JavaScript/HTML-safe escaping routine (e.g., escapeJavaScriptString or escapeForHtmlScript) and use that before calling oappend for addOption(dd,...). Specifically, replace the inline safeName.replace(...) logic with a call that: replaces backslash with "\\\\", single quote with "\\'", normalizes CR and LF to "\\n" or "\\r", and neutralizes "</script>" (for example by replacing "</" with "<\\/") so that the safeName variable passed to oappend cannot break the page; update all uses of safeName/oappend/addOption accordingly.
238-239:⚠️ Potential issue | 🔴 CriticalUse concrete types for TLS ownership to ensure correct destructor calls.
std::unique_ptr<Client> tlsOwnerstores derived pointers (NetworkClientSecure*orBearSSLClient*), but Arduino'sClientbase class has no virtual destructor. Deleting via base pointer is undefined behavior. Store concrete types instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` around lines 238 - 239, The unique_ptrs currently declared as std::unique_ptr<Client> tlsOwner and std::unique_ptr<WiFiClient> transportOwner must hold concrete types so their destructors run correctly; change tlsOwner to hold the exact TLS wrapper type you construct (e.g., std::unique_ptr<NetworkClientSecure> or std::unique_ptr<BearSSLClient> depending on which is actually created) and change transportOwner to hold the concrete WiFi transport implementation you instantiate (the specific BearSSL/WiFiClient-derived class), then update the construction sites to std::make_unique<ConcreteType>(...) so deletion occurs via the proper concrete destructor rather than through a non-virtual base.
🧹 Nitpick comments (2)
usermods/PhilipsHueV2/PhilipsHueV2.cpp (2)
153-175:xyToRGBcould divide by zero ify == 0.If
yis zero (malformed Hue data), the divisions(Y / y)produce infinity/NaN, which then propagate through the RGB calculation. A guard would make this more robust.🛡️ Suggested guard
static void xyToRGB(float x, float y, byte* rgb) { + if (y <= 0.0f) { rgb[0] = rgb[1] = rgb[2] = 255; return; } // fallback to white float z = 1.0f - x - y;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` around lines 153 - 175, The xyToRGB function can divide by zero when y is 0; add a guard at the start of xyToRGB to detect y == 0 (or fabs(y) < small epsilon) and handle it by returning a safe fallback (e.g., set rgb[0..2] to 0 or a default color) or by replacing y with a small epsilon before computing X and Z; ensure this prevents the (Y / y) divisions from producing inf/NaN and keeps the rest of the function (the r,g,b calculations and clamping) unchanged.
103-121: Consider error handling for filesystem write failure.
saveLightsToFSdoesn't check ifserializeJsonsucceeded or iff.close()encountered an error. On a full filesystem, the user won't know the save failed.🛠️ Optional improvement
File f = WLED_FS.open(F("/hue_lights.json"), "w"); - if (f) { serializeJson(doc, f); f.close(); } + if (f) { + size_t written = serializeJson(doc, f); + f.close(); + if (written == 0) { + DEBUG_PRINTF("[%s] Failed to write /hue_lights.json\n", _name); + } + } else { + DEBUG_PRINTF("[%s] Failed to open /hue_lights.json for writing\n", _name); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` around lines 103 - 121, The saveLightsToFS function currently ignores write errors from serializeJson and f.close(); update saveLightsToFS to detect and handle filesystem write failures by checking serializeJson's return value (bytes written) and verifying that f.close() succeeded (or checking f.flush/error state if available), only set lightsDiscovered = (count > 0) when the write fully succeeds, and log an error via DEBUG_PRINTF including the file name and count on failure; refer to saveLightsToFS, the local File variable f, serializeJson(doc, f), f.close(), count, and lightsDiscovered when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp`:
- Line 572: ssePlainClient.setTimeout(2) is setting the SSE client timeout to 2
milliseconds; change the timeout to a proper millisecond value (e.g., 2000 for 2
seconds) so the SSE client isn't timing out immediately—update the call to
ssePlainClient.setTimeout(...) with the correct millisecond value.
- Around line 583-584: The SSE client's timeout is incorrectly set to 4
(milliseconds) like the httpsRequest path; locate the SSE TLS client setup where
tlsClient->setInsecure(); tlsClient->setTimeout(4); is called and change
setTimeout(4) to setTimeout(4000) so the SSE connection uses a 4-second timeout
(update the occurrence for the SSE client in PhilipsHueV2.cpp where tlsClient is
configured).
- Around line 280-281: The timeout passed to tcpClient.setTimeout is in
milliseconds, so calling tcpClient.setTimeout(2) sets a 2 ms timeout; update the
call in PhilipsHueV2.cpp (where tcpClient.setTimeout is used before assigning
client = &tcpClient) to the intended value in milliseconds (e.g.,
tcpClient.setTimeout(2000) for 2 seconds) or replace the magic number with a
named constant (e.g., DEFAULT_HTTP_TIMEOUT_MS) to make the units clear.
---
Duplicate comments:
In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp`:
- Line 976: The loader reads pollInterval via
getJsonValue(top[F("pollInterval")], pollInterval, 2500UL) but does not enforce
the documented 1000 ms minimum; after that call (in PhilipsHueV2.cpp where
pollInterval is set), clamp pollInterval to at least 1000UL (e.g., pollInterval
= max(pollInterval, 1000UL)) so malformed or malicious configs cannot set it
below the advertised minimum; keep configComplete handling as-is and only adjust
the value used by the polling logic.
- Line 367: The DEBUG_PRINTF call printing the full auth response
(DEBUG_PRINTF("[%s] Auth response: %s\n", _name, response.c_str())) leaks the
API key (success.username); replace this by logging a non-sensitive summary
instead: do not print the whole response string, either log only fixed metadata
(e.g., connection status or success flag) or parse the JSON in the auth response
and redact the success.username value (replace with "<REDACTED>" or omit the
field) before passing to DEBUG_PRINTF, ensuring the change targets the
DEBUG_PRINTF call and the variables _name and response used there.
- Around line 327-337: The response loop in PhilipsHueV2.cpp using
client->connected() / client->available() may exit while unread buffered bytes
remain and silently break on RESPONSE_BUF_SIZE overflow; modify the loop in the
method that builds rawResponse to after the connected() loop run a draining loop
that reads remaining client->available() bytes (using client->read() or
readStringUntil) into rawResponse until no data remains or RESPONSE_BUF_SIZE is
reached, and when RESPONSE_BUF_SIZE is exceeded set/return an explicit error or
truncation flag instead of silently breaking; ensure idleStart/millis() logic
remains for normal reads and preserve yield() calls while draining.
- Around line 249-250: The timeout passed to tlsClient->setTimeout(4) is in
milliseconds, causing Stream reads (e.g., readStringUntil('\n')) to be
truncated; change the call in PhilipsHueV2.cpp to use seconds-to-milliseconds
(e.g., setTimeout(4000) or setTimeout(4 * 1000UL)) so the TLS client has a
4-second timeout and Stream::setTimeout behavior is correct.
- Around line 80-82: sseTlsOwner currently holds derived TLS client objects via
std::unique_ptr<Client>, causing undefined behavior because Client lacks a
virtual destructor; fix by making sseTlsOwner own the concrete TLS type or use a
custom deleter like the httpsRequest() tlsOwner fix: replace
std::unique_ptr<Client> sseTlsOwner with a
std::unique_ptr<BearSSL::WiFiClientSecure> (or the exact concrete TLS wrapper
class used elsewhere) or change it to std::unique_ptr<Client,
std::function<void(Client*)>> and provide a deleter that safely deletes the
concrete object; ensure sseClient still points to the concrete instance and
adjust sseTransportOwner handling accordingly.
- Around line 343-349: The code finds the HTTP body delimiter into rawResponse
using either "\r\n\r\n" or "\n\n" but always adds 4 to bodyStart; change the
logic so the substring offset matches the matched delimiter length: when you
detect "\r\n\r\n" keep +4, and when you detect "\n\n" use +2. Update the
bodyStart calculation (or introduce a delimiterLength variable) surrounding the
rawResponse.indexOf checks and the final rawResponse.substring(bodyStart + ...)
call so the correct number of bytes are skipped before returning the body.
- Around line 498-510: The code sets changed = true whenever newBri != lastBri
even if the actual working brightness variable bri wasn't modified; fix by
capturing the current bri (e.g. oldBri = bri) before applying
applyOnOff/applyBri logic, run the existing adjustments (the block that may set
bri and lastBri), then set changed = true only if bri != oldBri (and still
update lastBri = newBri as before). Refer to variables bri, lastBri, newBri and
the changed flag in the brightness-handling block to locate and apply the
change.
- Around line 1024-1030: The current manual escaping only handles single quotes
and leaves backslashes, CR/LF and sequences like </script> unescaped; implement
and call a proper JavaScript/HTML-safe escaping routine (e.g.,
escapeJavaScriptString or escapeForHtmlScript) and use that before calling
oappend for addOption(dd,...). Specifically, replace the inline
safeName.replace(...) logic with a call that: replaces backslash with "\\\\",
single quote with "\\'", normalizes CR and LF to "\\n" or "\\r", and neutralizes
"</script>" (for example by replacing "</" with "<\\/") so that the safeName
variable passed to oappend cannot break the page; update all uses of
safeName/oappend/addOption accordingly.
- Around line 238-239: The unique_ptrs currently declared as
std::unique_ptr<Client> tlsOwner and std::unique_ptr<WiFiClient> transportOwner
must hold concrete types so their destructors run correctly; change tlsOwner to
hold the exact TLS wrapper type you construct (e.g.,
std::unique_ptr<NetworkClientSecure> or std::unique_ptr<BearSSLClient> depending
on which is actually created) and change transportOwner to hold the concrete
WiFi transport implementation you instantiate (the specific
BearSSL/WiFiClient-derived class), then update the construction sites to
std::make_unique<ConcreteType>(...) so deletion occurs via the proper concrete
destructor rather than through a non-virtual base.
In `@usermods/PhilipsHueV2/readme.md`:
- Around line 43-58: Add a clear security note to the TLS Backend section
stating that certificate validation is disabled because the code calls
setInsecure(), explain that this means the Hue bridge is not authenticated (only
encrypted), and explicitly warn that the hue-application-key is transmitted over
an unvalidated TLS channel and may be intercepted by a LAN MITM; place the note
near the existing TLS Backend text and mention setInsecure() and the trade-off
for compatibility with self-signed Hue certificates.
---
Nitpick comments:
In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp`:
- Around line 153-175: The xyToRGB function can divide by zero when y is 0; add
a guard at the start of xyToRGB to detect y == 0 (or fabs(y) < small epsilon)
and handle it by returning a safe fallback (e.g., set rgb[0..2] to 0 or a
default color) or by replacing y with a small epsilon before computing X and Z;
ensure this prevents the (Y / y) divisions from producing inf/NaN and keeps the
rest of the function (the r,g,b calculations and clamping) unchanged.
- Around line 103-121: The saveLightsToFS function currently ignores write
errors from serializeJson and f.close(); update saveLightsToFS to detect and
handle filesystem write failures by checking serializeJson's return value (bytes
written) and verifying that f.close() succeeded (or checking f.flush/error state
if available), only set lightsDiscovered = (count > 0) when the write fully
succeeds, and log an error via DEBUG_PRINTF including the file name and count on
failure; refer to saveLightsToFS, the local File variable f, serializeJson(doc,
f), f.close(), count, and lightsDiscovered when making these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 07deeb40-18aa-4226-94e9-70a21a429881
📒 Files selected for processing (2)
usermods/PhilipsHueV2/PhilipsHueV2.cppusermods/PhilipsHueV2/readme.md
Comment on lines
+280
to
+281
| tcpClient.setTimeout(2); | ||
| client = &tcpClient; |
Contributor
There was a problem hiding this comment.
Plain HTTP timeout is also in milliseconds.
tcpClient.setTimeout(2) sets a 2 ms timeout, not 2 seconds.
🔧 Fix
- tcpClient.setTimeout(2);
+ tcpClient.setTimeout(2000); // 2 second timeout🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` around lines 280 - 281, The timeout
passed to tcpClient.setTimeout is in milliseconds, so calling
tcpClient.setTimeout(2) sets a 2 ms timeout; update the call in PhilipsHueV2.cpp
(where tcpClient.setTimeout is used before assigning client = &tcpClient) to the
intended value in milliseconds (e.g., tcpClient.setTimeout(2000) for 2 seconds)
or replace the magic number with a named constant (e.g.,
DEFAULT_HTTP_TIMEOUT_MS) to make the units clear.
|
|
||
| // --- create the appropriate client --- | ||
| if (usePlainHttp) { | ||
| ssePlainClient.setTimeout(2); |
Contributor
There was a problem hiding this comment.
SSE plain client timeout is 2 ms.
Same milliseconds issue.
🔧 Fix
- ssePlainClient.setTimeout(2);
+ ssePlainClient.setTimeout(2000);📝 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.
Suggested change
| ssePlainClient.setTimeout(2); | |
| ssePlainClient.setTimeout(2000); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` at line 572,
ssePlainClient.setTimeout(2) is setting the SSE client timeout to 2
milliseconds; change the timeout to a proper millisecond value (e.g., 2000 for 2
seconds) so the SSE client isn't timing out immediately—update the call to
ssePlainClient.setTimeout(...) with the correct millisecond value.
Comment on lines
+583
to
+584
| tlsClient->setInsecure(); | ||
| tlsClient->setTimeout(4); |
Contributor
There was a problem hiding this comment.
SSE client also has the 4 ms timeout issue.
Same as the httpsRequest path — setTimeout(4) should be setTimeout(4000).
🔧 Fix
- tlsClient->setTimeout(4);
+ tlsClient->setTimeout(4000);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` around lines 583 - 584, The SSE
client's timeout is incorrectly set to 4 (milliseconds) like the httpsRequest
path; locate the SSE TLS client setup where tlsClient->setInsecure();
tlsClient->setTimeout(4); is called and change setTimeout(4) to setTimeout(4000)
so the SSE connection uses a 4-second timeout (update the occurrence for the SSE
client in PhilipsHueV2.cpp where tlsClient is configured).
…tial timeout, adjusting idle timeout, and ensuring complete data read.
…havior, implement configurable auto-reconnect, and enhance error logging.
…racy, ensure proper scaling, and apply gamma correction.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (9)
usermods/PhilipsHueV2/PhilipsHueV2.cpp (9)
295-295:⚠️ Potential issue | 🟠 MajorTimeout is 4 milliseconds, not 4 seconds.
Stream::setTimeout()takes milliseconds. This will cause reads to timeout almost immediately.🔧 Fix
- tlsClient->setTimeout(4); + tlsClient->setTimeout(4000); // 4 second timeout🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` at line 295, The timeout passed to Stream::setTimeout is in milliseconds, but tlsClient->setTimeout(4) mistakenly sets 4ms; change the call in PhilipsHueV2.cpp (tlsClient->setTimeout) to a reasonable seconds-to-milliseconds value (e.g., 4000 for 4 seconds) or compute from a constant (e.g., TIMEOUT_SECONDS * 1000) so reads don't immediately time out.
1043-1043:⚠️ Potential issue | 🟡 MinorClamp
pollIntervalto documented minimum.
appendConfigData()advertises "min 1000" but this loader accepts any value. A bad config could turnloop()into back-to-back TLS polls.🔧 Fix
configComplete &= getJsonValue(top[F("pollInterval")], pollInterval, 2500UL); + if (pollInterval < 1000UL) pollInterval = 1000UL;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` at line 1043, The loader reads pollInterval via getJsonValue (configComplete &= getJsonValue(top[F("pollInterval")], pollInterval, 2500UL)) but does not enforce the advertised minimum; after reading into pollInterval (in the same loader function where configComplete and pollInterval are used) clamp it to at least 1000UL (e.g., set pollInterval = max(pollInterval, 1000UL) or conditional if <1000UL then =1000UL) so loop() cannot be driven into back-to-back TLS polls; mention appendConfigData and pollInterval in the change for clarity.
636-636:⚠️ Potential issue | 🟠 MajorSSE TLS client timeout is 4 milliseconds.
🔧 Fix
- tlsClient->setTimeout(4); + tlsClient->setTimeout(4000);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` at line 636, The TLS SSE client timeout is currently set to 4 milliseconds via tlsClient->setTimeout(4); — change this to a sensible timeout (e.g., 4000 for 4 seconds or a higher value like 10000 for 10 seconds) after verifying the units expected by TLSClient::setTimeout, or use the API's seconds-based setter if available; update the call to tlsClient->setTimeout(...) with the correct units/value so the SSE client isn't timing out almost immediately.
624-624:⚠️ Potential issue | 🟠 MajorSSE plain client timeout is 2 milliseconds.
🔧 Fix
- ssePlainClient.setTimeout(2); + ssePlainClient.setTimeout(2000);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` at line 624, The call ssePlainClient.setTimeout(2) uses an excessively small timeout (2 milliseconds); update the ssePlainClient.setTimeout invocation to a realistic timeout (for example 2000 for 2 seconds) or the correct units API expects so SSE connections aren't immediately dropped—locate the setTimeout call in PhilipsHueV2.cpp (ssePlainClient.setTimeout) and replace the value with an appropriate duration (e.g., 2000) or use the library's seconds-based setter if available.
419-419:⚠️ Potential issue | 🟠 MajorAuth response contains sensitive API key.
The full response is logged, which includes the
success.usernamefield that becomes the API key. This leaks credentials to debug output.🔧 Redact sensitive data
- DEBUG_PRINTF("[%s] Auth response: %s\n", _name, response.c_str()); + DEBUG_PRINTF("[%s] Auth response received (%u bytes)\n", _name, response.length());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` at line 419, The debug log prints the full auth response including the sensitive success.username API key; update the DEBUG_PRINTF that uses _name and response.c_str() (the auth-response log in PhilipsHueV2.cpp) to avoid leaking credentials by either parsing the JSON and removing or masking the success.username field before logging, or by logging only a non-sensitive summary (e.g., status/keys-present boolean) instead of the raw response; ensure the change targets the DEBUG_PRINTF call that currently prints response.c_str() so the API key is never emitted to logs.
395-401:⚠️ Potential issue | 🟡 MinorBody offset incorrect for
\n\ndelimiter.When the body delimiter is
\n\n(2 chars) instead of\r\n\r\n(4 chars), using+4will skip into the body content.🔧 Fix
int bodyStart = rawResponse.indexOf("\r\n\r\n"); - if (bodyStart < 0) bodyStart = rawResponse.indexOf("\n\n"); + int offset = 4; + if (bodyStart < 0) { + bodyStart = rawResponse.indexOf("\n\n"); + offset = 2; + } if (bodyStart < 0) { DEBUG_PRINTF("[%s] No HTTP body delimiter found in response\n", _name); return ""; } - return rawResponse.substring(bodyStart + 4); + return rawResponse.substring(bodyStart + offset);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` around lines 395 - 401, The code sets bodyStart by searching for "\r\n\r\n" then "\n\n" but always adds 4 when returning the body; update the logic in the function that uses rawResponse and bodyStart to track which delimiter matched (e.g., store delimLen = 4 for "\r\n\r\n" or delimLen = 2 for "\n\n") and return rawResponse.substring(bodyStart + delimLen) instead of always +4; reference the existing variables bodyStart and rawResponse and keep the existing debug path using DEBUG_PRINTF and _name.
1094-1096:⚠️ Potential issue | 🟠 MajorIncomplete JavaScript string escaping.
Only single quotes are escaped. Backslashes, newlines, and
</script>sequences in light names could break the settings page or enable injection.🔧 Proper escaping
String safeName = String(name ? name : "?"); + safeName.replace("\\", "\\\\"); safeName.replace("'", "\\'"); + safeName.replace("\r", "\\r"); + safeName.replace("\n", "\\n"); + safeName.replace("</", "<\\/"); oappend(safeName.c_str());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` around lines 1094 - 1096, The current escaping only handles single quotes for safeName before passing to oappend, which leaves backslashes, newlines, CRs and the "</script>" sequence unescaped and can break JS or allow injection; update the sanitization for String safeName (the variable built from name) to escape backslashes first, then single quotes, newlines (\n), carriage returns (\r) (and optionally tabs), and replace any "</script>" with "<\\/script>" before calling oappend(safeName.c_str()) so the resulting JS single-quoted string is safe.
325-325:⚠️ Potential issue | 🟠 MajorPlain HTTP timeout is 2 milliseconds.
Same milliseconds issue as the TLS path.
🔧 Fix
- tcpClient.setTimeout(2); + tcpClient.setTimeout(2000); // 2 second timeout🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` at line 325, tcpClient.setTimeout is currently being passed 2 which is interpreted as 2 milliseconds (too short); update the plain-HTTP timeout to match the TLS path by passing a millisecond value (e.g., 2000) or use a shared constant like HTTP_TIMEOUT_MS, i.e., replace tcpClient.setTimeout(2) with tcpClient.setTimeout(2000) or the shared timeout constant so both TLS and plain HTTP use the same sensible timeout.
81-82:⚠️ Potential issue | 🟠 Major
std::unique_ptr<Client>causes undefined behavior on destruction.Same issue as flagged for the local
tlsOwnerinhttpsRequest(): Arduino'sClientbase class has no virtual destructor, so deleting derived types (NetworkClientSecure,BearSSLClient) throughunique_ptr<Client>invokes undefined behavior. The derived destructor (which frees TLS context) is never called.Consider storing the concrete type or using a custom deleter that knows the actual type.
🔧 Suggested fix
- std::unique_ptr<Client> sseTlsOwner; // owns heap-allocated TLS wrapper +#ifdef HUE_TLS_BACKEND_NATIVE + `#ifdef` HUE_NATIVE_IS_NETWORK_CLIENT + std::unique_ptr<NetworkClientSecure> sseTlsOwner; + `#else` + std::unique_ptr<WiFiClientSecure> sseTlsOwner; + `#endif` +#elif defined(HUE_TLS_BACKEND_BEARSSL) + std::unique_ptr<BearSSLClient> sseTlsOwner; +#else + std::unique_ptr<Client> sseTlsOwner; // fallback (no TLS) +#endifThen update
sseConnect()to assignsseClient = sseTlsOwner.get()after.reset().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` around lines 81 - 82, sseTlsOwner and sseTransportOwner are declared as std::unique_ptr<Client> which causes undefined behavior because Client lacks a virtual destructor; change these owners to hold the concrete types (e.g., std::unique_ptr<NetworkClientSecure> or std::unique_ptr<BearSSLClient>/std::unique_ptr<WiFiClient>) or use unique_ptr<Client> with a custom deleter that calls the correct destructor, then in sseConnect() assign the raw pointer sseClient = sseTlsOwner.get() (mirroring the fix used for tlsOwner in httpsRequest()) so the derived destructor runs correctly when the owner resets.
🧹 Nitpick comments (1)
usermods/PhilipsHueV2/PhilipsHueV2.cpp (1)
270-270: Non-standard HTTP port 8088.Plain HTTP falls back to port 8088 instead of standard port 80. If this is for debugging with a local proxy, consider making it configurable or documenting the expectation. For production use with a real Hue bridge, port 80 would be more appropriate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp` at line 270, The code sets uint16_t port = usePlainHttp ? 8088 : 443 which uses a non‑standard HTTP port; change the behavior so plain HTTP defaults to port 80 instead of 8088 (or make the port configurable via a setting/constructor parameter) and update any comments/docs to explain when a non‑standard port like 8088 is used (e.g., for local proxy/debugging). Locate the assignment to port and modify the ternary or surrounding initialization logic to use 80 for usePlainHttp == true (or read the value from a configuration variable), and add a brief comment noting why a non‑standard port would ever be chosen.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp`:
- Line 295: The timeout passed to Stream::setTimeout is in milliseconds, but
tlsClient->setTimeout(4) mistakenly sets 4ms; change the call in
PhilipsHueV2.cpp (tlsClient->setTimeout) to a reasonable seconds-to-milliseconds
value (e.g., 4000 for 4 seconds) or compute from a constant (e.g.,
TIMEOUT_SECONDS * 1000) so reads don't immediately time out.
- Line 1043: The loader reads pollInterval via getJsonValue (configComplete &=
getJsonValue(top[F("pollInterval")], pollInterval, 2500UL)) but does not enforce
the advertised minimum; after reading into pollInterval (in the same loader
function where configComplete and pollInterval are used) clamp it to at least
1000UL (e.g., set pollInterval = max(pollInterval, 1000UL) or conditional if
<1000UL then =1000UL) so loop() cannot be driven into back-to-back TLS polls;
mention appendConfigData and pollInterval in the change for clarity.
- Line 636: The TLS SSE client timeout is currently set to 4 milliseconds via
tlsClient->setTimeout(4); — change this to a sensible timeout (e.g., 4000 for 4
seconds or a higher value like 10000 for 10 seconds) after verifying the units
expected by TLSClient::setTimeout, or use the API's seconds-based setter if
available; update the call to tlsClient->setTimeout(...) with the correct
units/value so the SSE client isn't timing out almost immediately.
- Line 624: The call ssePlainClient.setTimeout(2) uses an excessively small
timeout (2 milliseconds); update the ssePlainClient.setTimeout invocation to a
realistic timeout (for example 2000 for 2 seconds) or the correct units API
expects so SSE connections aren't immediately dropped—locate the setTimeout call
in PhilipsHueV2.cpp (ssePlainClient.setTimeout) and replace the value with an
appropriate duration (e.g., 2000) or use the library's seconds-based setter if
available.
- Line 419: The debug log prints the full auth response including the sensitive
success.username API key; update the DEBUG_PRINTF that uses _name and
response.c_str() (the auth-response log in PhilipsHueV2.cpp) to avoid leaking
credentials by either parsing the JSON and removing or masking the
success.username field before logging, or by logging only a non-sensitive
summary (e.g., status/keys-present boolean) instead of the raw response; ensure
the change targets the DEBUG_PRINTF call that currently prints response.c_str()
so the API key is never emitted to logs.
- Around line 395-401: The code sets bodyStart by searching for "\r\n\r\n" then
"\n\n" but always adds 4 when returning the body; update the logic in the
function that uses rawResponse and bodyStart to track which delimiter matched
(e.g., store delimLen = 4 for "\r\n\r\n" or delimLen = 2 for "\n\n") and return
rawResponse.substring(bodyStart + delimLen) instead of always +4; reference the
existing variables bodyStart and rawResponse and keep the existing debug path
using DEBUG_PRINTF and _name.
- Around line 1094-1096: The current escaping only handles single quotes for
safeName before passing to oappend, which leaves backslashes, newlines, CRs and
the "</script>" sequence unescaped and can break JS or allow injection; update
the sanitization for String safeName (the variable built from name) to escape
backslashes first, then single quotes, newlines (\n), carriage returns (\r) (and
optionally tabs), and replace any "</script>" with "<\\/script>" before calling
oappend(safeName.c_str()) so the resulting JS single-quoted string is safe.
- Line 325: tcpClient.setTimeout is currently being passed 2 which is
interpreted as 2 milliseconds (too short); update the plain-HTTP timeout to
match the TLS path by passing a millisecond value (e.g., 2000) or use a shared
constant like HTTP_TIMEOUT_MS, i.e., replace tcpClient.setTimeout(2) with
tcpClient.setTimeout(2000) or the shared timeout constant so both TLS and plain
HTTP use the same sensible timeout.
- Around line 81-82: sseTlsOwner and sseTransportOwner are declared as
std::unique_ptr<Client> which causes undefined behavior because Client lacks a
virtual destructor; change these owners to hold the concrete types (e.g.,
std::unique_ptr<NetworkClientSecure> or
std::unique_ptr<BearSSLClient>/std::unique_ptr<WiFiClient>) or use
unique_ptr<Client> with a custom deleter that calls the correct destructor, then
in sseConnect() assign the raw pointer sseClient = sseTlsOwner.get() (mirroring
the fix used for tlsOwner in httpsRequest()) so the derived destructor runs
correctly when the owner resets.
---
Nitpick comments:
In `@usermods/PhilipsHueV2/PhilipsHueV2.cpp`:
- Line 270: The code sets uint16_t port = usePlainHttp ? 8088 : 443 which uses a
non‑standard HTTP port; change the behavior so plain HTTP defaults to port 80
instead of 8088 (or make the port configurable via a setting/constructor
parameter) and update any comments/docs to explain when a non‑standard port like
8088 is used (e.g., for local proxy/debugging). Locate the assignment to port
and modify the ternary or surrounding initialization logic to use 80 for
usePlainHttp == true (or read the value from a configuration variable), and add
a brief comment noting why a non‑standard port would ever be chosen.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9ca4564c-2768-47d8-b57f-8dcdbf9711e4
📒 Files selected for processing (1)
usermods/PhilipsHueV2/PhilipsHueV2.cpp
…quivalents, deferred effect application, and updated documentation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Chores