Skip to content

Return 401 on incorrect OTA password instead of misleading success message #5222

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
Copilot wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Return 401 on incorrect OTA password instead of misleading success message #5222

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ce9a8fb
Initial plan
Copilot Dec 23, 2025
e9a366e
Add OTA password validation feedback for security settings
Copilot Dec 23, 2025
92a43ff
Code review and security check completed
Copilot Dec 23, 2025
b7485e2
Address PR feedback: invert flag logic to otaPassCorrect, simplify er…
Copilot Dec 23, 2025
38b159d
Return 401 immediately on incorrect password instead of using global …
Copilot Dec 23, 2025
3e5646d
Only return 401 when OTA settings are actually being changed
Copilot Dec 27, 2025
5760385
Check password validation before any operations to prevent partial saves
Copilot Dec 27, 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
1 change: 1 addition & 0 deletions _codeql_detected_source_root
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
.

@DedeHai DedeHai Dec 23, 2025

Copy link
Copy Markdown
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

Copilot AI Dec 23, 2025

Copy link
Copy Markdown
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.

4 changes: 4 additions & 0 deletions wled00/set.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -609,12 +609,16 @@ void handleSettingsSet(AsyncWebServerRequest *request, byte subPage)

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

@DedeHai DedeHai Dec 23, 2025

Copy link
Copy Markdown
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"

Copilot AI Dec 23, 2025

Copy link
Copy Markdown
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.

otaLock = request->hasArg(F("NO"));
wifiLock = request->hasArg(F("OW"));
#ifndef WLED_DISABLE_OTA
aOtaEnabled = request->hasArg(F("AO"));
#endif
otaSameSubnet = request->hasArg(F("SU"));
} else if (otaLock) {

@willmmiles willmmiles Dec 23, 2025

Copy link
Copy Markdown
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.

@DedeHai DedeHai Dec 27, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@copilot please update

Copilot AI Dec 27, 2025

Copy link
Copy Markdown
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.

// If OTA is locked and password is incorrect, flag it
otaPasswordIncorrect = true;
}
}

Expand Down
1 change: 1 addition & 0 deletions wled00/wled.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -897,6 +897,7 @@ WLED_GLOBAL byte optionType;

WLED_GLOBAL bool configNeedsWrite _INIT(false); // flag to initiate saving of config
WLED_GLOBAL bool doReboot _INIT(false); // flag to initiate reboot from async handlers
WLED_GLOBAL bool otaPasswordIncorrect _INIT(false); // flag to indicate OTA password was incorrect during security settings save

// status led
#if defined(STATUSLED)
Expand Down
13 changes: 11 additions & 2 deletions wled00/wled_server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -753,14 +753,23 @@ void serveSettings(AsyncWebServerRequest* request, bool post) {

char s[32];
char s2[45] = "";
bool otaPwdErr = otaPasswordIncorrect; // save state before clearing

switch (subPage) {
case SUBPAGE_WIFI : strcpy_P(s, PSTR("WiFi")); strcpy_P(s2, PSTR("Please connect to the new IP (if changed)")); break;
case SUBPAGE_LEDS : strcpy_P(s, PSTR("LED")); break;
case SUBPAGE_UI : strcpy_P(s, PSTR("UI")); break;
case SUBPAGE_SYNC : strcpy_P(s, PSTR("Sync")); break;
case SUBPAGE_TIME : strcpy_P(s, PSTR("Time")); break;
case SUBPAGE_SEC : strcpy_P(s, PSTR("Security")); if (doReboot) strcpy_P(s2, PSTR("Rebooting, please wait ~10 seconds...")); break;
case SUBPAGE_SEC :
strcpy_P(s, PSTR("Security"));
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."));

@DedeHai DedeHai Dec 23, 2025

Copy link
Copy Markdown
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

Copilot AI Dec 23, 2025

Copy link
Copy Markdown
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".

otaPasswordIncorrect = false; // clear flag after displaying message
}
break;
#ifdef WLED_ENABLE_DMX
case SUBPAGE_DMX : strcpy_P(s, PSTR("DMX")); break;
#endif
Expand All @@ -771,7 +780,7 @@ void serveSettings(AsyncWebServerRequest* request, bool post) {
case SUBPAGE_PINREQ : strcpy_P(s, correctPIN ? PSTR("PIN accepted") : PSTR("PIN rejected")); break;
}

if (subPage != SUBPAGE_PINREQ) strcat_P(s, PSTR(" settings saved."));
if (subPage != SUBPAGE_PINREQ && !(subPage == SUBPAGE_SEC && otaPwdErr)) strcat_P(s, PSTR(" settings saved."));

if (subPage == SUBPAGE_PINREQ && correctPIN) {
subPage = originalSubPage; // on correct PIN load settings page the user intended
Expand Down