users: Avoid reading /etc/shadow #19751
Merged
Conversation
This file contains 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
|
@allisonkarlitskaya what we talked about yesterday. My hunch is that this is too JSX-y for you, and the solution is just what we talked about. But just in case you want to have a look 😉 |
|
Code looks good, but I can't tell if the failing tests are flakes or not. |
|
They are unrelated flakes, but as always their severity depends on the "3x affected" dynamics. The TestPages tracer message is pretty stubborn, it already happened here and here. I'll just add a commit here to fix it, it's trivial to do. The arch 3x affected failures need some more work. I'll have a first look. |
The test quickly iterates through a lot of languages. This starts/stops tracer in quick succession, before it can finish. This commonly causes `internal-error` messages (with translated error descriptions), ignore them.
Avoid unnecessary function wrapper and use `?.`
That bug is still alive and well after 11 years, so stop claiming it is fixed.
Split them into the usual "third party" - "our lib" - "page local" categories.
We are going to use it from the main accounts page as well.
The hashed passwords is sensitive data, and it gets copied around in the browser a lot. This isn't a data leak or security issue, as a malicious page already has the privilege to read it by itself at any time, and browsers isolate websites pretty well. However, let's be careful. We only need /etc/shadow for two reasons: - Determine if an account is locked. Let's drop the file format assumption and use `passwd -S`, just as the details page already does. Use `get_locked()` from utils.js, and rework the code to get along with it being async. - Watch the file to get notified about account changes (locked, expiry, etc.). But we only need the event, not the actual data. So install a simple no-read file watch for this. Also avoid reading utmp, as we don't look at its contents. Thanks to @jeepingben for the suggestion!
cockpituous
reviewed
Dec 17, 2023
| handleShadow.watch(() => getLogins().then(setDetails), { read: false }); | ||
|
|
||
| return [handleUtmp, handleShadow]; | ||
| }, [], null, handles => handles.forEach(handle => handle.close())); |
There was a problem hiding this comment.
This added line is not executed by any test. Details
jelly
approved these changes
Dec 18, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The hashed passwords is sensitive data, and it gets copied around in the
browser a lot. This isn't a data leak or security issue, as a malicious
page already has the privilege to read it by itself at any time, and
browsers isolate websites pretty well. However, let's be careful.
We only need /etc/shadow for two reasons:
Determine if an account is locked. Let's drop the file format
assumption and use
passwd -S, just as the details page alreadydoes. Use
get_locked()from utils.js, and rework the code to getalong with it being async.
Watch the file to get notified about account changes (locked, expiry,
etc.). But we only need the event, not the actual data. So install a
simple no-read file watch for this.
Also avoid reading utmp, as we don't look at its contents.
Thanks to @jeepingben for the suggestion!