prevent /upload from overwriting wsec.json#5482
Open
Conversation
Contributor
WalkthroughAdds a new security flag to block uploading Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/wled_server.cpp`:
- Around line 207-210: The wsec.json filename check must execute for every
upload chunk (not only when index==0) to prevent multi-chunk bypass; move the
finalname.indexOf(FPSTR(s_wsec)) check out of the if (!index) block so it runs
at the top of the upload handler, mirror the correctPIN pattern (check on every
chunk and only call request->send(...) when isFinal is true), ensure you do not
open or write to _tempFile if the check denies access, and only send a single
HTTP response (403 or 200) tied to isFinal to avoid double responses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f3b3093e-a969-4677-91bf-17f7dfb1b395
📒 Files selected for processing (1)
wled00/wled_server.cpp
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
wled00/wled_server.cpp (1)
201-206: Follow theisFinalpattern to avoid sending multiple responses for multi-chunk uploads.The check correctly blocks wsec.json uploads, but unlike the
correctPINpattern at lines 197-199, this sends a 403 response on every chunk of a multi-chunk upload rather than only onisFinal. While the security goal is achieved (early return prevents file writes), aligning with the established pattern avoids callingrequest->send()multiple times for the same request.♻️ Proposed fix to align with the correctPIN pattern
if (filename.indexOf(FPSTR(s_wsec)) >= 0) { - request->send(403, FPSTR(CONTENT_TYPE_PLAIN), FPSTR(s_accessdenied)); // skip wsec.json + if (isFinal) request->send(403, FPSTR(CONTENT_TYPE_PLAIN), FPSTR(s_accessdenied)); // block wsec.json return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/wled_server.cpp` around lines 201 - 206, The wsec.json upload check currently calls request->send(403, ...) on every chunk; change it to follow the existing isFinal/correctPIN pattern by only invoking request->send and returning when the upload is the final chunk (isFinal/UPLOAD_FILE_END) so intermediate chunks are skipped but processing still exits early for wsec.json—locate the block that checks filename.indexOf(FPSTR(s_wsec)) and wrap the request->send(403, FPSTR(CONTENT_TYPE_PLAIN), FPSTR(s_accessdenied)) and return behind the same isFinal / upload->status final-condition used in the correctPIN handling to prevent multiple responses.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@wled00/wled_server.cpp`:
- Around line 201-206: The wsec.json upload check currently calls
request->send(403, ...) on every chunk; change it to follow the existing
isFinal/correctPIN pattern by only invoking request->send and returning when the
upload is the final chunk (isFinal/UPLOAD_FILE_END) so intermediate chunks are
skipped but processing still exits early for wsec.json—locate the block that
checks filename.indexOf(FPSTR(s_wsec)) and wrap the request->send(403,
FPSTR(CONTENT_TYPE_PLAIN), FPSTR(s_accessdenied)) and return behind the same
isFinal / upload->status final-condition used in the correctPIN handling to
prevent multiple responses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1422f5e5-46a8-4633-9a95-eacfb1ad35cc
📒 Files selected for processing (1)
wled00/wled_server.cpp
Author
|
The latest commit checks |
Collaborator
|
thanks, generally sounds good, however I am not sure it is not a breaking change: there may be users actually using this upload to update the password after a flash wipe. @willmmiles or @softhack007 may know more about it. |
Member
|
@DedeHai @FoamyGuy Maybe the wsec.json upload blocker should either have its own settings item (security settings) to enable it, or just block uploading it when other protective settings - like PIN protection - are active. Edit:
This concern is equally true for cfg.json, but we still allow uploading. If valid JSON with unexpected tags is the real concern, we should address that when loading/parsing the file, not during upload. |
softhack007
added
bug
discussion
connectivity
Member
|
blocking wsec.json upload might still be a good addition for more security - but the blocker should be user-configurable, to avoid breaking existing installations. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/wled.h`:
- Line 579: The global flag denyWsecUpload is currently initialized to false
which leaves wsec.json overwrite allowed by default; change its initialization
in wled.h (symbol: denyWsecUpload) to true to make upload-overwrite protection
enabled by default, and ensure any code that checks denyWsecUpload (e.g., the
POST /upload handler) treats true as denying overwrites; update any related
comments or documentation to reflect the new default behavior so users can opt
out if desired.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3cf41d0d-294f-41f7-804c-ec511ee15501
📒 Files selected for processing (6)
wled00/cfg.cppwled00/data/settings_sec.htmwled00/set.cppwled00/wled.hwled00/wled_server.cppwled00/xml.cpp
✅ Files skipped from review due to trivial changes (1)
- wled00/wled_server.cpp
| WLED_GLOBAL bool otaLock _INIT(false); // prevents OTA firmware updates without password. ALWAYS enable if system exposed to any public networks | ||
| #endif | ||
| WLED_GLOBAL bool wifiLock _INIT(false); // prevents access to WiFi settings when OTA lock is enabled | ||
| WLED_GLOBAL bool denyWsecUpload _INIT(false); // when true, POST /upload refuses to overwrite wsec.json |
Contributor
There was a problem hiding this comment.
Make denyWsecUpload secure-by-default.
Initializing this to false keeps wsec.json upload overwrite allowed until users manually enable it, so the protection is not enforced by default. Set default to true and let users opt out if needed.
🔒 Proposed change
-WLED_GLOBAL bool denyWsecUpload _INIT(false); // when true, POST /upload refuses to overwrite wsec.json
+WLED_GLOBAL bool denyWsecUpload _INIT(true); // when true, POST /upload refuses to overwrite wsec.json📝 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
| WLED_GLOBAL bool denyWsecUpload _INIT(false); // when true, POST /upload refuses to overwrite wsec.json | |
| WLED_GLOBAL bool denyWsecUpload _INIT(true); // when true, POST /upload refuses to overwrite wsec.json |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/wled.h` at line 579, The global flag denyWsecUpload is currently
initialized to false which leaves wsec.json overwrite allowed by default; change
its initialization in wled.h (symbol: denyWsecUpload) to true to make
upload-overwrite protection enabled by default, and ensure any code that checks
denyWsecUpload (e.g., the POST /upload handler) treats true as denying
overwrites; update any related comments or documentation to reflect the new
default behavior so users can opt out if desired.
Member
There was a problem hiding this comment.
@coderabbitai i am not sure if true or false is the better default here. Please create an FMEA - feared event is "loss of security settings" and "bootloop/bricked device". Consider consequences for existing setups (upgrade path) and fresh setups. Also assess if storing the flag in cfg.json or in wsec.json makes a difference.
Contributor
There was a problem hiding this comment.
@softhack007, here is the requested FMEA. The two feared top-level events are FE-1: Loss of security settings and FE-2: Bootloop/bricked device.
FMEA: denyWsecUpload Default Value
Failure Mode Table
| # | Scenario | Failure Mode | Default = false |
Default = true |
Severity |
|---|---|---|---|---|---|
| 1 | Attacker or script POSTs malformed wsec.json | Credentials overwritten / device bootloops | Allowed – FE-1 + FE-2 possible | Blocked – both events prevented | Critical |
| 2 | User uploads wsec.json to recover after flash wipe / credential loss | Intended recovery action | Works – upload succeeds | Blocked – recovery fails silently with HTTP 403 | High |
| 3 | wsec.json is corrupted / deleted on-device | Flag itself is lost; reverts to _INIT |
Falls back to false → upload re-enabled → recovery path exists |
Falls back to true → upload still blocked → serial recovery required |
Critical |
| 4 | cfg.json exists but wsec.json does not (partial wipe) | Flag read from cfg.json only if pwdCorrect |
Same as #3 |
Same as #3, but cfg.json value may survive if cfg.json intact |
Medium |
| 5 | Firmware upgrade on existing device (neither file has deny-wsec key) |
Key absent → _INIT default used |
No behaviour change – upload still allowed | Silent breaking change – upload now blocked on all existing installs without user action | High |
| 6 | Fresh install, no cfg.json, no wsec.json | _INIT default governs from first boot |
Upload allowed; user can provision wsec via /upload | Upload blocked; wsec.json can only be set via the web UI | Low–Medium |
The Critical Circular-Dependency Risk
Because denyWsecUpload is stored inside wsec.json (per the serializeConfigSec/deserializeConfigSec changes in this PR), the flag and the file it guards share the same fate:
wsec.json corrupted / deleted
│
▼
denyWsecUpload resets to _INIT
│
├─ _INIT(false) → upload re-enabled → user can POST a clean wsec.json → RECOVERY POSSIBLE
│
└─ _INIT(true) → upload still blocked → user cannot POST wsec.json → SERIAL FLASH REQUIRED
_INIT(true) directly worsens FE-2 (bricked device) in the scenario where wsec.json is the corrupted file. This is particularly relevant because the very threat this PR is defending against (a malformed wsec.json upload) can leave the device in exactly that state.
cfg.json vs wsec.json Storage — Does It Matter?
| Storage location | Consequence |
|---|---|
| wsec.json only | Flag shares fate with the file it guards. If wsec.json is gone, _INIT governs. |
| cfg.json only | Flag survives a wsec.json wipe. But cfg.json is not PIN-protected at the file level, and is itself uploadable via /upload. |
| Both (current PR) | cfg.json value applies when pwdCorrect; wsec.json value loaded in deserializeConfigSec. If wsec.json is missing, cfg.json may still carry the value — but only on an authenticated session. On first boot after a partial wipe, the deserialization order determines which "wins". |
Storing the flag in cfg.json is safer for recovery because cfg.json can survive an independent wsec.json corruption. The current dual-storage in the PR is a reasonable middle ground, but it still leaves the circular-dependency risk for a full wipe.
Upgrade-Path Assessment
| Default | Existing installs (upgrade) | Fresh installs |
|---|---|---|
false |
No regression — behaviour identical to pre-PR firmware; users opt in consciously | Upload allowed; consistent with historical WLED behaviour |
true |
Silent breaking change — all existing devices lose the upload-recovery path the moment they upgrade, with no UI warning | Upload blocked from first boot; stronger security posture but removes a documented recovery workflow |
Recommendation
Based on this FMEA, _INIT(false) is the safer default primarily because:
- Circular dependency —
_INIT(true)turns a corrupted/deletedwsec.jsoninto a non-recoverable situation without serial access. - Upgrade safety —
_INIT(false)preserves existing behaviour for all current installations; users who want the protection actively enable it. - Recovery parity with softhack007's argument — uploading
wsec.jsondoes not expose the previous secrets; it replaces them, so it is a valid recovery mechanism.
The real defence against malformed-JSON bootloops should be input validation at load time (as softhack007 noted applies equally to cfg.json), not upload blocking. The denyWsecUpload flag is still valuable as an optional hardening measure for installations that do not need the upload-recovery path, but should default to false and be documented as an advanced security option.
🧠 Learnings used
Learnt from: blazoncek
Repo: wled/WLED PR: 4700
File: wled00/wled_server.cpp:409-414
Timestamp: 2025-05-26T16:09:34.325Z
Learning: In wled00/wled_server.cpp, the OTA update access control logic intentionally allows OTA updates from different subnets when otaSameSubnet is true AND a PIN is set. This was a conscious design decision by blazoncek to provide flexibility for remote OTA updates with PIN protection, though they acknowledged it may not be the optimal security model.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/set.cpp:551-555
Timestamp: 2026-03-27T21:00:25.902Z
Learning: In WLED PR `#5048` (wled00/set.cpp lines 551-555), the CONFIG_IDF_TARGET_ESP32C5 block that unconditionally forces ntpEnabled = false is an intentional **temporary hotfix** by softhack007 for a known ESP32-C5 crash: `assert failed: udp_new_ip_type udp.c:1278 (Required to lock TCPIP core functionality!)`. Do not flag this as a permanent design issue; the TODO comment in the code already notes it should be resolved properly once the underlying IDF/TCPIP bug on C5 is fixed. A future permanent solution should use a target capability flag rather than overwriting the user's setting.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/wled.cpp:1037-1051
Timestamp: 2026-03-29T16:14:17.321Z
Learning: In WLED PR `#5048` (wled00/wled.cpp, CONFIG_IDF_TARGET_ESP32P4 block): `ESP_HostedOTA.h` and `updateEspHostedSlave()` are NOT in the git repository. They are provided by the pioarduino ESP32-P4 platform package (platform-espressif32.zip, referenced in platformio.ini [esp32p4] section). The header and function are part of the ESP-Hosted Wi-Fi co-processor framework integration bundled within the pioarduino framework for ESP32-P4, not a separate Arduino library. Do NOT flag `#include "ESP_HostedOTA.h"` or calls to `updateEspHostedSlave()` as missing/undefined for ESP32-P4 builds.
Learnt from: willmmiles
Repo: wled/WLED PR: 4998
File: wled00/ota_update.cpp:120-124
Timestamp: 2025-10-11T17:56:24.923Z
Learning: In the WLED OTA update implementation (wled00/ota_update.cpp), the parameter name for bypassing OTA validation is "skipValidation", not "ignoreRelease". The intent evolved from the original spec in issue `#4929` to disable all validation checks, not just release name checks, which is why the broader parameter name "skipValidation" is used.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/pin_manager.cpp:235-270
Timestamp: 2026-03-27T17:17:19.350Z
Learning: ESP32-C61 pin restrictions for isPinOk() in WLED (wled00/pin_manager.cpp), confirmed from official Espressif ESP-IDF GPIO docs and C61 datasheet:
- Strapping pins: MTMS, MTDI, GPIO7, GPIO8, GPIO9
- SPI flash/PSRAM reserved: GPIO14–GPIO17 AND GPIO19–GPIO21 (two non-contiguous ranges; GPIO18 is NOT reserved)
(SPICS1=14, SPICS0=15, SPIQ=16, SPIWP=17, SPIHD=19, SPICLK=20, SPID=21)
- USB-JTAG (same as C6): GPIO12 (D-) and GPIO13 (D+); gate under ARDUINO_USB_CDC_ON_BOOT==1 || ARDUINO_USB_DFU_ON_BOOT==1
- C61 is NOT a simple C6 GPIO clone: its SPI flash range is GPIO14-21 (with gap at 18), NOT GPIO24-30 like C6.
- Suggested isPinOk() guards: `if (gpio > 13 && gpio < 18) return false;` and `if (gpio > 18 && gpio < 22) return false;`
Learnt from: netmindz
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-03-20T15:29:26.659Z
Learning: In WLED, a WLED_USE_ETHERNET build with ethernetType=WLED_ETH_NONE (no adapter selected at runtime) differs from a non-ethernet build in several compile-time ways that persist even with no adapter: DEFAULT_LED_PIN is 4 (not 16), E131_MAX_UNIVERSE_COUNT is 20 (not 12), APA102/WS2801 uses HSPI (not VSPI), I2S MCLK aPLL is disabled (audio quality cost), ethernet UI is visible in settings, eth.type=0 is written to cfg.json, and net_debug.cpp uses ETH.hostByName() (which will fail since ETH is never initialized). The runtime-gated behaviors (pin reservation, ETH.begin(), WiFi disconnect, MAC/IP preference) are correctly skipped when ethernetType==WLED_ETH_NONE.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/cfg.cpp:673-673
Timestamp: 2026-03-27T21:17:51.985Z
Learning: In WLED PR `#5048` (wled00/cfg.cpp), the NTP crash on ESP32-C5 (pioarduino + IDF 5.5.x) is caused by both CONFIG_LWIP_TCPIP_CORE_LOCKING=y AND CONFIG_LWIP_CHECK_THREAD_SAFETY=y being enabled by default in the pioarduino/arduino-esp32 IDF 5.x sdkconfig. The tasmota core (IDF 5.3.x, used for C6) likely has CONFIG_LWIP_CHECK_THREAD_SAFETY=n, which is why the same violation does not crash on C6. Since pioarduino already has TCPIP_CORE_LOCKING enabled, wrapping ntpUdp.begin() (and any other raw lwIP API call) with LOCK_TCPIP_CORE()/UNLOCK_TCPIP_CORE() from lwip/tcpip.h is directly applicable without sdkconfig changes in platformio.ini.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/pin_manager.cpp:235-270
Timestamp: 2026-03-27T17:17:19.350Z
Learning: ESP32-C61 pin restrictions for isPinOk() in WLED (wled00/pin_manager.cpp):
- Strapping pins: GPIO7, GPIO8, GPIO9
- SPI flash reserved (same as C6): GPIO24 ~ GPIO30 (inferred from C6 hardware relationship; no dedicated C61 ESP-IDF GPIO docs page exists yet)
- USB-JTAG (confirmed by ESP32-C61 hardware design guidelines): GPIO12 (D-) and GPIO13 (D+)
- The C61 is a C6-derivative with Zigbee/802.15.4 radio removed; GPIO layout mirrors C6.
- Suggested CONFIG_IDF_TARGET_ESP32C61 branch should block gpio > 23 && gpio < 31 (SPI flash), and gpio == 12 || gpio == 13 under ARDUINO_USB_CDC_ON_BOOT/DFU_ON_BOOT guard.
Learnt from: netmindz
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-03-20T15:29:26.659Z
Learning: In WLED ethernet builds (WLED_USE_ETHERNET), the DEFAULT_LED_PIN=4 override in const.h:661 is an intentional safety measure that applies at compile-time regardless of the runtime ethernetType value. It protects against first-boot cfg.json writes saving GPIO16 as the LED pin on ethernet boards where GPIO16 is hard-wired to ethernet clock output (ETH_CLOCK_GPIO16_OUT), which causes crashes. It also protects custom build environments that forget to set DATA_PINS explicitly. This was confirmed by softhack007 in PR `#5432`.
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-08T19:07:13.970Z
Learning: Applies to wled00/const.h : New usermod IDs should be added into `wled00/const.h`
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-03-08T00:57:36.134Z
Learning: In WLED (wled00/cfg.cpp), `deserializeConfigFromFS()` only sets `needsSave=true` (triggering a write of cfg.json) when usermod settings are present in the JSON (lines 758-761). On a fresh install with no usermods, `needsSave=false` so `serializeConfigToFS()` is never called at first boot. Correct defaults are never written to cfg.json until the user manually saves. Structural fix: set `needsSave = needsSave || !cfgExists` where `cfgExists = WLED_FS.exists(s_cfg_json)` checked before reading the file.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-03-08T00:57:36.134Z
Learning: In WLED (wled00/cfg.cpp), `deserializeConfig()` is called with an empty JsonObject when cfg.json doesn't exist on fresh install. Any value read without the ArduinoJSON `|` fallback operator or CJSON macro will override correct constructor defaults with 0/null. Known affected values: `strip.setTargetFps(hw_led["fps"])` at line 179 (sets FPS to 0/unlimited instead of WLED_FPS=42). Fix: `strip.setTargetFps(hw_led["fps"] | WLED_FPS)`. The gamma issue (gammaCorrectCol/Bri) had the same root cause and was fixed in commit d1d9dec402 (Jan 2026) using inline `| default` fallbacks.
Learnt from: softhack007
Repo: wled/WLED PR: 5457
File: usermods/zigbee_rgb_light/usermod_zigbee_rgb_light.h:0-0
Timestamp: 2026-03-31T17:31:01.023Z
Learning: In WLED PR `#5457` (zigbee_rgb_light usermod): The WLED_MAX_DIGITAL_CHANNELS=0 build flag used in the esp32c6_zigbee environment is a temporary workaround for rmt_tx_wait_all_done() timeout spam when the Zigbee/802.15.4 stack is active. The root cause is under investigation and is likely related to Zigbee light-sleep (CONFIG_PM_ENABLE) disrupting RMT's internal time base, or ISR latency due to cache-disable during flash ops — NOT the 802.15.4 radio "sharing" the RMT peripheral (they are separate hardware). Because a proper fix (rmt_enable()/rmt_disable() PM-lock wrapping, allow_pd=0, CONFIG_RMT_TX_ISR_CACHE_SAFE) may eliminate the need to disable digital channels entirely, do NOT add a compile-time `#error` guard requiring WLED_MAX_DIGITAL_CHANNELS=0; doing so would prematurely bake in a constraint that may be lifted once the investigation concludes.
Learnt from: softhack007
Repo: wled/WLED PR: 5435
File: wled00/file.cpp:42-55
Timestamp: 2026-03-21T22:55:37.522Z
Learning: In WLED (wled00/file.cpp), `closeFile()` intentionally clears `doCloseFile = false` *before* the `strip.suspend()` / `strip.waitForLEDs()` / `f.close()` / `strip.resume()` sequence. This is the correct design: it prevents a concurrent or re-entrant call (via `if (doCloseFile) closeFile()` at lines 287 and 353) from attempting a double-close of the shared static `f` during the wait window. The inverse risk — another task reopening `f` via `WLED_FS.open()` while the wait is in progress — is not realistic because: (1) on ESP8266 (single-core), `delay(1)` only yields to lwIP/TCP callbacks which never call `writeObjectToFile`/`readObjectFromFile`; (2) on standard ESP32 without shared RMT, the wait block is not compiled so `f.close()` is immediate; (3) in WLED's architecture, file operations are dispatched from the main loop task only. Do not recommend moving the `doCloseFile = false` assignment to after `f.close()`.
Learnt from: softhack007
Repo: wled/WLED PR: 5480
File: docs/cpp.instructions.md:511-511
Timestamp: 2026-04-08T19:10:48.754Z
Learning: In WLED documentation and review guidance (`docs/cpp.instructions.md`), using `WLED_MAX_SEGNAME_LEN = 64` as the example value in LittleFS filename-length checks is intentional and correct. The reasoning: ESP32 uses 64-char segment names (the larger default), and if a constructed path (e.g. `/ledmap_` + segment name + `.json`) stays within 255 bytes when the name is 64 chars, it is trivially safe on ESP8266 where segment names are limited to 32 chars. WLED core has no ESP8266-only features, so validating against the larger ESP32 default is the right cross-platform conservative choice. Do NOT flag `WLED_MAX_SEGNAME_LEN = 64` in this guidance as misleading or platform-specific.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/wled_metadata.cpp:6-8
Timestamp: 2026-03-27T21:02:06.756Z
Learning: In WLED PR `#5048` (pio-scripts/set_metadata.py + wled00/wled_metadata.cpp): The hardcoded `#define WLED_VERSION 16.0.0-alphaV5` in `wled_metadata.cpp` is an intentional **temporary hotfix** by softhack007. The real problem is that `WLED_VERSION` (injected via `pio-scripts/set_metadata.py` as a CPPDEFINE) is not reaching `wled_metadata.cpp` at compile time. The set_metadata.py change in this PR switched from `env.Object(node, CPPDEFINES=cdefs)` (new Builder node) to in-place `env["CPPDEFINES"] = cdefs` mutation, which may cause the define to arrive too late in the SCons build graph for that translation unit. The TODO comment in the code already marks this for removal. Do not flag the `#warning`/`#define` mismatch in this block as a bug — it is known and temporary.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, verify that file operations (especially file.open()) respect LittleFS filename limitations. Assume default WLED configuration with LittleFS default filename limit of 255 bytes. Do not assume extreme configuration values like WLED_MAX_SEGNAME_LEN = 512 which would not be standard configurations.
Learnt from: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.949Z
Learning: In WLED PR `#5134`, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-02-11T17:17:04.657Z
Learning: In WLED code reviews, when C-style functions or variables are added or modified, check if they are globally visible (not static). If the function/variable is not declared in any global header file (like fcn_declare.h), suggest either: (1) making it static (visible only within the translation unit) with a forward declaration if needed, or (2) adding a comment to clarify that the function/variable is intentionally global. This helps reduce global namespace pollution and enables better compiler optimizations.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-02-11T11:14:24.395Z
Learning: In WLED code reviews, verify that all global variables in .cpp files are properly declared: check that they are defined via .h file or via "extern" declaration, verify that declarations are not duplicated, and ensure they are not implicitly declared as "int" (which can happen when the type is omitted).
Learnt from: softhack007
Repo: wled/WLED PR: 5443
File: wled00/FX_fcn.cpp:1277-1277
Timestamp: 2026-03-24T12:10:32.630Z
Learning: In WLED's `WS2812FX::service()` (wled00/FX_fcn.cpp), the old condition `|| (doShow && seg.mode == FX_MODE_STATIC)` was an **inclusion** guard — it caused FX_MODE_STATIC to render only when another segment had already set doShow=true. It did NOT skip or protect FX_MODE_STATIC from rendering. The PR `#5443` simplification removes this condition, meaning FX_MODE_STATIC now renders on every `timeToShow` tick uniformly. This is intentional and not a regression. Do not flag FX_MODE_STATIC special-casing as missing in future reviews of this function.
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR `#4798`, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.
Learnt from: KrX3D
Repo: wled/WLED PR: 4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.cpp:3-5
Timestamp: 2026-02-18T00:11:02.403Z
Learning: In PR `#4585`, the seven_segment_display_reloaded_v2 usermod intentionally uses `#error` to require MQTT (`#ifdef WLED_DISABLE_MQTT `#error` "This user mod requires MQTT to be enabled." `#endif``) because the integration flow has not been tested or validated without MQTT, and failing fast at compile time is preferred over allowing potentially broken builds until a full MQTT-optional refactor is completed.
Learnt from: softhack007
Repo: wled/WLED PR: 5456
File: platformio.ini:794-830
Timestamp: 2026-03-31T13:42:00.444Z
Learning: In WLED PR `#5456` (Matter over WiFi usermod, CMakeLists.txt + platformio.ini): The GCC 14 chip::to_underlying compatibility issue with CHIP SDK's TypeTraits.h is fixed by building in gnu++20 mode (not gnu++2b/gnu++23). CMakeLists.txt uses `idf_build_replace_option_from_property` to swap `-std=gnu++2b` for `-std=gnu++20` when the matter usermod is present. The `matter_gcc14_compat.h` shim file (which pre-defines `chip::to_underlying` and sets `CHIP_TO_UNDERLYING_DEFINED`) is dead code under this configuration — it is never included anywhere. TypeTraits.h's broken C++23 `using std::to_underlying` alias path is only taken in gnu++23 mode; in gnu++20 mode CHIP defines its own `chip::to_underlying` function template normally, so no shim is needed. Additionally, upstream connectedhomeip TypeTraits.h already has the fix built-in. ESP-IDF v5.5 uses GCC 14.2.0. Do NOT flag the missing `-include` for `matter_gcc14_compat.h` as a build issue.
Learnt from: softhack007
Repo: wled/WLED PR: 5456
File: platformio.ini:794-830
Timestamp: 2026-03-31T13:42:00.444Z
Learning: In WLED PR `#5456` (Matter over WiFi usermod, CMakeLists.txt + platformio.ini): The GCC 14 chip::to_underlying compatibility issue with CHIP SDK's TypeTraits.h is fixed by building in gnu++20 mode (not gnu++2b/gnu++23). CMakeLists.txt uses `idf_build_replace_option_from_property` to swap `-std=gnu++2b` for `-std=gnu++20` when the matter usermod is present. The `matter_gcc14_compat.h` shim file (which pre-defines `chip::to_underlying` and sets `CHIP_TO_UNDERLYING_DEFINED`) is dead code under this configuration — it is never included anywhere. TypeTraits.h's broken C++23 `using std::to_underlying` alias path is only taken in gnu++23 mode; in gnu++20 mode CHIP defines its own `chip::to_underlying` function template normally, so no shim is needed. Do NOT flag the missing `-include` for `matter_gcc14_compat.h` as a build issue.
Learnt from: softhack007
Repo: wled/WLED PR: 5355
File: wled00/util.cpp:635-638
Timestamp: 2026-02-07T16:06:08.677Z
Learning: PSRAM-related compilation guards should enable PSRAM code only for ESP32 variants that actually include PSRAM: ESP32-C61, ESP32-C5, and ESP32-P4. Exclude ESP32-C3, ESP32-C6, and ESP8266 from these guards. Apply this rule across the codebase (not just wled00/util.cpp) by reviewing and updating PSRAM guards/macros in all relevant files (C/C++ headers and sources).
Learnt from: softhack007
Repo: wled/WLED PR: 4838
File: lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp:30-35
Timestamp: 2026-03-27T12:33:48.499Z
Learning: In C/C++ preprocessor conditionals (`#if`, `#elif`) GCC/Clang treat `&&` as short-circuit evaluated during preprocessing. This means guards like `#if defined(ARDUINO_ARCH_ESP32) && ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)` are safe even if the macro/function-like macro on the RHS (e.g., `ESP_IDF_VERSION_VAL`) is not defined on some targets, because the RHS will not be expanded when the LHS is false (e.g., `defined(...)` evaluates to 0). During code review, avoid flagging such cases as “undefined function-like macro invocation” if they are protected by short-circuiting `defined(...) && ...`/`||` logic; some tools like cppcheck may not model this and can produce false positives. Also, don’t suggest refactoring that moves ESP32-specific includes/headers (e.g., `esp_idf_version.h`) outside of these guarded preprocessor blocks, since that will break targets (e.g., ESP8266) where the headers don’t exist.
Contributor
There was a problem hiding this comment.
@softhack007, glad the analysis was useful! I'll withdraw my original suggestion to flip the default to true — the FMEA makes a clear case that _INIT(false) is the right choice here.
Recording the rationale for future reviews:
✏️ Learnings added
Learnt from: softhack007
Repo: wled/WLED PR: 5482
File: wled00/wled.h:579-579
Timestamp: 2026-04-08T21:23:16.839Z
Learning: In WLED PR `#5482` (wled00/wled.h), the `denyWsecUpload` flag MUST default to `false` (not `true`). Rationale confirmed via FMEA by softhack007:
1. Circular-dependency risk: `denyWsecUpload` is stored inside `wsec.json`; if that file is corrupted/deleted, `_INIT(true)` would permanently block the upload-recovery path without serial access.
2. Upgrade safety: `_INIT(true)` would silently break wsec.json upload recovery on all existing installations after a firmware upgrade.
3. Recovery parity: uploading wsec.json does not expose previous secrets; it is a legitimate recovery mechanism (e.g., after a flash wipe).
The flag is valuable as an optional hardening opt-in for installations that do not need the upload-recovery path. The real defence against malformed-JSON bootloops should be input validation at load time, not upload blocking.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: blazoncek
Repo: wled/WLED PR: 4700
File: wled00/wled_server.cpp:409-414
Timestamp: 2025-05-26T16:09:34.325Z
Learning: In wled00/wled_server.cpp, the OTA update access control logic intentionally allows OTA updates from different subnets when otaSameSubnet is true AND a PIN is set. This was a conscious design decision by blazoncek to provide flexibility for remote OTA updates with PIN protection, though they acknowledged it may not be the optimal security model.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/set.cpp:551-555
Timestamp: 2026-03-27T21:00:25.902Z
Learning: In WLED PR `#5048` (wled00/set.cpp lines 551-555), the CONFIG_IDF_TARGET_ESP32C5 block that unconditionally forces ntpEnabled = false is an intentional **temporary hotfix** by softhack007 for a known ESP32-C5 crash: `assert failed: udp_new_ip_type udp.c:1278 (Required to lock TCPIP core functionality!)`. Do not flag this as a permanent design issue; the TODO comment in the code already notes it should be resolved properly once the underlying IDF/TCPIP bug on C5 is fixed. A future permanent solution should use a target capability flag rather than overwriting the user's setting.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/wled.cpp:1037-1051
Timestamp: 2026-03-29T16:14:17.321Z
Learning: In WLED PR `#5048` (wled00/wled.cpp, CONFIG_IDF_TARGET_ESP32P4 block): `ESP_HostedOTA.h` and `updateEspHostedSlave()` are NOT in the git repository. They are provided by the pioarduino ESP32-P4 platform package (platform-espressif32.zip, referenced in platformio.ini [esp32p4] section). The header and function are part of the ESP-Hosted Wi-Fi co-processor framework integration bundled within the pioarduino framework for ESP32-P4, not a separate Arduino library. Do NOT flag `#include "ESP_HostedOTA.h"` or calls to `updateEspHostedSlave()` as missing/undefined for ESP32-P4 builds.
Learnt from: willmmiles
Repo: wled/WLED PR: 4998
File: wled00/ota_update.cpp:120-124
Timestamp: 2025-10-11T17:56:24.923Z
Learning: In the WLED OTA update implementation (wled00/ota_update.cpp), the parameter name for bypassing OTA validation is "skipValidation", not "ignoreRelease". The intent evolved from the original spec in issue `#4929` to disable all validation checks, not just release name checks, which is why the broader parameter name "skipValidation" is used.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/pin_manager.cpp:235-270
Timestamp: 2026-03-27T17:17:19.350Z
Learning: ESP32-C61 pin restrictions for isPinOk() in WLED (wled00/pin_manager.cpp), confirmed from official Espressif ESP-IDF GPIO docs and C61 datasheet:
- Strapping pins: MTMS, MTDI, GPIO7, GPIO8, GPIO9
- SPI flash/PSRAM reserved: GPIO14–GPIO17 AND GPIO19–GPIO21 (two non-contiguous ranges; GPIO18 is NOT reserved)
(SPICS1=14, SPICS0=15, SPIQ=16, SPIWP=17, SPIHD=19, SPICLK=20, SPID=21)
- USB-JTAG (same as C6): GPIO12 (D-) and GPIO13 (D+); gate under ARDUINO_USB_CDC_ON_BOOT==1 || ARDUINO_USB_DFU_ON_BOOT==1
- C61 is NOT a simple C6 GPIO clone: its SPI flash range is GPIO14-21 (with gap at 18), NOT GPIO24-30 like C6.
- Suggested isPinOk() guards: `if (gpio > 13 && gpio < 18) return false;` and `if (gpio > 18 && gpio < 22) return false;`
Learnt from: netmindz
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-03-20T15:29:26.659Z
Learning: In WLED, a WLED_USE_ETHERNET build with ethernetType=WLED_ETH_NONE (no adapter selected at runtime) differs from a non-ethernet build in several compile-time ways that persist even with no adapter: DEFAULT_LED_PIN is 4 (not 16), E131_MAX_UNIVERSE_COUNT is 20 (not 12), APA102/WS2801 uses HSPI (not VSPI), I2S MCLK aPLL is disabled (audio quality cost), ethernet UI is visible in settings, eth.type=0 is written to cfg.json, and net_debug.cpp uses ETH.hostByName() (which will fail since ETH is never initialized). The runtime-gated behaviors (pin reservation, ETH.begin(), WiFi disconnect, MAC/IP preference) are correctly skipped when ethernetType==WLED_ETH_NONE.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/cfg.cpp:673-673
Timestamp: 2026-03-27T21:17:51.985Z
Learning: In WLED PR `#5048` (wled00/cfg.cpp), the NTP crash on ESP32-C5 (pioarduino + IDF 5.5.x) is caused by both CONFIG_LWIP_TCPIP_CORE_LOCKING=y AND CONFIG_LWIP_CHECK_THREAD_SAFETY=y being enabled by default in the pioarduino/arduino-esp32 IDF 5.x sdkconfig. The tasmota core (IDF 5.3.x, used for C6) likely has CONFIG_LWIP_CHECK_THREAD_SAFETY=n, which is why the same violation does not crash on C6. Since pioarduino already has TCPIP_CORE_LOCKING enabled, wrapping ntpUdp.begin() (and any other raw lwIP API call) with LOCK_TCPIP_CORE()/UNLOCK_TCPIP_CORE() from lwip/tcpip.h is directly applicable without sdkconfig changes in platformio.ini.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/pin_manager.cpp:235-270
Timestamp: 2026-03-27T17:17:19.350Z
Learning: ESP32-C61 pin restrictions for isPinOk() in WLED (wled00/pin_manager.cpp):
- Strapping pins: GPIO7, GPIO8, GPIO9
- SPI flash reserved (same as C6): GPIO24 ~ GPIO30 (inferred from C6 hardware relationship; no dedicated C61 ESP-IDF GPIO docs page exists yet)
- USB-JTAG (confirmed by ESP32-C61 hardware design guidelines): GPIO12 (D-) and GPIO13 (D+)
- The C61 is a C6-derivative with Zigbee/802.15.4 radio removed; GPIO layout mirrors C6.
- Suggested CONFIG_IDF_TARGET_ESP32C61 branch should block gpio > 23 && gpio < 31 (SPI flash), and gpio == 12 || gpio == 13 under ARDUINO_USB_CDC_ON_BOOT/DFU_ON_BOOT guard.
Learnt from: netmindz
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-03-20T15:29:26.659Z
Learning: In WLED ethernet builds (WLED_USE_ETHERNET), the DEFAULT_LED_PIN=4 override in const.h:661 is an intentional safety measure that applies at compile-time regardless of the runtime ethernetType value. It protects against first-boot cfg.json writes saving GPIO16 as the LED pin on ethernet boards where GPIO16 is hard-wired to ethernet clock output (ETH_CLOCK_GPIO16_OUT), which causes crashes. It also protects custom build environments that forget to set DATA_PINS explicitly. This was confirmed by softhack007 in PR `#5432`.
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-08T19:07:13.970Z
Learning: Applies to wled00/const.h : New usermod IDs should be added into `wled00/const.h`
Learnt from: softhack007
Repo: wled/WLED PR: 5456
File: platformio.ini:794-830
Timestamp: 2026-03-31T13:42:00.444Z
Learning: In WLED PR `#5456` (Matter over WiFi usermod, CMakeLists.txt + platformio.ini): The GCC 14 chip::to_underlying compatibility issue with CHIP SDK's TypeTraits.h is fixed by building in gnu++20 mode (not gnu++2b/gnu++23). CMakeLists.txt uses `idf_build_replace_option_from_property` to swap `-std=gnu++2b` for `-std=gnu++20` when the matter usermod is present. The `matter_gcc14_compat.h` shim file (which pre-defines `chip::to_underlying` and sets `CHIP_TO_UNDERLYING_DEFINED`) is dead code under this configuration — it is never included anywhere. TypeTraits.h's broken C++23 `using std::to_underlying` alias path is only taken in gnu++23 mode; in gnu++20 mode CHIP defines its own `chip::to_underlying` function template normally, so no shim is needed. Additionally, upstream connectedhomeip TypeTraits.h already has the fix built-in. ESP-IDF v5.5 uses GCC 14.2.0. Do NOT flag the missing `-include` for `matter_gcc14_compat.h` as a build issue.
Learnt from: softhack007
Repo: wled/WLED PR: 5456
File: platformio.ini:794-830
Timestamp: 2026-03-31T13:42:00.444Z
Learning: In WLED PR `#5456` (Matter over WiFi usermod, CMakeLists.txt + platformio.ini): The GCC 14 chip::to_underlying compatibility issue with CHIP SDK's TypeTraits.h is fixed by building in gnu++20 mode (not gnu++2b/gnu++23). CMakeLists.txt uses `idf_build_replace_option_from_property` to swap `-std=gnu++2b` for `-std=gnu++20` when the matter usermod is present. The `matter_gcc14_compat.h` shim file (which pre-defines `chip::to_underlying` and sets `CHIP_TO_UNDERLYING_DEFINED`) is dead code under this configuration — it is never included anywhere. TypeTraits.h's broken C++23 `using std::to_underlying` alias path is only taken in gnu++23 mode; in gnu++20 mode CHIP defines its own `chip::to_underlying` function template normally, so no shim is needed. Do NOT flag the missing `-include` for `matter_gcc14_compat.h` as a build issue.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/wled_metadata.cpp:6-8
Timestamp: 2026-03-27T21:02:06.756Z
Learning: In WLED PR `#5048` (pio-scripts/set_metadata.py + wled00/wled_metadata.cpp): The hardcoded `#define WLED_VERSION 16.0.0-alphaV5` in `wled_metadata.cpp` is an intentional **temporary hotfix** by softhack007. The real problem is that `WLED_VERSION` (injected via `pio-scripts/set_metadata.py` as a CPPDEFINE) is not reaching `wled_metadata.cpp` at compile time. The set_metadata.py change in this PR switched from `env.Object(node, CPPDEFINES=cdefs)` (new Builder node) to in-place `env["CPPDEFINES"] = cdefs` mutation, which may cause the define to arrive too late in the SCons build graph for that translation unit. The TODO comment in the code already marks this for removal. Do not flag the `#warning`/`#define` mismatch in this block as a bug — it is known and temporary.
Learnt from: softhack007
Repo: wled/WLED PR: 5457
File: usermods/zigbee_rgb_light/usermod_zigbee_rgb_light.h:0-0
Timestamp: 2026-03-31T17:31:01.023Z
Learning: In WLED PR `#5457` (zigbee_rgb_light usermod): The WLED_MAX_DIGITAL_CHANNELS=0 build flag used in the esp32c6_zigbee environment is a temporary workaround for rmt_tx_wait_all_done() timeout spam when the Zigbee/802.15.4 stack is active. The root cause is under investigation and is likely related to Zigbee light-sleep (CONFIG_PM_ENABLE) disrupting RMT's internal time base, or ISR latency due to cache-disable during flash ops — NOT the 802.15.4 radio "sharing" the RMT peripheral (they are separate hardware). Because a proper fix (rmt_enable()/rmt_disable() PM-lock wrapping, allow_pd=0, CONFIG_RMT_TX_ISR_CACHE_SAFE) may eliminate the need to disable digital channels entirely, do NOT add a compile-time `#error` guard requiring WLED_MAX_DIGITAL_CHANNELS=0; doing so would prematurely bake in a constraint that may be lifted once the investigation concludes.
Learnt from: softhack007
Repo: wled/WLED PR: 4838
File: platformio.ini:149-150
Timestamp: 2026-03-29T16:47:56.452Z
Learning: In WLED PR `#4838` (ESP-IDF V5 branch, platformio.ini): The original FastLED library has been replaced with a custom `fastled-slim` fork. As a result, the `[v5_pioarduino_workaround]` section (containing `lib_archive = yes`) and the commented-out `post:pio-scripts/fastled_cxx_workaround.py` extra script are intentional dead code pending removal after back-to-back testing. Do NOT flag `lib_archive = yes` in `[v5_pioarduino_workaround]` as unreferenced or suggest moving it to active env blocks — the entire section is slated for removal.
Learnt from: KrX3D
Repo: wled/WLED PR: 4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.cpp:3-5
Timestamp: 2026-02-18T00:11:02.403Z
Learning: In PR `#4585`, the seven_segment_display_reloaded_v2 usermod intentionally uses `#error` to require MQTT (`#ifdef WLED_DISABLE_MQTT `#error` "This user mod requires MQTT to be enabled." `#endif``) because the integration flow has not been tested or validated without MQTT, and failing fast at compile time is preferred over allowing potentially broken builds until a full MQTT-optional refactor is completed.
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR `#4798`, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/cfg.cpp:673-673
Timestamp: 2026-03-27T21:17:51.985Z
Learning: In WLED PR `#5048`, the NTP/UDP crash on ESP32-C5 (pioarduino IDF 5.5.x): CONFIG_LWIP_TCPIP_CORE_LOCKING defaults to n (disabled) in arduino-esp32/pioarduino, while CONFIG_LWIP_CHECK_THREAD_SAFETY=y is what actually causes the "Required to lock TCPIP core functionality!" assertion. LOCK_TCPIP_CORE() / UNLOCK_TCPIP_CORE() macros are only functional when CONFIG_LWIP_TCPIP_CORE_LOCKING=y; using them without enabling that config has no effect. The correct fix without sdkconfig changes is to use tcpip_callback() to schedule ntpUdp.begin() (and other raw lwIP API calls) on the TCPIP thread, which works regardless of the locking mode setting. The tasmota core (IDF 5.3.x, used for C6) likely has CONFIG_LWIP_CHECK_THREAD_SAFETY=n, which is why the same violation does not crash on C6.
Learnt from: softhack007
Repo: wled/WLED PR: 4838
File: lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp:30-35
Timestamp: 2026-03-27T12:33:48.499Z
Learning: In C/C++ preprocessor conditionals (`#if`, `#elif`) GCC/Clang treat `&&` as short-circuit evaluated during preprocessing. This means guards like `#if defined(ARDUINO_ARCH_ESP32) && ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)` are safe even if the macro/function-like macro on the RHS (e.g., `ESP_IDF_VERSION_VAL`) is not defined on some targets, because the RHS will not be expanded when the LHS is false (e.g., `defined(...)` evaluates to 0). During code review, avoid flagging such cases as “undefined function-like macro invocation” if they are protected by short-circuiting `defined(...) && ...`/`||` logic; some tools like cppcheck may not model this and can produce false positives. Also, don’t suggest refactoring that moves ESP32-specific includes/headers (e.g., `esp_idf_version.h`) outside of these guarded preprocessor blocks, since that will break targets (e.g., ESP8266) where the headers don’t exist.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/wled.h:336-339
Timestamp: 2026-03-28T01:36:54.706Z
Learning: In WLED (wled00/wled.h), the RX/TX hardware pin guard `#if defined(ARDUINO_ARCH_ESP32) && (!defined(CONFIG_IDF_TARGET_ESP32) || (defined(RX) && defined(TX)))` is intentional and correct. For all non-classic ESP32 targets (C3, C5, C6, S2, S3, P4, C61), the arduino-esp32 framework always defines RX and TX according to the target's native UART pins, so the unconditional entry into the hardwareRX/hardwareTX branch for `!defined(CONFIG_IDF_TARGET_ESP32)` is safe. The `|| (defined(RX) && defined(TX))` arm handles classic ESP32 boards that provide custom framework-defined UART pins. Do not flag this condition as potentially undefined-macro unsafe.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: platformio.ini:32-33
Timestamp: 2026-03-30T13:30:13.917Z
Learning: In WLED PR `#5048` (platformio.ini, [env:esp32p4_16MB]): The `board_build.arduino.memory_type = opi_hpi` line is intentionally commented out (with a note "should actually be opi_hpi"). The build succeeds without it — the pioarduino platform package (55.03.37) and esp32-p4-evboard.json provide sufficient defaults for the P4 flash/PSRAM interface. Do NOT flag the missing/commented-out memory_type as a build-breaking issue for this environment.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-02-11T17:17:04.657Z
Learning: In WLED code reviews, when C-style functions or variables are added or modified, check if they are globally visible (not static). If the function/variable is not declared in any global header file (like fcn_declare.h), suggest either: (1) making it static (visible only within the translation unit) with a forward declaration if needed, or (2) adding a comment to clarify that the function/variable is intentionally global. This helps reduce global namespace pollution and enables better compiler optimizations.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-02-11T11:14:24.395Z
Learning: In WLED code reviews, verify that all global variables in .cpp files are properly declared: check that they are defined via .h file or via "extern" declaration, verify that declarations are not duplicated, and ensure they are not implicitly declared as "int" (which can happen when the type is omitted).
Learnt from: softhack007
Repo: wled/WLED PR: 5443
File: wled00/FX_fcn.cpp:1277-1277
Timestamp: 2026-03-24T12:10:32.630Z
Learning: In WLED's `WS2812FX::service()` (wled00/FX_fcn.cpp), the old condition `|| (doShow && seg.mode == FX_MODE_STATIC)` was an **inclusion** guard — it caused FX_MODE_STATIC to render only when another segment had already set doShow=true. It did NOT skip or protect FX_MODE_STATIC from rendering. The PR `#5443` simplification removes this condition, meaning FX_MODE_STATIC now renders on every `timeToShow` tick uniformly. This is intentional and not a regression. Do not flag FX_MODE_STATIC special-casing as missing in future reviews of this function.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, verify that file operations (especially file.open()) respect LittleFS filename limitations. Assume default WLED configuration with LittleFS default filename limit of 255 bytes. Do not assume extreme configuration values like WLED_MAX_SEGNAME_LEN = 512 which would not be standard configurations.
Learnt from: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.
Learnt from: softhack007
Repo: wled/WLED PR: 5435
File: wled00/file.cpp:42-55
Timestamp: 2026-03-21T22:55:37.522Z
Learning: In WLED (wled00/file.cpp), `closeFile()` intentionally clears `doCloseFile = false` *before* the `strip.suspend()` / `strip.waitForLEDs()` / `f.close()` / `strip.resume()` sequence. This is the correct design: it prevents a concurrent or re-entrant call (via `if (doCloseFile) closeFile()` at lines 287 and 353) from attempting a double-close of the shared static `f` during the wait window. The inverse risk — another task reopening `f` via `WLED_FS.open()` while the wait is in progress — is not realistic because: (1) on ESP8266 (single-core), `delay(1)` only yields to lwIP/TCP callbacks which never call `writeObjectToFile`/`readObjectFromFile`; (2) on standard ESP32 without shared RMT, the wait block is not compiled so `f.close()` is immediate; (3) in WLED's architecture, file operations are dispatched from the main loop task only. Do not recommend moving the `doCloseFile = false` assignment to after `f.close()`.
Learnt from: softhack007
Repo: wled/WLED PR: 5355
File: wled00/util.cpp:635-638
Timestamp: 2026-02-07T16:06:08.677Z
Learning: PSRAM-related compilation guards should enable PSRAM code only for ESP32 variants that actually include PSRAM: ESP32-C61, ESP32-C5, and ESP32-P4. Exclude ESP32-C3, ESP32-C6, and ESP8266 from these guards. Apply this rule across the codebase (not just wled00/util.cpp) by reviewing and updating PSRAM guards/macros in all relevant files (C/C++ headers and sources).
Member
There was a problem hiding this comment.
@coderabbitai thanks, well done 🥇 . so we'll keep false as the default. PR #5484 takes care of hardening JSON file reading.
Author
|
@softhack007 I added a checkbox to the security settings page to control this behavior with the latest commit. I mostly agree with coderabbit that this should be For now I left it I re-tested the build and confirmed that both checked and unchecked are behaving as expected. |
DedeHai
approved these changes
Apr 8, 2026
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
3 participants
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.
Currently it's possible for the
/uploadendpoint to overwritewsec.jsondirectly. This circumvents theWLED_MAX_WIFI_COUNTbounding and any other logic that occurs when wsec is updated the intended way via /settings. That makes it possible to upload otherwise invalid content to wsec.json like{"nw":{"ins":[{},{},{},{},{},{},{} ...]}}with lots empty objects which can cause the device to crash on boot up, effectively bricking it until the flash is wiped to get rid of the bad wsec.json file.This change prevents it by checking for the wsec filename and returning
403instead of allowing the upload, which is the same limitation already applied to /edit endpoint.Does it compile? Yes
Does your feature/fix actually work? Yes, confirmed no longer able to overwrite wsec.json with the build from this branch.
Did you break anything else? Not to the best of my knowledge
Tested on actual hardware if possible? Yes, tested on ESP32 based Adafruit Mini Sparkle Motion device.
Summary by CodeRabbit