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

[5.0] Adjust ChromeDriver command to beta releases #644

Merged
merged 1 commit into from
May 1, 2019

Conversation

staudenmeir
Copy link
Contributor

@staudenmeir staudenmeir commented Apr 30, 2019

The first ChromeDriver beta version has been released and the ChromeDriver command would now download it by default. That's not really useful, as it doesn't work with the stable version of Chrome.

As suspected in #643 (comment), the index page doesn't allow us to differentiate between stable and beta releases. We can get the latest stable version by parsing http://chromedriver.chromium.org/home (which hopefully keeps the current structure).

The command hasn't been released yet, so this is not a breaking change.

@browner12
Copy link
Contributor

is there no HTTPS link we can download from?

@staudenmeir
Copy link
Contributor Author

staudenmeir commented Apr 30, 2019

The download URL itself has HTTPS: https://github.com/laravel/dusk/blob/5.0/src/Console/ChromeDriverCommand.php#L47

We are only using the HTTP URL to get the latest version number. The regular expression limits it to digits and dots, so I think this should be secure.

Do you see a possible MITM vulnerability?

@browner12
Copy link
Contributor

not necessarily, just more of an "always use HTTPS to be safe" comment.

@staudenmeir
Copy link
Contributor Author

Unfortunately, https://chromedriver.chromium.org/home doesn't work.

@driesvints
Copy link
Member

@staudenmeir could we maybe use Guzzle instead of the file_get_contents call? Wouldn't that be better/safer?

@staudenmeir
Copy link
Contributor Author

@driesvints I don't think there's a difference in security. I chose file_get_contents() because it doesn't require an additional dependency.

@Wiejeben
Copy link

Wiejeben commented Apr 30, 2019

I believe the latest stable release version can also be found at https://chromedriver.storage.googleapis.com/LATEST_RELEASE (see https://chromedriver.storage.googleapis.com/index.html).

Well, at least this version matches the latest stable version from http://chromedriver.chromium.org/home.

@staudenmeir
Copy link
Contributor Author

@Wiejeben Last week, the LATEST_RELEASE file still contained an outdated ChromeDriver version.

From http://chromedriver.chromium.org/downloads/version-selection:

Please don't rely on the LATEST_RELEASE file without a version suffix. It exists for backward compatibility only, and will be removed in the near future.

@peterfox
Copy link

peterfox commented May 1, 2019

Slight tangent I've been thinking would it not be better to build in a possible detection mechanism for which chrome version you have installed anyways? E.g. for Mac, we can expect the default location to be /Applications/Google\ Chrome.app/Contents/MacOS/Google\ Chrome and if you execute it with the --version flag it will spit out something like Google Chrome 73.0.3683.103. We could then parse that to grab the correct version as we probably shouldn't assume that the latest version is installed anyways. I've found updates to hit some of my devices quicker than other. Is it worth looking into or would it be considered further over kill? I think eventually more people will get hit by version compatibility problems.

@taylorotwell taylorotwell merged commit 393d984 into laravel:5.0 May 1, 2019
@staudenmeir staudenmeir deleted the chromedriver branch May 1, 2019 13:52
@staudenmeir
Copy link
Contributor Author

@peterfox I've been experimenting with that a while ago, it's definitely feasible.

I wouldn't make it the default behavior, but we could add an option like --auto or --detect.

@drbyte
Copy link

drbyte commented May 1, 2019

@peterfox I was thinking similarly for CI environments too, where their image installs a version we don't control. It'd be nice to just-use-it, or if dusk thinks it's too outdated "then" it can intelligently grab a compatible one.

@peterfox
Copy link

peterfox commented May 1, 2019

@staudenmeir I agree and was also thinking a --detect flag, could also make it an optional value flag to pass in a path to the chrome binary if they're using a custom path.

@staudenmeir
Copy link
Contributor Author

@peterfox The optional path is a great idea.

@staudenmeir
Copy link
Contributor Author

I've added the Chrome version detection to my updater package: staudenmeir/dusk-updater

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.

7 participants