Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
moved to using href only for suspect site #124
moved to using href only for suspect site #124
Changes from 3 commits
186af61
15202a6
433bc23
b2cbcfe
22e9b60
8c5c74b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch here @NicholasEllul
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was't certain about this one, would switching to suspectHostnamePlain be better?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.