New design for phishing warning page #52

aleksandar-mihajlovic · 2023-01-27T12:14:16Z

Explanation

Added new design for phishing warning page according to figma design.

Fixes #16602

Screenshots/Screencaps

Before

After

Manual Testing Steps

Production testing:

Go to one of the links: primalfrens.club, geishateahouse.io, cryptocity.fun
Phishing warning page is displayed
All links are working
Advisory provided by is written based on the source of the detection repo (in link there is info if the Metamask or Phishfort is detection repo)
When back to safety is clicked, tab is closed

Local testing:

static/ folder
Double click on index.html
Phishing warning page is displayed
All links are working expect continue to the site, because production app is needed for that
Advisory provided by is written based on the source of the detection repo (in link there is info if the Metamask or Phishfort is detection repo)
When back to safety is clicked, tab is closed

georgewrmarshall

Hey @aleksandar-mihajlovic, nice work matching the Figma design but I think it breaks some of the MetaMask design system conventions that I think should be resolved before this get's merged.

Hey @holantonela, I've left some comments in the Figma file https://www.figma.com/file/MKJnCnI8Xi2EVcRCNAHwOu?node-id=4:49#355806441

static/index.css

dzfjz · 2023-01-30T10:28:39Z

Verified by QA

holantonela · 2023-01-30T13:37:43Z

@georgewrmarshall commented in the figma. Someone introduced that change, but i bet it was a click error. Please, @aleksandar-mihajlovic lets remove the border and the shadow of the title. Thank you!

georgewrmarshall

Looking good! Have made one more suggestion. Thanks for your patience @aleksandar-mihajlovic 🙏

static/index.css

holantonela · 2023-01-31T18:54:24Z

static/index.html

        </p>
+      </br>
+        <p>Advisory provided by Ethereum Phishing Detector, CryptoScamDB, and PhishFort.</p>


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

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

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

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

This is the point 2 in the Acceptance Criteria

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

MetaMask/metamask-extension#16602

@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?

@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.

@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.

@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?

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

georgewrmarshall

Looking good. There maybe one more update needed to the button that is being resolved in the Figma file

static/index.html

NicholasEllul

This page looks fantastic! Glad to see it being implemented

static/index.html

aleksandar-mihajlovic · 2023-02-03T13:02:24Z

@georgewrmarshall can you check the attached gif, is the button okay now? I used the Button Secondary as mentioned in Figma.

georgewrmarshall

Looking good! I've created another PR with some more CSS suggestions #55 that you can review and merge into this PR if you think they are acceptable.

aleksandar-mihajlovic · 2023-02-06T12:31:33Z

I have added the CSS code that you suggested @georgewrmarshall if you can check it out.
I have added logic for setting dynamically text and link also, for phishing detection repo that detected phishing website, if you can check it out @holantonela.

weizman

minor security concern

weizman · 2023-02-06T16:19:14Z

static/index.html

-          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>


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

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>

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

AlexHerman1 · 2023-02-06T19:51:53Z

can we make sure that whatever ends up going to prod, we make the attribution prominent and point them towards the correct contributor to raise an issue?

georgewrmarshall

Looks great! Going to approve for visual and design system requirements. Nice work! 💯 Please don't merge until all reviewers have approved 🙏

holantonela · 2023-02-08T17:39:13Z

can we make sure that whatever ends up going to prod, we make the attribution prominent and point them towards the correct contributor to raise an issue?

Yes @AlexHerman1, we discussed it here

src/index.test.ts

src/index.ts

Gudahtt · 2023-02-14T21:52:51Z

static/index.html

@@ -11,6 +11,17 @@
        document.getElementById('antiClickjack').innerHTML =
          '#content__framed-body { display: none !important; }';
      }
