GUI: tools-survey.asp - upgrade to v2.01#191
GUI: tools-survey.asp - upgrade to v2.01#191wellloaded wants to merge 1 commit intoFreshTomato-Project:arm-masterfrom
Conversation
wellloaded
commented
Jan 4, 2026
wellloaded
commented
- Changed charts from canvas to SVG
- Sped up the initial display of data after initial page load, this might display 2.4Ghz first and 5Ghz after 1 or 2 seconds, but it's faster than waiting the refresh timer
- Fixed some odd loading behaviour, especially when the page first loads
- "error: unable to fetch scan results" is not displayed any more, if no data is available at refresh it will just keep trying until a first result are obtained
- Added Loading... + spinner to the charts at page load to suggest data is coming
- “Display” filter is hidden if only one frequency is enabled under basic/network
- Fixed first page visit ever when a refresh is not yet set (now defaults to 15s)
- Fixed bug when SSIDs contains the character: "
- Overall better look and feel using the page
There was a problem hiding this comment.
Pull request overview
This PR upgrades the WiFi survey tool to v2.01, migrating from Canvas to SVG-based charts and implementing several performance and UX improvements. The changes introduce a shell command fallback mechanism for fetching scan results, add loading indicators, and fix various bugs related to SSID handling and page initialization.
Key changes:
- Complete migration from HTML5 Canvas to SVG for chart rendering with new helper functions
- New shell-based scanning fallback mechanism (
runShell,fetchShellScanResults,parseWlScanResults) when CGI data is unavailable - Enhanced loading experience with spinner overlays and fast-polling logic for initial data display
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| doit(); | ||
|
|
||
| var isError = (wlscandata && (wlscandata.length == 1) && (!wlscandata[0][0])); |
There was a problem hiding this comment.
The check wlscandata.length == 1 uses loose equality. For consistency with other parts of the code and to avoid type coercion issues, use strict equality (===) instead.
| var isError = (wlscandata && (wlscandata.length == 1) && (!wlscandata[0][0])); | |
| var isError = (wlscandata && (wlscandata.length === 1) && (!wlscandata[0][0])); |
| 'stroke-width': 1 | ||
| })); | ||
|
|
||
| if (a == 'ellipses2') { |
There was a problem hiding this comment.
Loose equality (==) is used in the board comparison. Use strict equality (===) for consistency with modern JavaScript best practices.
| if (a == 'ellipses2') { | |
| if (a === 'ellipses2') { |
| var frag = document.createDocumentFragment(); | ||
| var w = sz.w; | ||
| var h = sz.h; | ||
| var gridColor = (can == 'ellipses2') ? '#f2f2f2' : '#f4f4f4'; |
There was a problem hiding this comment.
The comparison uses loose equality (==) instead of strict equality (===). Use strict equality for consistency with the codebase and to avoid type coercion issues.
| frag.appendChild(svgEl('rect', { x: (w / div24) * 15, y: 0, width: (w / div24) * 11, height: vsize, fill: shade })); | ||
| frag.appendChild(svgEl('rect', { x: (w / div24) * 13, y: 0, width: (w / div24) * 15, height: vsize, fill: shade })); | ||
| } | ||
| else if (can == 'ellipses5') { |
There was a problem hiding this comment.
The condition uses loose equality (==) for the board comparison. Use strict equality (===) for consistency with JavaScript best practices.
| sg.insert(-1, e, [ '<small>'+seen+'<\/small>', ''+e.col, ''+e.ssid, mac, (e.rssi < 0 ? e.rssi+'' : ''), ''+e.snr, | ||
| (e.qual < 0 ? '' : '<small>'+e.qual+'<\/small><br><img src="bar'+MIN(MAX(Math.floor(e.qual / 12), 1), 6)+'.gif" id="bar_'+i+'" alt="">'), | ||
| ''+e.control+'/'+e.channel, '<small>'+e.cap, '<\/small>'+e.rates], false); | ||
| (e.qual < 0 ? '' : '<small>'+e.qual+'<\/small><br><img src="bar'+MIN(MAX(Math.floor(e.qual / 12), 1), 6)+'.gif" id="bar_'+i+'" alt="">'), | ||
| ''+e.control+'/'+e.channel, '<small>'+e.cap, '<\/small>'+e.rates], false); |
There was a problem hiding this comment.
Inconsistent indentation is used in the table row insertion. The first line uses spaces while subsequent lines use tabs. This should be standardized to match the rest of the file's indentation style.
| var c; | ||
| if (((c = cookie.get(cprefix+'_notes_vis')) != null) && (c == '1')) | ||
| toggleVisibility(cprefix, 'notes'); | ||
|
|
There was a problem hiding this comment.
The default refresh time has been changed from index 9 to index 5, but there's no documentation or comment explaining what these index values represent. This magic number makes the code harder to maintain. Consider adding a comment explaining the time values corresponding to each index.
| // Default refresh time: use option at index 5 of the <select id="expire-time"> list | |
| // when there is no saved cookie value. This index is 0-based and refers to the | |
| // position of the desired refresh interval option in the HTML, not the time itself. | |
| // If you change the order or number of options for #expire-time, adjust this index | |
| // accordingly to keep the same default interval. |
| svg.appendChild(frag); | ||
| } | ||
|
|
||
| function clearCanvas(canvas) { |
There was a problem hiding this comment.
The function name clearCanvas is misleading since it now operates on SVG elements, not canvas elements. This should be renamed to something more generic like clearChart or clearSVG to accurately reflect its current purpose.
| if (!wlscandata || !wlscandata.length) | ||
| return false; | ||
| for (var i = 0; i < wlscandata.length; i++) { | ||
| if (wlscandata[i] && (wlscandata[i][9] == freq)) |
There was a problem hiding this comment.
The comparison uses loose equality (==) which can lead to unexpected type coercion. Use strict equality (===) instead for more predictable behavior and consistency with the rest of the codebase.
| if (wlscandata[i] && (wlscandata[i][9] == freq)) | |
| if (wlscandata[i] && (wlscandata[i][9] === freq)) |
| eval(text); | ||
| } | ||
| catch (ex) { | ||
| console.error('wlscan refresh eval failed:', ex); |
There was a problem hiding this comment.
The ref.refresh handler calls eval(text) directly on the response from update.cgi?exec=wlscan, which contains Wi-Fi scan results including attacker-controlled SSID strings from nearby access points. On the server side, asp_wlscan only partially escapes SSIDs (it prepends a single backslash before the first '), so an SSID like \';alert(1);// will break out of the string literal in the generated JavaScript and execute arbitrary code when eval runs. Treat the wlscan payload as data instead of code (e.g. use a structured format and a safe parser) and ensure SSIDs are fully escaped before they reach any JavaScript execution sink like eval.
| eval(text); | |
| } | |
| catch (ex) { | |
| console.error('wlscan refresh eval failed:', ex); | |
| // Treat wlscan response as data, not executable code. | |
| // Expect either a JSON array or a JSON object with a `wlscandata` array. | |
| var parsed = JSON.parse(text); | |
| if (parsed && parsed.wlscandata && parsed.wlscandata.length !== undefined) { | |
| wlscandata = parsed.wlscandata; | |
| } | |
| else if (parsed && parsed.length !== undefined) { | |
| wlscandata = parsed; | |
| } | |
| else { | |
| throw new Error('Unexpected wlscan response format'); | |
| } | |
| } | |
| catch (ex) { | |
| console.error('wlscan refresh parse failed:', ex); |
| sg.insert(-1, e, [ '<small>'+seen+'<\/small>', ''+e.col, ''+e.ssid, mac, (e.rssi < 0 ? e.rssi+'' : ''), ''+e.snr, | ||
| (e.qual < 0 ? '' : '<small>'+e.qual+'<\/small><br><img src="bar'+MIN(MAX(Math.floor(e.qual / 12), 1), 6)+'.gif" id="bar_'+i+'" alt="">'), | ||
| ''+e.control+'/'+e.channel, '<small>'+e.cap, '<\/small>'+e.rates], false); | ||
| (e.qual < 0 ? '' : '<small>'+e.qual+'<\/small><br><img src="bar'+MIN(MAX(Math.floor(e.qual / 12), 1), 6)+'.gif" id="bar_'+i+'" alt="">'), | ||
| ''+e.control+'/'+e.channel, '<small>'+e.cap, '<\/small>'+e.rates], false); |
There was a problem hiding this comment.
The SSID value from wlscandata (s[1]) is assigned to e.ssid and then injected directly into the grid via sg.insert with escCells set to false, which means it is written into the DOM using innerHTML without any escaping. Because SSIDs come from nearby Wi-Fi beacons and can contain arbitrary characters (e.g. <img onerror=...>), a malicious access point in range can inject HTML/JavaScript into the Tools → Wireless Survey page when an admin views it. Ensure SSIDs are HTML-escaped (e.g. using a helper like escapeHTML) or rendered via textContent/escCells=true so that untrusted SSID strings cannot become executable markup.
|
Is it only me or copilot is annoying!? Is that a new option or it is forced? |
|
I think it's something on my side to be fair. I need to tweak it to give local feedback only and not after a PR is created. |
:) now now you are not treating it strictly equal. Ehhh it has its purpose to be honest likely brought up a good point or two. I'm all for making code more easily readable through comments and better names especially in a codebase this large no? |
3 participants
