fix(core-contract): register_resolver has no caller authentication — critical auth bypass#244
Open
Userhorlie wants to merge 5 commits intoAlien-Protocol:mainfrom
Open
Conversation
…vailability check
…critical auth bypass
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 14 minutes and 52 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR addresses a critical authentication vulnerability in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant SDK as isUsernameAvailable
participant Hash as Username Hashing
participant Proof as Proof Generation
participant Circuit as snarkjs Circuit
participant VKey as Verification Key
participant Verify as groth16.verify
Client->>SDK: isUsernameAvailable(username, smtRoot, merkleTree)
SDK->>Hash: hashUsername(username)
Hash-->>SDK: usernameHash
SDK->>Proof: generateNonInclusionProof(usernameHash, smtRoot, merkleTree)
Proof-->>SDK: proof, witness inputs
SDK->>Circuit: fullProve(inputs, wasm, zkey)
Circuit-->>SDK: { proof, publicSignals }
SDK->>VKey: fetchVerificationKey()
VKey-->>SDK: vkey (JSON)
SDK->>Verify: groth16.verify(vkey, publicSignals, proof)
Verify-->>SDK: boolean result
SDK-->>Client: true/false
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
…critical auth bypass ci issue fix
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
zk/sdk/src/availability.ts (2)
5-10: Typenodes: anyloses type safety.The
SMTDatainterface usesanyfornodes, which bypasses TypeScript's type checking. Consider using a more specific type or generic constraint, especially since the actual circuit inputs require specific fields perNonInclusionInputintypes.ts.💡 Consider aligning with existing types
+import type { NonInclusionInput } from "./types"; + export interface SMTData { - // shape depends on your tree implementation - // keep generic for flexibility - nodes: any; + nodes: Record<string, string>; depth: number; }Or use a generic:
export interface SMTData<T = Record<string, unknown>> { nodes: T; depth: number; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@zk/sdk/src/availability.ts` around lines 5 - 10, The SMTData interface currently declares nodes: any which loses type safety; change SMTData to be generic (e.g., SMTData<T = Record<string, unknown>>) or use a concrete type that matches your circuit inputs (align with NonInclusionInput in types.ts) so nodes has a precise shape; update all usages of SMTData (e.g., function signatures, variables) to supply the appropriate generic type or the concrete type so the compiler enforces the expected fields and prevents accidental misuse.
43-46: Swallowing errors and returningfalsemay hide critical failures.Returning
falseon any error conflates "username is taken" with "system failure" (e.g., missing circuit files, network errors). Callers cannot distinguish between a valid negative result and an infrastructure problem.Consider throwing or returning a discriminated result type for non-recoverable errors.
💡 Suggested discriminated result type
type AvailabilityResult = | { status: "available"; proof: Groth16Proof } | { status: "taken" } | { status: "error"; error: Error }; export async function isUsernameAvailable( username: string, smtRoot: bigint, merkleTree: SMTData ): Promise<AvailabilityResult> { try { // ... proof generation if (isValid) { return { status: "available", proof }; } return { status: "taken" }; } catch (err) { console.error("Username availability check failed:", err); return { status: "error", error: err as Error }; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@zk/sdk/src/availability.ts` around lines 43 - 46, The current catch block in isUsernameAvailable swallows errors and returns false, which conflates "taken" with infrastructure failures; change the function to return a discriminated result type (e.g., AvailabilityResult with variants {status:"available", proof: Groth16Proof} | {status:"taken"} | {status:"error", error: Error}) instead of a boolean, update isUsernameAvailable to return {status:"available", proof} when the proof validates, {status:"taken"} when it does not, and in the catch log the original error via console.error and return {status:"error", error: err as Error}; adjust any callers of isUsernameAvailable to handle the new AvailabilityResult.zk/sdk/src/__tests__/availability.test.ts (1)
24-32: Test relies on implementation detail (error path) rather than explicit behavior.This test passes because
isUsernameAvailablecatches all errors and returnsfalse. The test description "returns false when proof fails" suggests testing proof verification failure, but it's actually testing that missing circuit artifacts trigger the catch block.For meaningful coverage, mock the dependencies and explicitly test:
- When
groth16.verifyreturnsfalse- When
generateNonInclusionProofthrows🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@zk/sdk/src/__tests__/availability.test.ts` around lines 24 - 32, The test is relying on a catch-all error path instead of explicitly exercising proof-failure and generator-error behaviors; update the tests for isUsernameAvailable to mock dependencies directly: create one test where you stub groth16.verify to return false (ensure generateNonInclusionProof returns a valid proof object) and assert isUsernameAvailable resolves to false, and a separate test where you stub generateNonInclusionProof to throw (and optionally have groth16.verify not called) and assert isUsernameAvailable resolves to false; use jest.mock/jest.spyOn to mock groth16.verify and generateNonInclusionProof, restore mocks after each test, and keep test descriptions matching the explicit scenarios.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gateway-contract/contracts/core_contract/src/lib.rs`:
- Around line 67-89: There are formatting/whitespace issues around the commit
block (lines with env.storage().persistent().has, panic_with_error!,
smt_root::SmtRoot::get_root, public_signals.old_root,
zk_verifier::ZkVerifier::verify_groth16_proof, ResolveData,
env.storage().persistent().set, and smt_root::SmtRoot::update_root) causing
cargo fmt --check to fail; run rustfmt (or execute `cargo fmt` in the
core_contract crate) to fix trailing whitespace and indentation, then re-run the
CI check and commit the formatted changes.
In `@zk/sdk/src/__tests__/availability.test.ts`:
- Around line 13-22: The test "returns true for username not in tree" asserts
only a boolean but actually exercises the catch path because circuit artifacts
and helpers are not mocked; update the test to either (A) mock
snarkjs.groth16.fullProve and groth16.verify, hashUsername,
generateNonInclusionProof, and the fetch for the verification key so
isUsernameAvailable("new_user_123", mockRoot, mockTree) executes the happy path
and assert result === true, or (B) change the test name and expectation to
reflect that it only verifies a boolean/handles the error path; target symbols:
isUsernameAvailable, groth16.fullProve, groth16.verify, hashUsername,
generateNonInclusionProof, and the global fetch used to load the verification
key.
In `@zk/sdk/src/availability.ts`:
- Around line 32-36: The code in isUsernameAvailable uses hardcoded relative
circuit files ("circuits/merkle_non_inclusion.wasm",
"circuits/merkle_non_inclusion.zkey"); change it to use the existing
MerkleProofGenerator and MerkleProofGeneratorConfig instead or accept circuit
paths via configuration: add a parameter of type MerkleProofGeneratorConfig to
isUsernameAvailable, instantiate new MerkleProofGenerator(config) and call its
proveNonInclusion (or equivalent) to obtain proof/publicSignals, and remove the
hardcoded groth16.fullProve call; also update imports to include
MerkleProofGenerator and MerkleProofGeneratorConfig from "./proof" or "./types"
as appropriate.
- Around line 52-56: The fetchVerificationKey function uses browser-only fetch
with an absolute path which fails in Node.js and assumes an HTTP endpoint;
modify fetchVerificationKey to be environment-agnostic by either (a) accepting
the verification key (or its JSON string) as a parameter to avoid I/O, or (b)
detect runtime (Node vs browser) and if Node use fs.readFile to load
"circuits/merkle_non_inclusion_vkey.json" and JSON.parse it, otherwise use fetch
and res.json(); update callers of fetchVerificationKey accordingly (or add an
overload) so the verification key can be injected when running in non-browser
environments.
- Around line 1-3: The import of generateNonInclusionProof from a non-existent
./merkleProofGenerator should be replaced with the actual implementation in
proof.ts: import the MerkleProofGenerator symbol from "./proof" and update
availability.ts to call the correct method (either
MerkleProofGenerator.generateNonInclusionProof if static, or instantiate new
MerkleProofGenerator(...) and call its generateNonInclusionProof method)
wherever generateNonInclusionProof is referenced so the module resolves at
runtime.
---
Nitpick comments:
In `@zk/sdk/src/__tests__/availability.test.ts`:
- Around line 24-32: The test is relying on a catch-all error path instead of
explicitly exercising proof-failure and generator-error behaviors; update the
tests for isUsernameAvailable to mock dependencies directly: create one test
where you stub groth16.verify to return false (ensure generateNonInclusionProof
returns a valid proof object) and assert isUsernameAvailable resolves to false,
and a separate test where you stub generateNonInclusionProof to throw (and
optionally have groth16.verify not called) and assert isUsernameAvailable
resolves to false; use jest.mock/jest.spyOn to mock groth16.verify and
generateNonInclusionProof, restore mocks after each test, and keep test
descriptions matching the explicit scenarios.
In `@zk/sdk/src/availability.ts`:
- Around line 5-10: The SMTData interface currently declares nodes: any which
loses type safety; change SMTData to be generic (e.g., SMTData<T =
Record<string, unknown>>) or use a concrete type that matches your circuit
inputs (align with NonInclusionInput in types.ts) so nodes has a precise shape;
update all usages of SMTData (e.g., function signatures, variables) to supply
the appropriate generic type or the concrete type so the compiler enforces the
expected fields and prevents accidental misuse.
- Around line 43-46: The current catch block in isUsernameAvailable swallows
errors and returns false, which conflates "taken" with infrastructure failures;
change the function to return a discriminated result type (e.g.,
AvailabilityResult with variants {status:"available", proof: Groth16Proof} |
{status:"taken"} | {status:"error", error: Error}) instead of a boolean, update
isUsernameAvailable to return {status:"available", proof} when the proof
validates, {status:"taken"} when it does not, and in the catch log the original
error via console.error and return {status:"error", error: err as Error}; adjust
any callers of isUsernameAvailable to handle the new AvailabilityResult.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b8e86064-1dbf-46a4-9f65-3da430f3aa11
📒 Files selected for processing (4)
gateway-contract/contracts/core_contract/src/lib.rszk/sdk/src/__tests__/availability.test.tszk/sdk/src/availability.tszk/sdk/src/index.ts
Comment on lines
+13
to
+22
| it("returns true for username not in tree", async () => { | ||
| const result = await isUsernameAvailable( | ||
| "new_user_123", | ||
| mockRoot, | ||
| mockTree as any | ||
| ); | ||
|
|
||
| expect(typeof result).toBe("boolean"); | ||
| // In real test: expect(result).toBe(true) | ||
| }); |
There was a problem hiding this comment.
Test assertion does not match the test description.
The test claims to verify "returns true for username not in tree" but only asserts typeof result === "boolean". Without mocking groth16, hashUsername, and generateNonInclusionProof, this test will always hit the catch block and return false (because circuit artifacts don't exist in the test environment).
Either mock the dependencies to test the happy path, or rename the test to reflect what it actually verifies.
💡 Suggested approach with mocks
import { isUsernameAvailable } from "../availability";
// Mock dependencies
jest.mock("snarkjs", () => ({
groth16: {
fullProve: jest.fn().mockResolvedValue({ proof: {}, publicSignals: [] }),
verify: jest.fn().mockResolvedValue(true),
},
}));
jest.mock("../usernameHasher", () => ({
hashUsername: jest.fn().mockReturnValue("mocked_hash"),
}));
jest.mock("../merkleProofGenerator", () => ({
generateNonInclusionProof: jest.fn().mockResolvedValue({}),
}));
// Mock fetch for verification key
global.fetch = jest.fn().mockResolvedValue({
json: () => Promise.resolve({}),
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@zk/sdk/src/__tests__/availability.test.ts` around lines 13 - 22, The test
"returns true for username not in tree" asserts only a boolean but actually
exercises the catch path because circuit artifacts and helpers are not mocked;
update the test to either (A) mock snarkjs.groth16.fullProve and groth16.verify,
hashUsername, generateNonInclusionProof, and the fetch for the verification key
so isUsernameAvailable("new_user_123", mockRoot, mockTree) executes the happy
path and assert result === true, or (B) change the test name and expectation to
reflect that it only verifies a boolean/handles the error path; target symbols:
isUsernameAvailable, groth16.fullProve, groth16.verify, hashUsername,
generateNonInclusionProof, and the global fetch used to load the verification
key.
Comment on lines
+32
to
+36
| const { proof, publicSignals } = await groth16.fullProve( | ||
| input, | ||
| "circuits/merkle_non_inclusion.wasm", | ||
| "circuits/merkle_non_inclusion.zkey" | ||
| ); |
There was a problem hiding this comment.
Hardcoded circuit paths don't match project conventions.
The paths "circuits/merkle_non_inclusion.wasm" and "circuits/merkle_non_inclusion.zkey" are hardcoded and relative. Per zk/sdk/src/__tests__/proof.test.ts, actual paths follow the pattern:
build/merkle_non_inclusion/wasm/merkle_non_inclusion_js/merkle_non_inclusion.wasmbuild/merkle_non_inclusion/merkle_non_inclusion_final.zkey
Consider accepting paths via configuration or using the existing MerkleProofGenerator class with MerkleProofGeneratorConfig.
💡 Suggested refactor using existing MerkleProofGenerator
import { MerkleProofGenerator } from "./proof";
import type { MerkleProofGeneratorConfig } from "./types";
export async function isUsernameAvailable(
username: string,
smtRoot: bigint,
merkleTree: SMTData,
config: MerkleProofGeneratorConfig
): Promise<boolean> {
try {
const generator = new MerkleProofGenerator(config);
// ... use generator.proveNonInclusion(input)
} catch (err) {
console.error("Username availability check failed:", err);
return false;
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@zk/sdk/src/availability.ts` around lines 32 - 36, The code in
isUsernameAvailable uses hardcoded relative circuit files
("circuits/merkle_non_inclusion.wasm", "circuits/merkle_non_inclusion.zkey");
change it to use the existing MerkleProofGenerator and
MerkleProofGeneratorConfig instead or accept circuit paths via configuration:
add a parameter of type MerkleProofGeneratorConfig to isUsernameAvailable,
instantiate new MerkleProofGenerator(config) and call its proveNonInclusion (or
equivalent) to obtain proof/publicSignals, and remove the hardcoded
groth16.fullProve call; also update imports to include MerkleProofGenerator and
MerkleProofGeneratorConfig from "./proof" or "./types" as appropriate.
Comment on lines
+52
to
+56
| async function fetchVerificationKey() { | ||
| // adjust path depending on your setup | ||
| const res = await fetch("/circuits/merkle_non_inclusion_vkey.json"); | ||
| return res.json(); | ||
| } No newline at end of file |
There was a problem hiding this comment.
fetch API assumes browser environment.
fetch("/circuits/merkle_non_inclusion_vkey.json") uses an absolute URL path, which:
- Won't work in Node.js without polyfills (Node.js
fetchrequires full URLs) - Assumes the verification key is served at a specific web endpoint
For SDK code that may run in Node.js, consider using fs.readFile or accepting the verification key as a parameter.
💡 Suggested fix for cross-environment support
+import fs from "fs/promises";
+import path from "path";
+
/**
* Loads verification key (can be cached in production)
*/
-async function fetchVerificationKey() {
- // adjust path depending on your setup
- const res = await fetch("/circuits/merkle_non_inclusion_vkey.json");
- return res.json();
+async function fetchVerificationKey(vkeyPath: string) {
+ if (typeof window !== "undefined") {
+ // Browser environment
+ const res = await fetch(vkeyPath);
+ return res.json();
+ } else {
+ // Node.js environment
+ const content = await fs.readFile(vkeyPath, "utf-8");
+ return JSON.parse(content);
+ }
}Or accept the key as a parameter to avoid file I/O assumptions entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@zk/sdk/src/availability.ts` around lines 52 - 56, The fetchVerificationKey
function uses browser-only fetch with an absolute path which fails in Node.js
and assumes an HTTP endpoint; modify fetchVerificationKey to be
environment-agnostic by either (a) accepting the verification key (or its JSON
string) as a parameter to avoid I/O, or (b) detect runtime (Node vs browser) and
if Node use fs.readFile to load "circuits/merkle_non_inclusion_vkey.json" and
JSON.parse it, otherwise use fetch and res.json(); update callers of
fetchVerificationKey accordingly (or add an overload) so the verification key
can be injected when running in non-browser environments.
ryzen-xp
approved these changes
Mar 27, 2026
ryzen-xp
added
CI Failed
Contributor
|
Hello @Userhorlie , fix you code why checks failed and resolve the conflicts |
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
2 participants
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.
close #173
Summary by CodeRabbit
New Features
Security
Tests