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

allow-tools should default to false #1940

Closed
sigurdm opened this issue Mar 5, 2019 · 6 comments
Closed

allow-tools should default to false #1940

sigurdm opened this issue Mar 5, 2019 · 6 comments

Comments

@sigurdm
Copy link

sigurdm commented Mar 5, 2019

I think that having dartdoc run arbitrary code on a default invocation is risky and for me seems like a surprising behavior.

I suggest to change the default here:

new DartdocOptionArgOnly<bool>('allowTools', true,

@devoncarew
Copy link
Member

This seems like a reasonable change to me.

I believe that @gspencergoog added this feature; pinging him for an opinion -

@gspencergoog
Copy link
Collaborator

No, it was @jcollins-g that added the flag, but I reviewed the PR.

It's not exactly "arbitrary code" though: you have to run Dartdoc with a configuration file that includes the path to the executable you want to run, and it will only run tools configured in the configuration file. The "name" of the tool to run is a key into this configuration file, not a path to (or even necessarily the name of) an executable.

We could make it default to false, but that doesn't actually give you any more security: the same configuration file could be used to set it to true, and also set the executable path.

@sigurdm
Copy link
Author

sigurdm commented Mar 7, 2019

If you download a third-party package and run dartdoc on it, the dartdoc_options.yaml file of that library is used?
I would not expect people to inspect the dartdoc_options file before running dartdoc.

But the second point is really good. Changing the default is not enough (cc @jonasfj @isoos ) - it is (IMO) questionable if we should allow enabling this from the dartdoc_options file itself.

@keertip
Copy link
Collaborator

keertip commented Jun 24, 2019

How about if we make it such that allow-tools cannot be configured from the dartdoc_options file. allow-tools defaults to false and the only way to run it on is to explicitly pass it in on command line.

This will be a breaking change. @jonasfj @isoos are there other packages apart from flutter using this? Do we know?

@jonasfj
Copy link
Member

jonasfj commented Jun 24, 2019

I'm not familiar with all users of dartdoc... Maybe the dart-sdk uses it, athom might know if the Dart release process uses it.

We obviously don't use it on pub.dev :)

@keertip
Copy link
Collaborator

keertip commented Jun 24, 2019

The SDK does not use it. if pub.dev does not use it, then it's just flutter.

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

No branches or pull requests

5 participants