-
Notifications
You must be signed in to change notification settings - Fork 25
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
Changes from 5 commits
2c19387
bdf23f9
e5612ba
0aa8238
99998dd
042e2c0
5df92e2
9c89d51
8d5e95f
92aa25d
b935f63
e948089
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,8 @@ import { sendTxAction } from "ducks/sendTx"; | |
import { useRedux } from "hooks/useRedux"; | ||
import { ActionStatus, AuthType, PaymentFormData } from "types/types.d"; | ||
|
||
import { IsAccountFlagged } from "./WarningMessages/IsAccountFlagged"; | ||
|
||
const TableEl = styled.table` | ||
width: 100%; | ||
|
||
|
@@ -242,6 +244,8 @@ export const ConfirmTransaction = ({ | |
</InfoBlock> | ||
)} | ||
|
||
{formData.isAccountUnsafe && <IsAccountFlagged flagType="unsafe" />} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
{status === ActionStatus.PENDING && | ||
settings.authType && | ||
settings.authType !== AuthType.PRIVATE_KEY && ( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,6 +29,8 @@ import { | |
import { PALETTE } from "constants/styles"; | ||
import { knownAccounts } from "constants/knownAccounts"; | ||
|
||
import { IsAccountFlagged } from "./WarningMessages/IsAccountFlagged"; | ||
|
||
const RowEl = styled.div` | ||
display: flex; | ||
flex-wrap: wrap; | ||
|
@@ -147,6 +149,11 @@ export const CreateTransaction = ({ | |
initialFormData.isAccountFunded, | ||
); | ||
|
||
const [isAccountUnsafe, setIsAccountUnsafe] = useState( | ||
initialFormData.isAccountUnsafe, | ||
); | ||
const [isAccountMalicious, setIsAccountMalicious] = useState(false); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
const knownAccount = | ||
knownAccounts[toAccountId] || knownAccounts[federationAddress || ""]; | ||
const [prevAddress, setPrevAddress] = useState( | ||
|
@@ -251,6 +258,19 @@ export const CreateTransaction = ({ | |
} | ||
}; | ||
|
||
const { flaggedAccounts } = useRedux("flaggedAccounts"); | ||
|
||
const checkIfAccountIsFlagged = (accountId: string) => { | ||
const flaggedTags = flaggedAccounts.data.reduce( | ||
(prev: string[], { address, tags }) => { | ||
return address === accountId ? [...prev, ...tags] : prev; | ||
}, | ||
[], | ||
); | ||
setIsAccountUnsafe(flaggedTags.includes("unsafe")); | ||
setIsAccountMalicious(flaggedTags.includes("malicious")); | ||
}; | ||
|
||
const checkAndSetIsAccountFunded = async (accountId: string) => { | ||
if (!accountId || !StrKey.isValidEd25519PublicKey(accountId)) { | ||
setIsAccountFunded(true); | ||
|
@@ -411,6 +431,7 @@ export const CreateTransaction = ({ | |
memoType, | ||
memoContent, | ||
isAccountFunded, | ||
isAccountUnsafe, | ||
}); | ||
} | ||
}; | ||
|
@@ -420,7 +441,9 @@ export const CreateTransaction = ({ | |
headlineText="Send Lumens" | ||
buttonFooter={ | ||
<> | ||
<Button onClick={onSubmit}>Continue</Button> | ||
<Button disabled={isAccountMalicious} onClick={onSubmit}> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. disable the Continue button if |
||
Continue | ||
</Button> | ||
<Button onClick={onCancel} variant={ButtonVariant.secondary}> | ||
Cancel | ||
</Button> | ||
|
@@ -478,6 +501,7 @@ export const CreateTransaction = ({ | |
|
||
setPrevAddress(e.target.value); | ||
setIsAccountIdTouched(false); | ||
checkIfAccountIsFlagged(e.target.value); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We probably want to reset this onChange event. |
||
}} | ||
error={inputErrors[SendFormIds.SEND_TO]} | ||
value={toAccountId} | ||
|
@@ -515,6 +539,17 @@ export const CreateTransaction = ({ | |
</RowEl> | ||
)} | ||
|
||
{isAccountUnsafe && ( | ||
piyalbasu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
<RowEl> | ||
<IsAccountFlagged flagType="unsafe" /> | ||
</RowEl> | ||
)} | ||
{isAccountMalicious && ( | ||
<RowEl> | ||
<IsAccountFlagged flagType="malicious" /> | ||
</RowEl> | ||
)} | ||
|
||
<RowEl> | ||
<CellEl> | ||
<Input | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
import React from "react"; | ||
|
||
import { InfoBlock, InfoBlockVariant } from "components/basic/InfoBlock"; | ||
|
||
export const IsAccountFlagged = ({ flagType = "" }) => ( | ||
<InfoBlock variant={InfoBlockVariant.error}> | ||
<p>This account has been flagged as being potentially {flagType}.</p> | ||
</InfoBlock> | ||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,72 @@ | ||
import { createAsyncThunk, createSlice } from "@reduxjs/toolkit"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
import { | ||
FLAGGED_ACCOUNT_STORAGE_ID, | ||
FLAGGED_ACCOUNT_DATE_STORAGE_ID, | ||
} from "constants/settings"; | ||
import { getFlaggedAccounts } from "helpers/getFlaggedAccounts"; | ||
import { ActionStatus, FlaggedAccounts } from "types/types.d"; | ||
|
||
const initialState: FlaggedAccounts = { | ||
data: [{ address: "", tags: [""] }], | ||
status: undefined, | ||
}; | ||
|
||
export const fetchFlaggedAccountsAction = createAsyncThunk( | ||
"action/fetchFlaggedAccountsAction", | ||
async () => { | ||
let accounts; | ||
const date = new Date(); | ||
const time = date.getTime(); | ||
const sevenDaysAgo = time - 7 * 24 * 60 * 60 * 1000; | ||
const flaggedAccountsCacheDate = Number( | ||
localStorage.getItem(FLAGGED_ACCOUNT_DATE_STORAGE_ID), | ||
); | ||
|
||
// if flaggedAccounts were last cached over seven days ago, make the request | ||
// flaggedAccountsCacheDate is coerced to 0 if not found in storage | ||
if (flaggedAccountsCacheDate < sevenDaysAgo) { | ||
try { | ||
accounts = await getFlaggedAccounts(); | ||
// store the accounts plus the date we've acquired them | ||
localStorage.setItem( | ||
FLAGGED_ACCOUNT_STORAGE_ID, | ||
JSON.stringify(accounts), | ||
); | ||
localStorage.setItem(FLAGGED_ACCOUNT_DATE_STORAGE_ID, time.toString()); | ||
} 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) || "[]", | ||
); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea! |
||
} | ||
|
||
return accounts; | ||
}, | ||
); | ||
|
||
const flaggedAccountsSlice = createSlice({ | ||
name: "flaggedAccounts", | ||
initialState, | ||
reducers: {}, | ||
extraReducers: (builder) => { | ||
builder.addCase( | ||
fetchFlaggedAccountsAction.pending, | ||
(state = initialState) => { | ||
state.status = ActionStatus.PENDING; | ||
}, | ||
); | ||
builder.addCase(fetchFlaggedAccountsAction.fulfilled, (state, action) => { | ||
state.status = ActionStatus.SUCCESS; | ||
state.data = action.payload; | ||
}); | ||
}, | ||
}); | ||
|
||
export const { reducer } = flaggedAccountsSlice; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
const UNSAFE_ACCOUNTS_URL = | ||
"https://api.stellar.expert/explorer/directory?limit=20000000&tag[]=malicious&tag[]=unsafe"; | ||
// setting limit very high as there doesn't appear to be a better way to get all entries from API | ||
|
||
const RESPONSE_TIMEOUT = 5000; | ||
// if API doesn't respond in this amount of time, we'll cancel the request | ||
|
||
export const getFlaggedAccounts = async () => { | ||
const controller = new AbortController(); | ||
const timeoutId = setTimeout(() => controller.abort(), RESPONSE_TIMEOUT); | ||
|
||
const flaggedAccountsRes = await fetch(UNSAFE_ACCOUNTS_URL, { | ||
signal: controller.signal, | ||
}); | ||
clearTimeout(timeoutId); | ||
const flaggedAccountsJson = await flaggedAccountsRes.json(); | ||
|
||
const { | ||
_embedded: { records: unsafeAccountsData }, | ||
} = flaggedAccountsJson; | ||
|
||
return unsafeAccountsData.map( | ||
({ address, tags }: { address: string; tags: [] }) => ({ address, tags }), | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,21 @@ | ||
import React, { useEffect } from "react"; | ||
import { useDispatch } from "react-redux"; | ||
import styled from "styled-components"; | ||
import { BalanceInfo } from "components/BalanceInfo"; | ||
import { TransactionHistory } from "components/TransactionHistory"; | ||
import { logEvent } from "helpers/tracking"; | ||
import { fetchFlaggedAccountsAction } from "ducks/flaggedAccounts"; | ||
|
||
const WrapperEl = styled.div` | ||
width: 100%; | ||
`; | ||
|
||
export const Dashboard = () => { | ||
const dispatch = useDispatch(); | ||
useEffect(() => { | ||
dispatch(fetchFlaggedAccountsAction()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. request flagged accounts as soon as dashboard loads |
||
logEvent("page: saw account main screen"); | ||
}, []); | ||
}, [dispatch]); | ||
|
||
return ( | ||
<WrapperEl> | ||
|
There was a problem hiding this comment.
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 thatreact-scripts
uses. Another option is to updatereact-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
There was a problem hiding this comment.
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. 👍
There was a problem hiding this comment.
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!