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.

Sign up for GitHub

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

New design for phishing warning page #52

Merged
merged 9 commits into from
Feb 15, 2023
Merged

New design for phishing warning page #52

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f8bf7b0
Added new design for phishing warning page
aleksandar-mihajlovic Jan 27, 2023
b4957e8
Changed static hex values for colors to token variables
aleksandar-mihajlovic Jan 30, 2023
a614c54
Updated font of button to use token variables
aleksandar-mihajlovic Jan 30, 2023
a133e54
Added new CSS corrections
aleksandar-mihajlovic Feb 3, 2023
4dfff44
Added new CSS changes and logic for dynamic text for detection repo
aleksandar-mihajlovic Feb 6, 2023
2866a6d
Code cleanup
aleksandar-mihajlovic Feb 6, 2023
ca16483
Merge branch 'main' into phishing-warning-page-update
aleksandar-mihajlovic Feb 6, 2023
842cd3b
Updated html to use href instead of onclick method
aleksandar-mihajlovic Feb 6, 2023
de479bc
Removed unnecessary code
aleksandar-mihajlovic Feb 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 21 additions & 29 deletions src/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,15 +71,13 @@ describe('Phishing warning page', () => {

it('should render page', async () => {
const container = window.document.getElementsByTagName('body')[0];
expect(
queryByText(container, 'MetaMask Phishing Detection'),
).not.toBeNull();
expect(queryByText(container, 'Deceptive site ahead')).not.toBeNull();
});

it('should have correct default "New issue" link', () => {
Gudahtt marked this conversation as resolved.
Show resolved Hide resolved
const newIssueLink = window.document.getElementById('new-issue-link');
expect(newIssueLink?.getAttribute('href')).toBe(
'https://github.com/metamask/eth-phishing-detect/issues/new',
expect(newIssueLink?.getAttribute('onclick')).toBe(
"window.open('https://github.com/metamask/eth-phishing-detect/issues/new')",
);
});

Expand Down Expand Up @@ -120,30 +118,6 @@ describe('Phishing warning page', () => {
'should add site to safelist when the user continues at their own risk',
);

it.todo(
'should redirect to the site after the user continues at their own risk',
);

it('should show a different message if the URL contains an unsupported protocol', async () => {
window.document.location.href = getUrl(
'example.com',
// eslint-disable-next-line no-script-url
'javascript:alert("example")',
);

await import('./index');
// non-null assertion used because TypeScript doesn't know the event handler was run
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
onDomContentLoad!(new Event('DOMContentLoaded'));

const redirectWarningMessage = window.document.getElementById(
'redirect-warning-message',
);
expect(redirectWarningMessage?.innerText).toBe(
"This URL does not use a supported protocol so we won't give you the option to skip this warning.",
);
});

it('should throw an error if the hostname is missing', async () => {
window.document.location.href = getUrl(undefined, 'https://example.com');

Expand Down Expand Up @@ -199,4 +173,22 @@ describe('Phishing warning page', () => {
'Unable to locate unsafe continue link',
);
});

it('should redirect when continue to the site is clicked', async () => {
window.document.location.href = getUrl(
'example.com',
'https://example.com',
);

await import('./index');
// non-null assertion used because TypeScript doesn't know the event handler was run
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
onDomContentLoad!(new Event('DOMContentLoaded'));

const continueLink = document.getElementById('unsafe-continue');

continueLink?.click();

expect(global.window.location.href).toContain('example.com');
});
});
10 changes: 0 additions & 10 deletions src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,16 +137,6 @@ function start() {
throw new Error('Unable to locate unsafe continue link');
}

if (isValidSuspectHref(suspectHref) === false) {
Gudahtt marked this conversation as resolved.
Show resolved Hide resolved
const redirectWarningMessage = document.getElementById(
'redirect-warning-message',
);
if (redirectWarningMessage) {
redirectWarningMessage.innerHTML = `<br />`;
redirectWarningMessage.innerText = `This URL does not use a supported protocol so we won't give you the option to skip this warning.`;
}
}

continueLink.addEventListener('click', async () => {
if (isValidSuspectHref(suspectHref) === false) {
console.log(`Disallowed Protocol, cannot continue.`);
Expand Down
73 changes: 40 additions & 33 deletions static/index.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,17 +32,38 @@ body {
width: 100vw;
height: 100vh;

background-color: var(--color-error-default);
background-color: #d03242;
aleksandar-mihajlovic marked this conversation as resolved.
Show resolved Hide resolved
font-family: Euclid, Roboto, Helvetica, Arial, sans-serif;
}

p {
margin: 2em;
}

p a {
text-decoration: underline;
color: var(--color-primary-default);
color: white;
aleksandar-mihajlovic marked this conversation as resolved.
Show resolved Hide resolved
cursor: pointer;
}

h1 {
color: black;
-webkit-text-fill-color: white;
-webkit-text-stroke-width: 1px;
-webkit-text-stroke-color: black;
text-shadow: 0px 4px rgba(0,0,0, 0.15);
}

ul {
padding-left: 2em;
}

button {
color: red;
background: white;
aleksandar-mihajlovic marked this conversation as resolved.
Show resolved Hide resolved
padding: 1em;
border-radius: 2em;
border: none;
float: right;
margin-top: 3em;
font-size: 0.9em;
font-family: Euclid, Roboto, Helvetica, Arial, sans-serif;
cursor: pointer;
}

Expand All @@ -51,48 +72,34 @@ p a {
flex-direction: column;
align-items: center;
justify-content: center;
width: 80%;
background-color: var(--color-background-default);
box-shadow: 0 0 15px var(--brand-colors-grey-grey500);
width: 50%;
padding: 5rem;
}

.content__header {
display: flex;
flex-direction: column;
align-items: center;
justify-content: center;
width: 100%;
color: var(--color-error-default);
border-bottom: 1px solid var(--color-border-default);
padding: 2em;
margin-bottom: 1rem;
}

.content__header h1 {
font-size: 1.5rem;
font-weight: normal;
}

.content__header h1 svg {
margin-right: 0.25em;
fill: var(--color-error-default);
width: 1em;
height: 1em;
vertical-align: middle;
font-size: 2rem;
font-weight: bold;
color: white;
float: left;
}

.content__header img {
margin-bottom: 3em;
width: 130px;
.content__header svg {
fill: white;
width: 4em;
height: 4em;
margin-bottom: 2em;
}

.content__body {
background-color: var(--color-background-alternative);
font-size: 1rem;
color: var(--color-text-default);
}

.content__framed-body {
background-color: var(--color-background-alternative);
font-size: 1rem;
color: var(--color-text-default);
color: white;
}
77 changes: 23 additions & 54 deletions static/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@
document.getElementById('antiClickjack').innerHTML =
'#content__framed-body { display: none !important; }';
}

window.onload = function() {
document.getElementById("suspect-link").innerHTML = document.referrer;
aleksandar-mihajlovic marked this conversation as resolved.
Show resolved Hide resolved
}
</script>
<title>MetaMask Phishing Detection</title>
<script
Expand Down Expand Up @@ -46,67 +50,32 @@
<body>
<div class="content">
<div class="content__header">
<img src="./metamask-fox.svg" alt="MetaMask Logo" />
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 512 512"><!--! Font Awesome Pro 6.2.0 by @fontawesome - https://fontawesome.com License - https://fontawesome.com/license (Commercial License) Copyright 2022 Fonticons, Inc. -->
<path d="M256 32c14.2 0 27.3 7.5 34.5 19.8l216 368c7.3 12.4 7.3 27.7 .2 40.1S486.3 480 472 480H40c-14.3 0-27.6-7.7-34.7-20.1s-7-27.8 .2-40.1l216-368C228.7 39.5 241.8 32 256 32zm0 128c-13.3 0-24 10.7-24 24V296c0 13.3 10.7 24 24 24s24-10.7 24-24V184c0-13.3-10.7-24-24-24zm32 224c0-17.7-14.3-32-32-32s-32 14.3-32 32s14.3 32 32 32s32-14.3 32-32z"/>
</svg>
<h1>
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 512 512" preserveAspectRatio="xMidYMid meet"><!-- Font Awesome Pro 5.15.4 by @fontawesome - https://fontawesome.com License - https://fontawesome.com/license (Commercial License) --><path d="M504 256c0 136.997-111.043 248-248 248S8 392.997 8 256C8 119.083 119.043 8 256 8s248 111.083 248 248zm-248 50c-25.405 0-46 20.595-46 46s20.595 46 46 46 46-20.595 46-46-20.595-46-46-46zm-43.673-165.346l7.418 136c.347 6.364 5.609 11.346 11.982 11.346h48.546c6.373 0 11.635-4.982 11.982-11.346l7.418-136c.375-6.874-5.098-12.654-11.982-12.654h-63.383c-6.884 0-12.356 5.78-11.981 12.654z"/></svg>
MetaMask Phishing Detection
Deceptive site ahead
</h1>
</div>
<div id="content__body" class="content__body">
<p>
This domain is currently on the MetaMask domain warning list. This
means that based on information available to us, MetaMask believes
this domain could currently compromise your security and, as an added
safety feature, MetaMask has restricted access to the site. To
override this, please read the rest of this warning for instructions
on how to continue at your own risk.
</p>
<p>
There are many reasons sites can appear on our warning list, and our
warning list compiles from other widely used industry lists. Such
reasons can include known fraud or security risks, such as domains
that test positive on the
<a href="https://github.com/metamask/eth-phishing-detect"
>Ethereum Phishing Detector</a
>. Domains on these warning lists may include outright malicious
websites and legitimate websites that have been compromised by a
malicious actor.
</p>
<p>
To read more about this site
<a id="csdbLink" href="https://cryptoscamdb.org/search"
>please search for the domain on CryptoScamDB</a
>.
</p>
<p>
Note that this warning list is compiled on a voluntary basis. This
list may be inaccurate or incomplete. Just because a domain does not
appear on this list is not an implicit guarantee of that domain's
safety. As always, your transactions are your own responsibility. <span id="redirect-warning-message">If
you wish to interact with any domain on our warning list, you can do
so by <a id="unsafe-continue">continuing at your own risk</a>.</span>
MetaMask flagged the site you're trying to visit as potentially deceptive. Attackers may trick you into doing something dangerous. <a id="csdbLink" href='' onclick="window.open('https://cryptoscamdb.org/search')">Learn more</a>
Copy link
Member

Choose a reason for hiding this comment

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

this call unsafely allows reverse tab nabbing attacks (read more here) - please fix by adding attribute rel="noopener"

Copy link
Member

Choose a reason for hiding this comment

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

also, why use onclick and open when you have the default behaviour of an anchor element?

<a id="csdbLink" rel="noopener" href='https://cryptoscamdb.org/search' target='_blank'">Learn more</a>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay no problem, if it is making security problems, i will changed it right now, you can look at it, if it is okay. @weizman

</p>
<p>
If you think this domain is incorrectly flagged or if a blocked
legitimate website has resolved its security issues,
<a
id="new-issue-link"
href="https://github.com/metamask/eth-phishing-detect/issues/new"
>please file an issue</a
>.
</p>
</div>
<div id="content__framed-body" class="content__framed-body">
<p>
This domain is currently on the MetaMask domain warning list. This
means that based on information available to us, MetaMask believes
this domain could currently compromise your security and, as an added
safety feature, MetaMask has restricted access to the site.
</p>
<p>
<a id="open-self-in-new-tab" target="_blank">Open this warning in a new tab</a> for more information
on why this domain is blocked, and how to continue at your own risk.
</br>
<p>Potential threats on <span id="suspect-link"></span> include:
<ul>
<li>Fake versions of MetaMask</li>
<li>Secret Recovery Phrase or password theft</li>
<li>Malicious transactions resulting in stolen assets </li>
</ul>
</p>
</br>
<p>Advisory provided by Ethereum Phishing Detector, CryptoScamDB, and PhishFort.</p>
Copy link

Choose a reason for hiding this comment

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

This list should be dynamic. We should expose ONLY the sources who are flagging the current URL @aleksandar-mihajlovic @dzfjz

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, as I understand, these websites are not scam websites, they are websites that give users info about how to avoid phishing, so they don't need to be listed dynamically. Am I wrong?
And about source that is scam website, the link will be shown in this sentence:
Potential threats on {someWebsite} include:
but i cannot demo it, because i cannot build the project and connect with extension to test it out, but website link will be there once it goes to production.
@holantonela

Copy link

Choose a reason for hiding this comment

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

To do so, we need to add a query string in the PhishingController to say which list flagged it. cc @409H

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 am not sure that I understand what I need to do here, because i am not sure what i am doing wrong, if you could explain it to me

Copy link

Choose a reason for hiding this comment

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

This is the point 2 in the Acceptance Criteria

  1. Users can read the attribution to who flagged the domain, probably passed to that page with a query string.

MetaMask/metamask-extension#16602

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@409H I currently implemented logic to detect through current query string, I would say that I found a decent solution for now, but in future when other detect repo's are added, this logic should be updated too, if that is okay?

Copy link
Member

Choose a reason for hiding this comment

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

@409H thanks for linking that, I had failed to consider this requirement when reviewing a cost-saving change to how we poll for the phishing list. Unfortunately this information will no longer be available after that change because we're now combining all lists into one to save on network bandwidth.

Still, we can fulfill this requirement without that data by checking on-demand after a user chooses to dispute a block. Tomorrow I will create a separate issue detailing how this needs to change.

Copy link
Member

Choose a reason for hiding this comment

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

@aleksandar-mihajlovic My apologies, I hadn't looked closely at this PR when I left my first comment.

I don't think we need the onload function you added for this, because that same logic is already handled by index.ts. It was implemented here: #23. We pass in a custom dispute URL if a site is blocked by Phishfort, otherwise we default to ours. As long as that newIssueUrl continues to be set dynamically on this link, no change to this behavior is needed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Gudahtt Okay, i removed that part of the code for linking on the onload function, and returned to the old state like you mentioned, is it okay now?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @aleksandar-mihajlovic , yes this thread looks resolved now

</br>
</br>
<p>If we're flagging a legitimate website, please <a id="new-issue-link" href='' onclick="window.open('https://github.com/metamask/eth-phishing-detect/issues/new')">report a detection problem.</a></p>
<p>If you understand the risks and still want to proceed, you can <a id="unsafe-continue">continue to the site.</a></p>
<button class="button btn--rounded btn-primary" type="submit" onclick="window.close()">Back to safety</button>
aleksandar-mihajlovic marked this conversation as resolved.
Show resolved Hide resolved
</div>
</div>
</body>
Expand Down