fix: add AllowInsecureUnlock guard to eth_sign and eth_signTypedData

[CharlieMc0]

Summary

• Add AllowInsecureUnlock check to Sign() and SignTypedData() in rpc/backend/sign_tx.go, consistent with the existing guard in SendTransaction()
• When json-rpc.allow-insecure-unlock=false, these methods now correctly reject signing requests instead of acting as an open signing oracle
• Add test cases verifying both methods reject requests when insecure unlock is disabled

Test plan

• go test -tags=test ./rpc/backend/... -run TestBackendTestSuite/TestSign — all pass including new guard test
• go test -tags=test ./rpc/backend/... -run TestBackendTestSuite/TestSignTypedData — all pass including new guard test
• CI passes

🤖 Generated with Claude Code

---

Note

Medium Risk
Changes signing RPC behavior to hard-fail when json-rpc.allow-insecure-unlock is disabled, which is security-sensitive and could break clients that relied on the previous permissive behavior.

Overview
Prevents the JSON-RPC server from acting as an open signing oracle. Sign (eth_sign) and SignTypedData (eth_signTypedData) now enforce the Cfg.JSONRPC.AllowInsecureUnlock gate, returning an error when insecure unlock is disabled (matching SendTransaction).

Tests are extended to cover the new rejection path for both signing methods when AllowInsecureUnlock=false.

^{Written by Cursor Bugbot for commit d8ea641. Configure here.}

Greptile Summary

This PR closes a security gap where eth_sign (Sign()) and eth_signTypedData (SignTypedData()) were acting as open signing oracles even when json-rpc.allow-insecure-unlock=false, while SendTransaction() already had the guard in place. The fix adds the missing AllowInsecureUnlock check to both methods, making all three functions consistent in their access control.

• Sign() and SignTypedData() in rpc/backend/sign_tx.go now return early with an error when AllowInsecureUnlock=false, before any keyring access.
• New test cases in sign_tx_test.go verify the reject-when-disabled behavior for both methods; test isolation is correct since SetupTest() resets the flag to true before each case.
• The default value of AllowInsecureUnlock remains true (defined in server/config/config.go), meaning the change is non-breaking for existing deployments. Operators who had already set the flag to false will now correctly have eth_sign and eth_signTypedData blocked.
• Minor: fmt.Errorf is used without format verbs in the new error returns — errors.New would be more idiomatic, and is consistent with standard Go best practices.

Confidence Score: 5/5

• This PR is safe to merge — it is a minimal, targeted security hardening with no functional side effects for default configurations.
• The change is a straightforward two-location guard addition that mirrors an already-proven pattern in SendTransaction(). Tests are well-structured and correctly isolated. No existing behavior is altered for the default (AllowInsecureUnlock=true) configuration. The only finding is a minor fmt.Errorf→errors.New style suggestion.
• No files require special attention.

Important Files Changed

Filename	Overview
rpc/backend/sign_tx.go	Adds AllowInsecureUnlock guard to Sign() and SignTypedData(), making them consistent with the existing guard in SendTransaction(). The guard is correctly placed before any keyring access, and the error message matches the existing pattern. Only a minor style note: fmt.Errorf without format verbs could be errors.New.
rpc/backend/sign_tx_test.go	Adds test cases for both Sign and SignTypedData verifying rejection when AllowInsecureUnlock=false. Tests are correctly placed as the first case in each suite so they run with a fresh SetupTest() reset (which sets AllowInsecureUnlock=true) before being overridden by registerMock().

Sequence Diagram

sequenceDiagram
    participant Client as JSON-RPC Client
    participant Backend as Backend

    Note over Backend: AllowInsecureUnlock guard (NEW in Sign & SignTypedData)

    Client->>Backend: eth_sign / eth_signTypedData
    alt AllowInsecureUnlock = false
        Backend-->>Client: error: "account unlock with HTTP access is forbidden"
    else AllowInsecureUnlock = true
        Backend->>Backend: Keyring.KeyByAddress(from)
        alt key not found
            Backend-->>Client: error: ErrNoMatch
        else key found
            Backend->>Backend: Keyring.SignByAddress(from, data)
            Backend-->>Client: signature (hexutil.Bytes)
        end
    end

    Client->>Backend: eth_sendTransaction
    alt AllowInsecureUnlock = false
        Backend-->>Client: error: "account unlock with HTTP access is forbidden"
    else AllowInsecureUnlock = true
        Backend->>Backend: sign + broadcast tx
        Backend-->>Client: txHash
    end

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: rpc/backend/sign_tx.go
Line: 127

Comment:
**Use `errors.New` instead of `fmt.Errorf`**

`fmt.Errorf` is used here without any format verbs, so `errors.New` is more idiomatic and avoids the minor overhead of format string parsing. The same applies to the equivalent line in `SignTypedData` (line 153) and the pre-existing pattern in `SendTransaction` (line 26).

```suggestion
		return nil, errors.New("account unlock with HTTP access is forbidden")
```

