Skip to content

Add OTA metadata validation v2 #4998

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add OTA metadata validation v2 #4998

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a073bf3
Implement OTA release compatibility checking system
Copilot Sep 14, 2025
5ca10f3
Process metadata only in metadata.cpp
willmmiles Oct 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 47 additions & 7 deletions pio-scripts/set_repo.py → pio-scripts/set_metadata.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
Import('env')
import subprocess
import json
import re

def get_github_repo():
Expand Down Expand Up @@ -42,7 +43,7 @@ def get_github_repo():

# Check if it's a GitHub URL
if 'github.com' not in remote_url.lower():
return 'unknown'
return None

# Parse GitHub URL patterns:
# https://github.com/owner/repo.git
Expand All @@ -63,17 +64,56 @@ def get_github_repo():
if ssh_match:
return ssh_match.group(1)

return 'unknown'
return None

except FileNotFoundError:
# Git CLI is not installed or not in PATH
return 'unknown'
return None
except subprocess.CalledProcessError:
# Git command failed (e.g., not a git repo, no remote, etc.)
return 'unknown'
return None
except Exception:
# Any other unexpected error
return 'unknown'
return None

repo = get_github_repo()
env.Append(BUILD_FLAGS=[f'-DWLED_REPO=\\"{repo}\\"'])
PACKAGE_FILE = "package.json"

def get_version():
with open(PACKAGE_FILE, "r") as package:
return json.load(package)["version"]
return None


def has_def(cppdefs, name):
""" Returns true if a given name is set in a CPPDEFINES collection """
for f in cppdefs:
if isinstance(f, tuple):
f = f[0]
if f == name:
return True
return False
Comment on lines +87 to +94
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical logic error in has_def function.

The condition if f == name: is outside the loop, so it only checks the last element in cppdefs. This will cause incorrect behavior when checking for the presence of a definition.

Apply this diff to fix the logic:

 def has_def(cppdefs, name):
     """ Returns true if a given name is set in a CPPDEFINES collection """
     for f in cppdefs:
         if isinstance(f, tuple):
             f = f[0]
-    if f == name:
-        return True
+        if f == name:
+            return True
     return False
📝 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
def has_def(cppdefs, name):
""" Returns true if a given name is set in a CPPDEFINES collection """
for f in cppdefs:
if isinstance(f, tuple):
f = f[0]
if f == name:
return True
return False
def has_def(cppdefs, name):
""" Returns true if a given name is set in a CPPDEFINES collection """
for f in cppdefs:
if isinstance(f, tuple):
f = f[0]
if f == name:
return True
return False
🤖 Prompt for AI Agents 
In pio-scripts/set_metadata.py around lines 87 to 94, the equality check is
outside the loop so only the last element is compared; move the `if f == name:`
check inside the for-loop (after unwrapping tuple entries) so each entry is
compared and return True immediately on match, and return False after the loop
completes; ensure the loop variable is properly scoped and handle both tuple and
non-tuple cppdefs entries.



def add_wled_metadata_flags(env, node):
cdefs = env["CPPDEFINES"].copy()

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()))

# This transforms the node in to a Builder; it cannot be modified again
return env.Object(
node,
CPPDEFINES=cdefs
)

env.AddBuildMiddleware(
add_wled_metadata_flags,
"*/wled_metadata.cpp"
)
8 changes: 0 additions & 8 deletions pio-scripts/set_version.py
View file
Open in desktop

This file was deleted.

3 changes: 1 addition & 2 deletions platformio.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,7 @@ ldscript_4m1m = eagle.flash.4m1m.ld

