fix(world-id): stop trusting foreign on-chain RP registrations (H1 #3910854) - #2221
fix(world-id): stop trusting foreign on-chain RP registrations (H1 #3910854)#2221Gr1dlock wants to merge 14 commits into
Conversation
The v4 rp-status production reconciliation path promoted a managed RP to `registered` from on-chain `initialized && active` alone, without comparing the on-chain signer to the Portal's stored `signer_address` — a check the staging path in the same file already performed. After a failed managed registration, a party who won the permissionless on-chain `register()` for that rp_id with their own signer could flip the row to `registered`, so proof-context served the legitimate app's verified branding bound to an attacker-controlled OPRF signer (impersonation). Extract the signer-trust logic into a shared `canTrustOnChainSigner` helper used by both the production and staging paths so they can't diverge again; preserve the DB status and log a warning on mismatch. Fixes H1 #3869823. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…signer Codex review (P2): when a managed row is still `pending` and the rp_id becomes initialized on-chain under a signer the Portal doesn't recognize (foreign takeover, or a rotation that never settled), the earlier signer check preserves `pending` but leaves `productionInitialized = true`. The existing pending->failed timeout only fires for `!productionInitialized`, so the row was wedged in `pending` forever — the dashboard polled endlessly and never exposed the retry path. Add an untrusted-initialized timeout that fails such rows once past grace, clocked on updated_at so an in-flight signer rotation (which bumps updated_at and settles within seconds) is never prematurely failed. Hoist the updated_at grace flag and reuse it for the staging timeout. Adds tests for both the takeout->failed transition and the rotation-not-failed guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codex review (P1): the untrusted-initialized timeout used the 5-minute grace, but a managed signer-rotation UserOp stays includable on-chain for USER_OP_MAX_VALIDITY_MS (30m). Failing after 5m could mark an RP whose rotation is still landable `failed`, and that non-pending response is cached for an hour — so a rotation that lands at, say, minute 10 leaves the DB `failed` over a trusted on-chain `registered`, blocking proof-context/verify until manual retry or cache expiry. Key the untrusted-initialized timeout off USER_OP_MAX_VALIDITY_MS + the settlement margin (updated_at clock) so an unsettled op is only failed once it is provably dead. Staging timeout keeps its short grace. Tests updated to cover the validity-window boundary (in-flight rotation preserved; provably-dead op failed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…910854) H1 #3910854 (and the still-open SEC-2787 / H1 #3869823) point at the same Portal-side defect from two angles: `rp_id` is `uint64(keccak256(app_id))` over a public `app_id`, and on-chain `register()` is permissionless, zero-fee and first-come — so anyone can compute an app's rp_id and claim it. The Portal then treated that foreign registration as its own. - `/api/v4/rp-status` promoted a managed RP to `registered` from on-chain `initialized && active` alone. The staging branch in the same file already compared the on-chain signer to the stored `signer_address`; the production branch did not, so a squatter flipped the row to `registered` and `proof-context` served the legitimate app's verified branding bound to the attacker's OPRF signer. Both branches now go through a shared `canTrustOnChainSigner`, preserve the stored status on mismatch, and gate the DB writeback on the trust flag. A managed row left wedged `pending` by an untrusted registration now fails once the UserOp validity window has elapsed, so the retry path is reachable instead of polling forever. - The MCP `get_world_id_registration_status` tool had the identical production-vs-staging split and wrote the poisoned status back too. - Managed registration read no on-chain state before spending a KMS manager key on a UserOp the contract rejects with `IdAlreadyInUse`. It now pre-checks the registry and returns `rp_id_taken`. The read is deliberately non-fatal — it prevents a wasted submission and a confusing error, the security boundary stays in the status reconciliation. - `rp-retry` submitted an `updateRp` whenever the on-chain signer differed, even when the on-chain manager was not our KMS manager. That op always reverts, yet the handler still wrote `status: pending` — wedging the row and burning gas per click. It now compares the manager first. - `register_rp` / `rotate_signer_key` accepted the zero address as a signer (`isAddress` allows it); both now reject it via a shared `isZeroAddress`, which also replaces the local copy in `switch-to-self-managed`. The signer-mismatch warning is throttled to one per rp_id per 10 minutes (fail-open) because rp-status caches pending statuses for 1s and would otherwise emit thousands of identical lines during a live takeover. Out of scope here (cross-repo): salting rp_id derivation, and the contract's missing caller binding / reclaim path in world-id-protocol. Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
![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: b66cd51600
ℹ️ 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".
| return ( | ||
| normalizeAddress(onChainSigner).toLowerCase() === | ||
| normalizeAddress(expectedSigner).toLowerCase() | ||
| ); |
There was a problem hiding this comment.
When an attacker front-runs the permissionless registration using the expected signer address but their own manager address, this signer-only comparison returns true and the status handlers promote the foreign RP to registered. The expected signer is visible in the registration calldata built by submitRegisterRpTransaction, and the attacker's manager can subsequently call updateRp to replace it; after that mismatch, the handler merely preserves the already-registered DB status, so /api/v4/verify and proof-context continue treating the compromised registration as active. Managed registrations must also verify the on-chain manager against the Portal-controlled manager rather than treating signer equality as proof of ownership.
Useful? React with 👍 / 👎.
Addresses the P1 from Codex review on #2221: the signer-only trust check it replaced was bypassable. A Portal registration publishes its intended signer in the `register` calldata, which stays readable on-chain even when the op failed (the SEC-2787 reporter quoted exactly that). So an attacker can re-register the same rp_id with the victim's signer but their own manager; the signer comparison then passes, the row is promoted to `registered`, and the attacker's manager calls `updateRp` to swap the signer in afterwards. The later mismatch only *preserves* the already-`registered` status, so proof-context keeps serving the app's verified branding over the attacker's OPRF signer. The manager is the real root of ownership — it is the only role the contract lets update an RP, and only the Portal can sign for its KMS manager key — so `canTrustOnChainSigner` becomes `evaluateOnChainTrust`, which requires BOTH roles to match. Note the reverse case is harmless: an attacker who passes our manager address controls nothing, since they cannot sign for that key. The manager address is not stored, so it is derived from the row's `manager_kms_key_id` via a new `resolveManagerAddress` with a process-lifetime cache — a KMS key's address comes from its immutable public key, so it never changes, and rp-status is a public polling endpoint that must not issue a KMS call per poll. Trust is now tri-state rather than boolean, because "cannot verify" and "is foreign" need different handling: `unknown` (manager address unresolvable, i.e. KMS down) preserves the status like `untrusted`, but is deliberately excluded from the wedged-pending timeout. Only `untrusted` may fail a row. Treating a KMS outage as evidence of takeover would turn a dependency blip into failed registrations for every healthy pending RP. Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19c376d251
ℹ️ 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".
| const expectedManager = dbRecord.manager_kms_key_id | ||
| ? await resolveManagerAddress(dbRecord.manager_kms_key_id) |
There was a problem hiding this comment.
When RP_REGISTRY_KMS_REGION differs from the general AWS_REGION_NAME, this call omits the region used to create and sign with the RP manager key. resolveManagerAddress consequently constructs its KMS client with AWS_REGION_NAME, so GetPublicKey fails and returns null; every managed registration is then classified as unknown, preventing trusted on-chain statuses from ever being written back and leaving registrations pending indefinitely. The MCP status path makes the same regionless call, so pass the configured RP registry KMS region in both places or make the resolver default to it.
Useful? React with 👍 / 👎.
Resolves the overlap with #2168 (dedicated manager keys). The collision pre-check keeps its place ahead of ClaimRpRegistration and the new KMS key creation, so a foreign on-chain rp_id still costs no key and no UserOp. The registration suite needed its own on-chain default: the top-level mock models an already-registered RP for the rotation and deactivation suites, which the collision guard correctly rejects as foreign. Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e96cdc9e3
ℹ️ 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".
| if (!expectedManager) { | ||
| return "unknown"; |
There was a problem hiding this comment.
Self-managed registrations are supposed to take on-chain state as authoritative, but this helper infers that case only from !expectedSigner. Legacy self-managed rows can still have signer_address populated because the original rp_registration.signer_address column was NOT NULL before 1770753504000_allow_nullable_signer_address, and those rows have no Portal manager_kms_key_id, so this new manager check returns unknown; /api/v4/rp-status and the MCP status tool then preserve pending/failed forever instead of writing registered after the developer completes on-chain setup. Please pass the RP mode or ignore expectedSigner for mode === "self_managed" before requiring a Portal manager.
Useful? React with 👍 / 👎.
…igner evaluateOnChainTrust inferred "self-managed, so on-chain is authoritative" from an absent signer_address. That holds today — the self-managed insert writes a null signer, and switch-to-self-managed nulls signer_address and manager_kms_key_id in the same mutation — but it is a convention spread across several files rather than a stated invariant. The day a self-managed row records the developer's on-chain signer for display, the inference breaks in the worst direction: such a row has no Portal manager key, so trust evaluates to `unknown`, and rp-status plus the MCP status tool would preserve `pending` forever instead of promoting the row once the developer finishes registering on-chain. Reading `mode` directly removes the coupling. Raised as a P2 by Codex on the merge commit. Its stated premise — that legacy self-managed rows already carry a signer because the column was NOT NULL before 1770753504000 — does not hold: self-managed mode and that migration shipped in the same commit (#1777), so no such row was ever created. The suggestion is still right, as the invariant was implicit. Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e411f8e884
ℹ️ 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".
| // Not provably ours. Preserve the DB status instead of promoting — for a | ||
| // managed RP this is an in-flight signer rotation, a foreign takeover of | ||
| // the rp_id, or (when `unknown`) an unresolvable manager key. | ||
| productionStatus = currentDbStatus; |
There was a problem hiding this comment.
When a managed row is already registered but the on-chain RP is now untrusted because the manager is foreign, this branch preserves and returns the DB status, then the response is cached as active; /api/v4/verify and proof-context gate on that DB registered state, so an RP that this check has just proven is not Portal-owned can continue to be treated as usable. Fresh evidence in this revision is that the new untrusted timeout below only handles currentDbStatus === Pending, so already-registered rows never leave the active state after a foreign-manager mismatch.
Useful? React with 👍 / 👎.
An already-`registered` row that reads `untrusted` is categorically worse than a pending one: proof-context and /api/v4/verify gate on the stored status, so the Portal is actively serving that app's verified branding over an OPRF signer we have just proven is not ours. Until now both cases produced the same throttled warning, so the severe one was indistinguishable from an ordinary in-flight rotation and nothing could alert on it. Still does not demote the row. This is an unauthenticated polling endpoint, and if any cohort of rows carries a stale manager_kms_key_id then auto-demoting would take World ID verification down for working apps on the strength of our own bad data — the wrong direction to be wrong in. The status is preserved and the condition escalated to error for out-of-band resolution; the two logged signer values are the discriminator, since a real takeover mismatches both manager and signer whereas our own key drift leaves the on-chain signer still matching signer_address. Raised as a P1 by Codex. The remediation for rows poisoned before this ships remains the operational audit noted in the PR description. Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 848d1d86b2
ℹ️ 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".
| const expectedManager = dbRecord.manager_kms_key_id | ||
| ? await resolveManagerAddress(dbRecord.manager_kms_key_id) | ||
| : null; |
There was a problem hiding this comment.
When the registration transaction is submitted but the final UpdateRpRegistration write throws, submitManagedRpRegistration intentionally leaves the claimed row in place without manager_kms_key_id. This new trust gate then sets expectedManager to null, so once the UserOp lands evaluateOnChainTrust returns unknown and the initialized RP is never promoted; the initialized+unknown case also skips the untrusted timeout, and rp-retry rejects rows without a manager key, leaving a Portal-owned registration stuck in pending with no working retry. Please persist or repair the manager key before relying on this check, or add a recovery path for these partial rows.
Useful? React with 👍 / 👎.
…egion Both trust call sites resolved the manager address without a region, so getKMSClient fell back to AWS_REGION_NAME while managed keys are created in RP_REGISTRY_KMS_REGION. Where those differ, GetPublicKey fails for every managed row, every verdict becomes `unknown`, and no managed RP is ever promoted to `registered` — the trust gate would have taken managed registration status down wholesale. Codex flagged this on an earlier revision and it was never addressed. Also stop wedging a managed row that is initialized on-chain but has no manager_kms_key_id recorded. submitManagedRpRegistration deliberately keeps the claimed row when the DB write after a successful on-chain submission throws, and such a row can never resolve to `trusted`. `unknown` is otherwise excluded from the timeout on purpose — it usually means a KMS outage, and failing healthy registrations because our own dependency is down is the wrong way to be wrong — but a missing key is a durable data defect, not an outage, so it now fails and is logged at error instead of polling `pending` forever. Before the trust gate these rows were promoted on `initialized && active` alone and healed themselves. rp-retry still rejects rows with no manager key, so the retry button on one reports a clear error rather than recovering. Fixing that properly means persisting manager_kms_key_id BEFORE the on-chain submission, which needs a new mutation (UpdateRpRegistration requires a non-null operation_hash) and a codegen run against a live Hasura. Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 306bf1a527
ℹ️ 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".
|
|
||
| try { | ||
| const kmsClient = await getKMSClient(kmsRegion); | ||
| const address = await getEthAddressFromKMS(kmsClient, managerKmsKeyId); |
There was a problem hiding this comment.
Fresh evidence after passing RP_REGISTRY_KMS_REGION is that this call still lets getEthAddressFromKMS call resolveKeyId(keyId) without a region. When manager_kms_key_id (or the shared manager key) is stored as a bare KMS key ID and KMS_LEGACY_ACCOUNT_ID is set, resolveKeyId builds an ARN with AWS_REGION_NAME; if that differs from RP_REGISTRY_KMS_REGION, GetPublicKey fails, this helper returns null, and managed RPs stay at trust unknown instead of syncing to registered. Pass the registry region through key resolution or pre-resolve the key ID here.
Useful? React with 👍 / 👎.
| rpId, | ||
| primaryConfig.contractAddress, | ||
| ); | ||
| if (existingOnChainRp.initialized) { |
There was a problem hiding this comment.
Because this on-chain collision guard runs before ClaimRpRegistration, an app that is already registered in Portal and initialized on-chain also satisfies this branch. A repeat Hasura/MCP registration call for a legitimately configured app will now return rp_id_taken with the foreign-manager/contact-support message instead of the existing already_registered response from the DB claim below, so callers get a misleading unrecoverable error. Move the DB existence/claim check before treating any initialized on-chain RP as foreign, or verify there is no Portal row first.
Useful? React with 👍 / 👎.
Two more Codex findings on the trust gate, both real. The collision check ran before ClaimRpRegistration, but a live managed RP reads as initialized on-chain exactly like a squatter's. So a repeat registration of a perfectly healthy app was answered with `rp_id_taken` and a contact-support message instead of the accurate `already_registered`. Claiming the slot first means only apps with no Portal row reach that check, so an initialized reading really is someone else's. The check now releases the slot it claimed before returning, otherwise every later attempt would report `already_registered` for a row that never registered anything. Passing RP_REGISTRY_KMS_REGION to getKMSClient was also not enough on its own: getEthAddressFromKMS resolved the key ID without a region, so a bare key ID plus KMS_LEGACY_ACCOUNT_ID expanded into an ARN carrying AWS_REGION_NAME and GetPublicKey still failed. The region now threads through key resolution at all three manager-address sites — the trust check, the shared-key registration path and rp-retry. The registration path matters as much as the check: if the address written at registration and the one derived for the comparison disagree, the comparison is meaningless. Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d7b00565a
ℹ️ 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".
| } else { | ||
| logger.warn( | ||
| "On-chain RP is not provably Portal-owned; preserving DB status", |
There was a problem hiding this comment.
When an MCP client polls get_world_id_registration_status after a managed registration is front-run on-chain, this untrusted branch only preserves the DB value (typically pending) and the later productionSynced guard skips any write because productionTrust !== "trusted"; unlike /api/v4/rp-status, this path never applies the untrusted-initialized timeout that moves the row to failed. Clients using the MCP status tool therefore keep seeing pending indefinitely, and the existing pending row keeps follow-up MCP registration/rotation actions from reaching a recoverable terminal state.
Useful? React with 👍 / 👎.
…atus tool The MCP status tool mirrors rp-status's trust check but had no timeout, so the trust gate wedged it the same way it wedged rp-status: after a managed registration is front-run on-chain the row is preserved at `pending`, the sync guard skips the write because the reading is not trusted, and an MCP-only client polls that forever. The surviving pending row also blocks follow-up MCP registration and rotation calls from reaching a recoverable state. The decision now lives in one shared shouldFailUntrustedRegistration helper used by both readers, including the UserOp validity window, rather than a second copy of a rule with several conditions. Two copies of one rule drifting apart is the vulnerability this PR exists to fix, so duplicating it here would have been the same mistake in a new place. Needs `updated_at` in the MCP app-context query to know whether the window has elapsed; added the same way this PR already added manager_kms_key_id, by hand to the .graphql and its generated document, since codegen needs a live Hasura. An unparseable timestamp yields NaN, every comparison against it is false, and the row is therefore left alone — the safe direction. Raised as a P2 by Codex. Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
…rror Two Codex findings, both the same shape: an unknown state was being reported as something permanent. Managed adoption converted an unresolved shared manager address straight to "not ours", so a transient KMS failure told the developer of a pre-claimed app that someone else owns their rp_id and to contact support. Unresolvable is now its own outcome — a retryable kms_error, with the claimed slot released so the retry is not met with already_registered. The self-managed placeholder-signer guard failed open whenever it could not run: no placeholder configured, or getRpRegistryConfig() null for an unrelated reason. For an app the Portal has pre-claimed that inserts a row which rp-status promotes (it trusts self-managed rows by mode) against a signer that can never sign. What skipping means now depends on whether pre-claims can exist at all: - pre-registration enabled but misconfigured -> config_error, a deploy fault - placeholder set but the chain unreadable -> rpc_error, retryable - placeholder unset, never enabled -> skip; no claims exist That last case is every environment that has not run the tool, so it must stay open — the alternative is making self-managed registration depend on config it has no reason to need. Noted the operational invariant in the code: once pre-registration has run somewhere, RP_ID_PRE_REGISTRATION_SIGNER has to stay set there, because the claims outlive the flag. Rebased onto #2221, resolving the overlap where the collision check moved after the DB claim: adoption keeps the slot, both failure paths release it. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64f6e87a98
ℹ️ 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".
| stagingStatus = | ||
| stagingTrust === "trusted" | ||
| ? onChainMappedStatus | ||
| : currentDbStagingStatus ?? onChainMappedStatus; |
There was a problem hiding this comment.
When a managed RP has no staging_status recorded and the staging contract is initialized by a foreign manager (or the manager check is unknown), this fallback still returns the mapped on-chain value, e.g. registered, even though stagingTrust !== "trusted". That makes /api/v4/rp-status report an untrusted staging registration as active instead of preserving the DB state (null/not mirrored), and the MCP status path has the same fallback, so clients can stop polling on a false staging success.
Useful? React with 👍 / 👎.
…'s own When nothing had been mirrored to staging yet, an untrusted staging reading fell back to the on-chain mapping, so a squatter's staging registration was reported as this app's `registered` and a client would stop polling on someone else's success. The production branch already preserves the stored value for exactly this reason; staging now follows the same rule, including when the stored value is null. The fallback predates this PR (it is on main), but the PR is what makes these trust verdicts mean anything, and the MCP status path carries the same pattern — both are fixed together. Raised as a P2 by Codex. Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5302dfaea8
ℹ️ 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".
| }); | ||
| // Release the slot: the app is not registered, and holding the row would | ||
| // make every later attempt report `already_registered` instead. | ||
| await getDeleteRpSdk(client).DeleteRpRegistration({ rp_id: rpIdString }); |
There was a problem hiding this comment.
When the pre-check has already proven existingOnChainRp.initialized, a transient failure from this cleanup mutation is caught by the broad catch below as though the read failed, so the flow continues into KMS creation and submits a registration UserOp for an RP ID it already knows is taken. That can leave the claimed row wedged behind a doomed operation instead of returning rp_id_taken or a DB cleanup error; keep the contract read's non-fatal catch separate from the slot-release failure.
Useful? React with 👍 / 👎.
The collision pre-check wrapped both the contract read and the slot release in one try. The read is deliberately non-fatal, so a transient failure from the release mutation was logged as "could not pre-check; continuing" and the flow carried on into KMS key creation and a registration UserOp for an rp_id it had already proven was taken — leaving the claimed row wedged behind a doomed operation, which is precisely what this check exists to prevent. Only the read is inside the try now. A failed release is logged at error and the answer stays rp_id_taken: the id genuinely is taken, so that is the actionable fact, and the wedged row is an ops signal rather than a reason to submit anyway. Raised as a P2 by Codex. Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e37b25f78f
ℹ️ 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".
| export async function getEthAddressFromKMS( | ||
| client: KMSClient, | ||
| keyId: string, | ||
| region?: string, |
There was a problem hiding this comment.
In deployments where RP_REGISTRY_KMS_REGION differs from AWS_REGION_NAME, KMS_LEGACY_ACCOUNT_ID is set, and RP registry keys are stored as bare key IDs, this new region-aware address lookup gets past GetPublicKey, but the same keys are later used by submitRegisterRpTransaction/submitRotateSignerTransaction through signEthDigestWithKms; signWithKms still calls resolveKeyId(keyId) without the region, so SignCommand builds an ARN in AWS_REGION_NAME and the registration/retry fails at signing. Please thread the region through the signing path as well.
Useful? React with 👍 / 👎.
…rror Two Codex findings, both the same shape: an unknown state was being reported as something permanent. Managed adoption converted an unresolved shared manager address straight to "not ours", so a transient KMS failure told the developer of a pre-claimed app that someone else owns their rp_id and to contact support. Unresolvable is now its own outcome — a retryable kms_error, with the claimed slot released so the retry is not met with already_registered. The self-managed placeholder-signer guard failed open whenever it could not run: no placeholder configured, or getRpRegistryConfig() null for an unrelated reason. For an app the Portal has pre-claimed that inserts a row which rp-status promotes (it trusts self-managed rows by mode) against a signer that can never sign. What skipping means now depends on whether pre-claims can exist at all: - pre-registration enabled but misconfigured -> config_error, a deploy fault - placeholder set but the chain unreadable -> rpc_error, retryable - placeholder unset, never enabled -> skip; no claims exist That last case is every environment that has not run the tool, so it must stay open — the alternative is making self-managed registration depend on config it has no reason to need. Noted the operational invariant in the code: once pre-registration has run somewhere, RP_ID_PRE_REGISTRATION_SIGNER has to stay set there, because the claims outlive the flag. Rebased onto #2221, resolving the overlap where the collision check moved after the DB claim: adoption keeps the slot, both failure paths release it. Co-Authored-By: Claude <noreply@anthropic.com>
yup cast the string "false" to boolean false, so a hand-built curl or wrapper script could make this endpoint spend real gas without ever passing a JSON boolean — and explicit opt-out is the whole safety property. Not fixed with .strict(): yup applies defaults during casting, which strict mode skips, so .strict().default(true) leaves an ABSENT dry_run as undefined and therefore falsy. That inverts the guarantee instead of tightening it — the local test suite caught it immediately. The check now runs on the raw body before yup sees it, and a test pins the omitted case so the same trap cannot be reintroduced. Also drops a duplicated foreign-manager block the previous rebase left behind. It sat after the adoption branch and returned rp_id_taken unconditionally, so adoption could never take effect — caught by the adoption test failing. Rebased onto #2221's slot-release fix; the release is now a shared helper used by both the kms_error and rp_id_taken paths. Co-Authored-By: Claude <noreply@anthropic.com>
… lookup The previous region fix got GetPublicKey right and stopped there. The same keys are then used to sign, and signWithKms still called resolveKeyId without a region, so SignCommand built an ARN in AWS_REGION_NAME. In a deployment where RP_REGISTRY_KMS_REGION differs and keys are stored as bare IDs, that moves the failure from status reconciliation to submission — registrations and retries fail at signing instead — which is not an improvement. signEthDigestWithKms now takes the region and passes it to both the address lookup and SignCommand, and all eight call sites in rp-transactions supply config.kmsRegion. Raised as a P2 by Codex, the third round on this one region. Recorded the two-hop trap in memory so the next person threading a KMS region checks both resolveKeyId sites rather than the obvious one. Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
PR Type
Description
Fixes H1 #3910854 — and with it the still-open SEC-2787 / H1 #3869823, whose fix in #2141 was closed unmerged, so
mainnever received it. Becauserp_idisuint64(keccak256(app_id))over a publicapp_idand on-chainregister()is permissionless, zero-fee and first-come, anyone can claim any not-yet-migrated app's rp_id — and the Portal then treated that foreign registration as its own: both/api/v4/rp-statusand the MCPget_world_id_registration_statustool promoted a managed RP toregisteredfrom on-chaininitialized && activealone (a signer check each file's own staging branch already performed), so a squatter's row flipped toregisteredandproof-contextserved the legitimate app's verified branding bound to an attacker-controlled OPRF signer. Both paths now share acanTrustOnChainSignerhelper, preserve the stored status on mismatch, gate the DB writeback on the trust flag, and fail a managed row left wedgedpendingby an untrusted registration once the UserOp validity window elapses so the retry path stays reachable instead of polling forever. Three supporting gaps in the same attack surface: managed registration now pre-reads the registry and returnsrp_id_takenrather than spending a KMS manager key on a UserOp the contract rejects withIdAlreadyInUse(deliberately non-fatal on RPC failure — the security boundary is the reconciliation, not this convenience check);rp-retrycompares the on-chain manager before submitting a rotation that always reverts yet still wrotestatus: pending, wedging the row and burning gas per click; andregister_rp/rotate_signer_keyreject the zero-address signer thatisAddresslets through, via a sharedisZeroAddressthat also replaces the local copy inswitch-to-self-managed.Two things worth flagging for review: the signer-mismatch warning is Redis-throttled to one per rp_id per 10 minutes (fail-open) because rp-status caches pending statuses for 1s and would otherwise emit thousands of identical lines during a live takeover, and this change prevents future poisoning but does not auto-heal a row already at
registeredfrom a prior exploit (mismatch preserves the stored status, matching the staging path) — that needs an operational audit of managed rows whose on-chain signer differs fromsigner_address. Salting the rp_id derivation and the contract's missing caller binding / reclaim path are out of scope here (world-id-protocol).Checklist