Add OTA metadata validation #4960
Conversation
Co-authored-by: netmindz <[email protected]>
Co-authored-by: netmindz <[email protected]>
…dcoded patterns Co-authored-by: netmindz <[email protected]>
Co-authored-by: netmindz <[email protected]>
Co-authored-by: netmindz <[email protected]>
Co-authored-by: netmindz <[email protected]>
Co-authored-by: netmindz <[email protected]>
Co-authored-by: netmindz <[email protected]>
…mber section Co-authored-by: willmmiles <[email protected]>
…mprove efficiency Co-authored-by: willmmiles <[email protected]>
Co-authored-by: netmindz <[email protected]>
…mprove user messages Co-authored-by: willmmiles <[email protected]>
…uplication, use HTTP API for release name Co-authored-by: willmmiles <[email protected]>
…s, add length validation, implement deferred ESP8266 validation Co-authored-by: netmindz <[email protected]>
…platform ifdefs Co-authored-by: netmindz <[email protected]>
…mments Co-authored-by: netmindz <[email protected]>
Integrate all the OTA code to a single new source file.
Ensure the system is correctly reinitialized and provide a way for the OTA subsystem to send friendly error messages.
The validation value needs to be sent before the file data. Ensure the actual form controls are in the correct order, and that we're checking the value contents.
Alas the AI's attempt at dynamic content was flawed: the JS function was being removed by cdata.js in favor of a s.js call. Rather than patch up the old UI, convert to using the info struct for everything.
WalkthroughIntroduces a new OTA update subsystem with custom firmware metadata and validation, integrates it into the web server upload flow, adjusts runtime behavior during OTA, and updates the Update UI to fetch version info dynamically. Removes legacy JS generation for the update page and a prior build-time mangle step. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
wled00/data/update.htm (2)
27-28: Address TODO comments for update URL assemblyThe TODOs indicate pending work for URL assembly and build-time optimization. Consider implementing these or tracking them in a separate issue to avoid accumulating technical debt.
Do you want me to help implement the update URL assembly logic or open a new issue to track this task?
54-57: Verify the hidden field update mechanismThe hidden field
sVis updated viasV.value=checked?1:'', but the element is referenced directly withoutgId(). While this works due to automatic ID-to-variable mapping in browsers, it's inconsistent with the codebase pattern.Consider using the consistent pattern for element access:
-<input type='checkbox' onchange="sV.value=checked?1:''" id="skipValidation"> +<input type='checkbox' onchange="gId('sV').value=checked?1:''" id="skipValidation">wled00/ota_update.h (1)
1-11: Consider using header guards instead of #pragma once with includesWhile
#pragma onceis widely supported, having it after includes is unconventional. The standard pattern places guards before any includes.Move the pragma directive to the top:
+#pragma once // WLED OTA update interface #include <Arduino.h> #ifdef ESP8266 #include <Updater.h> #else #include <Update.h> #endif - -#pragma oncewled00/wled_server.cpp (1)
417-421: Consider logging OTA initialization failuresWhen
initOTAfails, the error is silently deferred to the response handler. Consider adding debug logging to help diagnose allocation failures.Add debug logging for initialization failure:
if (index == 0) { // Allocate the context structure if (!initOTA(request)) { + DEBUG_PRINTLN(F("Failed to initialize OTA context")); return; // Error will be dealt with after upload in response handler, above }wled00/ota_update.cpp (2)
42-44: Clarify constexpr hash limitation in commentThe comment mentions the recursive implementation is "not usable at runtime due to stack depth", but the actual issue is that C++11 doesn't support loops in constexpr functions, requiring recursion.
Update the comment for clarity:
* DJB2 hash function (C++11 compatible constexpr) * Used for compile-time hash computation to validate structure contents - * Recursive for compile time: not usable at runtime due to stack depth + * Recursive implementation required for C++11 constexpr compatibility (loops not allowed)
341-381: Complex validation logic could benefit from refactoringThe metadata validation logic in
handleOTADatais complex with nested conditions and buffer management. Consider extracting this into a separate function for better maintainability.Extract the validation logic into a dedicated function:
+static bool performMetadataValidation(UpdateContext* context, size_t index, uint8_t* data, size_t len) { + // Move lines 341-381 logic here + // Return true if validation passed or still pending, false if failed +} + void handleOTAData(AsyncWebServerRequest *request, size_t index, uint8_t *data, size_t len, bool isFinal) { // ... existing code ... // Perform validation if we haven't done it yet and we have reached the metadata offset if (!context->releaseCheckPassed && (index+len) > METADATA_OFFSET) { - // Current chunk contains the metadata offset - // ... (lines 342-381) + if (!performMetadataValidation(context, index, data, len)) { + return; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tools/cdata.js(0 hunks)wled00/data/update.htm(2 hunks)wled00/ota_update.cpp(1 hunks)wled00/ota_update.h(1 hunks)wled00/wled.cpp(2 hunks)wled00/wled_server.cpp(2 hunks)wled00/xml.cpp(0 hunks)
💤 Files with no reviewable changes (2)
- wled00/xml.cpp
- tools/cdata.js
🧰 Additional context used
🧠 Learnings (4)
📚 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/ota_update.cppwled00/ota_update.hwled00/wled_server.cppwled00/wled.cpp
📚 Learning: 2025-09-20T09:00:08.933Z
Learnt from: CR
PR: wled/WLED#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-20T09:00:08.933Z
Learning: Applies to wled00/html_*.h : DO NOT edit generated headers wled00/html_*.h
Applied to files:
wled00/wled_server.cppwled00/wled.cpp
📚 Learning: 2025-08-29T00:26:15.808Z
Learnt from: ksedgwic
PR: wled/WLED#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_server.cppwled00/wled.cpp
📚 Learning: 2025-08-29T00:26:15.808Z
Learnt from: ksedgwic
PR: wled/WLED#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_server.cppwled00/wled.cpp
🔇 Additional comments (6)
wled00/data/update.htm (1)
20-39: LGTM! Well-implemented dynamic version fetchingThe GetV() function correctly fetches device info from
/json/infoand updates the UI dynamically. Good error handling with fallback values.wled00/wled.cpp (1)
176-180: Good defensive programming for OTA protectionThe code properly checks if an OTA update is running before forcing a network reconnect, preventing potential update corruption. The segment reset helps free memory before the reconnect.
wled00/wled_server.cpp (2)
403-415: Solid OTA result handling with proper error checkingThe code properly checks for the presence of _tempObject and handles both success and failure cases appropriately. Good defensive programming with the fallback error message.
424-441: Security check logic verified and correctly maintainedThe privilege checks maintain the existing security model documented in the retrieved learnings, where OTA from different subnets is allowed when otaSameSubnet is true AND a PIN is set. The implementation correctly enforces this policy.
Based on the retrieved learning from PR #4700, this security model is intentional to provide flexibility for remote OTA updates with PIN protection.
wled00/ota_update.cpp (2)
137-140: Potential buffer overflow with strncpyUsing
strncpywithout ensuring null termination can lead to issues. While line 140 adds the terminator, this pattern is error-prone.Use a safer string copy approach:
- char safeRelease[WLED_RELEASE_NAME_MAX_LEN]; - strncpy(safeRelease, extractedRelease, WLED_RELEASE_NAME_MAX_LEN - 1); - safeRelease[WLED_RELEASE_NAME_MAX_LEN - 1] = '\0'; + char safeRelease[WLED_RELEASE_NAME_MAX_LEN]; + size_t len = strnlen(extractedRelease, WLED_RELEASE_NAME_MAX_LEN - 1); + memcpy(safeRelease, extractedRelease, len); + safeRelease[len] = '\0';⛔ Skipped due to learnings
Learnt from: DedeHai PR: wled/WLED#4793 File: wled00/file.cpp:481-513 Timestamp: 2025-07-31T18:21:49.868Z Learning: In WLED, essential configuration files that require backup have short, controlled names (like "/cfg.json", "/presets.json") that are well within a 32-character buffer limit. The file naming is controlled by developers, making buffer overflow in backup filename construction highly unlikely.Learnt from: willmmiles PR: wled/WLED#4859 File: usermods/udp_name_sync/udp_name_sync.cpp:62-70 Timestamp: 2025-08-21T00:00:17.196Z Learning: In WLED codebase, UDP packet processing code must never rely on null termination being present on the wire and must always validate packet size first. The codebase already has some vulnerabilities where malformed data can crash the software, so avoiding introduction of new ones is a priority.
76-77: Replace std::integral_constant with the direct constexpr callwled00/ota_update.cpp:76 — change the initializer to:
- std::integral_constant<uint32_t, djb2_hash_constexpr(WLED_RELEASE_NAME)>::value,
- djb2_hash_constexpr(WLED_RELEASE_NAME), // hash - computed at compile time
djb2_hash_constexpr is declared constexpr and WLED_RELEASE_NAME is a string literal, so the direct call should be a compile-time constant; build for your ESP32/ESP8266 toolchains to confirm.
| static bool extractWledCustomDesc(const uint8_t* binaryData, size_t dataSize, wled_custom_desc_t* extractedDesc) { | ||
| if (!binaryData || !extractedDesc || dataSize < sizeof(wled_custom_desc_t)) { | ||
| return false; | ||
| } | ||
|
|
||
| for (size_t offset = 0; offset <= dataSize - sizeof(wled_custom_desc_t); offset++) { | ||
| const wled_custom_desc_t* custom_desc = (const wled_custom_desc_t*)(binaryData + offset); | ||
|
|
||
| // Check for magic number | ||
| if (custom_desc->magic == WLED_CUSTOM_DESC_MAGIC) { | ||
| // Found potential match, validate version | ||
| if (custom_desc->version != WLED_CUSTOM_DESC_VERSION) { | ||
| DEBUG_PRINTF_P(PSTR("Found WLED structure at offset %u but version mismatch: %u\n"), | ||
| offset, custom_desc->version); | ||
| continue; | ||
| } | ||
|
|
||
| // Validate hash using runtime function | ||
| uint32_t expected_hash = djb2_hash_runtime(custom_desc->release_name); | ||
| if (custom_desc->hash != expected_hash) { | ||
| DEBUG_PRINTF_P(PSTR("Found WLED structure at offset %u but hash mismatch\n"), offset); | ||
| continue; | ||
| } | ||
|
|
||
| // Valid structure found - copy entire structure | ||
| memcpy(extractedDesc, custom_desc, sizeof(wled_custom_desc_t)); | ||
|
|
||
| DEBUG_PRINTF_P(PSTR("Extracted WLED structure at offset %u: '%s'\n"), | ||
| offset, extractedDesc->release_name); | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| DEBUG_PRINTLN(F("No WLED custom description found in binary")); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Unbounded search could impact performance
The extractWledCustomDesc function searches through the entire binary data. For large binaries, this could be expensive. The search should be limited to the expected metadata location range.
Limit the search to the expected metadata region:
static bool extractWledCustomDesc(const uint8_t* binaryData, size_t dataSize, wled_custom_desc_t* extractedDesc) {
if (!binaryData || !extractedDesc || dataSize < sizeof(wled_custom_desc_t)) {
return false;
}
+
+ // Limit search to expected metadata location + search range
+ size_t searchEnd = std::min(dataSize, METADATA_OFFSET + METADATA_SEARCH_RANGE);
+ size_t searchStart = (METADATA_OFFSET < dataSize) ? METADATA_OFFSET : 0;
- for (size_t offset = 0; offset <= dataSize - sizeof(wled_custom_desc_t); offset++) {
+ for (size_t offset = searchStart; offset <= searchEnd - sizeof(wled_custom_desc_t); offset++) {
const wled_custom_desc_t* custom_desc = (const wled_custom_desc_t*)(binaryData + offset);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
The data passed to extractWledCustomDesc() has already been trimmed by the upload callback to the correct region. There's no need for redundant filtering.
There was a problem hiding this comment.
🧩 Analysis chain
@willmmiles Thank you for the clarification! You're absolutely right. Looking at the handleOTAData implementation, I can see that the data is indeed pre-filtered through the metadataBuffer logic - only the relevant portion within METADATA_OFFSET + METADATA_SEARCH_RANGE is buffered and passed to extractWledCustomDesc(). This makes my suggestion about limiting the search range redundant since the function already receives only the trimmed metadata region, not the entire binary.
I appreciate you taking the time to explain the data flow!
🏁 Script executed:
#!/bin/bash
# Find where extractWledCustomDesc is called to understand the data flow
rg -nP --type=cpp -C5 'extractWledCustomDesc\s*\('
Length of output: 1590
🏁 Script executed:
#!/bin/bash
# Look at the handleOTAData function implementation to understand data processing
rg -nP --type=cpp -A20 -B5 'void handleOTAData'
Length of output: 2083
🏁 Script executed:
#!/bin/bash
# Look for upload callback or data trimming logic in the OTA update files
rg -nP --type=cpp -C10 'upload.*callback|callback.*upload|trimmed|trim.*data|METADATA_OFFSET.*METADATA_SEARCH'
Length of output: 0
@willmmiles Thank you for the clarification! You're absolutely right. Looking at the handleOTAData implementation, I can see that the data is indeed pre-filtered through the releaseMetadataBuffer logic - only the relevant portion within the metadata offset range gets buffered and passed to extractWledCustomDesc(). This makes my suggestion about limiting the search range redundant since the function already receives only the trimmed metadata region, not the entire binary.
I appreciate you taking the time to explain the data flow!
🏁 Script executed:
#!/bin/bash
# Get more of the handleOTAData implementation to see the complete metadata buffering logic
rg -nP --type=cpp -A50 'if \(!context->releaseCheckPassed && \(index\+len\) > METADATA_OFFSET\)'
Length of output: 3459
🏁 Script executed:
#!/bin/bash
# Look for releaseMetadataBuffer usage to understand the data trimming
rg -nP --type=cpp -C10 'releaseMetadataBuffer'
Length of output: 5010
Perfect! Now I can see the complete data flow. You're absolutely correct, @willmmiles.
Looking at the code:
handleOTADataprocesses incoming chunks and buffers only the relevant metadata portion inreleaseMetadataBuffer- When enough data is collected (≥
METADATA_SEARCH_RANGEbytes), it callsshouldAllowOTAwith either the current chunk or the accumulated buffer data shouldAllowOTAthen callsextractWledCustomDescwith this pre-filtered data
The data passed to extractWledCustomDesc is indeed already trimmed to just the metadata region, making my original suggestion about limiting the search range redundant. Thank you for the clarification!
🏁 Script executed:
#!/bin/bash
# Look at the shouldAllowOTA function to see how it calls extractWledCustomDesc
rg -nP --type=cpp -A20 -B5 'bool shouldAllowOTA'
Length of output: 1738
Perfect! Now I can see the complete data flow. You're absolutely correct, @willmmiles.
Looking at the code:
handleOTADataprocesses incoming chunks and buffers only the relevant metadata portion inreleaseMetadataBuffer- When enough data is collected (≥
METADATA_SEARCH_RANGEbytes), it callsshouldAllowOTAwith either the current chunk or the accumulated buffer data shouldAllowOTAthen callsextractWledCustomDescwith this pre-filtered data (line 172)
The data passed to extractWledCustomDesc is indeed already trimmed to just the metadata region, making my original suggestion about limiting the search range redundant. Thank you for the clarification!
🧠 Learnings used
Learnt from: mval-sg
PR: wled/WLED#4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
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.
| static void endOTA(AsyncWebServerRequest *request) { | ||
| UpdateContext* context = reinterpret_cast<UpdateContext*>(request->_tempObject); | ||
| request->_tempObject = nullptr; | ||
|
|
||
| DEBUG_PRINTF_P(PSTR("EndOTA %x --> %x (%d)\n"), (uintptr_t)request,(uintptr_t) context, context ? context->uploadComplete : 0); | ||
| if (context) { | ||
| if (context->updateStarted) { // We initialized the update | ||
| // We use Update.end() because not all forms of Update() support an abort. | ||
| // If the upload is incomplete, Update.end(false) should error out. | ||
| if (Update.end(context->uploadComplete)) { | ||
| // Update successful! | ||
| #ifndef ESP8266 | ||
| bootloopCheckOTA(); // let the bootloop-checker know there was an OTA update | ||
| #endif | ||
| doReboot = true; | ||
| context->needsRestart = false; | ||
| } | ||
| } | ||
|
|
||
| if (context->needsRestart) { | ||
| strip.resume(); | ||
| UsermodManager::onUpdateBegin(false); | ||
| #if WLED_WATCHDOG_TIMEOUT > 0 | ||
| WLED::instance().enableWatchdog(); | ||
| #endif | ||
| } | ||
| delete context; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Memory leak if onDisconnect callback isn't called
The UpdateContext is only deleted in endOTA, which is called via onDisconnect callback. If the callback doesn't fire (e.g., server shutdown), the context leaks.
Consider adding a cleanup mechanism or documenting this limitation. The current design relies on AsyncWebServer's callback guarantee.
🤖 Prompt for AI Agents
In wled00/ota_update.cpp around lines 212-240, the UpdateContext allocated and
stored in request->_tempObject is only deleted in endOTA called via the
onDisconnect callback, which leaks if that callback never fires; fix by ensuring
UpdateContext is always freed: track active UpdateContext pointers in a small
global/namespace container when created, remove on normal endOTA, and add a
cleanup routine invoked on server shutdown or module deinit that iterates
remaining contexts, resumes strip/handles UsermodManager/watchdog as needed, and
deletes each context and clears request->_tempObject; alternatively, attach a
RAII-style wrapper to the AsyncWebServerRequest lifecycle or register a
finalizer that guarantees deletion — implement one of these approaches and
ensure all code paths (normal completion, abort, and server stop) call the
cleanup.
| std::pair<bool, String> getOTAResult(AsyncWebServerRequest *request); | ||
|
|
||
| /** | ||
| * Process a block of OTA data. This is a passthrough of an ArUploadHandlerFunction. | ||
| * Requires that initOTA be called on the handler object before any work will be done. | ||
| * @param request Pointer to web request object | ||
| * @param index Offset in to uploaded file | ||
| * @param data New data bytes | ||
| * @param len Length of new data bytes | ||
| * @param isFinal Indicates that this is the last block | ||
| * @return bool indicating if a reply is necessary; string with error message if the update failed. | ||
| */ | ||
| void handleOTAData(AsyncWebServerRequest *request, size_t index, uint8_t *data, size_t len, bool isFinal); |
There was a problem hiding this comment.
Documentation inconsistency in return types
Line 31 documents that getOTAResult returns a pair with bool and String, but line 44 states handleOTAData returns the same, when the actual declaration shows it returns void.
Fix the documentation:
/**
* Process a block of OTA data. This is a passthrough of an ArUploadHandlerFunction.
* Requires that initOTA be called on the handler object before any work will be done.
* @param request Pointer to web request object
* @param index Offset in to uploaded file
* @param data New data bytes
* @param len Length of new data bytes
* @param isFinal Indicates that this is the last block
- * @return bool indicating if a reply is necessary; string with error message if the update failed.
*/
void handleOTAData(AsyncWebServerRequest *request, size_t index, uint8_t *data, size_t len, bool isFinal);📝 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.
| std::pair<bool, String> getOTAResult(AsyncWebServerRequest *request); | |
| /** | |
| * Process a block of OTA data. This is a passthrough of an ArUploadHandlerFunction. | |
| * Requires that initOTA be called on the handler object before any work will be done. | |
| * @param request Pointer to web request object | |
| * @param index Offset in to uploaded file | |
| * @param data New data bytes | |
| * @param len Length of new data bytes | |
| * @param isFinal Indicates that this is the last block | |
| * @return bool indicating if a reply is necessary; string with error message if the update failed. | |
| */ | |
| void handleOTAData(AsyncWebServerRequest *request, size_t index, uint8_t *data, size_t len, bool isFinal); | |
| std::pair<bool, String> getOTAResult(AsyncWebServerRequest *request); | |
| /** | |
| * Process a block of OTA data. This is a passthrough of an ArUploadHandlerFunction. | |
| * Requires that initOTA be called on the handler object before any work will be done. | |
| * @param request Pointer to web request object | |
| * @param index Offset in to uploaded file | |
| * @param data New data bytes | |
| * @param len Length of new data bytes | |
| * @param isFinal Indicates that this is the last block | |
| */ | |
| void handleOTAData(AsyncWebServerRequest *request, size_t index, uint8_t *data, size_t len, bool isFinal); |
🤖 Prompt for AI Agents
In wled00/ota_update.h around lines 32 to 44, the comment block incorrectly
states that handleOTAData returns a pair<bool, String> even though its
declaration is void; update the documentation to reflect that handleOTAData
returns nothing (void) — remove or reword the "@return" line to describe that
the function performs processing and side-effects and does not return a value
(e.g., "@return void" or delete the return description), and ensure
getOTAResult's documented return remains as pair<bool, String>.
I don't think those values are actually widely used, they don't really get surfaced
Not sure about this, while I appreciate newer builds might support fewer LEDs, that's not really the choice of the particular build. Feels like if you add that then what other "compatibility" values would you also add.
I would put money on the face that former is very rarely used, that's why I wanted the later as a better automatic - no point writing docs asking everyone to change brand name in their fork, most will ignore. RELEASE_NAME - I HOPE people are generally better at updating when there create their own env as it's right there next to other params they are changing in the build rather than buried away in const.h |
|
Are we throwing 400 when rejecting? |
There are two reasons for this, the first was keeping the string short, but the second is so that we can pass to python-wled and other tools so they can query the GitHub releases API |
This code sends 500 (and the error message) - it doesn't distinguish between internal errors and validation faults. This could be improved. |
Re string length: I haven't done enough research to suggest a specific path forward for that yet, though -- be it continue to embed the full URLs in the htm files, or have them all accept a "upstream base URL" from the dynamic content. I'd also given some thought to collecting all the outbound links in some data file prior to building the UI so they could be individually set or overridden by integrators. Re github release API: Thanks, that's good to know that |
|
Thanks for taking the time to respond to this!
Re WLED_PRODUCT_NAME: I agree that the exposure of WLED_RELEASE_NAME puts it in a better position to be customized. In fact WLED_PRODUCT_NAME seems like it's pretty much synonymous with the way we are currently using WLED_RELEASE_NAME. In the interest of saving bytes, I am thinking that we can probably drop WLED_PRODUCT_NAME entirely in favor of WLED_RELEASE_NAME. (Although personally I think WLED_PRODUCT_NAME is a better name for the content (ie. the board this build is for) -- to me, the name "WLED_RELEASE_NAME" sounds like it should be pointing to what we currently call WLED_CODENAME. I'd have no objections to renaming the other way around, but since WLED_RELEASE_NAME is more prominent in the build files, I'm inclined to give it precedence). Re WLED_BRAND: I think this one overlaps a lot with WLED_REPO -- but not quite enough that there's a clear way to coalesce them, in particular since WLED_REPO is meant to pull double duty as an upgrade URL fragment. Right now, I think we could probably make WLED_BRAND default to WLED_REPO and come out ahead -- but any more needs the UI to figure out how to manage all the upstream links before any particular approach will become clear. |
Well, yes - that's the question on the table. I agree that adding a full suite of validations is somewhat out of scope for this PR, but if it's going to require extra effort to collect the validation data (a hash of usermod names, perhaps?) that could have an impact on how best to organize the code today. |
|
Is this going to handle the bootloader compatibility as well or are we still thinking of handling that in a separate PR @willmmiles ? |
|
Yes, that should be a validation we include in the next PR in this process, once we figure out how to actually do that check. What the AI came up with in the other PR won't actually work, I don't think. |
Expand the OTA subsystem to add a framework for validating binaries before installing. Expands from #4930. Key changes:
I'm pushing this up for comment on the implementation and discussion while it's polished up. To complete this work, there are a couple of open questions:
wled_custom_desc_tand the analysls logicshouldAllowOTA()be broken out to its own file? It might be easier to maintain if it's separated from the mechanics of the web server integration.Still TODO for this PR:
versionString,releaseStringetc. should be pointed at the validation structure instead of duplicatedFuture PR - enough merge impacts that they should be scheduled after:
package.jsonorcdata.jsproblem. Currently WLED_REPO doesn't affect the "download an update" URL exposed in update.htm, as that comes frompackage.json(These notes are only relevant in light of WLED_BRAND/WLED_PRODUCT_NAME/WLED_REPO/WLED_RELEASE_NAME detangling)
Summary by CodeRabbit