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 atom:// urls to be opened from the command line #15683

Merged
merged 1 commit into from Sep 19, 2017

Conversation

Projects
None yet
4 participants
@dreiss
Contributor

dreiss commented Sep 18, 2017

Requirements

  • All new code requires tests to ensure against regressions

I would love some guidance on how to write an automated test for this.

Description of the Change

Some Atom extensions (like Nuclide) have functionality that can only be
triggered by open-url events, which only work on Mac OS. With this
change, atom:// URLs can be passed on the command-line, and they will
opened with the normal openUrl method on all platforms.

This change doesn't set Atom up as the default handler for atom:// urls.
That will require some platform-specific changes.

Alternate Designs

I considered making a separate option for passing urls (--open-url), but this seems more convenient.

Why Should This Be In Core?

Very simple change. Brings the "open-url" interface to all platforms.

Benefits

Windows and Linux users can easily simulate "open-url" events.

Possible Drawbacks

Can't use the atom command line to open a file whose name starts with atom://, though you can still use ./atom://foo or just atom:/foo.

Allow atom:// urls to be opened from the command line
Some Atom extensions (like Nuclide) have functionality that can only be
triggered by open-url events, which only work on Mac OS.  With this
change, `atom://` URLs can be passed on the command-line, and they will
opened with the normal openUrl method on all platforms.

This change doesn't set Atom up as the default handler for atom:// urls.
That will require some platform-specific changes.
@hansonw

This comment has been minimized.

Show comment
Hide comment
@hansonw

hansonw Sep 18, 2017

Contributor

Maybe @BinaryMuse has some thoughts here? I'm not sure if there's existing use cases out there for opening atom:// paths on the command line.

Contributor

hansonw commented Sep 18, 2017

Maybe @BinaryMuse has some thoughts here? I'm not sure if there's existing use cases out there for opening atom:// paths on the command line.

@BinaryMuse

This comment has been minimized.

Show comment
Hide comment
@BinaryMuse

BinaryMuse Sep 18, 2017

Member

Thanks for the PR! Phew, it's been a really long time since I was in Atom's main process code, but here are some thoughts:

First, are you doing something special with open-url events in Nuclide? As far as I can tell, URLs parsed from the command line are just sent to AtomApplication#openUrl, which looks for a matching package with a urlMain field and opens that URL. In an ideal world, we could deprecate this behavior and take over the atom:// protocol for a more extensible system like #11399.

Second, there have been some security concerns raised from our internal appsec team around parsing URLs passed to Atom — but perhaps it's only a problem when Atom can be launched from the browser (e.g. when it's registered as a protocol handler).

That said, if this already works on macOS, I can't see why we wouldn't bring this into parity on other platforms. Pinging @atom/feedback in hopes that someone with more experience in the main process than I chimes in 😅

Member

BinaryMuse commented Sep 18, 2017

Thanks for the PR! Phew, it's been a really long time since I was in Atom's main process code, but here are some thoughts:

First, are you doing something special with open-url events in Nuclide? As far as I can tell, URLs parsed from the command line are just sent to AtomApplication#openUrl, which looks for a matching package with a urlMain field and opens that URL. In an ideal world, we could deprecate this behavior and take over the atom:// protocol for a more extensible system like #11399.

Second, there have been some security concerns raised from our internal appsec team around parsing URLs passed to Atom — but perhaps it's only a problem when Atom can be launched from the browser (e.g. when it's registered as a protocol handler).

That said, if this already works on macOS, I can't see why we wouldn't bring this into parity on other platforms. Pinging @atom/feedback in hopes that someone with more experience in the main process than I chimes in 😅

@dreiss

This comment has been minimized.

Show comment
Hide comment
@dreiss

dreiss Sep 18, 2017

Contributor

First, are you doing something special with open-url events in Nuclide? As far as I can tell, URLs parsed from the command line are just sent to AtomApplication#openUrl, which looks for a matching package with a urlMain field and opens that URL.

