Bind identity sponsors to OIDC proofs - #75
Conversation
|
Warning Review limit reached
Next review available in: 48 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds OIDC sponsor federation, intent-bound sponsor proofs, sponsor binding metadata, attestation grants, and append-only ledger storage. It updates server routes, SDKs, public types, scopes, tests, and documentation. ChangesOIDC Sponsorship and Attestations
Estimated code review effort: 5 (Critical) | ~90+ minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SponsorsRoute
participant SponsorOidcService
participant IdentitiesRoute
participant AttestationStorage
Client->>SponsorsRoute: POST /v1/sponsors/proof
SponsorsRoute->>SponsorOidcService: Verify ID token and issue proof
SponsorOidcService-->>SponsorsRoute: Return sponsor proof
Client->>IdentitiesRoute: Create identity with sponsor proof
IdentitiesRoute->>SponsorOidcService: Verify sponsor proof
IdentitiesRoute->>AttestationStorage: Persist identity and ledger entry
AttestationStorage-->>IdentitiesRoute: Return stored identity
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
a63df40 to
5a78855
Compare
5a78855 to
22e9cf4
Compare
22e9cf4 to
61b9acc
Compare
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61b9accc5e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (14)
packages/server/src/lib/events.ts (1)
1-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a top-level type import for
SponsorBinding.Line 6 uses an inline
import("@relayauth/types")type expression. The rest of the server code imports shared types at the top of the file, for examplepackages/server/src/storage/identity-types.tsline 1. A top-levelimport typekeeps the dependency visible and matches the existing style.♻️ Proposed refactor
+import type { SponsorBinding } from "`@relayauth/types`"; + export type IdentityCreatedPayload = { id: string; org: string; name?: string; sponsorId: string; - sponsorBinding: import("`@relayauth/types`").SponsorBinding; + sponsorBinding: SponsorBinding; };🤖 Prompt for 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. In `@packages/server/src/lib/events.ts` around lines 1 - 7, Update IdentityCreatedPayload to use a top-level import type for SponsorBinding from `@relayauth/types`, then replace the inline import type expression on sponsorBinding with the imported symbol while preserving the payload shape.packages/server/src/__tests__/sponsor-oidc-binding.test.ts (1)
104-124: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClose the test app after each test.
createTestAppreturns aclose()function that closes the backing SQLite storage. No test in this file calls it. Each test therefore leaves one storage handle open for the whole run. Eight tests in this file create an app.Register the cleanup with the test context, as the file already does for the OIDC fixture server.
♻️ Proposed refactor for the first test; apply the same pattern to the other tests
const app = createTestApp({ RELAYAUTH_SPONSOR_FEDERATIONS: JSON.stringify({ [org]: { sponsorBinding: "oidc", issuer, clientId: "chief-fixture", sponsorIdClaim: "sub", }, }), }); + t.after(() => app.close());🤖 Prompt for 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. In `@packages/server/src/__tests__/sponsor-oidc-binding.test.ts` around lines 104 - 124, Register each createTestApp result for cleanup through the test context by calling its close() method in t.after, matching the existing startOidcFixture cleanup pattern. Apply this to all tests in sponsor-oidc-binding.test.ts, including the app created in the test containing identityCreatedEvent, so every backing SQLite storage handle is closed after its test.packages/server/src/index.ts (1)
32-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider exporting
JsonWebKeySettoo.
SponsorFederationConfigis now public. Itsjwksfield has the typeJsonWebKeySet, whichpackages/server/src/lib/sponsor-binding.tsdeclares at line 84 withoutexport. A consumer that builds a federation config cannot name that type. Export it fromsponsor-binding.tsand re-export it here.🤖 Prompt for 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. In `@packages/server/src/index.ts` around lines 32 - 35, Export JsonWebKeySet from sponsor-binding.ts, then add it to the public type re-exports in the index.ts export block alongside SponsorFederationConfig and SponsorFederationMap.packages/server/src/server.ts (1)
64-64: 🩺 Stability & Availability | 🔵 TrivialPlan for cache observability on the shared service.
sharedSponsorOidcServicelives for the process lifetime. It holds the JWKS cache and the discovery cache for every organization. Entries are never evicted; they are only overwritten when a refresh succeeds. The key space is bounded by the configured issuers, so memory growth is limited.Add metrics for JWKS cache hits, forced refreshes, and IdP fetch latency. Those signals make the refresh behavior in
packages/server/src/lib/sponsor-binding.tsobservable in production.🤖 Prompt for 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. In `@packages/server/src/server.ts` at line 64, Add cache observability for the process-wide sharedSponsorOidcService: instrument JWKS cache hits and forced refreshes, and record IdP fetch latency in the sponsor-binding refresh/fetch flow. Ensure the metrics are emitted for the existing cache and refresh paths without changing cache behavior.packages/server/src/lib/sponsor-binding.ts (1)
510-518: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCall
optionalIntegeronce per field.Each of the three optional duration fields calls
optionalIntegertwice. The function is pure, so the result is the same. The repetition makes the object literal hard to read.♻️ Proposed refactor
+ const grantTtlSeconds = optionalInteger(value.grantTtlSeconds, "grantTtlSeconds"); + const maxIdTokenAgeSeconds = optionalInteger(value.maxIdTokenAgeSeconds, "maxIdTokenAgeSeconds"); + const clockSkewSeconds = optionalInteger(value.clockSkewSeconds, "clockSkewSeconds"); + return { sponsorBinding: "oidc", issuer, clientId, @@ ...(value.jwks !== undefined ? { jwks: validateJwks(value.jwks) } : {}), - ...(optionalInteger(value.grantTtlSeconds, "grantTtlSeconds") !== undefined - ? { grantTtlSeconds: optionalInteger(value.grantTtlSeconds, "grantTtlSeconds") } - : {}), - ...(optionalInteger(value.maxIdTokenAgeSeconds, "maxIdTokenAgeSeconds") !== undefined - ? { maxIdTokenAgeSeconds: optionalInteger(value.maxIdTokenAgeSeconds, "maxIdTokenAgeSeconds") } - : {}), - ...(optionalInteger(value.clockSkewSeconds, "clockSkewSeconds") !== undefined - ? { clockSkewSeconds: optionalInteger(value.clockSkewSeconds, "clockSkewSeconds") } - : {}), + ...(grantTtlSeconds !== undefined ? { grantTtlSeconds } : {}), + ...(maxIdTokenAgeSeconds !== undefined ? { maxIdTokenAgeSeconds } : {}), + ...(clockSkewSeconds !== undefined ? { clockSkewSeconds } : {}), };🤖 Prompt for 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. In `@packages/server/src/lib/sponsor-binding.ts` around lines 510 - 518, Update the object construction around grantTtlSeconds, maxIdTokenAgeSeconds, and clockSkewSeconds to call optionalInteger once per field, store or reuse each result for both the undefined check and emitted property, and preserve the existing omission behavior when a value is undefined.packages/server/src/routes/sponsors.ts (1)
65-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReturn 500 for unexpected errors, not 503.
The catch block maps every non-
SponsorBindingErrorfailure to 503. 503 signals a transient dependency failure and invites the client to retry. A defect in the handler is not transient.SponsorBindingErroralready carries 503 for genuine provider failures.♻️ Proposed change
- return c.json({ error: "Failed to create sponsor proof", code: "sponsor_proof_failed", requestId }, 503); + return c.json({ error: "Failed to create sponsor proof", code: "sponsor_proof_failed", requestId }, 500);🤖 Prompt for 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. In `@packages/server/src/routes/sponsors.ts` around lines 65 - 76, Update the non-SponsorBindingError branch in the sponsor proof handler’s catch block to return HTTP 500 instead of 503, while preserving SponsorBindingError’s existing error.status response and the current response body.packages/server/src/db/migrations/0007_attestation_ledger.sql (1)
39-44: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRemove the redundant ledger index.
UNIQUE (org_id, org_seq)at Line 39 already creates a B-tree index on the same columns in the same order.idx_attestation_ledger_org_seqduplicates it and adds write cost on every append.♻️ Proposed change
-CREATE INDEX IF NOT EXISTS idx_attestation_ledger_org_seq - ON attestation_ledger (org_id, org_seq); -🤖 Prompt for 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. In `@packages/server/src/db/migrations/0007_attestation_ledger.sql` around lines 39 - 44, Remove the redundant CREATE INDEX statement for idx_attestation_ledger_org_seq, while preserving the UNIQUE (org_id, org_seq) constraint and the other unique constraint in the attestation ledger definition.packages/server/src/routes/attestations.ts (2)
293-295: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
normalizeRequiredStringonly forwards tonormalizeOptionalString.The two functions are identical. The name implies a stronger guarantee than the implementation provides, and every call site still has to check for
undefined. Remove the alias and callnormalizeOptionalStringdirectly.🤖 Prompt for 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. In `@packages/server/src/routes/attestations.ts` around lines 293 - 295, Remove the redundant normalizeRequiredString helper and replace every call to it with normalizeOptionalString directly. Preserve the existing undefined handling at each call site.
41-49: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAuthenticate before you read and validate the body.
The handler reads the full request body and validates
latebefore it callsauthenticateAndAuthorizeFromContextat Line 50. An unauthorized caller therefore drives JSON parsing and receives a 400 that distinguishes body shape from an authorization failure. Move the authorization check to the top of the handler.🔒️ Proposed change
attestations.post("/grants", async (c) => { + const auth = await authenticateAndAuthorizeFromContext( + c, + ATTEST_GRANT_SCOPE, + matchScope, + ); + if (!auth.ok) { + return c.json({ error: auth.error, code: auth.code }, auth.status); + } + const body = await parseJsonObjectBody<GrantRequest>(c.req.raw); if (!body) { return c.json({ error: "Invalid JSON body", code: "invalid_request" }, 400); } if (body.late !== undefined && typeof body.late !== "boolean") { return c.json({ error: "late must be a boolean", code: "invalid_request" }, 400); } const late = body.late === true; - const auth = await authenticateAndAuthorizeFromContext( - c, - ATTEST_GRANT_SCOPE, - matchScope, - ); - if (!auth.ok) { - return c.json({ error: auth.error, code: auth.code }, auth.status); - }🤖 Prompt for 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. In `@packages/server/src/routes/attestations.ts` around lines 41 - 49, Move the authenticateAndAuthorizeFromContext call to the beginning of the handler, before parseJsonObjectBody and the body.late validation. Preserve the existing unauthorized response and only parse or validate GrantRequest after authentication succeeds.packages/server/src/__tests__/attestations.test.ts (2)
142-146: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the
expiredvariable.At Line 142
expiredholds the grant before the test expires it. The expiry happens at Line 144. The name contradicts the state. Rename it tograntBeforeExpiry.🤖 Prompt for 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. In `@packages/server/src/__tests__/attestations.test.ts` around lines 142 - 146, Rename the variable `expired` in the attestation grant test to `grantBeforeExpiry`, updating its assertion reference while preserving the existing grant retrieval and expiry update behavior.
106-114: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
ts: payload.tsasserts nothing.The expected object reads
tsfrom the actual payload, sodeepEqualalways passes for that field. The test cannot detect a missing, empty, or malformed timestamp. Assert the shape separately.💚 Proposed change
const payload = verifyJws(finalized.attestations[0]!.jws); + assert.equal(typeof payload.ts, "string"); + assert.ok(Number.isFinite(Date.parse(payload.ts as string)), "ts must be an ISO timestamp"); assert.deepEqual(payload, {🤖 Prompt for 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. In `@packages/server/src/__tests__/attestations.test.ts` around lines 106 - 114, Update the attestation payload assertion in the test around the payload deep-equality check: remove the self-referential ts: payload.ts expectation and add a separate assertion that validates the timestamp’s required shape and value, including detection of missing, empty, or malformed timestamps. Preserve the existing assertions for the other payload fields.packages/server/src/lib/sign-rs256.ts (2)
71-94: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the duplicated signing body.
signCanonicalRs256repeats every step ofsignRs256except the payload encoding. Extract the shared key import, header encoding, and signature steps into one internal function that accepts the encoded payload.♻️ Proposed refactor
+async function signEncodedPayload( + encodedPayload: string, + key: CryptoKey | string, + kid: string, +): Promise<string> { + const privateKey = typeof key === "string" ? await importRsaPrivateKey(key) : key; + const encodedHeader = encodeJsonAsBase64Url({ alg: "RS256", typ: "JWT", kid }); + const signingInput = `${encodedHeader}.${encodedPayload}`; + const signature = await crypto.subtle.sign( + { name: "RSASSA-PKCS1-v1_5" }, + privateKey, + textEncoder.encode(signingInput), + ); + return `${signingInput}.${encodeBytesAsBase64Url(signature)}`; +}Then
signRs256andsignCanonicalRs256only build their encoded payload. Keep the distincttypvalue from the previous comment by passing it as a parameter.🤖 Prompt for 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. In `@packages/server/src/lib/sign-rs256.ts` around lines 71 - 94, Extract the shared signing flow from signRs256 and signCanonicalRs256 into one internal helper that accepts the encoded payload and typ value, including key import, header encoding, signing, and result assembly. Update both public functions to only construct their respective payload encoding and delegate to the helper, preserving each function’s existing typ value.
78-82: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse a distinct
typfor ledger signatures.
signCanonicalRs256currently adds ledger entries withtyp: "JWT"and the RS256kid; use an explicit ledger/JWS media type so verifiers can reject ledger entries in token/bearer contexts before signature handling. Update ledger-signing tests that decode the JWS header if it assertstyp.🤖 Prompt for 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. In `@packages/server/src/lib/sign-rs256.ts` around lines 78 - 82, Update signCanonicalRs256’s encodedHeader to use the explicit ledger/JWS media type instead of typ: "JWT", while preserving the existing alg and kid values. Adjust ledger-signing tests that decode or assert the JWS header to expect the new typ.packages/server/src/storage/interface.ts (1)
470-470: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
AttestationLedgerEntryType | stringerases the union.TypeScript reduces
AttestationLedgerEntryType | stringtostring. Callers get no checking and no completion for the known entry types. If arbitrary types must remain allowed, use thestring & {}idiom to keep completions; otherwise use the union alone.♻️ Proposed change
- entryType: AttestationLedgerEntryType | string; + entryType: AttestationLedgerEntryType | (string & {});Apply the same change at Line 485 in
AppendAttestationLedgerEntryInput.Also applies to: 485-485
🤖 Prompt for 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. In `@packages/server/src/storage/interface.ts` at line 470, Update the entryType declarations in the visible interface and AppendAttestationLedgerEntryInput to avoid the collapsed AttestationLedgerEntryType | string union: use string & {} when arbitrary values remain supported, or AttestationLedgerEntryType alone when they are not. Preserve the intended known-type completions and type checking.
🤖 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 `@packages/server/src/lib/sponsor-binding.ts`:
- Around line 198-206: The sponsor-proof flow permits repeated IdP JWKS fetches
triggered by an unverified kid. In
packages/server/src/lib/sponsor-binding.ts:198-206, add a per-issuer cooldown
before the forced `#resolveJwks`(config, true) call and enforce a minimum floor
for parseCacheSeconds so max-age=0 cannot disable caching; in
packages/server/src/routes/sponsors.ts:46-64, apply the existing
identityCreateRateLimiter pattern to POST /proof, keyed by organization and API
key.
- Around line 574-586: Update mapSponsorId so every claim is encoded with
encodeBytesAsBase64Url before concatenating it with prefix, eliminating the
literal-versus-encoded ambiguity. Preserve the existing prefix validation and
SPONSOR_ID_PATTERN validation, and apply the change consistently to all claim
values.
In `@packages/server/src/routes/attestations.ts`:
- Around line 230-240: Refactor signLedgerPayload to resolve and import the
signing key and compute the kid once, returning or capturing a signer that
reuses the imported CryptoKey. Initialize this signer once before the commit
loop in the finalize handler and once in the grant handler, then pass each
payload through it; import and use importRsaPrivateKey from ../lib/sign-rs256.js
while preserving the existing key validation and kid behavior.
In `@packages/server/src/routes/identities.ts`:
- Around line 565-567: Update the identity-created observer event payload to use
the authoritative local sponsorBinding computed in the surrounding creation
flow, rather than createdIdentity.sponsorBinding or a legacy fallback. Preserve
the ledger’s OIDC/legacy binding value and allow persistence mismatches to
remain visible.
In `@packages/server/src/storage/sqlite.ts`:
- Around line 4543-4563: Update normalizeSponsorBinding so a declared mode of
"oidc" with invalid or incomplete issuer, subject, or iat is rejected rather
than converted to legacy. Preserve the existing normalized OIDC field
construction, and return { mode: "legacy" } only when the binding is absent or
explicitly legacy.
- Around line 2297-2311: The attestation transaction blocks around the identity
and related storage methods must reject an already-active transaction before
starting. Add one shared helper that checks db.inTransaction, throws explicitly
when true, then performs BEGIN IMMEDIATE; replace the direct BEGIN IMMEDIATE
calls in all referenced methods with it, while retaining each method’s existing
rollback and commit flow.
- Around line 3100-3133: Update prepareAttestationLedgerEntry so entryHash is
computed from a canonical preimage containing all ledger metadata—orgId, orgSeq,
entryType, optional identifiers, payloadJson, jws, prevHash, and
createdAt—rather than only payloadJson and prevHash. Compute normalized jws and
createdAt once, reuse them in the returned PreparedAttestationLedgerEntry, and
update the attestation recomputation assertions to use the identical preimage
and field ordering.
---
Nitpick comments:
In `@packages/server/src/__tests__/attestations.test.ts`:
- Around line 142-146: Rename the variable `expired` in the attestation grant
test to `grantBeforeExpiry`, updating its assertion reference while preserving
the existing grant retrieval and expiry update behavior.
- Around line 106-114: Update the attestation payload assertion in the test
around the payload deep-equality check: remove the self-referential ts:
payload.ts expectation and add a separate assertion that validates the
timestamp’s required shape and value, including detection of missing, empty, or
malformed timestamps. Preserve the existing assertions for the other payload
fields.
In `@packages/server/src/__tests__/sponsor-oidc-binding.test.ts`:
- Around line 104-124: Register each createTestApp result for cleanup through
the test context by calling its close() method in t.after, matching the existing
startOidcFixture cleanup pattern. Apply this to all tests in
sponsor-oidc-binding.test.ts, including the app created in the test containing
identityCreatedEvent, so every backing SQLite storage handle is closed after its
test.
In `@packages/server/src/db/migrations/0007_attestation_ledger.sql`:
- Around line 39-44: Remove the redundant CREATE INDEX statement for
idx_attestation_ledger_org_seq, while preserving the UNIQUE (org_id, org_seq)
constraint and the other unique constraint in the attestation ledger definition.
In `@packages/server/src/index.ts`:
- Around line 32-35: Export JsonWebKeySet from sponsor-binding.ts, then add it
to the public type re-exports in the index.ts export block alongside
SponsorFederationConfig and SponsorFederationMap.
In `@packages/server/src/lib/events.ts`:
- Around line 1-7: Update IdentityCreatedPayload to use a top-level import type
for SponsorBinding from `@relayauth/types`, then replace the inline import type
expression on sponsorBinding with the imported symbol while preserving the
payload shape.
In `@packages/server/src/lib/sign-rs256.ts`:
- Around line 71-94: Extract the shared signing flow from signRs256 and
signCanonicalRs256 into one internal helper that accepts the encoded payload and
typ value, including key import, header encoding, signing, and result assembly.
Update both public functions to only construct their respective payload encoding
and delegate to the helper, preserving each function’s existing typ value.
- Around line 78-82: Update signCanonicalRs256’s encodedHeader to use the
explicit ledger/JWS media type instead of typ: "JWT", while preserving the
existing alg and kid values. Adjust ledger-signing tests that decode or assert
the JWS header to expect the new typ.
In `@packages/server/src/lib/sponsor-binding.ts`:
- Around line 510-518: Update the object construction around grantTtlSeconds,
maxIdTokenAgeSeconds, and clockSkewSeconds to call optionalInteger once per
field, store or reuse each result for both the undefined check and emitted
property, and preserve the existing omission behavior when a value is undefined.
In `@packages/server/src/routes/attestations.ts`:
- Around line 293-295: Remove the redundant normalizeRequiredString helper and
replace every call to it with normalizeOptionalString directly. Preserve the
existing undefined handling at each call site.
- Around line 41-49: Move the authenticateAndAuthorizeFromContext call to the
beginning of the handler, before parseJsonObjectBody and the body.late
validation. Preserve the existing unauthorized response and only parse or
validate GrantRequest after authentication succeeds.
In `@packages/server/src/routes/sponsors.ts`:
- Around line 65-76: Update the non-SponsorBindingError branch in the sponsor
proof handler’s catch block to return HTTP 500 instead of 503, while preserving
SponsorBindingError’s existing error.status response and the current response
body.
In `@packages/server/src/server.ts`:
- Line 64: Add cache observability for the process-wide
sharedSponsorOidcService: instrument JWKS cache hits and forced refreshes, and
record IdP fetch latency in the sponsor-binding refresh/fetch flow. Ensure the
metrics are emitted for the existing cache and refresh paths without changing
cache behavior.
In `@packages/server/src/storage/interface.ts`:
- Line 470: Update the entryType declarations in the visible interface and
AppendAttestationLedgerEntryInput to avoid the collapsed
AttestationLedgerEntryType | string union: use string & {} when arbitrary values
remain supported, or AttestationLedgerEntryType alone when they are not.
Preserve the intended known-type completions and type checking.
🪄 Autofix
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 Plus
Run ID: e247d7e9-d6c8-4127-943a-5f3668973a4c
📒 Files selected for processing (30)
README.mdpackages/core/src/scope-parser.tspackages/sdk/python/relayauth/__init__.pypackages/sdk/python/relayauth/client.pypackages/sdk/python/relayauth/types.pypackages/sdk/python/tests/test_relayauth.pypackages/sdk/typescript/src/__tests__/client-identities.test.tspackages/sdk/typescript/src/client.tspackages/sdk/typescript/src/scope-parser.tspackages/server/src/__tests__/attestations.test.tspackages/server/src/__tests__/get-identity.test.tspackages/server/src/__tests__/list-identities.test.tspackages/server/src/__tests__/sponsor-oidc-binding.test.tspackages/server/src/__tests__/test-helpers.tspackages/server/src/db/migrations/0007_attestation_ledger.sqlpackages/server/src/env.tspackages/server/src/index.tspackages/server/src/lib/canonical-json.tspackages/server/src/lib/events.tspackages/server/src/lib/sign-rs256.tspackages/server/src/lib/sponsor-binding.tspackages/server/src/routes/attestations.tspackages/server/src/routes/identities.tspackages/server/src/routes/sponsors.tspackages/server/src/server.tspackages/server/src/storage/identity-types.tspackages/server/src/storage/interface.tspackages/server/src/storage/sqlite.tspackages/types/src/identity.tspackages/types/src/scope.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Bind every ledger column into the chain hash. The preimage covered only payloadJson and prevHash, so entryType, jws, createdAt and the identifiers were rewritable with every hash still verifying — an attestation.late entry could be relabelled issued, or a signature swapped between entries. Refuse to nest a storage transaction. SQLite has no nested transactions, so a second BEGIN IMMEDIATE threw and the surrounding catch rolled back the outer transaction. One helper now tracks the open transaction per connection and fails explicitly instead. Re-resolve discovery on the forced JWKS refresh. A provider that rotates its signing key and its jwks_uri together published the new key only at the new URI, so the forced refresh refetched the retired one and every sponsor proof failed until the discovery cache expired. The path stays behind the per-issuer cooldown, so an unknown kid still cannot amplify. Import the signing key once per finalize request instead of once per commit. Verification: build, typecheck, test (480 pass, 0 fail) and the SDK contract check all exited 0. The two new OIDC tests were confirmed red before the fix.
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 `@packages/server/src/routes/discovery.ts`:
- Line 37: Add grant to the OpenAPI components.schemas.Action enum so the
published contract includes this supported built-in scope action. Update the
schema definition rather than changing the discovery route or existing parsers.
In `@packages/server/src/routes/sponsors.ts`:
- Around line 41-45: Update the sponsor-proof handler in
packages/server/src/routes/sponsors.ts, including the rate-limit branch and all
custom 400, 401, 403, 429, and 503 branches, to emit the ErrorResponse fields
type, title, status, code, and message. Keep specs/openapi.yaml lines 272-307
referencing ErrorResponse only after every documented failure uses that shape,
and synchronize specs/openapi.yaml lines 1631-1636 with the handler’s 503
response.
In `@specs/openapi.yaml`:
- Around line 1919-1921: Update the sponsorProof description in the OpenAPI
schema to explicitly state that the required proof intent is exactly
identity.create, rather than describing it generically as intent-bound. Preserve
the existing OIDC sponsor-binding context.
🪄 Autofix
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 Plus
Run ID: 07efae1a-7f06-44ab-8101-01cd32c6d649
📒 Files selected for processing (11)
packages/server/src/__tests__/attestations.test.tspackages/server/src/__tests__/sponsor-oidc-binding.test.tspackages/server/src/__tests__/storage-sqlite.test.tspackages/server/src/__tests__/well-known-discovery.test.tspackages/server/src/lib/sponsor-binding.tspackages/server/src/routes/attestations.tspackages/server/src/routes/discovery.tspackages/server/src/routes/identities.tspackages/server/src/routes/sponsors.tspackages/server/src/storage/sqlite.tsspecs/openapi.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/server/src/routes/identities.ts
- packages/server/src/storage/sqlite.ts
- packages/server/src/lib/sponsor-binding.ts
- packages/server/src/routes/attestations.ts
…binding Add trigger and grant to the OpenAPI Action enum. The enum had drifted from the parser's ACTIONS list, so a client validating scopes against the spec rejected two actions the server accepts — grant among them, which is the action the attestation endpoint requires. State the required sponsor-proof intent. The identity-create field described the proof as intent-bound without naming the intent, leaving callers to infer that any intent-bound proof is accepted; identity creation requires exactly identity.create, and a proof bound to another intent or another sponsor is refused. Contract check and the full suite exit 0.
Resolves conflicts with #75 (sponsor-binding OIDC proofs), which landed on main and independently touched several of the same files: - Adopted main's beginSqliteTransaction/commitSqliteTransaction guard (refuses nested transactions) for all ledger transaction call sites. - Adopted main's full-coverage attestationLedgerEntryHash (hashes orgId/orgSeq/entryType/jti/commitSha/repo/agentId/sponsorId/payloadJson/ jws/createdAt/prevHash, not just payloadJson+prevHash) plus its test coverage for tamper detection on each of those fields. - Kept this branch's redeemedAt-inside-transaction, entry_hash org scoping, sparse-array/proto canonicalization, sha lowercasing, ledger signing-key hoisting, and lastInsertRowid optimization. - Identity creation now always writes a signed identity.created ledger entry via createIdentityWithLedgerEntry: OIDC-bound orgs use the sponsor-proof ledger payload signed by SponsorOidcService (main's path, previously only wired for OIDC mode), legacy orgs use this branch's ledger-signing helper (previously only wired for legacy mode, since main still called the un-ledgered storage.identities.create for that path). Additional review fixes from the second cubic/CodeRabbit pass: - ledger-signing: fail closed when RELAYAUTH_SIGNING_KEY_PEM_PUBLIC is unset instead of signing with a kid that's never published at /.well-known/jwks.json. - sqlite: log (not just silently null) when a persisted attestation grant row fails to hydrate, so a corrupted row is an operator signal even though it still surfaces as invalid_finalize_key to the caller. - discovery: the relayauth:attest:grant scope only authorizes the wildcard path today (matchPath requires an exact-or-"*" match against the fixed required scope), so stop advertising repo-scoped examples that would always 403. - canonical-json: also reject a sparse array whose hole is compensated by a non-index own property (Object.keys().length alone misses it; findIndex visits every index including holes). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2 participants
Summary
identity.createorapprovalidentity.createproof when an organization enables OIDC sponsor bindingissuer,subject,iat, optionaljti)identity.created, the proof intent, and exact canonical RS256-signed evidence to the per-organization append-only ledger in the same transaction as identity creationOrganizations without an OIDC binding entry retain the existing legacy behavior. Invalid federation configuration, cross-purpose proofs, inactive ordinary attestation grants, malformed stored bindings, and ledger persistence failures fail closed. Sponsor-proof exchange is rate-limited per organization and credential, and unknown IdP key IDs cannot drive unbounded JWKS refreshes.
Validation
payload_json, includesintent: "identity.create", and verifies the RS256 signatureLanding status
Kept unmerged for coordinated landing. This branch is based on the shared attestation-ledger contract and includes the atomic identity-create integration.