fix(security): scope onboarding status to caller's tenant#1784
Open
corvid-agent wants to merge 7 commits intomainfrom
Open
fix(security): scope onboarding status to caller's tenant#1784corvid-agent wants to merge 7 commits intomainfrom
corvid-agent wants to merge 7 commits intomainfrom
Conversation
The flock_reputation_refresh handler (added in #1776) had no direct tests. This adds: - New test file for execFlockReputationRefresh: empty flock (0 updated), populated flock (N updated), and error path (table dropped → failed status) - getActionCategory('flock_reputation_refresh') → 'lightweight' in priority-rules.test.ts - 'flock_reputation_refresh' added to valid action types list in scheduler-pipeline.test.ts All 62 tests pass. Zero TSC errors. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When recording a failed auth attempt to the audit database fails, the catch block was silently swallowing the error with only a comment. This meant that if the audit record failed to persist (DB connection issue, schema mismatch, etc.), there was zero visibility — no log line, no metric, nothing. Security forensics could be blind to audit failures. Now we log a warning when the audit record fails while preserving the best-effort semantics (the rejection still happens, auditing is not blocking). Fixes silent error in server/middleware/auth.ts:247. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Adds a test that mocks recordAudit to throw, exercising the catch branch in checkHttpAuth that logs audit failures. Fixes codecov/patch check on PR #1781. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove unused spyOn import (TS6133) and fix mock.module restore in auth-middleware test. The previous restore called require() which returned the already-mocked module, poisoning 369 downstream tests. Now saves a reference to the real module before mocking. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The 'still rejects when audit logging throws' test used mock.module to replace ../db/audit with a throwing stub. Bun's mock.module is persistent across test files, so the mock leaked and caused every subsequent test calling recordAudit to throw "DB connection lost". The auth rejection (403) behavior is already covered by the existing test above. The catch path is a trivial log.warn that cannot alter the response, so removing this test loses no meaningful coverage. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds a test that mocks recordAudit to throw, exercising the catch block that logs a warning when audit recording fails. Fixes codecov/patch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The GET /api/onboarding/status endpoint was leaking cross-tenant information by querying agent and project counts without tenant isolation. Fixed by: 1. Import RequestContext from middleware/guards 2. Add context parameter to handleOnboardingRoutes and pass through to handleOnboardingStatus 3. Update listAgents() and listProjects() calls to include context.tenantId 4. Update call site in routes/index.ts to pass context 5. Update tests to provide mock context with tenantId='default' This ensures multi-tenant deployments only return data for the authenticated tenant, preventing cross-tenant data exposure. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Collaborator
Author
Collaborator
Author
Suggested queue (Kite)
All five open PRs were CONFLICTING vs |
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
Fixed a security issue where the GET /api/onboarding/status endpoint leaked cross-tenant information in multi-tenant deployments. The endpoint was returning agent and project counts from the 'default' tenant regardless of the authenticated caller's actual tenant.
Root Cause
The
handleOnboardingRoutesfunction inserver/routes/onboarding.tsdid not accept aRequestContextparameter, so it couldn't passtenantIdto the database queries forlistAgents()andlistProjects().Changes
server/routes/onboarding.ts:
RequestContextfrom../middleware/guardscontext: RequestContextparameter tohandleOnboardingRoutescontexttohandleOnboardingStatuslistAgents(db)→listAgents(db, context.tenantId)listProjects(db)→listProjects(db, context.tenantId)server/routes/index.ts:
handleOnboardingRoutesat line 360 to includecontextparameterserver/tests/testnet-onboarding.test.ts:
RequestContextimportmockContextwithtenantId: 'default'handleOnboardingRoutesto passmockContextTest Results
bun x tsc --noEmit --skipLibCheckSecurity Impact
In multi-tenant deployments, the endpoint now correctly returns onboarding status scoped to the authenticated caller's tenant, preventing cross-tenant data exposure.
🤖 Generated with Claude Code