+
+      window.onload = function() {
+        document.getElementById("suspect-link").innerText = document.referrer;


Nit: Could you move this code to the start function in src/index.ts? That is where all of our other "page initialization" code is.

Gudahtt

LGTM! I can address the remaining two nits in a follow-up PR.

Requested change has been made

This test was recently removed in #52 due to a change on that PR that was since undone. It is still relevant, so it has been restored.

The "Open in new tab" link that we show on the warning page when it's rendered in an iframe was accidentally removed in #52. We need to preserve this button so that users can better understand why their iframe'd page is blocked, and so they can dispute or bypass the block if necessary.

Added new design for phishing warning page

f8bf7b0

aleksandar-mihajlovic requested a review from a team as a code owner January 27, 2023 12:14

aleksandar-mihajlovic marked this pull request as draft January 27, 2023 12:16

georgewrmarshall requested changes Jan 27, 2023

View reviewed changes

static/index.css Outdated Show resolved Hide resolved

static/index.css Outdated Show resolved Hide resolved

static/index.css Outdated Show resolved Hide resolved

Changed static hex values for colors to token variables

b4957e8

aleksandar-mihajlovic marked this pull request as ready for review January 30, 2023 14:10

holantonela mentioned this pull request Jan 30, 2023

Improve general warning page UI #39

Closed

georgewrmarshall reviewed Jan 30, 2023

View reviewed changes

static/index.css Outdated Show resolved Hide resolved

Updated font of button to use token variables

a614c54

holantonela reviewed Jan 31, 2023

View reviewed changes

georgewrmarshall reviewed Jan 31, 2023

View reviewed changes

static/index.html Outdated Show resolved Hide resolved

NicholasEllul reviewed Jan 31, 2023

View reviewed changes

static/index.html Outdated Show resolved Hide resolved

NicholasEllul reviewed Jan 31, 2023

View reviewed changes

static/index.html Outdated Show resolved Hide resolved

Added new CSS corrections

a133e54

georgewrmarshall mentioned this pull request Feb 6, 2023

Phishing warning page update more CSS suggestions #55

Closed

georgewrmarshall reviewed Feb 6, 2023

View reviewed changes

Added new CSS changes and logic for dynamic text for detection repo

4dfff44

aleksandar-mihajlovic and others added 2 commits February 6, 2023 13:32

Code cleanup

2866a6d

Merge branch 'main' into phishing-warning-page-update

ca16483

weizman previously requested changes Feb 6, 2023

View reviewed changes

Updated html to use href instead of onclick method

842cd3b

georgewrmarshall previously approved these changes Feb 6, 2023

View reviewed changes

Removed unnecessary code

de479bc

aleksandar-mihajlovic dismissed georgewrmarshall’s stale review via de479bc February 7, 2023 11:29

Gudahtt reviewed Feb 14, 2023

View reviewed changes

src/index.test.ts Show resolved Hide resolved

src/index.ts Show resolved Hide resolved

Gudahtt reviewed Feb 14, 2023

View reviewed changes

holantonela mentioned this pull request Feb 15, 2023

Improve designs for dynamic lookup the source of a block #56

Open

Gudahtt requested a review from weizman February 15, 2023 20:34

Gudahtt approved these changes Feb 15, 2023

View reviewed changes

Gudahtt merged commit f365563 into MetaMask:main Feb 15, 2023

Gudahtt added a commit that referenced this pull request Feb 15, 2023

Restore test

0c518c6

This test was recently removed in #52 due to a change on that PR that was since undone. It is still relevant, so it has been restored.

Gudahtt mentioned this pull request Feb 15, 2023

Restore test #60

Merged

Gudahtt added a commit that referenced this pull request Feb 15, 2023

Restore test (#60)

d3f6da0

This test was recently removed in #52 due to a change on that PR that was since undone. It is still relevant, so it has been restored.

Gudahtt mentioned this pull request Feb 17, 2023

2.0.0 #70

Merged

Gudahtt mentioned this pull request Feb 20, 2023

Restore iframe warning and "open in new tab" link #73

Merged

Gudahtt mentioned this pull request Feb 21, 2023

Update the PhishingController to v2 and update phishing warning page MetaMask/metamask-extension#17835

Merged

8 tasks

bschorchit mentioned this pull request Feb 23, 2023

Improve our phishing warning page MetaMask/metamask-extension#16602

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New design for phishing warning page #52

New design for phishing warning page #52

aleksandar-mihajlovic commented Jan 27, 2023 •

edited

Loading

georgewrmarshall left a comment

dzfjz commented Jan 30, 2023

holantonela commented Jan 30, 2023

georgewrmarshall left a comment

holantonela Jan 31, 2023

aleksandar-mihajlovic Jan 31, 2023

holantonela Feb 1, 2023

aleksandar-mihajlovic Feb 1, 2023

holantonela Feb 1, 2023

aleksandar-mihajlovic Feb 6, 2023

Gudahtt Feb 6, 2023

Gudahtt Feb 6, 2023

aleksandar-mihajlovic Feb 7, 2023

Gudahtt Feb 14, 2023

georgewrmarshall left a comment

NicholasEllul left a comment

aleksandar-mihajlovic commented Feb 3, 2023

georgewrmarshall left a comment •

edited

Loading

aleksandar-mihajlovic commented Feb 6, 2023

weizman left a comment

weizman Feb 6, 2023

weizman Feb 6, 2023

aleksandar-mihajlovic Feb 6, 2023

AlexHerman1 commented Feb 6, 2023

georgewrmarshall left a comment

holantonela commented Feb 8, 2023

Gudahtt Feb 14, 2023

Gudahtt left a comment

New design for phishing warning page #52

New design for phishing warning page #52

Conversation

aleksandar-mihajlovic commented Jan 27, 2023 • edited Loading

Explanation

Screenshots/Screencaps

Before

After

Manual Testing Steps

georgewrmarshall left a comment

Choose a reason for hiding this comment

dzfjz commented Jan 30, 2023

holantonela commented Jan 30, 2023

georgewrmarshall left a 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

georgewrmarshall left a comment

Choose a reason for hiding this comment

NicholasEllul left a comment

Choose a reason for hiding this comment

aleksandar-mihajlovic commented Feb 3, 2023

georgewrmarshall left a comment • edited Loading

Choose a reason for hiding this comment

aleksandar-mihajlovic commented Feb 6, 2023

weizman left a 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

AlexHerman1 commented Feb 6, 2023

georgewrmarshall left a comment

Choose a reason for hiding this comment

holantonela commented Feb 8, 2023

Choose a reason for hiding this comment

Gudahtt left a comment

Choose a reason for hiding this comment

aleksandar-mihajlovic commented Jan 27, 2023 •

edited

Loading

georgewrmarshall left a comment •

edited

Loading