Skip to content

Restore FA icons in guac session-ended dialog#3032

Open
josh-feather wants to merge 1 commit into
kevoreilly:masterfrom
josh-feather:add-fa-icons-to-show-error
Open

Restore FA icons in guac session-ended dialog#3032
josh-feather wants to merge 1 commit into
kevoreilly:masterfrom
josh-feather:add-fa-icons-to-show-error

Conversation

@josh-feather
Copy link
Copy Markdown
Contributor

Extends the dialog rendering in guac-main.js so the heading shows a Font Awesome icon alongside the title, replacing the previous static 'Session Error' heading that was retained verbatim regardless of why the session ended.

  • Introduce _showDialog(title, detail, icon) and thin _showError / _showWarning / showSuccess wrappers around it. Each wrapper passes its corresponding module-level ICON* constant.
  • Route 514 (server timeout) and the default branch to _showError (red exclamation-circle), 522 (idle timeout) to _showWarning (amber exclamation-triangle), and 515 (backing VM disconnected = normal completion) to _showSuccess (green check-circle).
  • Simplify the dialog markup: drop the unused .message paragraph and the dead error_msg class; give the surviving body paragraph an id (#dialog-message) to mirror #dialog-heading.
  • Keep 'Session Ended' as a neutral static heading so the dialog still reads sensibly if JS fails to populate it before the dialog is shown.
  • Drive-by: remove a duplicate var apiUrl declaration in stopTask() left over from the master merge in 1230fa0 — it was a SyntaxError under strict mode.

Extends the dialog rendering in guac-main.js so the heading shows a
Font Awesome icon alongside the title, replacing the previous static
'Session Error' heading that was retained verbatim regardless of why
the session ended.

- Introduce _showDialog(title, detail, icon) and thin _showError /
  _showWarning / _showSuccess wrappers around it. Each wrapper passes
  its corresponding module-level ICON_* constant.
- Route 514 (server timeout) and the default branch to _showError
  (red exclamation-circle), 522 (idle timeout) to _showWarning (amber
  exclamation-triangle), and 515 (backing VM disconnected = normal
  completion) to _showSuccess (green check-circle).
- Simplify the dialog markup: drop the unused .message paragraph and
  the dead error_msg class; give the surviving body paragraph an
  id (#dialog-message) to mirror #dialog-heading.
- Keep 'Session Ended' as a neutral static heading so the dialog still
  reads sensibly if JS fails to populate it before the dialog is shown.
- Drive-by: remove a duplicate `var apiUrl` declaration in stopTask()
  left over from the master merge in 1230fa0 — it was a SyntaxError
  under strict mode.
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the session dialog system to support multiple notification types (Error, Warning, Success) with specific icons, updating both the HTML template and the JavaScript logic. It also removes a redundant variable declaration in the stopTask function. A security concern was identified regarding the use of .html() for injecting dialog content, which poses a risk of Cross-Site Scripting (XSS); using .text() or createTextNode is recommended instead.

Comment on lines +163 to +164
dialog.find('#dialog-heading').html(`${iconHtml}${title}`);
dialog.find('#dialog-message').html(detail);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

security-high high

Using .html() to inject title and detail can lead to Cross-Site Scripting (XSS) vulnerabilities. This is particularly concerning for the detail parameter, which can contain error messages originating from the server (e.g., via the Guacamole protocol). It is safer to use .text() for text content. For the heading, you can set the icon HTML and then append the title as a text node to ensure any malicious characters in the title are not executed.

Suggested change
dialog.find('#dialog-heading').html(`${iconHtml}${title}`);
dialog.find('#dialog-message').html(detail);
dialog.find('#dialog-heading').html(iconHtml).append(document.createTextNode(title));
dialog.find('#dialog-message').text(detail);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant