Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion wled00/data/index.css
Original file line number Diff line number Diff line change
Expand Up @@ -794,7 +794,7 @@ input[type=range]::-moz-range-thumb {
/* buttons */
.btn {
padding: 8px;
/*margin: 10px 4px;*/
margin: 10px 4px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

are you sure this wont break anything else in the UI?

Copy link
Member Author

Choose a reason for hiding this comment

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

Buttons for a touch UI should always have a margin as otherwise you risk clicking the wrong item.
I'll have a check through however for other used of that class to see if this change is an improvement elsewhere too however

Copy link
Collaborator

Choose a reason for hiding this comment

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

I did not see anything obvious. just wondering why it was commented. Mayb just to save a few bytes.

width: 230px;
font-size: 19px;
color: var(--c-d);
Expand Down
191 changes: 191 additions & 0 deletions wled00/data/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -693,6 +693,8 @@ function parseInfo(i) {
// gId("filterVol").classList.add("hide"); hideModes(" ♪"); // hide volume reactive effects
// gId("filterFreq").classList.add("hide"); hideModes(" ♫"); // hide frequency reactive effects
// }
// Check for version upgrades on page load
checkVersionUpgrade(i);
}

//https://stackoverflow.com/questions/2592092/executing-script-elements-inserted-with-innerhtml
Expand Down Expand Up @@ -3304,6 +3306,195 @@ function simplifyUI() {
gId("btns").style.display = "none";
}

// Version reporting feature
var versionCheckDone = false;

function checkVersionUpgrade(info) {
// Only check once per page load
if (versionCheckDone) return;
versionCheckDone = true;

// Suppress feature if in AP mode (no internet connection available)
if (info.wifi && info.wifi.ap) return;

// Fetch version-info.json using existing /edit endpoint
fetch(getURL('/edit?func=edit&path=/version-info.json'), {
method: 'get'
})
.then(res => {
if (res.status === 404) {
// File doesn't exist - first install, show install prompt
showVersionUpgradePrompt(info, null, info.ver);
return null;
}
if (!res.ok) {
throw new Error('Failed to fetch version-info.json');
}
return res.json();
})
.then(versionInfo => {
if (!versionInfo) return; // 404 case already handled

// Check if user opted out
if (versionInfo.neverAsk) return;

// Check if version has changed
const currentVersion = info.ver;
const storedVersion = versionInfo.version || '';

if (storedVersion && storedVersion !== currentVersion) {
// Version has changed, show upgrade prompt
showVersionUpgradePrompt(info, storedVersion, currentVersion);
} else if (!storedVersion) {
// Empty version in file, show install prompt
showVersionUpgradePrompt(info, null, currentVersion);
}
})
.catch(e => {
console.log('Failed to load version-info.json', e);
// On error, save current version for next time
if (info && info.ver) {
updateVersionInfo(info.ver, false);
}
});
}

function showVersionUpgradePrompt(info, oldVersion, newVersion) {
// Determine if this is an install or upgrade
Copy link
Member

Choose a reason for hiding this comment

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

Can we test if the update server is accessible here before showing the prompt? It might save a lot of "failed to report" cases if the setup device is also offline.

Copy link
Member Author

Choose a reason for hiding this comment

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

Overkill I'd say. It's suppressed already for ap mode, plus to check that connectivity would mean making a connection without consent so definitely not a good idea

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it's too bad that we can't directly perform DNS resolution from the browser without connecting to anyone. :( As below I am mostly worried about annoying offline people with repeated prompts.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are correct that we could do a hostname lookup only if we are trying to detect internet access rather than connectivity to the usage platform specifically, which would be a GDPR compliant approach.

If folk are always offline, then they can just click never.

Let's see how things go and can always improve based on feedback

Copy link
Member

Choose a reason for hiding this comment

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

As far as I could find, only Firefox offers a DNS resolution API. Sadly other browsers offer no solution for testing connectivity other than opening a connection somewhere. Maybe someday..

const isInstall = !oldVersion;

// Create overlay and dialog
const overlay = d.createElement('div');
overlay.id = 'versionUpgradeOverlay';
overlay.style.cssText = 'position:fixed;top:0;left:0;width:100%;height:100%;background:rgba(0,0,0,0.7);z-index:10000;display:flex;align-items:center;justify-content:center;';

const dialog = d.createElement('div');
dialog.style.cssText = 'background:var(--c-1);border-radius:10px;padding:25px;max-width:500px;margin:20px;box-shadow:0 4px 6px rgba(0,0,0,0.3);';

// Build contextual message based on install vs upgrade
const title = isInstall
? '🎉 Thank you for installing WLED!'
: '🎉 WLED Upgrade Detected!';

const description = isInstall
? `You are now running WLED <strong style="text-wrap: nowrap">${newVersion}</strong>.`
: `Your WLED has been upgraded from <strong style="text-wrap: nowrap">${oldVersion}</strong> to <strong style="text-wrap: nowrap">${newVersion}</strong>.`;

const question = 'Help make WLED better with a one-time hardware report? It includes only device details like chip type, LED count, etc. — never personal data or your activities.'

dialog.innerHTML = `
<h2 style="margin-top:0;color:var(--c-f);">${title}</h2>
<p style="color:var(--c-f);">${description}</p>
<p style="color:var(--c-f);">${question}</p>
<p style="color:var(--c-f);font-size:0.9em;">
<a href="https://kno.wled.ge/about/privacy-policy/" target="_blank" style="color:var(--c-6);">Learn more about what data is collected and why</a>
</p>
<div style="margin-top:20px;">
<button id="versionReportYes" class="btn">Yes</button>
<button id="versionReportNo" class="btn">Not Now</button>
<button id="versionReportNever" class="btn">Never Ask</button>
</div>
`;

overlay.appendChild(dialog);
d.body.appendChild(overlay);

// Add event listeners
gId('versionReportYes').addEventListener('click', () => {
reportUpgradeEvent(info, oldVersion);
d.body.removeChild(overlay);
});

gId('versionReportNo').addEventListener('click', () => {
// Don't update version, will ask again on next load
d.body.removeChild(overlay);
});

gId('versionReportNever').addEventListener('click', () => {
updateVersionInfo(newVersion, true);
d.body.removeChild(overlay);
showToast('You will not be asked again.');
});
}

function reportUpgradeEvent(info, oldVersion) {
showToast('Reporting upgrade...');

// Fetch fresh data from /json/info endpoint as requested
fetch(getURL('/json/info'), {
method: 'get'
})
.then(res => res.json())
.then(infoData => {
// Map to UpgradeEventRequest structure per OpenAPI spec
// Required fields: deviceId, version, previousVersion, releaseName, chip, ledCount, isMatrix, bootloaderSHA256
const upgradeData = {
deviceId: infoData.deviceId, // Use anonymous unique device ID
version: infoData.ver || '', // Current version string
previousVersion: oldVersion || '', // Previous version from version-info.json
releaseName: infoData.release || '', // Release name (e.g., "WLED 0.15.0")
chip: infoData.arch || '', // Chip architecture (esp32, esp8266, etc)
ledCount: infoData.leds ? infoData.leds.count : 0, // Number of LEDs
isMatrix: !!(infoData.leds && infoData.leds.matrix), // Whether it's a 2D matrix setup
bootloaderSHA256: infoData.bootloaderSHA256 || '', // Bootloader SHA256 hash
brand: infoData.brand, // Device brand (always present)
product: infoData.product, // Product name (always present)
flashSize: infoData.flash // Flash size (always present)
};

// Add optional fields if available
if (infoData.psram !== undefined) upgradeData.psramSize = infoData.psram;
// Note: partitionSizes not currently available in /json/info endpoint

// Make AJAX call to postUpgradeEvent API
return fetch('https://usage.wled.me/api/usage/upgrade', {
method: 'POST',
headers: {
'Content-Type': 'application/json'
},
body: JSON.stringify(upgradeData)
});
})
.then(res => {
if (res.ok) {
showToast('Thank you for reporting!');
updateVersionInfo(info.ver, false);
} else {
showToast('Report failed. Please try again later.', true);
Copy link
Member

Choose a reason for hiding this comment

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

Future: we might want to consider saving "user gave permission, but we failed to report" in the version info file. That way we could avoid re-prompting the user with a question they've already answered (and driving more people to click "never again" to just to get it to go away).

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice idea, but for the first release let's keep this to a strict simple relationship between consent and action. If it's well received we can consider "lasting consent"

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough. I'm just concerned that being annoying about reporting will exacerbate any privacy concerns.

// Do NOT update version info on failure - user will be prompted again
}
})
.catch(e => {
console.log('Failed to report upgrade', e);
showToast('Report failed. Please try again later.', true);
// Do NOT update version info on error - user will be prompted again
});
}

function updateVersionInfo(version, neverAsk) {
const versionInfo = {
version: version,
neverAsk: neverAsk
};

// Create a Blob with JSON content and use /upload endpoint
const blob = new Blob([JSON.stringify(versionInfo)], {type: 'application/json'});
const formData = new FormData();
formData.append('data', blob, 'version-info.json');

fetch(getURL('/upload'), {
method: 'POST',
body: formData
})
.then(res => res.text())
.then(data => {
console.log('Version info updated', data);
})
.catch(e => {
console.log('Failed to update version-info.json', e);
});
}
Comment on lines 3312 to 3496
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 26, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Version reporting flow is coherent; update prompt wording and consider minor cleanups

The end‑to‑end flow — detecting install/upgrade via version-info.json, prompting once per version, posting only hardware/version metadata to https://usage.wled.me/api/usage/upgrade, and persisting version-info.json via /upload — looks logically sound and matches the “opt‑in, non‑behavioral usage reporting” intent from earlier work. Based on learnings, the fields you send are all device/firmware level, not behavioral.

A few focused points to address or consider:

  1. Prompt text doesn’t yet match the PR’s privacy‑clarified wording

    The current question string:

    const question = 'Would you like to help the WLED development team by reporting your installation? This helps us understand what hardware and versions are being used.'

    doesn’t incorporate the “one time action” and “no personal or behavioral data” clarification mentioned in the PR description. To align both with the PR and with what reportUpgradeEvent actually sends, I’d update it along these lines:

  • const question = 'Would you like to help the WLED development team by reporting your installation? This helps us understand what hardware and versions are being used.'
  • const question = 'Would you like to help the WLED development team by reporting your installation? This is a one time action that sends details of your hardware configuration, but no personal or behavioral data, e.g which esp32 chip variant, how many LED pixels etc';



2. **`reportUpgradeEvent` doesn’t really need the `info` parameter**

Inside `reportUpgradeEvent(info, oldVersion)` you immediately fetch fresh data from `/json/info` and build `upgradeData` from `infoData`. The only remaining use of `info` is:

```js
updateVersionInfo(info.ver, false);

To simplify the API and avoid dependence on the outer info object, you could rely purely on infoData:

-	function reportUpgradeEvent(info, oldVersion) {
+	function reportUpgradeEvent(oldVersion) {-	.then(infoData => {
+	.then(infoData => {-	.then(res => {
+	.then(res => {
		if (res.ok) {
			showToast('Thank you for reporting!');
-			updateVersionInfo(info.ver, false);
+			updateVersionInfo(infoData.ver || '', false);

and at the call site:

-	reportUpgradeEvent(info, oldVersion);
+	reportUpgradeEvent(oldVersion);

This keeps the function self‑contained and matches the previous CodeQL feedback about an unnecessary info parameter.

  1. InnerHTML/XSS and the CodeQL warning

    dialog.innerHTML = \…${title}…${description}…${question}…`; is what CodeQL is flagging. In practice here the only interpolated values are firmware‑controlled strings (oldVersion/newVersion) plus a constant question, so this doesn’t introduce a new, attacker‑controlled XSS surface beyond existing innerHTML` uses in this file.

    If you want to harden further or quiet CodeQL, an incremental improvement would be to construct the dialog content with DOM APIs and set dynamic pieces via textContent instead of string interpolation, while keeping the link/button structure the same. Given the current data sources, I’d treat that as a future security‑hardening refactor rather than a blocker for this PR.

  2. Process reminder for web UI changes

    Since wled00/data/index.js is modified, remember to run npm run build before producing firmware binaries so that the embedded web assets stay in sync with the updated JS.

🧰 Tools
🪛 GitHub Check: CodeQL

[failure] 3385-3397: Client-side cross-site scripting
Cross-site scripting vulnerability due to user-provided value.

Copy link
Member Author

Choose a reason for hiding this comment

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

Please update your comment to reflect the current code @coderabbitai

Copy link
Contributor

Choose a reason for hiding this comment

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

@netmindz, I've reviewed the current code in the diff and my observations remain accurate. Here's an updated assessment:

Current State Analysis

The version reporting implementation is functionally sound, but the prompt wording still doesn't match your proposed privacy-clarified text from the PR description.

1. Prompt Text Needs Update (Line 3383)

Current code:

const question = 'Would you like to help the WLED development team by reporting your installation? This helps us understand what hardware and versions are being used.'

Should be (per PR description):

const question = 'Would you like to help the WLED development team by reporting your installation?  This is a one time action that sends details of your hardware configuration, but no personal or behavioral data, e.g which esp32 chip variant, how many LED pixels etc';

This aligns the UI text with what's actually being collected and matches the privacy transparency discussed in PR #5093.

2. Redundant Parameter (Lines 3419-3455)

The reportUpgradeEvent(info, oldVersion) function receives info but only uses info.ver at the end (line 3455). Since it fetches fresh data from /json/info anyway, you could simplify to:

function reportUpgradeEvent(oldVersion) {
  // ... fetch infoData ...
  .then(res => {
    if (res.ok) {
      showToast('Thank you for reporting!');
      updateVersionInfo(infoData.ver || '', false); // use infoData instead
    }
  // ...
}

And update the call site (line 3403):

reportUpgradeEvent(oldVersion);

3. CodeQL XSS Warning (Lines 3385-3397)

The dialog.innerHTML with template literals triggers a CodeQL warning. While not immediately exploitable (values come from firmware version strings), hardening with DOM APIs would be more secure. This is not a blocker but a future improvement opportunity.

4. Build Process

Remember to run npm run build after JS changes to update the embedded web assets.

The core logic—detecting install/upgrade, one-time prompting, hardware-only reporting, and persistence—is solid and aligns with the opt-in usage reporting design.


🧠 Learnings used
Learnt from: netmindz
Repo: wled/WLED PR: 5093
File: wled00/util.cpp:1159-1182
Timestamp: 2025-11-20T00:04:04.829Z
Learning: In WLED PR #5093, the deviceId feature is designed for opt-in usage reporting that tracks only version/upgrade information (non-behavioral data), not user activity patterns. The deterministic salt approach (MAC + "WLED" + chip model/revision) is acceptable for this limited use case, as correlating MAC addresses to version history represents minimal privacy risk compared to behavioral tracking.

Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.


size();
_C.style.setProperty('--n', N);

Expand Down