Harden WebAccess request handling and rendered output#2026
Open
cmuellner wants to merge 4 commits into
Open
Conversation
Origin: reported in PR mcallegari#1955 and still reproducible after the upstream partial WebAccess null-widget fixes. Enable WebAccess, open an authenticated WebSocket session, and send `API|getWidgetSubIdList|<missing-id>`. The handler looked up the VC widget and immediately switched on `widget->type()`, so an unknown widget id dereferenced a null pointer. Return without a response when the widget id is not present, matching the existing behavior for malformed API commands in this handler. No automated regression test exists for this WebSocket handler path. Signed-off-by: Christoph Müllner <christophm30@gmail.com>
Origin: found while reviewing adjacent WebAccess command validation issues around PR mcallegari#1955. With WebAccess authentication enabled, an authenticated super-admin could send QLC+AUTH|ADD_USER or QLC+SYS|NETWORK|wlan0|dhcp and reach handlers that indexed missing command fields. A low-level user could send GM_VALUE|0 and change the Grand Master before the Simple Desk and VC permission gate. In classic WebAccess, short widget commands such as <cue-id>|STEP, <frame-id>|FRAME_DISABLE or <matrix-id>|MATRIX_KNOB|1 had the same missing argument checks. The network profile code also built nmcli arguments as one string and split it on spaces. A wireless SSID or WPA passphrase containing spaces was therefore passed to nmcli as multiple arguments and could not be configured. Reject malformed auth, network, Grand Master and classic widget commands before reading their fields. Apply the Simple Desk and VC permission gate to GM_VALUE in both WebAccess backends. Build nmcli calls as QStringList arguments so SSIDs and passphrases are preserved. No automated regression test exists for this WebAccess command and NetworkManager profile path. Signed-off-by: Christoph Müllner <christophm30@gmail.com>
Origin: S1 was reported in PR mcallegari#1955. The remaining device and configuration cases were derived while validating that report. Reproduce by naming a VC multipage frame page </script><script>alert(1)</script> or a universe <img src=x onerror=alert(1)>, then open WebAccess / or /simpleDesk. Those values were written into inline script or HTML. Similar unescaped values appeared on /config and /system for plugin, audio, fixture, user, interface and Wi-Fi strings. Add a JavaScript string escaper for inline WebAccess data and use Qt HTML escaping for text and attribute contexts in the affected pages. Added webaccessescaping_test to cover plain text, quotes, backslashes, script-breaking characters and control characters. Signed-off-by: Christoph Müllner <christophm30@gmail.com>
Origin: independent finding in the WebAccess fixture upload path. Reproduce by authenticating to WebAccess as a super-admin and posting to /loadFixture with a multipart filename such as ../../tmp/outside.qxf, fixture.php or an overlong fixture name. The upload parser accepted path-bearing names and normalized them to a basename before fixture storage instead of rejecting the malformed filename. Reject empty, dot, dot-dot, overlong, unexpected-extension and path-containing fixture upload names before calling fixture storage. Plain .qxf and .d4 uploaded filenames continue to be accepted unchanged. Moved the filename validation helper into a small testable source file and added webaccessupload_test for the accepted and rejected filename cases. Signed-off-by: Christoph Müllner <christophm30@gmail.com>
|
coverage: 35.037%. remained the same — cmuellner:pr-1955-fixes-p3 into mcallegari:master |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Summary of Changes:
This PR fixes WebAccess reliability issues found while reviewing stale
PR #1955 and adjacent WebAccess request handling.
The changes handle missing widgets, validate malformed WebSocket
commands, apply the expected permission gate to Grand Master updates,
escape rendered project/device strings by context, and reject fixture
upload filenames that contain path syntax. Well-formed WebAccess
requests and fixture upload filenames are handled as before.
Commit Guide
</script><script>alert(1)</script>or a universe name like<img src=x onerror=alert(1)>./loadFixturewith a multipart filename such as../../tmp/outside.qxf,fixture.php, or an overlong fixture name.Testing
Test Cases:
This PR adds focused WebAccess unit tests for the pure escaping and
upload filename validation helpers. The commit guide lists the remaining
manual/protocol reproductions.
Test Results:
webaccessescaping_testwebaccessupload_testManual Testing Gaps
The escaping and upload filename changes have focused automated tests.
The WebSocket command-validation changes do not have an automated
regression test; reviewers can verify them by opening an
authenticated WebAccess session and sending the malformed commands
listed in the commit guide, then confirming QLC+ rejects them without
crashing or applying unauthorized Grand Master changes.
The NetworkManager argument change needs a suitable Wi-Fi setup for full
manual verification. A reviewer with that setup can configure an SSID
and WPA passphrase containing spaces and confirm they are passed to
NetworkManager unchanged.
Additional Notes
The fixes are grouped because they all affect WebAccess. The commits are
kept separate so pointer validation, command validation, rendering
escaping and upload filename validation can be reviewed independently.
Each commit body contains the full rationale and reproduction details.
Checklist
{on a new line for functions and class definitions.