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

Add 'allowedHosts' option #899

Merged
merged 4 commits into from
Jun 14, 2017
Merged

Conversation

orteth01
Copy link
Contributor

@orteth01 orteth01 commented May 1, 2017

What kind of change does this PR introduce?
feature/bugfix

Did you add or update the examples/?
no

Summary
Host checking was added by default in v2.4.3 for security reasons. Users now have to specify the host/URL being used to access the dev-server more info here

This PR allows users to specify multiple hosts that that will pass the check. Recommended by @edmorley here: #882 (comment)

Does this PR introduce a breaking change?
no

Example usage in webpack config:

devServer: {
    allowedHosts: [
        'exampleHost.com',
        'differentExampleHost.com',
        ...
    ]
}

subdomain wildcard:

mimicking django's ALLOWED_HOSTS, a value beginning with "." can be used as a subdomain wildcard. '.example.com' will match example.com, www.example.com, and any other subdomain of example.com.

devServer: {
    allowedHosts: [
        '.example.com'
    ]
}

@codecov
Copy link

codecov bot commented May 1, 2017

Codecov Report

Merging #899 into master will increase coverage by 0.93%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #899      +/-   ##
==========================================
+ Coverage    71.3%   72.23%   +0.93%     
==========================================
  Files           4        4              
  Lines         453      461       +8     
  Branches      133      138       +5     
==========================================
+ Hits          323      333      +10     
+ Misses        130      128       -2
Impacted Files Coverage Δ
lib/Server.js 79.69% <100%> (+0.51%) ⬆️
lib/OptionsValidationError.js 60.86% <0%> (+1.73%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a26ab4...0085961. Read the comment docs.

@mgol
Copy link

mgol commented May 2, 2017

Django docs say:

A value beginning with a period can be used as a subdomain wildcard: '.example.com' will match example.com, www.example.com, and any other subdomain of example.com.

Could this be added as well?

@orteth01
Copy link
Contributor Author

orteth01 commented May 2, 2017

@mgol I didn't realize that there were already a few tests for Server.prototype.checkHost
https://github.com/webpack/webpack-dev-server/blob/master/test/Validation.test.js#L65-L118

I can either add tests for allowedHosts there or I can remove those tests because the tests that I added cover those cases. Which do you prefer?

@mgol
Copy link

mgol commented May 3, 2017

@orteth01 I guess it makes sense to nest that under existing tests (a separate inner describe sounds good).

Note, though, that I'm just a user, not a member of Webpack's core team so you might want to wait for someone on the team to decide. I'm just providing my own feedback.

@orteth01
Copy link
Contributor Author

orteth01 commented May 3, 2017

I guess it makes sense to nest that under existing tests (a separate inner describe sounds good).

Yea that makes more sense, I think. I've gone with that option. thanks, @mgol

@orteth01 orteth01 force-pushed the allowed-hosts branch 3 times, most recently from 2658f96 to 5f3924a Compare May 3, 2017 15:14
@orteth01
Copy link
Contributor Author

orteth01 commented May 5, 2017

There seem to be a few open issues that this PR would remedy. Any thoughts, @SpaceK33z ??

@ssilve1989
Copy link

What's the status on this?

@orteth01
Copy link
Contributor Author

What's the status on this?

@ssilve1989 waiting on review from one of Webpack's core team members

@prencher
Copy link

prencher commented May 26, 2017

@SpaceK33z @sokra I hate to be that guy, but this is preventing us from moving to a more secure setup -- The entire point of the change that prompted this PR. Can we please get movement?

I am happy to assist if I can.

@Justus-Maier
Copy link

Can I put an IP address with wildcards in here too? I sometimes get new IP from DHCP and I would like to share my current work with my collegues (just open a browser). Can this be done without me registering a local private domain?

@orteth01
Copy link
Contributor Author

orteth01 commented Jun 7, 2017

Can I put an IP address with wildcards in here too?

Currently, you can add an ip address as an allowed host but not with wildcards.

Only the subdomain wildcard is supported, meaning that only the first part of your allowed host can be dynamic. In theory I guess you could have something like this

devServer: {
    allowedHosts: [
        '.123.123'
    ]
}

which would allow any IP's that take the form *.*.123.123. However, I think that would not be recommended

@mgol
Copy link

mgol commented Jun 7, 2017

@orteth01 For IPs I'd expect a wildcard going in a different direction, e.g. 192.168..

@orteth01
Copy link
Contributor Author

orteth01 commented Jun 7, 2017

For IPs I'd expect a wildcard going in a different direction, e.g. 192.168..

@mgol Would adding this wildcard be secure? I can certainly add support for a wildcard at the end of a specified host I just want to make sure that it is safe to do so.

@edmorley
Copy link

edmorley commented Jun 7, 2017

As I mentioned in some of the other threads, it should be safe to whitelist all IP-addresses-like hosts names by default, which would save people having to whitelist them manually.

@orteth01
Copy link
Contributor Author

orteth01 commented Jun 7, 2017

it should be safe to whitelist all IP-addresses-like hosts names by default

@edmorley it seems like a separate PR would be appropriate for that. I'll create an issue.

@shellscape shellscape merged commit ab889c3 into webpack:master Jun 14, 2017
@shellscape
Copy link
Contributor

@orteth01 merged the PR, as its a solid feature and has been in waiting for quite some time, but we still need example/readme updates for this. we've got a little bit of time before we release, would be great if you would be able to add that.

@orteth01
Copy link
Contributor Author

we still need example/readme updates for this

@shellscape I'm more than happy to help with documentation! I'm assuming that I should update this readme. Are there any other places where I should add information regarding this feature?

@shellscape
Copy link
Contributor

@orteth01 awesome. yes, please do update that readme. It's also suggested to create an example directory to demo the option.

@Justus-Maier
Copy link

Justus-Maier commented Jun 19, 2017

@orteth01 please also add some documentation for the disabling flag of the allowedHosts feature:
config.devServer.disableHostCheck = true
With security warnings obviously ;D
#887

@Mario-Eis
Copy link

Can this be set via command line?

@orteth01
Copy link
Contributor Author

orteth01 commented Jul 5, 2017

Can this be set via command line?

@Mario-Eis not currently. i'll try to get a PR open by the end of the week.

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.

None yet

8 participants