Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Dec 23, 2025

Solution: Fix OTA Lock Password Feedback Issue ✅

Problem

When users tried to modify OTA lock settings with incorrect password, the UI misleadingly showed "Security settings saved." even though settings were NOT saved.

Solution (Final)

Return a 401 status code immediately when password validation fails AND user attempts to change OTA-related settings, BEFORE processing any other operations:

  1. Modified set.cpp:
    • Moved password validation to the beginning of the security settings handler
    • Check if any OTA-related settings (NO, OW, AO, SU) are being changed
    • Return 401 error BEFORE processing any other changes (PIN, etc.) to prevent partial saves
    • Only return 401 if OTA is locked, password is incorrect, AND user tried to modify OTA settings
    • Allows users to change other security settings (like PIN) without error when they don't touch OTA settings

User Experience

  • Before: "Security settings saved." (misleading when password wrong)
  • After (incorrect password + OTA change attempt): "Error" / "Password incorrect" with 401 status - NO partial saves
  • After (incorrect password + no OTA change): Settings saved normally (PIN, etc. can be changed)

Technical Improvement

  • Correct HTTP pattern: 401 only returned when authentication is actually needed
  • No false positives: Users can change non-OTA settings without requiring OTA password
  • No partial saves: Password validation happens BEFORE any other operations
  • No global state: Error returned within same request context

Testing Completed

  • Web UI build successful
  • All unit tests pass (16/16)
  • Password validation checked before any operations performed
Original prompt

This section details on the original issue you should resolve

<issue_title>Cannot update WLED on Quinled dig-uno, OTA permanently locked</issue_title>
<issue_description>### What happened?

I'm trying to update my Quinled Dig-Uno from version 0.15.0 to 0.15.2. Initially through HA, since that's what prompted me to update. However, HA cannot update the device and the report stays.

So instead I went to the web UI for this device and tried updating the device from there. This also fails with the message Please unlock OTA in security settings!

And that is where it gets weird. When I do go to settings -> Security & updates the Lock wireless (OTA) software update checkbox is checked, so I uncheck it and save the page.

Then I go back to the main screen and on the Info tab, try the update WLED button again. And here I get the exact same message. Please unlock OTA in security settings!

It seems that the lock setting for OTA updates is not saved and is permanently on. The Manual OTA Update button on the security page has the same problem and won't work, because this lock cannot be removed.

Apart from flashing the thing over USB, is there any way to unlock this OTA update feature so that I can update the device?

To Reproduce Bug

  • Attempt update, either from the info screen or the settings -> security screen
  • OTA lock is in place so update will fail
  • disable the lock in security screen and save
  • Attempt the update again: lock has been restored or was never removed, so updating still fails.

Expected Behavior

Once OTA lock checkbox is cleared and the page is saved, the lock state is actually updated and software updating can commence.

Install Method

Binary from WLED.me

What version of WLED?

0.15.0

Which microcontroller/board are you seeing the problem on?

ESP32

Relevant log/trace output

I don't know where to get these from.

Anything else?

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct</issue_description>

<agent_instructions>This issue revealed that:

If the Lock wireless (OTA) software update box is checked, the correct password must be entered in the password box to save unchecking it. Unfortunately, if the correct password is not entered, the UI provides no helpful feedback - it just says "saved" anyways and ignores the attempt to change the security settings.
please propose a solution that uses minimal code addition.</agent_instructions>

Comments on the Issue (you are @copilot in this section)

@DedeHai can you please provide your config.json file? @netmindz Please can you also confirm you are trying to do the update from the same network as the WLED device, check the IP address of your laptop/phone/pc and the WLED device are the same for the first 3 groups of numbers @willmmiles > And that is where it gets weird. When I do go to `settings -> Security & updates` the `Lock wireless (OTA) software update` checkbox is checked, so I uncheck it and save the page.

