Add OTA metadata validation v2 #4998
Conversation
Implement a comprehensive solution for validating a firmware before an OTA updated is committed. WLED metadata such as version and release is moved to a data structure located at near the start of the firmware binary, where it can be identified and validated. Co-authored-by: netmindz <[email protected]>
Improves cache utilization as fewer things are passed via CFLAGS to all files. In the event that no metadata is available, let the cpp file handle warning about default usage.
WalkthroughConsolidates build-time repo/version scripting into one middleware script, embeds build metadata into binaries, adds metadata extraction/validation, implements a stateful chunked OTA pipeline with server/UI integration, and removes legacy global version/release variables. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Attention recommended:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings🧬 Code graph analysis (1)pio-scripts/set_metadata.py (1)
🪛 Ruff (0.14.1)pio-scripts/set_metadata.py67-67: Consider moving this statement to an (TRY300) 75-75: Do not catch blind exception: (BLE001) 118-118: Undefined name (F821) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
🔇 Additional comments (6)
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: 7
🧹 Nitpick comments (12)
wled00/data/update.htm (2)
27-28: Address TODO comments before merging.The TODO comments suggest incomplete functionality:
- "assemble update URL"
- "can this be done at build time?"
Please resolve these items or create tracking issues for them.
Do you want me to open a new issue to track these tasks, or would you like suggestions on how to implement them?
25-26: Consider defensive checks for API response properties.While the error handling catches network failures, missing or malformed properties in the JSON response (e.g.,
data.brand,data.ver,data.release) would display "undefined" in the UI.Apply this diff to add defensive fallbacks:
-.then(data => { - document.querySelector('.installed-version').textContent = `${data.brand} ${data.ver} (${data.vid})`; - document.querySelector('.release-name').textContent = data.release; +.then(data => { + const brand = data.brand || 'Unknown'; + const ver = data.ver || 'Unknown'; + const vid = data.vid || 'Unknown'; + const release = data.release || 'Unknown'; + document.querySelector('.installed-version').textContent = `${brand} ${ver} (${vid})`; + document.querySelector('.release-name').textContent = release;pio-scripts/set_metadata.py (1)
69-77: Consider more specific exception handling.The bare
except Exception:clause at line 75 catches all exceptions, which can mask unexpected errors and make debugging difficult.Consider catching specific exceptions or at least logging the error:
except subprocess.CalledProcessError: # Git command failed (e.g., not a git repo, no remote, etc.) return None - except Exception: + except Exception as e: # Any other unexpected error + # Optionally log: print(f"Unexpected error in get_github_repo: {e}") return Nonewled00/ota_update.h (1)
42-51: Correct the handleOTAData docsComment claims the function returns a bool/string pair, but the signature is
void. Update the docblock to match the actual return type.wled00/ota_update.cpp (5)
194-234: Make metadata windowing robust and bounded (avoid false negatives and dynamic growth)Always accumulate a bounded window from start until METADATA_OFFSET+METADATA_SEARCH_RANGE before validating; then search that buffer once. This prevents edge cases where the first “crossing” chunk is large and doesn’t include bytes just before METADATA_OFFSET, and avoids repeated vector reallocs.
Example adjustment within this block:
- Before validation: append incoming data while index+len <= METADATA_OFFSET+METADATA_SEARCH_RANGE (cap buffer to that size).
- When buffer.size() >= METADATA_OFFSET+METADATA_SEARCH_RANGE, validate once using the buffer, then clear it.
This keeps memory bounded and improves reliability without increasing complexity.
236-243: Abort update immediately when validation never completesCall abort to free resources ASAP when final chunk arrives without passing validation.
Apply this diff:
if (isFinal && !context->releaseCheckPassed) { DEBUG_PRINTLN(F("OTA failed: Validation never completed")); // Don't write the last chunk to the updater: this will trip an error later context->errorMessage = F("Release check data never arrived?"); + #if defined(ESP32) + Update.abort(); + #endif return; }
145-146: Narrow the lambda captureUse [request] instead of [=] to avoid capturing unrelated locals and reduce risk.
Apply this diff:
- request->onDisconnect([=]() { endOTA(request); }); // ensures we restart on failure + request->onDisconnect([request]() { endOTA(request); }); // ensures we restart on failure
158-163: Docstring out of date with return type/semanticsComment says “Returns pointer to error message…”, but function returns pair<bool, String>. Update for clarity.
Apply this diff:
-// Returns pointer to error message, or nullptr if OTA was successful. +// Returns {done, message}. done=true when an HTTP response can be produced. +// message is empty on success.
221-225: Error text should match UI wordingUI says “Ignore release name check”; message says “Ignore firmware validation”. Align for consistency.
Apply this diff:
- context->errorMessage += F(" Enable 'Ignore firmware validation' to proceed anyway."); + context->errorMessage += F(" Enable 'Ignore release name check' to proceed anyway.");wled00/wled_metadata.cpp (3)
21-24: Compile‑time length check can misbehave if RELEASE_NAME isn’t a literalsizeof(WLED_RELEASE_NAME) only works for string literals. If it’s passed from build flags/macros that resolve to a non-literal, this may break. Consider a runtime assert/log or a static_assert on array length via a helper when literal is guaranteed.
64-72: Provide safe defaults for product/brand if integrator doesn’t define themPrevent build breaks when WLED_PRODUCT_NAME/WLED_BRAND aren’t set.
Apply this diff:
+#ifndef WLED_PRODUCT_NAME +#define WLED_PRODUCT_NAME "WLED" +#endif +#ifndef WLED_BRAND +#define WLED_BRAND "WLED" +#endif
120-128: Update the function doc to match the signatureThe comment mentions binaryData/dataSize, but the function takes a descriptor. Fix the doc to avoid confusion.
Apply this diff:
- * @param binaryData Pointer to binary file data (not modified) - * @param dataSize Size of binary data in bytes - * @param errorMessage Buffer to store error message if validation fails + * @param firmwareDescription Extracted firmware metadata to validate + * @param errorMessage Buffer to store error message if validation fails
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
pio-scripts/set_metadata.py(3 hunks)pio-scripts/set_version.py(0 hunks)platformio.ini(1 hunks)tools/cdata.js(0 hunks)wled00/data/update.htm(2 hunks)wled00/dmx_input.cpp(1 hunks)wled00/e131.cpp(1 hunks)wled00/ota_update.cpp(1 hunks)wled00/ota_update.h(1 hunks)wled00/wled.cpp(2 hunks)wled00/wled.h(1 hunks)wled00/wled_metadata.cpp(1 hunks)wled00/wled_metadata.h(1 hunks)wled00/wled_server.cpp(2 hunks)wled00/xml.cpp(0 hunks)
💤 Files with no reviewable changes (3)
- pio-scripts/set_version.py
- wled00/xml.cpp
- tools/cdata.js
🧰 Additional context used
📓 Path-based instructions (5)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/e131.cppwled00/wled_metadata.cppwled00/wled.cppwled00/ota_update.cppwled00/wled_server.cppwled00/dmx_input.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/ota_update.hwled00/wled_metadata.h
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/update.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/update.htm
🧠 Learnings (1)
📚 Learning: 2025-05-26T16:09:34.325Z
Learnt from: blazoncek
PR: wled/WLED#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/wled.cppwled00/ota_update.cppwled00/wled_server.cpp
🧬 Code graph analysis (5)
pio-scripts/set_metadata.py (1)
tools/cdata.js (1)
version(94-94)
wled00/ota_update.cpp (1)
wled00/wled_metadata.cpp (4)
findWledMetadata(82-117)findWledMetadata(82-82)shouldAllowOTA(129-157)shouldAllowOTA(129-129)
wled00/ota_update.h (1)
wled00/ota_update.cpp (8)
initOTA(140-150)initOTA(140-140)setOTAReplied(152-156)setOTAReplied(152-152)getOTAResult(159-177)getOTAResult(159-159)handleOTAData(181-257)handleOTAData(181-181)
wled00/wled_server.cpp (1)
wled00/ota_update.cpp (8)
getOTAResult(159-177)getOTAResult(159-159)initOTA(140-150)initOTA(140-140)setOTAReplied(152-156)setOTAReplied(152-152)handleOTAData(181-257)handleOTAData(181-181)
wled00/wled_metadata.h (1)
wled00/wled_metadata.cpp (4)
findWledMetadata(82-117)findWledMetadata(82-82)shouldAllowOTA(129-157)shouldAllowOTA(129-129)
🪛 Ruff (0.13.3)
pio-scripts/set_metadata.py
67-67: Consider moving this statement to an else block
(TRY300)
75-75: Do not catch blind exception: Exception
(BLE001)
116-116: Undefined name env
(F821)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
🔇 Additional comments (13)
wled00/data/update.htm (3)
46-46: LGTM!The onload handler correctly triggers the device info fetch when the page loads.
49-50: LGTM!The "Loading..." placeholders provide clear user feedback while the device info is being fetched. The class names (
installed-version,release-name) are descriptive and align with the querySelector calls in GetV().
54-54: LGTM!The hidden input pattern correctly handles the skipValidation parameter for form submission.
platformio.ini (1)
113-113: LGTM! Metadata handling consolidated.The consolidation of
set_version.pyandset_repo.pyinto a singleset_metadata.pyscript streamlines the build process and aligns with the new metadata system introduced in this PR.pio-scripts/set_metadata.py (1)
116-119: Theenvvariable is provided by PlatformIO.The static analysis warning about
envbeing undefined can be ignored. PlatformIO injects this variable into the script environment at runtime via theImport('env')statement at line 1.wled00/dmx_input.cpp (1)
58-60: LGTM! Variable renamed to avoid naming conflict.The rename from
versionStringtodmxWledVersionStringavoids potential naming conflicts with the removed global variables and makes the variable's purpose clearer.wled00/wled.cpp (2)
4-4: LGTM! Include added for new OTA system.The inclusion of
ota_update.his necessary to support the new centralized OTA update handling introduced in this PR.
177-179: LGTM! Prevents reconnect during OTA update.The addition of
!Update.isRunning()check prevents forcing a reconnect while an OTA update is in progress, which is critical for update stability.wled00/wled_server.cpp (3)
4-5: LGTM! OTA includes consolidated.The consolidation of OTA-related includes into a single
ota_update.hheader aligns with the new centralized OTA handling system.
402-415: LGTM! Cleaner OTA result handling.The new OTA result handling using
getOTAResult()is cleaner and more maintainable than the previous approach. The logic correctly:
- Checks for OTA context presence
- Handles errors with appropriate HTTP 500 responses
- Handles success with HTTP 200 and reboot messaging
- Provides clear error messages to users
417-444: LGTM! Improved OTA initialization and privilege checks.The new OTA flow is well-structured:
- Initializes OTA context on first chunk (
index == 0)- Performs comprehensive privilege checks (subnet, PIN, otaLock)
- Uses
setOTAReplied()to prevent duplicate responses- Delegates data handling to
handleOTAData()The privilege checks maintain the existing security model while being more maintainable.
Based on learnings: The OTA access control logic allowing updates from different subnets when a PIN is set is intentional, as noted in previous reviews.
wled00/wled.h (1)
197-197: wled_metadata string definitions verified
All usages of versionString, releaseString, and repoString correctly map to the new definitions in wled_metadata.h/.cpp with compatible types and remain accessible in all contexts.wled00/ota_update.cpp (1)
120-124: skipValidation is admin-protected
beginOTA() (which honors skipValidationParam) is only invoked after the POST /update handler’s same-subnet, correct-PIN and otaLock checks. No changes needed.
|
I've had a quick look through the code, looks good Can you perhaps put together some testing notes for others to try @willmmiles ? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pio-scripts/set_metadata.py (1)
105-108: Optional: Cache get_version() result to avoid redundant call.The function calls
get_version()twice (lines 106 and 108). Consider caching the result.Apply this diff to eliminate the redundant call:
if not has_def(cdefs, "WLED_REPO"): repo = get_github_repo() if repo: cdefs.append(("WLED_REPO", f"\\\"{repo}\\\"")) if not has_def(cdefs, "WLED_VERSION"): version = get_version() if version: - cdefs.append(("WLED_VERSION", get_version())) + cdefs.append(("WLED_VERSION", version))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pio-scripts/set_metadata.py(3 hunks)wled00/wled_metadata.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wled00/wled_metadata.cpp
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: willmmiles
PR: wled/WLED#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.
🧬 Code graph analysis (1)
pio-scripts/set_metadata.py (1)
tools/cdata.js (1)
version(94-94)
🪛 Ruff (0.14.1)
pio-scripts/set_metadata.py
67-67: Consider moving this statement to an else block
(TRY300)
75-75: Do not catch blind exception: Exception
(BLE001)
116-116: Undefined name env
(F821)
🔇 Additional comments (2)
pio-scripts/set_metadata.py (2)
46-77: LGTM: Semantic improvement by returning None instead of 'unknown'.The change from returning
'unknown'to returningNonefor error cases is more Pythonic and aligns well with how the function is used inadd_wled_metadata_flags(line 102:if repo:).
87-94: LGTM: Past critical issue has been resolved.The indentation of the
if f == name:check (line 92) is now correctly inside the loop, resolving the critical logic error flagged in the previous review. Each element incppdefsis now properly checked.
|
The naming of the release bins is broken, they are named None @willmmiles |
2 participants
Replaces #4960, fixes #4929.
Expand the OTA subsystem to add a framework for validating binaries before installing. Expands from #4960, squashed to make porting to 0.15 easier.
Key changes:
This implementation validates only release names. Additional validations can be added to
shouldAllowOTA()in future PRs.Summary by CodeRabbit
New Features
Improvements
Bug Fixes