Validate all Send Addresses against stellar.expert's list of malicious/unsafe addresses #245

piyalbasu · 2020-12-22T22:02:16Z

Github Issue: #241

…s/unsafe addresses

piyalbasu · 2020-12-22T22:24:15Z

package.json

+  "resolutions": {
+    "**/@typescript-eslint/eslint-plugin": "^4.1.1",
+    "**/@typescript-eslint/parser": "^4.1.1"
+  },


I ran into a very weird bug with react-scripts. It was falsely throwing an eslint error in a module exporting an array. What I've done here is kind of a band aid fix: it upgrades the versions of typescript-eslint that react-scripts uses. Another option is to update react-scripts itself BUT that causes a whole host of other eslint issues to crop up which were out of scope of this issue. This was the least invasive method I was able to find.

Further reading here:
https://stackoverflow.com/a/63919395
facebook/create-react-app#9515

I haven't seen this one before. I think your fix is good. 👍

Yeah Haven't seen resolutions before but that seems like a useful tool for the toolbox!

piyalbasu · 2020-12-22T22:27:50Z

src/hooks/useUnsafeAccounts.ts

+      let accounts;
+
+      try {
+        accounts = await getFlaggedAccounts();


this function will throw an error if it doesn't resolve in time

piyalbasu · 2020-12-22T22:33:24Z

src/hooks/useUnsafeAccounts.ts

+      } catch (e) {
+        accounts = JSON.parse(
+          localStorage.getItem(FLAGGED_ACCOUNT_STORAGE_ID) || "[]",
+        );


per the Issue spec, if the request to stellar.expert doesn't resolve in time, let's try to grab a locally stored version of the list

piyalbasu · 2020-12-22T23:12:27Z

src/components/SendTransaction/CreateTransaction.tsx

@@ -288,6 +305,9 @@ export const CreateTransaction = ({
        ) {
          message =
            'Stellar address or public key is invalid. Public keys are uppercase and begin with letter "G."';
+        } else if (isAccountMalicious) {
+          message =
+            "This account has been flagged as being potentially malicious.";


if something is malicious, show an error message and don't allow form submission

src/components/SendTransaction/CreateTransaction.tsx

quietbits · 2020-12-23T14:37:07Z

package.json

+  "resolutions": {
+    "**/@typescript-eslint/eslint-plugin": "^4.1.1",
+    "**/@typescript-eslint/parser": "^4.1.1"
+  },


I haven't seen this one before. I think your fix is good. 👍

quietbits · 2020-12-23T14:46:55Z

src/components/SendTransaction/CreateTransaction.tsx

@@ -147,6 +148,9 @@ export const CreateTransaction = ({
    initialFormData.isAccountFunded,
  );

+  const [isAccountUnsafe, setIsAccountUnsafe] = useState(false);
+  const [isAccountMalicious, setIsisAccountMalicious] = useState(false);


Typo: there is an extra is in setIsisAccountMalicious.

quietbits · 2020-12-23T14:50:12Z

src/components/SendTransaction/CreateTransaction.tsx

+  const checkIfAccountIsFlagged = (accountId: string) => {
+    const flaggedAccountData = flaggedAccounts.find(
+      ({ address }: { address: string }) => address === accountId,
+    );
+    if (flaggedAccountData?.tags) {
+      const { tags } = flaggedAccountData;
+      setIsAccountUnsafe(tags.includes("unsafe"));
+      setIsisAccountMalicious(tags.includes("malicious"));
+    }
+  };


We're not resetting setIsAccountUnsafe and setIsAccountMalicious to false after it's been set to true, in case the user enters a malicious address followed by a valid one. We could do that at the top of this function.

piyalbasu · 2021-01-04T20:21:45Z

src/ducks/flaggedAccounts.ts

@@ -0,0 +1,72 @@
+import { createAsyncThunk, createSlice } from "@reduxjs/toolkit";


moving flaggedAccounts into redux to leverage PENDING and SUCCESS action states to prevent UI from loading before we have all the data we need

piyalbasu · 2021-01-04T20:22:45Z

src/pages/Dashboard.tsx

  useEffect(() => {
+    dispatch(fetchFlaggedAccountsAction());


request flagged accounts as soon as dashboard loads

piyalbasu · 2021-01-04T20:24:09Z

src/components/SendTransaction/ConfirmTransaction.tsx

@@ -242,6 +244,8 @@ export const ConfirmTransaction = ({
        </InfoBlock>
      )}

+      {formData.isAccountUnsafe && <IsAccountFlagged flagType="unsafe" />}


show unsafe account message on Confirm Transaction.

We don't bother showing the malicious warning as the user should not be able to get to this screen with a malicious destination address

piyalbasu · 2021-01-04T20:25:15Z

src/components/SendTransaction/CreateTransaction.tsx

+  const [isAccountUnsafe, setIsAccountUnsafe] = useState(
+    initialFormData.isAccountUnsafe,
+  );
+  const [isAccountMalicious, setIsAccountMalicious] = useState(false);


use initialFormData to populate isAccountSafe to persist the data

We don't bother with isAccountMalicious as a user should be stuck on this screen if the address is malicious

piyalbasu · 2021-01-04T20:26:02Z

src/components/SendTransaction/CreateTransaction.tsx

@@ -420,7 +441,9 @@ export const CreateTransaction = ({
      headlineText="Send Lumens"
      buttonFooter={
        <>
-          <Button onClick={onSubmit}>Continue</Button>
+          <Button disabled={isAccountMalicious} onClick={onSubmit}>


disable the Continue button if isAccountMalicious

quietbits · 2021-01-04T21:00:11Z

src/ducks/flaggedAccounts.ts

+      } catch (e) {
+        // in case of error, try to use what's in localStorage, even if it's old
+        accounts = JSON.parse(
+          localStorage.getItem(FLAGGED_ACCOUNT_STORAGE_ID) || "[]",
+        );
+      }
+    } else {
+      // otherwise, simply use what we have in localStorage to prevent an unnecessary request
+      accounts = JSON.parse(
+        localStorage.getItem(FLAGGED_ACCOUNT_STORAGE_ID) || "[]",
+      );


Do you think it would work if we returned the below line as the default value and then change it only if new fetch was successful? Those two lines are exactly the same.

accounts = JSON.parse( localStorage.getItem(FLAGGED_ACCOUNT_STORAGE_ID) || "[]", );

quietbits · 2021-01-04T21:11:32Z

src/components/SendTransaction/CreateTransaction.tsx

@@ -478,6 +501,7 @@ export const CreateTransaction = ({

            setPrevAddress(e.target.value);
            setIsAccountIdTouched(false);
+            checkIfAccountIsFlagged(e.target.value);


We probably want to reset this onChange event.

quietbits

👏 🎉

stellar-jenkins · 2021-01-06T10:25:19Z

Preview is available here:
http://account-viewer-v2-pr245.previews.kube001.services.stellar-ops.com/

stellar-jenkins · 2021-01-06T19:48:21Z

Preview is available here:
http://account-viewer-v2-pr245.previews.kube001.services.stellar-ops.com/

stellar-jenkins · 2021-01-08T19:52:57Z

Preview is available here:
http://account-viewer-v2-pr245.previews.kube001.services.stellar-ops.com/

stellar-jenkins · 2021-01-11T20:06:10Z

Preview is available here:
http://account-viewer-v2-pr245.previews.kube001.services.stellar-ops.com/

Validate all Send Addresses against stellar.expert's list of maliciou…

2c19387

…s/unsafe addresses

piyalbasu commented Dec 22, 2020

View reviewed changes

piyalbasu added 2 commits December 22, 2020 17:25

remove debugger

bdf23f9

no need to remove space

e5612ba

piyalbasu commented Dec 22, 2020

View reviewed changes

forgot to remove ts nocheck

0aa8238

piyalbasu commented Dec 22, 2020

View reviewed changes

src/components/SendTransaction/CreateTransaction.tsx Outdated Show resolved Hide resolved

quietbits reviewed Dec 23, 2020

View reviewed changes

move flaggedAccounts to redux and address UI issues

99998dd

piyalbasu commented Jan 4, 2021

View reviewed changes

Rename warning message component

042e2c0

quietbits reviewed Jan 4, 2021

View reviewed changes

piyalbasu added 3 commits January 4, 2021 16:37

only show "malicious" warning in case it's both "unsafe" && "malicious"

5df92e2

default to the localStorage and replace with API if applicable

9c89d51

matching error message spacing to createTransaction

8d5e95f

quietbits approved these changes Jan 4, 2021

View reviewed changes

update UI of warning messages

92aa25d

update stellar.expert url

b935f63

trigger a new PR preview

e948089

piyalbasu merged commit 4bac41e into master Jan 11, 2021

piyalbasu deleted the feature/validate-addresses-as-unsafe branch January 11, 2021 20:48

quietbits linked an issue Jan 12, 2021 that may be closed by this pull request

Feature Request: Improvements for checking valid addresses #241

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate all Send Addresses against stellar.expert's list of malicious/unsafe addresses #245

Validate all Send Addresses against stellar.expert's list of malicious/unsafe addresses #245

piyalbasu commented Dec 22, 2020 •

edited

piyalbasu Dec 22, 2020

quietbits Dec 23, 2020

vcarl Dec 23, 2020

piyalbasu Dec 22, 2020

piyalbasu Dec 22, 2020

piyalbasu Dec 22, 2020

quietbits Dec 23, 2020

quietbits Dec 23, 2020

quietbits Dec 23, 2020

piyalbasu Jan 4, 2021

piyalbasu Jan 4, 2021

piyalbasu Jan 4, 2021

piyalbasu Jan 4, 2021

piyalbasu Jan 4, 2021

quietbits Jan 4, 2021

piyalbasu Jan 4, 2021

quietbits Jan 4, 2021

quietbits left a comment

stellar-jenkins commented Jan 6, 2021

stellar-jenkins commented Jan 6, 2021

stellar-jenkins commented Jan 8, 2021

stellar-jenkins commented Jan 11, 2021

		@@ -0,0 +1,72 @@
		import { createAsyncThunk, createSlice } from "@reduxjs/toolkit";

Validate all Send Addresses against stellar.expert's list of malicious/unsafe addresses #245

Validate all Send Addresses against stellar.expert's list of malicious/unsafe addresses #245

Conversation

piyalbasu commented Dec 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quietbits left a comment

Choose a reason for hiding this comment

stellar-jenkins commented Jan 6, 2021

stellar-jenkins commented Jan 6, 2021

stellar-jenkins commented Jan 8, 2021

stellar-jenkins commented Jan 11, 2021

piyalbasu commented Dec 22, 2020 •

edited