@DutchessNicole If the Lock wireless (OTA) software update box is checked, the correct password must be entered in the password box to save unchecking it. Unfortunately, if the correct password is not entered, the UI provides no helpful feedback - it just says "saved" anyways and ignores the attempt to change the security settings. (I'd class that as a bug for sure - definitely something we can work on.)

I'm inferring that you weren't expecting the lock to be enabled, and consequently don't have a password to put in. The default OTA password is "wledota"; maybe try that. If that doesn't work, unfortunately recovering a lost password is (intentionally) rather difficult. If this is the case, you may need to back up your config and presets and factory reset your device to unstick it. (If you try that, I also suggest editing the config backup to set { "ota": { "lock": false } } to avoid having the backup restore cause the same problem.)

As far as the "lock OTA" box being unexpectedly set - maybe it was ticked accidentally, maybe some bad config was pushed at some point, maybe some other bug -- hard to know. :(

(Re the subnet questions: 0.16 (the current development branch/nightlies) adds a security feature where, by default, WLED will no longer accept OTAs from other subnets, as a weak but cheap defense for those devices exposed when they shouldn't be. This feature can be disabled via the security settings - you'll probably want to adjust if or when you update to that version.)</comment_new>
<comment_new>@willmmiles
@dimmas375 It looks like you're using the ESP02 build, which is for an ESP8266 with only 2MB of flash. It might not be possible to OTA update those devices as the firmware may be too big to store both the new...


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 23, 2025

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.


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

Copilot AI and others added 2 commits December 23, 2025 06:05
Co-authored-by: DedeHai <6280424+DedeHai@users.noreply.github.com>
Co-authored-by: DedeHai <6280424+DedeHai@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix OTA update lock issue on Quinled Dig-Uno Add password validation feedback for OTA lock security settings Dec 23, 2025
Copilot AI requested a review from DedeHai December 23, 2025 06:10
wled00/set.cpp Outdated

if (pwdCorrect) //allow changes if correct pwd or no ota active
{
otaPasswordIncorrect = false; // clear flag on successful save
Copy link
Collaborator

Choose a reason for hiding this comment

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

invert the logic, call it "otaPassCorrect"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in commit b7485e2. Renamed flag to otaPassCorrect with inverted logic.

if (doReboot) strcpy_P(s2, PSTR("Rebooting, please wait ~10 seconds..."));
else if (otaPwdErr) {
strcpy_P(s, PSTR("Settings NOT saved!"));
strcpy_P(s2, PSTR("Incorrect password. Enter correct OTA password to change OTA lock settings."));
Copy link
Collaborator

Choose a reason for hiding this comment

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

use "Error" for s and "Password incorect" for s2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in commit b7485e2. Changed to "Error" / "Password incorrect".

@@ -0,0 +1 @@
. No newline at end of file
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove this leftover file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in commit b7485e2. Removed the leftover symlink.

@DedeHai
Copy link
Collaborator

DedeHai commented Dec 23, 2025

@copilot please update according to review comments

…ror message, remove leftover file

Co-authored-by: DedeHai <6280424+DedeHai@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Dec 23, 2025

@copilot please update according to review comments

All review comments have already been addressed in commit b7485e2:

  • ✅ Flag renamed to otaPassCorrect with inverted logic
  • ✅ Error message changed to "Error" / "Password incorrect"
  • ✅ Leftover _codeql_detected_source_root file removed

No further changes are needed.

@DedeHai DedeHai marked this pull request as ready for review December 23, 2025 10:44
@DedeHai DedeHai requested a review from willmmiles December 23, 2025 10:44
@DedeHai
Copy link
Collaborator

DedeHai commented Dec 23, 2025

tested, works as intended

Copy link
Member

@willmmiles willmmiles left a comment

Choose a reason for hiding this comment

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

This is not the correct approach - we should not be adding a global variable that makes one HTTP request change the result of another. If a request is denied due to an incorrect password, it should return a 401.

@DedeHai
Copy link
Collaborator

DedeHai commented Dec 23, 2025

@copilot please update according to the requested change.

…flag

Co-authored-by: DedeHai <6280424+DedeHai@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Dec 23, 2025

@copilot please update according to the requested change.

Done in commit 38b159d. Removed global flag and now return 401 status immediately when password is incorrect, within the same HTTP request context.

Copilot AI changed the title Add password validation feedback for OTA lock security settings Return 401 on incorrect OTA password instead of misleading success message Dec 23, 2025
Copilot AI requested a review from DedeHai December 23, 2025 18:51
wled00/set.cpp Outdated
aOtaEnabled = request->hasArg(F("AO"));
#endif
otaSameSubnet = request->hasArg(F("SU"));
} else if (otaLock) {
Copy link
Member

Choose a reason for hiding this comment

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

This is better, but we should only return failure if any of the OTA-related values have been changed without a correct password. We do not want to return 401s if none of these values were altered. For example, the code as written would generate errors to a user who was enabling a settings PIN, even if they didn't attempt to make any OTA settings changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@copilot please update

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in commit 3e5646d. Now checks if any OTA-related settings (NO, OW, AO, SU) are being changed before returning a 401 error. Users can modify other security settings (like PIN) without requiring the OTA password when they don't attempt to change OTA settings.

Co-authored-by: DedeHai <6280424+DedeHai@users.noreply.github.com>
Copy link
Member

@willmmiles willmmiles left a comment

Choose a reason for hiding this comment

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

We're almost there. @copilot Can you please make the requested changes?

wled00/set.cpp Outdated
otaSameSubnet = request->hasArg(F("SU"));
} else if (otaLock && otaSettingsChanged) {
// If OTA is locked and password is incorrect AND user tried to change OTA settings, return error immediately
serveMessage(request, 401, F("Error"), F("Password incorrect"), 254);
Copy link
Member

Choose a reason for hiding this comment

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

If we're going to fail out a request, this must be checked and handled before any other operations are performed. Otherwise the user could be surprised by a request being "partially handled".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in commit 5760385. Password validation is now checked at the beginning of the handler, before any other operations are performed. This prevents partial saves where PIN might be updated but OTA settings fail.

Co-authored-by: willmmiles <6540455+willmmiles@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Dec 27, 2025

We're almost there. @copilot Can you please make the requested changes?

Done in commit 5760385. The validation now happens before any other operations to prevent partial saves.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cannot update WLED on Quinled dig-uno, OTA permanently locked

3 participants