feat: temp access tokens - #3603
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
💤 Files with no reviewable changes (9)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds scoped short-lived temp tokens end-to-end: DB model with encryption and migration, ConfigStore CRUD, scope registry and mint/validate service, OAuth mint/cleanup, middleware fallback, sweep worker, frontend fragment handling and RTK Query header integration, plus tests and routes. ChangesTemporary Token Authentication System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
af65005 to
88e9e5f
Compare
f649709 to
791502b
Compare
88e9e5f to
2dc32c9
Compare
2dc32c9 to
3cb1787
Compare
3cb1787 to
d6036cb
Compare
791502b to
bb4e5d4
Compare
Confidence Score: 5/5This PR is safe to merge. The new temp-token path is additive — cookie and Bearer auth take strict precedence, and all existing auth flows are unmodified. The mint/validate/delete lifecycle is carefully constructed: SHA-256 hash lookup avoids storing plaintext, AES-256-GCM at-rest encryption mirrors the established sessions pattern, and route matching is exact after resource_id substitution so a token for one flow cannot authorize another. Migration, sweep worker, and lifecycle cleanup are all correctly wired. No files require special attention. Important Files Changed
Reviews (5): Last reviewed commit: "feat: temp access tokens backend" | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
framework/temptoken/service_test.go (2)
75-84: ⚡ Quick winUse
MCPAuthScopeNamein tests to avoid scope-name drift.The test suite hardcodes
"mcp_auth"in multiple places. Reusing the exported constant keeps tests coupled to the canonical scope contract.♻️ Proposed fix
func mcpAuthScope() Scope { return Scope{ - Name: "mcp_auth", + Name: MCPAuthScopeName, @@ func TestMintRejectsTTLOverMax(t *testing.T) { svc, _ := newServiceWithMcpAuth(t) - _, err := svc.Mint(context.Background(), "mcp_auth", "flow-1", time.Hour) + _, err := svc.Mint(context.Background(), MCPAuthScopeName, "flow-1", time.Hour) @@ func TestValidateHappyPath(t *testing.T) { svc, _ := newServiceWithMcpAuth(t) - tok, err := svc.Mint(context.Background(), "mcp_auth", "flow-abc", 5*time.Minute) + tok, err := svc.Mint(context.Background(), MCPAuthScopeName, "flow-abc", 5*time.Minute)Also applies to: 99-123, 140-174
🤖 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 `@framework/temptoken/service_test.go` around lines 75 - 84, Replace the hardcoded scope name "mcp_auth" in the test helper mcpAuthScope() with the exported constant MCPAuthScopeName (i.e. set Scope{Name: MCPAuthScopeName, ...}) and likewise update any other test occurrences that directly use the literal "mcp_auth" so tests reference the canonical MCPAuthScopeName constant; update mcpAuthScope(), any direct string usages in assertions or setup, and imports if needed so tests compile against the exported symbol.
192-218: ⚡ Quick winAdd unit tests for
DeleteByResourceIDlifecycle cleanup.This API is part of the auth-flow invalidation contract but currently has no direct coverage.
✅ Suggested tests
+func TestDeleteByResourceIDRemovesMatchingTokens(t *testing.T) { + svc, _ := newServiceWithMcpAuth(t) + _, _ = svc.Mint(context.Background(), MCPAuthScopeName, "flow-abc", 5*time.Minute) + _, _ = svc.Mint(context.Background(), MCPAuthScopeName, "flow-abc", 5*time.Minute) + _, _ = svc.Mint(context.Background(), MCPAuthScopeName, "flow-other", 5*time.Minute) + + n, err := svc.DeleteByResourceID(context.Background(), MCPAuthScopeName, "flow-abc") + if err != nil { + t.Fatalf("delete: %v", err) + } + if n != 2 { + t.Fatalf("expected 2 deleted, got %d", n) + } +} + +func TestDeleteByResourceIDEmptyInputsNoOp(t *testing.T) { + svc, _ := newServiceWithMcpAuth(t) + n, err := svc.DeleteByResourceID(context.Background(), "", "flow-abc") + if err != nil || n != 0 { + t.Fatalf("expected no-op delete for empty scope, got n=%d err=%v", n, 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 `@framework/temptoken/service_test.go` around lines 192 - 218, Add unit tests for the DeleteByResourceID lifecycle: create a Registry (NewRegistry), Register a Scope that includes ResourceIDInPath (e.g., "{id}") and AllowedRoutes containing the placeholder, Issue/Create at least two tokens tied to the same resource ID and one tied to a different resource, then call DeleteByResourceID(resourceID) and assert that tokens for that resource are removed while others remain; also assert calling DeleteByResourceID on a non-existent resource is a no-op. Use existing test helpers/patterns from TestRegistry* (e.g., mcpAuthScope or similar scope construction) and reference DeleteByResourceID and Registry methods so the tests exercise lifecycle cleanup and invalidation behavior.ui/components/tempTokenScope.tsx (1)
30-30: ⚡ Quick winAlign component filename with PascalCase convention.
The export is PascalCase, but the file is
tempTokenScope.tsx. Rename toTempTokenScope.tsxfor consistency with the TSX component naming rule.As per coding guidelines,
ui/**/*.tsxReact component files must use PascalCase for component exports and filenames.🤖 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 `@ui/components/tempTokenScope.tsx` at line 30, Rename the file from tempTokenScope.tsx to TempTokenScope.tsx to match the exported React component TempTokenScope and PascalCase conventions; update any imports referencing the old filename throughout the codebase to import from "TempTokenScope" instead of "tempTokenScope", and ensure the component export signature export default function TempTokenScope({ name: _name, children }: TempTokenScopeProps) remains unchanged.ui/lib/store/apis/baseApi.ts (1)
7-7: ⚡ Quick winUse the UI alias import for tempToken helpers.
Replace
./tempTokenwith@/lib/store/apis/tempTokento stay consistent with the UI import convention.Based on learnings, prefer alias imports using
@/...in UI code (e.g.,@/lib/...) over relative imports.🤖 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 `@ui/lib/store/apis/baseApi.ts` at line 7, In baseApi.ts replace the relative import for the tempToken helpers with the UI alias: update the import line that currently imports getActiveTempToken and getSuppressGlobal401 from "./tempToken" to import them from "`@/lib/store/apis/tempToken`" so the file uses the standard UI alias import for these helpers.
🤖 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 `@framework/configstore/store.go`:
- Around line 258-265: The mutating temp-token methods must accept an optional
transaction so callers can join their DB transaction; change the signatures of
CreateTempToken, DeleteTempTokensByResourceID and DeleteExpiredTempTokens to
accept a variadic tx parameter (e.g. tx ...*gorm.DB) and update their
implementations to use the provided tx[0] when present or fall back to the store
DB connection otherwise; then update callers (e.g. OAuth/session lifecycle code)
to pass the active *gorm.DB transaction when creating or deleting temp tokens so
token ops commit/rollback with the business-row transaction.
In `@framework/configstore/tables/temp_token.go`:
- Around line 37-57: The BeforeSave/AfterFind hooks on TempToken currently tie
setting t.EncryptionStatus to t.Token being non-empty which breaks partial
selects; change BeforeSave so that when encrypt.IsEnabled() is true you always
set t.EncryptionStatus = EncryptionStatusEncrypted (even if t.Token == ""), but
only call encryptString(&t.Token) when t.Token != ""; likewise update AfterFind
to only call decryptString(&t.Token) when t.Token != "" while still relying on
t.EncryptionStatus to determine whether decryption should be attempted. Ensure
you modify the TempToken.BeforeSave and TempToken.AfterFind code paths and keep
references to encrypt.IsEnabled(), encryptString, decryptString, t.Token and
t.EncryptionStatus as described.
In `@framework/oauth2/main.go`:
- Around line 72-80: The cleanupFlow function currently uses the incoming
request ctx so cancellations can abort the DeleteOauthUserSession and temp token
DeleteByResourceID calls; change it to run detached from the request context by
performing the deletes using a non-cancelable context (e.g. context.Background()
or an existing provider background context like p.bgCtx) and run the cleanup in
a goroutine to ensure it always executes; keep the same calls to
p.configStore.DeleteOauthUserSession and p.tempTokens.DeleteByResourceID (with
temptoken.MCPAuthScopeName and sessionID) and retain the logger.Warn error
handling inside the goroutine.
In `@framework/temptoken/scope.go`:
- Around line 100-106: Register currently stores the AllowedRoutes slice by
reference and Lookup returns it directly, allowing callers to mutate the backing
array and race with validations; update Register (the method that assigns
r.scopes[s.Name] = s) to deep-copy s.AllowedRoutes into a new slice before
persisting the scope (copy the slice header and elements) and update Lookup to
return a copy of the AllowedRoutes slice (not the original backing slice) so
callers receive an immutable view; ensure you reference the AllowedRoutes field
on the scope value and the Register and Lookup methods when making these
changes.
In `@transports/bifrost-http/handlers/middlewares.go`:
- Around line 918-921: APIMiddleware currently treats "/api/oauth" as
whitelisted in shouldSkip(), which also matches "/api/oauth/per-user/..." and
skips auth; update shouldSkip() (in APIMiddleware) to exclude per-user OAuth
routes by either narrowing the whitelist entry for the OAuth routes or adding an
explicit early check that returns false when the request path has the prefix
"/api/oauth/per-user/". Ensure this change preserves the existing behavior for
other "/api/oauth" endpoints but forces requests to reach
m.tryTempTokenOrUnauthorized(ctx, next) (or normal session auth) for per-user
flows.
In `@transports/bifrost-http/handlers/oauth2.go`:
- Around line 86-95: The failure redirect paths need to mirror the
success-branch's session-awareness: locate the OAuth2 callback logic around
cookieToken and validateSession (use symbols cookieToken, validateSession,
ctx.Redirect and the handler that references h.store.ConfigStore) and change any
per-user failure/denial redirects that currently point to
"/workspace/mcp-sessions?...” so that when there is no valid session
(cookieToken == "" or validateSession returns false) they instead redirect to
the public fallback "/workspace/mcp-sessions/auth-success" (use the same
fasthttp.StatusFound handling), while preserving the existing dashboard redirect
for validated sessions.
In `@transports/bifrost-http/server/server.go`:
- Around line 1454-1462: When handlers.RegisterTempTokenScopes(s.TempTokens)
fails, the previously started handlers.NewSignedWSTicketStore assigned to
s.WSTicketStore must be stopped to avoid leaking the running ticket store;
modify the error path in the Bootstrap block so that on regErr != nil you call
the store's shutdown method (e.g. s.WSTicketStore.Stop() or Close(), depending
on the store API) before returning the fmt.Errorf, and handle or log any
returned error from that shutdown call; reference symbols: s.WSTicketStore,
handlers.NewSignedWSTicketStore, s.TempTokens, handlers.RegisterTempTokenScopes.
In `@ui/components/tempTokenScope.tsx`:
- Around line 42-57: The render-time side effects inside the useState
initializer must be moved to a layout effect: remove calls to setActiveTempToken
and setSuppressGlobal401 from the useState(() => { ... }) initializer and
instead compute the initial token there (or return null) and then in a
useLayoutEffect run once (checking window and parsing via
parseTokenFromFragment(window.location.hash)) call setActiveTempToken(token) and
setSuppressGlobal401(true) only when a token is present so mutations occur after
commit but before children effects; also rename the file to PascalCase
(TempTokenScope.tsx) to match the project convention.
---
Nitpick comments:
In `@framework/temptoken/service_test.go`:
- Around line 75-84: Replace the hardcoded scope name "mcp_auth" in the test
helper mcpAuthScope() with the exported constant MCPAuthScopeName (i.e. set
Scope{Name: MCPAuthScopeName, ...}) and likewise update any other test
occurrences that directly use the literal "mcp_auth" so tests reference the
canonical MCPAuthScopeName constant; update mcpAuthScope(), any direct string
usages in assertions or setup, and imports if needed so tests compile against
the exported symbol.
- Around line 192-218: Add unit tests for the DeleteByResourceID lifecycle:
create a Registry (NewRegistry), Register a Scope that includes ResourceIDInPath
(e.g., "{id}") and AllowedRoutes containing the placeholder, Issue/Create at
least two tokens tied to the same resource ID and one tied to a different
resource, then call DeleteByResourceID(resourceID) and assert that tokens for
that resource are removed while others remain; also assert calling
DeleteByResourceID on a non-existent resource is a no-op. Use existing test
helpers/patterns from TestRegistry* (e.g., mcpAuthScope or similar scope
construction) and reference DeleteByResourceID and Registry methods so the tests
exercise lifecycle cleanup and invalidation behavior.
In `@ui/components/tempTokenScope.tsx`:
- Line 30: Rename the file from tempTokenScope.tsx to TempTokenScope.tsx to
match the exported React component TempTokenScope and PascalCase conventions;
update any imports referencing the old filename throughout the codebase to
import from "TempTokenScope" instead of "tempTokenScope", and ensure the
component export signature export default function TempTokenScope({ name: _name,
children }: TempTokenScopeProps) remains unchanged.
In `@ui/lib/store/apis/baseApi.ts`:
- Line 7: In baseApi.ts replace the relative import for the tempToken helpers
with the UI alias: update the import line that currently imports
getActiveTempToken and getSuppressGlobal401 from "./tempToken" to import them
from "`@/lib/store/apis/tempToken`" so the file uses the standard UI alias import
for these helpers.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: be613e15-a0b8-4a85-afdb-849a4fe440d1
📒 Files selected for processing (24)
core/schemas/bifrost.goframework/configstore/encryption.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/temp_token.goframework/oauth2/main.goframework/oauth2/sync.goframework/temptoken/scope.goframework/temptoken/service.goframework/temptoken/service_test.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/handlers/oauth2.gotransports/bifrost-http/handlers/temp_token_scopes.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/server/server.goui/app/clientLayout.tsxui/app/workspace/mcp-sessions/auth-success/layout.tsxui/app/workspace/mcp-sessions/auth-success/page.tsxui/app/workspace/mcp-sessions/auth/layout.tsxui/app/workspace/mcp-sessions/auth/page.tsxui/components/tempTokenScope.tsxui/lib/store/apis/baseApi.tsui/lib/store/apis/tempToken.ts
a3710a1 to
0894aaa
Compare
0894aaa to
e8a3444
Compare
bb4e5d4 to
5f7a5b6
Compare
Merge activity
|
The base branch was changed.
e8a3444 to
adb3920
Compare
## Summary This PR cuts the `v1.5.11` / `v1.3.11` release across core, framework, and all plugins, and introduces a new Claude skill (`release-checklist`) for pre-release migration safety auditing. ## Changes - **`release-checklist` skill** — Adds `.claude/skills/release-checklist/SKILL.md`, a read-only pre-release audit tool that scans Go-defined database migrations changed in a release for high-scale deadlock/lock-contention risks and boot-time-blocking operations. It produces a structured `PASS`/`WARN`/`FAIL` report with a concrete remediation plan per finding. The skill is designed to grow via an extensible Checks Registry. - **Version bumps** — `core` → `1.5.11`, `framework` → `1.3.11`, `transports` → `1.5.3`, `plugins/governance` → `1.5.11`, `plugins/logging` → `1.5.11`, `plugins/semanticcache` → `1.5.11`, `plugins/otel` → `1.2.11`, `plugins/maxim` → `1.6.11`, `plugins/prompts` → `1.0.11`, and remaining plugins bumped accordingly. - **Changelogs populated** — All per-package changelogs updated with the full set of features and fixes shipping in this release. Key highlights in this release: - Temporary access tokens for scoped, time-limited API access - MCP per-user OAuth flow refactor - Bedrock Mantle inference engine support - Azure Realtime provider with enriched session tracking - Direct access control (DAC) and virtual key rotation - Cluster-aware log metadata and per-node usage aggregation - Feature flag framework - Config-hash-based file value override of DB on restart - Semantic cache plugin rewrite - Numerous streaming stability, Bedrock, Anthropic, and Gemini fixes - AWS SDK and dependency security updates ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [x] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [x] Providers/Integrations - [x] Plugins - [x] UI (React) - [ ] Docs ## How to test ```sh # Verify version files reflect the new release cat core/version # expect 1.5.11 cat framework/version # expect 1.3.11 cat transports/version # expect 1.5.3 # Core/Transports go test ./... ``` To exercise the new `release-checklist` skill, invoke it via Claude with: ``` /release-checklist origin/dev...HEAD ``` Expected output: a structured report with `PASS`/`WARN`/`FAIL` per check and a Remediation Plan table for any findings. ## Screenshots/Recordings N/A ## Breaking changes - [ ] Yes - [x] No ## Related issues #3603, #3565, #3489, #3334, #3335, #3435, #3554, #3590, #3444, #3198, #3581, #3610, #3599, #3567, #3382, #3461 and others listed in the changelogs. ## Security considerations - AWS SDK and dependency security updates are included (#3461). - `FullyRedacted()` for proxy passwords and `MarshalForStorage()` for `ProxyConfig` prevent partial secret leakage in API responses (#3445). - The `release-checklist` skill is strictly read-only and never modifies files. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable

Summary
Adds a temp-access-token primitive that authorizes browser access to specific, narrow-scoped
pages without requiring a full dashboard login. The motivating case: when dashboard auth is
enabled with username/password, an end user holding only a VK couldn't complete an MCP per-user
OAuth flow because
/workspace/mcp-sessions/auth?flow=<id>required a dashboard session — theyhad no way to authenticate. Now the inline-401 URL the server emits carries a short-lived token
in the URL fragment (
#t=…); the auth page validates that token against a per-scope routeallowlist and renders without dashboard chrome. The same primitive is reusable for any future
flow that needs "anonymous visitor with a server-emitted URL".
Changes
Backend — framework/temptoken (new package)
Service.Mint(scope, resourceID, ttl)→ returns plaintext, persists hash + encrypted at-restcopy. SHA-256 lookup column, AES-256-GCM via
framework/encrypt(mirrors the SessionsTablepattern).
Service.Validate(plaintext, method, path)→ row lookup, expiry check, scope's allowed-routescheck (with
{id}substitution against the row'sresource_id).Service.DeleteByResourceID(scope, resourceID)→ lifecycle-driven invalidation. Caller burnsthe token when the work it authorized completes.
Registryholds startup-registeredScopedefinitions (allowed routes, MaxTTL,ResourceIDInPath).
Backend — DB schema
temp_tokenstable:id, token (encrypted), token_hash (SHA-256, unique), scope, resource_id, expires_at, created_at, updated_at, encryption_status. Indexes onscope,resource_id,expires_at.migrationAddTempTokensTable), wired intoEncryptPlaintextRowsforat-rest re-encryption.
Backend — wiring
mcp_authscope registered at server boot inhandlers/temp_token_scopes.go. Routes:GET /api/oauth/per-user/flows/{id}and.../start. MaxTTL 15min.OAuth2Provider.InitiateUserOAuthFlowmints amcp_authtoken bound to the flow's session IDand appends
#t=<plaintext>to the returned frontend URL. Mint failure is non-fatal —dashboard-authenticated callers still get a usable URL.
OAuth2Provider.cleanupFlowhelper deletes both the flow row and any associated temp tokens onevery terminal transition in
CompleteUserOAuthFlow(success or any of 5 failure branches).DeleteExpiredTempTokens(the comment claiming thisexisted was previously aspirational).
no valid cookie" path). Cookie/Bearer auth still takes precedence.
admins →
/workspace/mcp-sessions?completed=1, anonymous temp-token visitors → new/workspace/mcp-sessions/auth-success.Frontend — React
<TempTokenScope>wrapper: reads the#t=…fragment in auseStateinitializer (synchronous,before child effects fire), installs module-level state used by
baseApi.prepareHeaderstoattach
X-Bifrost-Temp-Token, sets a global-401-suppression flag, and strips the fragment viahistory.replaceStateso it doesn't leak in Referer headers on outbound navigation.ClientLayoutlearns two staticData flags:tempTokenScoped(route is anonymous-friendly;per-visitor layout decision based on
is-auth-enabledprobe + fragment presence) andpublicShell(always render MinimalShell, no auth probe, no API calls — for the post-OAuthsuccess page).
/workspace/mcp-sessions/auth-success(public landing), updated/workspace/mcp-sessions/authwith InvalidLinkView for expired/missing tokens.baseApi: attachX-Bifrost-Temp-Tokenwhen set, skip global/loginredirect on 401 whensuppression flag is active.
Notable design decisions
and TTL. New flows register a scope at startup; the framework owns mint/validate/delete.
cleanupFlowcallsDeleteByResourceIDrather thanON DELETE CASCADEon a foreign key. Keepstemp_tokens.resource_idopaque — future scopes thatbind to non-OAuth resources stay clean.
consumed_atcolumn — invalidation is row deletion.Atomic, no Validate-side gymnastics, smaller schema. Dropped a column I'd originally added during
scaffolding.
#t=…never reaches the server in logs or Referer. Strippedfrom the URL on mount so it doesn't survive into outbound navigation either.
tempTokenScopedadvertises "this route may be visitedanonymously"; the actual MinimalShell vs. full-chrome decision is made per-request from a cheap
whitelisted auth probe.
Type of change
Affected areas
core/schemas/bifrost.gobifrost/private/temp-tokens.md; no user-facing docs added)How to test