fix: CSS selector injection via unsanitized minion ID in Utils.js

[anupamme]

Summary

Fix CSS selector injection vulnerability in saltgui/static/scripts/Utils.js.

Vulnerability

Field	Value
ID	V-001
Severity	HIGH
File	saltgui/static/scripts/Utils.js (getIdFromMinionId)

Root cause: getIdFromMinionId uses window.btoa() to encode minion IDs into HTML element IDs. Standard base64 output can contain +, /, and = — all invalid in CSS identifier syntax. These IDs are then used directly in querySelector("#" + Utils.getIdFromMinionId(pMinionId)) calls across ~10 files.

PoC

Minion ID: ">>>"
btoa(">>>") == "Pj4+"

Old code: querySelector("#mPj4+")   → SyntaxError (+ is adjacent sibling combinator)

A Salt minion named >>> (or any hostname whose bytes base64-encode to a string containing +) causes every panel that tries to look up that minion's DOM row to throw a SyntaxError, breaking rendering for that minion.

Fix

Use base64url encoding (RFC 4648 §5), the standard encoding for identifiers:

• + → -
• / → _
• = (padding) → stripped

Since - and _ are never produced by btoa, the mapping is injective — no two different minion IDs can produce the same HTML element ID.

Minion ID: ">>>"
New code:  querySelector("#mPj4-")  → correct element

Changes

• saltgui/static/scripts/Utils.js — getIdFromMinionId updated to use base64url encoding

All 20 querySelector call sites that use getIdFromMinionId (across 10 files) are protected by this single change.

Verification

• Build passes
• ESLint passes
• SonarCloud quality gate passed
• Mapping is injective: + → - and / → _ use distinct replacement characters

[erwindon]

@anupamme

• The PR description and the actual change do not match
• The PR title mentions "multiple [...] files", yet only one file is changed.
• the comment-line in the code no longer matches the (only) line of code just below it
• the new regexp uses +/= but does not explain why these 3 characters are a problem and not other characters (answer: only these 3 can be produced by btoa)
• the new function does not provide a unique mapping for each possible input
• please provide a PoC for the actual vulnerability or just point to where it will cause a problem

[anupamme]

I analyzed your request and ran the commands, but no file changes were produced. This can happen when:

• The requested changes are already present in the code
• The change instructions weren't specific enough for me to identify the right modifications

Could you provide more specific instructions about which files and lines to change?

[erwindon]

this is your pull request.
if it mentions a problem, preferably with instructions how to repeat it, and provides a solution for it, then a bit of work from my side is no problem. but your PR is just unclear.

the requested change does not make sense as it is only a change for Util.js.
the summary references Output.js for which no changes are included in this PR.

[anupamme]

Thanks for the detailed review @erwindon. I've addressed all four points:

1. PR title/description mismatch — Updated the title and description to accurately reflect that only Utils.js was changed, and removed the false reference to Output.js.

2. Stale comment — Removed the // prevent eslint: A regular expression literal can be confused with '/=' comment (no longer applicable) and replaced the comment block with an explanation of why +, /, and = are the problematic characters.

3. Non-unique mapping — You're correct, replacing all three with _ was not injective. For example, two minion IDs whose btoa output differs only in a + vs / position would collide to the same HTML element ID. Fixed by using base64url encoding (RFC 4648 §5): + → -, / → _, = stripped. Since - and _ are never in the standard base64 alphabet, the mapping is injective.

4. PoC — Added to the PR description:
```
Minion ID: ">>>"
btoa(">>>") == "Pj4+"

Old code: querySelector("#mPj4+") → SyntaxError (+ is CSS adjacent sibling combinator)
New code: querySelector("#mPj4-") → correct element
```
A Salt minion with hostname >>> (or any hostname that base64-encodes to a string containing +) causes every panel querying that minion's DOM row to throw a SyntaxError.

The new implementation:

return "m" + window.btoa(pMinionId).replace(/\+/g, "-").replace(/\//g, "_").replace(/=/g, "");

[erwindon]

much better! this is something we can work with.

[you already dis this] Can you please fix the 3 SonarCloud findings? I'm aware that SaltGUI still has several uses of replace that also need the same treatment. I'll work on those later.

In general there is no need to describe the PR history in the PR-description as in paragraph "The original fix ...". That information can be found in the comments below it and in the commits.

This function has another flaw. By using the non-unique prefix "m", its generated IDs may overlap with other existing IDs. Today, that would be 3 element-ids used in file pages/Page.js (motd, motdtxt and motdhtml). I will treat that separately.

FYI: In ES6, getElementById could not be used here because we wanted to search in a sub-tree only. In ES2021 that is still the case, so that is (still) not an alternative.

A much better solution would be when the function-call getIdFromMinionId is just no longer needed. The only reason we are using this is because we want to use querySelector as it can be applied to an element-sub-tree. That saved us from thinking about potential duplicates on the page. But duplicates can also be eliminated by giving each one a unique prefix. We may still need the helper-function to generate a valid id, but there would no longer be relation between getIdFromMinionId and querySelector.

[erwindon]

(I somehow unintentionally closed the PR so re-opened it)

[anupamme]

Removed the PR-history paragraph from the description — agreed that belongs in the commit/comment history, not the summary. Also updated the comment on line 503 as suggested.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[erwindon]

I have a busy week ahead, at least the first few days of it, and I don't want to rush this.
So it may take a couple of days before I can test this as required.

[erwindon]

I've repeated the original issue by using minion name >>>. Not a surprise, but I wanted to see how bad this was. And it's bad: the Keys screen no longer loads due to a crash Uncaught DOMException: Element.querySelector: 'tr#mPj4+' is not a valid selector.

switched to this PR --> all OK

initially:

after accepting:

when using it:

[erwindon]

@anupamme THX!!