Add support for WPA-Enterprise #5194
Conversation
commit 70fe1fc Author: Benjamin Kraus <ben@benkraus.com> Date: Fri Oct 31 20:52:13 2025 -0400 Added support for enterprise WiFi.
WalkthroughThis change introduces WPA2 Enterprise WiFi support to WLED by extending the WiFiConfig structure with encryption type and identity fields, updating the settings UI to accept enterprise credentials, and implementing platform-specific WiFi connection logic for PEAP authentication on both ESP8266 and ESP32. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Rationale: The change involves heterogeneous modifications across configuration structures, HTML UI with new JavaScript logic, C++ handlers for credential parsing, platform-specific conditional WiFi initialization paths, and JSON serialization—requiring separate reasoning for each domain. The logic density is moderate with multiple interdependent changes (encryption type switching, credential validation, platform branching), though individual components follow consistent patterns. Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/set.cpp (1)
24-75: Identity change detection usesstrncmpincorrectlyThe new enterprise identity handling is functionally inverted:
strncmp(...)returns 0 when strings are equal.- The current code sets
forceReconnectwhen the new anon/identity matches the old values, not when they differ:if (!strncmp(multiWiFi[n].enterpriseAnonIdentity, oldAnon, 64)) { forceReconnect = true; } if (!strncmp(multiWiFi[n].enterpriseIdentity, oldIden, 64)) { forceReconnect = true; }This causes unnecessary reconnects when nothing changed and (more importantly) relies on other conditions to trigger reconnects when identities actually do change.
A minimal fix is to flip the condition and use the full buffer size:
- if (!strncmp(multiWiFi[n].enterpriseAnonIdentity, oldAnon, 64)) { - forceReconnect = true; - } - if (!strncmp(multiWiFi[n].enterpriseIdentity, oldIden, 64)) { - forceReconnect = true; - } + if (strncmp(multiWiFi[n].enterpriseAnonIdentity, oldAnon, sizeof(oldAnon)) != 0) { + forceReconnect = true; + } + if (strncmp(multiWiFi[n].enterpriseIdentity, oldIden, sizeof(oldIden)) != 0) { + forceReconnect = true; + }This will only set
forceReconnectwhen either enterprise identity actually changed.
🧹 Nitpick comments (2)
wled00/wled.cpp (1)
684-723: Clarify behavior when WPA‑Enterprise is compiled outBranching on
multiWiFi[selectedWiFi].encryptionTypeand separating PSK vs WPA2‑Enterprise handling looks correct, including clearing ESP8266 enterprise state before PSK connects and using the PEAPWiFi.beginoverload on ESP32.One edge case: if
WLED_DISABLE_WPA_ENTERPRISEis defined but a config/UI entry still hasencryptionType == WIFI_ENCRYPTION_TYPE_ENTERPRISE, theelsebranch becomes empty and noWiFi.beginis issued, so the device will never actively try to connect for that SSID.Consider normalizing such entries back to PSK (or logging a warning and ignoring the enterprise type) when enterprise support is disabled, so behavior is explicit rather than a silent no‑op.
wled00/fcn_declare.h (1)
71-74: Initializer list order does not match member declaration order.
encryptionTypeis declared on line 61, beforestaticIP/staticGW/staticSN(lines 64-66), but appears last in the initializer list. C++ initializes members in declaration order regardless of initializer list order, which triggers-Wreordercompiler warnings.- : staticIP(ip) - , staticGW(gw) - , staticSN(subnet) - , encryptionType(enc_type) + : encryptionType(enc_type) + , staticIP(ip) + , staticGW(gw) + , staticSN(subnet)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
platformio.ini(2 hunks)wled00/cfg.cpp(2 hunks)wled00/const.h(1 hunks)wled00/data/settings_wifi.htm(3 hunks)wled00/fcn_declare.h(1 hunks)wled00/set.cpp(2 hunks)wled00/wled.cpp(1 hunks)wled00/wled.h(1 hunks)wled00/xml.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
wled00/**/!(html_*)*.h
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for non-generated C++ header files (.h)
Files:
wled00/const.hwled00/wled.hwled00/fcn_declare.h
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/set.cppwled00/cfg.cppwled00/wled.cppwled00/xml.cpp
platformio.ini
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use platformio.ini as the single source of truth for hardware build targets and settings
Files:
platformio.ini
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/settings_wifi.htm
wled00/data/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
wled00/data/**: When modifying web UI files, runnpm run buildto regenerate embedded headers before any firmware build
For web UI changes, edit files only under wled00/data (not firmware or generated files)
Files:
wled00/data/settings_wifi.htm
wled00/data/settings*.htm
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Name settings pages as settings*.htm within the web UI
Files:
wled00/data/settings_wifi.htm
🧠 Learnings (15)
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/html_*.h : DO NOT edit generated embedded web header files (wled00/html_*.h)
Applied to files:
wled00/const.hwled00/data/settings_wifi.htmwled00/wled.hwled00/fcn_declare.hwled00/xml.cpp
📚 Learning: 2025-03-29T01:22:54.617Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4623
File: usermods/word-clock-matrix/word-clock-matrix.cpp:332-332
Timestamp: 2025-03-29T01:22:54.617Z
Learning: In the WLED project, hardcoded usermod IDs (like 500 in the WordClockMatrix's getId() method) are intentionally used as part of a strategy to avoid modifying core code. There are plans to remove numeric usermod IDs entirely in the future.
Applied to files:
wled00/const.h
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
Repo: wled/WLED PR: 4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Applied to files:
wled00/const.h
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to platformio.ini : Use platformio.ini as the single source of truth for hardware build targets and settings
Applied to files:
platformio.ini
📚 Learning: 2025-11-22T20:33:14.840Z
Learnt from: softhack007
Repo: wled/WLED PR: 5107
File: boards/adafruit_matrixportal_esp32s3.json:1-66
Timestamp: 2025-11-22T20:33:14.840Z
Learning: In WLED, board JSON files in the boards/ directory may reference partition files that don't exist or have slightly different names than actual files in tools/. This is intentional because platformio.ini or platformio_override.sample.ini explicitly override the partition file using the board_build.partitions setting, which takes precedence over the board JSON partition reference. This pattern exists in multiple local board definitions and should not be flagged as an issue.
Applied to files:
platformio.ini
📚 Learning: 2025-09-02T02:27:39.220Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4890
File: lib/NeoESP32RmtHI/src/NeoEsp32RmtHI.S:8-16
Timestamp: 2025-09-02T02:27:39.220Z
Learning: The ESP32 macro is defined when compiling code on all ESP32 variants (ESP32, ESP32-S2, ESP32-S3, ESP32-C3, etc.) in the Arduino framework, providing consistent library compatibility across the entire ESP32 family. For target-specific detection, CONFIG_IDF_TARGET_* macros should be used instead.
Applied to files:
platformio.ini
📚 Learning: 2025-09-02T01:45:58.047Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4890
File: lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h:31-36
Timestamp: 2025-09-02T01:45:58.047Z
Learning: For the NeoEsp32RmtHI driver, RISC-V ESP32-C3 support is currently disabled via bus_wrapper.h rather than compile-time guards, as the maintainer willmmiles is working on resolving underlying nested interrupt issues and prefers to centralize the workaround in one location.
Applied to files:
platformio.ini
📚 Learning: 2025-02-19T12:43:34.199Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
Applied to files:
platformio.iniwled00/wled.h
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/data/** : For web UI changes, edit files only under wled00/data (not firmware or generated files)
Applied to files:
wled00/data/settings_wifi.htm
📚 Learning: 2025-08-29T00:22:34.115Z
Learnt from: ksedgwic
Repo: wled/WLED PR: 4883
File: usermods/usermod_v2_skystrip/rest_json_client.cpp:40-46
Timestamp: 2025-08-29T00:22:34.115Z
Learning: For ESP32/ESP8266 HTTP requests in WLED, set timeouts on the underlying WiFiClient (using client.setTimeout()) before calling http_.begin(), rather than using HTTPClient::setTimeout(). This pattern is used successfully in the Klipper usermod to prevent connection hangs.
Applied to files:
wled00/wled.cppwled00/wled.h
📚 Learning: 2025-08-29T00:26:15.808Z
Learnt from: ksedgwic
Repo: wled/WLED PR: 4883
File: usermods/usermod_v2_skystrip/rest_json_client.h:6-14
Timestamp: 2025-08-29T00:26:15.808Z
Learning: WLED uses a vendored ArduinoJson library (version 6) located at "src/dependencies/json/ArduinoJson-v6.h" which is included through wled.h. Usermods should not directly include ArduinoJson headers but instead rely on wled.h for ArduinoJson symbols. The standard pattern is to include wled.h and use JsonObject, JsonArray, DynamicJsonDocument, etc. without additional includes.
Applied to files:
wled00/wled.h
📚 Learning: 2025-08-29T00:26:15.808Z
Learnt from: ksedgwic
Repo: wled/WLED PR: 4883
File: usermods/usermod_v2_skystrip/rest_json_client.h:6-14
Timestamp: 2025-08-29T00:26:15.808Z
Learning: In WLED projects, ArduinoJson.h is not directly included via #include <ArduinoJson.h> - the ArduinoJson symbols are made available through the WLED build system and wled.h transitive includes, so explicitly adding #include <ArduinoJson.h> is not necessary and may not work.
Applied to files:
wled00/wled.h
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/html_*.h : Always commit generated html_*.h files along with related source changes
Applied to files:
wled00/wled.h
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/**/!(html_*)*.h : Use 2-space indentation for non-generated C++ header files (.h)
Applied to files:
wled00/wled.h
📚 Learning: 2025-08-29T01:34:34.358Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4853
File: wled00/util.cpp:779-781
Timestamp: 2025-08-29T01:34:34.358Z
Learning: On ESP8266 systems, avoid adding no-op stub functions across translation units due to limited code memory constraints, as the compiler cannot inline away the function calls, resulting in wasteful memory usage.
Applied to files:
wled00/wled.h
🧬 Code graph analysis (1)
wled00/wled.cpp (1)
wled00/util.cpp (2)
prepareHostname(131-152)prepareHostname(131-131)
🔇 Additional comments (7)
platformio.ini (1)
519-532: Formatting-only changes – build matrix unaffectedThe edits here are purely whitespace/comment tweaks in existing envs; no PlatformIO options or default envs changed, so hardware targets and settings remain consistent with platformio.ini being the single source of truth.
Also applies to: 640-657
wled00/cfg.cpp (1)
90-128: WiFi enterprise fields are serialized/deserialized consistently and remain backward‑compatible
enc_typedefaults to PSK when missing, and enterprise identities are only emitted whenencryptionType == WIFI_ENCRYPTION_TYPE_ENTERPRISE, so old configs still load as PSK while new WPA‑Enterprise data round‑trips correctly.Also applies to: 866-887
wled00/wled.h (1)
76-96: Scoped ESP8266 WPA‑Enterprise include looks correctConditionally including
wpa2_enterprise.honly on ESP8266 and only whenWLED_DISABLE_WPA_ENTERPRISEis not defined cleanly encapsulates the feature without impacting ESP32 builds.wled00/const.h (1)
211-214: WiFi encryption type constants integrate cleanlyDefining
WIFI_ENCRYPTION_TYPE_PSKandWIFI_ENCRYPTION_TYPE_ENTERPRISEas simple macros is consistent with the rest of this header and matches how the new UI, config, and connection code branch on encryption type.wled00/xml.cpp (1)
190-212: JS WiFi payload now aligned with enterprise config modelThe updated
addWiFi(...)call correctly passesencryptionType, anonymous identity, and identity alongside the existing fields, in the same parameter order thatsettings_wifi.htmexpects, while keeping the password masked and IP fields encoded as before.wled00/data/settings_wifi.htm (1)
90-99: WiFi settings UI is correctly extended for Enterprise modeThe updated
addWiFi()signature, ET/EA/EI field names, and the newE(i)helper cleanly add WPA/WPA2‑Enterprise support on the page: PSK vs Enterprise is selectable per network, enterprise identities are only shown when needed, and the naming aligns with whatxml.cppemits andset.cppconsumes.Also applies to: 121-145, 197-205
wled00/fcn_declare.h (1)
59-82: LGTM on the WiFiConfig struct extension for WPA-Enterprise support.The buffer handling is correct:
strncpywith explicit null-termination prevents overflow, and the 65-byte buffers adequately accommodate enterprise identity fields.WIFI_ENCRYPTION_TYPE_PSKis properly defined inconst.hand included beforefcn_declare.hin the compilation chain, ensuring the default parameter value is accessible. The struct cleanly extends existing WiFi configuration to support enterprise authentication.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
wled00/fcn_declare.h(2 hunks)wled00/set.cpp(3 hunks)wled00/wled.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wled00/wled.cpp
🧰 Additional context used
📓 Path-based instructions (2)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/set.cpp
wled00/**/!(html_*)*.h
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for non-generated C++ header files (.h)
Files:
wled00/fcn_declare.h
🧠 Learnings (2)
📚 Learning: 2025-11-30T15:29:00.726Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4456
File: usermods/deep_sleep/deep_sleep.cpp:224-230
Timestamp: 2025-11-30T15:29:00.726Z
Learning: In WLED, `touchThreshold` is a global variable defined in wled00/wled.h and is accessible to all usermods. It stores the touch sensitivity threshold for touch-capable GPIO pins on ESP32 variants and should not be flagged as undefined when used in usermods.
Applied to files:
wled00/set.cpp
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/html_*.h : DO NOT edit generated embedded web header files (wled00/html_*.h)
Applied to files:
wled00/fcn_declare.h
🧬 Code graph analysis (1)
wled00/fcn_declare.h (3)
usermods/audioreactive/audio_reactive.cpp (2)
appendConfigData(1904-1959)appendConfigData(1904-1904)wled00/um_manager.cpp (4)
appendConfigData(26-26)appendConfigData(26-26)appendConfigData(95-100)appendConfigData(95-95)usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp (2)
appendConfigData(1094-1097)appendConfigData(1094-1094)
🔇 Additional comments (4)
wled00/set.cpp (2)
24-28: LGTM!The new field key definitions for encryption type and enterprise identity fields follow the existing pattern correctly.
35-55: LGTM!The old value storage and conditional handling based on encryption type is well-structured. The buffer sizes match the struct definitions and the encryption type change detection is correct.
wled00/fcn_declare.h (2)
61-68: LGTM!New struct members are appropriately sized and logically grouped. The 65-byte buffers correctly accommodate 64-character identities plus null terminator.
69-81: LGTM — constant verified and indentation compliant.Constructor extension is well-implemented with appropriate defaults and correct null-termination for string fields. The
WIFI_ENCRYPTION_TYPE_PSKconstant is properly defined inwled00/const.hand widely used throughout the codebase. Indentation complies with the 2-space requirement for non-generated header files. Default parameter values maintain backward compatibility.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wled00/set.cpp (1)
35-35: Minor style: combine declaration and initialization.The split declaration-then-assignment can be simplified to a single statement.
- byte oldType; oldType = multiWiFi[n].encryptionType; + byte oldType = multiWiFi[n].encryptionType;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/set.cpp(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/set.cpp
🧠 Learnings (1)
📚 Learning: 2025-11-30T15:29:00.726Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4456
File: usermods/deep_sleep/deep_sleep.cpp:224-230
Timestamp: 2025-11-30T15:29:00.726Z
Learning: In WLED, `touchThreshold` is a global variable defined in wled00/wled.h and is accessible to all usermods. It stores the touch sensitivity threshold for touch-capable GPIO pins on ESP32 variants and should not be flagged as undefined when used in usermods.
Applied to files:
wled00/set.cpp
🔇 Additional comments (2)
wled00/set.cpp (2)
24-27: LGTM!The new parameter key variables (
et,ea,ei) follow the established pattern used for other WiFi configuration parameters in this loop.
37-60: LGTM!The enterprise credential handling logic is correct:
- Old values are properly saved before modification
- Encryption type change triggers reconnect
- PSK mode correctly clears enterprise fields
- Enterprise mode properly copies identity values from the request
- The
strncmp(...) != 0comparisons correctly detect changes and trigger reconnectionThe previous inverted
strncmplogic has been properly fixed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wled00/set.cpp (1)
24-61: Enterprise WiFi handling is sound; consider minor robustness tweaksThe new handling for
ETn/EAn/EInlooks correct and safe: you copy into fixed-size buffers withstrlcpy(..., 65)and compare withstrncmp(..., 64) != 0, and you correctly triggerforceReconnectwhen encryption type, SSID, or enterprise identities change.Two small, non-blocking suggestions:
To make the comparisons resilient to future buffer-size changes, you could avoid hard‑coded
64and instead use the local buffer sizes, e.g.:if (strncmp(multiWiFi[n].enterpriseAnonIdentity, oldAnon, 64) != 0) {
if (strncmp(multiWiFi[n].enterpriseAnonIdentity, oldAnon, sizeof(oldAnon) - 1) != 0) { forceReconnect = true; }
if (strncmp(multiWiFi[n].enterpriseIdentity, oldIden, 64) != 0) {
if (strncmp(multiWiFi[n].enterpriseIdentity, oldIden, sizeof(oldIden) - 1) != 0) { forceReconnect = true; }
- Right now, all non‑
WIFI_ENCRYPTION_TYPE_PSKvalues fall into the “WPA2-Enterprise”elsebranch. That matches today’s two‑option UI, but if more encryption types are ever added, they will also end up treated as Enterprise and get EA/EI copied. Consider tightening this to an explicit check for your Enterprise enum value (with a default branch that just clears the enterprise fields) to stay future‑proof.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/set.cpp(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/set.cpp
🧠 Learnings (1)
📚 Learning: 2025-11-30T15:29:00.726Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4456
File: usermods/deep_sleep/deep_sleep.cpp:224-230
Timestamp: 2025-11-30T15:29:00.726Z
Learning: In WLED, `touchThreshold` is a global variable defined in wled00/wled.h and is accessible to all usermods. It stores the touch sensitivity threshold for touch-capable GPIO pins on ESP32 variants and should not be flagged as undefined when used in usermods.
Applied to files:
wled00/set.cpp
🔇 Additional comments (2)
wled00/set.cpp (2)
329-331: Touch button branch structure remains correctThis block still correctly handles invalid touch pins by deallocating and clearing the button type, and (when
SOC_TOUCH_VERSION_2is defined) attaches the interrupt only on valid touch‑capable pins via theelse touchAttachInterrupt(...). No behavioral concerns here.
981-982: Non-functional spacing change before brightness handlingThe added blank separation before the “set brightness” section is purely cosmetic and does not affect control flow or behavior. Layout stays consistent with surrounding code.
|
thanks for updating. I am not sure on how to proceed on this one but it can not be in the default builds as this eats up way too much flash, 80k on ESP8266 and over 100k on ESP32 variants. |
You know I'm not a fan of From an implementation standpoint:
Other notes:
|
Thanks @willmmiles and @DedeHai for the feedback. I'll make these changes and do my best to omit the UI fields entirely at built time if the feature is disabled (without overly complicating the code).
I considered this, but I wasn't sure if there was a cost to putting more things into the secure config. It is an easy enough change if we think the anonymous identity and/or identity should be kept more secure. Based on my (admittedly limited) understanding of WPA enterprise, I believe the "anonymous identity" is presented unencrypted when establishing the connection but the "identity" is only sent over an encrypted channel, so I could see an argument for treating the anonymous identity similarly to the SSID and treating the identity/username similar to the password. As two random data points: Neither my Linux laptop or my Android phone do anything to obscure the identity and anonymous identity on their WiFi configuration pages (they are both visible by default).
The whitespace changes were done automatically by Visual Studio Code and it was actually a little hard to undo the changes (Visual Studio Code kept generously fixing them again for me) so I left them in my initial commit. I debated whether it would be more distracting to leave them in or to add a later change reverting them. I'll revert all the unrelated whitespace-only changes. |
IIRC there was a PR that used a script to inject JS code at build time but don't ask me which one ;)
I have auto-remove disabled but have white-spaces show in red, I think that is a VSc plugin (I would recommend to get @willmmiles if you are using VScode ;) ) |
just if you mean inject_syslog_ui.py : #4664 |
|
thanks @KrX3D, that exactly :) |
|
Oof, using a python script to manipulate the HTML files is a world of complexity I'd rather not take on. I was expecting that it should be done via the |
|
Yeah this definitely needs to be excluded by default given it's increased flash usege and very niche usage |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
wled00/xml.cpp (1)
201-211: Consider escaping special characters in enterprise identity fields.The
enterpriseAnonIdentityandenterpriseIdentityfields are inserted directly into a JavaScript string literal. If these fields contain characters like",\, or newlines, they could break the generated JavaScript or potentially allow script injection.The same concern exists for
clientSSIDin the existing code, but SSIDs have a more limited character set. Enterprise identity fields (often email addresses or usernames) may contain a wider range of characters.🔎 Suggested approach
Consider adding a helper function to escape strings for JavaScript insertion, or ensure that the
strlcpyoperations inset.cppsanitize problematic characters on input. At minimum, characters like",\,<, and>should be escaped or stripped.wled00/set.cpp (1)
57-84: Enterprise settings handling looks correct; minor defensive coding suggestion.The logic for handling enterprise settings is well-structured:
- Old values are saved before modification for change detection
- Default to PSK when enterprise fields are absent
- Enterprise identity fields are cleared when switching to PSK mode
- Reconnection is triggered when encryption type or identities change
The
strncmp(...) != 0comparisons correctly detect when values differ (addressing the previous review feedback).One minor suggestion for defensive coding:
🔎 Consider using strncpy instead of strcpy for old value copies
- char oldAnon[65]; strcpy(oldAnon, multiWiFi[n].enterpriseAnonIdentity); - char oldIden[65]; strcpy(oldIden, multiWiFi[n].enterpriseIdentity); + char oldAnon[65]; strncpy(oldAnon, multiWiFi[n].enterpriseAnonIdentity, 64); oldAnon[64] = 0; + char oldIden[65]; strncpy(oldIden, multiWiFi[n].enterpriseIdentity, 64); oldIden[64] = 0;This ensures bounds safety even if the source strings are somehow corrupted.
wled00/data/settings_wifi.htm (1)
121-148: Well-structured UI for enterprise settings; consider HTML escaping for identity values.The conditional rendering of encryption type fields when
type >= 0is a clean approach that naturally hides enterprise UI when the feature is disabled (type defaults to -1 from non-enterprise builds).However, the
anonandidentvalues are inserted directly into HTML attribute values without escaping:value="${anon}" value="${ident}"If these contain characters like
"or<, it could break the HTML structure.🔎 Consider escaping special HTML characters
Add a simple escape helper or use existing sanitization:
function escHtml(s) { return s.replace(/&/g,'&').replace(/</g,'<').replace(/>/g,'>').replace(/"/g,'"'); }Then use
${escHtml(anon)}and${escHtml(ident)}in the template.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
wled00/cfg.cppwled00/const.hwled00/data/settings_wifi.htmwled00/fcn_declare.hwled00/set.cppwled00/wled.cppwled00/wled.hwled00/xml.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- wled00/const.h
- wled00/cfg.cpp
- wled00/wled.cpp
🧰 Additional context used
📓 Path-based instructions (5)
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/settings_wifi.htm
wled00/data/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
wled00/data/**: When modifying web UI files, runnpm run buildto regenerate embedded headers before any firmware build
For web UI changes, edit files only under wled00/data (not firmware or generated files)
Files:
wled00/data/settings_wifi.htm
wled00/data/settings*.htm
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Name settings pages as settings*.htm within the web UI
Files:
wled00/data/settings_wifi.htm
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/set.cppwled00/xml.cpp
wled00/**/!(html_*)*.h
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for non-generated C++ header files (.h)
Files:
wled00/wled.hwled00/fcn_declare.h
🧠 Learnings (13)
📓 Common learnings
Learnt from: netmindz
Repo: wled/WLED PR: 5093
File: wled00/util.cpp:1159-1182
Timestamp: 2025-11-20T00:04:04.829Z
Learning: In WLED PR #5093, the deviceId feature is designed for opt-in usage reporting that tracks only version/upgrade information (non-behavioral data), not user activity patterns. The deterministic salt approach (MAC + "WLED" + chip model/revision) is acceptable for this limited use case, as correlating MAC addresses to version history represents minimal privacy risk compared to behavioral tracking.
Learnt from: blazoncek
Repo: wled/WLED PR: 4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
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: ksedgwic
Repo: wled/WLED PR: 4883
File: usermods/usermod_v2_skystrip/rest_json_client.cpp:40-46
Timestamp: 2025-08-29T00:22:34.115Z
Learning: For ESP32/ESP8266 HTTP requests in WLED, set timeouts on the underlying WiFiClient (using client.setTimeout()) before calling http_.begin(), rather than using HTTPClient::setTimeout(). This pattern is used successfully in the Klipper usermod to prevent connection hangs.
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/data/** : For web UI changes, edit files only under wled00/data (not firmware or generated files)
Applied to files:
wled00/data/settings_wifi.htmwled00/wled.h
📚 Learning: 2025-05-26T16:09:34.325Z
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.
Applied to files:
wled00/set.cpp
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/html_*.h : DO NOT edit generated embedded web header files (wled00/html_*.h)
Applied to files:
wled00/xml.cppwled00/wled.hwled00/fcn_declare.h
📚 Learning: 2025-10-10T18:34:06.550Z
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.
Applied to files:
wled00/xml.cpp
📚 Learning: 2025-08-29T00:26:15.808Z
Learnt from: ksedgwic
Repo: wled/WLED PR: 4883
File: usermods/usermod_v2_skystrip/rest_json_client.h:6-14
Timestamp: 2025-08-29T00:26:15.808Z
Learning: WLED uses a vendored ArduinoJson library (version 6) located at "src/dependencies/json/ArduinoJson-v6.h" which is included through wled.h. Usermods should not directly include ArduinoJson headers but instead rely on wled.h for ArduinoJson symbols. The standard pattern is to include wled.h and use JsonObject, JsonArray, DynamicJsonDocument, etc. without additional includes.
Applied to files:
wled00/wled.h
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/html_*.h : Always commit generated html_*.h files along with related source changes
Applied to files:
wled00/wled.h
📚 Learning: 2025-08-29T00:26:15.808Z
Learnt from: ksedgwic
Repo: wled/WLED PR: 4883
File: usermods/usermod_v2_skystrip/rest_json_client.h:6-14
Timestamp: 2025-08-29T00:26:15.808Z
Learning: In WLED projects, ArduinoJson.h is not directly included via #include <ArduinoJson.h> - the ArduinoJson symbols are made available through the WLED build system and wled.h transitive includes, so explicitly adding #include <ArduinoJson.h> is not necessary and may not work.
Applied to files:
wled00/wled.h
📚 Learning: 2025-09-18T03:17:30.107Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1183-1194
Timestamp: 2025-09-18T03:17:30.107Z
Learning: In WLED bus management code, dead code branches in error handling (like unreachable else breaks) are intentionally preserved to accommodate potential future API changes, even when currently unreachable.
Applied to files:
wled00/wled.h
📚 Learning: 2025-08-29T00:22:34.115Z
Learnt from: ksedgwic
Repo: wled/WLED PR: 4883
File: usermods/usermod_v2_skystrip/rest_json_client.cpp:40-46
Timestamp: 2025-08-29T00:22:34.115Z
Learning: For ESP32/ESP8266 HTTP requests in WLED, set timeouts on the underlying WiFiClient (using client.setTimeout()) before calling http_.begin(), rather than using HTTPClient::setTimeout(). This pattern is used successfully in the Klipper usermod to prevent connection hangs.
Applied to files:
wled00/wled.h
📚 Learning: 2025-02-19T12:43:34.199Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
Applied to files:
wled00/wled.h
📚 Learning: 2025-09-02T02:27:39.220Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4890
File: lib/NeoESP32RmtHI/src/NeoEsp32RmtHI.S:8-16
Timestamp: 2025-09-02T02:27:39.220Z
Learning: The ESP32 macro is defined when compiling code on all ESP32 variants (ESP32, ESP32-S2, ESP32-S3, ESP32-C3, etc.) in the Arduino framework, providing consistent library compatibility across the entire ESP32 family. For target-specific detection, CONFIG_IDF_TARGET_* macros should be used instead.
Applied to files:
wled00/wled.h
📚 Learning: 2025-09-18T02:59:33.666Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4928
File: wled00/FX_fcn.cpp:1181-1181
Timestamp: 2025-09-18T02:59:33.666Z
Learning: In WLED, getFreeHeapSize() is a platform-agnostic function defined in fcn_declare.h that provides accurate free heap reporting across ESP32 and ESP8266 platforms. On ESP32, it uses heap_caps_get_free_size(MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT) instead of ESP.getFreeHeap() because the latter can include other memory types. On ESP8266, it uses ESP.getFreeHeap().
Applied to files:
wled00/wled.h
🔇 Additional comments (4)
wled00/wled.h (1)
80-82: LGTM — Conditional enterprise header inclusion is correctly gated.The
wpa2_enterprise.hinclude is properly placed within the ESP8266 block and conditionally compiled only whenWLED_ENABLE_WPA_ENTERPRISEis defined. This aligns with the PR objective to gate the feature behind a build-time flag to avoid flash overhead in default builds.wled00/set.cpp (1)
30-34: LGTM — Enterprise token arrays correctly defined.The token arrays for encryption type (
ET), enterprise anonymous identity (EA), and enterprise identity (EI) follow the existing pattern used for other WiFi settings tokens.wled00/data/settings_wifi.htm (1)
201-208: LGTM — Toggle function for enterprise identity fields.The
E(i)function correctly toggles the visibility of the IDS block based on the encryption type selection. Usingstyle.displaywith empty string to show and"none"to hide is the standard approach.wled00/fcn_declare.h (1)
66-85: LGTM — Clean conditional extension of WiFiConfig for enterprise support.The struct extension is well-implemented:
- Enterprise fields are only present when
WLED_ENABLE_WPA_ENTERPRISEis defined, avoiding memory overhead in default builds- The 65-byte arrays correctly accommodate 64 characters plus null terminator
- The
strncpywith explicit null-termination at index 64 is safe- Default parameter value
WIFI_ENCRYPTION_TYPE_PSKis defined in const.h and properly included before this header- Indentation follows the 2-space requirement for this file pattern
|
I just pushed an update to address your review feedback. I couldn't find a clean way to completely remove the JavaScript relating to the settings page when I did run into one question when I was debugging: As the code is currently written, if you are running firmware with |
|
Another question: I see the notice that "All checks have passed", but I imagine none of those checks sets the |
5 participants
This change adds support for enterprise WiFi requiring a username and anonymous identity.
This change is a revival of this old PR: #3195
The WiFi setup page was augmented with a drop-down menu to choose between "None/WPA/WPA2" and "WPA/WPA2-Enterprise". When you select "WPA/WPA2-Enterprise", two new fields are shown to enter your anonymous identity and identity (username).
I tested using an ESP32.
Fixes #2749
Summary by CodeRabbit
Release Notes
New Features
✏️ Tip: You can customize this high-level summary in your review settings.