moved to using href only for suspect site #124
Conversation
|
No dependency changes detected. Learn more about Socket for GitHub ↗︎ 👍 No dependency changes detected in pull request |
| @@ -178,8 +198,8 @@ function start() { | |||
| } | |||
|
|
|||
| const newIssueParams = `?title=[Legitimate%20Site%20Blocked]%20${encodeURIComponent( | |||
| suspectHostname, | |||
| )}&body=${encodeURIComponent(suspectHref)}`; | |||
| suspectHrefPlain, | |||
There was a problem hiding this comment.
Here we change from using the hostname in the issue title to the href. This can potentially result in long issue title names if the href if very long. I don't have strong opinions, but just want to verify that this was intentional
There was a problem hiding this comment.
I was't certain about this one, would switching to suspectHostnamePlain be better?
There was a problem hiding this comment.
After thinking abou it a bit more, since the issue would be regarding unblocking a domain (the path is not relevant), I think adding only the hostname makes the most sense
There was a problem hiding this comment.
It looks like this was kept as the full href, rather than the hostname. Was this a mistake?
Not a huge issue either way, but I am inclined to agree with @NicholasEllul that it would be better to just reference the hostname. The extra information in the href is effectively ignored. Including that in the eth-phishing-detect issue might be confusing, and it certainly wouldn't be helpful.
| throw new Error(`Invalid 'href' query parameter`); | ||
| } |
There was a problem hiding this comment.
Throwing an error here is a good call so that we reject untrusted inputs. However, this will break us setting up the back to safety button on the page.
Can we restructure this so that we ensure that the back to safety button is set up correctly before we process this input? That way, if we do raise an error on the page, at least the button will still let the user navigate back to their original page.
There was a problem hiding this comment.
Great catch here @NicholasEllul
witmicko
changed the title
Added Dev build with watchify.
fixes https://github.com/MetaMask/pm-security/issues/230