🐛 deno v1.13 breaks shims using --allow-plugin

[rivy]

With the change in v1.13 from --allow-plugin to --allow-ffi (PR #11152), prior shims using --allow-plugin are now broken...

error: Found argument '--allow-plugin' which wasn't expected, or isn't valid in this context
        Did you mean --allow-run?

USAGE:
    deno run <SCRIPT_ARG>... --allow-env=<allow-env> --allow-net=<allow-net> --allow-read=<allow-read> --allow-run=<allow-run> --allow-write=<allow-write>

For more information try --help

IMO, --allow-plugin should have been deprecated but allowed until a later date (maybe v2). Changing the command line API like this can suddenly break working setups (as it did mine). To me, this seems to be the definition of a semver API change requiring a major version update.

[dsherret]

Plugins and FFI are unstable. That said, it seems like something could be added to --allow-ffi's argument description to make this more clear.

deno/cli/flags.rs

Line 1277 in df084b9

.help("Allow loading dynamic libraries"),

[rivy]

Unfortunately, it's a bit more problematic.

For my installs, using deno install -A ... creates a shim with all the individual flags in the command line (including --allow-plugin for < v1.13; and, I assume, --allow-ffi for v1.13+). So, anyone installing an application with deno install -A ... on versions earlier than v1.13 will have a broken shim after upgrading to v1.13, even if they never used (or knew about) the --allow-plugin flag.

And if --allow-ffi goes away, it'll break all over again.

[x7y62]

They should make the --allow-plugin flag an alias for the --allow-ffi. When v2 is out, they can remove the the alias with a notice/warning.

[MarkTiedemann]

They should make the --allow-plugin flag an alias for the --allow-ffi.

They cannot be aliased because they are not the same thing: One is a native plugin API, the other is a FFI API. They are not compatible.

And if --allow-ffi goes away, it'll break all over again.

Yes, it's unstable, it may break. Deno's stability is described in more detail in the manual: https://deno.land/manual@v1.13.2/runtime/stability

"You should be aware that many unstable APIs have not undergone a security review, are likely to have breaking API changes in the future, and are not ready for production."

---

PS: If you are not sure about the stability of an unstable API, sometimes it helps to check the roadmap (#11168). Usually, if an unstable API is mentioned, it's either "stabilize" or "remove". For example, "Remove native plugins API, introduce FFI API" has been the number 1 item on the roadmap for a while now. An example of an unstable API being stabilized is the new HTTP server which is mentioned as "Stabilize native HTTP server API".

[rivy]

@MarkTiedemann ,

Yes, it's unstable, it may break. Deno's stability is described in more detail in the manual: https://deno.land/manual@v1.13.2/runtime/stability

"You should be aware that many unstable APIs have not undergone a security review, are likely to have breaking API changes in the future, and are not ready for production."

PS: If you are not sure about the stability of an unstable API, sometimes it helps to check the roadmap (#11168). Usually, if an unstable API is mentioned, it's either "stabilize" or "remove". For example, "Remove native plugins API, introduce FFI API" has been the number 1 item on the roadmap for a while now. An example of an unstable API being stabilized is the new HTTP server which is mentioned as "Stabilize native HTTP server API".

I get it, and I agree with the process. But you're missing the point that normal use of deno is injecting this unstable API into normal shims. deno install -A ... is injecting --allow-plugin (or now --allow-ffi?) into the shim without any notice to the user that they are in unstable API territory. And, then upgrading to a next v1.x arbitrarily breaks previously working shims. IMO, that's a bug and not a good experience for users. At the very least, very visible notice about the possible problem (and solution) should be pushed with any version of Deno that might trigger this kind of issue.

[MarkTiedemann]

deno install -A ... is injecting --allow-plugin without any notice to the user that they are in unstable API territory

You need both --allow-ffi (or --allow-all, which includes --allow-ffi) and --unstable for this unstable feature to work (e.g. deno run --unstable --allow-ffi script.ts).

--allow-ffi allows using Deno.dlopen. But without --unstable, Deno.dlopen is not defined (i.e. you'll get an error message like Property 'dlopen' does not exist on type 'typeof Deno').

[rivy]

@MarkTiedemann , thanks for taking the time to look at this issue.

I do feel that I'm not correctly communicating the problem to you; I'll try to restate it and improve my explanation...

I understand that --unstable is necessary to use the unstable APIs. I also understand that people who want to use them are knowingly buying into the unstable API with all that entails. I'm not complaining or concerned about the proper use of the unstable command line flags by devs using an unstable API.

But the important point here is that deno, during ordinary usage, is adding the unstable --allow-plugin (and --allow-ffi?) command line flag to all app shims installed using deno install -A ...; eg, here is what deno install -A ... creates...

% generated by deno install %
@deno "run" "--allow-read" "--allow-write" "--allow-net" "--allow-env" "--allow-run" "--allow-plugin" "--allow-hrtime" "https://deno.land/x/..." %*

and

#!/bin/sh
# generated by deno install
deno "run" "--allow-read" "--allow-write" "--allow-net" "--allow-env" "--allow-run" "--allow-plugin" "--allow-hrtime" "https://deno.land/x/..." "$@"

This is not specifically requested by the user installing the app and is for the most part opaque to casual users. And, then, this acts as a future trip mine which, without any warning, breaks all those shims when upgrading from v1.12 to v1.13, blocking use of all the apps installed using that installation directive.

error: Found argument '--allow-plugin' which wasn't expected, or isn't valid in this context
        Did you mean --allow-run?

USAGE:
    deno run <SCRIPT_ARG>... --allow-env=<allow-env> --allow-net=<allow-net> --allow-read=<allow-read> --allow-run=<allow-run> --allow-write=<allow-write>

For more information try --help

Again, I see this as a bug which needs to be addressed going forward to avoid tripping up ordinary users.

Likely, a good solution going forward is to never add unstable flags when installing using deno install -A .... Teach deno to add unstable command line flags only when explicitly requested, ie, for access to an unstable API, the dev must use deno install -A --unstable --allow-ffi ....

[MarkTiedemann]

@rivy Thanks for clarifying!

I was not aware that deno install -A ... produced a deno run shim including all --allow-* flags.

This seems like a design issue IMO, too.

I would have expected deno install -A ... to produce a deno run -A ... shim.

[rivy]

@MarkTiedemann , expanding the -A in deno install -A ... into specific --allow-... arguments makes good sense in order to avoid unintended permission expansion if/when more --allow-... types are added with a later deno version. But only stable flags should be included in that automatic expansion. Any unstable flag (and --unstable itself) should require explicit opt-in.

[MarkTiedemann]

I don't think there's such a thing as "unintended permission expansion" when using -A.

-A is short for --allow-all which specifically means "Allow all permissions". Using -A is an intended permission expansion, if you will.

In this respect deno install behaves differently than deno run, which I think is an issue:

In deno run, --allow-all means "allow all permissions that are present in the currently used Deno runtime". In deno install, --allow-all instead means "allow all permissions that were present in the Deno runtime used to install the script, which may be a different one than the runtime that is currently executing the script"...

I think that's unexpected and should be fixed.

[rivy]

Yes, @MarkTiedemann, I believe we're saying the same thing...

When I say "unintended permission expansion" for a later version of deno, it means the same thing as don't add additional permissions that weren't present in the deno runtime version that installed the shim. To prevent such an unintended expansion of permissions, deno install -A ... has to expand the -A not to --allow-all but to the individual permissions, as it currently does.

I'm suggesting that the bug is that the expansion of -A should not add either --unstable (which it doesn't) nor any unstable permission (eg, --allow-plugin or --allow-ffi, which it does) to the expansion of -A. Otherwise the current behavior is reasonable and necessary.

[MarkTiedemann]

Yes, @MarkTiedemann, we're saying the same thing...

No, we're not.

I'm suggesting that the bug is that the expansion of -A should not add (...) any unstable permission (eg, --allow-plugin or --allow-ffi, which it does) to the expansion of -A.

I'm saying that the bug is that -A should not be expanded.

There should not be a difference between deno run -A and deno install -A. I expect deno install -A to produce a shim that executes deno run -A.

[rivy]

I'm saying that the bug is that -A should not be expanded.

There should not be a difference between deno run -A and deno install -A. I expect deno install -A to produce a shim that executes deno run -A.

Thanks for clarifying.

Hmm, I don't really agree; the current creation of a shim with individual --allow-... flags from deno install -A ... seems to be a good decision from a security standpoint. Using -A/--allow-all within the shim leaves a security hole if/when a future deno runtime expands the list of permission flags. For example, if a future deno adds a hypothetical --allow-SECRETKEY flag then a previously installed script gains that permission without dev/user notice or opt-in.

You could argue that, by using -A, the dev/user is opting in to all permissions, in perpetuity, for a given shim. But that seems to be a weaker security position and, IMO, the more surprising possibility to a dev/user. And if that's the stance, then I would at least warn the dev/user about that case when installing and in the docs.

But, it would be a simpler implementation.

[MarkTiedemann]

Using --allow-all is effectively saying that you want no security.

I don't understand the argument that you might want all flags now, but not a potential future flag. In that case, I think, you should specify all flags one by one rather than using --allow-all.

[hayd]

If you have -A and that expands to include --allow-run what precisely could a future flag add that would be more powerful than --allow-run?

I get what it is trying to do, but I don't think it's reasonable. If a user says they want -A then we should trust they want -A. (Maybe we can print a warning to say "you installed with -A but we strongly recommend enumerating the permissions...")

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

[rivy]

Not stale.

This should be addressed at least as a policy statement going forward so that it doesn't happen again...

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

[rivy]

Not stale; not addressed as of yet.

[bartlomieju]

@rivy what is the expected solution for this issue?

[dsherret]

Opened #13325

[rivy]

@bartlomieju , the problem is that automatically adding unstable flags when installing potentially breaks future executions with a newer deno which doesn't have that flag. My initial suggestion was...

Likely, a good solution going forward is to never add unstable flags when installing using deno install -A .... Teach deno to add unstable command line flags only when explicitly requested, ie, for access to an unstable API, the dev must use deno install -A --unstable --allow-ffi ....

edit

I see @dsherret just added issue #13325 with a resolution similar to the noted suggestion. I think that will work as a solution.

[bartlomieju]

I believe requirement for --unstable is already in place. I think @dsherret's PR should address the problem of forward compatibility.