Harden PR follow-up ingestion auth#334
Conversation
There was a problem hiding this comment.
AI Automated Review
Full PR review.
Analysis engine: review@https://litellm.jory.dev/v1 (openai)
Recommendation
Approve. This PR addresses critical security vulnerabilities identified in a tech debt audit by hardening authentication for PR follow-up ingestion endpoints and implementing a global sync lock to prevent race conditions during concurrent synchronization tasks.
Change-by-Change Findings
Security Hardening
src/app/api/pr-followup/sync/route.ts: AddedauthorizeRequest()to the POST handler. This ensures that the mutating sync endpoint now requires valid authentication (Bearer, Basic, or OIDC), addressing the vulnerability where unauthenticated requests could trigger issue synchronization.src/app/api/pr-followup/webhook/route.ts:- Added
authorizeRequest()to the POST handler. - Implemented a fail-closed security model for webhook signature verification. If
WEBHOOK_SECRETis not configured, requests are rejected by default. - Introduced
WEBHOOK_GATEWAY_MODE=trueas an explicit opt-out for deployments behind authenticated API gateways, preventing accidental exposure.
- Added
Concurrency Control
src/lib/sync-lock.ts(New): Implements a database-backed global lock using thesyncLocktable. It includes logic to handle stale locks (older than 30 minutes) and uses transactions to ensure atomic lock acquisition.src/app/api/automation/sync/route.ts&src/app/api/sync/route.ts: Integrated the newacquireLockandreleaseLockutilities. This prevents multiple sync processes (automation, manual, or scheduled) from running simultaneously and causing database contention or inconsistent state.src/app/api/sync/scheduled/route.ts: Refactored to use the centralizedsync-lockmodule instead of the previous local implementation, ensuring consistent locking behavior across all sync types.
Schema & Metadata
prisma/schema.prisma: UpdatedIssueSyncRun.syncTypedefault value to includeautomation, ensuring the schema correctly reflects the new sync categories.
Standards Compliance
- Security: Complies with the requirement to validate inputs and authorize requests before database operations. The fail-closed webhook implementation aligns with best practices for secure defaults.
- Error Handling: Uses the
error instanceof Errorpattern and returns appropriate HTTP status codes (401 for unauthorized, 409 for conflict, 500 for server errors). - Code Standards: Follows the repository's pattern of using shared authorization helpers and centralized locking logic.
Linked Issue Fit
- Matches PR 312: The PR directly implements the recommendations from the audit finding in issue PR 312, specifically requiring
authorizeRequeston the pull sync endpoint and making webhook signature requirements explicit/fail-closed.
Unknowns or Needs Verification
- Deployment Configuration: Users deploying behind an API gateway must now explicitly set
WEBHOOK_GATEWAY_MODE=trueif they intend to rely on the gateway for webhook authentication, as the default behavior is now to reject unverified webhooks.
28e038f to
f4c48ec
Compare
- Add authorizeRequest() to sync/route.ts POST handler for Bearer token auth - Add authorizeRequest() to webhook/route.ts POST handler - Make webhook signature verification fail-closed by default - Introduce WEBHOOK_GATEWAY_MODE=true opt-out for gateway deployments - Update documentation comments to reflect new security model
cf0a648 to
a87cf16
Compare
There was a problem hiding this comment.
AI Automated Review
Full PR review.
Analysis engine: MiniMax-M2.7@https://litellm.jory.dev/v1 (anthropic)
PR Review: Harden PR follow-up ingestion auth (PR 334)
Recommendation: Approve
This PR correctly addresses issue PR 312 by hardening authentication on the PR follow-up ingestion endpoints. The changes are well-targeted and follow established patterns in the codebase.
Change-by-Change Findings
1. src/app/api/pr-followup/sync/route.ts (+8/-2)
- Added
authorizeRequest()import from@/lib/auth - Changed
POST()signature to acceptNextRequestparameter - Added auth check at entry point, returning 401 with
{ error: "Unauthorized" }for unauthenticated requests - Verdict: Correct — addresses the first requirement from issue PR 312 to "require
authorizeRequeston pull sync"
2. src/app/api/pr-followup/webhook/route.ts (+28/-7)
- Added
authorizeRequest()import from@/lib/auth - Added
isSignatureVerificationEnabled()helper function that returnstruewhenWEBHOOK_SECRETis set OR whenWEBHOOK_GATEWAY_MODE=true - Added auth check before signature verification, returning 401 for unauthenticated requests
- Changed signature verification logic to be fail-closed: if
WEBHOOK_SECRETis unset andWEBHOOK_GATEWAY_MODEis not "true", requests are rejected - Updated JSDoc to document the new security model
- Verdict: Correct — addresses the second requirement from issue PR 312 to "make webhook signature requirements explicit/fail-closed unless an intentional gateway mode is configured"
3. src/app/api/sync/route.test.ts (+25/-6)
- Added
mockTxClientfor transaction structure - Extended prisma mocks with
syncLock.create,issueSyncRun.create,automationSyncRun.create/update, andissuemodel - Verdict: Test infrastructure update — appears to be preparing test mocks for auth hardening, consistent with commit
a0bbafd("fix(tests): mock syncLock, transaction, and prisma tables for auth-hardening PR") in the repo history
4. src/app/api/automation/sync/route.test.ts (+39/-4)
- Added
mockTxClientfor transaction structure - Extended prisma mocks with additional table stubs (
syncLock.create,issueSyncRun.create,automationSyncRun,automationRepo.update) - Added mock for
@/lib/githubmodule with all fetch functions - Verdict: Test infrastructure update — same pattern as sync/route.test.ts
Standards Compliance
- Auth pattern: Uses
authorizeRequest()from@/lib/auth, matching the established pattern used by other mutating endpoints (e.g.,/api/agent-runs,/api/automation/repos) - 401 response: Returns
NextResponse.json({ error: "Unauthorized" }, { status: 401 }), consistent with existing API conventions found throughout the codebase - Error handling: Uses appropriate HTTP status codes (401 for auth failures)
- Environment variable naming:
WEBHOOK_GATEWAY_MODEfollows existing naming conventions in the codebase (e.g.,DISPATCH_AUTH_MODE,DISPATCH_URL)
Linked Issue Fit
Issue PR 312 explicitly stated:
"P1 — Harden PR follow-up ingestion auth: require
authorizeRequeston pull sync and make webhook signature requirements explicit/fail-closed unless an intentional gateway mode is configured."
This PR fully satisfies both requirements:
- ✅
authorizeRequestis now required on the pull-based sync endpoint - ✅ Webhook signature verification is fail-closed by default, with
WEBHOOK_GATEWAY_MODE=trueas an explicit opt-out
Evidence Provider Findings
No evidence providers configured for this repository scan.
Tool Harness Findings
No tool harness findings available (planned requests: 0).
Unknowns / Needs Verification
-
Test coverage for auth behavior: The test file changes add infrastructure mocks but do not include explicit test cases verifying the 401 response for unauthenticated requests on the pr-followup endpoints. While other routes (e.g.,
automation/sync/route.test.tslines 111-126) have dedicated auth-failure tests, this PR does not add equivalent tests for the pr-followup routes. This is not a blocker but worth noting for future test coverage. -
WEBHOOK_GATEWAY_MODEdocumentation: The new environment variableWEBHOOK_GATEWAY_MODEis introduced in code comments but may need to be added to.env.exampleand README.md for operator visibility. This is a non-blocking documentation gap.
Repository Impact Scan Notes
- The PR does not modify any environment variable configuration files (
.env.example) or main documentation (README.md,AGENTS.md) - Repository history shows a previous auth-hardening PR (PR 150, commit
693a46f) that introducedauthorizeRequestfor mutating endpoints; this PR extends that pattern to the pr-followup endpoints that were missed
Fixes #312
Changes
sync/route.ts — Add route-level authorization
authorizeRequest()at the top of the POST handler/api/sync,/api/pr-fix-queue/enqueue)webhook/route.ts — Fail-closed signature verification + auth
authorizeRequest()at the top of the POST handler for Bearer token / Basic Auth / OIDC session authWEBHOOK_SECRETis not configured, requests are rejectedWEBHOOK_GATEWAY_MODE=trueenvironment variable as an explicit opt-out for deployments behind an API gateway that performs its own authentication and signature verificationSecurity impact
Both endpoints were identified in the weekly tech debt audit as lacking proper authentication. The sync endpoint was a mutating POST with no auth check at all. The webhook endpoint silently skipped signature verification when
WEBHOOK_SECRETwas unset, creating a risk of unauthenticated event injection.