Studio: Write auth secret files with a trailing newline - #7576
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ee981f7b4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if raw != _bootstrap_file_bytes(_bootstrap_password): | ||
| try: | ||
| _persist_bootstrap_password(_bootstrap_password) |
There was a problem hiding this comment.
This rewrite only runs inside generate_bootstrap_password(), but the normal upgrade case already has an admin row, so ensure_default_admin() returns through its existing-user branch and calls _load_bootstrap_password() instead of this code. In that scenario a legacy .bootstrap_password written without an LF remains unchanged, so cat still welds the shell prompt into the copied password. Please share this migration with _load_bootstrap_password() or the existing-admin path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and fixed in d404cad (before this review landed, so the reasoning matches independently).
ensure_default_admin() short-circuits to _load_bootstrap_password() once the admin row exists, which is exactly the upgrade case, so the migration was sitting on a path only fresh installs reach. Both readers now share _read_persisted_bootstrap_password().
Verified against the real path rather than by calling generate_bootstrap_password() directly: test_upgrade_normalises_the_bootstrap_file seeds the admin row first and goes through ensure_default_admin().
for more information, see https://pre-commit.ci
Both writers used text mode, so on Windows the trailing newline became CRLF. The Windows Studio smoke jobs run under bash and read the file with OLD=$(cat ...), which strips the LF but leaves the CR attached, so the credential goes into the login body as "<secret>\r" and the request fails. Write bytes in the backend and pin newline in the CLI so the file is "<secret>\n" on every platform. generate_bootstrap_password() also returned early on an existing file, so upgraded installs kept the original problem; it now rewrites anything that isn't already exactly "<secret>\n", best-effort so a read-only auth dir cannot fail startup. The raw test assertions used read_text(), which decodes CRLF back to "\n" and would have stayed green on Windows. They read bytes now.
for more information, see https://pre-commit.ci
ensure_default_admin() short-circuits to _load_bootstrap_password() once the admin row exists, so the normalisation added in the previous commit sat on generate_bootstrap_password(), which only fresh installs reach. An upgraded install kept its newline-less file. Both readers now share _read_persisted_bootstrap_password(). Make the write atomic while it is here: it can now rewrite a live file, and a partial write would destroy the only plaintext copy of the recovery credential. Same mkstemp plus os.replace shape the CLI writer already uses. Tests cover the upgrade path through ensure_default_admin(), a well-formed file not being rewritten on every start, a failing migration not blocking startup, and the atomic replace.
for more information, see https://pre-commit.ci
99a6e02 to
3765f04
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3765f04407
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # "<secret>\n". Best-effort: a read-only auth dir must not fail startup. | ||
| if raw != _bootstrap_file_bytes(password): | ||
| try: | ||
| _persist_bootstrap_password(password) |
There was a problem hiding this comment.
When another Studio process changes the password between read_bytes() and this normalization write, clear_bootstrap_password() or the CLI cleanup can delete the credential after committing the new hash, but _persist_bootstrap_password(password) then atomically recreates the deleted file from the stale bytes. This leaves revoked plaintext on disk and allows that old bootstrap password to be re-seeded after a later auth DB reset; synchronize normalization with credential cleanup or verify that the source file is still present and unchanged before replacing it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, fixed in 729cb51.
The rename was the problem: os.replace recreates the path, so a clear_bootstrap_password() or CLI cleanup landing between the read and the write had its deletion undone, leaving revoked plaintext for a later auth.db reset to re-seed.
Normalisation now opens the existing file without O_CREAT and rewrites through that descriptor, so a deleted file cannot be resurrected, and it re-checks the contents through the same descriptor so an in-place truncation or a rotated credential is not clobbered either.
That gives up the atomic rename, so the in-place path is restricted to trailing-whitespace fixes. Every partial state is then the secret plus leftover whitespace, which still strips to the same credential. Files with leading whitespace are left unnormalised and keep working, since every reader strips. Creation still goes through the atomic writer.
Covered by test_normalising_never_recreates_a_cleared_bootstrap_file, test_normalising_does_not_overwrite_a_rotated_bootstrap_file and test_leading_whitespace_bootstrap_file_is_left_alone.
…leared The rename-based rewrite could recreate the file: if a password change ran clear_bootstrap_password(), or the CLI cleanup deleted it, between the read and the write, os.replace put the revoked plaintext back on disk, where a later auth.db reset would re-seed it. Open the existing file without O_CREAT instead, so a deleted file cannot be resurrected, and re-check the contents through that descriptor so an in-place truncation or a rotated credential is not overwritten either. That gives up the atomic rename, so the in-place path is restricted to trailing-whitespace fixes. Every partial state is then the secret plus leftover whitespace, which still strips to the same credential. Files with leading whitespace are left alone; every reader strips, so they keep working. Creation still goes through the atomic writer.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 729cb51e44
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| os.write(fd, data) | ||
| os.ftruncate(fd, len(data)) | ||
| try: | ||
| os.fchmod(fd, 0o600) |
There was a problem hiding this comment.
On Windows, os.fchmod is unavailable, so upgrading with a legacy bootstrap file triggers an AttributeError here after normalization. The surrounding handler catches only OSError, allowing the exception to escape through _read_persisted_bootstrap_password() and abort the first Studio startup for Windows users who still have the bootstrap credential; use the path-based chmod fallback or guard this call by platform/attribute availability.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 1fcdfb3.
os.fchmod is Unix-only until 3.13 ("Changed in version 3.13: Added support on Windows", gh-113191), and pyproject.toml declares >=3.9,<3.15, so 3.9 through 3.12 on Windows hit an AttributeError that is not an OSError and escapes both handlers.
Caught as AttributeError alongside OSError rather than falling back to a path-based chmod: this function deliberately stays on the descriptor after the open so a concurrently deleted or rotated file cannot be clobbered, and the mode was already set by the writer that created the file.
Covered by test_normalising_works_without_fchmod, which delattr's os.fchmod.
| if os.read(fd, len(raw) + 1) != raw: | ||
| return | ||
| os.lseek(fd, 0, os.SEEK_SET) | ||
| os.write(fd, data) |
There was a problem hiding this comment.
os.write() may legally return after writing fewer than len(data) bytes, but this return value is ignored and the file is then extended to len(data). If a short write occurs while migrating a legacy credential, the extension is NUL-filled, so subsequent reads produce a password containing \0 that no longer matches the hash in auth.db, locking the user out after restart; loop until all bytes are written or treat a short write as a failed normalization without truncating.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 1fcdfb3, now a write loop.
Reproduced the consequence exactly as described: a one-byte-short write leaves b"legacy-bootstrap-secret\x00" after the ftruncate NUL-extends it, and \x00 is not whitespace so .strip() keeps it, meaning the next start hands out a credential that no longer matches the hash in auth.db. POSIX allows the short write ("Note that a successful write() may transfer fewer than count bytes") and ftruncate specifies the zero fill ("If the file size is increased, the extended area shall appear as if it were zero-filled").
A stalled write now returns without truncating, which leaves the secret plus leftover trailing whitespace, and that still strips to the same credential.
While fixing this I found a third defect in the same block that is more serious than either item, so 1fcdfb3 covers it too: os.open does not add O_BINARY on Windows and CPython never changes the CRT default of _O_TEXT, so the descriptor was in text mode. os.write turned the LF back into CRLF and ftruncate then cut the LF off, leaving "\r" -- the original bug, reintroduced by the migration, and a fixed point that never converges. os.read translates in reverse, so a genuinely CRLF file also failed verification and was skipped. Covered by test_normalising_opens_the_file_in_binary_mode and test_normalising_survives_a_short_write.
Three defects in the in-place normalisation, all on the Windows upgrade path. os.open does not add O_BINARY on Windows and CPython never changes the CRT default of _O_TEXT, so the descriptor was in text mode: os.write turned the LF straight back into CRLF and ftruncate then cut the LF off, leaving "<secret>\r". That is the bug this PR exists to fix, reintroduced by the migration itself, and it is a fixed point that never converges. os.read translates in reverse too, so a genuinely CRLF file failed verification and was silently skipped. os.write may return having written fewer bytes than asked; ftruncate would then NUL-extend the credential so it no longer matched the hash in auth.db. os.fchmod only reached Windows in 3.13 and AttributeError is not OSError, so on 3.9 to 3.12 it escaped both handlers and aborted the first start after upgrade.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1fcdfb3048
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| while written < len(data): | ||
| n = os.write(fd, data[written:]) |
There was a problem hiding this comment.
When password clearing cannot unlink this open file (notably on Windows), clear_bootstrap_password() falls back to truncating it through another descriptor. If that truncation occurs after the equality check on line 90 but before this write, normalization restores the revoked plaintext, defeating the cleanup and allowing a later database reset to reuse the old bootstrap credential. The migration needs synchronization with clearing or must avoid rewriting once concurrent invalidation can occur.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 9a21e8f. Normalisation is now append-only.
The window is real: clear_bootstrap_password() falls back to write_text("") through its own descriptor when the unlink fails, and on Windows the unlink fails precisely because this descriptor is open (the CRT opens without FILE_SHARE_DELETE). A truncation landing between the equality check and the write meant the rewrite restored the revoked plaintext on the same inode, which the fd-only open did not protect against.
Rather than synchronise, the rewrite is gone. The migration now appends a single LF, and only to a file whose bytes are exactly the credential. An append cannot restore a revoked secret: over a cleared file the result is a lone newline, which strips to empty and reads back as no bootstrap password. Verified for both clearing paths, unlink and truncate, and for a concurrently rotated file.
Restricting it to that one shape costs nothing, because releases before the newline wrote the password with no terminator at all (write_text(_bootstrap_password) on main), so it is the only shape in the wild. A CRLF file could only come from an unreleased build of this branch. Everything else is left byte-identical and keeps working, since every reader strips.
Never truncating also removes the short-write NUL-fill hazard from the previous round entirely, so that loop is gone. O_BINARY stays, or Windows would turn the appended LF into CRLF.
Covered by test_clearing_by_truncation_mid_normalisation_is_not_undone, test_normalising_never_recreates_a_cleared_bootstrap_file, test_normalising_does_not_overwrite_a_rotated_bootstrap_file and test_only_an_exactly_untermimated_bootstrap_file_is_touched.
clear_bootstrap_password() falls back to truncating the file through its own descriptor when the unlink fails, which is what happens on Windows while this one is open. That truncation could land after the equality check and before the write, so the rewrite put the revoked plaintext back. Append a single LF instead, and only to a file that is exactly the credential. An append cannot restore a revoked secret: over a cleared file the result is a lone newline, which strips to empty and reads back as no bootstrap password. Releases before the newline wrote the password with no terminator at all, so that is the only shape in the wild; anything else is left alone and keeps working because every reader strips. Never truncating also removes the short-write NUL-fill hazard entirely, so the write loop is gone. O_BINARY stays: without it Windows would turn the appended LF into CRLF.
for more information, see https://pre-commit.ci
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
3 participants
.bootstrap_passwordand.desktop_secretwere written without a trailing newline, socatruns the credential straight into the shell prompt and the prompt gets selected along with it. The paste then fails as "Login failed", which reads as a wrong password rather than a bad copy.Fixes both writers the backend's
generate_bootstrap_password()(the one that runs on a normal server boot) and the CLI's shared_write_auth_secret(). Every reader already strips: backend storage, both src-tauri readers, and$(cat)inCI. Updates four tests that read these files without stripping, and adds coverage that the files end in
\nand still authenticate.