Add OTA metadata validation v2 #4998

willmmiles · 2025-10-11T14:56:29Z

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:

Adds a new metadata struct placed in a section where it is guaranteed to appear early in the .bin file
When performing an OTA update, validate that the release agrees with the current value (prevents ETH -> non-ETH downgrades, etc.)
Add a tickbox to the update UI to override checking
Print release info on update page.
Convert update page to using JSON API for retrieving metadata
Fix races and cleanup handling of OTA web requests

This implementation validates only release names. Additional validations can be added to shouldAllowOTA() in future PRs.

Summary by CodeRabbit

New Features
- Robust OTA update flow with per-request validation and clearer success/error responses.
- Option to ignore firmware validation during OTA.
- Update page now auto-fetches and displays device brand, version, and release (shows "Loading..." while fetching).
Improvements
- Prevents forced reconnects while an OTA is active.
- Unified, more reliable OTA upload handling across platforms.
- Build now embeds repository and version metadata for OTA validation.
Bug Fixes
- Fixed inconsistent update-page script/version handling.

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.

coderabbitai · 2025-10-11T14:56:37Z

Walkthrough

Consolidates repo/version build scripting into a single middleware, embeds build metadata into firmware, adds metadata extraction/validation, implements a stateful chunked OTA pipeline with server/UI integration, and removes legacy global version/release variables.

Changes

Cohort / File(s)	Summary
Build scripts & config `pio-scripts/set_metadata.py`, `pio-scripts/set_version.py`, `platformio.ini`	Replaces two pre-scripts with `set_metadata.py` (middleware-style CPPDEFINES injection for repo/version), removes the old `set_version.py`, and updates `platformio.ini` to call the new script.
Build metadata core `wled00/wled_metadata.h`, `wled00/wled_metadata.cpp`, `wled00/wled.h`	Adds packed `wled_metadata_t`, `WLED_BUILD_DESCRIPTION` section, compile-time/runtime hash helpers, `findWledMetadata` and `shouldAllowOTA`, exposes repo/product/brand strings, and removes legacy global version/release/repo variables.
OTA implementation & integration `wled00/ota_update.h`, `wled00/ota_update.cpp`, `wled00/wled_server.cpp`, `wled00/wled.cpp`	Introduces AsyncWebServer-based OTA context and handlers (init, chunk handling, result reporting), performs firmware metadata validation during upload, centralizes OTA flow, and prevents forcing reconnect while OTA is running.
OTA UI & XML/tooling `wled00/data/update.htm`, `wled00/xml.cpp`, `tools/cdata.js`	`update.htm` queries `/json/info`, displays dynamic installed/version/release info and an "Ignore firmware validation" checkbox; removes SUBPAGE_UPDATE XML generation and a PAGE_update mangle in `tools/cdata.js`.
Release packaging `pio-scripts/output_bins.py`	Switches release naming to read version from `package.json` instead of relying on CPP defines.
Minor/compatibility fixes `wled00/dmx_input.cpp`, `wled00/e131.cpp`	Renames local DMX version variable to `dmxWledVersionString`; fixes pointer cast for safe strtol usage.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Attention recommended:

wled00/ota_update.cpp — concurrency, memory/watchdog handling, Update/Updater APIs, chunked write/finalize, error propagation, and reboot behavior.
wled00/wled_metadata.cpp / wled00/wled_metadata.h — binary section placement, packed struct layout, constexpr/runtine hash correctness, and PROGMEM/FlashStringHelper usage.
wled00/wled_server.cpp — new request->_tempObject lifecycle, auth/subnet checks order, and changed HTTP error/success responses.
pio-scripts/set_metadata.py / pio-scripts/output_bins.py — middleware injection correctness across build targets and reading version from package.json.
UI/tooling interactions: wled00/data/update.htm, wled00/xml.cpp, and tools/cdata.js — ensure consistent UI behavior and generated assets.

Possibly related PRs

Revert disable OTA logic & optional Arduino OTA #4748 — Overlaps OTA-related code paths and handlers; likely touches similar server/OTA flow areas.
"Unlimited" buses #4529 — Modifies getSettingsJS/XML generation; touches the same XML/UI control paths changed here.

Suggested reviewers

netmindz
blazoncek
DedeHai

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title "Add OTA metadata validation v2" is concise and directly related to the primary change in the pull request. It accurately summarizes the main objective of implementing a metadata validation framework for OTA updates, with the "v2" notation appropriately indicating this is a revised version. The title is specific enough that a reviewer scanning the commit history would understand the core purpose of this changeset.
Linked Issues Check	✅ Passed	All primary requirements from issue #4929 are addressed in the implementation. The PR introduces metadata structure and extraction mechanisms (wled_metadata.h/cpp, findWledMetadata) to validate release names from binary data [#4929], implements shouldAllowOTA() to reject incompatible releases [#4929], updates the UI with a checkbox for overriding validation checks (update.htm) [#4929], and modifies the build process to embed accessible metadata in the binary via the new set_metadata.py script [#4929]. The OTA handling pipeline (ota_update.h/cpp) integrates validation with request-based state management, and wled_server.cpp routes through this new validation framework.
Out of Scope Changes Check	✅ Passed	All changes are directly related to implementing the OTA metadata validation framework and necessary refactoring. The global variable removals from wled.h and corresponding additions in wled_metadata.h reflect the migration of version-related declarations to a centralized metadata structure. Updates to dmx_input.cpp and e131.cpp adjust local variable references due to the removal of global versionString, and xml.cpp removes legacy SUBPAGE_UPDATE handling that is superseded by the new JSON-based update page. The tools/cdata.js change removes outdated script injection logic no longer needed with the refactored update interface.
Docstring Coverage	✅ Passed	Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

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 None
wled00/ota_update.h (1)

42-51: Correct the handleOTAData docs

Comment 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 completes

Call 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 capture

Use [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/semantics

Comment 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 wording

UI 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 literal

sizeof(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 them

Prevent 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 signature

The 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

📥 Commits

Reviewing files that changed from the base of the PR and between 7f1f986 and 5ca10f3.

📒 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