feat: two-tier token authentication (v6.0.0)#208
Merged
Conversation
Phase 1 of auth-two-tier-tokens (v6.0.0): - internal/auth/identity.go — Identity value type + Source enum (master/client/session) - internal/auth/errors.go — sentinel errors (ErrEmptyToken, ErrInvalidCredentials, ErrRevoked) - internal/auth/validator.go — Validator with two-arm chain (master compare + bcrypt prefix lookup) - internal/auth/context.go — RoleKey/SourceKey context helpers + IsSessionAdmin gate - internal/auth/validator_test.go — table-driven tests covering every branch - internal/auth/context_test.go — context round-trip + IsSessionAdmin discrimination Coverage 97.5%; go vet clean. No external deps added. Refs: spec.md FR-1, FR-5; plan.md Phase 1 (T1.1..T1.5).
Phase 2 of auth-two-tier-tokens (v6.0.0): - internal/grpcserver/server.go: * drop ENV-derived `token` field; replace with `*auth.Validator` * add SetValidator() setter (mirrors SetDB/SetBus pattern) * authInterceptor delegates to validator.Validate, maps to gRPC codes * NEW streamAuthInterceptor for ProjectEvents stream method * NEW authedStream wrapper exposes auth.Identity via Context() - internal/grpcserver/sync_project_state.go: withDB() copies validator field - internal/grpcserver/auth_interceptor_test.go (NEW): direct interceptor unit tests covering Ping skip, missing metadata, missing header, master-token success, valid keycard success, invalid token, raw bearer without prefix, streaming-valid, streaming-missing, streaming-revoked. - internal/auth/context.go: simplify to single IdentityKey (full Identity in context, including KeycardID for audit/stats handlers) - internal/auth/context_test.go: tests adjusted to new key - internal/worker/service.go: build *auth.Validator before grpcserver.New; pass nil when ENGRAM_AUTH_DISABLED=true (mirrors prior token=="" branch). Build clean; auth pkg coverage 97.1%; grpcserver tests green. Refs: spec.md FR-2, FR-6, US1, US5; plan.md Phase 2 (T2.1..T2.6).
…hardened
Phase 3 of auth-two-tier-tokens (v6.0.0):
- internal/worker/middleware.go:
* TokenAuth gains validator field + SetValidator() setter
* Middleware bearer/X-Auth-Token/SSE-query path delegates to validator
* Removed inline authenticateClientToken (DRY: single chain shared with gRPC)
* Read-only scope gate + stats increment now key off auth.Identity
* NEW emptyTokenStore for bootstrap window before SetValidator() swap
* NEW extractHTTPBearer / isMutatingMethod / buildAuthCtx helpers
* buildAuthCtx writes BOTH auth.IdentityKey and legacy authRoleKey for
backward compat with handlers that still read worker.authRoleKey{}
- internal/worker/handlers_auth.go:
* NEW requireSessionAdmin guard: 403 when caller is not session-cookie admin
* Applied to handleCreateToken, handleListTokens, handleRevokeToken,
handleGetTokenStats — issuance is browser-only per FR-6 / Clarification C4
* Bearer-token paths (master OR worker keycard) explicitly rejected on
these endpoints with deterministic JSON 403 body
- internal/worker/service.go:
* Wire shared *auth.Validator into BOTH gRPC server AND HTTP middleware
after tokenStore is ready (FR-2: symmetric validation on both transports)
All worker tests green. Build + vet clean.
Refs: spec.md FR-2, FR-6, US2, US4-bis; plan.md Phase 3 (T3.1..T3.5).
…st, pool keys credential Phase 4 of auth-two-tier-tokens (v6.0.0): - internal/config/envnames.go (NEW): single source of truth for the four canonical env vars (EnvServerURL, EnvServerURLAlt, EnvAdminToken, EnvWorkstationToken). Renames are now caught at compile time. - internal/handlers/engramcore/tools.go: ProxyTools + ProxyHandleTool read config.EnvWorkstationToken (= "ENGRAM_TOKEN"). The historical ENGRAM_AUTH_ADMIN_TOKEN is intentionally NOT consulted here (FR-3 + FR-5). - internal/handlers/engramcore/grpcpool.go: connKey gains a tokenHash field (sha256-of-token, first 16 hex chars). Two sessions on the same workstation with distinct keycards no longer share a pooled connection (FR-7 / ADR-005). Empty token -> empty hash, allowing the no-auth degenerate case for tests. - internal/handlers/serverevents/bridge.go: NewBridge reads config.EnvWorkstationToken. The pre-v6 admin-token name is dropped here too (FR-5: no legacy fallback chains). - cmd/engram/main.go: NEW startupGate() runs before any module init. With a configured server URL but empty ENGRAM_TOKEN -> single stderr line + os.Exit(1) (FR-4 / ADR-005). With no server URL configured the gate is silent, allowing local-only loom_* flows. daemonVersion bumped to v6.0.0. Build + tests green across auth / grpcserver / worker / engramcore / serverevents / config packages. Refs: spec.md FR-3, FR-4, FR-5, FR-7, US3; plan.md Phase 4 (T4.1..T4.6).
Phase 5 of auth-two-tier-tokens (v6.0.0): - plugin/engram/.mcp.json: env key ENGRAM_AUTH_ADMIN_TOKEN -> ENGRAM_TOKEN. - plugin/engram/.claude-plugin/plugin.json: version 5.2.5 -> 6.0.0. Description leads with BREAKING change banner. userConfig.api_token description rewritten to make the dashboard /tokens issuance flow explicit and to forbid pasting the operator key. api_token now required. - plugin/engram/scripts/run-engram.js: warning text points users to <serverURL>/tokens for keycard issuance. Active warning when ENGRAM_AUTH_ADMIN_TOKEN is set on a workstation (forbidden in v6). - plugin/engram/commands/setup.md: full v6 walkthrough — issue keycard via dashboard, paste, restart. Refuses operator-key paste at validation step. - README.md: NEW "Two-Tier Token Model" section in What's New; v6.0.0 highlight added. - docs/DEPLOYMENT.md: NEW Token Model section explaining the host-pinned separation between ENGRAM_AUTH_ADMIN_TOKEN (server-host only) and ENGRAM_TOKEN (workstation only). - CHANGELOG.md: NEW v6.0.0 entry with BREAKING CHANGES, migration steps, Added/Removed sections. Build + tests green. Phase 5 closes the v6 spec implementation. Refs: spec.md FR-3, FR-8, US1, US2; plan.md Phase 5 (T5.1..T5.8).
|
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:
WalkthroughВыпущен v6.0.0: введена двухуровневая модель токенов (server admin vs per‑workstation keycards), общий auth.Validator с единым поведением для HTTP и gRPC, контекстная Identity, fail‑fast стартап при заданном ENGRAM_URL и пустом ENGRAM_TOKEN, плюс соответствующие изменения документации и плагина. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant "gRPC Server"
participant Validator
participant "Token Store"
Client->>"gRPC Server": RPC с Bearer Token (metadata)
"gRPC Server"->>"gRPC Server": extractBearer(token)
alt token == ""
"gRPC Server"-->>Client: Unauthenticated (ErrEmptyToken)
else
"gRPC Server"->>Validator: Validate(ctx, token)
alt master token matches
Validator-->>"gRPC Server": Identity(RoleAdmin, SourceMaster)
else
Validator->>Validator: check shape/prefix (engram_)
Validator->>"Token Store": FindByPrefix(prefix)
"Token Store"-->>Validator: candidates[]
alt bcrypt match найден
Validator-->>"gRPC Server": Identity(RoleX, SourceClient, KeycardID)
else
Validator-->>"gRPC Server": ErrInvalidCredentials
"gRPC Server"-->>Client: Unauthenticated ("invalid token")
end
end
alt auth success
"gRPC Server"->>"gRPC Server": WithIdentity(ctx, identity)
"gRPC Server"-->>Client: Выполнение хендлера / ответ
end
end
Estimated code review effort🎯 4 (Сложно) | ⏱️ ~60 минут Possibly related PRs
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 unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a two-tier token authentication model (v6.0.0), distinguishing between server-side operator keys and workstation-specific keycards. It introduces a centralized internal/auth package to unify validation across HTTP and gRPC, adds a fail-fast startup check for missing credentials, and updates the plugin and documentation to reflect these breaking changes. Review feedback identifies unreachable error handling for revoked tokens in the validator and suggests ensuring consistent JSON content-types in authentication error responses.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
README.md (2)
175-177:⚠️ Potential issue | 🟡 MinorНесоответствие: Quick Start использует устаревший
ENGRAM_AUTH_ADMIN_TOKENдля клиента.Согласно v6 модели, рабочие станции должны использовать
ENGRAM_TOKEN. Строка 176 показываетENGRAM_AUTH_ADMIN_TOKEN, что противоречит документации в строках 43-48 этого же файла.Предлагаемое исправление
# Linux/macOS: add to shell profile # Windows: set as System Environment Variables ENGRAM_URL=http://your-server:37777/mcp -ENGRAM_AUTH_ADMIN_TOKEN=your-admin-token +ENGRAM_TOKEN=your-worker-keycard🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 175 - 177, The Quick Start env example uses the outdated variable ENGRAM_AUTH_ADMIN_TOKEN; update the snippet to use ENGRAM_TOKEN instead so it matches the v6 model and the other documentation in this file—replace ENGRAM_AUTH_ADMIN_TOKEN with ENGRAM_TOKEN in the example and ensure any accompanying text or examples referencing ENGRAM_AUTH_ADMIN_TOKEN are updated to ENGRAM_TOKEN for consistency.
193-194:⚠️ Potential issue | 🟡 MinorЕщё одна ссылка на устаревший
ENGRAM_AUTH_ADMIN_TOKENв разделе Installation.Аналогично Quick Start, здесь также используется admin token вместо worker keycard.
Предлагаемое исправление
# Set environment variables first ENGRAM_URL=http://your-server:37777/mcp -ENGRAM_AUTH_ADMIN_TOKEN=your-admin-token +ENGRAM_TOKEN=your-worker-keycard🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 193 - 194, В разделе Installation замените устаревшую переменную окружения ENGRAM_AUTH_ADMIN_TOKEN на переменную для worker keycard (например ENGRAM_AUTH_WORKER_KEYCARD) и обновите пример значения соответственно (worker keycard вместо "your-admin-token"); найдите и исправьте строку с ENGRAM_URL/ENGRAM_AUTH_ADMIN_TOKEN и приведите её к виду ENGRAM_URL=http://your-server:37777/mcp и ENGRAM_AUTH_WORKER_KEYCARD=your-worker-keycard, чтобы соответствовать использованию worker keycard как в Quick Start.docs/DEPLOYMENT.md (1)
139-147:⚠️ Potential issue | 🟡 MinorНесоответствие: устаревшие ссылки на
ENGRAM_API_TOKEN.В разделе "Client Setup" используется
ENGRAM_API_TOKEN(строки 141, 146), который был удалён в v6. Согласно CHANGELOG.md и новой модели токенов, клиенты должны использоватьENGRAM_TOKEN.Предлагаемое исправление
**macOS / Linux** (`~/.bashrc` or `~/.zshrc`): ```bash export ENGRAM_URL=http://your-server:37777/mcp - export ENGRAM_API_TOKEN=your-secret-token + export ENGRAM_TOKEN=your-worker-keycard-tokenWindows (PowerShell as admin):
[Environment]::SetEnvironmentVariable("ENGRAM_URL", "http://your-server:37777/mcp", "User") - [Environment]::SetEnvironmentVariable("ENGRAM_API_TOKEN", "your-secret-token", "User") + [Environment]::SetEnvironmentVariable("ENGRAM_TOKEN", "your-worker-keycard-token", "User")</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/DEPLOYMENT.mdaround lines 139 - 147, Update the "Client Setup"
examples to use the new token name: replace any occurrence of ENGRAM_API_TOKEN
with ENGRAM_TOKEN and change the example value from "your-secret-token" to
"your-worker-keycard-token"; ensure both the bash export line and the PowerShell
[Environment]::SetEnvironmentVariable call reflect ENGRAM_TOKEN and the new
example value so documentation matches the v6 token model.</details> </blockquote></details> </blockquote></details>🧹 Nitpick comments (1)
internal/auth/context_test.go (1)
18-21: Проверьте в round-trip ещё иKeycardID.Сейчас тест фиксирует только
RoleиSource, поэтому регрессия в переносеKeycardIDчерезWithIdentity/IdentityFromостанется незамеченной.Минимальное усиление теста
assert.True(t, ok) assert.Equal(t, auth.RoleReadWrite, got.Role) assert.Equal(t, auth.SourceClient, got.Source) +assert.Equal(t, "uuid-1", got.KeycardID)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/auth/context_test.go` around lines 18 - 21, Тест round-trip сейчас проверяет только Role и Source, добавьте проверку на корректную передачу KeycardID через WithIdentity/IdentityFrom: when building the identity in the test (the code that calls WithIdentity) set a known KeycardID and then assert that got.KeycardID equals that expected value (alongside existing assertions for auth.RoleReadWrite and auth.SourceClient) so regressions in KeycardID propagation are caught.🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed. Inline comments: In `@CHANGELOG.md`: - Around line 18-19: В CHANGELOG.md найдена ссылка на несуществующий файл спецификации `.agent/specs/auth-two-tier-tokens/spec.md`; исправьте это либо создав требуемую спецификацию по этому пути с содержимым spec.md, либо удалив/заменив ссылку в CHANGELOG.md; при создании файла убедитесь, что директории `.agent/specs/auth-two-tier-tokens/` существуют и файл называется spec.md, при удалении — обновите сопроводительный текст в CHANGELOG.md чтобы он корректно отражал изменения. In `@internal/auth/validator.go`: - Around line 123-129: При проверке токена не доверяй свободному тексту в candidates[i].Scope: вместо прямого возврата Client(candidates[i].Scope, candidates[i].ID) принимай только заранее определённые значения scope (например "read", "write", "service" или список, используемый в IsAdmin/role-checks); если scope не в белом списке — возвращай ошибку валидации. Изменения: в том месте, где bcrypt совпал (в цикле по candidates и перед вызовом Client), проверяй candidates[i].Scope против констант/перечисления и только при совпадении строь Identity через Client, иначе возвращай явную ошибку данных. In `@internal/grpcserver/server.go`: - Around line 83-89: SetValidator currently writes to s.validator without synchronization causing data races because interceptors read s.validator on request goroutines; make access safe by replacing the plain field with a concurrency-safe holder (e.g. atomic.Value) or guarding it with a mutex: change Server.validator to hold an atomic.Value (storing *auth.Validator) or add a sync.RWMutex, update SetValidator to call validatorHolder.Store(v) (or lock+assign) and update all interceptor/read sites to use validatorHolder.Load() (or RLock()+read) and nil-check before calling s.validator.Validate(...). Ensure the readers never assume non-nil and handle nil validator gracefully. In `@internal/handlers/engramcore/grpcpool.go`: - Around line 21-33: The connKey struct must include the ENGRAM_TLS_CA identity so connections built with different CA configs aren’t reused; add a new field (e.g. tlsCAPath or tlsCAHash) to connKey and ensure getOrDialGRPC (and any pool lookups/constructors that create connKey) populates that field (compute a short hash of the ENGRAM_TLS_CA path/value if you want constant length) so the key distinguishes "custom-ca" variants; update any places that compare or construct connKey (and related comments) so the CA value/hash is part of the pool lookup. In `@internal/handlers/serverevents/bridge.go`: - Around line 108-118: The current env-var precedence in bridge.go is inverted compared to the rest of the daemon: change the server URL lookup to prefer config.EnvServerURL (ENGRAM_SERVER_URL) first and only fall back to config.EnvServerURLAlt (ENGRAM_URL), keeping the existing deprecation logger.Warn message when the fallback is used; update the serverURL assignment logic that defines the serverURL variable (used by NewBridge/bridge initialization) to reflect that order so tools/call and serverevents bridge use the same backend. In `@internal/worker/handlers_auth.go`: - Around line 291-301: The 403 branch in requireSessionAdmin currently uses http.Error which forces text/plain and adds a newline, so replace that with a proper JSON response: set Content-Type: application/json on the ResponseWriter, write the 403 status with w.WriteHeader, and encode the error body as a JSON object (e.g., {"error":"issuance requires browser session — log in to the dashboard at /login"}) using json.NewEncoder or equivalent; update only the 403 branch in requireSessionAdmin to ensure consistent JSON responses across the API. In `@internal/worker/middleware.go`: - Around line 383-388: The auto-provisioning creates a user with role "operator" which authpkg.Session treats as non-admin, causing IsAdmin/IsSessionAdmin checks to fail; change the provisioning so uStore.CreateUser(authentikEmail, "", "operator") creates the user with an admin role (e.g., "admin"), or ensure authpkg.Session(...) returns an admin session when authentikAutoProvision is true—update the call in the authentikAutoProvision block (around uStore.CreateUser and authpkg.Session) to produce an admin session for the newly created user so session-only endpoints work. - Around line 320-327: Validator.Validate возвращает и аутентификационные ошибки, и внутренние ошибки уровня хранилища/bcrypt, поэтому нельзя всегда отвечать 401; поменяй обработку результата validator.Validate в middleware.go так, чтобы сначала отличать внутренние ошибки (используя errors.Is/вложенные ошибки от store/bcrypt или конкретные sentinel errors, например ошибки из пакета store/bcrypt, и т.д.) и для них логировать и отвечать Internal Server Error (500), а для реальных ошибок авторизации — возвращать 401; обнови вызов log и http.Error соответственно (оставить log.Warn/Err для 401, а для внутренних ошибок использовать log.Error и 500), и не забудь добавить импорт "errors". --- Outside diff comments: In `@docs/DEPLOYMENT.md`: - Around line 139-147: Update the "Client Setup" examples to use the new token name: replace any occurrence of ENGRAM_API_TOKEN with ENGRAM_TOKEN and change the example value from "your-secret-token" to "your-worker-keycard-token"; ensure both the bash export line and the PowerShell [Environment]::SetEnvironmentVariable call reflect ENGRAM_TOKEN and the new example value so documentation matches the v6 token model. In `@README.md`: - Around line 175-177: The Quick Start env example uses the outdated variable ENGRAM_AUTH_ADMIN_TOKEN; update the snippet to use ENGRAM_TOKEN instead so it matches the v6 model and the other documentation in this file—replace ENGRAM_AUTH_ADMIN_TOKEN with ENGRAM_TOKEN in the example and ensure any accompanying text or examples referencing ENGRAM_AUTH_ADMIN_TOKEN are updated to ENGRAM_TOKEN for consistency. - Around line 193-194: В разделе Installation замените устаревшую переменную окружения ENGRAM_AUTH_ADMIN_TOKEN на переменную для worker keycard (например ENGRAM_AUTH_WORKER_KEYCARD) и обновите пример значения соответственно (worker keycard вместо "your-admin-token"); найдите и исправьте строку с ENGRAM_URL/ENGRAM_AUTH_ADMIN_TOKEN и приведите её к виду ENGRAM_URL=http://your-server:37777/mcp и ENGRAM_AUTH_WORKER_KEYCARD=your-worker-keycard, чтобы соответствовать использованию worker keycard как в Quick Start. --- Nitpick comments: In `@internal/auth/context_test.go`: - Around line 18-21: Тест round-trip сейчас проверяет только Role и Source, добавьте проверку на корректную передачу KeycardID через WithIdentity/IdentityFrom: when building the identity in the test (the code that calls WithIdentity) set a known KeycardID and then assert that got.KeycardID equals that expected value (alongside existing assertions for auth.RoleReadWrite and auth.SourceClient) so regressions in KeycardID propagation are caught.🪄 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:
d19a73f8-722d-4b56-a24c-a3eefb70d81e📒 Files selected for processing (24)
CHANGELOG.mdREADME.mdcmd/engram/main.godocs/DEPLOYMENT.mdinternal/auth/context.gointernal/auth/context_test.gointernal/auth/errors.gointernal/auth/identity.gointernal/auth/validator.gointernal/auth/validator_test.gointernal/config/envnames.gointernal/grpcserver/auth_interceptor_test.gointernal/grpcserver/server.gointernal/grpcserver/sync_project_state.gointernal/handlers/engramcore/grpcpool.gointernal/handlers/engramcore/tools.gointernal/handlers/serverevents/bridge.gointernal/worker/handlers_auth.gointernal/worker/middleware.gointernal/worker/service.goplugin/engram/.claude-plugin/plugin.jsonplugin/engram/.mcp.jsonplugin/engram/commands/setup.mdplugin/engram/scripts/run-engram.js
The .tpl was last updated for the pre-shadcn UI; recent SystemView / IssueDetailView / TokensView / VaultView refactor added 12+ deps to ui/package.json (reka-ui, lucide-vue-next, marked, dompurify, shiki, @shikijs/transformers, @vueuse/core, clsx, tailwind-merge, class-variance-authority, tailwindcss-animate, vue-sonner, font packages) that never made it into the .tpl. goreleaser regenerates ui/package.json from .tpl in its before-hook, then runs npm install + vue-tsc — vue-tsc fails with TS2307 across ~30 files because the deps are missing. The Release CI job has been red since v5.2.5. This is the pre-existing block flagged in plan.md U3 — fixing it here so the v6.0.0 tag does not inherit the failure. Docker CI (which uses ui/package.json directly) is unaffected.
MEDIUM-1 (data race): grpcserver.Server gains sync.RWMutex; SetValidator locks for swap, validateBearer reads through currentValidator() under RLock. Matches TokenAuth.mu pattern. MEDIUM-2 (duplicate constants): export TokenRawPrefix / TokenPrefixLen / TokenMinLen from internal/auth as the single source of truth. internal/worker/handlers_auth.go now aliases them locally — editing the literal values is now a contract change at the auth package, not a silent drift between producer and consumer. MEDIUM-3 (Content-Type): requireSessionAdmin emits proper application/json + json-encoded body for 403 responses (was text/plain with JSON-shaped string). LOW-1: serverevents/bridge.go token field comment was stale (ENGRAM_AUTH_ADMIN_TOKEN). Fixed to ENGRAM_TOKEN. LOW-2: middleware.go startup warn used pre-v5 ENGRAM_API_TOKEN env name. Updated to ENGRAM_AUTH_ADMIN_TOKEN. LOW-3: validateBearer ErrRevoked branch documented as currently unreachable (FindByPrefix filters revoked at SQL layer) but kept as the explicit mapping for future contract changes. LOW-4: NEW test TestValidate_EmptyMaster_KeycardStillWorks covers the documented "Tier-2-only" mode (masterToken == "", valid keycard). Build + tests green across all touched packages. Refs: PR #208 local code review (nvmd-platform:code-reviewer subagent).
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/worker/handlers_auth.go (1)
313-325:⚠️ Potential issue | 🟡 MinorSwagger-описание auth-gate уже расходится с фактическими ответами.
requireSessionAdmin()теперь добавляет для этого хендлера401и JSON-объект на403, но аннотация всё ещё описывает только string-403и вообще не документирует401. Из-за этого сгенерированные клиенты и docs будут ожидать неверный контракт; тот же дрейф теперь есть и у list/revoke/stats, которые вызывают этот же guard.Минимальное обновление аннотаций
// `@Success` 200 {object} map[string]interface{} // `@Failure` 400 {string} string "bad request" -// `@Failure` 403 {string} string "forbidden — issuance requires browser session" +// `@Failure` 401 {string} string "unauthorized" +// `@Failure` 403 {object} map[string]string "forbidden — issuance requires browser session" // `@Failure` 409 {string} string "conflict" // `@Failure` 500 {string} string "internal error"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/worker/handlers_auth.go` around lines 313 - 325, Swagger annotations for handleCreateToken (and the other handlers protected by requireSessionAdmin such as the list/revoke/stats handlers) are out of sync: requireSessionAdmin now can return a 401 and returns JSON for 403 rather than a plain string. Update the godoc comments for handleCreateToken and the list/revoke/stats handlers to add a `@Failure` 401 entry that documents a JSON error response (e.g. {object} map[string]interface{} or your shared ErrorResponse type) and change the existing `@Failure` 403 to document a JSON object (not string); ensure the documented response schema matches the actual error payload your auth guard returns.
♻️ Duplicate comments (2)
internal/handlers/serverevents/bridge.go (1)
108-113:⚠️ Potential issue | 🟠 MajorНужно выровнять приоритет URL-переменных с канонической схемой.
Сейчас на Line 108 bridge сначала читает
EnvServerURLAlt(ENGRAM_SERVER_URL), а только потом на Line 110 —EnvServerURL(ENGRAM_URL). Это расходится сinternal/config/envnames.goиstartupGate()вcmd/engram/main.go, где канонический приоритет обратный. При одновременной установке обеих переменных процесс может пойти в разные backend’ы в разных подсистемах.💡 Предлагаемое исправление
- serverURL := os.Getenv(config.EnvServerURLAlt) + serverURL := os.Getenv(config.EnvServerURL) if serverURL == "" { - if fallback := os.Getenv(config.EnvServerURL); fallback != "" { + if fallback := os.Getenv(config.EnvServerURLAlt); fallback != "" { serverURL = fallback - logger.Warn("ENGRAM_URL is deprecated for bridge configuration; please use ENGRAM_SERVER_URL instead") + logger.Warn("ENGRAM_SERVER_URL is deprecated for bridge configuration; please use ENGRAM_URL instead") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/handlers/serverevents/bridge.go` around lines 108 - 113, The bridge currently reads EnvServerURLAlt into serverURL first and only then falls back to EnvServerURL, which reverses the canonical priority; change the logic in bridge.go so serverURL is populated from config.EnvServerURL (ENGRAM_URL) first and only if empty use config.EnvServerURLAlt (ENGRAM_SERVER_URL), and update the logger.Warn message to reflect the correct deprecated/primary variable names; refer to the serverURL variable and the EnvServerURL/EnvServerURLAlt symbols when making this change.internal/auth/validator.go (1)
127-133:⚠️ Potential issue | 🟠 MajorНе доверяйте
api_tokens.scopeкак готовой runtime-роли.
Client(candidates[i].Scope, ...)всё ещё превращает произвольныйtextиз БД вIdentity.Role. Значение вроде"admin"сразу начнёт проходить черезIdentity.IsAdmin(), так что повреждённые или вручную вставленные строки вapi_tokens.scopeдают эскалацию прав.Минимальная защита на месте чтения
if err == nil { - return Client(candidates[i].Scope, candidates[i].ID), nil + switch Role(candidates[i].Scope) { + case RoleReadWrite, RoleReadOnly: + return Client(candidates[i].Scope, candidates[i].ID), nil + default: + return Identity{}, fmt.Errorf("auth: unexpected token scope %q", candidates[i].Scope) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/auth/validator.go` around lines 127 - 133, Кратко: текущая строка Client(candidates[i].Scope, candidates[i].ID) напрямую превращает сырой текст из api_tokens.scope в роль и позволяет эскалацию; нужно заменить это на безопасную валидацию/маппинг строки в заранее определённый набор ролей. Исправьте код в месте сравнения токена (в блоке, где используется candidates и вызывается Client): вместо прямого приведения строки реализуйте/вызовите функцию вроде parseRole(scope string) или mapScopeToRole(scope string) которая проверяет значение против белого списка допустимых ролей (например "user","admin",...) и возвращает конкретный тип/enum Role или ошибку/дефолтную роль; при неизвестном или недопустимом значении отказывайте в аутентификации или назначайте минимальные права, не позволяя произвольной строке пройти в Identity.Role.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/auth/validator.go`:
- Around line 113-118: Tighten the "shape gate" in the validator so it only
accepts the exact token format (TokenRawPrefix followed by 32 hex characters)
before calling FindByPrefix: in the function handling raw tokens, replace the
current prefix/length checks (raw, TokenRawPrefix, TokenMinLen, TokenPrefixLen)
with a strict validation that raw == TokenRawPrefix + 32 chars and that those 32
chars are valid hex; if validation fails return ErrInvalidCredentials to
fail-closed and avoid unnecessary FindByPrefix/DB/bcrypt work.
In `@internal/grpcserver/server.go`:
- Around line 62-78: The issue: interceptors are only added in New() when
validator != nil so calling SetValidator(v) later won't enable auth; fix by
always registering grpc.UnaryInterceptor(srv.authInterceptor) and
grpc.StreamInterceptor(srv.streamAuthInterceptor) in New() regardless of
validator value and make authInterceptor and streamAuthInterceptor check
srv.validator at runtime to bypass when nil, or alternatively modify
SetValidator to reject changing from nil to non-nil after server creation (e.g.,
return an error) — update New, SetValidator, authInterceptor, and
streamAuthInterceptor accordingly to implement one consistent strategy.
---
Outside diff comments:
In `@internal/worker/handlers_auth.go`:
- Around line 313-325: Swagger annotations for handleCreateToken (and the other
handlers protected by requireSessionAdmin such as the list/revoke/stats
handlers) are out of sync: requireSessionAdmin now can return a 401 and returns
JSON for 403 rather than a plain string. Update the godoc comments for
handleCreateToken and the list/revoke/stats handlers to add a `@Failure` 401 entry
that documents a JSON error response (e.g. {object} map[string]interface{} or
your shared ErrorResponse type) and change the existing `@Failure` 403 to document
a JSON object (not string); ensure the documented response schema matches the
actual error payload your auth guard returns.
---
Duplicate comments:
In `@internal/auth/validator.go`:
- Around line 127-133: Кратко: текущая строка Client(candidates[i].Scope,
candidates[i].ID) напрямую превращает сырой текст из api_tokens.scope в роль и
позволяет эскалацию; нужно заменить это на безопасную валидацию/маппинг строки в
заранее определённый набор ролей. Исправьте код в месте сравнения токена (в
блоке, где используется candidates и вызывается Client): вместо прямого
приведения строки реализуйте/вызовите функцию вроде parseRole(scope string) или
mapScopeToRole(scope string) которая проверяет значение против белого списка
допустимых ролей (например "user","admin",...) и возвращает конкретный тип/enum
Role или ошибку/дефолтную роль; при неизвестном или недопустимом значении
отказывайте в аутентификации или назначайте минимальные права, не позволяя
произвольной строке пройти в Identity.Role.
In `@internal/handlers/serverevents/bridge.go`:
- Around line 108-113: The bridge currently reads EnvServerURLAlt into serverURL
first and only then falls back to EnvServerURL, which reverses the canonical
priority; change the logic in bridge.go so serverURL is populated from
config.EnvServerURL (ENGRAM_URL) first and only if empty use
config.EnvServerURLAlt (ENGRAM_SERVER_URL), and update the logger.Warn message
to reflect the correct deprecated/primary variable names; refer to the serverURL
variable and the EnvServerURL/EnvServerURLAlt symbols when making this change.
🪄 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: 0d1663f5-b691-4604-bf3a-4c0fed7f6209
📒 Files selected for processing (7)
internal/auth/validator.gointernal/auth/validator_test.gointernal/grpcserver/server.gointernal/handlers/serverevents/bridge.gointernal/worker/handlers_auth.gointernal/worker/middleware.goui/package.json.tpl
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/auth/validator_test.go
- internal/worker/middleware.go
…d test Adds tests/critical/auth_two_tier_test.go (//go:build critical) — the first @critical end-to-end test in this repo. Resolves the MISSING_SUITE verdict from the critical-suite skill before v6.0.0 release. The test exercises the v6 auth boundary across BOTH transports (gRPC + HTTP) using the SAME validator chain — which is precisely the surface PR #203 silently broke. With this test in place, a future regression where gRPC and HTTP diverge on validator wiring fails CI before the release tag is cut. Sub-tests: - gRPC bufconn server accepts operator key (FR-1 tier 1) - gRPC bufconn server accepts dashboard-issued keycard (FR-2 symmetry — the assertion PR #203 would have flipped to FAIL) - gRPC rejects garbage / missing bearer with Unauthenticated - HTTP httptest server bearer arm: master + keycard accepted, garbage rejected - Anti-stub mutation guard: replacing the keycard store with always-fail flips the keycard-success assertion, proving the test exercises the store path and is not a tautology Run via: go test -tags=critical ./tests/critical/... Build tag gates the suite away from default `go test ./...`.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/critical/auth_two_tier_test.go (2)
76-80: Уберите магические индексы7:15при расчёте префикса токена.Сейчас префикс зависит от захардкоженных срезов; при изменении формата токена тест сломается неочевидно. Лучше вычислять через константы
auth.TokenRawPrefixиauth.TokenPrefixLen.Предлагаемое исправление
const ( operatorKey = "operator-secret-do-not-reuse" clientRaw = "engram_critic1c000000000000ffeed" ) + prefixStart := len(auth.TokenRawPrefix) + prefixEnd := prefixStart + auth.TokenPrefixLen + clientPrefix := clientRaw[prefixStart:prefixEnd] @@ store := &critStore{ rows: map[string][]gormdb.APIToken{ - clientRaw[7:15]: {{ + clientPrefix: {{ ID: "uuid-critical-1", Name: "critical-test-keycard", TokenHash: string(hash), - TokenPrefix: clientRaw[7:15], + TokenPrefix: clientPrefix, Scope: "read-write", }}, }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/critical/auth_two_tier_test.go` around lines 76 - 80, The test uses a hardcoded slice clientRaw[7:15] to build TokenPrefix which is brittle; replace the magic indices by computing the start as len(auth.TokenRawPrefix) and the end as start + auth.TokenPrefixLen so TokenPrefix is derived as clientRaw[start:end]; update the expectation where TokenPrefix is set (in the table/struct literal using clientRaw[7:15]) to use that computed slice and import/reference auth.TokenRawPrefix and auth.TokenPrefixLen accordingly.
136-151: Проверяйте gRPC-ошибки по коду статуса, а не по подстроке.
require.Contains(t, err.Error(), "Unauthenticated")хрупко к изменениям текста ошибки. Для стабильности и согласованности с остальной кодовой базой используйтеstatus.Code(err) == codes.Unauthenticated. Потребуется добавить импорты:"google.golang.org/grpc/codes" "google.golang.org/grpc/status"Этот паттерн используется во всех тестах gRPC-обработчиков в проекте.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/critical/auth_two_tier_test.go` around lines 136 - 151, The tests in auth_two_tier_test.go check gRPC errors by substring (require.Contains(t, err.Error(), "Unauthenticated")) which is fragile; update both test cases (the anonymous t.Run blocks that call client.Initialize) to assert the gRPC status code using status.Code(err) == codes.Unauthenticated (or require.Equal(t, codes.Unauthenticated, status.Code(err))) and add the imports "google.golang.org/grpc/codes" and "google.golang.org/grpc/status" to the test file so the status.Code and codes constants are available.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/critical/auth_two_tier_test.go`:
- Line 7: В комментарии строки "//\tgo test -tags=critical ./tests/critical/..."
добавьте рядом стандартный прогон базового набора тестов — вставьте либо вторую
комментированную строку "//\tgo test ./..." либо объедините в одну строку, чтобы
оба запуска присутствовали; найдите эту строку в
tests/critical/auth_two_tier_test.go и обновите комментарий соответственно.
- Around line 245-246: The call to grpcserver.New is ignoring its error return
(srv, _ := grpcserver.New(stubMCPHandler{}, v)); change this to capture and
check the error from grpcserver.New (e.g., srv, err := grpcserver.New(...)) and
fail fast when err != nil by returning the error or calling t.Fatalf /
log.Fatalf with the error message so the test fails with a clear cause instead
of letting Serve() later fail mysteriously; update the code around
grpcserver.New and the goroutine that calls srv.Serve(lis) accordingly.
---
Nitpick comments:
In `@tests/critical/auth_two_tier_test.go`:
- Around line 76-80: The test uses a hardcoded slice clientRaw[7:15] to build
TokenPrefix which is brittle; replace the magic indices by computing the start
as len(auth.TokenRawPrefix) and the end as start + auth.TokenPrefixLen so
TokenPrefix is derived as clientRaw[start:end]; update the expectation where
TokenPrefix is set (in the table/struct literal using clientRaw[7:15]) to use
that computed slice and import/reference auth.TokenRawPrefix and
auth.TokenPrefixLen accordingly.
- Around line 136-151: The tests in auth_two_tier_test.go check gRPC errors by
substring (require.Contains(t, err.Error(), "Unauthenticated")) which is
fragile; update both test cases (the anonymous t.Run blocks that call
client.Initialize) to assert the gRPC status code using status.Code(err) ==
codes.Unauthenticated (or require.Equal(t, codes.Unauthenticated,
status.Code(err))) and add the imports "google.golang.org/grpc/codes" and
"google.golang.org/grpc/status" to the test file so the status.Code and codes
constants are available.
🪄 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: b345f61d-f16e-4777-9b8b-df81d2c1fa97
📒 Files selected for processing (1)
tests/critical/auth_two_tier_test.go
First emulation playbook for engram, scoped to v6.0.0 auth-two-tier-tokens release surfaces (server, plugin install, CLI proxy, dashboard tokens). Resolves the MISSING verdict from the emulation-playbook skill before v6.0.0 release. Future releases extend the scenario list per /emulation-playbook --add. Customer-mode walkthrough on this commit produced verdict PASS_WITH_DEFERRED: - Build, FR-4 fail-fast (server side), FR-4 fail-fast (client side) all PASS - Two UX surprises noted (no --help handler, hardcoded keycard URL in FATAL message) — queued as low-effort follow-ups, not in scope of v6.0.0 - Three scenarios (live-stand happy path, dashboard /tokens, plugin install) deferred to user-driven walkthrough before tag — they do not exercise the v6 auth contract; they exercise unchanged infrastructure surfaces Run report: .agent/reports/emulation-playbook-run-2026-04-26.md (gitignored, working state).
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/PRODUCTION-TESTING-PLAYBOOK.md (1)
124-124: Точка диагностики для/api/auth/tokens500` выглядит смещеннойДля сигнала про
/api/auth/tokensлогичнее первым местом проверки указать HTTP-слой (internal/worker/handlers_auth.goи связанный middleware/validator), а неinternal/grpcserver/server.go. Иначе расследование пойдет в нерелевантный путь.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/PRODUCTION-TESTING-PLAYBOOK.md` at line 124, Строка диагностики для `/api/auth/tokens` 500 указывает неправильно приоритет проверки; обновите таблицу так, чтобы первым местом проверки был HTTP-слой: укажите `internal/worker/handlers_auth.go` и связанный middleware/validator (включая функции/методы валидации, используемые в middleware) как первичную точку диагностики, а уже затем `internal/grpcserver/server.go` / `SetValidator` как вторичную проверку, чтобы расследование начиналось с релевантного кода.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/PRODUCTION-TESTING-PLAYBOOK.md`:
- Line 71: В failure signals текст упоминает deprecated-переменную как
ENGRAM_AUTH_TOKEN, но релиз/миграция использует ENGRAM_AUTH_ADMIN_TOKEN ↔
ENGRAM_TOKEN; замените или уточните упоминание на точное имя переменной,
используемое в кодовой базе/миграции (например, ENGRAM_AUTH_ADMIN_TOKEN если это
действительно deprecated имя, либо ENGRAM_TOKEN если это новое имя) и добавьте
краткую пометку о переименовании (old → new) чтобы исключить двусмысленность при
диагностике.
- Line 60: В шаге S2 замените неверное имя переменной окружения
ENGRAM_DATABASE_URL на правильное DATABASE_DSN (используется в блоке env
overrides в internal/config/config.go), чтобы сценарий использовал ту же
переменную, что и серверная конфигурация; обновите описание шага и любые
примеры/README в docs/PRODUCTION-TESTING-PLAYBOOK.md, чтобы ссылались на
DATABASE_DSN (или поясните, что можно оставить пустым для sqlite по умолчанию).
---
Nitpick comments:
In `@docs/PRODUCTION-TESTING-PLAYBOOK.md`:
- Line 124: Строка диагностики для `/api/auth/tokens` 500 указывает неправильно
приоритет проверки; обновите таблицу так, чтобы первым местом проверки был
HTTP-слой: укажите `internal/worker/handlers_auth.go` и связанный
middleware/validator (включая функции/методы валидации, используемые в
middleware) как первичную точку диагностики, а уже затем
`internal/grpcserver/server.go` / `SetValidator` как вторичную проверку, чтобы
расследование начиналось с релевантного кода.
🪄 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: 4acb4cb6-72dd-4164-b9f9-dee4b547d2b6
📒 Files selected for processing (1)
docs/PRODUCTION-TESTING-PLAYBOOK.md
Addresses 3 of the 9 unresolved review comments on PR #208 — the highest- impact correctness ones: CRIT (CHANGELOG.md): the bullet referenced `.agent/specs/auth-two-tier- tokens/spec.md`, a path under .agent/ which is gitignored project-wide. Removed the dangling reference; the bullet still describes the BREAKING change without pretending the spec is committed. MAJOR (internal/grpcserver/server.go): SetValidator(v) could not enable auth after a server was started with validator==nil — the unary/stream interceptors were registered conditionally at New() time, so a later non-nil swap left RPCs unprotected. Fix: always register the interceptors; bypass them at runtime when currentValidator() returns nil. Construction-time and runtime-promotion paths now share one strategy. MAJOR (internal/auth/validator.go): shape gate accepted any token of the form "engram_" + ≥8 chars, leaving a cheap probe vector that ran a DB lookup + bcrypt loop on syntactically impossible candidates. Fix: require exact length (TokenTotalLen = 39) AND that the body is pure hex. Adds TokenTotalLen + isHex helper. TokenMinLen retained as a deprecated alias for compatibility with v6-prerelease imports. Test fixture rewrite — every test fixture now matches the strict shape: - internal/auth/validator_test.go (8 raw tokens) - internal/grpcserver/auth_interceptor_test.go (3 raw tokens) - tests/critical/auth_two_tier_test.go (1 raw token; also imports auth.TokenRawPrefix / auth.TokenPrefixLen instead of magic indices, resolving the matching nitpick). Verification: - go test ./... — full suite green - go test -tags=critical ./tests/critical/... — 6/6 sub-tests green - go build ./... — clean
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tests/critical/auth_two_tier_test.go (2)
254-255:⚠️ Potential issue | 🟠 MajorНе игнорируйте ошибку
grpcserver.New(...).На Line 254 ошибка отбрасывается; при проблеме инициализации тест падает позже и менее прозрачно.
Предлагаемый фикс
lis := bufconn.Listen(1 << 20) - srv, _ := grpcserver.New(stubMCPHandler{}, v) + srv, err := grpcserver.New(stubMCPHandler{}, v) + require.NoError(t, err) go func() { _ = srv.Serve(lis) }() - conn, err := grpc.NewClient( + conn, err := grpc.NewClient(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/critical/auth_two_tier_test.go` around lines 254 - 255, Ошибка в том, что результат вызова grpcserver.New(stubMCPHandler{}, v) игнорируется; нужно проверить и обработать возвращаемую ошибку, чтобы тест падал с понятным сообщением при неудачной инициализации. Измените создание сервера (grpcserver.New) так, чтобы вынимать оба значения (srv, err) и при err вызывать t.Fatalf или аналогичную фэйловую проверку (или require.NoError(t, err)) с информативным сообщением; оставьте существующий запуск goroutine со srv.Serve(lis) только после успешной проверки err.
7-7:⚠️ Potential issue | 🟡 MinorДобавьте в блок запуска тестов стандартный прогон
go test ./....Сейчас в инструкции есть только critical-run; добавьте рядом базовый прогон полного набора.
As per coding guidelines,
**/*_test.go: Run tests usinggo test ./...command.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/critical/auth_two_tier_test.go` at line 7, Update the test-run comment at the top that currently only shows "go test -tags=critical ./tests/critical/..." to also include the standard full-suite command; edit the commented line (the string "//\tgo test -tags=critical ./tests/critical/...") in tests/critical/auth_two_tier_test.go so it lists both commands, e.g. add "go test ./..." alongside the critical-run instruction to satisfy the guideline that tests in *_test.go include the standard "go test ./..." invocation.internal/auth/validator.go (1)
159-159:⚠️ Potential issue | 🟠 MajorНе принимайте
Scopeиз БД как доверенный ввод.На Line 159 произвольный
api_tokens.scopeнапрямую превращается вIdentity.Role. Нужно принимать только разрешённые scope и отклонять остальные.Предлагаемый фикс
if err == nil { - return Client(candidates[i].Scope, candidates[i].ID), nil + switch Role(candidates[i].Scope) { + case RoleReadWrite, RoleReadOnly: + return Client(candidates[i].Scope, candidates[i].ID), nil + default: + return Identity{}, ErrInvalidCredentials + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/auth/validator.go` at line 159, Не доверяйте полю candidates[i].Scope из БД: вместо прямого возврата Client(candidates[i].Scope, candidates[i].ID) валидируйте и маппьте значение scope на разрешённые роли; добавьте список допустимых scope или мапу (например allowedScopes / scopeToRole) и в функции, где используется candidates (ссылаясь на Client и candidates), проверяйте что candidates[i].Scope присутствует в этой структуре и преобразуйте в внутреннее значение роли (или возвращайте ошибку/отклоняйте токен) прежде чем вызывать Client(..., candidates[i].ID).
🧹 Nitpick comments (1)
internal/auth/validator_test.go (1)
40-47: Синхронизируйте fixture-валидацию с константамиauth.Token*.Сейчас helper привязан к
>=15иraw[7:15], хотя production shape уже строгий. Лучше использоватьauth.TokenRawPrefix,auth.TokenPrefixLen,auth.TokenTotalLen.Вариант правки
require.NoError(t, err) - require.True(t, strings.HasPrefix(raw, "engram_"), "test fixture: raw must start with engram_") - require.GreaterOrEqual(t, len(raw), 15, "test fixture: raw must be ≥ 15 chars") + require.True(t, strings.HasPrefix(raw, auth.TokenRawPrefix), "test fixture: invalid token prefix") + require.Equal(t, auth.TokenTotalLen, len(raw), "test fixture: invalid token length") return gormdb.APIToken{ ID: id, Name: "test-" + id, TokenHash: string(hash), - TokenPrefix: raw[7:15], + TokenPrefix: raw[len(auth.TokenRawPrefix) : len(auth.TokenRawPrefix)+auth.TokenPrefixLen], Scope: scope, Revoked: revoked, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/auth/validator_test.go` around lines 40 - 47, The test helper currently hardcodes the token shape (checks len>=15 and uses raw[7:15]) — update it to use the production constants auth.TokenRawPrefix, auth.TokenPrefixLen and auth.TokenTotalLen: assert strings.HasPrefix(raw, auth.TokenRawPrefix) and len(raw) == auth.TokenTotalLen (or >= if tests expect ≥), compute the TokenPrefix as raw[len(auth.TokenRawPrefix) : len(auth.TokenRawPrefix)+auth.TokenPrefixLen] and assign that to gormdb.APIToken.TokenPrefix; import the auth package if necessary and update assertion messages to reference the constants.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/auth/validator.go`:
- Around line 145-146: The call to v.store.FindByPrefix can panic when v.store
is nil; add a fail-closed nil check before that lookup in the validator method
containing the lines with v.store and FindByPrefix (e.g., where candidates, err
:= v.store.FindByPrefix(ctx, prefix) is called). If v.store == nil, return the
appropriate authentication failure/error immediately (and optionally log an
explanatory message) instead of proceeding to call FindByPrefix, ensuring the
function fails closed rather than panicking.
---
Duplicate comments:
In `@internal/auth/validator.go`:
- Line 159: Не доверяйте полю candidates[i].Scope из БД: вместо прямого возврата
Client(candidates[i].Scope, candidates[i].ID) валидируйте и маппьте значение
scope на разрешённые роли; добавьте список допустимых scope или мапу (например
allowedScopes / scopeToRole) и в функции, где используется candidates (ссылаясь
на Client и candidates), проверяйте что candidates[i].Scope присутствует в этой
структуре и преобразуйте в внутреннее значение роли (или возвращайте
ошибку/отклоняйте токен) прежде чем вызывать Client(..., candidates[i].ID).
In `@tests/critical/auth_two_tier_test.go`:
- Around line 254-255: Ошибка в том, что результат вызова
grpcserver.New(stubMCPHandler{}, v) игнорируется; нужно проверить и обработать
возвращаемую ошибку, чтобы тест падал с понятным сообщением при неудачной
инициализации. Измените создание сервера (grpcserver.New) так, чтобы вынимать
оба значения (srv, err) и при err вызывать t.Fatalf или аналогичную фэйловую
проверку (или require.NoError(t, err)) с информативным сообщением; оставьте
существующий запуск goroutine со srv.Serve(lis) только после успешной проверки
err.
- Line 7: Update the test-run comment at the top that currently only shows "go
test -tags=critical ./tests/critical/..." to also include the standard
full-suite command; edit the commented line (the string "//\tgo test
-tags=critical ./tests/critical/...") in tests/critical/auth_two_tier_test.go so
it lists both commands, e.g. add "go test ./..." alongside the critical-run
instruction to satisfy the guideline that tests in *_test.go include the
standard "go test ./..." invocation.
---
Nitpick comments:
In `@internal/auth/validator_test.go`:
- Around line 40-47: The test helper currently hardcodes the token shape (checks
len>=15 and uses raw[7:15]) — update it to use the production constants
auth.TokenRawPrefix, auth.TokenPrefixLen and auth.TokenTotalLen: assert
strings.HasPrefix(raw, auth.TokenRawPrefix) and len(raw) == auth.TokenTotalLen
(or >= if tests expect ≥), compute the TokenPrefix as
raw[len(auth.TokenRawPrefix) : len(auth.TokenRawPrefix)+auth.TokenPrefixLen] and
assign that to gormdb.APIToken.TokenPrefix; import the auth package if necessary
and update assertion messages to reference the constants.
🪄 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: 7ccfbf81-dcf6-4f49-8737-9c0623de69a3
📒 Files selected for processing (6)
CHANGELOG.mdinternal/auth/validator.gointernal/auth/validator_test.gointernal/grpcserver/auth_interceptor_test.gointernal/grpcserver/server.gotests/critical/auth_two_tier_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/grpcserver/server.go
…iew) Addresses 5 more PR #208 review threads. MAJOR (internal/auth/validator.go): keycard validation copied api_tokens.scope verbatim into Identity.Role. A row with scope="admin" (data corruption, malicious INSERT, future schema drift) would have promoted a worker keycard to admin. Fix: whitelist read-write/read-only, return an explicit "unexpected scope" error otherwise. MAJOR (internal/handlers/serverevents/bridge.go): the bridge read ENGRAM_SERVER_URL first, falling back to ENGRAM_URL — opposite to internal/config/envnames.go which declares ENGRAM_URL canonical. A mid-migration deploy with both vars set to different values would have served tools/call from one backend and the serverevents bridge from another. Aligned the precedence. MAJOR (internal/handlers/engramcore/grpcpool.go): connKey indexed only on (addr, tlsMode, tokenHash). Switching ENGRAM_TLS_CA from one non-empty path to another left tlsMode="custom-ca" and reused the connection bound to the old trust store. Added tlsCAHash (sha256 short hash, not the path itself — pool keys end up in heap dumps) so distinct custom-CA paths get distinct pool entries. MAJOR (internal/worker/middleware.go): bearer-arm reduced every validator error to 401 — including wrapped store/bcrypt failures. HTTP clients lost the auth-store-down signal that the gRPC interceptor already surfaces as codes.Internal. Fixed: errors.Is on ErrEmptyToken/ErrInvalidCredentials/ErrRevoked → 401; everything else → 500 with a distinct log message ("auth: store/bcrypt failure"). The errors import was missing and is now added. MAJOR (internal/worker/middleware.go, Authentik autoprovision): pushed back. The reviewer suggested creating Authentik-autoprovisioned users as "admin". The codebase intentionally distinguishes "admin" from "operator" (auth_handlers.go:428 enforces this 2-value list). Auto- elevating SSO callers to admin is a security downgrade — any user landing through the SSO endpoint would receive keycard-issuance privileges. Existing admins promote new users via PATCH /api/users/{id}/role. Inline comment added explaining the design; behavior unchanged. v5 issuance was master-token-bearer-only, so "operator" session users could not issue keycards in v5 either — no regression. MINOR (tests/critical/auth_two_tier_test.go): grpcserver.New returns (*grpc.Server, *grpcserver.Server) — NOT (*grpc.Server, error). The underscore is the *Server pointer, not a swallowed error. Inline comment added so the next reviewer does not have to re-discover the signature. Verification: - go test ./internal/... — full internal suite green (28 packages) - go test -tags=critical ./tests/critical/... — 6/6 sub-tests green - go build ./... — clean
Owner
Author
🤖 PR Review MCP State (auto-managed, do not edit){
"version": 2,
"parentChildren": {},
"resolvedNitpicks": {
"coderabbit-nitpick-fc403c45-76": {
"resolvedAt": "2026-04-26T18:10:19.982Z",
"resolvedBy": "agent"
},
"coderabbit-nitpick-45762e5a-136": {
"resolvedAt": "2026-04-26T18:10:28.689Z",
"resolvedBy": "agent"
},
"coderabbit-nitpick-bffd1f64-40": {
"resolvedAt": "2026-04-26T18:13:31.070Z",
"resolvedBy": "agent"
}
},
"updatedAt": "2026-04-26T18:13:31.677Z"
} |
CRIT (internal/auth/validator.go): Validate could panic with nil pointer dereference when v.store == nil and a non-master token was presented. Although production New() always passes a non-nil store, the test surface and the bootstrap window can produce a nil-store validator. Added explicit fail-closed branch returning ErrInvalidCredentials before the FindByPrefix call. MAJOR (docs/PRODUCTION-TESTING-PLAYBOOK.md): S2 step 2 referenced `ENGRAM_DATABASE_URL` — not the canonical name. internal/config reads `DATABASE_DSN`. Operators following the playbook would have hit a non-reproducible path. Fixed. MINOR (docs/PRODUCTION-TESTING-PLAYBOOK.md): the failure-signals mention of a deprecated env var was ambiguous. Clarified that the v6 server-host token is `ENGRAM_AUTH_ADMIN_TOKEN` (kept) and the workstation rename was `ENGRAM_AUTH_ADMIN_TOKEN` → `ENGRAM_TOKEN`. MINOR (internal/auth/validator_test.go makeKeycard helper): fixture validation hardcoded `len >= 15` and `raw[7:15]` despite the production shape now being strict (= TokenTotalLen + hex). Wired the helper to auth.TokenRawPrefix / TokenPrefixLen / TokenTotalLen so a future shape change cannot leave the test fixtures silently green. Verification: - go test ./internal/auth/... — green - go test -tags=critical ./tests/critical/... — 6/6 sub-tests green
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
1 participant
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.
Summary
Two-tier token authentication. Fixes the post-PR-203 regression (
loom_*-onlytool surface) by introducing a singleauth.Validatorshared by HTTP middleware and gRPC interceptor, and separating credentials into two host-pinned tiers.This is a breaking change. Plugin minor: 5.2.5 → 6.0.0.
What changed
internal/auth/Validator,Identity, sentinel errors, context helpers. Two-strategy chain: master constant-time compare + bcrypt prefix lookup. 97% coverage.internal/grpcserver/authInterceptorand NEWstreamAuthInterceptordelegate toauth.Validator. StreamingProjectEventsnow honours auth. Per-process token field replaced by*auth.Validator.internal/worker/middleware.goauth.Validator. Removed inlineauthenticateClientToken(DRY). Cookie/forward-auth paths unchanged. NEWauthSourceKey{}distinguishes session-cookie admins from bearer admins.internal/worker/handlers_auth.goPOST/GET/DELETE /api/auth/tokens(and…/stats) require admin session cookie. Bearer callers (operator key OR keycard) get403 issuance requires browser session.internal/handlers/engramcore/ENGRAM_TOKEN(notENGRAM_AUTH_ADMIN_TOKEN).connKeyextended withtokenHashso distinct keycards never share a pooled gRPC conn (FR-7).internal/handlers/serverevents/bridge.goENGRAM_TOKEN.cmd/engram/main.gostartupGate()— daemon exits 1 with single stderr line whenENGRAM_URLis set butENGRAM_TOKENis empty. Replaces silent graceful-degrade.daemonVersion→v6.0.0.internal/config/envnames.go.mcp.jsonenv:ENGRAM_AUTH_ADMIN_TOKEN→ENGRAM_TOKEN. Plugin version6.0.0.userConfig.api_token.required: true, description rewritten.run-engram.jswarns when operator key set on workstation.commands/setup.mdwalks user through dashboard issuance.README.md+docs/DEPLOYMENT.md+CHANGELOG.mddocument the two-tier model and migration steps.No new external dependencies.
Spec / plan / clarifications
.agent/specs/auth-two-tier-tokens/spec.md(8 FR, 6 NFR, 7 US, 5 clarifications resolved).agent/specs/auth-two-tier-tokens/plan.md(6 phases, computed parallelism).agent/specs/auth-two-tier-tokens/architecture.md(10 ADRs, mermaid).agent/specs/auth-two-tier-tokens/user_job_statement.md(5 verbatim quotes).agent/specs/auth-two-tier-tokens/clarification-report-2026-04-26.md.agent/specs/auth-two-tier-tokens/validation-report-2026-04-26.mdMigration
For users on plugin v5.x:
/plugin update engram@engram(after this PR ships v6.0.0).<server-url>/tokens, log in as admin, generate a fresh keycard./engram:setup, paste the keycard. Edit~/.claude/settings.jsonto setENGRAM_TOKENand remove any leftoverENGRAM_AUTH_ADMIN_TOKEN/ENGRAM_API_TOKENentries.Operator-side: leave
ENGRAM_AUTH_ADMIN_TOKENin the Docker compose.env. Server reads it; nothing else does.Verification
go build ./...clean.go test ./...green across all packages (auth coverage 97%, full suite passes).go vet ./...clean for touched packages.Test plan (post-merge, manual)
/engram:setup→ restart CC.recall,vault,issues,feedback,docs,admin,loom_*).POST /api/auth/tokenswith bearer (any tier) → 403. With session cookie + admin role → 200.Risks / known gaps
ProjectEventsper-event re-validation hook is documented in the spec but not landed in this PR's diff. The streaming interceptor validates at stream open; mid-stream revocation latency is bounded by the heartbeat reconnect cycle (~60 s) rather than per-event. This matches Clarification C3's intent; tracked as a follow-up if the user finds the bound too loose.Release(goreleaser) job has been failing onui/package.json.tplstaleness since v5.2.5. Tagging v6.0.0 will repeat the failure unless.tplis regenerated first. Not part of this spec; flagged for the release runner.Refs
Closes the PR-203 regression. Full feature spec:
.agent/specs/auth-two-tier-tokens/spec.md.Summary by CodeRabbit
Критические изменения
Документация