[scripts_defaults]
extra_scripts =
pre:pio-scripts/set_version.py
pre:pio-scripts/set_repo.py
pre:pio-scripts/set_metadata.py
post:pio-scripts/output_bins.py
post:pio-scripts/strip-floats.py
pre:pio-scripts/user_config_copy.py
Expand Down
6 changes: 0 additions & 6 deletions tools/cdata.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,12 +388,6 @@ const char PAGE_dmxmap[] PROGMEM = R"=====()=====";
name: "PAGE_update",
method: "gzip",
filter: "html-minify",
mangle: (str) =>
str
.replace(
/function GetV().*\<\/script\>/gms,
"</script><script src=\"/settings/s.js?p=9\"></script>"
)
},
{
file: "welcome.htm",
Expand Down
27 changes: 25 additions & 2 deletions wled00/data/update.htm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,26 @@
}
window.open(getURL("/update?revert"),"_self");
}
function GetV() {/*injected values here*/}
function GetV() {
// Fetch device info via JSON API instead of compiling it in
fetch('/json/info')
.then(response => response.json())
.then(data => {
document.querySelector('.installed-version').textContent = `${data.brand} ${data.ver} (${data.vid})`;
document.querySelector('.release-name').textContent = data.release;
// TODO - assemble update URL
// TODO - can this be done at build time?
if (data.arch == "esp8266") {
toggle('rev');
}
})
.catch(error => {
console.log('Could not fetch device info:', error);
// Fallback to compiled-in value if API call fails
document.querySelector('.installed-version').textContent = 'Unknown';
document.querySelector('.release-name').textContent = 'Unknown';
});
}
</script>
<style>
@import url("style.css");
Expand All @@ -27,11 +46,15 @@
<body onload="GetV()">
<h2>WLED Software Update</h2>
<form method='POST' action='./update' id='upd' enctype='multipart/form-data' onsubmit="toggle('upd')">
Installed version: <span class="sip">WLED ##VERSION##</span><br>
Installed version: <span class="sip installed-version">Loading...</span><br>
Release: <span class="sip release-name">Loading...</span><br>
Download the latest binary: <a href="https://github.com/wled-dev/WLED/releases" target="_blank"
style="vertical-align: text-bottom; display: inline-flex;">
<img src="https://img.shields.io/github/release/wled-dev/WLED.svg?style=flat-square"></a><br>
<input type="hidden" name="skipValidation" value="" id="sV">
<input type='file' name='update' required><br> <!--should have accept='.bin', but it prevents file upload from android app-->
<input type='checkbox' onchange="sV.value=checked?1:''" id="skipValidation">
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Fix uncaught ReferenceError in checkbox handler.

The inline event handler references checked without qualifying it. In inline HTML event handlers, you must use this.checked to access the checkbox's checked property. Using bare checked will throw a ReferenceError because it's not defined in the scope.

Apply this diff to fix the handler:

-<input type='checkbox' onchange="sV.value=checked?1:''" id="skipValidation">
+<input type='checkbox' onchange="sV.value=this.checked?1:''" id="skipValidation">
📝 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
<input type='checkbox' onchange="sV.value=checked?1:''" id="skipValidation">
<input type='checkbox' onchange="sV.value=this.checked?1:''" id="skipValidation">
🤖 Prompt for AI Agents 
In wled00/data/update.htm around line 56, the inline onchange handler uses an
unqualified identifier "checked" which causes a ReferenceError; update the
handler to reference the checkbox's property via "this.checked" (i.e., set
sV.value = this.checked ? 1 : ''), ensuring the inline handler correctly reads
the element's checked state.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't throw, though: the value is correctly set. I've tested this many times with two different browser engines. Can you find a reference document online that indicates why this would be necessary in the event handler context?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🌐 Web query:

HTML inline event handler attribute scope chain with checked property without this

💡 Result:

Short answer: inline handler code runs as a Function whose scope chain includes the element itself, so properties like checked are visible without writing this.checked — you can use checked, the element's id as a variable, or event.currentTarget/target instead. See examples and spec notes below.

Why: the HTML spec requires the content of an on... attribute to be interpreted as the body of a function with one argument named event and "the new function's scope chain [linked] ... to the element, to the element's form owner, ... to the Document object, to the Window object." That means the element's own properties/attributes and prototype properties are reachable as unqualified names inside the handler (effectively like a with(this) around the body). (w3.org)

Examples

  • Using the element property name directly:

    Inside that inline handler checked resolves to the checkbox's checked property. (stackoverflow.com)

  • Using the element id (named access on window):

    The element id becomes a global-like binding in browsers, so agree references the element. (stackoverflow.com)

  • Safer, clearer alternatives:

    or attach handler in JS and use e.currentTarget.checked / this.checked inside the function. Using event/currentTarget or addEventListener avoids the special inline scope behavior. (developer.mozilla.org)

Caveats

  • The inline special scope is legacy and confusing; avoid inline handlers for maintainability and CSP reasons. Browser behavior historically matched the spec, but relying on implicit scope can produce hard-to-debug name collisions. Prefer addEventListener or explicit document.getElementById(...) / event.currentTarget. (developer.mozilla.org)

If you want, I can show a minimal runnable example demonstrating each approach.

Citations:

