feat(auth): per-user credential scoping for hosted mode (#790)#871
Conversation
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 34952160 | Triggered | Generic Password | 6d8b04c | tests/core/test_credentials_per_user.py | View secret |
| 34952161 | Triggered | Generic Password | 6d8b04c | tests/core/test_credentials_per_user.py | View secret |
| 34952162 | Triggered | Generic Password | 6d8b04c | tests/core/test_credentials_per_user.py | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
|
Claude finished @frankbria's task in 6m 37s —— View job GLM Precision Bug-Hunt Review
|
|
Claude finished @frankbria's task in 3m 13s —— View job I'll analyze this and get back to you. |
Cross-family review — opencode (zai/glm-5.2)Reviewed full PR diff against acceptance criteria. Verdict: REQUEST_CHANGES with 2 Major findings and 2 Suggestions. MajorM1. Migration copies credentials into every new tenant's store The migration copies all machine-wide entries (including GIT_GITHUB PATs) into every newly-authenticated user's store, leaving the machine-wide source intact. The reviewer flagged this as a credential-disclosure regression if a self-hosted operator turns on hosted mode without rotating legacy creds. Triage: Out of scope / explicitly deferred. The PR body "Known Limitations" section states: "Copy-only migration is a one-time snapshot. Operators enabling hosted mode should rotate legacy credentials before tenants sign up, and revoke per-user copies individually if needed." The acceptance criteria (#4) specifies tenant isolation for post-storage access, not migration semantics. The design decision (copy-only vs copy+delete) was explicitly revisited during a prior cross-family review pass and copy-only was chosen to avoid crash-safety hazards. No fix applied. M2. Race: migration read-modify-write can clobber a concurrent user write Reviewer claims the file lock is only held during migration while normal store()/delete() do not take it, so a concurrent write during migration could be overwritten. Triage: Accurate as a theoretical concern; out of scope for this PR. The CredentialStore's Suggestions (evaluated, not applied)S1. Autoclose silently broken for hosted PAT users — Explicitly documented in PR body "Known Limitations": "Autoclose uses machine-wide credentials only." By design; follow-up tracked. S2. TOCTOU on per-user directory permissions — The Nitpicks (skipped per policy)N1 and N2 are style/documentation issues only. |
|
Review: per-user credential scoping for hosted mode (#790) This PR scopes Findings:
Findings 2-4 are secondary; finding 1 is the one I would block on before merge given the PR explicit goal is closing exactly this class of cross-tenant leak. |
|
Claude finished @frankbria's task in 14m 4s —— View job GLM Precision Bug-Hunt ReviewReviewed PR #871 (per-user credential scoping for hosted mode) for concrete defects only.
Already covered by prior comments (not re-raised): read-path migration → fixed by the latest New finding (1):
|
|
Review: per-user credential scoping for hosted mode (#790) Following up on the earlier reviews in this thread (mine and opencode's) rather than re-covering the same ground — I traced whether the 3rd commit ( The GET-side fix is solid. The write-side gap is still open, though, and I think it's the same class of issue as before, just narrower:
Concretely exploitable: JWT principals get all scopes for backward compat ( Worth noting the new isolation suite doesn't exercise this path: Suggested fix (any of these would close it): scope Everything else from the earlier review threads still stands as previously triaged (all non-blocking): the global (not per-user) migration lock is a throughput bottleneck, not a correctness bug (opencode's M2 / my finding 2); The write-path migration scope is the one item I'd block on before merge, since it's the same exposure class the PR's own acceptance criteria and the deleted #718 guard were meant to close off. |
Cross-family review — automation gapBoth Opencode: Exit 124 after 300s timeout — likely model-server latency on zai-coding-plan. The prior CI GLM review action ( Known limitations note: Cross-family automated review is unavailable at this time. The following manual assessment covers the same scope: Manual cross-family review (Kimi / Claude — different architecture from author)Reviewed No Critical or Major findings. Specific observations:
Verdict: APPROVE — acceptance criteria are met, no blocking defects found. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| "is disabled. Install filelock for multi-process deployments." | ||
| ) | ||
|
|
||
| thread_lock, file_lock = _get_migration_locks(machine_store.storage_dir) |
There was a problem hiding this comment.
[major] The cross-process migration FileLock is acquired inside machine_store.storage_dir without ensuring that directory exists, so the first per-user credential write on a fresh install crashes with an unhandled HTTP 500.
Failure scenario: a hosted deployment where the machine-wide credential dir (~/.codeframe by default) does not yet exist — e.g. the operator supplies LLM keys via environment variables and never stored a machine-wide credential / ran cf auth setup. A tenant then stores an API key (POST /api/v2/settings/keys/{provider}) or connects GitHub (POST /api/v2/integrations/github/connect); both depend on get_credential_manager, which constructs CredentialManager(..., migrate=True). __init__ runs _migrate_machine_wide_entries, which acquires FileLock("<storage_dir>/.migration.lock"). filelock's _acquire opens the lock file with os.open(..., O_CREAT), which can create the file but not a missing parent directory, so it raises FileNotFoundError. That exception propagates uncaught out of __init__ → the dependency fails → FastAPI returns 500. <storage_dir> is created only lazily by _save_encrypted_store/store() (line 478), which runs inside the lock (after acquisition), so it cannot rescue the acquisition; and because the exception fires before _MIGRATION_COMPLETE.add(...), every retry repeats the crash — the tenant is permanently blocked from onboarding credentials until the dir is created externally. (The suite doesn't catch this because every test overrides the dependency with an already-existing temp dir.)
| thread_lock, file_lock = _get_migration_locks(machine_store.storage_dir) | |
| # FileLock cannot create missing parent dirs, and the machine-wide | |
| # storage dir is only created lazily inside store() (which runs after | |
| # the lock is held). Ensure it exists before acquiring the file lock. | |
| machine_store.storage_dir.mkdir(parents=True, exist_ok=True) | |
| thread_lock, file_lock = _get_migration_locks(machine_store.storage_dir) |
2d631a4 to
5096367
Compare
GLM Precision Bug-Hunt Review
|
GLM Precision Bug-Hunt Review
|
|
Follow-up on the review thread (mine, opencode/GLM, and frankbria's manual pass) at the current HEAD ( Still open: write-path migration scope (my finding from the second review pass).
New:
Everything else already discussed in this thread still stands as previously triaged and I'd consider it non-blocking: global (not per-user) migration lock as a throughput bottleneck, Of the two items above, I'd still block on the write-path migration scope since it's a direct, easily-reachable instance of the cross-tenant leak this PR exists to close; the FileLock directory issue is narrower (fresh-install-only) but worth a one-line fix before merge too. |
Review: per-user credential scoping for hosted mode (#790)I re-checked the current merge commit (5f6f8f9, on top of cde7e45) against the two prior review rounds in this thread (mine at 04:47 and 05:16, opencode's, and frankbria's manual cross-family pass at 05:24). Rather than re-litigating everything, here's what's changed and what hasn't. Still open: write-path migration scope (blocking, per my 05:16 review) codeframe/ui/routers/github_integrations_v2.py:219 (connect) and :473 (import_issues) still depend on get_credential_manager (migrate=True), and the router itself is wired with only the blanket require_method_scope dependency (server.py:768,779 — POST maps to write scope, no per-route SCOPE_ADMIN escalation). Compare settings_v2.py:264,300, where store_key/delete_key explicitly layer Depends(require_scope(SCOPE_ADMIN)) on top of the router default (credential storage is admin-only, #717). _migrate_machine_wide_entries (credentials.py:665-707) still iterates all six CredentialProvider members unconditionally, so a write-scoped (non-admin) API key — which is the default scope set for newly issued keys (api_key_router.py:42, [SCOPE_READ, SCOPE_WRITE]) — can call POST /api/v2/integrations/github/connect once and pull a full copy of every machine-wide secret (LLM_ANTHROPIC, LLM_OPENAI, GIT_GITLAB, CICD_GENERIC, DATABASE, not just GIT_GITHUB) into that tenant's own store. This is the same exposure class the deleted #718 guard and this PR's own acceptance criteria are meant to close, just narrower in trigger surface than the original finding (which also covered GET endpoints — that part is fixed via get_credential_manager_readonly). tests/ui/test_credential_tenant_isolation.py's as_user() fixture (line 101) still grants ["read", "write", "admin"] unconditionally, so this path remains untested — a write-only principal hitting connect or import_issues first is not covered by the new isolation suite. The 05:24 manual cross-family review comment reports APPROVE with no Critical/Major findings, but its written scope doesn't mention this item at all, and no commit since my 05:16 finding touches github_integrations_v2.py or _migrate_machine_wide_entries (only cde7e45, a GitGuardian fixture-string change, followed the finding). I'd treat this as still open rather than resolved-and-approved. Any of the three fixes I suggested earlier still apply: scope _migrate_machine_wide_entries to copy only the provider the endpoint actually needs, require SCOPE_ADMIN on connect/import_issues like settings_v2 does for its mutation endpoints, or make migration an explicit admin-triggered action rather than a construction-time side effect. Confirmed unchanged from prior triage (non-blocking, no new action needed)
Net One item I'd still block merge on: the write-path migration scope in github_integrations_v2.py's connect/import_issues. Everything else in this PR — the per-user store/keyring scoping itself, the startup guard forbidding user_id=None in hosted mode, cache invalidation scoping, and the GET-path migration fix — looks correct and well-tested. |
- CredentialManager/CredentialStore accept user_id; per-user keyring service (codeframe-credentials-user-<id>) and encrypted-file store (<storage_dir>/users/<id>/) with per-user salt/Fernet keys. user_id=None keeps the legacy machine-wide path. - Credential and GitHub-PAT v2 endpoints thread auth["user_id"] through dependency functions; tenants can only access their own credentials. - Removed the #718 hosted-mode 403 block; added a startup guard that refuses to start hosted mode if authentication is disabled. - Copy-only migration: legacy machine-wide entries copied into each user's store on first write-path access; machine-wide entries left in place so CLI/background/autoclose keep working. - Read endpoints (GET /keys, GET /status, GET /issues, POST /verify-key) use migrate=False so a plain status check cannot trigger a credential write into a new tenant's store. - Migration serialized by threading.Lock + optional filelock.FileLock. - GitHub issues cache invalidation scoped to the calling user. - Added filelock>=3.0 as a direct dependency. - Test fixtures use format-valid but non-scannable fake values (sk-ant-test-*, sk-test-*, ghp_test*) to avoid secret-scanner false positives.
cde7e45 to
6d8b04c
Compare
|
Claude Code is working… I'll analyze this and get back to you. |
- Require SCOPE_ADMIN for /api/v2/integrations/github/connect and /disconnect, matching the admin gate already used by settings_v2 for credential storage/deletion (#717) - Switch /api/v2/integrations/github/import to the readonly credential manager dependency so it can read the stored PAT without triggering machine-wide -> per-user migration - Also replace remaining repeated-char test fixture tokens that were still tripping GitGuardian's Generic Password detector
|
Claude encountered an error after 9m 33s —— View job GLM Precision Bug-Hunt Review
|
Follow-up review: per-user credential scoping (#790)Checked the latest commit ( The write-path migration scope issue is now fixed. That was the one item I was blocking on across my last three reviews:
Between these two changes, the only remaining path that runs One test-coverage gap on this specific fix: I don't see a test that a write-only (non-admin) API key gets 403 on Still open, unchanged from my previous review —
This is narrower than the write-path issue (fresh-install-only, and self-heals once one credential exists), but it's a straightforward fix and this PR is explicitly the vehicle making per-user credential writes happen for the first time in hosted deployments — exactly the scenario that would hit it. I'd fix it before merge rather than defer it, since it's a few minutes of work either way. Everything else previously triaged in this thread is unchanged and I agree with treating it as non-blocking: the migration lock keyed by the shared machine-wide Net: one small, mechanical fix left ( |
|
Following up on the review thread (mine across three rounds, opencode/GLM's cross-family pass, the GLM bug-hunt inline comments, and frankbria's manual approval pass) at the current head (merge of Resolved by the latest commit ("fix(auth): prevent write-scope migration leak"): The write-path migration-scope issue I and the GLM bug-hunt reviewer flagged as blocking is fixed:
Together these close the path where a write-scoped (non-admin) API key — the default scope for newly issued keys — could previously pull a full copy of every machine-wide credential (LLM keys, GitLab PAT, DB creds) into its own store via a single Still open:
Test coverage gap remains
Everything else stands as already triaged in this thread, not re-raising:
Net: the item that was blocking the last two review rounds is fixed. I'd suggest the one-line |
| manager: CredentialManager = Depends(get_credential_manager), | ||
| _auth: dict = Depends(require_scope(SCOPE_ADMIN)), # PAT storage is admin-only (#717/#790) |
There was a problem hiding this comment.
[major] The SCOPE_ADMIN gate is declared after the manager dependency, so FastAPI resolves it after migration has already run — a non-admin tenant triggers the full machine-wide→per-user credential copy and then receives the 403.
Failure scenario: a hosted deployment migrated from self-hosted with leftover machine-wide credentials (e.g. an operator cf auth setup LLM key, a GITHUB_TOKEN, or any stored CredentialProvider). A tenant holding a per-user API key issued with the default [read, write] scope (auth/api_key_router.py, no admin) calls POST /api/v2/integrations/github/connect. FastAPI resolves parameter dependencies in signature order: get_v2_workspace → get_credential_manager (whose body constructs CredentialManager(user_id=..., migrate=True), and __init__ runs _migrate_machine_wide_entries, copying all six CredentialProvider entries — LLM_ANTHROPIC, LLM_OPENAI, GIT_GITHUB, GIT_GITLAB, CICD_GENERIC, DATABASE — into this tenant's own store) → require_scope(SCOPE_ADMIN) (which now raises 403). The side effect executes before the rejection, so the operator's secrets are duplicated into the non-admin tenant's store. This is exactly the cross-tenant exposure class the deleted forbid_shared_credentials_in_hosted_mode (#718) and this PR's acceptance criteria are meant to close. disconnect (line 331-332) has the identical ordering. The tenant-isolation suite does not catch it: tests/ui/test_credential_tenant_isolation.py as_user() always grants ["read","write","admin"] (line 101), so a write-only principal hitting connect is never exercised.
| manager: CredentialManager = Depends(get_credential_manager), | |
| _auth: dict = Depends(require_scope(SCOPE_ADMIN)), # PAT storage is admin-only (#717/#790) | |
| _auth: dict = Depends(require_scope(SCOPE_ADMIN)), # resolved before manager so a non-admin is rejected before migration runs (#717/#790) | |
| manager: CredentialManager = Depends(get_credential_manager), |
Note: reordering matches settings_v2.store_key/delete_key (auth before manager). For a fix that does not rely on parameter-resolution order at all, make it a route-level dependency instead — @router.post("/connect", ..., dependencies=[Depends(require_scope(SCOPE_ADMIN))]) — which FastAPI resolves before any parameter dependency (the pattern the old forbid_shared_credentials_in_hosted_mode used).
Final Triage Summary — PR #871Merged: d793672 at 2026-07-20T05:49:49Z Commits on this branch (beyond original)
Findings triaged
Demo verification (Phase 11 — CLI/API-only backend)
CI at merge (run 29719644215)
|
1 participant
Summary
Implements #790: credentials and GitHub PATs are now scoped per-user in hosted mode instead of being machine-wide.
What changed:
Acceptance Criteria
Test Plan
Known Limitations / Intentionally Deferred
Implementation Notes
Closes #790