Studio: auto-generate admin password for headless public launches, plus opt-in one-time link token#7392
Conversation
…ches, add opt-in one-time link token Part A (core): a public launch (--secure, or --cloudflare producing a public tunnel, or Colab start(cloudflare=True)) that supplies no password and still has the seeded must-change admin password now auto-generates a strong secrets.token_urlsafe(24) password, commits it through the existing update_password path (which clears must_change_password, rotates the JWT secret, revokes refresh tokens, and deletes the on-disk bootstrap password), and prints the username + password once to stderr (or into the Colab cell). The credential is never persisted to disk and never placed on argv. Because must_change_password becomes 0, the run.py terminal password gate returns without prompting and the Colab _bootstrap_password_pending() check passes, so the tunnel proceeds headlessly instead of failing closed or relying on the bootstrap shutdown deadline. The unsloth_cli studio parent gate mirrors the same behavior before re-exec, keeping the two in sync; a real terminal still gets the interactive prompt, and a supplied --password is respected and never overwritten. Part B (opt-in, default off): a one-time, short-TTL (~10 min) HMAC-signed link token for the Colab same-tab proxy URL only. The token is signed with a key derived from the admin's JWT secret (domain-separated so it is never accepted as a bearer access token), single-use via a link_tokens nonce table, constant-time verified, expiry-enforced, and bound to the admin identity. New backend endpoint POST /api/auth/link-exchange consumes a valid token once and issues the normal JWT + refresh pair. The Colab helper appends ?link_token=... to the same-tab URL only (never the shared Cloudflare link) when enabled via start(link_token=True) or UNSLOTH_STUDIO_COLAB_LINK_TOKEN=1; the token is never logged. Frontend consume + history.replaceState scrub is left as an inline TODO rather than shipping a half-wired UI, so today the token is emitted but harmless and the login page still works. Adds pytest coverage for auto-generation, supplied-password precedence, non-public no-op, and link-token mint/exchange/single-use/expiry/tamper cases; updates the backend and CLI gate tests to the new headless contract.
for more information, see https://pre-commit.ci
![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: 74f6962b96
ℹ️ 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".
| Logs a plaintext copy (so it survives even if HTML display fails) and renders | ||
| a branded card. Shown once only; never persisted. | ||
| """ | ||
| logger.info(f"🔐 Unsloth Studio admin login — username: {username} password: {password}") |
There was a problem hiding this comment.
When start(cloudflare=True) auto-generates a password, this call happens after run_server() has initialized its stdout/stderr tee to the retained session-log file, and the backend logging configuration writes structlog output to that stdout. As a result, the plaintext admin password is retained in ~/.unsloth/studio/logs/server/ (by default for up to 20 sessions), contrary to the intended one-time/non-persistent credential flow; anyone with later access to those logs can authenticate to the public Studio instance.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. structlog writes to sys.stdout and run._setup_server_disk_logging tees stdout/stderr to a retained session log, so the logged password was persisted on disk for up to 20 sessions. Fixed in 04c0c3c: _display_admin_credentials no longer logs; it surfaces the credential only through the IPython display channel (iopub display_data), with a plain-text fallback on the same channel and nothing when no notebook is present, so the password never reaches stdout. Regression test test_display_admin_credentials_never_writes_to_stdout asserts the password is absent from captured stdout/stderr.
| DELETE FROM link_tokens | ||
| WHERE jti = ? AND username = ? AND expires_at >= ? | ||
| RETURNING jti | ||
| """, |
There was a problem hiding this comment.
This DELETE ... RETURNING requires SQLite 3.35+, but the package supports Python 3.9 without declaring a SQLite minimum; supported distro Python installations can still be linked against older SQLite versions (for example Ubuntu 20.04's 3.31). On those environments token creation succeeds, but exchanging it raises sqlite3.OperationalError at this statement and /api/auth/link-exchange returns 500 instead of consuming the token.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, DELETE ... RETURNING needs SQLite >= 3.35 (2021-03-12). Fixed in 04c0c3c: consume_link_token now uses a WHERE-qualified DELETE and checks cur.rowcount == 1. A WHERE-qualified DELETE reports an exact rowcount, and SQLite serializes writers under a single database-level write lock (busy_timeout=5000), so of two concurrent exchanges only one DELETE removes the row (rowcount 1) and the other matches nothing (rowcount 0); single-use is preserved without the version dependency. Added test_consume_link_token_is_single_use.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e104875e6
ℹ️ 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".
| key = _link_token_key(subject) | ||
| if key is None: | ||
| return None | ||
| expected_sig = hmac.new(key, payload_b64.encode("ascii"), hashlib.sha256).digest() |
There was a problem hiding this comment.
A password change can race this validation: if exchange_link_token() reads the old JWT-derived key here, then update_password() rotates the JWT secret, the already-validated exchange can still consume its JTI and the route will issue a new session using the new secret. This violates the stated invalidation guarantee for outstanding link tokens and lets a holder of a pre-change link token regain access during that window. Bind consumption to the current secret/version in the same transaction, or delete the user's link-token rows as part of update_password().
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 04c0c3c by deleting the user's link-token rows inside update_password's transaction (unconditionally, in the same commit that rotates the JWT secret). Since the link-token signing key is derived from that secret, a leftover token already fails its signature check after rotation; deleting the rows atomically closes the narrow window where an in-flight exchange read the old derived key before the rotation and could still consume its jti. New test test_password_change_deletes_outstanding_link_tokens asserts the jti row is gone after update_password and that the exchange is then rejected.
…time link tokens Do not log the auto-generated Colab admin password: structlog writes to sys.stdout, which run._setup_server_disk_logging tees to a retained on-disk session log, so the plaintext credential was persisted for up to 20 sessions. Show it only through the IPython display channel (iopub display_data), with a plain-text fallback on the same channel and nothing when no notebook is present. Consume one-time link tokens with a WHERE-qualified DELETE and rowcount check instead of DELETE ... RETURNING, which needs SQLite >= 3.35; SQLite serializes writers so single-use is preserved without the version dependency. Delete a user's outstanding link tokens inside update_password's transaction so a password change atomically invalidates them, closing the race where an in-flight exchange read the old JWT-derived key before the secret rotation. Add regression tests and fix the in-venv broken-backend CLI test to stub the servable-frontend guard so it reaches the backend-load guard under test.
for more information, see https://pre-commit.ci
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6877d0ed48
ℹ️ 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".
| # and surface it once. This also protects the api-only / TIMEOUT=0 launches | ||
| # that the deadline never covered. | ||
| generated = _auto_generate_admin_password(_admin) | ||
| _print_auto_generated_credentials(_admin, generated, out = sys.stderr) |
There was a problem hiding this comment.
For a headless direct backend launch such as python studio/backend/run.py --secure, run_server() has already called _setup_server_disk_logging() before this gate runs, and that helper replaces sys.stderr with _TeeStream. Thus this banner, including the generated password, is copied to a retained logs/server/server-*.log file despite the promise that it is shown once and not persisted. Print before enabling the tee, or write the credential to the original unwrapped stderr.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. run_server() installs the _TeeStream over sys.stderr before this gate runs, so the banner was being mirrored into the retained logs/server/server-*.log. Fixed: the credential now writes to the raw console stream behind the tee via _console_only_stream(sys.stderr), so it reaches the operator once but never the on-disk log. Colab already goes through the IPython display channel, not stderr, so it was unaffected. Added a regression test asserting the password lands on the console but not in the log handle.
| generated = secrets.token_urlsafe(24) | ||
| _cli_update_password(conn, DEFAULT_ADMIN_USERNAME, generated) | ||
| _echo_auto_generated_credentials(DEFAULT_ADMIN_USERNAME, generated) |
There was a problem hiding this comment.
When a link token was minted and its exchange has already passed signature validation, this new headless CLI path rotates the JWT secret through _cli_update_password but leaves its link_tokens row intact. The concurrent exchange can then consume that stale jti after the password change and mint a session using the new secret. Backend storage.update_password() deletes these rows in the same transaction specifically to close this race; extend the CLI mirror transaction to do the same.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. The CLI mirror now deletes link_tokens for the user in the same transaction that rotates the JWT secret, matching backend storage.update_password. _connect_auth_db also creates the link_tokens table so the DELETE never errors on a DB the CLI touched before the backend ran. Regression test asserts an outstanding row is gone after _cli_update_password.
|
|
||
| def save_link_token(jti: str, username: str, expires_at: str) -> None: | ||
| """Record a minted one-time link token so it can be consumed exactly once. | ||
|
|
||
| Only the opaque jti (a random id) is stored; the token signature never | ||
| touches disk. | ||
| """ | ||
| conn = get_connection() | ||
| try: | ||
| conn.execute( | ||
| "INSERT INTO link_tokens (jti, username, expires_at) VALUES (?, ?, ?)", | ||
| (jti, username, expires_at), |
There was a problem hiding this comment.
Every opt-in start(link_token=True) inserts a row here, but expired rows are only removed inside consume_link_token. The frontend is explicitly not yet wired to exchange these tokens, so those rows are normally never consumed and accumulate permanently on every rerun. Remove expired rows when saving/minting too (and consider indexing expires_at) so the auth database does not grow indefinitely.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done. save_link_token now purges expired rows in the same transaction as the insert (mirroring the purge already in consume_link_token), so the table stays bounded even while the frontend does not yet exchange these tokens. Also added an index on expires_at so both purge sites stay cheap. Regression test asserts a stale row is reclaimed on the next mint.
… revoke/purge link tokens Three security-hardening fixes for the headless auto-password path: - run.py: a direct `python run.py --secure` launch installs a _TeeStream over sys.stderr (session disk logging) before the pre-tunnel gate runs, so the one-time auto-generated admin password was mirrored into the retained logs/server/server-*.log despite the banner promising it is not persisted. Write the credential to the raw console stream behind the tee via a new _console_only_stream() helper so it reaches the operator once but never disk (OWASP CWE-532). The Colab path already uses the IPython display channel and was unaffected. - unsloth_cli/commands/studio.py: _cli_update_password now deletes the user's link_tokens in the same transaction that rotates the JWT secret, mirroring backend storage.update_password, so a concurrent exchange that read the pre-rotation derived key cannot consume a stale jti after the change. _connect_auth_db creates the link_tokens table so the DELETE never errors on a DB the CLI created before the backend ran. - auth/storage.py: save_link_token purges expired rows in the same transaction as the insert (mirroring consume_link_token) and an index on expires_at keeps both purge sites cheap, so the table stays bounded while the frontend does not yet exchange these tokens. Adds regression tests for each: credential absent from the tee log handle, link_tokens revoked by the CLI password change, and expired link tokens reclaimed on the next mint.
|
@codex review |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0be9197c9e
ℹ️ 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".
| # _TeeStream that _setup_server_disk_logging() installed: the tee mirrors | ||
| # everything into a retained server-*.log, and this password must never be | ||
| # persisted (the banner itself promises it is not written to disk). | ||
| _print_auto_generated_credentials(_admin, generated, out = _console_only_stream(sys.stderr)) |
There was a problem hiding this comment.
In direct headless launches where sys.stderr is None (for example GUI/service wrappers), this call passes None as file, and Python's print(file=None) falls back to the already tee'd sys.stdout. Fresh evidence for this log-persistence edge case is that _console_only_stream only unwraps _TeeStream and otherwise returns the original object, so an absent stderr still writes the generated admin password into the retained logs/server/server-*.log after _auto_generate_admin_password has rotated the only credential. Detect a missing raw stream before generating/committing the password, or fail closed instead of letting print choose stdout.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9c296c2. Resolved the console stream before generating/committing the credential: _one_time_secret_stream() unwraps the tee and prefers stderr then stdout, and when neither has a real raw stream (stderr and stdout both None, e.g. a pythonw/service wrapper) the gate now fails closed WITHOUT rotating, rather than letting print(file=None) fall back to the tee'd stdout and persist the password into the session log (CWE-532). Regression tests cover the stdout fallback and the no-console fail-closed.
| # commit it (which clears must_change so the tunnel proceeds headlessly), | ||
| # and surface it once. This also protects the api-only / TIMEOUT=0 launches | ||
| # that the deadline never covered. | ||
| generated = _auto_generate_admin_password(_admin) |
There was a problem hiding this comment.
For a direct backend launch such as python studio/backend/run.py --secure with no TTY, when cloudflared is missing or cannot be downloaded, this line still commits a new random admin password before the tunnel is attempted; the later secure and not _cloudflare_url check then exits without exposing any public link. In that failure mode the bootstrap recovery credential has been replaced solely by a one-time stderr banner, so supervisors/nohup-style launches that do not preserve it can leave the user locked out until reset-password. Delay the rotation until the tunnel can start, or do the same early tunnel-availability check the CLI path performs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9c296c2. The direct backend --secure path now mirrors the CLI: _tunnel_binary_confirmed_unavailable() runs before rotating, and when cloudflared is provably unavailable the gate refuses and leaves the existing recovery credential intact instead of rotating it behind a one-time banner a supervisor/nohup launch may drop. Scoped to --secure only (a --cloudflare wildcard bind still serves the raw port, so it still rotates); both cases have regression tests.
…/studio-secure-auto-password
…/danielhanchen/unsloth-staging-2 into feat/studio-secure-auto-password
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8dd5ad3177
ℹ️ 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".
| return Token( | ||
| access_token = create_access_token(subject = username), | ||
| refresh_token = create_refresh_token(subject = username), |
There was a problem hiding this comment.
A password change can still lose the race when an exchange validates and consumes its link-token row just before update_password() commits: that password change finds no row to delete, then this route creates the access and refresh tokens afterward using the newly rotated JWT secret, leaving the pre-change link-token holder authenticated. Fresh evidence is that consume_link_token commits its DELETE before exchange_link_token returns, while this route creates both credentials only afterward. Bind consumption and session issuance to a password/JWT-secret version in one transaction (or otherwise recheck it) so a password change cannot be bypassed by this ordering.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9c296c2. exchange_link_token_with_secret() now returns the JWT secret whose derived key validated the token, and the route binds issuance to it: after minting, if storage.get_jwt_secret() no longer equals that secret a rotation raced the consumption, so it revokes the just-minted refresh token and returns 401. A rotation that lands after the recheck is caught by that same refresh-token revocation plus the JWT signature change, so no link-exchange session outlives a password change. Regression test simulates the concurrent rotation between consume and issuance.
…nel lockout, and link-exchange rotation race
|
@codex review |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: daa5fbb329
ℹ️ 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".
| _cli_update_password(conn, DEFAULT_ADMIN_USERNAME, generated) | ||
| _echo_auto_generated_credentials(DEFAULT_ADMIN_USERNAME, generated) |
There was a problem hiding this comment.
When the parent unsloth studio --secure path is non-interactive and stderr is absent or unusable (for example a Windows service/pythonw launcher or a closed stderr), this commits the generated password before _echo_auto_generated_credentials() tries to display it. Typer/Click can no-op or raise in that state, so the launch can continue or exit without ever showing the only new credential, after the bootstrap password has been deleted; mirror the backend console-stream preflight/fail-closed behavior before committing the rotation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. Fixed in 555c811: the non-interactive --secure path now resolves a usable console stream (stderr, then stdout) via _one_time_secret_console_stream() BEFORE rotating, mirroring run.py's _one_time_secret_stream preflight. When neither is usable (pythonw/service wrapper or closed stderr) it refuses without rotating, so the seeded bootstrap password stays intact for recovery, and the credential is printed to the exact stream that was verified usable. Regression test test_studio_default_non_tty_no_console_preserves_bootstrap.
| if generated is not None: | ||
| _display_admin_credentials(DEFAULT_ADMIN_USERNAME, generated) |
There was a problem hiding this comment.
When IPython.display is unavailable or display() raises, _display_admin_credentials() catches the failure and returns silently, but this path has already committed the generated password and then continues toward starting the Cloudflare tunnel. In that case the notebook shows neither the new password nor a usable bootstrap credential, so the user cannot log in to the published Studio without resetting the password; make display success explicit or fail closed before publishing the link.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. Fixed in 555c811: _display_admin_credentials now returns whether the credential actually reached the IPython display channel, and start_cloudflare_tunnel fails closed (no tunnel published) when the rotated password could not be shown, rather than publishing a shared link under a password nobody saw. It still never falls back to stdout/logging (the server tees those to disk). Regression tests test_start_cloudflare_tunnel_fails_closed_when_display_fails plus the True/False return cases.
| class LinkTokenRequest(BaseModel): | ||
| """One-time link-token exchange payload (opt-in Colab same-tab handoff).""" | ||
|
|
||
| link_token: str = Field(..., description = "One-time, short-lived link token to exchange") |
There was a problem hiding this comment.
For public Studio instances, /api/auth/link-exchange is unauthenticated and /api/auth is not covered by the request-body cap in MaxBodyMiddleware; this new schema accepts an unbounded string, so a client can post a very large link_token and force FastAPI plus exchange_link_token_with_secret() to hold, scan, decode, and HMAC attacker-controlled data before rejecting it. Add a small max_length appropriate for these ~hundreds-byte tokens, or include the auth route in the body limiter.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 555c811 at both layers. /api/auth now joins the ASGI MaxBodyMiddleware protected prefixes with its own small dedicated cap (AUTH_REQUEST_BODY_MAX_BYTES = 64 KiB), bounding the buffered body before FastAPI reads it; and LinkTokenRequest.link_token gets max_length = 4096 so an oversized token is a 422 before exchange_link_token_with_secret() scans/decodes/HMACs it. Regression tests test_link_token_schema_caps_length, test_link_exchange_route_rejects_oversized_token, and test_auth_routes_are_body_capped.
…hown, and cap link-token size - CLI --secure headless: resolve a usable console stream before rotating the seeded recovery password. On a Windows pythonw/service wrapper or a closed stderr, typer.echo(err=True) silently no-ops and the one-time credential is lost after the bootstrap password was deleted, locking the operator out. Mirror run.py's _one_time_secret_stream preflight: refuse without rotating so the bootstrap password stays intact, and print the credential to the exact stream that was verified usable. - Colab public launch: _display_admin_credentials now reports whether the credential reached the IPython display channel. start_cloudflare_tunnel fails closed (no tunnel) when the rotated password could not be shown, instead of publishing a shared link under a password nobody ever saw. Never falls back to stdout/logging (the server tees those to an on-disk log). - Cap /api/auth/link-exchange input: LinkTokenRequest.link_token gets a max_length, and /api/auth joins the ASGI body limiter with a small dedicated cap, so an unauthenticated client cannot force FastAPI to buffer and the exchange to scan/decode/HMAC an oversized token. - Regression tests for all three paths.
…/danielhanchen/unsloth-staging-2 into feat/studio-secure-auto-password
for more information, see https://pre-commit.ci
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1014bc24ad
ℹ️ 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".
| for candidate in (sys.stderr, sys.stdout): | ||
| raw = _console_only_stream(candidate) | ||
| if raw is not None: | ||
| return raw |
There was a problem hiding this comment.
For direct python run.py --secure headless launches where sys.stderr is a closed stream (and stdout is absent or also closed), this helper returns that stream because it only checks for None. The gate then commits the generated password—which clears the bootstrap credential—before _print_auto_generated_credentials raises on the closed stream, so the operator never receives the only new password and is locked out. Mirror the CLI helper's closed/writable-stream checks before returning a stream.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. _one_time_secret_stream only checked for None, so a closed sys.stderr on a headless launch was returned and _auto_generate_admin_password rotated the seeded credential before _print_auto_generated_credentials raised ValueError on the closed stream, locking the operator out. Mirroring the CLI _one_time_secret_console_stream closed/writable checks on the unwrapped raw stream so we fail closed WITHOUT rotating when no usable console exists. Regression test added.
… non-writable _one_time_secret_stream only checked the unwrapped raw stream for None, so a headless launch inheriting a closed or non-writable sys.stderr returned that stream. The gate then committed the generated password (clearing the seeded bootstrap credential) before _print_auto_generated_credentials raised ValueError on the closed stream, so the operator never received the only new password and was locked out. Mirror the CLI _one_time_secret_console_stream closed/writable preflight on the unwrapped stream so the direct python run.py path fails closed WITHOUT rotating when no usable console exists.
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bdc68ecce
ℹ️ 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".
| if getattr(raw, "closed", False): | ||
| continue | ||
| if not callable(getattr(raw, "write", None)): | ||
| continue | ||
| except (AttributeError, ValueError): | ||
| continue | ||
| return raw |
There was a problem hiding this comment.
When this is launched headlessly with stderr/stdout redirected (for example nohup, systemd, Docker logging, or > server.log 2>&1), those streams are still writable and therefore pass this check. The subsequent startup banner writes the newly generated admin password into that retained file or journal, despite the one-time-password path promising not to persist it. Require an actual TTY (or otherwise refuse this auto-generation path) before rotating the credential; the CLI mirror has the same predicate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 6e44308. _one_time_secret_stream unwrapped only the internal session-log tee, so under nohup / systemd / > log 2>&1 the underlying stderr/stdout is a writable but non-tty file/journal and the auto-generated password landed there, persisting the plaintext despite the no-disk banner. It now requires a real TTY: a non-tty console is skipped, and with no tty the gate fails closed WITHOUT rotating the seeded recovery credential. A headless launch supplies --password / UNSLOTH_STUDIO_PASSWORD / stdin. Regression: test_gate_fails_closed_when_console_is_redirected_file.
| # Same-tab URL may carry the one-time token; the shared link never does. | ||
| same_tab_url = _append_link_token(url, same_tab_link_token) |
There was a problem hiding this comment.
With start(link_token=True) or the environment opt-in, this appends a bearer token to the iframe URL but no frontend source consumes link_token or calls /api/auth/link-exchange (the TODO immediately above confirms this). Consequently users still get the normal login page, while the unused credential remains in the page URL/history until expiry instead of being exchanged and scrubbed; either wire the frontend exchange/scrub before exposing the option or do not mint/append the token.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
link_token is an explicit opt-in, default OFF (start(link_token=True) or UNSLOTH_STUDIO_COLAB_LINK_TOKEN=1); the frontend TODO is documented in _show_and_embed and the start() docstring. On the default path nothing is minted or appended, so there is no leak for a normal user. When opted in, the token rides only the same-tab proxy URL in the operator's own browser (never the shared Cloudflare link) and is single-use with a short TTL. Keeping this as the tracked TODO rather than changing behavior: the frontend exchange + history scrub lands with the same opt-in.
Resolve conflicts in studio/backend/colab.py and studio/Unsloth_Studio_Colab.ipynb. colab.py: keep main's refactored Colab embed structure (kernel-port vs HTML iframe split, ready-card, show_link, cloudflare auto-enable, warn-on-missing) and layer the secure-launch flow on top. The shared Cloudflare link keeps the auto-generated admin password (rotated, refresh tokens revoked, bootstrap file removed), the fail-closed display (no tunnel when the credential cannot be shown in the cell), no credential on the shared link, no password or link token to stdout or logs, and the opt-in one-time same-tab link token threaded only onto the HTML iframe src. start() routes the shared-link credential through start_cloudflare_tunnel's auto-generate path instead of finalizing and caching the bootstrap password to disk, so the password is never persisted. Notebook: keep main's structure and carry the auto-generated-password note plus the UNSLOTH_STUDIO_PASSWORD override. Adjust _embed_html_iframe mocks for the new same_tab_url kwarg and add tests asserting start() never finalizes or caches credentials and threads the opt-in link token.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f56ccb346
ℹ️ 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".
| @router.post("/link-exchange", response_model = Token) | ||
| async def link_exchange(payload: LinkTokenRequest) -> Token: | ||
| """Exchange a one-time, short-TTL link token for normal session tokens. | ||
|
|
||
| Powers the opt-in Colab same-tab handoff: the same-tab URL carries a | ||
| single-use ``?link_token=...`` the UI posts here to obtain the same JWT the | ||
| login form issues. The token is consumed here (a replay is rejected) and is | ||
| never logged. Unauthenticated by design -- the token itself is the credential. | ||
| """ | ||
| exchanged = exchange_link_token_with_secret(payload.link_token) |
There was a problem hiding this comment.
On a public Studio instance, this new unauthenticated endpoint accepts arbitrary invalid tokens and immediately performs SQLite secret lookup plus HMAC/base64 processing for each request. Unlike /login, which applies the per-IP/account buckets before touching storage, failed exchanges are never throttled; an attacker can repeatedly post a small syntactically valid token (for example, one claiming unsloth) and block the async server event loop on database work. Apply a bounded per-IP failure limit (or equivalent shared limiter) before calling exchange_link_token_with_secret.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6e44308. /api/auth/link-exchange now applies the same per-IP failure limiter as /login (via _unknown_user_key, since there is no username here), checked BEFORE exchange_link_token_with_secret does any SQLite/HMAC work, and cleared on a successful exchange. Regression: test_link_exchange_rate_limits_repeated_failures (a valid token is 429'd while blocked, proving the gate runs before the exchange) and test_link_exchange_success_clears_rate_limit_bucket.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f56ccb346
ℹ️ 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".
| for candidate in (sys.stderr, sys.stdout): | ||
| try: | ||
| if candidate is None or getattr(candidate, "closed", False): | ||
| continue | ||
| if not callable(getattr(candidate, "write", None)): | ||
| continue | ||
| except (AttributeError, ValueError): | ||
| continue | ||
| return candidate |
There was a problem hiding this comment.
On a headless launch where stderr/stdout is a writable pipe or file (for example systemd, CI, or nohup), this helper returns that stream even though it is not an interactive console. The subsequent auto-password path writes the only admin credential there, retaining it in service/CI/nohup logs despite the no-persistence guarantee and exposing it to log consumers. Require a TTY or an explicitly configured secure credential sink before auto-generating in this case.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 6e44308. The CLI mirror _one_time_secret_console_stream had the same gap: a writable non-tty stream (systemd, CI, nohup) passed the closed/writable preflight, so the auto-generated password was written into that retained stream. It now requires a real TTY and returns None otherwise, so the CLI fails closed and preserves the seeded bootstrap credential rather than leak it. Regression: test_one_time_secret_console_stream_requires_tty.
…mit link-token exchange - run.py / CLI: the auto-generated admin password is surfaced only on a real terminal. A writable but non-tty console (nohup.out, a systemd-journald pipe, a `> log 2>&1` redirect) would persist the plaintext to a file/journal that log consumers can read (CWE-532), breaking the "shown once, not written to disk" guarantee, so _one_time_secret_stream / _one_time_secret_console_stream now skip non-tty streams and the gate fails closed WITHOUT rotating the seeded recovery credential. A headless launch supplies --password / UNSLOTH_STUDIO_PASSWORD / stdin instead. - routes/auth.py: rate-limit /api/auth/link-exchange per IP like /login, checked BEFORE exchange_link_token_with_secret touches storage, and cleared on a successful exchange. - Regression tests: tty requirement (backend + CLI) and the exchange rate limit.
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. 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". |
1 participant
Summary
Two changes to Unsloth Studio's public-launch auth path.
Part A (core): auto-generate + print an admin password for headless public launches
Today a public launch (
--secure,--cloudflareproducing a public tunnel, or Colabstart(cloudflare=True)) that supplies no password refuses to tunnel while the seededmust_change_passwordadmin password is unchanged:run.pyforces an interactive masked change andcolab.py_bootstrap_password_pending()blocks the tunnel.When such a launch has no supplied password (no
--password/UNSLOTH_STUDIO_PASSWORD/ stdin) and the admin still has the default must-change password, it now:secrets.token_urlsafe(24)password,update_passwordpath, which clearsmust_change_password, rotates the JWT secret, revokes refresh tokens, and deletes the on-disk.bootstrap_password,The credential is never persisted to disk and never placed on argv. Because
must_change_passwordbecomes0, therun.pyterminal password gate returns without prompting and the Colab pending check passes, so the tunnel proceeds headlessly instead of failing closed or relying on the bootstrap shutdown deadline. Theunsloth_clistudio parent gate mirrors the same behavior before re-exec so the two stay in sync. A real interactive terminal still gets the masked prompt, and a supplied--passwordis respected and never overwritten. On--securewith a provably unavailable tunnel the launch still refuses early (no point rotating a recovery credential for a launch that cannot start).Part B (opt-in, default off): one-time short-TTL link token for the Colab same-tab URL
A one-time, ~10 minute HMAC-signed link token for the Colab same-tab proxy URL only. It is signed with a key derived from the admin's JWT secret (domain-separated so a link token is never accepted as a bearer access token), single-use via a
link_tokensnonce table, constant-time verified, expiry-enforced, and bound to the admin identity. It is never logged.POST /api/auth/link-exchangeconsumes a valid unexpired unused token exactly once and returns the same JWT + refresh pair the login path issues.?link_token=...to the same-tab URL only (never the shared Cloudflare link) when enabled viastart(link_token=True)orUNSLOTH_STUDIO_COLAB_LINK_TOKEN=1.The frontend consume +
history.replaceStatescrub is left as an inline TODO rather than shipping a half-wired UI, so today the token is emitted but harmless and the same-tab login page still works normally.Tests
studio/backend/tests/test_link_token.py: mint/exchange round-trip, single-use rejection, expiry, tampered signature, tampered payload, unknown subject, malformed input, and the/link-exchangeroute (issue once, reject replay).studio/backend/tests/test_colab_secure_autopassword.py: auto-generate sets + clears must-change, no-op when a password is already set,start_cloudflare_tunnelauto-generates then proceeds (and fails safe if generation fails), and the opt-in same-tab link-token wiring.studio/backend/tests/test_password_prompt_backstop.pyandunsloth_cli/tests/test_studio_password_prompt.pyto the new headless auto-generate contract.