Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 12 commits into from
Jan 11, 2021

Conversation

piyalbasu
Copy link
Contributor

@piyalbasu piyalbasu commented Dec 22, 2020

Github Issue: #241
Screen Shot 2021-01-04 at 2 28 43 PM
Screen Shot 2021-01-04 at 2 28 18 PM
Screen Shot 2021-01-04 at 2 28 11 PM

"resolutions": {
"**/@typescript-eslint/eslint-plugin": "^4.1.1",
"**/@typescript-eslint/parser": "^4.1.1"
},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

let accounts;

try {
accounts = await getFlaggedAccounts();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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.";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

const [isAccountUnsafe, setIsAccountUnsafe] = useState(false);
const [isAccountMalicious, setIsisAccountMalicious] = useState(false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: there is an extra is in setIsisAccountMalicious.

Comment on lines 260 to 269
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"));
}
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

useEffect(() => {
dispatch(fetchFlaggedAccountsAction());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

request flagged accounts as soon as dashboard loads

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

{formData.isAccountUnsafe && <IsAccountFlagged flagType="unsafe" />}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const [isAccountUnsafe, setIsAccountUnsafe] = useState(
initialFormData.isAccountUnsafe,
);
const [isAccountMalicious, setIsAccountMalicious] = useState(false);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

disable the Continue button if isAccountMalicious

Comment on lines 37 to 47
} 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) || "[]",
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) || "[]",
);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea!

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

setPrevAddress(e.target.value);
setIsAccountIdTouched(false);
checkIfAccountIsFlagged(e.target.value);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably want to reset this onChange event.

Copy link
Contributor

@quietbits quietbits left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏 🎉

@stellar-jenkins
Copy link

@stellar-jenkins
Copy link

@stellar-jenkins
Copy link

@stellar-jenkins
Copy link

@piyalbasu piyalbasu merged commit 4bac41e into master Jan 11, 2021
@piyalbasu piyalbasu deleted the feature/validate-addresses-as-unsafe branch January 11, 2021 20:48
@quietbits quietbits linked an issue Jan 12, 2021 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Improvements for checking valid addresses
4 participants