You would also need to add `"errors"` to the import block (it's already imported in the file), and can remove `fmt` if it's no longer used elsewhere. Since `fmt` is still used in other error returns, only the call sites need updating.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: rpc/backend/sign_tx.go
Line: 153

Comment:
**Use `errors.New` instead of `fmt.Errorf`**

Same as above — no format verbs are used, so `errors.New` would be more idiomatic.

```suggestion
		return nil, errors.New("account unlock with HTTP access is forbidden")
```

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "fix: add AllowInsecureUnlock guard to et..." | Re-trigger Greptile}

Greptile also left 2 inline comments on this PR.

Summary by CodeRabbit

• Bug Fixes

  • Enhanced security by preventing account unlocking over HTTP when insecure unlock is disabled. The system now returns an error for unlock requests that violate the security configuration.

• Tests

  • Added test cases to verify signing operations properly fail when insecure unlock is disabled.

[coderabbitai]

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: debc1267-b251-4730-b84a-c9586450c9f7

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

This change adds a security configuration guard to the Sign and SignTypedData functions in the RPC backend. When insecure unlock is disabled via configuration, both functions log a debug message and return an error before proceeding with key lookup and signing operations.

Changes

Cohort / File(s)	Summary
Security Guard Implementation rpc/backend/sign_tx.go	Added configuration check to Backend.Sign and Backend.SignTypedData that returns an error ("account unlock with HTTP access is forbidden") when AllowInsecureUnlock is disabled, preventing further execution.
Test Coverage rpc/backend/sign_tx_test.go	Added negative test cases to TestSign and TestSignTypedData that verify signing functions properly fail when AllowInsecureUnlock is false.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and concisely describes the primary change: adding an AllowInsecureUnlock guard to eth_sign and eth_signTypedData RPC methods.
Description check	✅ Passed	The description fully covers the PR objectives, testing approach, and rationale. All key sections including summary, test plan with execution evidence, and risk assessment are present and complete.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch worktree-fix-eth-sign-insecure-unlock

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

rpc/backend/sign_tx_test.go (1)

156-164: Consider asserting the specific rejection error, not just err != nil.

Right now, any unrelated error would pass these cases. Adding an expected error string/type per case will make the tests stricter and less brittle.

Proposed test tightening

 testCases := []struct {
     name         string
     registerMock func()
     fromAddr     common.Address
     inputBz      hexutil.Bytes
+    expErrSubstr string
     expPass      bool
 }{
     {
         "fail - insecure unlock not allowed",
         func() {
             s.backend.Cfg.JSONRPC.AllowInsecureUnlock = false
         },
         from,
         nil,
+        "account unlock with HTTP access is forbidden",
         false,
     },
@@
 if tc.expPass {
     ...
 } else {
     s.Require().Error(err)
+    if tc.expErrSubstr != "" {
+        s.Require().Contains(err.Error(), tc.expErrSubstr)
+    }
 }

Also applies to: 216-224

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@rpc/backend/sign_tx_test.go` around lines 156 - 164, The test case that flips
s.backend.Cfg.JSONRPC.AllowInsecureUnlock = false (in
rpc/backend/sign_tx_test.go) currently only asserts err != nil; update the test
table entries (including the similar cases around lines 216-224) to include an
expected error value or substring and replace the generic non-nil check with an
assertion that the returned error matches that expected error (by comparing
error type or using strings.Contains(err.Error(), expectedErrSubstring) or
errors.Is for concrete sentinel errors), referencing the failing case entry that
sets AllowInsecureUnlock and the test helper that invokes the sign/authorization
path to validate the specific rejection error.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@rpc/backend/sign_tx_test.go`:
- Around line 156-164: The test case that flips
s.backend.Cfg.JSONRPC.AllowInsecureUnlock = false (in
rpc/backend/sign_tx_test.go) currently only asserts err != nil; update the test
table entries (including the similar cases around lines 216-224) to include an
expected error value or substring and replace the generic non-nil check with an
assertion that the returned error matches that expected error (by comparing
error type or using strings.Contains(err.Error(), expectedErrSubstring) or
errors.Is for concrete sentinel errors), referencing the failing case entry that
sets AllowInsecureUnlock and the test helper that invokes the sign/authorization
path to validate the specific rejection error.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 132dc9e4-ab22-47a7-9244-0a80f3bd600c

📥 Commits

Reviewing files that changed from the base of the PR and between 9c796d6 and d8ea641.

📒 Files selected for processing (2)

• rpc/backend/sign_tx.go
• rpc/backend/sign_tx_test.go

[ws4charlie]

lgtm. The code style commented by Greptile have low priority but might worth fixing.

[CharlieMc0]

@cursor update the changelog to incldue this change

[cursor]

I've updated the changelog to include this PR. The entry has been added to the "Fixes" section under "Release ReForge" at line 26:

* [4562](https://github.com/zeta-chain/node/pull/4562) - add AllowInsecureUnlock guard to eth_sign and eth_signTypedData

The change has been committed and pushed to the worktree-fix-eth-sign-insecure-unlock branch.

  

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!