Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

login: Warn before logging into multiple hosts #20861

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

login: Warn before logging into multiple hosts #20861

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 8 additions & 0 deletions pkg/static/login.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,14 @@
<span id="banner-message" class="pf-v5-c-alert__title"></span>
</div>

<div id="multihost-warning" class="pf-v5-c-alert pf-m-info pf-m-inline dialog-error" aria-label="inline danger alert" hidden="true">
<svg fill="currentColor" viewBox="0 0 448 512" aria-hidden="true">
<path d="M224 512c35.32 0 63.97-28.65 63.97-64H160.03c0 35.35 28.65 64 63.97 64zm215.39-149.71c-19.32-20.76-55.47-51.99-55.47-154.29 0-77.7-54.48-139.9-127.94-155.16V32c0-17.67-14.32-32-31.98-32s-31.98 14.33-31.98 32v20.84C118.56 68.1 64.08 130.3 64.08 208c0 102.3-36.15 133.53-55.47 154.29-6 6.45-8.66 14.16-8.61 21.71.11 16.4 12.98 32 32.1 32h383.8c19.12 0 32-15.6 32.1-32 .05-7.55-2.61-15.27-8.61-21.71z" />
</svg>
<p id="multihost-message" class="pf-v5-c-alert__title"></p>
<button id="multihost-get-me-there" class="pf-v5-c-button">Go there</button>
</div>

<span id="badge"></span>

<div class="container" id="main">
Expand Down
12 changes: 10 additions & 2 deletions pkg/static/login.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,8 +355,16 @@ import "./login.scss";
}
}

if (cur_machine && !environment.page.allow_multihost)
redirect_to_current_machine();
if (cur_machine) {
if (!environment.page.allow_multihost)
redirect_to_current_machine();
Copy link
Contributor

Choose a reason for hiding this comment

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

This added line is not executed by any test.

else {
id("multihost-message").textContent = format(_("You are already connected to '$0' in this browser session. Connecting to other hosts will allow them to execute arbitrary code on each other. Please be careful."),
cur_machine == "." ? "localhost" : cur_machine);
Copy link
Contributor

Choose a reason for hiding this comment

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

This added line is not executed by any test.

id("multihost-get-me-there").addEventListener("click", redirect_to_current_machine);
show('#multihost-warning');
}
}
}

function boot() {
Expand Down
4 changes: 2 additions & 2 deletions pkg/static/login.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,14 +354,14 @@ label.checkbox {
display: none;
}

.login-pf #banner {
.login-pf #banner, .login-pf #multihost-warning {
margin-block: 1rem 0.5rem;
margin-inline: 0;
grid-area: banner;
inline-size: 100%;
}

#banner-message {
#banner-message, #multihost-message {
Comment on lines -357 to +364
Copy link
Member

Choose a reason for hiding this comment

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

We really shouldn't target IDs in the CSS. We already do, I know, but we shouldn't add more. (The selectors should be targeting classes.)

We especially should not have IDs nested under other selectors like this.

Ideally, we'd fix the older parts by changing those too.

Anyway, I'm not expecting you to have to fix this, just pointing out that it's bad form. We're already doing bad stuff anyway in the file, so it's not a huge deal. But it's important for new code and we should (at some point) clean up instances where IDs are used in CSS. It's important everyone on the team understands this, as using IDs leads to other bad things, like CSS not working as expected, resulting in !important and other bad things.

https://developer.mozilla.org/en-US/docs/Learn/CSS/Building_blocks/Selectors/Type_Class_and_ID_Selectors#id_selectors

The ID selector has high specificity. This means styles applied based on matching an ID selector will overrule styles applied based on other selector, including class and type selectors. Because an ID can only occur once on a page and because of the high specificity of ID selectors, it is preferable to add a class to an element instead of an ID. If using the ID is the only way to target the element — perhaps because you do not have access to the markup and cannot edit it — consider using the ID within an attribute selector, such as p[id="header"]. Learn specificity.

(Note: We shouldn't use an attribute selector as it suggests, but actually fix the HTML.)

IDs are great and to be used in JavaScript for quick specific element selecting (as there's only one unique instance of an ID allowed on a document), of course. But it's bad for CSS.

Copy link
Member

Choose a reason for hiding this comment

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

(BTW: I'm not blocking on this in this PR or nitpicking, just pointing out something that would have really bad side effects, especially elsewhere in Cockpit.)

white-space: pre-wrap;
max-block-size: 12em;
overflow: auto;
Expand Down