DMD-1825 feat(auth): browser login via PKCE + device authorization (programmatic auth) - #535
DMD-1825 feat(auth): browser login via PKCE + device authorization (programmatic auth)#535zajca wants to merge 40 commits into
Conversation
Implements the plan in docs/programmatic-auth-login-plan.md: a new
`kbagent auth login|status|logout` group that signs in through the
browser and stores a user-scoped Keboola programmatic session
(kbc_at_* access token + rotating kbc_rt_* refresh token), used against
Storage and Manage as `Authorization: Bearer` + `X-KBC-ProjectId`.
Static Storage-token auth is unchanged and remains the default; both
modes coexist so existing users and CI need no changes.
Design highlights:
- No config.json schema change and no CURRENT_CONFIG_VERSION bump.
Session state lives in a sibling auth.json (0600); a session-registered
project carries the sentinel token `kbc-session://{project_id}`.
- auth.json uses a real cross-platform `filelock`, not ConfigStore's
fcntl helper, which is a silent no-op on Windows -- unserialized
rotation there could persist a stale pair and trigger server-side
refresh-token family revocation (a hard logout).
- Bearer auth is injected through an additive, keyword-only
`http_auth: httpx.Auth | None` on the shared HTTP clients, so none of
the ~150 (stack_url, token) factory call sites change shape. The hook
propagates to the queue/query/encryption/sync-actions sub-clients.
- PKCE falls back to the device flow ONLY on pre-exchange failures; a
state mismatch or an `error=` redirect is terminal, never retried
through another channel.
- Re-login persists the new session before revoking the one it replaces,
and records an orphan when revocation cannot be confirmed.
- v1 scope is Storage + Manage. `kbagent serve`, the importable SDK, the
MCP subprocess and the AI/data-science/metastore/dev-portal/stream
clients fail fast on a sentinel project with
AUTH_NOT_SUPPORTED_ON_STACK naming the static-token fallback, rather
than sending the sentinel as if it were a credential.
Also fixes a defect found while testing the reactive path: a 401-driven
force_refresh could re-adopt the token the server had just rejected,
because the "another process already rotated" shortcut only checked
nominal expiry. Revocation is not expiry, so the rejected token is now
excluded from that shortcut.
E2E coverage for the bearer capability matrix is written and gated, but
has not been run against a stack with programmatic-auth enabled.
zajca
changed the title
Plan implemented (v0.77.0)This PR is no longer docs-only: the plan in What landedNew
All four blocking review findings are implemented: B-1 persist-new-before-revoking-old with orphan recording, B-2 body-based revoke contract, B-3 a Manage credential abstraction with privileged commands kept on the stronger static token, B-4 a real cross-platform Two defects found and fixed while building
Verification
What has now been exercised against a real stackCorrection to the earlier version of this comment, which said no flagged stack was available.
The session created during that run was revoked immediately afterwards and its scratch config dir deleted. Still unverified — please read before mergingOnly the login half of the flow has met a real server. The capability matrix in
Review finding NB-3 asked that no merge advertise an unverified auth path. Suggest running |
`auth login` printed the accessible-project table but registered nothing
unless `--register-projects` was passed, so the natural next step failed:
$ kbagent auth login --stack connection.keboola.com # lists project 9840
$ kbagent storage buckets --project 9840
Error: Project '9840' not found ... Run 'kbagent project add'
Three defects behind that: `--register-projects` was undiscoverable, the
numeric project id is never a valid alias (aliases come from the slugified
project NAME), and the remedy pointed at `project add`, which is useless to
a session user with no static token to paste.
- new `kbagent auth register-projects [--stack] [--all] [--project-id ID]
[--alias ID=ALIAS] [--yes]`, and a TTY offer to run the same picker right
after a successful login. The plan listed the picker as an optional
alternative to the flag in section 4.5 step 5; only the flag had shipped.
- `project_not_found_error` now points session users at the picker (and
names the id-is-not-an-alias trap) when a sibling auth.json holds a
session; static installs see the unchanged message.
- duplicate project names no longer collapse: previously the second project
sharing a slug was silently skipped and left unusable. Aliases now walk
`base` -> `base-{id}` -> `base-{id}-2`. A collision with an existing
static-token project also suffixes, never overwrites.
- already-registered projects are matched on (project_id, stack_url), not
on the alias string, so a hand-picked alias is not offered again.
- the picker's selection prompt defaults to `all`: by then the user has
opted in twice, and defaulting to `none` meant two bare Enters dropped
them back into the exact broken state above.
- alias format validation moved to `config_store.validate_alias_format`,
shared with `project edit --new-alias` so the two cannot drift.
Registration is additive and never overwrites; `--deny-writes` blocks the
new command via a `auth.register-projects` write entry. `--json` keeps a
single JSON document on stdout, with the hint on stderr, and a failure in
the optional post-login hook never changes a successful login's exit code.
Follow-up: interactive project picker (273d02b)Dogfooding the login flow surfaced a usability hole worth fixing before merge. Three separate defects: What changed
Verification
The "Still unverified" list in the comment above is unaffected: this is all local config-registration behaviour and needed no flagged stack. |
|
Serve REST surface for |
Real-server bug found in the refresh path (d361033)The "Still unverified" list above named "the 401 -> refresh -> retry path against a real server 401". It has now had its first real-server contact, and the assumption behind it was wrong.
FixClassification moved into
Why it survived review and unit testsThe pre-existing test asserted a 400 carrying Worth noting for anyone reviewing the local pre-checks: this could not have been caught client-side. Verification
Still openWhy the refresh token was rejected server-side is unknown. The session refreshed successfully about 90 minutes earlier, so it was not a plain 30-day expiry. Worth a look from whoever owns the auth service — a shorter real refresh TTL, or family revocation triggered by something we are not accounting for, would both be worth knowing before release. The CLI now degrades correctly either way. |
zajca
changed the title
Implements the plan in docs/programmatic-auth-login-plan.md: a new
`kbagent auth login|status|logout` group that signs in through the
browser and stores a user-scoped Keboola programmatic session
(kbc_at_* access token + rotating kbc_rt_* refresh token), used against
Storage and Manage as `Authorization: Bearer` + `X-KBC-ProjectId`.
Static Storage-token auth is unchanged and remains the default; both
modes coexist so existing users and CI need no changes.
Design highlights:
- No config.json schema change and no CURRENT_CONFIG_VERSION bump.
Session state lives in a sibling auth.json (0600); a session-registered
project carries the sentinel token `kbc-session://{project_id}`.
- auth.json uses a real cross-platform `filelock`, not ConfigStore's
fcntl helper, which is a silent no-op on Windows -- unserialized
rotation there could persist a stale pair and trigger server-side
refresh-token family revocation (a hard logout).
- Bearer auth is injected through an additive, keyword-only
`http_auth: httpx.Auth | None` on the shared HTTP clients, so none of
the ~150 (stack_url, token) factory call sites change shape. The hook
propagates to the queue/query/encryption/sync-actions sub-clients.
- PKCE falls back to the device flow ONLY on pre-exchange failures; a
state mismatch or an `error=` redirect is terminal, never retried
through another channel.
- Re-login persists the new session before revoking the one it replaces,
and records an orphan when revocation cannot be confirmed.
- v1 scope is Storage + Manage. `kbagent serve`, the importable SDK, the
MCP subprocess and the AI/data-science/metastore/dev-portal/stream
clients fail fast on a sentinel project with
AUTH_NOT_SUPPORTED_ON_STACK naming the static-token fallback, rather
than sending the sentinel as if it were a credential.
Also fixes a defect found while testing the reactive path: a 401-driven
force_refresh could re-adopt the token the server had just rejected,
because the "another process already rotated" shortcut only checked
nominal expiry. Revocation is not expiry, so the rejected token is now
excluded from that shortcut.
E2E coverage for the bearer capability matrix is written and gated, but
has not been run against a stack with programmatic-auth enabled.
`auth login` printed the accessible-project table but registered nothing
unless `--register-projects` was passed, so the natural next step failed:
$ kbagent auth login --stack connection.keboola.com # lists project 9840
$ kbagent storage buckets --project 9840
Error: Project '9840' not found ... Run 'kbagent project add'
Three defects behind that: `--register-projects` was undiscoverable, the
numeric project id is never a valid alias (aliases come from the slugified
project NAME), and the remedy pointed at `project add`, which is useless to
a session user with no static token to paste.
- new `kbagent auth register-projects [--stack] [--all] [--project-id ID]
[--alias ID=ALIAS] [--yes]`, and a TTY offer to run the same picker right
after a successful login. The plan listed the picker as an optional
alternative to the flag in section 4.5 step 5; only the flag had shipped.
- `project_not_found_error` now points session users at the picker (and
names the id-is-not-an-alias trap) when a sibling auth.json holds a
session; static installs see the unchanged message.
- duplicate project names no longer collapse: previously the second project
sharing a slug was silently skipped and left unusable. Aliases now walk
`base` -> `base-{id}` -> `base-{id}-2`. A collision with an existing
static-token project also suffixes, never overwrites.
- already-registered projects are matched on (project_id, stack_url), not
on the alias string, so a hand-picked alias is not offered again.
- the picker's selection prompt defaults to `all`: by then the user has
opted in twice, and defaulting to `none` meant two bare Enters dropped
them back into the exact broken state above.
- alias format validation moved to `config_store.validate_alias_format`,
shared with `project edit --new-alias` so the two cannot drift.
Registration is additive and never overwrites; `--deny-writes` blocks the
new command via a `auth.register-projects` write entry. `--json` keeps a
single JSON document on stdout, with the hint on stderr, and a failure in
the optional post-login hook never changes a successful login's exit code.
8a531c0 to
34fb6b6
Compare
padak
left a comment
padak
left a comment
There was a problem hiding this comment.
Security review — token-handling core
Scoped review of the crypto / credential-lifecycle code only: auth/pkce.py, auth/device.py, auth/state_store.py, auth/token_provider.py, auth/auth_client.py, auth/environment.py, the http_base.py bearer plumbing, and the rotation/revocation logic in services/auth_service.py. Architecture, plugin-drift, test-coverage and CI-gate compliance are not covered here.
Reviewed at 34fb6b6. Verdict: comment — no blocker in the cryptographic construction, four findings in the credential-lifecycle logic. One (F1) defeats a guarantee this PR names as blocking finding B-1 and prints to the user.
What holds up
I tried to break these and could not:
stateis compared withhmac.compare_digestbefore theerror=branch (auth/pkce.py:201), so a forgederror=callback cannot bypass it. Verifier 384-bit / state 256-bit viasecrets.token_urlsafe,S256only.- The loopback listener binds
127.0.0.1/[::1]only, never0.0.0.0;log_messageis silenced so the authorization code never reaches stderr;_open_silentlyswallows everything sothreading.excepthookcannot print the URL. auth.jsonis created withos.open(..., 0o600)before the first byte, written tmp+os.replace, and re-narrowed on load if widened.filelockrather than thefcntlhelper is the right call — that helper really is a silent no-op on Windows.- Bearer mode omits
X-StorageApi-Token/X-KBC-ManageApiTokenentirely rather than sending an empty or sentinel value, and the auth hook is propagated to the queue / query / encryption / sync-actions / stream sub-clients (client/_core.py:117), so none of them silently fall back to unauthenticated. - No token value reaches stdout, a log record, or an error message on any path I traced.
- Threading model is sound: one shared provider per
(auth.json, stack)means the in-process lock actually serializes, and the thread-lock → file-lock ordering has no inversion.
F1 · auth login silently discards previously recorded orphan session ids
services/auth_service.py:259-291
new_session is constructed without orphaned_session_ids, so it defaults to []. previous is read afterwards (line 277) and put_session(new_session) (line 280) replaces the whole StackSession row — the previous session's orphan list is dropped before anything can carry it forward.
Failure scenario:
auth login→ session A.auth loginagain, revoke of A comes back unconfirmed → session B persisted,orphaned_session_ids = ["A"]. The user is told: "kbagent auth logoutwill retry it."auth logina third time → session C persisted withorphaned_session_ids = []; only B is revoked.auth logoutretries C's list. Session A is still live server-side and no local state remembers it exists, so nokbagentcommand can ever revoke it.
This defeats the B-1 guarantee for the exact scenario B-1 was built for, and the promise is made in the warning text login itself prints. No test covers it — tests/test_auth_service.py:479 asserts orphaned_session_ids == [] after a login, but starts from a session that had none, so it cannot distinguish "was empty" from "was cleared".
Fix is to read previous before constructing new_session and seed orphaned_session_ids=list(previous.orphaned_session_ids) if previous else [].
Related, and worth a sentence in docs/auth.md rather than a code change: logout step 4 deletes the whole row including orphans_remaining, so an orphan that could not be revoked is also forgotten there. That one is at least reported back in LogoutResult.orphans_remaining; F1 is silent.
F2 · AUTH_REFRESH_MAX_WALL_CLOCK is not a wall-clock bound
constants.py:585-598
AUTH_REFRESH_TIMEOUT = httpx.Timeout(connect=5.0, read=5.0, write=2.0, pool=2.0)
AUTH_REFRESH_MAX_WALL_CLOCK = sum(...) # 14.0httpx's read and write timeouts are per I/O operation, not per request. Verified against the installed httpx 0.28.1 — httpcore._sync.http11.HTTP11Connection._receive_event passes timeout= to self._network_stream.read(...) inside a while True: loop, so the deadline resets on every chunk. There is no total-duration option in httpx.
So a server that dribbles the response — or a stalled proxy, or a hostile stack — keeps the refresh alive well past 14 s. SessionTokenProvider._refresh_locked holds auth.json.lock across that call, so the hold outlasts AUTH_LOCK_TIMEOUT (30 s) and every concurrent kbagent process — including a read-only auth status — fails with ConfigError "Another kbagent process may be stuck holding it" and exit 5. That is precisely the misleading failure B5 says this bound prevents.
The test asserting AUTH_REFRESH_MAX_WALL_CLOCK * 2 <= AUTH_LOCK_TIMEOUT verifies the arithmetic, not the property. Real options: enforce a deadline around the call (thread + monotonic budget), or stop holding the file lock across the network call and re-validate after reacquiring. Documenting the limit honestly would also be an improvement over the current claim.
F3 · _GRANT_REJECTION_MARKERS is broad enough to purge a valid session on our own bug
auth/auth_client.py:62-67
_GRANT_REJECTION_MARKERS = (
"invalid_grant",
"invalid refresh token",
"expired refresh token",
"refresh token", # <-- catch-all
)_is_rejected_grant's docstring states the 400-requires-a-marker rule exists because "a 400 can also mean we sent a malformed body, i.e. our own bug — and purging on that would destroy a still-valid refresh token". The bare "refresh token" substring re-opens exactly that hole: a server-side validation error naming the field in prose ("The refresh token must be a string.", "refresh token is required") matches, is classified SESSION_EXPIRED, and SessionTokenProvider._perform_refresh then deletes the session from auth.json — forcing an avoidable re-login on a credential that was never rejected.
The first three markers plus the unconditional-401 rule already cover the real wire shapes the 2026-07-28 comment documents. Dropping the fourth entry restores the stated invariant.
F4 · verificationUriComplete is opened in the browser without scheme validation (nit)
services/auth_service.py:350-351
The value is fully server-supplied and goes straight to _browser_opener → webbrowser.open. normalize_stack_url constrains the stack URL to https://, but nothing constrains this one, so a rogue or compromised stack can hand back a file:// URL, a custom scheme handler, or a --prefixed string that some openers read as a flag.
Low severity — you have already chosen to trust the stack you are logging into, and a hostile stack has better options. But an https:// prefix check before the open is one line and matches the posture already taken on the stack URL.
Not findings, recorded so they are not re-derived
BearerAuth.auth_flowre-yields the samehttpx.Requeston 401. That would raiseStreamConsumedfor a request with a non-replayable body, but I traced every streaming body inclient/(storage_tables.py:558/568/585/596/602) and all five go to the cloud provider through a separate barehttpx.Client, never the bearer-authed one. No exposure today; worth remembering if a streaming upload is ever moved onto the Storage client.- A
KeboolaApiErrorraised out offorce_refreshpropagates cleanly throughBaseHttpClient._do_request(it catches onlyTimeoutException/ConnectError), soSESSION_EXPIREDsurvives to the exception handlers. Confirms the D5 mapping works. normalize_stack_urlnow dropsuser:pass@userinfo thatmainpersisted verbatim. That is a real improvement, but it is also a silent behaviour change for anyone who had one — the URL changes value on next save and basic-auth stops being sent, surfacing as an opaque 401 rather than an error. A changelog line would be cheap insurance.- The
SESSION_AUTH_FEATUREruntime guard is by construction inert for the bearer-capable clients (KeboolaClient,ManageClient,AuthClientall leave itNone), so a directKeboolaClient(url, project.token)that skipsmake_client_factorystill puts the sentinel on the wire as a header value. That is whatscripts/check_sentinel_guards.pyexists for — whether the AST analysis actually closes it is outside this review's scope.
padak
left a comment
padak
left a comment
There was a problem hiding this comment.
Review of #535 — DMD-1825 feat(auth): browser login via PKCE + device authorization (programmatic auth)
Scope note: this comment covers architecture/layering, CONTRIBUTING.md &
CLAUDE.md compliance (3-layer split, ErrorCode enum, dataclasses,
file-size budgets, the Plugin synchronization map), test-coverage
adequacy, and the newscripts/check_sentinel_guards.pyCI gate. A
separate comment on this PR covers the security review of the
crypto/token-handling core (auth/pkce.py,auth/device.py,
auth/state_store.py,auth/token_provider.py,auth/auth_client.py,
http_base.pybearer plumbing, rotation/revocation logic). The two are
complementary; this one does not re-derive F1–F4 or the "what holds up"
list from that review.Generated by
kbagent-pr-reviewersubagent. Verdict and findings below
are advisory; the human author retains every veto. CI-coverable issues
(lint, format, tests) are confirmed viamake check, not duplicated here.
Summary
This PR adds kbagent auth login/status/logout/register-projects: a
browser-based (PKCE, falling back to RFC 8628 device flow) alternative to a
hand-copied static Storage token, with session credentials in a new
auth.json (0600) and a kbc-session://{project_id} sentinel left in
config.json. Outside the crypto core, the engineering is unusually
thorough: make check passes clean (5151 passed / 8 skipped — matches the
PR body's own figure exactly), every hand-maintained plugin/doc
sync-surface is updated and cross-checked, the 8 new error codes and the
OPERATION_REGISTRY/FLAG_ESCALATIONS entries are wired and actually
enforced, and every sentinel-preservation call site I traced
(config_store.edit_project, project_service, org_service.refresh_tokens)
is correctly guarded. Verdict: REQUEST CHANGES — the new
scripts/check_sentinel_guards.py CI gate (the mechanism meant to keep all
of the above honest going forward, and the one the security review
explicitly left open) has real, demonstrated coverage gaps and ships with
zero tests of its own.
Verdict
- Verdict: REQUEST CHANGES
- Blocking findings: 1
- Non-blocking findings: 2
- Nits: 0
Blocking findings
[B-1] scripts/check_sentinel_guards.py — does not close the "SESSION_AUTH_FEATURE is inert" gap, and Check 1 has two proven blind spots of its own
Direct answer to the question the security review left open: no, this
script does not close it. SESSION_AUTH_FEATURE being None on
_CoreClient/ManageClient/AuthClient means a direct
KeboolaClient(url, project.token) that skips make_client_factory puts a
sentinel on the wire as a literal header value — but Check 2
(_undecided_clients, lines 202-217) only inspects class definitions:
does _CoreClient's ClassDef declare SESSION_AUTH_FEATURE, or is its
name in the hardcoded BEARER_CAPABLE_CLIENTS allowlist? Once a class is
allowlisted, the script never again looks at how many places construct it
or with what token — there is no check anywhere in this file that walks
KeboolaClient(/ManageClient( call sites. I confirmed the 3 real
construction sites today (lib.py:253, services/base.py:128 and the
make_client_factory closure at services/base.py:150-165) are all
correctly gated by require_static_token/is_session_token — but that
safety is entirely a property of manual review, not of this gate. A future
service that does KeboolaClient(stack_url, project.token) directly
(bypassing the factory, e.g. copying a pre-0.77.0 pattern) would compile,
pass check-sentinel-guards, and pass make check, while silently sending
kbc-session://{id} as X-StorageApi-Token on the wire.
Separately, Check 1 (_unguarded_credential_writers, lines 180-199) has two
independently-reproduced blind spots of the same family: (a) it exempts an
entire file from scrutiny if any of 6 sentinel-aware names appears
anywhere in its raw text (lines 187-189), not scoped to the call site —
I built a synthetic fixture with one correctly-guarded function and one
unrelated, genuinely-unguarded config_store.edit_project(alias, token=new_token) elsewhere, and the whole file was skipped; (b) it only
fires on an explicit literal token= keyword (line 197) — a **kwargs
passthrough (edit_project(alias, **updates), the exact idiom the
codebase's own guarded call site at services/project_service.py:299
already uses) has kw.arg=None in the AST and is structurally invisible to
the scan.
Compounding all three: there is no tests/test_check_sentinel_guards.py at
all. Its sibling gate, scripts/check_command_sync.py, has
tests/test_check_command_sync.py, whose own docstring states the
convention this PR should have followed: prove the checker "actually
detects each drift class... on synthetic inputs." Nothing here is failing
today (make check-sentinel-guards reports "OK" on the live tree), and
this is not a request to re-litigate the credential/session design the
security review covered — it's that the enforcement mechanism named as the
safety net for exactly that design (D2 in the PR body) is narrower than
its docstring and the PR's own description claim. Fix: add a Check 4 that
walks KeboolaClient(/ManageClient( (and any future bearer-capable
client) construction call sites outside services/base.py and lib.py's
guarded lines and flags any that don't pass through a sentinel-aware
factory; scope Check 1's exemption to the enclosing function; detect
dict-unpacking keywords; add a synthetic-fixture test file mirroring
test_check_command_sync.py.
Non-blocking findings
[NB-1] tests/test_e2e_auth.py — the 4 new CLI commands themselves are never driven end-to-end
The "bearer capability matrix" (TestBearerCapabilityMatrix,
TestBearerReactiveRefresh, TestBearerManageApi) is valuable and does hit
a real stack, but it constructs AuthClient/SessionTokenProvider/
BearerAuth/KeboolaClient directly — it never invokes kbagent auth login, status, logout, or register-projects themselves (the one
CLI-driven scenario, the device flow, is @pytest.mark.skip'd as a manual
runbook). Per CONTRIBUTING.md's "every CLI command must have E2E coverage"
(line 377), none of the four new commands has that; CLI-layer coverage
stops at mocked CliRunner tests (tests/test_cli_auth.py). The PR is
candid that provisioning E2E_SESSION_REFRESH_TOKEN/
E2E_SESSION_PROJECT_ID into CI is a separate ops task, and 3 of the 4
commands are genuinely hard to automate unattended — but auth status
needs no browser and no interactivity, and driving it once through
CliRunner/subprocess against the same provisioned session would close
most of the practical gap cheaply.
[NB-2] PR body's self-reported metrics are stale versus the actual code
Two numbers the description asks reviewers to trust "so a reviewer does not
have to re-measure" don't match. keboola-expert.md is stated as "60 326 B
against the 60 KiB (61 440 B) hard cap" — the actual file is 60,645 B
(confirmed via wc -c and via len(agent_body.encode("utf-8")), the same
measurement tests/test_agent_prompt.py:31 uses), and the real enforced
constant there is PROMPT_BYTE_BUDGET = 62_000, not 61,440.
commands/auth.py is stated as 564 LOC; wc -l shows 570. Neither
discrepancy breaks anything (60,645 < 62,000; 570 is still well under the
800-LOC soft ceiling), so this is informational, not a defect — worth a
quick re-measure before merge since the description explicitly invites
skipping verification of them.
Nits
(none)
Verification log
- Working tree:
git rev-parse HEAD→34fb6b6..., confirmed equal to the
PR head before drawing any conclusions from file contents. gh pr view 535 --json title,body,files,additions,deletions,baseRefName,headRefName,labels,state
→ state OPEN,feat(auth):prefix matches the change type;gh's
filesfield caps at 100,git diff --stat main...HEADconfirms the
real total: 104 files, +16755/-219, 34 new files, 70 modified.- Read
CONTRIBUTING.md"Checklist: Adding a New CLI Command", "Plugin
synchronization map", "Releasing a new version";CLAUDE.mdconvention
#17/#18 and "All CLI Commands";keboola-expert.md§1/§2/§3. - Version/changelog:
pyproject.toml0.76.3→0.77.0;plugin.json+
marketplace.jsonsynced;changelog.pyhas a"0.77.0"key covering
the feature.errors.pydefines all 8 newErrorCodemembers
(AUTH_NOT_SUPPORTED_ON_STACK,AUTH_FLOW_TIMEOUT,AUTH_FLOW_DENIED,
AUTH_FLOW_EXPIRED,AUTH_BROWSER_UNAVAILABLE,AUTH_STATE_MISMATCH,
SESSION_EXPIRED,SESSION_NOT_FOUND). permissions.py:auth.login/auth.status/auth.logout/
auth.register-projectsall present inOPERATION_REGISTRY;
"auth.logout --remove-projects": "admin"inFLAG_ESCALATIONS, and
traced its enforcement tocommands/auth.py:432
(check_cli_operation(ctx, "auth.logout --remove-projects")) — a real,
wired escalation, not just a declared dict entry.- Plugin synchronization map — every hand-maintained ("NO" CI-coverage)
surface checked present and consistent:commands/context.py
AGENT_CONTEXT,CLAUDE.mdcommand list,keboola-expert.md(§2 matrix
row, §1 Rule 6 version gate, §3 gotchas — auth section is 60,645 B total
file, under the 62,000 B budget),SKILL.md(triggers + table +
workflow link),commands-reference.md,gotchas.md(tagged
(since v0.77.0)), newauth-workflow.md(302 lines). - Layer-violation greps (typer/console in
services/, httpx in
commands/, formatter/typer in client-layer files) → all empty, no
violations.SESSION_AUTH_FEATUREspread confirmed across
ai_client.py,data_science_client.py,metastore_client.py,
scheduler_client.py,stream_client.py. python3 scripts/check_sentinel_guards.py --list→ 12 guards, 9
SESSION_UNSUPPORTED_FEATURESentries, matches the PR body's disclosed
list (including the "dev-portal has no guard" correction).- Built a synthetic fixture in scratchpad and re-ran Check 1's exact logic
against it to confirm both blind spots in[B-1](file-wide exemption;
**kwargsinvisibility viakw.arg is None) — reproduced, not
theoretical. Confirmed notests/test_check_sentinel_guards.pyexists
(find/grep); readtests/test_check_command_sync.py's docstring for
the sibling-gate testing convention. - Traced all 3 real
KeboolaClient(/ManageClient(construction call
sites outside test code (lib.py:253,services/base.py:128,
services/base.py:150-165) — all correctly gated today by
require_static_token/is_session_token, confirming[B-1]'s claim
that this safety is manual-review-only, not gate-enforced. - Test counts by layer:
test_auth_service.py48 tests,
test_cli_auth.py54 tests (39CliRunnerinvocations across all 4
subcommands via grep),test_auth_register_projects.py19,
test_e2e_auth.py8 (all skip without live session env vars; none
drive the CLI layer, per[NB-1]). make check(full pipeline, backgrounded, ~124s) → exit 0:ruff checkclean,ruff format --checkclean,ty check"Found 3
diagnostics" (all pre-existing and unrelated to this PR — confirmed via
git diff --name-only main...HEADnot touching the 3 flagged files:
scripts/hatch_build.py,tests/test_mcp_deprecation_warnings.py,
tests/test_tool_call_permissions.py— out of scope for this PR),
SKILL.mdup-to-date, version in sync,check_command_sync.py"OK: all
255 CLI commands registered/documented",changelog-check"All 44
stable releases have changelog entries",check_error_codes.py"OK: no
raw error_code literals",check_sentinel_guards.py"OK" (passes on the
live tree — consistent with[B-1]being a latent/future-risk gap, not
an active failure today), pytest "5151 passed, 8 skipped, 144
deselected in 124.28s" — exact match to the PR body's claimed test
counts, and internally consistent (5151+8=5159 selected, matching
"collected 5303 items / 144 deselected / 5159 selected").server/app.pyrouter list (app.include_router(...)) has no auth
router — consistent with the PR's stated, reasoned skip of the REST
surface forlogin/logout/register-projects.README.md,.gitignorediffs spot-checked — new "Browser login" setup
section, correctly scoped.gitignoreaddition.- Did not re-derive
auth/pkce.py,auth/device.py,
auth/state_store.py,auth/token_provider.py,auth/auth_client.py,
or thehttp_base.pybearer plumbing/rotation logic line-by-line — left
to the parallel security review per this run's scope split.
Open questions for the author
(none)
Implements the plan in docs/programmatic-auth-login-plan.md: a new
`kbagent auth login|status|logout` group that signs in through the
browser and stores a user-scoped Keboola programmatic session
(kbc_at_* access token + rotating kbc_rt_* refresh token), used against
Storage and Manage as `Authorization: Bearer` + `X-KBC-ProjectId`.
Static Storage-token auth is unchanged and remains the default; both
modes coexist so existing users and CI need no changes.
Design highlights:
- No config.json schema change and no CURRENT_CONFIG_VERSION bump.
Session state lives in a sibling auth.json (0600); a session-registered
project carries the sentinel token `kbc-session://{project_id}`.
- auth.json uses a real cross-platform `filelock`, not ConfigStore's
fcntl helper, which is a silent no-op on Windows -- unserialized
rotation there could persist a stale pair and trigger server-side
refresh-token family revocation (a hard logout).
- Bearer auth is injected through an additive, keyword-only
`http_auth: httpx.Auth | None` on the shared HTTP clients, so none of
the ~150 (stack_url, token) factory call sites change shape. The hook
propagates to the queue/query/encryption/sync-actions sub-clients.
- PKCE falls back to the device flow ONLY on pre-exchange failures; a
state mismatch or an `error=` redirect is terminal, never retried
through another channel.
- Re-login persists the new session before revoking the one it replaces,
and records an orphan when revocation cannot be confirmed.
- v1 scope is Storage + Manage. `kbagent serve`, the importable SDK, the
MCP subprocess and the AI/data-science/metastore/dev-portal/stream
clients fail fast on a sentinel project with
AUTH_NOT_SUPPORTED_ON_STACK naming the static-token fallback, rather
than sending the sentinel as if it were a credential.
Also fixes a defect found while testing the reactive path: a 401-driven
force_refresh could re-adopt the token the server had just rejected,
because the "another process already rotated" shortcut only checked
nominal expiry. Revocation is not expiry, so the rejected token is now
excluded from that shortcut.
E2E coverage for the bearer capability matrix is written and gated, but
has not been run against a stack with programmatic-auth enabled.
0f15928 to
cbb915e
Compare
`auth login` printed the accessible-project table but registered nothing
unless `--register-projects` was passed, so the natural next step failed:
$ kbagent auth login --stack connection.keboola.com # lists project 9840
$ kbagent storage buckets --project 9840
Error: Project '9840' not found ... Run 'kbagent project add'
Three defects behind that: `--register-projects` was undiscoverable, the
numeric project id is never a valid alias (aliases come from the slugified
project NAME), and the remedy pointed at `project add`, which is useless to
a session user with no static token to paste.
- new `kbagent auth register-projects [--stack] [--all] [--project-id ID]
[--alias ID=ALIAS] [--yes]`, and a TTY offer to run the same picker right
after a successful login. The plan listed the picker as an optional
alternative to the flag in section 4.5 step 5; only the flag had shipped.
- `project_not_found_error` now points session users at the picker (and
names the id-is-not-an-alias trap) when a sibling auth.json holds a
session; static installs see the unchanged message.
- duplicate project names no longer collapse: previously the second project
sharing a slug was silently skipped and left unusable. Aliases now walk
`base` -> `base-{id}` -> `base-{id}-2`. A collision with an existing
static-token project also suffixes, never overwrites.
- already-registered projects are matched on (project_id, stack_url), not
on the alias string, so a hand-picked alias is not offered again.
- the picker's selection prompt defaults to `all`: by then the user has
opted in twice, and defaulting to `none` meant two bare Enters dropped
them back into the exact broken state above.
- alias format validation moved to `config_store.validate_alias_format`,
shared with `project edit --new-alias` so the two cannot drift.
Registration is additive and never overwrites; `--deny-writes` blocks the
new command via a `auth.register-projects` write entry. `--json` keeps a
single JSON document on stdout, with the hint on stderr, and a failure in
the optional post-login hook never changes a successful login's exit code.
padak
left a comment
padak
left a comment
There was a problem hiding this comment.
Security re-review — token lifecycle, at bdd9a78
Re-verification of F1–F4 from my earlier security review. Scope unchanged: credential lifecycle only. The CI-gate blocker from the other review thread is being re-verified separately.
All four are fixed, and three of them go further than what I asked for. One new question falls out of the F2 rewrite — see below.
Verified closed
F1 — orphans dropped on re-login. auth_service.py:274,291. previous is now read before new_session is built and the list is carried forward. Pinned by test_orphans_recorded_by_an_earlier_login_survive_the_next_login and test_third_login_accumulates_orphans_instead_of_forgetting_the_first — the second is exactly the three-login scenario I described, so the regression cannot come back silently.
F2 — AUTH_REFRESH_MAX_WALL_CLOCK was not a wall-clock bound. Rewritten rather than patched: the file lock is no longer held across the network call at all, cross-process serialization moved to a self-expiring refresh lease in auth.json, and the ceiling is now enforced for real by a worker thread plus join(timeout). The docstring in _refresh_within_budget states the httpx per-I/O-operation semantics explicitly, and — worth calling out — is honest about what abandoning does not do: the request is not aborted, the worker outlives the call, and under kbagent serve repeated stalls can park several of them. That is the right way to document a limitation you have chosen to accept.
F3 — over-broad "refresh token" marker. Replaced with a three-part test (auth_client.py:105-137): invalid_grant is decisive alone; otherwise a 400 must name the credential and pass a verdict on it and not read as a complaint about request shape. My counterexample "The refresh token must be a string." is now vetoed twice over (must be, no verdict marker). The comment spelling out the asymmetry — refusing to purge leaves a recoverable error, purging wrongly destroys a working credential — is the correct framing and better than the narrowing I suggested.
F4 — unvalidated verificationUriComplete. _is_browser_safe_url gates the open on an https:// prefix, and a non-https value is reported to the user rather than silently skipped. Better than the silent drop I proposed. Covered by test_non_https_link_is_reported_and_never_opened.
Lease handling around the edges also checks out: refresh_leases is a sibling of sessions so a session write cannot clobber a claim, release/extend are holder-scoped so nobody can drop a claim they do not own, and is_live's forward-skew horizon is named honestly as covering only one skew direction.
New · AUTH_REFRESH_ABANDON_GRACE is set to exactly the server's grace window
constants.py:609-618
AUTH_REFRESH_MAX_WALL_CLOCK = 14.0
AUTH_REFRESH_LEASE_TTL = 16.0
AUTH_REFRESH_WAIT_TIMEOUT = 20.0
AUTH_REFRESH_ABANDON_GRACE = 30.0 # == the server's idempotent grace window
The intent is stated as "nobody may re-present that token until the server's idempotent grace window has passed." Trace the timeline for the case that matters — the abandoned request did reach the server and rotate:
t≈0request sent; server consumes the old refresh token att≈ε. Its 30 s idempotent window runsε … ε+30.t=14client abandons; lease extended tot=44.t=44earliest any process may re-present the stored (now consumed) token.
44 > ε+30. The retry lands outside the window by design, and re-presenting a consumed refresh token past the grace is precisely what triggers family revocation — a hard logout. The grace window exists to forgive exactly this re-presentation, and the current arithmetic guarantees we arrive after it has closed.
The two clocks also start at different events: the server's runs from when it processed the token, the extension from when we gave up waiting. Setting the extension equal to the window is the one value that guarantees landing at or past the boundary regardless of which way the difference falls. Clearly shorter (retry inside the window, accept a bounded concurrent-presentation risk, recover idempotently) or clearly longer (treat the credential as dead and drive a clean auth login instead of an opaque revocation) are both defensible; equal is the coin-flip.
I cannot settle this from the client side — it depends on grace-window semantics only the auth service knows, and the 2026-07-28 comment already records one server-side refresh rejection whose cause is still unknown. Worth confirming with whoever owns the auth service before release, since it decides whether an abandoned refresh self-heals or logs the user out.
Secondary UX consequence of the same numbers: AUTH_REFRESH_WAIT_TIMEOUT (20 s) is shorter than the abandon-extended lease (30 s), so for ~30 s after any abandoned refresh every command against that stack stalls 20 s and then fails with "Another kbagent process is still refreshing your Keboola login". Truthful, but a stall worth knowing about — and it argues for the shorter-grace option above.
Not blocking from my side
Nothing in the lifecycle code is. The F2 rewrite is a bigger change than the finding strictly required and it landed with real concurrency tests — cross-process lease takeover, abandoned-request lease retention, fast-clock skew, and a rotation landing after the ceiling. That is the right level of proof for this kind of primitive.
padak
left a comment
padak
left a comment
There was a problem hiding this comment.
Re-verification of [B-1] — sentinel-guard CI gate (scripts/check_sentinel_guards.py)
Scope note: this comment verifies only my earlier BLOCKING finding B-1
from the architecture/CI-gate review (scripts/check_sentinel_guards.py
coverage gaps + missing tests), against headbdd9a78(commits102bcd9
andcbb915e). It does not re-review anything else from that review, and
it does not touch the token-lifecycle findings (F1–F4) from the separate
security review — those are being re-verified independently.
Verdict: B-1 is PARTIALLY CLOSED. Two of the three sub-claims are
robustly fixed; the third (the direct-construction gap) is fixed for the
literal scenario I named, but adversarial probing against the real class
hierarchy found a concrete, unguarded route to the exact same wire-level
vulnerability one level down. Details and evidence below.
1. Check 1 blind spots (file-wide exemption + **kwargs invisibility)
**kwargs-passthrough invisibility: CLOSED. Re-ran the exact family of
shapes from my original finding (edit_project(alias, **updates), plus a
nested **{**base, "token": t} merge I added). Both are now detected;
matches the shipped parametrized test test_every_credential_passing_shape_is_detected.
File-wide exemption → scoped-to-function: CLOSED, verified independently.
My literal original probe (is_session_token used in one function,
genuinely unguarded config_store.edit_project(alias, token=new_token) in
an unrelated second function of the same file) still slipped through when
I re-ran it byte-for-byte — but I traced this to a red herring: my probe's
own docstring for the unguarded function narrated the bug in prose
("never calls is_session_token/require_static_token on THIS call
site"), and the new scope check reads the enclosing function's full source
text, docstring included. Stripping only that docstring (same guarded
function, same unguarded call, same two-function layout) gets it caught
immediately. This confirms the fix is real, not cosmetic — it matches the
shipped test test_a_guard_elsewhere_in_the_file_does_not_excuse_a_second_write.
New, narrower finding from adversarial probing (not a reopening of B-1):
the underlying signal is still "does one of six magic strings appear
anywhere in the enclosing function's rendered text" — not "is one of them
actually called." A comment or docstring inside the same function that
merely names a guard (# is_session_token check happens elsewhere) still
silences both Check 1 and Check 4 (they share _scope_is_sentinel_aware).
Blast radius is now one function instead of a whole file, which is the
fix that mattered, but this residual mechanism is untested and undisclosed
in the script's own "Known limits" section. Suggest a follow-up issue, not
a merge blocker.
2. Check 2/4 direct-construction gap (KeboolaClient(url, project.token) bypassing make_client_factory)
The literal scenario I named: CLOSED, robustly. New Check 4
(_unguarded_project_clients) catches every realistic authorship shape I
tried, verified two ways: an isolated AST harness, and — for full fidelity
— a copy of the real tree with new services/*.py files added, run through
the literal python scripts/check_sentinel_guards.py that make check-sentinel-guards invokes:
- direct construction (
KeboolaClient(project.stack_url, project.token)) → caught, exit 1 - aliased import (
from ..client import KeboolaClient as _StorageClient) → caught, exit 1 - module-qualified call (
from .. import clientthenclient.KeboolaClient(...)) → caught, exit 1 - factory indirection (helper
_build_client(stack_url, token)receiving the token as a parameter, notproject.tokeninline) → caught, exit 1
All four report the correct file:line (in <function>) and the script exits
1 with the expected remediation message. make_client_factory's own
bearer/static branches, the SDK's lib.py:253, and ManageClient all stay
green, as before.
But: PARTIALLY CLOSED overall — I found a real, concrete gap one level down
the same hierarchy. PROJECT_CREDENTIAL_CLIENTS = {"KeboolaClient"} seeds
Check 4's protected set with the composed class and walks its
descendants. But the vulnerable code — headers["X-StorageApi-Token"] = token whenever http_auth is None — actually lives in _CoreClient.__init__
(client/_core.py:28), which is an ancestor of KeboolaClient, not a
descendant. _CoreClient is already named explicitly in BEARER_CAPABLE_CLIENTS
by this same script ("the supported bearer path" — Check 2's own comment
even says "the composed KeboolaClient all sit under _CoreClient"), so
the script's author already reasons about this class elsewhere in the same
file. Verified against the real tree:
# services/report_service_core_client.py — planted in a full copy of the real tree
from ..client._core import _CoreClient
def fetch_usage_report(project):
client = _CoreClient(stack_url=project.stack_url, token=project.token)
...→ python scripts/check_sentinel_guards.py prints OK, exit 0.
Same result constructing any of the ten endpoint-family mixins directly
(tried _StorageTablesMixin, same _CoreClient.__init__ inherited
unmodified) — also clean. None of this is tested by the new test suite
either (_CoreClient only appears in a Check-2 fixture, never in a Check-4
one). This is a materially different way to put the exact same
kbc-session://{id} sentinel on the wire as a literal X-StorageApi-Token
header — the precise failure mode B-1 was about — just reached through a
class one step up the family instead of through KeboolaClient by name.
Suggested minimal fix: seed PROJECT_CREDENTIAL_CLIENTS = {"KeboolaClient", "_CoreClient"} (mirroring how BEARER_CAPABLE_CLIENTS already seeds
_CoreClient for Check 2), so _descendants_of naturally pulls in every
mixin and any future subclass as a protected construction site too.
Confirmed accurate, not a new finding: the script's own disclosed
"Known limits" — a local variable bound to the class (ctor = KeboolaClient; ctor(...)) and a bare module-level reassignment (_Aliased = KeboolaClient)
— both still slip through, exactly as documented. Not counted against the
verdict since it's honestly disclosed rather than hidden.
3. Does the new test file pin the gate's behavior, or only happy paths?
Not merely happy-path — genuinely adversarial. All 28 tests in
tests/test_check_sentinel_guards.py pass (uv run pytest tests/test_check_sentinel_guards.py -q → 28 passed). It explicitly
encodes fixtures matching my original finding's language almost verbatim
(test_a_guard_elsewhere_in_the_file_does_not_excuse_a_second_write,
test_every_credential_passing_shape_is_detected parametrized over
keyword/**kwargs/model-object/positional shapes) plus the new Check 4
matrix (aliased import, subclass indirection, bearer-branch exemption,
ManageClient exemption) and Check 2's chain-walking fix.
Coverage gaps found: (a) no test for the module-qualified-call shape for
Check 4 (works today, but a regression there would go unnoticed); (b) the
_CoreClient/mixin ancestor gap above has zero coverage, positive or
documented; (c) the disclosed local-binding / indirection limitations are
prose in a docstring, not pinned by any test in either direction.
No new false positives found
Re-confirmed the real tree is clean (make check-sentinel-guards → OK,
exit 0; test_live_tree_is_clean passes) and traced the real
allow_credential_type_change=True call site
(services/project_service.py:299) and the real make_client_factory
branches (services/base.py:128,150,165) against the new logic — all
still correctly recognized as guarded, by design (an explicit
allow_credential_type_change=True keyword is itself the intended
self-documenting escape hatch). No legitimate call site is newly flagged.
Net: **kwargs invisibility and the file-wide exemption are genuinely
fixed. The direct-construction gap is fixed for KeboolaClient by name
across every realistic authorship shape, but the same vulnerability is
still reachable via _CoreClient or any of its ten mixins directly — a
concrete, reproduced, real-tree-verified gap, not a synthetic edge case.
I'd keep this open rather than clear it outright; the fix is a one-line
seed-set change ({"KeboolaClient", "_CoreClient"}) plus a test, not a redesign.
|
@zajca — I pushed one commit to this branch ( Pushed: the last piece of the sentinel-gate blocker
Check 4 resolved its targets with Seeded the ancestor instead, plus two tests ( Everything else you fixed I re-verified and it holds — details in the two review comments above. Left for you:
|
… of the CLI The set of command groups that refuse a session project was documented only in the plugin skill and the error-code catalogue, so a user discovered each restriction reactively, one failed command at a time. `auth login --register-projects`, the post-login hook and `auth register-projects` now print that list once something was registered, and ship it as `session_unsupported_features` in `--json`. `SESSION_UNSUPPORTED_FEATURES` is single-sourced and derived by enumerating the real `require_static_token` call sites, which corrected the hand-written list twice over: `dev-portal` was on it despite having no guard at all (it authenticates with its own identity, never a project token), while the Scheduler Service (`flow schedule`, `flow schedule-remove`) and `sharing` without a master token were missing. `auth_register_projects` held the selection-mode orchestration -- which selector wins, whether to introspect at all, and assembling the selection list -- in the command layer. That now lives behind `AuthService.register_projects`, with only the usage check and the terminal-capability check left in the command, plus the interactive branch that needs the candidate list in hand. `--project-id` still costs exactly one introspect and an unknown id still raises before any write; both are asserted. The move pushed `auth_service.py` past its soft ceiling, so the alias/candidate logic is extracted to `_auth_registration.py` exactly as the review's nit 2 names it, re-exported so no importer needed touching (946 -> 770 plus 397). Server-controlled strings reaching a Rich console are escaped, following the `commands/config.py` precedent: a project name is settable by anyone with rename rights on a shared project, and `[link=...]` in one rendered as a clickable, deceptively-labelled hyperlink in another admin's terminal during their own `auth status`. `CheckboxItem.label` and the `typer.prompt` text are deliberately left unescaped -- `FormattedTextControl` takes style/text tuples and click echoes verbatim, so escaping there would print literal backslashes. Also: two bare tuple returns become frozen dataclasses, the two assigned no-op lambdas become named functions, `login` / `logout` use `AuthClient` as a context manager with the provider-registry reset kept in its own `try/finally`, and the login help text points at a public command instead of a private symbol. Reviewed-in: review-pr-535-implementation.md [N1] [N6] [N7] [N15] [N3] nits 3, 7, 9 (D1, D3)
… code `SESSION_EXPIRED` and `SESSION_NOT_FOUND` are raised as `KeboolaApiError`, so the REST surface answered 502 for both -- wrong on the facts, since nothing upstream failed, and unhelpful, since a remote caller cannot fix it. Both now answer 401 with a remedy naming `kbagent auth login` on the host, because a browser login only completes where a human sits. Every other `KeboolaApiError` keeps 502; the branch keys on the error code, not on an isinstance check. `_config_error_handler` hardcoded `CONFIG_ERROR` and so discarded the code of any subclass, which meant the sentinel guard reported a generic config problem over HTTP and a caller could not tell "this path does not take browser-login projects" from anything else. It now preserves a subclass's code while keeping the status at 400, the analogue of exit 5 on the CLI. This is narrow by construction: `SessionAuthUnsupportedError` is the only `ConfigError` subclass in the repo, and plain `ConfigError` carries no `error_code` at all. The JSON envelope is unchanged. `server/dependencies.py` records why there is no sentinel guard in `server/` -- serve inherits both bearer support and the fail-fast guards purely by delegating -- and the accepted risk that follows from supporting session projects there. Reviewed-in: review-pr-535-implementation.md [B6] (D1, D5)
`grep -c auth Makefile` returned 0: the 404-line `tests/test_e2e_auth.py` ran nowhere, because `make test` excludes it via the `e2e` marker and `make test-e2e` selected other files. The bearer path this feature introduces therefore had no automated verification in any target a human or CI invokes, which CONTRIBUTING requires per command. It now has its own `test-e2e-auth` target following the existing per-feature pattern, an `e2e_auth` marker, and inclusion in the default `make test-e2e`, which is what makes it a gate. Verified safe: with the session env vars unset all eight tests skip cleanly and `make test-e2e` still exits 0. Provisioning `E2E_SESSION_REFRESH_TOKEN` / `E2E_SESSION_PROJECT_ID` into CI remains an ops task -- the wiring exists, the credential does not. Also corrects a cross-reference to a test file that does not exist. Reviewed-in: review-pr-535-implementation.md [B3] nit 4 (D4)
There was no user-facing auth documentation: README's Setup listed three ways to register a project and browser login is the fourth and, for a human, the first to try, while the only auth document shipped was the internal design plan. `docs/auth.md` covers the flow, the two credential models, why no session -> static fallback exists or is constructible, the capability matrix, the errors and what to do about them, and the accepted risk of serving a session project. The human-at-a-browser requirement leads the page. Two facts were wrong across most surfaces and are corrected against the code rather than the prose. `kbagent serve` does support session projects -- it reaches Storage and Manage by delegating to the same already-guarded services -- which contradicted CLAUDE.md, the plan and the exception docstring. And `dev-portal` was listed as refusing session projects although it has no guard at all, authenticating with its own identity, while the Scheduler Service and `sharing` without a master token were genuinely restricted and named nowhere. Each surface now defers to `SESSION_UNSUPPORTED_FEATURES` instead of restating the list by hand, which is what let it rot in seven files at once. The plugin surfaces convention #17 lists are updated for this feature and for the behaviour added alongside it: the session skip in `project refresh`, the warn-and-allow conversion in `project edit --token`, the `Auth` / `auth_mode` output, the preserved per-project `error_code`, and the refresh timeout's exit code. `keboola-expert.md` was 135 B under its 60 KiB cap; stale per-command rows were collapsed to per-group ones as CONTRIBUTING directs, leaving 1.1 KiB. SKILL.md's description had zero headroom against its own 1024-char limit, so prose was tightened to fit the new auth triggers. Reviewed-in: review-pr-535-implementation.md [N9] [N10] [N11] [N12] nits 1, 11, 13 (D1, D8)
`session_unsupported_features` is a field on `LoginResult` and `RegisterProjectsResult` only, and `_render_session_restrictions` has exactly two call sites, so `auth status` neither prints the list nor carries it in `--json`. The guide claimed it did and omitted `auth register-projects`, which does. `config new` is also flag-dependent rather than uniformly broken on a session project: the scaffold that reaches the AI Service is skipped only for `--push --no-files` (`commands/config.py:1335`), so `--no-validate` is irrelevant to the default form, while `--push --no-files` stays on the Storage path as long as validation does not fire.
…in class `auth logout --remove-projects` deletes config.json project entries -- the same observable effect as `project remove`, which is registered `admin` -- yet the whole `auth.logout` operation sat in the `write` class. A policy denying `cli:admin` to keep an agent out of the project registry therefore still let it de-register projects through `auth`. The registry comment also claimed the operation was "same risk class as `project add`" while assigning `write`. The bare logout stays `write`: ending your own session is not an admin act, and escalating it would block a legitimate logout for an agent that only has write. The flag is escalated separately through a new `FLAG_ESCALATIONS` map, which `_matches_pattern` consults ahead of the registry. It is deliberately a separate map rather than an extra registry key, because `check_command_sync` requires OPERATION_REGISTRY to hold exactly one key per live command and rejects any key matching none. `permissions list` surfaces the escalation as its own row so the higher class is visible. `auth register-projects` is left at `write` as D7 specifies: it writes only session sentinels, never a pasted credential. The CLI-level test writes the policy into config.json directly, because `permissions set` requires a human to type a confirmation code at a real terminal and has no bypass. Reviewed-in: review-pr-535-implementation.md [N8] (D7)
Two problems with guarding at the factory. The claim that the lazy import in `make_client_factory` kept `filelock` off the static-token startup path was false: the module-level `from ..auth.sentinel import ...` executes `auth/__init__.py`, which re-exported `state_store` and so imported `filelock` in every process that merely touched a sentinel helper. And the guard was opt-in per call site, so a constructor nobody remembered stayed unguarded -- `client/stream.py` was the standing evidence, building a `StreamClient` with neither a guard nor the bearer hook, which in session mode sends an empty `X-StorageApi-Token` and draws an opaque 401. `auth/__init__.py` now re-exports nothing, so importing a submodule costs only that submodule: `filelock` and `auth.state_store` are absent from `sys.modules` after importing `services.base`, verified. `BaseHttpClient` gains `SESSION_AUTH_FEATURE`; a subclass that names it rejects a sentinel on construction, so the check cannot be skipped by a caller who forgot it. The five static-token-only clients declare it and the eight now-redundant factory guards are gone. `KeboolaClient`, `ManageClient` and `AuthClient` leave it `None` because they reach Storage and Manage over bearer, and `DeveloperPortalClient` leaves it `None` because it authenticates with its own identity and never sees a project token -- guarding it would break a working command, which the prose around this feature repeatedly got wrong. The six remaining `require_static_token` calls are not client constructions and stay: the MCP subprocess env, the sharing master-token path, kai's pre-flight, `semantic-layer token --encrypt`, the SDK entry point, and the static-token Storage factory itself. `client/stream.py` now propagates `http_auth`, and `StreamClient` accepts it. Both new guarantees are pinned by tests that were mutation-checked: removing the propagation fails the sub-client test, and disabling the constructor check fails the rejection test. Reviewed-in: review-pr-535-implementation.md [N4] [N5] (D2)
"The seventeenth service forgets" is a CI problem, not a review problem. Two of this release's blocking findings were channel-B misuse -- a static token written over a `kbc-session://` sentinel -- and grepping for a missing `require_static_token` call would never have found either, because they were writes, not reads. A third, `dev-portal` being documented as session-restricted although it has no guard at all, survived seven documentation surfaces. `make check-sentinel-guards` closes all three, following the `check-error-codes` / `command-sync-check` precedent, and runs inside `make check`. It parses with `ast` rather than grepping: a broad "does this file mention a token" search flags every service that correctly hands `project.token` to its injected bearer-aware factory, which is noise. The three checks: a caller of `add_project` / `edit_project` that passes a token must be sentinel-aware; a `BaseHttpClient` subclass must either declare `SESSION_AUTH_FEATURE` or be recorded as bearer-capable with a reason, so "nobody decided" stops being a valid state; and every `require_static_token` feature string must appear in `SESSION_UNSUPPORTED_FEATURES`, which is what `auth login` discloses and what every doc surface now defers to instead of restating the list by hand. Verified by mutation, not by a passing run: removing `StreamClient`'s `SESSION_AUTH_FEATURE` makes the gate fail with that file and line. Reviewed-in: review-pr-535-implementation.md, "Plus a CI gate" (D2)
…link `put_session` replaces the whole per-stack row, and login built the new session with an empty `orphaned_session_ids`. A third login therefore dropped the orphan recorded by the second one: a session still live on the server that no `auth logout` could reach, while login's own warning promised logout would retry it. The list is now seeded from the previous session. The two existing tests could not catch this -- both start from a session with no orphans, so `orphaned_session_ids == []` cannot tell "preserved" from "cleared". The new tests start from a session that already holds an orphan, and drive three consecutive logins. `verificationUriComplete` is server-supplied and went straight to the browser opener, which honours `file://`, a registered custom scheme, or a leading `-` read as a flag by the underlying command. It is now held to the `https://` rule `normalize_stack_url` already applies to the stack URL. A rejected value is reported rather than dropped in silence: the login still completes from the printed URI + user code, so the only thing lost is the convenience open. Also documents that `auth logout` forgets an orphan it could not revoke, which is the last place such a session is reported.
…rkers
`AUTH_REFRESH_MAX_WALL_CLOCK` was a sum of the httpx phase timeouts,
which is not a bound on anything: httpx applies `read` and `write` per
I/O operation and offers no total-duration option, so a server trickling
a response stays inside them indefinitely. Because that call happens with
`auth.json.lock` held, an unbounded hold outlasts the `AUTH_LOCK_TIMEOUT`
every other process waits, and a merely slow auth service is reported as
a stuck lock -- a `ConfigError` at exit 5, blaming the wrong thing. The
ceiling is now enforced where the lock is held: the request runs on a
daemon thread and is abandoned at the deadline, and unwinding closes the
client under it so the socket is aborted rather than left dangling.
Persistence deliberately stays on the lock-holding thread. A rotation
that lands after the deadline is discarded instead of written with no
lock held; the resulting one-generation-stale refresh token is the same
state a lost response leaves behind, which the server's idempotent grace
window exists to forgive.
The bare `"refresh token"` rejection marker subsumed the three scoped
markers and re-opened the hole `_is_rejected_grant`'s own docstring
forbids: a 400 validation error naming the field in prose ("The refresh
token must be a string.") was classified `SESSION_EXPIRED`, and the
provider purges `auth.json` on that code -- an avoidable re-login on a
credential the server never rejected. Every remaining marker states a
verdict on the token rather than naming it. Production's real shape (401
with no `invalid_grant`) is untouched: a 401 needs no marker at all.
The sentinel gate had four blind spots and no tests of its own, while the
sibling gate it was modelled on documents the convention that a checker
must prove it detects each drift class on synthetic inputs.
Check 1 keyed on a literal `token=` keyword. That matched exactly one of
the four shapes this codebase uses: `add_project(alias, ProjectConfig(...))`
carries the credential inside a model object, `edit_project(alias, **updates)`
hides it in a dict unpacking (`kw.arg` is None), and a positional argument
has no keyword at all -- so the check never fired on the very call shape
`project_service` and `org_service` actually write with. Since the
credential-carrying shape cannot be told apart reliably, every writer call
now has to sit in a scope that proves sentinel awareness.
That exemption was also file-wide: any file mentioning a sentinel helper
anywhere in its text was skipped whole, so a file that guarded one write
correctly excused a second, unguarded one. It is now scoped to the
enclosing functions, with outer scopes counting too -- deciding before
returning a closure is the correct pattern, not a miss. Call sites that
are safe for a reason outside their own function move into an explicit
map that records the reason.
New Check 4 walks Storage-client construction sites. `SESSION_AUTH_FEATURE`
is structurally unable to cover this: the clients that legitimately speak
bearer leave it unset, so a direct `KeboolaClient(stack_url, project.token)`
that skips `make_client_factory` would compile, pass every gate, and put
`kbc-session://{id}` on the wire as an `X-StorageApi-Token` header.
`ManageClient` stays out of scope -- a manage token is never a sentinel.
The checks take a root path so they can run against synthetic trees.
…quence The bearer capability matrix builds `AuthClient` / `SessionTokenProvider` / `KeboolaClient` directly, so none of the four new commands had E2E coverage of its own -- CLI-layer coverage stopped at mocked `CliRunner` tests. `auth status` is the one of the four that needs neither a browser nor a terminal, so it is the one that can close that gap unattended: it exercises `--config-dir` resolution, `AuthStateStore.from_config_store`, a real proactive refresh, introspection, the exit-code mapping and the `--json` envelope. `login` needs a human, `logout` would destroy the credential the suite shares, and `register-projects` writes config.json. These tests skip without session credentials like the rest of the file, so they have not yet run against a live stack; the non-network path (missing session -> exit 3) was verified locally. The changelog recorded that credentials embedded in a stack URL are dropped, but not what a user with one would observe: basic auth stops being sent and the stack answers an opaque 401.
main released 0.77.0 for config --change-description (#542) while this PR was in review, so its own 0.77.0 tag collides. Bumps pyproject.toml, adds a distinct 0.78.0 changelog entry (0.77.0 stays main's), and rewrites every auth-owned version reference across docs, the plugin skill/agent surfaces, and tests. Config --change-description references (#505) are left at 0.77.0 since that shipped separately on main.
Both the method docstring and the test fake claimed that unwinding past `with self._build_client()` closes the client and thereby aborts the in-flight socket. Measured against httpx 0.28.1, `Client.close()` returns immediately and leaves a request already in flight running: the worker outlives the call until its own per-phase timeout fires. The property the ceiling exists for is unaffected -- the caller unwinds and releases `auth.json.lock` on time, which is what the tests assert -- but the consequence is now stated instead of wished away, including that a long-running `kbagent serve` can park several such workers against an unresponsive auth service. This is the same class of mistake as the finding that produced the ceiling: asserting behaviour of a third-party library without measuring it.
Holding `auth.json.lock` across the refresh request enforced the real invariant -- a refresh token is presented to the server by at most one request at a time, since a second concurrent presentation is the replay that triggers family revocation -- but it did so at the cost of an unbounded hold, and the wall-clock ceiling that bounded the hold broke the invariant it was protecting: once the request was abandoned the lock came free while the token was still travelling, so the next attempt could present it concurrently. Cross-process serialisation is now a lease recorded in `auth.json`. The file lock is taken only for local reads and writes. The holder refreshes; anyone else polls until the holder persists a rotated pair, which the existing step-5 check adopts, or gives up with a truthful "another process is refreshing" error. A lease expires on its own, so a crashed holder self-heals. An abandoned request keeps its claim, extended by AUTH_REFRESH_ABANDON_GRACE, because the token may still be in flight; a request that completed, successfully or not, releases the lease at once. `_AbandonedRefresh` is what tells those two apart. `refresh_leases` is a sibling of `sessions` rather than a field on `StackSession`: a session write replaces the whole per-stack row, so a lease inside it could be dropped by an unrelated `put_session`. `delete_session` drops the lease on purpose, so a fresh login never waits out a claim nobody can release. The design record described the superseded behaviour and claimed the sum of the httpx phase timeouts was a bound; both are corrected there.
The three literal markers only matched the phrasings someone happened to write down. "The refresh token has expired.", "Refresh token revoked." and "Refresh token family revoked." -- the last being the exact server-side consequence this subsystem exists to avoid -- all fell through to the generic mapping, which means the dead session is never purged and every later command repeats one opaque error forever. A 400 now counts when it carries `invalid_grant`, or when it names the refresh token AND passes a verdict on it. Requiring both halves keeps the distinction the previous fix was for: "The refresh token must be a string." names the field and passes no verdict, so it stays a malformed request rather than a reason to delete a valid credential.
Three demonstrated bypasses, each an ordinary naming choice rather than an attempt to evade the gate: Check 1 recognised a config store only if the receiver's spelling contained "store", so `self._cfg = config_store` made byte-identical unsafe logic invisible. The holder is now resolved from a parameter annotated `ConfigStore`, from a direct construction, or from a parameter whose own name gives it away -- the last so the check does not depend on an annotation being present. Check 4 compared the constructor's spelling at the call site, so `KeboolaClient as _Storage` hid the construction, and a subclass hid it too. Import aliases are now mapped back to their origin and any descendant of a project-credential client counts. Check 2 read only the immediate base class, so a client two steps down the chain was never evaluated at all. It now walks the full chain -- and inherits the decision as well as the behaviour, since everything under a bearer-capable entry is already settled.
Clearing the stack's refresh lease in `delete_session` was written as an unconditional save, so a delete that removed nothing still wrote the file. On a machine that never used browser login, `auth logout` created an empty `auth.json` -- a side effect nobody asked for, and one that makes the file's mere existence a misleading signal about how this machine authenticates. The write now happens only when a session or a lease was actually dropped. A lease without a session is still cleared, even though the return value stays False: that is what keeps a fresh login from waiting out a claim nobody is left to release.
Two findings from the round-3 review, both reproduced before being fixed. A lease crosses processes, so its expiry is a wall-clock instant -- and the clock that wrote it may have been wrong. A claim persisted by a host whose clock ran an hour fast was honoured by every later, correctly-clocked process for that whole hour, so a holder that crashed locked the stack out of every command with no recovery short of `auth logout` and a full re-login. That defeated the exact self-healing property the TTL exists for. A reader now refuses an expiry further out than any TTL this code grants: such a value cannot come from a correct clock. The opposite skew cannot be detected from the payload and is documented as the residual. The 400 classifier asked for a subject and a verdict anywhere in the message, but "invalid" and "unknown" describe a malformed field as naturally as a dead credential: "the refresh token field contains an invalid character" and "refreshToken field is invalid: value must not exceed 512 characters" were both classified as rejected grants, and the provider deletes the session on that -- turning a bug on our own side into a forced re-login. A request-shape marker now vetoes the classification, because the two mistakes are not symmetric. The gate's docstring implied coverage it does not have; its three static limits are now stated, along with why the runtime guards are the actual protection.
main shipped 0.78.0, 0.79.0 and the unreleased 0.80.0 while this PR was in review, so the branch's own 0.78.0 target collides with a release that has already gone out with different content. Moves the auth changelog entry to a distinct 0.81.0 key (main's 0.78.0/0.79.0/0.80.0 entries stay untouched), bumps pyproject.toml plus the version-synced plugin.json, marketplace.json and uv.lock, and rewrites every auth-owned version reference across docs, the plugin skill/agent surfaces and tests. Main-owned 0.78.0 references (deferred Windows self-update, UTF-8 --json, the file-size baseline) are left alone.
74da82d to
881ca79
Compare
…header Check 4 resolved its target classes with `_descendants_of`, which walks DOWN the hierarchy, but seeded it with `KeboolaClient` -- while the construction that actually puts a sentinel on the wire, `headers["X-StorageApi-Token"] = token`, lives in `_CoreClient.__init__`, an ANCESTOR of that leaf. `_CoreClient(url, project.token)` and each of the ten endpoint-family mixins therefore passed the gate clean while running the very `__init__` the check exists to protect. Seed the ancestor instead. `KeboolaClient` stays listed because it is the name a reader looks for here; it is now covered as a descendant. No call site in `src/` constructs `_CoreClient` or a mixin directly, so this widens coverage without flagging any existing code -- `test_live_tree_is_clean` still passes.
The abandon pause held the refresh lease for the full length of the server's grace window, measured from our own abandon rather than from the rotation the window is anchored to, so the earliest retry landed at or past the boundary -- where reuse stops being forgiven and the token family is revoked. Reading the server settles the trade-off the pause was hedging against: the refresh transaction opens with SELECT ... FOR UPDATE on the session row, so a genuinely concurrent pair is serialised and the second presentation is answered from the rotation cache. Waiting therefore protects nothing and only spends a budget that is stack-side (PROGRAMMATIC_AUTH_GRACE_PERIOD_SECONDS, floor 1 s), never reported on a response, and not restarted by a grace replay. The pause becomes a short anti-stampede hold, and the skew horizon is taken over both TTLs so shrinking one cannot push it below the other and reject the claims this code writes itself.
|
Direction 1, and the server settles the one thing you said you had no basis to pick on. I went and read Your only objection to the short grace does not apply — a concurrent pair is already serialised, by the server. The refresh transaction opens by locking the session row: $connection->beginTransaction();
// SELECT ... FOR UPDATE to serialize concurrent refresh requests on same session
$this->sessionRepository->lockForUpdate($sessionId);
Two further facts push the same way, both of which make the wait worse than you described:
Pushed as
Your secondary consequence resolves itself: the extended lease is now 17 s against a 20 s Two notes while I was in there, both outside what you raised:
|
A rotation that deadlocks on bi_programmaticSessions / bi_manageTokens is rolled back whole, so the submitted token is still the session's current one. The server reports it as 503 with the string code auth.token.refreshContention and a Retry-After, and asks the client to replay the request -- which refresh, being deliberately outside the retry loop, did not do. The failure surfaced as a plain error and the recovery was left to the next command. Replaying is safe here for the same reason a blanket retry is not: there is no rotated-out token to re-present, so no replay for the grace window to forgive. The signal is therefore the string code paired with the status, never the status alone -- a 503 from a proxy or from load shedding proves nothing about whether the rotation committed, and replaying blind past the grace window is what revokes the token family. Bounded to one replay, Retry-After clamped, inside the existing wall-clock ceiling and under the lease, so no other process can present the token in between. Exhausted contention stays a retryable API error and never SESSION_EXPIRED: a database lock is not a verdict on the credential and must not purge a live session.
|
Followed up on the
Keyed on the code paired with the status, never the status alone. A plain Details worth flagging:
|
Three defects on the same path, found in review of the contention replay. A body nested past the interpreter's recursion limit raises RecursionError, not ValueError -- malformed JSON and too-deep JSON defeat the decoder through different exception hierarchies, and only the first was caught. `Retry-After: nan` parses as a float and survives clamping, because every comparison against NaN is False, so min/max pass it through to a time.sleep that rejects it. Both escaped the refresh call as neither _AbandonedRefresh nor KeboolaApiError, which is the third defect: the lease holder keyed its release on those two types, so an exception outside them left a claim standing that no process could release, stalling every command against the stack until the TTL ran out. The release is now keyed on "not abandoned" -- only an abandoned request may still have the token in flight; anything else reached a verdict and must not keep the claim. Each new test was checked to fail without its fix.
…nable Two gaps from review of the replay path. The shared retry loop logs every decision it takes; this loop opts out of that infrastructure and logged nothing, so a stack seeing repeated rotation deadlocks left no trace distinguishing a replay from a 503 that was not contention from an exhausted give-up. Adds debug lines for all three, none carrying token material. The give-up message was the generically mapped one, and the generic extractor prefers an `exception` key over `message`. On the serializer that nests the string code under `exception.code` -- one of the three shapes this endpoint is known to answer with -- the user therefore read a JSON fragment and had nothing to act on. The message is now our own and identical across all three shapes, with the classification untouched and the mapped error kept as the cause. Also records why a worker paused between replay attempts finds its client closed: that is the intended end of an abandoned refresh, not a path needing rescue, since the calling thread has already extended the lease to stop a second presentation. Each new test was checked to fail without its fix.
The abandon-pause rationale claimed a prompt retry "holds for every value" the server's grace window may take. It does not, and the arithmetic was already on the page: the earliest a retry can land is AUTH_REFRESH_MAX_WALL_CLOCK plus the pause -- 17 s -- while the window's documented floor is 1 s. Recovery is reliable near the 30 s default and fails as a stack approaches that floor. No value of AUTH_REFRESH_ABANDON_GRACE closes the gap, because the ceiling alone already exceeds the floor and fires before we know there is anything to retry, so the text now names the ceiling as the lever and the pause as the minor term. A test pins that ordering, since the advice stops being true once the pause grows to rival the ceiling. Scope of the limit, also now stated: it is the abandon path only. A refresh that completes, successfully or not, releases the lease at once and the next command retries with no added delay.
|
Ran a review pass over both commits before handing them back to you — security, silent-failure and adversarial lenses, each verifying claims against the server source rather than my summary of it. Five defects came out of it, plus one overstated claim of mine. Three commits: Fixed: three defects on the contention-replay path (
|
2 participants
DMD-1825 feat(auth): browser login via PKCE + device authorization (programmatic auth)
What this changes
kbagentcan now authenticate against a Keboola stack through a browserlogin instead of a hand-copied static Storage token.
kbagent auth loginruns a PKCE authorization-code flow by default, falling back to the RFC 8628
device flow when a loopback browser is not reachable. The result is a
USER-scoped programmatic session (a
kbc_at_*access token plus akbc_rt_*refresh token) that the CLI refreshes on its own.Why: obtaining a static token today means opening the Keboola UI, minting a
token by hand, and pasting it into a terminal, where it lands in shell history
and stays valid until someone remembers to revoke it. A session is short-lived,
server-revocable, tied to the actual human, and never typed anywhere.
The static-token path is untouched. Both credential kinds coexist per project
inside one
config.json, and every existing command keeps working exactly asbefore for static projects.
Observable differences
project add --token)kbagent auth loginopens a browser; no token is ever typed or shownconfig.jsonheld a literal token per projectkbc-session://{project_id}sentinel inconfig.json; the live credential lives inauth.json(0600, sibling ofconfig.json)SESSION_EXPIREDkbagent auth statusreports live / expired / missing distinctly, refreshing an expired access token rather than reporting a false negativekbagent auth logoutrevokes the session server-side, reporting an unconfirmed revoke distinctly from a clean one, and always clears local statekbagent project listshows an auth-mode column, so a session project is visibly different from a static oneconfig.json's schema andCURRENT_CONFIG_VERSIONare unchanged -- asentinel is a value in the existing
tokenfield, not a new shape. Existingconfigs load untouched, and downgrading a session project to static works via
project edit --token(with a warning, see D6 below).Commands added
auth register-projectsdeserves its own note, because it fixes a usabilitybug rather than adding a nicety:
auth loginprinted a table of accessibleprojects but registered nothing unless
--register-projectswas passed,and the alias it offered was a slug of the project name -- so the numeric
project id shown in that table never resolved as
--project. The new commandlists every accessible project with a collision-free suggested alias and lets
the caller pick. Default mode is an arrow-key + spacebar checkbox picker;
--alland--project-idare the non-interactive selectors, and a non-TTY or--jsoncontext with neither fails fast instead of hanging on a prompt. Itnever overwrites an existing alias.
All four commands require a human at a browser and are unsuitable for
unattended automation; CI and headless callers keep using a static Storage
token. This is stated in the command help and in
docs/auth.md.v1 scope
Bearer sessions are wired through the Storage and Manage paths. Both the
CLI commands and
kbagent servereach them:servenever turns aProjectConfiginto credentials itself -- every service in its registryresolves its own client factory -- so the REST surface inherits the bearer
support and the fail-fast guards from the services it delegates to
(
server/dependencies.py).Consumers that do not support sessions fail fast with
AUTH_NOT_SUPPORTED_ON_STACKnaming the static-token fallback, rather thansending the sentinel string as a credential: the importable SDK (
lib.py), theMCP subprocess, and the AI / data-science / metastore / dev-portal / stream
clients.
Supporting session projects in
serveis a deliberate trade for web-UIusability, and it carries two consciously accepted properties, documented in
docs/web-server.md> "Session-registered projects":KBAGENT_SERVE_TOKENacts as thesigned-in Keboola user for as long as that session lives. The serve token is
not that user's Keboola identity and the REST surface has no second identity
layer to distinguish them.
for weeks the crash window between persisting a rotated pair and revoking the
previous one stays open far longer, and a crash inside it leaves a
server-side session no later
auth logoutcan revoke.A session that expires while
serveruns answers HTTP 401 witherror_code: SESSION_EXPIREDand a message namingkbagent auth loginonthe host -- a browser login only completes where a human sits, so the daemon
cannot recover on a REST caller's behalf.
Why the
serverouter is skippedPer CONTRIBUTING > "Adding a new command", a skipped REST surface needs an
explicit reason so reviewers do not flag it:
auth loginneeds a loopback HTTP redirect back to the machine runningthe browser. Exposing that over a remote REST call would either bind a
listener on the server's host for a browser that is somewhere else entirely,
or require proxying the callback -- neither is a sensible v1 surface.
auth logoutandauth register-projectsdefault to interactiveconfirmation and a terminal checkbox picker respectively.
register-projectsdoes have non-interactive selectors (
--all,--project-id), so it is themost plausible future addition of the three.
auth statusis genuinely not terminal-only and would map cleanly ontoa
GET. It is deferred, not rejected: a read-only endpoint reportingsession liveness is worth having, and the reason it is not in this PR is
scope, not design. Filed as follow-up work rather than smuggled in
untested.
New error codes:
AUTH_NOT_SUPPORTED_ON_STACK,AUTH_FLOW_TIMEOUT,AUTH_FLOW_DENIED,AUTH_FLOW_EXPIRED,AUTH_BROWSER_UNAVAILABLE,AUTH_STATE_MISMATCH,SESSION_EXPIRED,SESSION_NOT_FOUND.Security notes
auth.jsonstores tokens in plaintext at0600, a deliberate deviation fromthe RFC and consistent with how
config.jsonalready stores static tokens.The file is created with
os.open(..., 0o600)before any byte is written.Rationale in
docs/programmatic-auth-login-plan.mdsection 4.2.secrets.token_urlsafe(384-bit verifier, 256-bit state),S256only.
stateis compared withhmac.compare_digestbefore any otherbranching in the callback handler, so a forged
error=callback cannotbypass it. The loopback server binds
127.0.0.1/[::1]only, never0.0.0.0, and its access log is silenced so no code reaches stderr.filelock) onauth.json, because the existingconfig_storelock helper is a no-op onWindows and unserialized rotation there would trigger server-side family
revocation -- a hard logout.
refreshis a deliberate single attempt outside the shared retry loop: aretry re-presents the same refresh token, which is exactly the replay the
server's 30 s grace window forgives, and past that window it triggers family
revocation.
Review findings addressed
This PR incorporates a full implementation review (findings
B1-B7,N1-N15, and decisionsD1-D8). Resolved in this branch:B1/B2(D6) -- a session sentinel could be silently overwritten by astatic token via
project refresh,org setup --refreshandproject edit --token, orphaning the session inauth.jsonbeyond the reachof
logout. Guarded at one chokepoint inConfigStore.edit_project;project edit --tokenopts in explicitly and warns,org_serviceskipssession projects at the selection layer.
B3(D4) -- the bearer E2E suite ran in no Makefile target. Now wiredinto a new
make test-e2e-authand into the defaultmake test-e2e, with ane2e_authmarker; it skips cleanly without session credentials.B4-- the sentinel guard lostAUTH_NOT_SUPPORTED_ON_STACKinmulti-project paths, degrading to a generic error code.
B5-- refresh could holdauth.json.locklonger than other processeswait to acquire it, making a merely slow auth service look like a stuck lock.
Bounded by
AUTH_REFRESH_TIMEOUT;AUTH_REFRESH_MAX_WALL_CLOCKis derivedfrom it by summing the sequential phases so the two cannot drift, and a test
asserts
AUTH_REFRESH_MAX_WALL_CLOCK * 2 <= AUTH_LOCK_TIMEOUT.B6(D1) -- the v1 scope ofservewas self-contradictory: the codesupports session projects, three documents said it fails fast. The code is
the authority; the documents are corrected (see "v1 scope" above).
D5-- a runtime session expiry surfaced fromserveas HTTP 502, asthough an upstream service had failed. Now 401 with
SESSION_EXPIRED, mappedcentrally in the
server/app.pyexception handlers rather than per-router.D3(N3) -- an unused Manage-credential abstraction was deleted ratherthan shipped without a caller. It returns with its first real caller and a
test.
D8(N11) -- user-facing documentation: a newdocs/auth.mdplus aREADME setup bullet.
N13/N14--project listsurfaces an auth-mode column so a sessionproject is not silently indistinguishable from a static one.
Nits: stale test cross-reference,
keboola-expert.mdtrimmed back under itssize cap, and the plugin/doc surfaces below.
N4/N5(D2) -- the channel-A guard is now a property of the clientrather than something fourteen factories remember:
BaseHttpClientcarriesSESSION_AUTH_FEATURE, so a client that cannot speak bearer names the featureit is and construction on a sentinel fails fast.
DeveloperPortalClientdeliberately declares nothing (own identity, never a project token).
auth/__init__.pyre-exports nothing, sofilelockandauth.state_storestay unloaded on the static-token path -- verified with a
sys.modulescheck,since the previous docstring claimed this while the opposite was true. Fixes
client/stream.py, which built aStreamClientwith neither the guard norhttp_auth, in passing.N1/N6/N7/N15-- server-supplied strings are escaped beforereaching a Rich console (a project name containing
[link=...]rendered as aclickable hyperlink in another admin's terminal); selection-mode orchestration
moved from the command into
AuthService.register_projects; two bare tuplereturns became frozen dataclasses; and registration now discloses the v1
restriction list up front instead of leaving each limit to be discovered on
first failure.
N8(D7) --auth.logoutstayswrite, while--remove-projectsisescalated to the admin class through a new
FLAG_ESCALATIONSmap. It is aseparate map rather than an extra
OPERATION_REGISTRYkey becausecheck_command_syncrequires exactly one registry key per live command.make check-sentinel-guards(D2) -- a new CI gate, insidemake check,that rejects three kinds of drift: a credential write that is not
sentinel-aware, a
BaseHttpClientsubclass that neither declaresSESSION_AUTH_FEATUREnor is recorded as bearer-capable, and arequire_static_tokenguard missing fromSESSION_UNSUPPORTED_FEATURES. Itparses with
ast, not grep, so a service correctly handingproject.tokentoits bearer-aware factory is not flagged.
N2and nits 3 / 5 / 6 / 7 / 8 / 9 / 10 / 14 -- stack URLs canonicalizetheir host (two spellings of one stack no longer address two sessions, and
credentials in a URL are dropped rather than persisted), a bound-socket leak
and a
close()-hangs-forever hazard in the PKCE callback server, thewslviewprobe down from three spawns per login to one, and two testscorrected to claim only what they prove.
One factual correction the review itself had wrong
The review, and seven files that followed it, listed
dev-portalamong thesurfaces that refuse a session project. It has no guard at all -- it
authenticates with its own identity and never receives a project token -- while
the Scheduler Service (
flow schedule,flow schedule-remove) andsharingwithout a master token are real restrictions that were named nowhere.
SESSION_UNSUPPORTED_FEATURES(services/_auth_registration.py) is now thesingle source, every surface defers to it, and the new CI gate keeps it honest.
Second review round (
F1-F4,B-1,NB-1,NB-2)A security review of the token-handling core and an architecture/compliance
review landed after the first round's fixes. All seven findings were reproduced
against the code before being fixed, and each fix carries a test that was
verified to FAIL without it.
Two of the seven were introduced BY the first round's fixes and had therefore
never been reviewed by anything -- worth stating plainly, because it is a process
gap rather than a coding one: the fix wave ran implementers only, with no review
pass over its own output.
F1--auth loginsilently discarded previously recorded orphan sessionids.
put_sessionreplaces the whole per-stack row, and the new session wasbuilt with an empty
orphaned_session_ids, so a third login dropped the firstorphan: a session still live server-side that no
auth logoutcould everreach, while login's own warning promised logout would retry it. The list is
now carried forward from the previous session. Two tests pin it, one starting
from a session that already holds an orphan (so the assertion distinguishes
"preserved" from "was empty anyway" -- the flaw in the old test that let this
through), one driving three consecutive logins.
docs/auth.mdnow also statesthat
logoutforgets an orphan it could not revoke, which stays a documentedproperty rather than a code change.
F2--AUTH_REFRESH_MAX_WALL_CLOCKwas not a wall-clock bound. httpxapplies
read/writeper I/O operation and has no total-duration option, sosumming the phases described a hope. A server trickling a response could hold
auth.json.lockpast theAUTH_LOCK_TIMEOUTevery other process waits, makinga merely slow auth service look like a stuck lock (
ConfigError, exit 5) --precisely the failure
B5was raised to prevent. The ceiling is now enforcedin
SessionTokenProvider._refresh_within_budget, which is where the lock isheld: the request runs on a daemon thread, is abandoned at the deadline, and
unwinding closes the client under it to abort the socket. Persistence stays on
the lock-holding thread, so a rotation landing late is discarded rather than
written without the lock. Verified by removing the enforcement and watching the
test suite block for a full 20 s instead of failing in 1 s.
This finding came directly from a wrong instruction in the
B5fix briefing("derive the bound arithmetically from the constants"), which presupposed the
additive behaviour httpx does not have. The agent satisfied exactly the stated
criterion, including a test asserting the arithmetic.
Superseded by the third review round below. The ceiling is still enforced,
but the refresh no longer holds the file lock at all -- and the reason is that
this fix broke the invariant it was protecting. See
B-1 (round 3).F3-- the_GRANT_REJECTION_MARKERScatch-all could purge a validsession. The bare
"refresh token"substring subsumed the three scopedmarkers and re-opened the hole its own docstring forbids: a 400 validation
error naming the field in prose ("The refresh token must be a string.") was
classified
SESSION_EXPIRED, andSessionTokenProviderdeletes the session onthat code -- an avoidable re-login on a credential the server never rejected.
Entry dropped; every remaining marker states a verdict on the token, not just
its name. Production's real shape (401, no
invalid_grant) is unaffected: 401needs no marker.
F4--verificationUriCompletereached the browser opener unvalidated.A fully server-supplied value went to
webbrowser.open, which honoursfile://, a registered custom scheme, or a leading-read as a flag. It isnow held to the same
https://rulenormalize_stack_urlapplies to the stackURL, and a rejected value is reported rather than skipped in silence -- the
login still completes from the printed URI + code, so only the convenience open
is lost.
B-1-- the sentinel-guard CI gate had real blind spots and no tests.All confirmed and closed, plus a fourth the review did not name:
Check 1 keyed on a literal
token=keyword, butadd_project(alias, ProjectConfig(...))carries the credential inside a model object, so thecheck never matched that call shape at all. Check 1 now flags every
config-store writer call whose enclosing scope does not prove sentinel
awareness, which covers the keyword,
**kwargs, positional and model-objectshapes uniformly; the exemption is scoped to the enclosing function instead of
the whole file (a file that guarded one write no longer excuses a second); and
call-site-level exemptions moved into an explicit
CREDENTIAL_WRITE_ALLOWEDmap where each entry states its reason. A new Check 4 walks
KeboolaClientconstruction sites and flags any built from a project credential outside a
sentinel-aware scope -- the gap
SESSION_AUTH_FEATUREis structurally unableto close, since the bearer-capable clients leave it unset. The gate now has
tests/test_check_sentinel_guards.py(21 tests) asserting each drift class isdetected on synthetic trees, and a measured comparison confirms the previous
implementation missed all five.
NB-1--auth statusis now driven through the real CLI intests/test_e2e_auth.py; see Testing below for what that does and does notprove.
login/logout/register-projectsstay out, with reasons.NB-2-- every self-reported figure re-measured; see the next section.Two items from the security review's "not findings" list were also acted on: the
changelog now names the observable consequence of dropping
user:pass@userinfofrom a stack URL (basic auth stops being sent, the stack answers 401), and the
AuthClient.refreshdocstring no longer claims a bound it does not enforce.Third review round (
B-1,NB-1,NB-2,NB-3)Rebased onto
mainand retargeted at 0.78.0:mainreleased 0.77.0 forconfig --change-description(#542) while this PR was in review, so the numberwas taken.
changelog.pycarries both keys -- main's released0.77.0and thisfeature's
0.78.0.A security/architecture review of the second round's own fixes then found four
things. Every one was reproduced before being fixed, and each fix carries a test
verified to fail without it.
B-1-- the wall-clock ceiling broke the invariant it was protectingThe deepest of the four, and it invalidates part of
F2above. Holdingauth.json.lockacross the refresh request did enforce the real invariant -- arefresh token is presented to the server by at most one request at a time,
because a second concurrent presentation is the replay that triggers family
revocation, a hard logout. But the ceiling
F2added released that lock whilethe abandoned request was still in flight, so the next attempt (a retry in this
process, or the new process the error message invites) could read the un-rotated
token from
auth.jsonand present it concurrently. The ceiling and the invariantwere in direct conflict; the fix for a misleading
exit 5had opened a path to ahard logout.
Cross-process serialisation is now a refresh lease recorded in
auth.json:auth.json.lockis taken only for local reads and writes -- never across thenetwork call. This is what makes the unbounded hold structurally impossible
rather than merely bounded.
rotated pair (adopted by the existing step-5 re-read) or gives up with a
truthful "another kbagent process is refreshing".
expires_atmakes a crashed holder self-healing.AUTH_REFRESH_ABANDON_GRACE, because its token may still be travelling. Arequest that completed -- successfully or with a server error -- releases the
lease at once.
_AbandonedRefreshis what distinguishes the two.refresh_leasesis a sibling ofsessionsinAuthState, not a field onStackSession: a session write replaces the whole per-stack row, so a leaseinside it could be dropped by an unrelated
put_session-- the exact shape ofF1, applied as a lesson rather than repeated.The load-bearing test measures the property from inside the request (the fake
client probes
filelock.FileLock(timeout=0)while its own refresh is running);under the previous design it fails.
Accepted, newly observable cost: for
AUTH_REFRESH_ABANDON_GRACE(30 s)after an abandoned request, other commands against that stack wait and then
report contention. That is deliberate -- 30 s of waiting beats a hard logout --
but it is a real behaviour change, not an internal detail.
NB-1-- a 400 is classified by subject plus verdict, not by fixed phrasesThe three literal markers left from
F3only matched phrasings someone hadwritten down.
"The refresh token has expired.","Refresh token revoked."and"Refresh token family revoked."-- the last being the very server-sideconsequence this subsystem exists to avoid -- all fell through to the generic
mapping, which leaves the dead session unpurged so every later command repeats
one opaque error forever. A 400 now counts when it carries
invalid_grant, orwhen it names the refresh token AND passes a verdict on it. Both halves
are required, which preserves
F3's distinction:"The refresh token must be a string."names the field, passes no verdict, and stays a malformed requestrather than a reason to delete a valid credential.
NB-2/NB-3-- three demonstrated bypasses of the sentinel gateEach an ordinary naming choice, not an attempt to evade anything:
"store", so
self._cfg = config_storemade byte-identical unsafe logicinvisible. The holder is now resolved from a
ConfigStoreannotation, a directconstruction, or the parameter's own name -- the last so the check does not
depend on an annotation being present.
KeboolaClient as _Storagehid the construction, and so did a subclass. Importaliases are mapped back to their origin and any descendant counts.
chain was never evaluated at all. It now walks the full chain, and inherits the
decision along with the behaviour (everything under a bearer-capable entry is
already settled, which is why the Storage mixins are not findings).
The gate's own suite is up to 28 tests, and a measured old-versus-new comparison
confirms the previous implementation missed all three.
NB-4-- the design record described code that no longer existsdocs/programmatic-auth-login-plan.mdstill documented the lock-held-across-the-network algorithm and repeated the claim that the sum of the httpx phase timeouts
is a bound. Both corrected there, and the superseded passage is marked as such --
the same drift class as
B6in the first round, where three documents disagreedwith the code.
Fourth review round -- and two findings left open for this review
An independent review of the third round's own fixes found five things. Three are
fixed below; two are deliberately left open, because closing them is a design
decision that deserves more eyes than a single author -- they are stated here
rather than quietly carried.
Every fixed finding was reproduced first. That matters more than usual here,
because this PR has now shown the same pattern three rounds running: the fix for
one round's finding introduced the next round's.
B5(unbounded lock hold)F2-- the sum of httpx phases is not a boundF2(wall-clock ceiling)B-1-- lock released while the token was still in flightB-1(refresh lease)auth.jsonwritten by a no-op deleteFixed
function, 3 of 10 realistic messages were misclassified, all in the
destructive direction:
"The refresh token field contains an invalid character."and"refreshToken field is invalid: value must not exceed 512 characters."were read as rejected grants, and the provider deletes thesession on that -- turning a bug on our own side (a truncated or mis-encoded
value) into a forced browser re-login.
"invalid"and"unknown"describe amalformed field as naturally as a dead credential, and a substring test
cannot tell which noun the verdict attaches to. A request-shape marker
(
field,parameter,must be,exceed, ...) now vetoes theclassification, because the two mistakes are not symmetric: refusing to purge
leaves an error
auth loginclears, purging wrongly destroys a workingcredential. Now 0 of 12 misclassified.
processes, so its expiry has to be a wall-clock instant -- and the clock that
wrote it may have been wrong. Reproduced: a claim persisted by a host running
an hour fast was honoured by every later, correctly-clocked process for that
whole hour, so a holder that crashed blocked every command against the
stack with no recovery short of
auth logoutand a full re-login. Thatdefeats precisely the self-healing property the TTL exists for. A reader now
refuses an expiry further out than any TTL this code grants
(
AUTH_REFRESH_LEASE_MAX_HORIZON): such a value cannot come from a correctclock. The opposite skew cannot be detected from the payload and is documented
as the residual, in
RefreshLease.is_live.delete_sessionwrote an emptyauth.json. Clearing the stack'slease was written as an unconditional save, so a delete that removed nothing
still created the file on a machine that never used browser login. The write
now happens only when a session or a lease was actually dropped. Correction to
an earlier description of this: it is not reachable from
auth logout,which raises
SESSION_NOT_FOUNDfirst -- it was real at the API level andunreachable from any command.
Open:
AUTH_REFRESH_ABANDON_GRACEis a fixed guess at an unbounded durationWhen the ceiling abandons a refresh, the lease is extended by a flat 30 s so
nobody re-presents a token that may still be travelling. But the code's own
reasoning for needing a ceiling at all -- httpx applies
read/writeper I/Ooperation, so a trickling response is unbounded -- applies just as well to
defeating that 30 s. If the real request outlives claim + ceiling + grace
(~44 s), the lease frees up and a second process can present the same
never-rotated refresh token while the first is still on the wire: the family
revocation this redesign exists to prevent.
Not fixed here, deliberately. The clean fix is a watchdog holding the lease for
exactly as long as the worker thread lives, which is a third concurrency
addition to credential rotation in as many rounds, and the table above is the
argument for not making it unreviewed. One concrete trap is already mapped:
claim_refresh_leasepermits re-claiming one's own lease, so a watchdog wouldneed a holder id distinct from the provider's, or the same process would take the
lease straight back and fire the duplicate itself.
Whether the exposure is real also depends on how slow the auth service and
network path can plausibly be in production -- a fact about the server, not about
this repo.
Open: the sentinel gate does not see a
ConfigStorereached through indirectionself._cfg = provider.get_store()is not recognised as holding aConfigStore,so an unguarded
add_projecton it passes Check 1. Demonstrated on a synthetictree; absent from the repo today (every service takes a directly-typed,
conventionally-named constructor parameter). The same limit applies to a client
constructed through a local binding.
This cannot be fully closed by matching more syntactic shapes -- it is
AST-versus-dynamic-dispatch. The robust fix is a runtime assertion in
ConfigStore.add_project/edit_project, the chokepoint the gate's own commentnames, which is a change to the path every credential write in the CLI goes
through and wants its own review. What is done here instead: the gate's docstring
now states its three static limits rather than implying coverage it lacks, and
records that the runtime guards (
require_static_token,make_client_factory)are the actual protection -- the gate only stops a new unguarded path from landing
quietly.
Measured against repo budgets
Every figure below was re-measured with
wcagainst the current head, after thesecond review round's changes. An earlier revision of this section quoted three
numbers that were wrong (
NB-2), including akeboola-expert.mdsize that wasnever true at any commit and a byte cap that is not the one CI enforces -- so
treat this table as measured, not as narrated:
services/auth_service.pyservices/_auth_registration.pycommands/auth.pyscripts/check_sentinel_guards.pyauth/token_provider.pyplugins/kbagent/agents/keboola-expert.mdPROMPT_BYTE_BUDGETintests/test_agent_prompt.pyThe agent-prompt budget IS enforced, by
tests/test_agent_prompt.py-- theearlier claim that there is "no CI enforcement" of it was wrong too. The
constant is 62 000 B, not the 61 440 B (60 KiB) this section previously measured
against, leaving 1 084 B of headroom. Re-measured after every change in
all three review rounds, not carried forward.
Files touched beyond
src/andtests/Release checklist step 11 requires listing the plugin and documentation
surfaces, because none of them are covered by a CI freshness check and all of
them ship a working AI agent or a user-facing document:
CLAUDE.mdauthblock in "All CLI Commands"; v1 scope under D1README.mddocs/auth.mddocs/web-server.mdserve+ session projects, and the accepted risk under D1docs/error-codes.mddocs/sdk.mddocs/programmatic-auth-login-plan.mdsrc/keboola_agent_cli/commands/context.pyAGENT_CONTEXT, read bykbagent contextplugins/kbagent/.claude-plugin/CLAUDE.mdplugins/kbagent/.claude-plugin/plugin.jsonplugins/kbagent/agents/keboola-expert.mdplugins/kbagent/skills/kbagent/SKILL.mdplugins/kbagent/skills/kbagent/references/auth-workflow.mdplugins/kbagent/skills/kbagent/references/commands-reference.mdplugins/kbagent/skills/kbagent/references/gotchas.md(since v0.78.0)entries.gitignore.cache/directory and root-anchored per-PR review write-upsMakefilecheck-sentinel-guards+test-e2e-authtargetsscripts/check_sentinel_guards.pyB-1)src/keboola_agent_cli/changelog.pyThat is 19 files. The count grew twice: the first correction wave added
docs/auth.md,docs/web-server.md,docs/sdk.md,plugins/kbagent/.claude-plugin/CLAUDE.md,README.md,plugin.json,docs/programmatic-auth-login-plan.mdand.gitignore; the second added theMakefile, the new CI gate script andchangelog.py.Testing
New test modules:
test_auth_client.py,test_auth_device.py,test_auth_environment.py,test_auth_models.py,test_auth_picker.py,test_auth_pkce.py,test_auth_register_projects.py,test_auth_sentinel.py,test_auth_sentinel_guards.py,test_auth_service.py,test_auth_state_store.py,test_check_sentinel_guards.py,test_checkbox_select.py,test_cli_auth.py,test_e2e_auth.py,test_http_base_auth.py,test_token_provider.py.(
multiprocessing.get_context("spawn")), including a delayed-stale-writercase, rather than simulating concurrency in-process.
tests/test_e2e_auth.pyis the recorded bearer capability matrix: onereal call per directly called service (Storage, Queue, Query, Encryption,
Sync Actions), a reactive-401-refresh test, and a Manage call, all against a
feature-flagged stack. It needs
E2E_URL,E2E_SESSION_REFRESH_TOKENandE2E_SESSION_PROJECT_IDand skips cleanly without them. The device flow is adocumented, deliberately skipped manual runbook -- approving a device
authorization requires a human in a browser and cannot be automated honestly.
auth statusis additionally driven through the real CLI (CliRunneroverkbagent --json --config-dir ... auth status), so one of the four new commandshas genuine CLI-layer E2E coverage rather than only mocked coverage (
NB-1).tests/test_check_sentinel_guards.pytests the CI gate itself on syntheticfixture trees -- each of the five drift classes it now detects is asserted to be
detected, following the convention
tests/test_check_command_sync.pystates forits sibling gate.
process experiences it: an independent
filelock.FileLock(timeout=0)probe mustacquire
auth.json.lockafter a stalled refresh gives up.make checkpasses: lint, format,tytypecheck, SKILL.md freshness, versionsync, command-sync, changelog, error codes, the new sentinel-guard gate, and
5227 passing unit tests (8 skipped, exit 0).
tyreports 3 diagnostics, allpre-existing and in files this PR does not touch (
scripts/hatch_build.py,tests/test_mcp_deprecation_warnings.py,tests/test_tool_call_permissions.py).tests/test_e2e_auth.pyskips cleanly with no session credentials, so itsinclusion in the default
make test-e2edoes not break that target.Provisioning
E2E_SESSION_REFRESH_TOKEN/E2E_SESSION_PROJECT_IDinto the CIsecret store is a separate ops task: the wiring exists, the credential does not,
so the eleven bearer E2E tests skip in CI until then -- including the three new
auth statusCLI tests, which have therefore never been executed against a livestack. Their non-network path (missing session -> exit 3, enveloped
--json)was verified locally; the live-session assertions have not been.
Notes for the reviewer
docs/programmatic-auth-login-plan) no longer matches thecontent. CONTRIBUTING mandates nothing about branch naming and renaming would
lose the PR, so it stays.
docs/programmatic-auth-login-plan.mdis a design record, not currentdocumentation. It is kept for the rationale behind the deviations (plaintext
auth.json, single-attempt refresh, thefilelockdependency); its stalescope claims have been corrected, but read
docs/auth.mdfor currentbehaviour.