feat: add secure OAuth connection core - #179
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughVersion 3 introduces strict, non-secret OAuth connection declarations, canonical binding and credential storage primitives, persistent lifecycle coordination, redacted auditing, migration support, and fail-closed runtime rejection until native authorization is enabled. ChangesOAuth Version 3 configuration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Config
participant Runtime
participant OAuthConnectionLifecycle
participant OAuthConnectionRegistry
participant OAuthCredentialStore
Config->>Runtime: resolve version 3 OAuth configuration
Runtime-->>Config: reject with OAUTH_AUTHORIZATION_NOT_ENABLED
OAuthConnectionLifecycle->>OAuthConnectionRegistry: manage connection metadata
OAuthConnectionLifecycle->>OAuthCredentialStore: load or persist credentials
OAuthConnectionLifecycle-->>OAuthConnectionRegistry: commit lifecycle state
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/config/migrate-config.ts`:
- Around line 132-138: Remove the redundant conditional guard and its duplicate
error path in the config migration flow, leaving a single unsupported-version
throw after the existing version handling for "3", "1", and "2". Preserve the
current MiftahError code and message behavior.
In `@src/config/schema.ts`:
- Around line 1204-1212: Update the addConfigIssue call in the hasMergedHeader
authorization conflict branch to report the diagnostic against the connection or
its upstream/headers location instead of [...path, "resource"]. Keep the
existing error code and messages unchanged.
In `@src/oauth/local-lock.ts`:
- Around line 120-137: Update the lock-acquisition flow around
inspectLocalLockPort and tryAcquireLocalLock so a given key cannot split across
candidate ports: reserve one deterministic port for each key, or scan all ports
for an existing same-key holder before acquiring any available port. Preserve
the existing timeout and OAuthLocalLockUnavailableError behavior while ensuring
contenders for the same key serialize across processes.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: dcc12ba8-bbfd-40e3-879d-91467223a11c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (50)
CHANGELOG.mddocs/architecture.mddocs/config.mddocs/library-api.mddocs/oauth-console-threat-model.mddocs/oauth-support.mddocs/security.mdexamples/generic.miftah.jsonexamples/github.miftah.jsonexamples/multi-upstream.miftah.jsonexamples/plugins.miftah.jsonexamples/sentry.miftah.jsonpackage.jsonsrc/audit/audit-trail.tssrc/audit/audit-types.tssrc/cli/exit-codes.tssrc/config/generate-json-schema.tssrc/config/migrate-config.tssrc/config/presets.tssrc/config/schema.tssrc/config/types.tssrc/config/validate-config.tssrc/config/versions.tssrc/index.tssrc/oauth/audit.tssrc/oauth/canonical-resource.tssrc/oauth/connection-lifecycle.tssrc/oauth/connection-registry.tssrc/oauth/connection-types.tssrc/oauth/local-lock.tssrc/oauth/secure-credential-store.tssrc/runtime/resolve-runtime-config.tssrc/upstream/headers.tssrc/upstream/upstream-process-manager.tssrc/utils/errors.tstests/cli-exit-codes.test.tstests/config-diagnostics.test.tstests/config-migration.test.tstests/config-schema-contract.test.tstests/oauth-audit.test.tstests/oauth-canonical-resource.test.tstests/oauth-config.test.tstests/oauth-connection-lifecycle.test.tstests/oauth-connection-registry.test.tstests/oauth-connection-types.test.tstests/oauth-console-threat-model-docs-contract.test.tstests/oauth-secure-credential-store.test.tstests/oauth-support-docs-contract.test.tstests/package-contract.test.tstests/public-api.test.ts
All three findings were addressed in commit 4e2dd28, replied to with focused validation evidence, and the review threads are resolved. CodeRabbit completed its incremental current-head review successfully with no new findings.
|
Final review exception record for exact head 4e2dd28:\n\n- CodeRabbit completed a substantive review of the preceding head and raised three findings. All three were fixed in 4e2dd28, replied to, and their threads are resolved.\n- CodeRabbit incremental comments on 4e2dd28 contained no new findings, but its final status is explicitly Review rate limited.\n- Per maintainer policy, a local Claude Opus review was attempted twice: first for the full development-to-head diff for more than five minutes, then for only commit 4e2dd28 for more than two minutes. Both processes remained active but produced no output and were terminated at bounded limits. They are not counted as reviews.\n- The incremental commit was manually reviewed for lock acquisition races, fail-closed timeout and cleanup behavior, diagnostic compatibility, and regression-test validity.\n- Exact-head CI run 29906763664 is green: Linux quality, Ubuntu, macOS, Windows Node 20, 22 and 24, and Verify all passed. Local exact-head npm test and coverage each passed 1,167 active tests with 23 platform skips; all release checks passed.\n\nGitHub reports the PR clean and mergeable, with no unresolved review threads. |
Closes #81\n\n## What changed\n\n- introduces configuration version 3 with profile-bound OAuth connection records that contain opaque oauthconn references only\n- binds each record to the exact profile, upstream, canonical HTTPS resource, issuer, client registration, and scope set\n- stores credential envelopes only in the native operating-system credential vault and keeps non-secret metadata in a restrictive atomic registry\n- serializes connect, refresh, and disconnect lifecycles with single-flight refresh, cancellation, timeout, compensation, and redacted audit events\n- keeps version 1 and version 2 non-OAuth configuration compatible\n- fails closed with OAUTH_AUTHORIZATION_NOT_ENABLED when a version 3 OAuth connection is configured before the authorization engine is available\n- rejects static Authorization headers when an OAuth connection is configured\n\n## Boundary\n\nThis PR is the secure connection and credential-lifecycle core only. Browser launch, callback handling, provider discovery, token exchange, refresh over the network, and upstream header injection remain intentionally out of scope for #81 and will be implemented by the dependent OAuth roadmap issues.\n\n## Validation\n\n- focused OAuth tests: 156 passed\n- npm run test:coverage: 1,166 passed, 23 skipped; statements 95.57 percent, branches 91.94 percent, functions 98.33 percent, lines 95.57 percent\n- npm run test:core: 402 passed, 23 skipped\n- npm run lint\n- npm run typecheck\n- npm run build\n- npm run smoke:cli\n- npm run check:pack: 43 files verified\n- npm run test:package: 18 passed\n- git diff --check\n\nA plain aggregate npm test invocation encountered existing intermittent readiness and operation-fixture failures outside the OAuth diff. Each exact failure passed five focused repetitions, the exact final coverage suite passed the full active test set, and evidence is recorded under canonical issue #122 without weakening tests or changing timeouts.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor