fix: add input validation for account creation - #3490
Conversation
- Validate connector type against known connectors - Check that config is a non-empty string - Check that name is a non-empty string - Return clear error messages for each validation failure - Add unit tests for all validation cases - Resolves TODO comments for missing validations
📝 WalkthroughWalkthroughThis PR adds comprehensive input validation to the account-adding workflow, ensuring connector type validity, config presence, and account name non-emptiness before processing. Corresponding test cases validate error handling for these validation steps alongside existing functionality. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/extension/background-script/actions/accounts/add.ts`:
- Around line 17-23: The validation for newAccount.config in add.ts incorrectly
requires a string; update it to accept either a string or an object matching
Record<string, unknown> (per AlbyProvider) so objects like { url, macaroon }
passed from promptAdd/ConfirmAddAccount are allowed; specifically, modify the
check around newAccount.config to allow typeof === "object" (and non-null) or a
non-empty string, and ensure subsequent code that uses newAccount.config handles
both serialized strings and object shapes (or normalize by JSON.stringify only
if you choose to serialize upstream).
🧹 Nitpick comments (1)
src/extension/background-script/actions/accounts/__tests__/add.test.ts (1)
152-201: New validation tests cover the happy error paths well.The three tests correctly verify the error responses for invalid connector, empty config, and empty name.
One optional improvement: consider adding tests for whitespace-only inputs (e.g.,
config: " "andname: " ") since the validation usestrim()— these would exercise that specific branch.💡 Example additional test cases
+ test("returns error for whitespace-only config", async () => { + const mockState = defaultMockState; + state.getState = jest.fn().mockReturnValue(mockState); + + const invalidMessage: MessageAccountAdd = { + ...message, + args: { + ...message.args, + config: " ", + }, + }; + + expect(await addAccount(invalidMessage)).toStrictEqual({ + error: "Account config is required", + }); + }); + + test("returns error for whitespace-only name", async () => { + const mockState = defaultMockState; + state.getState = jest.fn().mockReturnValue(mockState); + + const invalidMessage: MessageAccountAdd = { + ...message, + args: { + ...message.args, + name: " ", + }, + }; + + expect(await addAccount(invalidMessage)).toStrictEqual({ + error: "Account name is required", + }); + });
| if ( | ||
| !newAccount.config || | ||
| typeof newAccount.config !== "string" || | ||
| newAccount.config.trim() === "" | ||
| ) { | ||
| return { error: "Account config is required" }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1. Find the Account type definition
echo "=== Checking Account type definition ==="
find . -name "*.ts" -type f | xargs rg -l "type Account|interface Account" | head -5Repository: getAlby/lightning-browser-extension
Length of output: 272
🏁 Script executed:
#!/bin/bash
# 2. Check the alby provider method signature
echo "=== Checking alby provider signature ==="
cat -n src/extension/providers/alby/index.ts | head -50Repository: getAlby/lightning-browser-extension
Length of output: 1441
🏁 Script executed:
#!/bin/bash
# 3. Find all files that reference addAccount
echo "=== Files referencing addAccount ==="
rg -l "addAccount" --type=ts src/ | grep -v node_modulesRepository: getAlby/lightning-browser-extension
Length of output: 1241
🏁 Script executed:
#!/bin/bash
# 4. Check how the add action is called from connectors
echo "=== Tracing the call flow to add action ==="
rg -B5 -A5 "\.add\(" src/ --type=ts | head -100Repository: getAlby/lightning-browser-extension
Length of output: 7954
🏁 Script executed:
#!/bin/bash
# Check the Account type definition in detail
cat -n src/types.ts | grep -A 10 "type Account\|interface Account"Repository: getAlby/lightning-browser-extension
Length of output: 932
🏁 Script executed:
#!/bin/bash
# Check the add action file more thoroughly
cat -n src/extension/background-script/actions/accounts/add.tsRepository: getAlby/lightning-browser-extension
Length of output: 2323
🏁 Script executed:
#!/bin/bash
# Check how addAccount is called in the router
cat -n src/extension/background-script/router.ts | grep -B 10 -A 10 "addAccount"Repository: getAlby/lightning-browser-extension
Length of output: 1760
🏁 Script executed:
#!/bin/bash
# Check the MessageAccountAdd type definition
rg -B 5 -A 10 "MessageAccountAdd" src/types.tsRepository: getAlby/lightning-browser-extension
Length of output: 556
🏁 Script executed:
#!/bin/bash
# Check the promptAdd action to understand the flow
cat -n src/extension/background-script/actions/accounts/promptAdd.tsRepository: getAlby/lightning-browser-extension
Length of output: 1121
🏁 Script executed:
#!/bin/bash
# Check how connectors actually send data to addAccount
rg -B 10 -A 5 "promptAdd\|addAccount" src/app/screens/connectors/ConnectAlby/index.tsxRepository: getAlby/lightning-browser-extension
Length of output: 61
🏁 Script executed:
#!/bin/bash
# Check the ConfirmAddAccount screen to see how it transforms config
cat -n src/app/screens/ConfirmAddAccount/index.tsxRepository: getAlby/lightning-browser-extension
Length of output: 3899
🏁 Script executed:
#!/bin/bash
# Check what happens after confirmation
rg -B 5 -A 10 "confirmAddAccount\|addAccount" src/app/screens/ConfirmAddAccount/index.tsxRepository: getAlby/lightning-browser-extension
Length of output: 61
🏁 Script executed:
#!/bin/bash
# Check the message flow in the background router more carefully
cat -n src/extension/background-script/router.ts | head -110Repository: getAlby/lightning-browser-extension
Length of output: 4651
🏁 Script executed:
#!/bin/bash
# Check how msg.request works - might do serialization
rg -B 5 -A 15 "request.*function\|export.*request\|const request" src/common/lib/msg.ts | head -60Repository: getAlby/lightning-browser-extension
Length of output: 61
🏁 Script executed:
#!/bin/bash
# Look at the actual test to see if config is string or object
cat -n src/extension/background-script/actions/accounts/__tests__/add.test.ts | head -100Repository: getAlby/lightning-browser-extension
Length of output: 3051
🏁 Script executed:
#!/bin/bash
# Check if there's any JSON serialization happening in the flow
rg "JSON.stringify" src/app/screens/ConfirmAddAccount/ src/common/lib/msg.ts src/extension/background-script/Repository: getAlby/lightning-browser-extension
Length of output: 2513
🏁 Script executed:
#!/bin/bash
# Check how msg.request implementation works to see if it serializes
find . -name "msg.ts" -o -name "msg.js" | grep -E "common/lib" | head -3Repository: getAlby/lightning-browser-extension
Length of output: 102
🏁 Script executed:
#!/bin/bash
# Read the msg.ts implementation
cat -n src/common/lib/msg.tsRepository: getAlby/lightning-browser-extension
Length of output: 1384
🏁 Script executed:
#!/bin/bash
# Check if there are any other usages showing config type expectations
rg -B 3 -A 3 "config.*object\|config.*Record" src/app/screens/connectors/ --type=ts | head -50Repository: getAlby/lightning-browser-extension
Length of output: 61
🏁 Script executed:
#!/bin/bash
# Check one specific connector to see how config is handled
cat -n src/app/screens/connectors/ConnectLnd/index.tsx | head -150Repository: getAlby/lightning-browser-extension
Length of output: 5524
🏁 Script executed:
#!/bin/bash
# Search for where config might be stringified in connector screens
rg "JSON.stringify.*config\|config.*JSON.stringify" src/app/screens/connectors/ --type=tsxRepository: getAlby/lightning-browser-extension
Length of output: 106
🏁 Script executed:
#!/bin/bash
# Check how connectors are setting config before calling promptAdd/addAccount
rg -B 10 "msg.request.*addAccount\|promptAdd" src/app/screens/connectors/ --type=tsx | head -80Repository: getAlby/lightning-browser-extension
Length of output: 106
🏁 Script executed:
#!/bin/bash
# Look for more connector examples to confirm the pattern
rg -B 5 -A 5 "config.*\{" src/app/screens/connectors/ConnectNWC/index.tsx | head -40Repository: getAlby/lightning-browser-extension
Length of output: 318
🏁 Script executed:
#!/bin/bash
# Check if there's any middleware that transforms the message before it reaches add.ts
cat -n src/extension/background-script/index.tsRepository: getAlby/lightning-browser-extension
Length of output: 8580
🏁 Script executed:
#!/bin/bash
# Search for message listeners that might transform config to string
rg "runtime.onMessage\|sendMessage" src/extension/background-script/ --type=ts -B 3 -A 10 | head -100Repository: getAlby/lightning-browser-extension
Length of output: 61
🏁 Script executed:
#!/bin/bash
# Double-check by looking at one more connector to confirm the pattern
rg -B 3 -A 8 "msg.request.*addAccount" src/app/screens/connectors/ConnectLnbits/ -A 10 | head -30Repository: getAlby/lightning-browser-extension
Length of output: 1225
Fix type mismatch: config arrives as an object but validation expects a string.
In the actual connector UI flow, config is created as an object (e.g., { url, macaroon } in ConnectLnd), passed through promptAdd (which validates typeof config !== "object" at line 17), and forwarded unchanged by ConfirmAddAccount. However, the validation at lines 17-23 in add.ts requires config to be a string and will reject the object. Since browser.runtime.sendMessage() preserves object types with no serialization in between, the validation will fail when called from the UI.
Either serialize config to a JSON string before this point, or update the validation to accept objects matching the Record<string, unknown> signature defined in AlbyProvider.
🤖 Prompt for AI Agents
In `@src/extension/background-script/actions/accounts/add.ts` around lines 17 -
23, The validation for newAccount.config in add.ts incorrectly requires a
string; update it to accept either a string or an object matching Record<string,
unknown> (per AlbyProvider) so objects like { url, macaroon } passed from
promptAdd/ConfirmAddAccount are allowed; specifically, modify the check around
newAccount.config to allow typeof === "object" (and non-null) or a non-empty
string, and ensure subsequent code that uses newAccount.config handles both
serialized strings and object shapes (or normalize by JSON.stringify only if you
choose to serialize upstream).
|
@abhishek-8081 where this validations are really needed? we have fron't end forms. where user can't submit unless we get required information. there is validateAccount call that's being runned to check if anything is wrong. Connector type - TypeScript enforces - all callers use valid hardcoded strings i guess we can close this unless we need more defensive code there cc @reneaaron |
2 participants
Fixes the missing input validation when adding a new account.
What changed
The
addfunction insrc/extension/background-script/actions/accounts/add.tshad no validationon the incoming account data before encrypting and saving it. There were TODO comments saying
// TODO: add validationsand// TODO: make sure a password is set.This PR adds three checks before saving:
Each check returns a clear error message if it fails.
Tests
Added 3 new test cases in
add.test.ts:All 5 tests pass (2 existing + 3 new).
Fixes : #3489
Summary by CodeRabbit
Bug Fixes
Tests