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.

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

MythX plugin #1999

Merged
merged 10 commits into from
Jun 14, 2019
Merged

MythX plugin #1999

merged 10 commits into from
Jun 14, 2019

Conversation

aquiladev
Copy link
Contributor

src/remixAppManager.js Outdated Show resolved Hide resolved
@muellerberndt
Copy link

@aquiladev what are the steps to get run an analysis?

I started your remix branch and compiled a couple of contracts, but the MythX tab keeps showing the message "You need to compile your contract first!". Not sure if I'm doing it wrong.

Screen Shot 2019-05-22 at 21 02 16

@aquiladev
Copy link
Contributor Author

@b-mueller Yeah, there is an issue with throwing events on the local environment. I'm figuring out it.
You can open http://remix-alpha.ethereum.org and Connect to Local Plugin in order to test it

image

@muellerberndt
Copy link

@aquiladev works pretty nicely! Some feedback on the reporting:

  1. Left-clicking on the SWC link opens the code editor (in Safari at least).
  2. It's not really clear that clicking on the issue title leads to code highlighting. How about putting that link only on the "line:column" part (but with link style)?

E.g.

[1:0] A floating pragma is set [SWC-1xx]
^ Link to code                           ^ Link to SWC
  1. How about displaying the long description on the report page as well?

@Gtonizuka
Copy link

looks good to me. @aquiladev are you planning implement more features from mythx?

@aquiladev
Copy link
Contributor Author

@b-mueller I'll take a look. There is an issue with SWC link, coz the plugin is rendered in an iframe I think.
@Gtonizuka what features? I open to discuss

@muellerberndt
Copy link

@aquiladev I think feature-wise this is pretty complete. What is still missing is some tool tips and explanation what the analysis actually does and how to obtain the API credentials. Probably also a hint that the analysis takes around 2 minutes to complete.

Not sure where to best put all of this design-wise. I'll also look over the texts in the Remix-plugin repo.

@muellerberndt
Copy link

@aquiladev One feature that would be nice to have would be a possibility to view full issue descriptions (which are returned by the API in the 'tail' field), or even view/export a detailed report that contains the full descriptions.

We are currently also working on returning call traces that show how a particular issue/exception is triggered which will be help users to determine the cause of an issue, so some space in the UI will be needed to display that additional info.

We are planning to offer a reports dashboard on the MythX website itself, but this is still a few months away :(

@@ -110,11 +110,20 @@ export class RemixAppManager extends AppManagerApi {
icon: 'data:image/svg+xml;base64,PD94bWwgdmVyc2lvbj0iMS4wIiA/PjxzdmcgaGVpZ2h0PSIxMDI0IiB3aWR0aD0iMTAyNCIgeG1sbnM9Imh0dHA6Ly93d3cudzMub3JnLzIwMDAvc3ZnIj48cGF0aCBkPSJNOTUwLjE1NCAxOTJINzMuODQ2QzMzLjEyNyAxOTIgMCAyMjUuMTI2OTk5OTk5OTk5OTUgMCAyNjUuODQ2djQ5Mi4zMDhDMCA3OTguODc1IDMzLjEyNyA4MzIgNzMuODQ2IDgzMmg4NzYuMzA4YzQwLjcyMSAwIDczLjg0Ni0zMy4xMjUgNzMuODQ2LTczLjg0NlYyNjUuODQ2QzEwMjQgMjI1LjEyNjk5OTk5OTk5OTk1IDk5MC44NzUgMTkyIDk1MC4xNTQgMTkyek01NzYgNzAzLjg3NUw0NDggNzA0VjUxMmwtOTYgMTIzLjA3N0wyNTYgNTEydjE5MkgxMjhWMzIwaDEyOGw5NiAxMjggOTYtMTI4IDEyOC0wLjEyNVY3MDMuODc1ek03NjcuMDkxIDczNS44NzVMNjA4IDUxMmg5NlYzMjBoMTI4djE5Mmg5Nkw3NjcuMDkxIDczNS44NzV6Ii8+PC9zdmc+',
location: 'sidePanel'
}
var mythx = {
name: 'remythx',
displayName: 'MythX',

Choose a reason for hiding this comment

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

Change to:

displayName: 'MythX Security Verification (Beta)'
description: 'Perform Static and Dynamic Security Analysis using the MythX Cloud Service'

@aquiladev
Copy link
Contributor Author

@aquiladev One feature that would be nice to have would be a possibility to view full issue descriptions (which are returned by the API in the 'tail' field), or even view/export a detailed report that contains the full descriptions.

tail is displaying on hover of issue. it is not obvious, but when I display it fully it is too messy. I need to think about UX

@aquiladev
Copy link
Contributor Author

@aquiladev I think feature-wise this is pretty complete. What is still missing is some tool tips and explanation what the analysis actually does and how to obtain the API credentials. Probably also a hint that the analysis takes around 2 minutes to complete.

I've added info tooltips

@muellerberndt
Copy link

I tested the plugin pretty extensively and it's quite awesome. The only real drawback from the user's view is that there's no option to view a full security report that also contains the detailed descriptions and the soon-to-be-added call traces. In some cases this will mean that the results are not particularly useful (for example when we detect a reachable exception but don't output the transaction sequence / inputs that trigger it).

This is also a question for @yann300 - is it possible to to add a pop/overlay that displays a full report on request? Possibly we can set up an additional bounty for this. Also tagging @Gtonizuka @aquiladev

@yann300
Copy link
Collaborator

yann300 commented Jun 3, 2019

is it possible to to add a pop/overlay that displays a full report on request?

@b-mueller
not at the moment, but we are thinking of it. It is also possible to build a mainPanel plugin, that will be displayed as a tab. That why the view has more space for displaying more detailed informations.
cc @GrandSchtroumpf

@yann300
Copy link
Collaborator

yann300 commented Jun 5, 2019

btw you can also provide a documentation property which should be an URL pointing to the doc if you have one.

@aquiladev
Copy link
Contributor Author

@yann300 if readme.md is enough I can add it

@yann300
Copy link
Collaborator

yann300 commented Jun 5, 2019

yes I suppose that should be fine for the moment

@yann300 yann300 merged commit 01597bb into ethereum:master Jun 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants