Repository navigation
openspec: propose support-user-apikey - #78
Merged
Merged
Conversation
Add proposal/design/specs/tasks for optional auth mode with users + API keys. Default-off for backward compat; preview sharing preserved via separate --protect-previews switch.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds optional API-key authentication, preview protection, owner-scoped session access, CLI user/key management, credential propagation for ChangesAPI Key Auth and Owner Scoping
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Server
participant Store
CLI->>Server: Request with Authorization: Bearer API key
Server->>Store: Verify API key
Store-->>Server: User identity
Server->>Store: Check ownership or query owned sessions
Store-->>Server: Authorization result
Server-->>CLI: Return session response or auth error
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openspec/changes/support-user-apikey/design.md`:
- Around line 45-47: The owner-check isolation currently covers
list/search/peers but misses download access, allowing cross-user reads when
authentication is enabled. Update the session ownership validation path to
include handleDownload alongside the other read endpoints, using the same SELECT
user_id FROM sessions WHERE session_id=? check and returning 403 on mismatch so
authenticated users cannot download another user’s session by ID.
In `@openspec/changes/support-user-apikey/specs/user-auth/spec.md`:
- Around line 81-89: Clarify the interaction between `--protect-previews` and
the existing auth flow in the user-auth spec: explicitly state whether preview
protection is ignored when `--auth` is off or whether `GET /s/<id>/...` and `GET
/s/<id>/ws` still require API key validation in that mode. Update the
preview-auth scenarios around `--protect-previews` and the `--auth`-disabled
behavior so the rule is unambiguous and consistent with the `user-auth` spec.
- Around line 29-30: The api_keys schema description is missing the per-record
salt and hash algorithm version needed for stable verification and future
migration. Update the user-auth spec entry for api_keys to explicitly include
salt and hash_algo alongside the existing key identifier, hash value, user id,
created time, and optional revocation flag, and make sure the wording still
states that only the one-way hash is persisted. Use the api_keys table
definition in the spec to keep the fields and validation rules aligned.
- Around line 113-125: Clarify and enforce how `sth user revoke-key` resolves
`<key-prefix|id>` in the user-auth spec: in the `revoke-key` flow, detect when a
provided prefix matches multiple keys and fail closed instead of revoking an
arbitrary one. Update the `sth user revoke-key` scenario/requirements to state
that only a unique prefix or exact id is accepted, and otherwise the command
must return an error prompting for a longer prefix or a full identifier.
In `@openspec/changes/support-user-apikey/tasks.md`:
- Line 27: Update the task item for handlePreview and /s/<id>/ws so it no longer
mentions owner checks; the preview protection contract should state that
protectPreviews only enforces a valid API key. Adjust the wording in the task
entry to reflect API-key-only authorization and remove any reference to
session-owner gating.
- Around line 53-55: The task list currently incorrectly asks to expose
STH_API_KEY in the server container, but that value is a client-side credential
for send/watch rather than a server runtime setting. Update the task item in the
support-user-apikey plan so docker-compose.yml only includes server-side
settings like STH_AUTH and STH_PROTECT_PREVIEWS, and remove STH_API_KEY from any
server environment or command requirements; keep .env.example focused on the
relevant server variables. Reference the affected task entries in the
support-user-apikey checklist when revising the wording.
- Line 33: The task item for preview protection currently couples
--protect-previews with --auth, which should be removed. Update the task text to
state that --protect-previews is an independent default-off switch and must not
imply, require, or auto-enable --auth; keep the scope focused on preview
protection behavior using the existing task entry in tasks.md.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: be11d5bf-b21f-4b30-8473-ca77e7083a57
📒 Files selected for processing (5)
openspec/changes/support-user-apikey/.openspec.yamlopenspec/changes/support-user-apikey/design.mdopenspec/changes/support-user-apikey/proposal.mdopenspec/changes/support-user-apikey/specs/user-auth/spec.mdopenspec/changes/support-user-apikey/tasks.md
|
|
||
| - [ ] 4.1 `parseArgs`（`internal/cli/cli.go`）与 `runStart` 新增 `--auth`、`--protect-previews` flag | ||
| - [ ] 4.2 新增 `STH_AUTH`、`STH_PROTECT_PREVIEWS` 环境变量读取，flag 优先级高于 env | ||
| - [ ] 4.3 校验：开启 `--protect-previews` 时若未开 `--auth`，启动报错或自动隐含开启（明确选定一种并在日志说明） |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not couple --protect-previews to --auth.
Preview protection is a separate default-off switch; forcing auth on, or failing startup when --auth is absent, changes the public contract.
♻️ Suggested task update
- [ ] 4.3 校验：开启 `--protect-previews` 时若未开 `--auth`，启动报错或自动隐含开启（明确选定一种并在日志说明）
+ [ ] 4.3 校验：`--protect-previews` 必须可独立于 `--auth` 开启，并沿用自己的 flag/env 优先级📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - [ ] 4.3 校验：开启 `--protect-previews` 时若未开 `--auth`，启动报错或自动隐含开启（明确选定一种并在日志说明） | |
| [ ] 4.3 校验：`--protect-previews` 必须可独立于 `--auth` 开启，并沿用自己的 flag/env 优先级 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openspec/changes/support-user-apikey/tasks.md` at line 33, The task item for
preview protection currently couples --protect-previews with --auth, which
should be removed. Update the task text to state that --protect-previews is an
independent default-off switch and must not imply, require, or auto-enable
--auth; keep the scope focused on preview protection behavior using the existing
task entry in tasks.md.
7 actionable comments from CodeRabbit (PR #78): - design D4: include handleDownload in owner-check isolation (avoid cross-user reads via /api/sessions/<id>/download when auth is on) - spec preview: clarify --protect-previews <-> --auth interaction; --protect-previews implies --auth; previews are API-key-only, not owner-gated; add scenarios for all combinations - spec api_keys: add per-record salt and hash_algo to schema fields - spec revoke-key: fail closed when prefix matches multiple keys - tasks 3.5: previews are API-key-only, drop owner check - tasks 7.x: STH_API_KEY is a client credential, not a server runtime var; remove from server container env - tasks 4.3: --protect-previews implies --auth (not error/coupled)
Implements all 41 tasks of the support-user-apikey change. Auth defaults to OFF for full backward compatibility; existing tests pass unchanged. Data layer (internal/session/auth.go): - users table (id, username UNIQUE, created_at) - api_keys table: stores only SHA-256(salt||key), never plaintext; per- record salt + hash_algo for future algorithm migration - sessions.user_id nullable column via existing ensureColumns migration - CreateUser/ListUsers/FindUserByUsername/IssueAPIKey/VerifyAPIKey/ RevokeAPIKey (fail-closed on ambiguous prefix)/SetSessionOwner/ SessionOwner/CountAPIKeysByUser - owner-scoped variants: FilterOptions.OwnerID, GetPeersForOwner Server (internal/server/auth.go, server.go): - authMiddleware gates all non-preview paths when auth on; previews gated only by --protect-previews - requireOwner: 403 for non-owners on per-session read/mutate endpoints (download, metadata, peers, tags/category/project, update-files, delete) - create handlers stamp owner; home list/search scoped by owner CLI: - start: --auth / --protect-previews (implies --auth) + STH_AUTH / STH_PROTECT_PREVIEWS env, flag-over-env, posture logged on startup - send/watch: --api-key / STH_API_KEY (flag wins), Authorization: Bearer, actionable hint on 401 - new 'sth user' subcommand: add/issue-key/revoke-key/list (local DB) - send-file.py passes through --api-key / STH_API_KEY Deployment: - docker-compose exposes STH_AUTH/STH_PROTECT_PREVIEWS to server (NOT STH_API_KEY, which is a client credential) - .env.example documents all vars with client/server grouping Tests (all green, go vet clean): - session: user/key lifecycle, plaintext-never-stored, revoke, ambiguity - server: middleware off=pass-through, missing/invalid/revoked=401, valid=200, preview matrix, protectPreviews-implies-auth, owner 403 - cli: --api-key header, STH_API_KEY env, flag>env, 401 hint Docs: README Authentication section; updated sth --help. openspec validate passes. Archive deferred to post-merge per workflow.
Four e2e tests exercising the real CLI + server stack (file-backed SQLite shared between them, live HTTP): - AuthFullFlow: auth-off legacy OK; auth-on send w/o key → 401 hint; send w/ key OK; owner isolation (bob 403 on alice's metadata + delete, alice 200 delete own) - PreviewProtection: --protect-previews → preview 401 w/o key, passes w/ any valid key (not owner-scoped) - UserRevokeKey: revoked key → 401 immediately - HomePageOwnerScoping: alice's home shows only her sessions Found + fixed a real client bug surfaced by e2e: runSend checked 401 after JSON unmarshal, but the auth middleware returns plain text, so the 'uninvalid response' error masked the actionable --api-key hint. Reordered 401/403 checks to run before JSON parsing.
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
internal/session/store.go (1)
297-304: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd an index on
sessions.user_idfor owner-scoped queries.When auth is enabled, list/search endpoints filter by owner. Without an index on
user_id, these queries full-scan thesessionstable. Theapi_keystable already gets indexes onuser_idandkey_prefix—sessions.user_idshould get the same treatment.♻️ Proposed fix: add index after column migration
if !columns["user_id"] { if _, err := s.db.Exec(`ALTER TABLE sessions ADD COLUMN user_id TEXT DEFAULT NULL`); err != nil { return err } + // Index for owner-scoped list/search queries when auth is enabled. + if _, err := s.db.Exec(`CREATE INDEX IF NOT EXISTS idx_sessions_user_id ON sessions(user_id)`); err != nil { + return err + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/session/store.go` around lines 297 - 304, The `sessions.user_id` migration only adds the nullable column, but owner-scoped queries need an index to avoid full table scans. Update the migration logic in the `sessions` schema setup path around the `s.db.Exec` block so that, after adding the `user_id` column, it also ensures an index exists on `sessions.user_id` similar to the existing `api_keys` indexes. Make the migration idempotent by checking for the index before creating it, and keep the change local to the same session-table bootstrap/migration code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cli/auth_e2e_test.go`:
- Around line 190-193: The auth E2E checks in the session metadata and preview
flows currently only reject unauthorized/forbidden responses, so they can still
pass on other failures like 404 or 500. Update the assertions around fx2.httpDo
in the relevant test cases (including the alice metadata check and the similar
preview/metadata checks later in the file) to require http.StatusOK explicitly,
so the tests verify the request actually succeeds rather than just not failing
with auth errors.
In `@internal/cli/cli.go`:
- Around line 352-356: The unauthorized-response error in cli.go should not end
with punctuation because Staticcheck ST1005 flags it. Update the error string
returned from the 401 branch in the response handling logic near the
http.StatusUnauthorized check to remove the trailing period, keeping the message
otherwise the same.
In `@internal/cli/watch.go`:
- Around line 70-91: The validateSession helper currently uses
http.DefaultClient without any deadline, so the metadata check can hang and
block watch startup; update the request flow in validateSession to use a
timeout-bound HTTP client or context-based deadline around the existing
http.NewRequest/http.DefaultClient.Do call. While touching this block, also
remove the trailing period from the 401 unauthorized error string returned after
the resp.StatusCode check.
In `@internal/server/auth.go`:
- Around line 85-90: The owner assignment error is being ignored in
assignOwnerIfNeeded, which can let create flows succeed even when
SetSessionOwner fails. Change assignOwnerIfNeeded in Server to return the
SetSessionOwner error instead of discarding it, then update the create handlers
that call it to stop and return a failure response before writing the success
status when owner assignment fails. Use the existing currentUser,
assignOwnerIfNeeded, and SetSessionOwner call sites to locate the affected
create path.
In `@internal/server/server.go`:
- Around line 486-490: SetAuthEnabled currently breaks the protectPreviews ⇒
authEnabled invariant by allowing auth to be disabled while previews remain
protected. Update Server.SetAuthEnabled so that when enabled is set to false it
also clears protectPreviews, or otherwise enforces that protectPreviews can only
remain true when authEnabled is true. Make sure the logic in SetAuthEnabled and
any related guard paths in Server preserves this invariant consistently.
In `@internal/session/auth_test.go`:
- Around line 142-181: Update TestRevokeAPIKey_PrefixAmbiguousFailsClosed so it
actually exercises the ErrAPIKeyAmbiguous path by creating two still-active API
keys whose prefixes collide and revoking using that shared prefix, then assert
the ambiguous error is returned; keep the existing “already revoked” and “not
found” coverage separate. Also correct the misleading comment in the test body
near the RevokeAPIKey call to describe prefix-based revocation rather than
“exact-id” behavior, and use the relevant symbols RevokeAPIKey, IssueAPIKey,
ErrAPIKeyAmbiguous, and KeyPrefixLen to locate the changes.
In `@internal/session/auth.go`:
- Around line 250-282: Fix RevokeAPIKey in Store so prefix matching escapes LIKE
wildcards in idOrPrefix before using it in the COUNT and UPDATE queries, and
wrap the count-check plus revoke update in a single transaction to avoid TOCTOU
races. Use the existing RevokeAPIKey method to locate the logic, and ensure the
escaped prefix is consistently reused for both the ambiguity check and the final
revocation update.
- Around line 311-323: `SessionOwner` currently returns the same result for a
missing session row and a session with no owner, which causes the auth-sensitive
mutating handlers to treat unknown session IDs as unauthorized instead of not
found. Update `SessionOwner` in `Store` to return a distinct not-found signal
when the query yields `sql.ErrNoRows`, and keep the existing nil-error false
result only for rows with an empty or null `user_id`; then adjust
`handleDeleteSession`, `handleAddTags`, `handleSetCategory`, `handleSetProject`,
and `handleUpdateFiles` to detect that not-found case and return 404 instead of
403.
In `@skills/sth/scripts/send-file.py`:
- Line 51: The secret handling in send-file.py is converting STH_API_KEY from
the environment into a subprocess argv value, which exposes credentials. Update
the command-building logic around api_key and the subprocess call in the send
flow so sth send does not receive --api-key from the parent process; instead let
the child inherit STH_API_KEY by default, and only pass an explicit API key via
the child environment when one is intentionally provided. Refer to the
send-file.py command assembly and subprocess invocation paths that use api_key
and the 207-223 block.
---
Nitpick comments:
In `@internal/session/store.go`:
- Around line 297-304: The `sessions.user_id` migration only adds the nullable
column, but owner-scoped queries need an index to avoid full table scans. Update
the migration logic in the `sessions` schema setup path around the `s.db.Exec`
block so that, after adding the `user_id` column, it also ensures an index
exists on `sessions.user_id` similar to the existing `api_keys` indexes. Make
the migration idempotent by checking for the index before creating it, and keep
the change local to the same session-table bootstrap/migration code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 79836f3b-3f25-4cd4-b2f7-503bc2950074
📒 Files selected for processing (18)
.env.example.gitignoreREADME.mddocker-compose.ymlinternal/cli/auth_e2e_test.gointernal/cli/auth_test.gointernal/cli/cli.gointernal/cli/user.gointernal/cli/watch.gointernal/server/auth.gointernal/server/auth_test.gointernal/server/server.gointernal/session/auth.gointernal/session/auth_test.gointernal/session/metadata.gointernal/session/store.goopenspec/changes/support-user-apikey/tasks.mdskills/sth/scripts/send-file.py
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- openspec/changes/support-user-apikey/tasks.md
Comment on lines
+85
to
+90
| func (s *Server) assignOwnerIfNeeded(r *http.Request, sessionID string) { | ||
| uid, ok := currentUser(r) | ||
| if !ok { | ||
| return | ||
| } | ||
| _ = s.store.SetSessionOwner(sessionID, uid) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Propagate owner-assignment failures.
Line 90 drops SetSessionOwner errors, so an auth-enabled create can return 201 for a session that remains unowned and then fails owner checks. Return the error and have the create handlers fail before sending the success response.
Proposed fix
-func (s *Server) assignOwnerIfNeeded(r *http.Request, sessionID string) {
+func (s *Server) assignOwnerIfNeeded(r *http.Request, sessionID string) error {
uid, ok := currentUser(r)
if !ok {
- return
+ return nil
}
- _ = s.store.SetSessionOwner(sessionID, uid)
+ return s.store.SetSessionOwner(sessionID, uid)
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/server/auth.go` around lines 85 - 90, The owner assignment error is
being ignored in assignOwnerIfNeeded, which can let create flows succeed even
when SetSessionOwner fails. Change assignOwnerIfNeeded in Server to return the
SetSessionOwner error instead of discarding it, then update the create handlers
that call it to stop and return a failure response before writing the success
status when owner assignment fails. Use the existing currentUser,
assignOwnerIfNeeded, and SetSessionOwner call sites to locate the affected
create path.
Comment on lines
+311
to
+323
| func (s *Store) SessionOwner(sessionID string) (userID string, ok bool, err error) { | ||
| var uid sql.NullString | ||
| if e := s.db.QueryRow(`SELECT user_id FROM sessions WHERE session_id = ?`, sessionID).Scan(&uid); e != nil { | ||
| if errors.Is(e, sql.ErrNoRows) { | ||
| return "", false, nil | ||
| } | ||
| return "", false, e | ||
| } | ||
| if !uid.Valid || uid.String == "" { | ||
| return "", false, nil | ||
| } | ||
| return uid.String, true, nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify requireOwner behavior when SessionOwner returns ok=false
rg -n -A 20 'func.*requireOwner' --type go internal/server/Repository: sun-praise/static-html
Length of output: 1493
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== requireOwner call sites =="
rg -n "requireOwner\(" internal/server --type go
echo
echo "== Session lookup patterns around handlers =="
rg -n -A 8 -B 8 "SessionOwner\(|session_id|sessionID" internal/server --type goRepository: sun-praise/static-html
Length of output: 30785
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== delete session handler =="
sed -n '1748,1766p' internal/server/server.go
echo
echo "== auth tests around requireOwner/delete =="
rg -n -A 20 -B 10 "requireOwner|delete session|StatusNotFound|StatusForbidden|Session not found" internal/server/auth_test.go internal/server/server_test.go --type goRepository: sun-praise/static-html
Length of output: 9568
Separate missing sessions from unowned sessions in SessionOwner
SessionOwner still collapses “not found” and “no owner” into ("", false, nil). That makes mutating paths like handleDeleteSession, handleAddTags, handleSetCategory, handleSetProject, and handleUpdateFiles return 403 for unknown session IDs when auth is enabled. Return a distinct not-found signal so those paths can emit 404 instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/session/auth.go` around lines 311 - 323, `SessionOwner` currently
returns the same result for a missing session row and a session with no owner,
which causes the auth-sensitive mutating handlers to treat unknown session IDs
as unauthorized instead of not found. Update `SessionOwner` in `Store` to return
a distinct not-found signal when the query yields `sql.ErrNoRows`, and keep the
existing nil-error false result only for rows with an empty or null `user_id`;
then adjust `handleDeleteSession`, `handleAddTags`, `handleSetCategory`,
`handleSetProject`, and `handleUpdateFiles` to detect that not-found case and
return 404 instead of 403.
Security/correctness: - SessionOwner now 3-state (Owned/Unowned/Missing): missing sessions return 404 not 403 under auth, so clients distinguish not-found from forbidden - RevokeAPIKey: escape LIKE wildcards in prefix + wrap count/update in a transaction to close a TOCTOU race and prevent literal %/_ mismatches - assignOwnerIfNeeded returns its error; create handlers fail loudly instead of silently leaking an unowned session under auth - SetAuthEnabled(false) now clears protectPreviews, preserving the invariant that preview protection requires auth infra - send-file.py passes the API key via child env (STH_API_KEY), not argv, so it is not exposed in process listings (ps) Performance: - add idx_sessions_user_id for owner-scoped list/search queries Reliability: - validateSession uses a 10s-timeout client instead of http.DefaultClient Test rigor: - e2e assertions require 200 (not just 'not 401/403'), catching 404/500 - TestRevokeAPIKey split: real ambiguous-prefix path + LIKE-wildcard escape test + new missing-session-404 test Style: - drop trailing periods from 401 error strings (ST1005) All tests green, go vet clean.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/server/auth.go (1)
42-45: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDon’t collapse auth store errors into 401 responses.
VerifyAPIKeycan return query/scan errors, but this call path treats everyverifyBearerfailure as invalid credentials. Return an error fromverifyBearerand respond with 500/503 for store failures so outages aren’t misreported as bad keys.Proposed fix
- userID, ok := s.verifyBearer(r) + userID, ok, err := s.verifyBearer(r) + if err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } if !ok { w.Header().Set("WWW-Authenticate", `Bearer realm="sth"`) http.Error(w, "Unauthorized: a valid API key is required.", http.StatusUnauthorized)// Update verifyBearer accordingly: func (s *Server) verifyBearer(r *http.Request) (string, bool, error) { // parse Bearer token... userID, ok, err := s.store.VerifyAPIKey(token) if err != nil { return "", false, err } return userID, ok, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/server/auth.go` around lines 42 - 45, The auth failure path in verifyBearer and its caller is collapsing backend store errors into a generic 401. Update Server.verifyBearer to return an error alongside userID and ok, propagate errors from s.store.VerifyAPIKey, and have the handler distinguish invalid credentials from store/query failures. Keep 401 only for bad API keys, and return an appropriate 500/503 from the auth handler when verifyBearer reports a store outage or scan/query error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/session/auth.go`:
- Around line 261-283: In RevokeAPIKey, the fallback prefix lookup can match
every active key when idOrPrefix is empty or too short because the LIKE pattern
becomes too broad. Add a guard before building the escaped pattern so the method
fails closed unless idOrPrefix is non-empty and meets the minimum prefix length,
then only proceed with the LIKE-based revocation for valid prefixes after the
exact-id update misses.
---
Outside diff comments:
In `@internal/server/auth.go`:
- Around line 42-45: The auth failure path in verifyBearer and its caller is
collapsing backend store errors into a generic 401. Update Server.verifyBearer
to return an error alongside userID and ok, propagate errors from
s.store.VerifyAPIKey, and have the handler distinguish invalid credentials from
store/query failures. Keep 401 only for bad API keys, and return an appropriate
500/503 from the auth handler when verifyBearer reports a store outage or
scan/query error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8d65b377-a69d-4937-bbbe-cdd3f3cf6aac
📒 Files selected for processing (10)
internal/cli/auth_e2e_test.gointernal/cli/cli.gointernal/cli/watch.gointernal/server/auth.gointernal/server/auth_test.gointernal/server/server.gointernal/session/auth.gointernal/session/auth_test.gointernal/session/store.goskills/sth/scripts/send-file.py
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/session/store.go
- internal/server/auth_test.go
- internal/cli/watch.go
- internal/cli/auth_e2e_test.go
- internal/server/server.go
- internal/cli/cli.go
Address the 2 remaining CodeRabbit comments (round 3): - verifyBearer now returns (userID, ok, err): store/query failures surface as 500, only genuinely bad keys return 401. Previously a DB outage was misreported as invalid credentials. - RevokeAPIKey rejects prefixes shorter than KeyPrefixLen before building the LIKE pattern, so an empty/short input can't match every active key (e.g. '' → '%'). Fail closed. Tests updated: ambiguous-prefix test uses a >=KeyPrefixLen colliding prefix; new TestRevokeAPIKey_TooShortFailsClosed covers the guard; wildcard-escape test uses an adequately long prefix.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/session/auth_test.go (1)
171-171: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: comment says "13 chars" but
"sth_collideX"is 12 characters.The test logic is correct — 12 chars is still ≥
KeyPrefixLen(since"sth_real"at 8 chars is rejected). Just a comment inaccuracy.📝 Fix comment
- err = store.RevokeAPIKey("sth_collideX") // 13 chars, >= KeyPrefixLen, matches both + err = store.RevokeAPIKey("sth_collideX") // 12 chars, >= KeyPrefixLen, matches both🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/session/auth_test.go` at line 171, Correct the inline comment for the RevokeAPIKey("sth_collideX") test case to state that the value is 12 characters, while retaining the existing test logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/session/auth_test.go`:
- Line 171: Correct the inline comment for the RevokeAPIKey("sth_collideX") test
case to state that the value is 12 characters, while retaining the existing test
logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a461ff08-4f00-4990-8ab5-ee015637c76b
📒 Files selected for processing (3)
internal/server/auth.gointernal/session/auth.gointernal/session/auth_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/server/auth.go
- internal/session/auth.go
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
背景
sth目前完全没有鉴权：任何能访问端口的客户端都能上传、增量覆盖、删除任意 session，也能拉取全部列表/搜索结果。sessions表也没有 owner 概念。这契合单人本地预览，但团队/共享部署时存在越权覆盖与数据泄露风险。本 PR 是 OpenSpec 变更提案（proposal + design + spec + tasks），尚不包含实现代码。合并后用
/opsx:apply进入实现阶段。方案要点（与你确认过的）
--auth/STH_AUTH控制，默认关闭，关闭时行为与旧版完全一致。sessions加user_id列。/s/<id>/默认仍开放（保留分享核心用法），可用--protect-previews收紧。--api-key/STH_API_KEY，Authorization: Bearer <key>。sth user子命令：本地直连 DB 管理 user 与 key（签发明文一次性输出）。Artifacts（4/4，已
openspec validate通过）proposal.mddesign.mdspecs/user-auth/spec.mdtasks.md关键设计取舍
sth的核心 UX 是上传后分享/s/<id>/给他人查看。一刀切鉴权会让分享场景失效，故把防写入越权与防预览泄露解耦。user_id为 NULL 的旧 session 在鉴权模式下不可触达（本期不引入管理员角色）。sth user管理 + API key 表征，与项目体量匹配。下一步
合并本 PR 后，在新 worktree 中运行
/opsx:apply support-user-apikey按tasks.md顺序实现。Summary by CodeRabbit
New Features
sth, including session owner isolation for protected operations and owner-scoped listings/search/peers.--protect-previews) that can require an API key while keeping previews shareable when disabled.sth usercommands to create users, issue/revoke API keys, and list active key counts (plaintext key shown only once).Documentation
.env.example, and compose config forSTH_AUTH/STH_PROTECT_PREVIEWS, plus client usage via--api-key/STH_API_KEY.Tests
Closes #79