add "-o" option to serve command which opens server URL #4144

Merged
merged 1 commit into from Nov 29, 2015

Conversation

Projects
None yet
6 participants
@friedenberg
Contributor

friedenberg commented Nov 15, 2015

Hi Jekyll maintainers,

We were setting up a new Jekyll blog to play around and thought it would be helpful to add a "-o" option to the serve command. This option would open the URL of the server on the device's default browser. We used the "system" ruby method, but we're not sure if that's your preferred approach to opening URL's. It's also unclear if this would work on Windows, as we only tested this on OS X.

We're happy to modify this request to open the URL in a different way, if there are issues with the current implementation. All the unit tests passed when we ran them locally.

cc @Ckearns1210

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Nov 15, 2015

Contributor

This is broken on Linux (and I don't think it will work on Windows) either, open does not do what you think it does on Ubuntu and Debian where it's actually openvt

Contributor

envygeeks commented Nov 15, 2015

This is broken on Linux (and I don't think it will work on Windows) either, open does not do what you think it does on Ubuntu and Debian where it's actually openvt

@friedenberg

This comment has been minimized.

Show comment
Hide comment
@friedenberg

friedenberg Nov 15, 2015

Contributor

aha, that's what worried me. Is there any safe way to do this with multiple platform support, or is this a lost cause?

Contributor

friedenberg commented Nov 15, 2015

aha, that's what worried me. Is there any safe way to do this with multiple platform support, or is this a lost cause?

@friedenberg

This comment has been minimized.

Show comment
Hide comment
@friedenberg

friedenberg Nov 15, 2015

Contributor

would it be crazy to do something like this?
python -m webbrowser #{server_address}

It might not be safe to assume that python is installed.

Contributor

friedenberg commented Nov 15, 2015

would it be crazy to do something like this?
python -m webbrowser #{server_address}

It might not be safe to assume that python is installed.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Nov 15, 2015

Contributor
  • Windows: start url
  • Linux: xdg-open url
  • OS X: As is.
Contributor

envygeeks commented Nov 15, 2015

  • Windows: start url
  • Linux: xdg-open url
  • OS X: As is.
@friedenberg

This comment has been minimized.

Show comment
Hide comment
@friedenberg

friedenberg Nov 15, 2015

Contributor

alright, updated to include different OS commands. (note to self: create PR's off of fork branches and not fork master)

Contributor

friedenberg commented Nov 15, 2015

alright, updated to include different OS commands. (note to self: create PR's off of fork branches and not fork master)

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Nov 18, 2015

Contributor

/cc @jekyll/core

Contributor

envygeeks commented Nov 18, 2015

/cc @jekyll/core

@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Nov 18, 2015

Member

👍

I know in our Rakefile, we use Launchy to open the built Jekyll site for the preview task.

Member

alfredxing commented Nov 18, 2015

👍

I know in our Rakefile, we use Launchy to open the built Jekyll site for the preview task.

lib/jekyll/commands/serve.rb
@@ -45,7 +46,24 @@ def process(options)
file_handler_options
)
- Jekyll.logger.info "Server address:", server_address(s, options)
+ server_address = server_address(s, options)

This comment has been minimized.

@parkr

parkr Nov 18, 2015

Member

this reassignment of server_address is confusing – could you rename the variable on the left here, please?

@parkr

parkr Nov 18, 2015

Member

this reassignment of server_address is confusing – could you rename the variable on the left here, please?

@friedenberg

This comment has been minimized.

Show comment
Hide comment
@friedenberg

friedenberg Nov 28, 2015

Contributor

ok, fixed the naming of the variable. I'll keep in mind Launchy for stuff like this in the future, too.

Contributor

friedenberg commented Nov 28, 2015

ok, fixed the naming of the variable. I'll keep in mind Launchy for stuff like this in the future, too.

parkr added a commit that referenced this pull request Nov 29, 2015

@parkr parkr merged commit 64e87bf into jekyll:master Nov 29, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

parkr added a commit that referenced this pull request Nov 29, 2015

@osfameron

This comment has been minimized.

Show comment
Hide comment
@osfameron

osfameron Nov 29, 2015

There's a possible injection attack here - server_address seems to pass through baseurl: if that has shell characters in it then this could be disastrous. Would it be better to do something like:

system( command_name, server_address_str) if command_name

instead?

There's a possible injection attack here - server_address seems to pass through baseurl: if that has shell characters in it then this could be disastrous. Would it be better to do something like:

system( command_name, server_address_str) if command_name

instead?

This comment has been minimized.

Show comment
Hide comment
@friedenberg

friedenberg Nov 29, 2015

Owner

That's a fair point. The alternative would be to raise an exception in an else clause that would bypass the system command.

In my opinion, raising an exception would be a clearer indication of this bad state.

Owner

friedenberg replied Nov 29, 2015

That's a fair point. The alternative would be to raise an exception in an else clause that would bypass the system command.

In my opinion, raising an exception would be a clearer indication of this bad state.

This comment has been minimized.

Show comment
Hide comment
@friedenberg

friedenberg Nov 29, 2015

Owner

But, it would also be appropriate to add shell-escaping to server_address_url

Owner

friedenberg replied Nov 29, 2015

But, it would also be appropriate to add shell-escaping to server_address_url

This comment has been minimized.

Show comment
Hide comment
@osfameron

osfameron Nov 29, 2015

Calling system with multiple arguments should completely bypass the shell I think? (In which case it's even better than shell-escaping, arguably.)

Calling system with multiple arguments should completely bypass the shell I think? (In which case it's even better than shell-escaping, arguably.)

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

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