Yes. I don't know exactly how it works, but on Mac clicking an atom:// link can tell Nuclide to open a specific file (in a repository-aware way) or trigger interactions with other tools. (Like, I get an email with a link, and I can click it to auto-clone my co-worker's work-in-progress code.)

In an ideal world, we could deprecate this behavior and take over the atom:// protocol for a more extensible system like #11399.

Yep. I think that's compatible with what Nuclide is doing now, though Hanson would know better. This PR should be orthogonal to that change. It just allows non-Mac users to send these URLs to Atom, regardless of how they will be dispatched once they get there.

security concerns

On my desktop environment (MATE with Firefox and Chrome), opening one of these links from the browser requires a confirmation. If there are more specific concerns, though, I'm happy to address them.

Contributor

dreiss commented Sep 18, 2017

First, are you doing something special with open-url events in Nuclide? As far as I can tell, URLs parsed from the command line are just sent to AtomApplication#openUrl, which looks for a matching package with a urlMain field and opens that URL.

Yes. I don't know exactly how it works, but on Mac clicking an atom:// link can tell Nuclide to open a specific file (in a repository-aware way) or trigger interactions with other tools. (Like, I get an email with a link, and I can click it to auto-clone my co-worker's work-in-progress code.)

In an ideal world, we could deprecate this behavior and take over the atom:// protocol for a more extensible system like #11399.

Yep. I think that's compatible with what Nuclide is doing now, though Hanson would know better. This PR should be orthogonal to that change. It just allows non-Mac users to send these URLs to Atom, regardless of how they will be dispatched once they get there.

security concerns

On my desktop environment (MATE with Firefox and Chrome), opening one of these links from the browser requires a confirmation. If there are more specific concerns, though, I'm happy to address them.

@hansonw

This comment has been minimized.

Show comment
Hide comment
@hansonw

hansonw Sep 19, 2017

Contributor

@BinaryMuse we ended up implementing a really scrappy url-main script to pass the URLs back to an open Atom window, including a lot of hackery to start a new Atom window if there isn't one already open (https://github.com/facebook/nuclide/blob/master/lib/url-main.js).

Would love to be able to use #11399! I think one thing to note with the proposed API is that it may send the URL message before all packages have loaded, so URLs opening in new windows may not be handle-able by user packages (unless I misunderstood something).

Contributor

hansonw commented Sep 19, 2017

@BinaryMuse we ended up implementing a really scrappy url-main script to pass the URLs back to an open Atom window, including a lot of hackery to start a new Atom window if there isn't one already open (https://github.com/facebook/nuclide/blob/master/lib/url-main.js).

Would love to be able to use #11399! I think one thing to note with the proposed API is that it may send the URL message before all packages have loaded, so URLs opening in new windows may not be handle-able by user packages (unless I misunderstood something).

@BinaryMuse BinaryMuse requested a review from nathansobo Sep 19, 2017

@BinaryMuse

This comment has been minimized.

Show comment
Hide comment
@BinaryMuse

BinaryMuse Sep 19, 2017

Member

Ah yeah I see Nuclide is using urlMain right now. I think this is fine but would be more comfortable with some 👀 first

Member

BinaryMuse commented Sep 19, 2017

Ah yeah I see Nuclide is using urlMain right now. I think this is fine but would be more comfortable with some 👀 first

@nathansobo

This looks harmless enough to me.

@BinaryMuse BinaryMuse merged commit 0d1be47 into atom:master Sep 19, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@BinaryMuse

This comment has been minimized.

Show comment
Hide comment
@BinaryMuse

BinaryMuse Sep 19, 2017

Member

Just a note that when we start thinking about default protocol handlers (e.g. in #11399) this may have to change due to security vulnerabilities in Windows and the extremely primitive way it handles protocol handling apps.

Member

BinaryMuse commented Sep 19, 2017

Just a note that when we start thinking about default protocol handlers (e.g. in #11399) this may have to change due to security vulnerabilities in Windows and the extremely primitive way it handles protocol handling apps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment