bugfix: add address validation and improve safelyExecute #78
Conversation
src/AddressBookController.ts
Outdated
| */ | ||
| set(address: string, name: string) { | ||
| if (!isValidAddress(address)) { | ||
| return; |
There was a problem hiding this comment.
return false? (comment says it returns a boolean)
src/PreferencesController.ts
Outdated
| @@ -31,6 +31,10 @@ export class PreferencesController extends BaseController<BaseConfig, Preference | |||
| */ | |||
| name = 'PreferencesController'; | |||
|
|
|||
| private isLocalURL(url: string) { | |||
| return /:\/\/localhost:/.test(url); | |||
There was a problem hiding this comment.
This fixe the port issue but the we should also be checking for any other non public IP like 127.0.0.1.
Mitigation: Simply match against 127.0.0.1, localhost, or any non-public IP address.
There was a problem hiding this comment.
This suggestion from the audit was a little odd; AFAIK, there's no way to know how local hosts are configured on someone's machine. For example, I could locally resolve my local development node to http://bitpshr.dev:1337 if I set up my host file accordingly. I guess checking readily-known local URLs is better than nothing.
There was a problem hiding this comment.
I was talking about IPs not hosts. See https://stackoverflow.com/a/11327345
There was a problem hiding this comment.
I'm all for adding a cover-all regex for local IPs (candidly I did not know this was a matchable pattern, thanks for this) but as discussed offline, I worry a little about the usefulness of filtering out locally-hosted nodes. Whether it's a developer constantly hitting a local node during development, or a power user constantly hitting a local node for security, it feels like we should save these as "frequent", almost more-so than a public custom RPC network that may've been added by a user to test a one-off dapp.
The audit expresses concern that our checking of local URLs is insufficient:
There is a condition in the PreferencesController that checks if the RPC URL is http://localhost:8545, presumably to check if the node is a local testing node. However, a user may conceivably run their node on a different port, which would fail this check.
I think it's safe to ignore the suggestion from the audit since we should probably remove this local URL condition altogether. Even if we don't remove the local URL check, this is purely a usability concern (whether to save one URL vs another URL for quick access) and has no security implications.
| try { | ||
| return await operation(); | ||
| } catch (error) { | ||
| /* tslint:disable-next-line:no-empty */ | ||
| logError && console.error(error); |
Codecov Report
@@ Coverage Diff @@
## master #78 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 21 21
Lines 1250 1252 +2
Branches 153 154 +1
=====================================
+ Hits 1250 1252 +2
Continue to review full report at Codecov.
|
Type tests have been added for some types and type guards in `misc.ts`.
This pull request does the following:
safelyExecuteoptionally logs errors and provides an optional retry logic path.AddressBookControllervalidates newly-added addresses.PreferencesControllerloosely checks local RPC URLs (though this was technically unnecessary since we're checking against a constant string value we set internally.)Fixes #65
Fixes #67
Fixes #70