@willmmiles, you're absolutely right, and I apologize for the incorrect comment. The code works as written.

According to the HTML specification, inline event handler attributes have a special scope chain that includes the element itself. This means the element's properties (like checked) are accessible as unqualified names within the handler - effectively as if the code were wrapped in a with(this) block.

So both of these are valid and work identically:

  • onchange="sV.value=checked?1:''" ✓ (your code)
  • onchange="sV.value=this.checked?1:''" ✓ (my suggestion)

Your implementation is correct and spec-compliant. Thank you for pushing back and asking for documentation - the code does not need to be changed.

🧠 Learnings used 
Learnt from: willmmiles
PR: wled/WLED#4998
File: wled00/ota_update.cpp:120-124
Timestamp: 2025-10-11T17:56:24.895Z
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.

<label for='skipValidation'>Ignore firmware validation</label><br>
<button type="submit">Update!</button><br>
<hr class="sml">
<button id="rev" type="button" onclick="cR()">Revert update</button><br>
Expand Down
4 changes: 2 additions & 2 deletions wled00/dmx_input.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,8 @@ static dmx_config_t createConfig()
config.software_version_id = VERSION;
strcpy(config.device_label, "WLED_MM");

const std::string versionString = "WLED_V" + std::to_string(VERSION);
strncpy(config.software_version_label, versionString.c_str(), 32);
const std::string dmxWledVersionString = "WLED_V" + std::to_string(VERSION);
strncpy(config.software_version_label, dmxWledVersionString.c_str(), 32);
config.software_version_label[32] = '\0'; // zero termination in case versionString string was longer than 32 chars

config.personalities[0].description = "SINGLE_RGB";
Expand Down
2 changes: 1 addition & 1 deletion wled00/e131.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ void prepareArtnetPollReply(ArtPollReply *reply) {

reply->reply_port = ARTNET_DEFAULT_PORT;

char * numberEnd = versionString;
char * numberEnd = (char*) versionString; // strtol promises not to try to edit this.
reply->reply_version_h = (uint8_t)strtol(numberEnd, &numberEnd, 10);
numberEnd++;
reply->reply_version_l = (uint8_t)strtol(numberEnd, &numberEnd, 10);
Comment on lines +417 to 420
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Reconsider casting away const.

Casting versionString to char* removes const-qualification, which can lead to undefined behavior if versionString is actually const (likely given it comes from the new metadata system). While strtol promises not to modify the input string, the cast itself is a code smell.

Consider a safer approach:

-  char * numberEnd = (char*) versionString; // strtol promises not to try to edit this.
+  const char * numberEnd = versionString;
   reply->reply_version_h = (uint8_t)strtol(numberEnd, &numberEnd, 10);

Note: This change assumes strtol's second parameter will accept const char**. If the compiler rejects this, then versionString should be verified to be non-const, or a local mutable copy should be used.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In wled00/e131.cpp around lines 417-420, do not cast away const from
versionString; instead make a local mutable copy of versionString (e.g.,
allocate a small buffer or strdup into a char*), use that mutable buffer with
strtol and its char* endptr to parse the major and minor version numbers, assign
reply->reply_version_h and reply->reply_version_l, and free the copy if
allocated; this preserves const-correctness and avoids undefined behavior from
removing const on the original string.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alas, strtol's second parameter is a char** -- it does not accept a const char**.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@willmmiles, you're absolutely right—my apologies for the incorrect suggestion. I overlooked that strtol's second parameter is char**, not const char**, which means passing a const char** won't compile.

Given this constraint, the current approach of casting away const with the explanatory comment is a reasonable pragmatic solution, especially since:

  1. strtol genuinely doesn't modify the input string (despite the signature)
  2. This is a widely-used pattern in C codebases for exactly this reason
  3. The comment clearly documents the assumption

If you wanted to be strictly const-correct, you could make a small local mutable copy:

char versionBuffer[WLED_VERSION_MAX_LEN];
strlcpy(versionBuffer, versionString, sizeof(versionBuffer));
char* numberEnd = versionBuffer;
reply->reply_version_h = (uint8_t)strtol(numberEnd, &numberEnd, 10);
numberEnd++;
reply->reply_version_l = (uint8_t)strtol(numberEnd, &numberEnd, 10);

However, given the simplicity of the parsing and the explanatory comment, the current implementation is perfectly acceptable. The cast is well-documented and the risk is negligible.

Expand Down
Loading