Repository navigation
feat(auth): opt-in unattended sign-in with a stored authenticator - #76
Conversation
MAHE's Entra sign-in frequency ends the session about every 5 days and always asks for a second factor again. With an authenticator-app secret stored by `auth totp-setup`, the CLI completes the PhoneAppOTP step itself, so an agent keeps working without a person re-entering the password and MFA. - `auth totp-setup` / `auth totp-remove`: seal or delete the secret (hidden prompt or stdin, never argv), bound to the saved username. - `auth login` uses the stored authenticator when no `--totp` is given and `--mfa-method` is unset or `app`; the code is generated after BeginAuth, submitted once and never shown. - Replay-safe reads on the built-in connection sign in once and retry once when the session is expired or the cookies are missing. An expired session can answer 403, so a 403 counts as expiry only when whoami is signed out too. Writes, quiz flows and read-only clients never sign in. - `session_renewal`: cross-process `renewal_lock`, strictly advancing time steps, and the `relogin_state.json` breaker that stops repeated failed sign-ins. - `auth status` checks whoami and reports `renewed` and `unattended_login`. Docs: docs/auth-unattended.md (setup, failure categories, trade-off).
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — full initial review of all 35 files (one commit), read end-to-end against the diff, with focused specialist passes on the MFA method semantics, the locking/breaker design, and P0 secret hygiene. All three came back clean.
totp.py+credential_store.py: RFC 6238 primitives (stdlib-only, RFC 6238 Appendix B vectors pinned) and a sealedauthenticator.jsonholding the secret, its bound username, and the last used time step; strict load validation with fixed-text errors,repr=Falseon every secret-bearing field, symlink-protected paths.session_renewal.py:renewal_lock(thread lock +flock, one 120 s monotonic deadline, non-re-entrant with a regression test), the strictly-advancing time-step reservation persisted before code generation, waiter reuse of another caller's saved cookies, and the plaintext no-secrets circuit breaker inrelogin_state.json(corrupt state fails closed across a 13-case matrix, auth-class failures block,networkgets a 5-minute cooldown).ms_auth.pyprovider mode: code generated only after a successfulBeginAuth, exactly oneEndAuthsubmission (single_submissionraisescode_rejectedon any non-success before theAuthenticationPreviouslyCompletedhandler can read a foreign checkpoint), human checkpoint writes/reads fully disabled (byte-identical checkpoint survival pinned), banner suppressed, 5xx classified asnetworkbefore page parsing.api.pyrecovery: replay-safe GET/HEAD on the built-in connection recover once — stored authenticator first, CDP fallback second; missing cookies recover only via the stored authenticator; the 403-whoami disambiguation (expired session vs genuine denial) is single-attempt with a 15 s timeout and ~20 dedicated tests covering permission-vs-expired, double-sign-in prevention, closed folders, quiz checks, and read-only clients.auth.py/cli.py/commands.py:totp-setup/totp-remove(secret via hidden prompt or stdin, never argv;GetPassWarningbecomes an error before anything is echoed), theuse_stored_authenticatorgate (any--totp,--totp -, or explicit non-appmethod keeps the person-driven flow — pinned per method), the login tail now save→probe→store under the lock, andauth statuson whoami withrenewed/unattended_loginJSON fields.- Docs and tests: new
docs/auth-unattended.md(setup, category table, clock, trade-off, revocation), README/AGENTS/ADR/runbook updates;.secrets.baselinechanges are line-number moves only; new provider/multiprocess (real spawn +flockcontention)/sentinel suites plus rewritten mock seams in the extended suites. Deleted test lines are mock-seam rewrites paired with equivalent-or-stronger replacements, not dropped coverage.
Verification: ruff, mypy, lint-imports, and deptry all pass; pytest is 2655/2658 green — the 3 failures are pre-existing tests/test_quiz_media.py breakage that reproduces on main in this environment and is untouched by this PR. The PR's own L1–L5 live checks against MAHE are listed as pending in the description; everything verifiable locally checks out.
glm-5.3 ｜ 𝕏
CI's gitleaks job flagged the OTHER_SECRET base32 fixture in tests/test_auth_totp.py as a generic API key. It is a test-only RFC 6238 style key, already allowlisted for detect-secrets; an inline `gitleaks:allow` now covers gitleaks too.
GPT-6.1 Sol found four ways the breaker could let automatic sign-in run again after a failure: - A failure that could not be recorded (unwritable config directory) was repeated on every read. Automatic sign-in now writes the breaker state before Microsoft sees the password, and stops with `store` if it can't. - A clock that went back ended the 5-minute `network` cooldown at once. A negative elapsed time now keeps it. - A state file whose category was a non-string (a list or object) raised instead of failing closed. - A `network` state missing its timestamps, or a clear state carrying them, was read as valid. Both now fail closed as `unknown`. CodeRabbit found that after a fast clock reserved a future time step, neither fix in the docs worked: `auth login` kept failing `clock_skew`, and `totp-setup` with the same key kept the future step. Setup now keeps a recorded step only up to two steps ahead of the clock, and the docs name `totp-setup` as the recovery for `code_rejected` and `clock_skew`. GLM 5.3 found that the docs said an unreadable state file is `unknown`; only corrupt content is, and a read failure is `store`.
… the chooser Grok 4.7 found that `auth login --help` still said a plain interactive login shows Microsoft's verification methods. With a matching stored authenticator, login uses it instead, with no chooser and no code prompt. The help for the command and for --mfa-method now says so.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| if type(doc["v"]) is not int or doc["v"] != _STATE_VERSION or type(blocked) is not bool: | ||
| return _CORRUPT_STATE | ||
| allowed = BLOCKING_CATEGORIES if blocked else {None, "network"} | ||
| if category not in allowed: |
There was a problem hiding this comment.
[WARNING]: Corrupt state with a non-string category raises TypeError instead of failing closed
category comes from unvalidated JSON. If a corrupt or hand-edited relogin_state.json holds "category": [] or {}, the category not in allowed membership test raises TypeError: unhashable type. _load_state calls _state_from_document outside its try (which only guards JSON ValueError), and renew_session calls _load_state before its own try, so the error bypasses the breaker's fail-closed mapping. unattended_status() catches only RenewalError, so lighthouse auth status dies with a raw traceback - violating both the documented "corrupt state fails closed" contract and the no-raw-traceback convention. Guard with isinstance(category, str) before the membership test.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| error = _as_renewal_error(exc) | ||
| _record_failure(store, state, error.category) | ||
| raise error from None | ||
| if result.signed_in: |
There was a problem hiding this comment.
[WARNING]: A failed post-success clear_breaker() reports a completed sign-in as a failure
By this point _attempt has signed in, saved and validated the new cookies. If clear_breaker() raises RenewalError("store") (non-FileNotFoundError OSError on unlink, or a path-validation failure), callers convert the fully successful renewal into automatic sign-in failed: store / an auth error, while the breaker file is left in place. The same pattern exists in store_authenticator/remove_authenticator (breaker cleared after a successful save/delete). Consider making the clear best-effort (contextlib.suppress(OSError)) or reporting success with a cleanup warning.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| the meantime are returned instead of being overwritten. | ||
| """ | ||
| try: | ||
| with renewal_lock(): |
There was a problem hiding this comment.
[WARNING]: First cookie access of any command can block on the cross-process sign-in lock, and lock contention is reported as session expiry
_load_migrating_legacy_cookies takes renewal_lock() (120 s deadline) and runs on the first .cookies access of any command - including non-recoverable POSTs, since this happens before can_recover matters. While another process renews, ordinary commands can block for up to 2 minutes and then get SessionExpiredError("Session expired (cookie upgrade busy)"): a lock-contention failure surfaced as session expiry, a new failure class for POST paths that previously never raised it. Consider a shorter deadline for the migration read and a non-"expired" error for the busy case.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| json_output, | ||
| ) | ||
| try: | ||
| with renewal_lock(): |
There was a problem hiding this comment.
[WARNING]: The global sign-in lock is held across a network probe
renewal_lock() now spans probe_cookies(cookies) - a whoami GET with a 30 s timeout and up to 4 retryable attempts - plus the credential save. A slow D2L endpoint can pin the lock for minutes, so every concurrent sign-in path (auth login, totp-setup/totp-remove, automatic renewal, legacy-cookie migration, import-session) waits out the 120 s deadline and fails "busy". The atomicity goal (no automatic sign-in publishing cookies in between) is valid, but consider probing within the shorter _SIGNED_OUT_CHECK_TIMEOUT budget, or releasing the lock before the probe and re-checking only if cookies.json changed.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| raise exc.error from None | ||
| except Exception as exc: # every failure maps to a category | ||
| error = _as_renewal_error(exc) | ||
| _record_failure(store, state, error.category) |
There was a problem hiding this comment.
[SUGGESTION]: _record_failure failure can mask the original failure category
If _write_state fails (read-only dir, full disk), _record_failure raises RenewalError("store"), which replaces the original error before raise error runs - a credentials_rejected failure would then be reported as store, sending callers down the wrong recovery path. Wrapping this call in contextlib.suppress(RenewalError) preserves the real category.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| turn echo off; that warning is raised as an error before anything is read. | ||
| """ | ||
| if not _is_interactive(): | ||
| return sys.stdin.readline(_MAX_SECRET_LINE) |
There was a problem hiding this comment.
[SUGGESTION]: stdin read cap (4096) diverges from the secret validator's limit (2048)
totp.py rejects input over _MAX_INPUT_CHARS = 2048, but this non-interactive read truncates at 4096 and leaves the unread remainder in the pipe for the next reader. Matching 2048 (or draining stdin) would make the rejection deterministic and the fixed error immediate.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| "valid": True, | ||
| "cookies": list(cookies.keys()), | ||
| "renewed": client.renewed_session, | ||
| "unattended_login": unattended_status(), |
There was a problem hiding this comment.
[SUGGESTION]: Two unattended_status() snapshots can disagree
unattended is snapshotted at line 1041 (used for the no-cookies gate) and recomputed here and in _status_payload (line 1088). The breaker can flip between snapshots (e.g. during the whoami call), so the reported unattended_login may not match the state that gated the earlier branch. Threading a single snapshot through the payload would be consistent.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| ``` | ||
|
|
||
| `renewed` is `true` when this command signed in again. On failure the document | ||
| has `"valid": false`, an `error`, and, if automatic sign-in failed, its |
There was a problem hiding this comment.
[SUGGESTION]: category is documented as conditional but is always present
The implementation always includes category in failure documents, with null when no automatic sign-in was attempted (_status_payload; tests assert "category": None). A consumer written strictly from this wording could mishandle the always-present null. Suggested wording: "and a category (null when no automatic sign-in was attempted)".
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| ) | ||
| except RenewalError as exc: | ||
| outcome = f"blocked:{exc.category}" if failure_category is None else f"error:{exc.category}" | ||
| except BaseException: |
There was a problem hiding this comment.
[SUGGESTION]: Bare except BaseException discards the exception, and outcome labels come from test intent
The worker labels its outcome from failure_category is None rather than the observed result, and the bare except drops the exception entirely - a regression that mislabels a gate refusal as a sign-in failure (or vice versa) would still report error:<category>. The parent assertions do cover the tested paths, so this is diagnosability rather than a false pass, but embedding the exception class name in the reported outcome would make failures trustworthy.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| def time_step(for_time: float, period: int = TOTP_PERIOD) -> int: | ||
| """Return the RFC 6238 time-step counter for a Unix timestamp.""" | ||
| return int(for_time // period) |
There was a problem hiding this comment.
[SUGGESTION]: Out-of-domain inputs surface raw stdlib exceptions
period=0 raises ZeroDivisionError here; NaN/inf timestamps raise raw ValueError/OverflowError downstream, and a counter at or above 2**64 raises struct.error. All current callers pass time.time() and the validated period (30), so this is unreachable in practice - noted because the module's stated contract is that failures carry fixed, non-echoing messages.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 27 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (16 files)
Incremental note: reviewed commits Fix these issues in Kilo Cloud Previous Review Summaries (4 snapshots, latest commit 6256fe0)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 6256fe0)Status: 24 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (12 files)
Incremental note: reviewed commits Fix these issues in Kilo Cloud Previous review (commit 56673b2)Status: 23 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (14 files)
Incremental note: reviewed commits Fix these issues in Kilo Cloud Previous review (commit 783702e)Status: 24 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (14 files)
Incremental note: the previous corrupt-state Fix these issues in Kilo Cloud Previous review (commit 55e806b)Status: 24 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
[Snapshot truncated.] Additional previous summary content was truncated to keep this comment within platform limits. Reviewed by glm-5.3-flash · Input: 89.4K · Output: 28K · Cached: 1.2M Review guidance: REVIEW.md from base branch |
|
Droid finished @rabesss's task —— View job The renewal gate protects individual write requests, but write commands can still sign in unattended during prerequisite reads. Carry the write-command boundary through those reads so an expired session cannot trigger automatic sign-in before an upload or assessment write. 1 inline comment posted |
![factory-droid[bot]](https://avatars.githubusercontent.com/in/358017?s=60&v=4){kind=small}
| # when its response reports an expired session. Never refresh and | ||
| # send that body again. The read-only auth mode also forbids | ||
| # sign-in, browser refresh and cookie persistence for GET/HEAD. | ||
| can_recover = retryable and not self._read_only_auth |
There was a problem hiding this comment.
[P1] Disable unattended sign-in during write-command preflight reads
The recovery gate checks only this individual request, so a write command can still sign in automatically on its prerequisite GET and then perform the write. For example, AssessmentAPI.write() fetches /d2l/home via get_csrf_token() before its POST, and cmd_submit() uses a normal client to resolve its course and folder before uploading. With expired or missing cookies, those GETs renew the session and the subsequent write proceeds, contrary to the documented rule that uploads and other writes never sign in automatically. Carry the no-renewal policy through the entire write workflow, not just its POST.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — re-review of the four commits pushed since the initial review (253a88e, b61af23, 0962d8b, 783702e), which land the round-1 bot feedback: fail-closed breaker handling, fast-clock recovery, help text, and the gitleaks fixture markers.
- Hardened the breaker parser (
_state_from_document): non-string categories are now rejected by a type guard that runs before thecategory not in allowedmembership test (so an unhashable[]/{}in a hand-corrupted file returns_CORRUPT_STATEinstead of raisingTypeError), andsince/last_attemptmust pair withcategory— a recorded failure has both timestamps, a clear state neither. That invariant matches every writer:_record_failurealways writes both, and the new pre-write rewrites a validated state as-is. - Made automatic sign-in prove the breaker writable before the password is sent:
_attemptnow calls_write_state(store, state)after_usable_pairand before_sign_in, so an unwritable config dir fails fast with unrecordedstoreand zero Microsoft logins — pinned bytest_automatic_sign_in_needs_a_writable_breaker_firstassertingsso.logins == []. The rewrite preserves cooldown timestamps, the success path still clears, and a pre-write failure stays unrecorded (store∈_UNRECORDED_CATEGORIES), so there is no nested-write failure path. - Added fast-clock recovery to
auth totp-setup:store_authenticatorkeeps a previously recorded step only when it is at most_MAX_FUTURE_STEPS(2) ahead of the clock; anything further came from a clock running fast and is dropped, so re-running setup with the same secret recovers fromclock_skew. Both sides of the boundary are tested, and the doc's "up to 90 seconds more" matches the provider'swait <= period + 1sleep-window arithmetic. - Held the network cooldown across a backwards clock: the
0 <=lower bound is gone, so a clock that jumps back keeps the 300 s cooldown instead of re-arming failed attempts on every read; the gate error remains unrecorded, so it cannot extend itself. Covered bytest_network_cooldown_holds_when_the_clock_goes_back. - Documented the stored authenticator in
auth login --helpand the command docstring, matching theuse_stored_authenticatorsemantics exactly (no--totpand method unset orapp, username match); pinned by a help-text test. - Silenced the gitleaks fixture findings two ways: the inline
gitleaks:allowonOTHER_SECRETcovers findings anchored to future commits, and the commit-pinned.gitleaksignoreentry covers the finding anchored to the already-pushed first commit (an inline marker added later cannot retroactively cover it). This repo squash-merges, so post-merge the pinned commit never lands onmainand the squash commit re-adds the line with the inline marker — the scan stays green either way, as the greenlint / types / architecture / depsjob on this head confirms.
Verification: the three touched suites pass locally (108 tests: test_session_renewal.py, test_auth_totp.py, test_secret_absence.py, including the seven new corrupt-state matrix cases and the manual-login-clears-corrupt-state path), ruff check . and strict mypy are clean, .secrets.baseline is line-number moves plus a generated_at refresh only, and CI — including the full-history gitleaks scan — is green on 783702e.
｜ View workflow run ｜ Using glm-5.3 ｜ 𝕏
glm-5.3 ｜ 𝕏
| if state.category == "network" and state.last_attempt: | ||
| # A clock that went back keeps the cooldown until it catches up. | ||
| elapsed = (_utc_now() - _parse_utc(state.last_attempt)).total_seconds() | ||
| if elapsed < NETWORK_COOLDOWN_SECONDS: |
There was a problem hiding this comment.
[SUGGESTION]: The new clock-rollback hold on the network cooldown is not reflected in the user-facing docs
Dropping the 0 <= bound makes a clock set backwards keep the network cooldown until the clock catches up — potentially days (intentional and covered by test_network_cooldown_holds_when_the_clock_goes_back). But the network row of the troubleshooting table in docs/auth-unattended.md still advises only "Retry later", which never succeeds after a large rollback, and the updated clock paragraph there covers only code_rejected and clock_skew. A user following the docs stays stuck in a network loop with no working advice; the docs should mention the rollback hold and that auth totp-setup/auth totp-remove (or a manual auth login) clears it.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
GPT-6.1 Sol found two ways the round-1 fixes let a sign-in repeat: - `totp-setup` with the same key dropped a recorded step more than two steps ahead of the clock. After a successful sign-in at step S and a clock that went back three steps, setup saved S-3, and once the clock was corrected the next login sent the code for S again. Grok 4.7, GLM 5.3 and DeepSeek V4.1 Flash found the same replay. Setup with the same key again keeps the newer step. The recovery from a fast clock is now `totp-remove`, then `totp-setup`, once the clock is right; the docs and the `clock_skew` login message (Grok 4.7) say so. - The breaker write before contacting Microsoft only proved the file was writable. A process killed while Microsoft had the password left the state clear, so the next read sent it again. Automatic sign-in now records itself as blocked (`unknown`) first, and the outcome replaces that: success clears it, a failure records its category, and a `store` failure restores the previous state. DeepSeek V4.1 Flash found the related case where recording the failure itself fails; the in-flight block now stays. CodeRabbit found that the docs' fix for `unknown` (`auth login --mfa-method choose`) never clears the block; it is now `auth login`. DeepSeek V4.1 Flash found that a fast clock can stretch the `network` cooldown; the docs now name `auth login` for that.
…n help Factory Droid found that the docs listed `submit` among the things that never sign in automatically, but the reads a write command makes first (course and folder lookup, the CSRF fetch) are ordinary reads and can. That is the reviewed design: the rule is per request, and a write is never sent twice. The docs now say so. CodeRabbit found that `auth login --help` left out that the stored authenticator is used only for the username it is bound to.
GPT-6.1 Sol found that `totp-setup` treated an authenticator file it could not read as absent. After a sign-in at step S and a clock that went back three steps, a corrupted or foreign-key `authenticator.json` was replaced with S-3, and once the clock was corrected the next login sent the code for S again. Setup now refuses to replace a file it cannot read and names `totp-remove`, then `totp-setup`. DeepSeek V4.1 Flash found that setup kept a newer step only when the saved username was unchanged. Re-storing the same key under an alias dropped it the same way. The step now follows the key, whatever the username.
…lock GLM 5.3, Grok 4.7, Muse Spark 1.3 and DeepSeek V4.1 Flash found that an `unknown` stored-login failure still said to run `auth login --mfa-method choose`. That login gives a session but leaves automatic sign-in blocked; round 2 fixed the same advice in the docs only. The message now says to run `auth login` again, and that the chooser leaves automatic sign-in blocked. GLM 5.3 found that the docs called `store` failures unrecorded, but one during an automatic sign-in can leave the in-flight `unknown` block in place. The docs now say so.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — re-review of the two commits pushed since the last review (34bf5ca, 56673b2), which close out the round-2 bot feedback: the used-step replay window and the breaker's in-flight visibility, plus the matching docs/help.
- Blocked the breaker while an automatic sign-in runs:
_attemptnow persistsblocked/unknownbefore the password is sent, instead of rewriting the prior state. A process killed mid-sign-in (KeyboardInterrupt bypasses the failure handler by design) leaves the block behind; a recorded failure overwrites it with its real category; an unrecordedstorefailure restores the previous state and stays blocked if that restore fails; success clears it. Every one of those paths is pinned by a new test, including the interrupted-then-refused sequence (sso.logins == 1) and the failure-that-cannot-be-recorded case. - Removed the fast-clock step drop from
totp-setup: re-storing the same key now keeps the newer recorded step unconditionally. Round 2's_MAX_FUTURE_STEPSwindow could forget a step whose code Microsoft had already accepted and resend it — a genuine replay vector — so recovery fromclock_skewis nowtotp-remove+ re-setup. The clock-rollback replay case is pinned bytest_setup_after_a_clock_rollback_never_replays_an_accepted_step, and the same-key retention is pinned at both the old boundary (+2) and an extreme (+100). - Aligned docs and user messages with the new semantics: the category table now counts an interrupted automatic sign-in as
unknown, the clock section describes the never-resend rule and the remove+setup recovery, the fixedclock_skewmessage names that recovery, and thenetworkrow notes a fast clock stretches the cooldown. The write-request scope ("a write is never sent twice; a write command's earlier reads are ordinary reads") now matchescan_recover's per-request predicate exactly. - Qualified the stored-authenticator help:
auth login --helpand the docstring now say the stored authenticator applies "for that username", matchinguse_stored_authenticator's username-match gate; pinned by the help-text test.
Verification: 277 tests green across the touched suites (test_session_renewal, test_auth_totp, test_cli_contracts, test_secret_absence, test_session_renewal_multiprocess), ruff check . clean, and strict mypy clean with the pinned toolchain — the one complaint from an unpinned mypy/types-requests run landed on pre-existing ms_auth.py code outside this delta and vanishes under requirements-dev.txt versions. The breaker state machine (reuse → gates → load → in-flight block → sign-in → record/restore/clear) was traced end-to-end against the code on disk.
｜ View workflow run ｜ Using glm-5.3 ｜ 𝕏
glm-5.3 ｜ 𝕏
| } | ||
| # Setup cannot keep the used steps of an authenticator it cannot read. | ||
| _UNREADABLE_AUTHENTICATOR = ( | ||
| "The stored authenticator could not be read. Check the system clock, " |
There was a problem hiding this comment.
[SUGGESTION]: "Check the system clock" names only one of the ways an authenticator becomes unreadable
Nothing in the read path consults the clock (the failure comes from the file content, the envelope, or the key source), so for the common causes - a corrupt/truncated file or a foreign key after a changed passphrase/keyring - this first step is a dead end. The clock matters only when the user is re-storing after a fast-clock clock_skew (setup would then reserve an ahead-of-clock step again). Consider naming the other cause too, e.g. "Check the system clock and the encryption key source, then run: ...", matching the store row's "check the key source" advice in docs/auth-unattended.md.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — re-review of the two commits pushed since the last review (ba2abbd, 6256fe0), which close out the round-3 feedback: the unreadable-authenticator replay window, cross-username step retention, and the unknown recovery message.
- Made
totp-setuprefuse to replace an authenticator it cannot read:store_authenticatornow raisesCredentialStoreError(_UNREADABLE_AUTHENTICATOR) from Nonewhenload_totp()fails, instead of treating the previous binding as absent — an unreadable file's used steps are unknown, and overwriting it could resend a code Microsoft already accepted. The raise surfaces through_clean_auth_command'sCredentialStoreErrorhandler into the exact-match allowlist, so the fixed message renders verbatim with exit 1 and the underlying error is discarded. Pinned bytest_setup_never_replaces_an_authenticator_it_cannot_read, which also checks the file is left untouched and thattotp-remove→ setup recovers. - Extended same-key step retention across usernames: the
previous.username == usernamecondition is gone, so a step recorded under any username is kept when the same key is re-stored. Correct — a TOTP code depends only on the key and the time step, so a step burned under another username must never be resent either. Both the old boundary (+2) and an extreme (+100) are now pinned perbound_tousername. - Pointed the
unknownfailure at the login that clears the block: the message now leads with plainauth login(never breaker-gated, uses the stored path), offers--mfa-method chooseas the fallback, and states that automatic sign-in stays blocked until a stored login succeeds — which matches the breaker semantics exactly (chooser success signs in but does not clear the block). Thestoredocs row now notes an automatic-sign-instorefailure can leave the block asunknown, and thetotp-setupdocs row names the refuse-then-totp-removebehavior.
Verification: 432 tests green across the touched suites (test_auth_totp, test_auth_login, test_session_renewal, test_cli_contracts, test_secret_absence), ruff check . clean, and mypy shows only the known environmental playwright.sync_api import-not-found on this runner. The raise path, the retention predicate, and both new user-facing messages were traced end-to-end against the code on disk.
glm-5.3 ｜ 𝕏
| missing_cookie_names, | ||
| save_cookies, | ||
| ) | ||
| from .session_renewal import ( |
There was a problem hiding this comment.
[P1] Preserve ordinary API access on non-POSIX platforms
This unconditional import loads session_renewal.py, which imports the Unix-only fcntl module. On Windows, previously independent commands such as lighthouse courses --json, grades, downloads and authentication now fail during import with ModuleNotFoundError, even when unattended sign-in is not configured; the JSON command also produces no JSON output. Make the locking dependency platform-aware and keep unsupported renewal functionality from preventing ordinary cookie-authenticated access.
| "credentials_rejected", "code_rejected", "method_unavailable", "clock_skew", | ||
| "interrupt", "network", "store", "busy", "pending_human_login", "unknown", | ||
| }) | ||
| _RENEWAL_FAILURE_RE = re.compile(r"\(automatic sign-in failed: ([a-z_]{1,32})\)") |
There was a problem hiding this comment.
[P2] Preserve renewal categories when formatting an error twice
This regex recognizes the original exception text but not the formatter's own output, so formatting a sanitized renewal error again removes its category. For example, download 123 --assignment 7 --json sanitizes a failed folder fetch in _assignment_selector_snapshot, then sanitizes it again through _safe_error_entries; a code_rejected failure consequently becomes only Session expired. Run: lighthouse auth login in both diagnostics and JSON. Recognize the emitted category format too so existing rendering paths retain the failure reason.
Factory Droid found that `api.py` now imports `session_renewal`, which imported `fcntl` at module level. On a system without it, such as Windows, ordinary commands like `courses --json` died with a raw `ModuleNotFoundError`; on main they ran. The login cookie save, `import-session`, the CDP refresh save and the legacy cookie migration also take the sign-in lock, whose lock file needs POSIX-only open flags. `fcntl` is now optional. Without it the lock covers only threads, and stored-authenticator sign-in is off: `totp-setup` refuses before reading the secret, `auth login` keeps the person-driven flow, automatic sign-in is never configured, and `renew_session` refuses before doing anything. The docs' setup section says so.
Factory Droid found that `format_user_error` applied to its own output dropped the automatic sign-in category, because it matched only the raw "(automatic sign-in failed: X)" form. `download --assignment --json` formats twice, so a `code_rejected` failure became a plain "Session expired". The pattern now also matches the formatter's own form; the category is still checked against the known set. The secrets baseline refresh only moves line numbers in tests/test_cli_contracts.py.
| _WITHOUT_FCNTL = """ | ||
| import sys | ||
| sys.modules["fcntl"] = None # importing it fails, as on Windows | ||
| from lighthouse_cli import api, auth, cli, commands, session_import, session_renewal |
There was a problem hiding this comment.
[SUGGESTION]: The import list omits the two modules that still hard-import fcntl
quiz_preview_session.py and quiz_learner_session.py still import fcntl unconditionally (loaded lazily via assessment_commands for the instructor/student quiz subcommands) and are excluded from this snippet. On a non-POSIX system those subcommands remain the only CLI entry points that die with a raw ImportError traceback, while this PR establishes clean degradation (_UNATTENDED_UNSUPPORTED, the unattended_supported() gates) everywhere else, and docs/auth-unattended.md now says "the other commands work as before". The test name test_modules_import_without_fcntl also overclaims: a reader can assume whole-package coverage. Consider documenting the exclusion in the snippet (or naming the test after the listed modules), and/or giving the lazy quiz imports the same clean non-POSIX error treatment.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| RenewalError: Fixed-text failure with a ``category``. | ||
| """ | ||
| if not unattended_supported(): | ||
| raise RenewalUnavailableError() |
There was a problem hiding this comment.
[SUGGESTION]: The unsupported-platform refusal reuses advice that dead-ends on that platform
RenewalUnavailableError's fixed message is "No stored authenticator matches the saved credentials. Run: lighthouse auth totp-setup" - but on the non-POSIX system that triggers this branch, auth totp-setup refuses with "Unattended sign-in needs a POSIX system..." (_UNATTENDED_UNSUPPORTED), so the recovery hint points at a command that cannot succeed there. Unreachable through the CLI today (both call sites gate on unattended_configured() / _stored_authenticator_matches first), but as defense-in-depth for future direct callers consider a distinct message naming the platform restriction.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| @contextmanager | ||
| def _process_lock(deadline: float) -> Iterator[None]: | ||
| if not _HAS_FLOCK: | ||
| # Stored-authenticator sign-in is off here, so only this process's |
There was a problem hiding this comment.
[SUGGESTION]: The no-flock degradation also drops cross-process serialization for cookie migration and CDP saves
Besides stored-authenticator sign-in, renewal_lock() also serializes the legacy-cookie migration (api._load_migrating_legacy_cookies) and the CDP cookie write (api._refresh_from_browser). On a non-POSIX system this branch reduces those to thread-only locking, so two concurrent CLI processes can interleave the migration read-reread-write (last writer wins on cookies.json) - a state impossible before this change because the module could not import there at all. The comment here and docs/auth-unattended.md mention only sign-in serialization; worth noting the cookie-write scope in one of them.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — re-review of the two commits pushed since the last review (e097ba1, 4db986f), which close out the round-4 Factory Droid findings: fcntl no longer breaks ordinary commands on non-POSIX systems, and a twice-formatted error keeps its sign-in category.
- Made
fcntloptional — the import is nowtry/except ImportErrorbehind_HAS_FLOCK, and a newunattended_supported()predicate gates every stored-authenticator entry point:totp-setuprefuses up front with a fixed POSIX message (allowlisted in_SAFE_FIXED_AUTH_MESSAGES),auth loginkeeps the person-driven flow,renew_sessionraisesRenewalUnavailableErrorbefore touching the lock, andunattended_configured()folds in the platform check soauth statusandapi.py's recovery path report and use it consistently — an unsupported platform falls through to the CDP browser refresh, exactly as onmain.renewal_lockdegrades to thread-only serialization via the new_process_lockno-op without creatingrelogin.lock, so the human login tail,import-session, and the CDP save still share the lock safely. - Kept the sign-in category through a second format —
_RENEWAL_FAILURE_REnow matches both the raw"(automatic sign-in failed: X)"shape and the formatter's own"automatic sign-in failed (X)"output, soformat_user_erroris idempotent; pinned per category across all tenRELOGIN_CATEGORIES, with the unknown-category and missing-paren cases still ignored. - Pinned the platform behavior with tests — a subprocess test injects a missing
fcntl(sys.modules["fcntl"] = None) and assertsapi/auth/cli/commands/session_import/session_renewalstill import andunattended_supported()isFalse; no-flock tests cover the person-driven login, setup's refuse-before-reading, thread-only lock serialization (norelogin.lockcreated), and stored sign-in staying off.AGENTS.mdanddocs/auth-unattended.mdstate the POSIX requirement.
Verification: 436 tests green across the four touched suites (test_session_renewal, test_cli_contracts, test_auth_login, test_auth_totp) in a clean venv install — the single failure seen under this runner's user-site pip install -e is a python -I path artifact that a proper install, as CI uses, does not hit — plus ruff check and strict mypy clean. All prior pullfrog reviews had zero open threads, so nothing was carried forward.
glm-5.3 ｜ 𝕏
Live test L4 on MAHE: of two concurrent reads with an expired session, the one that waited on the sign-in lock and reused the other's cookies also printed "Signing in again with the stored authenticator...". renew_session takes an on_sign_in callback and calls it once, right before it contacts Microsoft: after the cookie reuse check, the breaker and cooldown gates, the username match and the in-flight breaker write. api.py prints the notice from that callback, so a process that reuses cookies, is refused by a gate or has no usable authenticator stays quiet. A failing notice (closed stderr) is ignored instead of being recorded as a failed sign-in. Plan review (gpt-6.1-sol): added the breaker-write and interrupted- notice boundary tests and a real blocking category for the api test.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — re-review of the single commit pushed since the last review (0ee2e16), which closes the L4 live-test note: of two concurrent reads with an expired session, only the process that actually signs in now says so.
- Moved the sign-in notice behind an
on_sign_incallback —_recover_sessionno longer prints "Signing in again with the stored authenticator..." unconditionally; it passes_announce_sign_in, andrenew_sessioninvokes the callback at exactly one point: after the cookie-reuse check, the breaker/cooldown/pending gates, the username match, and the in-flight breaker write, and immediately before Microsoft is contacted. A waiter that reuses another process's cookies, a gate refusal, and an unmatched authenticator all stay quiet now. - Kept the notice non-fatal in both directions — a notice that fails (closed stderr) is suppressed so it can never count as a failed sign-in, while a
KeyboardInterruptfrom the notice still propagates and leaves the breaker blocked asunknownwith zero Microsoft logins, matching the established interrupt semantics. - Preserved the manual path's single banner —
auth login's stored-authenticator route does not pass the callback; it keeps its own "Using the stored authenticator for verification." message, so there is no double notice. - Pinned the contract with tests — seven new unit tests (notice-before-login ordering, silence on reuse / each gate / no-usable-pair, failing notice, interrupted notice), the api-transport fixture now emulates the callback, the 403-failure test uses a real blocking category, and the real-spawn multiprocess counters assert
["notice", "login"]— the reusing or blocked worker contributes nothing.
Verification: 407 tests green in a clean venv across test_api_transport, test_session_renewal, test_session_renewal_multiprocess, test_secret_absence, and test_auth_login (573 passed under the ambient install; the one failure is the documented python -I user-site runner artifact, not PR code), ruff check . clean, and strict mypy clean across all 39 source files. One bookkeeping note: the PR description's accepted-trade-offs list still says the notice "is printed before the breaker gates run" — stale as of this commit, worth striking when the L4 "fix in progress" note is updated.
glm-5.3 ｜ 𝕏
Conflicts: tests/test_auth_login.py imports (keep both sides) and .secrets.baseline (main's, rescanned; only line numbers moved).
1 participant
Follows up #52 (closed): what was actually needed was a login that persists, not mail access.
What
Opt-in unattended sign-in with a stored authenticator-app secret, so an agent keeps working past the ~5-day session limit with no password or MFA prompt.
auth totp-setupreads an authenticator secret (base32 orotpauth://totp/URI) from a hidden prompt or stdin, never argv. It seals the secret inauthenticator.json, bound to the saved username.auth totp-removedeletes it.auth loginuses the stored authenticator when there is no--totp,--mfa-methodis unset orapp, and the username matches. It runs the existing pure-HTTP flow onPhoneAppOTP: the code is generated afterBeginAuth, submitted once, and never shown._replay_safe, notread_only_auth, built-in connection) whose session has expired (401, 401/403 fromwhoami, or a 403 thatwhoamiconfirms as signed out) or whose cookies are missing signs in once and retries once. Write requests, quiz-attempt flows and overridden connections never sign in. A write command's earlier reads (for examplesubmitlooking up the course) are ordinary reads and can, and the write is still sent once.auth statuschecksGET /d2l/api/lp/1.47/users/whoamiinstead of/d2l/api/versions/, so it reports whether the session is really signed in. It addsrenewedandunattended_login: {configured, blocked}.Safety
renewal_lock()is a thread lock plusflockonrelogin.lock, with a 120 s deadline. Waiters reuse cookies another process saved. The human login tail,import-session, the CDP refresh save and the legacy cookie migration take the same lock only around their writes. Withoutfcntl(not POSIX), the lock covers only threads and stored-authenticator sign-in is off:totp-setuprefuses,auth loginkeeps the person-driven flow, and reads behave as on main.totp-setupwith the same key under any username. Setup refuses to replace an authenticator file it can't read. A clock going backwards givesclock_skew. Onlytotp-removeforgets used steps; the docs give remove plus setup as the recovery from a fast clock, once the clock is right.relogin_state.jsonis plaintext and holds no secrets. Auth-class failures (credentials_rejected,code_rejected,method_unavailable,clock_skew,interrupt,unknown) block automatic attempts until a successful stored login,totp-setuportotp-remove.networkgives a 5-minute cooldown. Corrupt state fails closed. Automatic sign-in records itself as blocked (unknown) before contacting Microsoft, and the outcome replaces that, so an unwritable breaker or a process killed mid-sign-in never lets every read send the password again.ErrCodes are known codes only,repr=Falseis set on secret fields, and re-raised errors usefrom None. Sentinel tests check stdout, stderr, exception text, tracebacks and the config dir for the secret, the decoded key, codes, the password, cookies and the username.docs/auth-unattended.mdand thetotp-setuphelp). The password and the second factor share one sealed store. Anyone who can unseal it can sign in to the whole MAHE M365 account.Deviations from the reviewed plan (v2.5)
_CREDENTIAL_ERROR_CODESalso counts 50034, 50056, 50133 and 50144 ascredentials_rejected, alongside the plan's 50126/50053/50055/50057. They are all password or account refusals.RenewalUnavailableis namedRenewalUnavailableError(ruff N818).mfa_pending.json. A deferred two-step challenge would otherwise keep blocking automatic sign-in.totp-setupreads the secret before taking the lock, so a person typing doesn't hold it. It reserves the current step, so the code used to confirm registration in Microsoft is never replayed. The first login after setup can therefore wait up to 30 s.unknown) inrelogin_state.jsonbefore the password is sent, and stops withstoreif it can't. Success clears it, a failure records its category, and astorefailure restores the previous state (review rounds 1 and 2).totp-setupwith the same key keeps a newer recorded step. Round 1 let it drop a step more than two ahead of the clock; round 2 reverted that because it could resend an accepted code. Round 3 made the step follow the key whatever the username, and setup refuses to replace a file it can't read._safe_auth_error_messageby exact match, not by prefix.use_stored_authenticator), not a newLoginPlanmode.displaycan't importms_errors(layering), so it keeps a mirror of the category set. A test pins it equal toRELOGIN_CATEGORIES.check_auth()keeps its contract: a 503 givesFalse, andNetworkErrorstill propagates.coursessaid "Permission denied (HTTP 403)". On a 403 from a recoverable read, the client now askswhoamionce, without recovery. Only a signed-outwhoami(401, 403 or a login redirect) turns the 403 into an expiry, and the usual single sign-in and retry follow. Any other answer re-raises the original 403, so genuine denials such as closed-folder history keep it. The check is one attempt with a 15 s timeout and nothing is cached, so each genuine 403 costs one extra request. Without the stored authenticator, this expired-session 403 now tries the CDP refresh and reports "Session expired" like a 401.Tests
test_totp.py(RFC 6238 vectors, parsing),test_session_renewal.py(lock, reservation, gates, breaker),test_session_renewal_multiprocess.py(two spawned processes: one sign-in plus one reuse; a failure blocks the second process),test_ms_auth_provider.py(provider mode, failure categories, 5xx),test_auth_totp.py.test_api_transport.py,test_read_only_auth.py,test_cli_contracts.py,test_assessment_workflows.py,test_secret_absence.py,test_auth_login.py,test_credential_store.py,test_secrets_store.py.ruff check,mypy,lint-imports,deptry,xenon,check_secretsandpytest(2695 passed). The.secrets.baselinerefresh only moves line numbers of audited entries; new fixture lines use inlinepragma: allowlist secret.Live tests (MAHE, read-only, with Ravish present): passed
Run on 2026-10-15 at head 4db986f. Outputs were checked by field names and counts only; no secrets or personal data were printed.
totp-setupfrom stdin, thenauth login --jsonsigned in with no prompt (BeginAuth, EndAuth, ProcessAuth, KMSI, SAML).auth statusgavevalid: trueandunattended_login: {configured: true, blocked: false}.PhoneAppOTPproof.cookies.jsondeleted,courses --jsonsigned in once (one BeginAuth) and returned the course list.courses --jsonsigned in once and replaced the cookies.auth status --jsonruns while signed out: one signed in (renewed: true, one BeginAuth), the other reused its cookies (renewed: false, no BeginAuth). Both reportedvalid: true. The second process still printed "Signing in again with the stored authenticator..." although it didn't sign in; fixed in 0ee2e16, so only the process that signs in prints it.interruptfailure can't be triggered on demand, so it is covered by unit tests only.Found along the way: an expired MAHE password made a sign-in with a correct code fail with the generic "Authentication failed". #77 reports it as an expired password.
Not in this PR
auth refresh --cdp-portagainst Helium: its debug port has no/json/version, andNetwork.getAllCookiesis not available on the browser target. Separate follow-up.quiz_learner_session.pyandquiz_preview_session.pyimportfcntlat module level. Ordinary commands load them lazily, so only the quiz flows are affected. Separate follow-up.Review
clock_skewmessage, fixed in 34bf5ca)unknownlogin message, fixed in 6256fe0)unknownmessage and thestoredocs row, fixed in 6256fe0)unknownmessage, fixed in 6256fe0)unknownmessage, fixed in 6256fe0; setup kept the step only for the same username, fixed in ba2abbd)fcntlbroke ordinary commands on non-POSIX systems (e097ba1), and formatting an error twice dropped the sign-in category (4db986f)fcntlimport, as on main; see "Not in this PR")fcntlneeded outside unattended sign-in), 4db986f (sign-in category survives a second format), ba2abbd (setup never forgets used steps), 6256fe0 (unknownlogin message andstoredocs row), 34bf5ca (no step is ever resent; automatic sign-in blocked while it runs; recovery docs and message), 56673b2 (write-request scope in the docs; login help names the username match), 253a88e and 783702e (gitleaks fixture marker; ignore entry for the fixture in 55e806b), b61af23 (breaker fails closed; fast-clock recovery; docs), 0962d8b (auth login --helpexplains the stored authenticator).RenewalError(includingstore,busy,pending_human_login), the CDP browser refresh is not tried. A configured stored authenticator owns recovery, and the CDP save needs the same key source.whoamirequest (addendum A).storeand automatic sign-in stays blocked (unknown). The saved cookies keep working, andauth loginclears the block.auth loginis not marked as in flight. A caller started it and sees an interruption, and the breaker never refuses it.auth statusreportsblocked: true.auth login --save-credentials, the command reportsstore; the session is saved and works. The human login path warns and succeeds instead.networkfailure recorded while the clock was fast keeps the cooldown until the corrected clock passes it.auth loginworks meanwhile, and the docs say so.whoamimust return a positive integerIdentifierto count as signed in. Live tests L1–L3 check this on MAHE.$Config) isinterruptand blocks, as the plan specifies: it usually means Microsoft changed the page. Navigation outages arenetwork.unattended_login.configuredonly checks that both sealed files exist; it does not unseal them to compare usernames. After saving credentials for another account, it staystrue, and expiry falls back to the browser refresh.Success: trueiscode_rejected; a reply that is not a JSON object isinterrupt. Both block automatic sign-in.--totpwith--mfa-method app) is not recorded. If a slow clock is corrected inside the registration step, or a stored login follows a manual one within the same 30 s, the CLI can send that code again. Microsoft then either accepts it or rejects it ascode_rejected, which blocks. Setup step 6 says to wait for a new code first.totp-setupcannot read the stored authenticator, its message names the clock becausetotp-removeforgets used steps. The file may also be corrupt or sealed with another key; remove plus setup is the way out in every case, sincetotp-removedoesn't unseal.