fix(server): fallthrough non `GET` and `HEAD` request to routes defined in after option #2374

jvasseur · 2019-12-26T13:26:28Z

For Bugs and Features; did you add new tests?

Yes

Motivation / Use-Case

Before #2150, non-GET request where forwarded to any middleware defined in the after option, since #2150, they are handled by serve-index instead that always reply with a 405 error.

This fix issue #2370.

Breaking Changes

No

Additional Info

Closes #2370

jsf-clabot · 2019-12-26T13:26:30Z

All committers have signed the CLA.

codecov · 2019-12-26T13:32:44Z

Codecov Report

Merging #2374 into master will decrease coverage by 0.2%.
The diff coverage is 62.5%.

@@            Coverage Diff             @@
##           master    #2374      +/-   ##
==========================================
- Coverage   93.95%   93.75%   -0.21%     
==========================================
  Files          34       34              
  Lines        1291     1297       +6     
  Branches      368      370       +2     
==========================================
+ Hits         1213     1216       +3     
- Misses         77       79       +2     
- Partials        1        2       +1

Impacted Files	Coverage Δ
lib/Server.js	`96.71% <62.5%> (-0.63%)`	⬇️

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 f4c8f94...5b86099. Read the comment docs.

alexander-akait · 2019-12-26T13:34:47Z

lib/Server.js

@@ -390,13 +390,25 @@ class Server {

    if (Array.isArray(contentBase)) {
      contentBase.forEach((item) => {
-        this.app.use(contentBasePublicPath, serveIndex(item));
+        this.app.use(contentBasePublicPath, (req, res, next) => {
+          if (req.method !== 'GET') {


Not sure it is good solution, developers can want to handle GET in own middleware too

AFAIK, serve-index let GET request that it can't handle fall-back to the next middleware, it's just non-GET request that are blindly replied as 405.

This is basically a revert of the change from app.get to app.use done in #2150 while still keeping the contentBasePublicPath feature.

Suggested change

if (req.method !== 'GET') {

if (req.method !== 'GET' && req.method !== 'HEAD') {

alexander-akait

But based on previous implementation, we handle only GET, so i think we can merge this, we need fix it for server-static too https://github.com/webpack/webpack-dev-server/pull/2150/files#diff-15fb51940da53816af13330d8ce69b4eR341

/cc @iamandrewluca

jvasseur · 2019-12-26T13:43:47Z

@evilebottnawi the serve-static doesn't exhibit this problem because it already forward non-GET request to the next middleware by default : https://github.com/expressjs/serve-static/blob/master/index.js#L73-L76

iamandrewluca · 2019-12-26T13:45:16Z

I'm taking a look into serve-index implementation

lib/Server.js

iamandrewluca · 2019-12-26T14:07:54Z

serve-static has fallthrough, but we don't use it in our case, it must be added.
serve-index does not have fallthrough, and I agree with the workaround in this PR.

@jvasseur also make sure you check also for HEAD method

if (req.method !== 'GET' && req.method !== 'HEAD') {
// ...

Also I think serveIndex needs to be returned

return serveIndex(item)(req, res, next);

jvasseur · 2019-12-26T14:10:04Z

@iamandrewluca true is already the default value for the fallthrough parameter of serve-static.

I've added the check for HEAD in the code.

iamandrewluca · 2019-12-26T14:12:57Z

recommend to rename PR into

fix(server): fallthrough non `GET` and `HEAD` request to routes defined in after option

alexander-akait

Good job, thanks!

/cc @hiroppy @Loonride need review

iamandrewluca · 2019-12-26T14:53:14Z

@jvasseur pls add a comment in code before if, that serve-index does not fallthrough like serve-static

Fix a bug introduced in cee700d where the serveIndex feature where always replying instead of forwarding requests to the next middleware.

jvasseur · 2019-12-26T15:55:51Z

@iamandrewluca done

iamandrewluca · 2019-12-26T18:50:17Z

@jvasseur add at the end of PR description this. It will automatically close the issue when PR is merged.

Closes #2370

jvasseur · 2019-12-26T19:54:50Z

Done

wizardofhogwarts

lgtm

hiroppy · 2019-12-29T03:47:58Z

Thanks!

iamandrewluca · 2020-01-31T15:43:29Z

Released in https://github.com/webpack/webpack-dev-server/releases/tag/v3.10.2

jvasseur requested review from alexander-akait, hiroppy and knagaitsev as code owners December 26, 2019 13:26

alexander-akait reviewed Dec 26, 2019

View reviewed changes

iamandrewluca reviewed Dec 26, 2019

View reviewed changes

lib/Server.js Outdated Show resolved Hide resolved

jvasseur force-pushed the fix-serve-index-post branch from 0bf6e29 to 02320d3 Compare December 26, 2019 14:05

iamandrewluca previously approved these changes Dec 26, 2019

View reviewed changes

alexander-akait previously approved these changes Dec 26, 2019

View reviewed changes

jvasseur changed the title ~~fix(server): fix POST request forward to routes defined in after option~~ fix(server): fallthrough non GET and HEAD request to routes defined in after option Dec 26, 2019

fix(server): fix POST request forward to routes defined in after option

5b86099

Fix a bug introduced in cee700d where the serveIndex feature where always replying instead of forwarding requests to the next middleware.

jvasseur dismissed stale reviews from alexander-akait and iamandrewluca via 5b86099 December 26, 2019 15:55

jvasseur force-pushed the fix-serve-index-post branch from 02320d3 to 5b86099 Compare December 26, 2019 15:55

alexander-akait approved these changes Dec 27, 2019

View reviewed changes

rishabh3112 approved these changes Dec 27, 2019

View reviewed changes

iamandrewluca approved these changes Dec 27, 2019

View reviewed changes

jamesgeorge007 approved these changes Dec 28, 2019

View reviewed changes

anshumanv approved these changes Dec 28, 2019

View reviewed changes

wizardofhogwarts approved these changes Dec 28, 2019

View reviewed changes

hiroppy approved these changes Dec 29, 2019

View reviewed changes

hiroppy merged commit ebe8eca into webpack:master Dec 29, 2019

jvasseur deleted the fix-serve-index-post branch December 29, 2019 10:47

logicool mentioned this pull request Jan 2, 2020

xhr.js:160 POST http://localhost:9527/dev-api/user/login 405 (Method Not Allowed) PanJiaChen/vue-element-admin#2897

Closed

jvasseur mentioned this pull request Jan 9, 2020

Lock WDS version in package.json thetribeio/node-react-starter-kit#293

Merged

PanJiaChen mentioned this pull request Jan 10, 2020

#2929 fixed mock server 出现新问题 PanJiaChen/vue-element-admin#2934

Open

alexander-akait mentioned this pull request Jan 15, 2020

devserver app request post url get 405 #2392

Closed

2 tasks

iamandrewluca mentioned this pull request Jan 31, 2020

feat(react-scripts): allow PUBLIC_URL in develoment mode facebook/create-react-app#7259

Merged

14 tasks

This was referenced Feb 22, 2020

[Snyk] Upgrade webpack-dev-server from 3.2.1 to 3.10.2 saurabharch/react-ecommerce#5

Open

[Snyk] Upgrade webpack-dev-server from 3.1.14 to 3.10.2 raindigi/eShopOnContainers#2

Open

snyk-bot mentioned this pull request Mar 5, 2020

[Snyk] Upgrade webpack-dev-server from 3.1.10 to 3.10.3 sarvaniupadrasta/learnReact#2

Open

vpetersson mentioned this pull request Mar 6, 2020

[Snyk] Upgrade webpack-dev-server from 3.1.14 to 3.10.3 WoTTsecurity/api#757

Closed

snyk-bot mentioned this pull request Mar 21, 2020

[Snyk] Upgrade webpack-dev-server from 2.9.4 to 3.10.3 lwojcik/sc2pte-panel-frontend#85

Closed

This was referenced Apr 4, 2020

[Snyk] Upgrade webpack-dev-server from 3.7.2 to 3.10.3 CollageLabs/collagelabs.org#60

Merged

[Snyk] Upgrade webpack-dev-server from 3.9.0 to 3.10.3 cds-snc/webpack-starter#8

Open

This was referenced May 5, 2020

[Snyk] Upgrade webpack-dev-server from 3.1.8 to 3.10.3 Barnstorm-Online/barnstorm-cli#6

Open

[Snyk] Upgrade webpack-dev-server from 3.1.14 to 3.10.3 hzhz/material-ui#5

Open

This was referenced Mar 12, 2021

Bump webpack-dev-server from 3.1.10 to 3.11.2 danbruder/ionic-4-elm#7

Merged

Bump webpack-dev-server from 3.1.4 to 3.11.2 in /frontend Quartech/QSLContacts#81

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(server): fallthrough non `GET` and `HEAD` request to routes defined in after option #2374

fix(server): fallthrough non `GET` and `HEAD` request to routes defined in after option #2374

jvasseur commented Dec 26, 2019 •

edited

jsf-clabot commented Dec 26, 2019 •

edited

codecov bot commented Dec 26, 2019 •

edited

alexander-akait Dec 26, 2019

jvasseur Dec 26, 2019

iamandrewluca Dec 26, 2019

alexander-akait left a comment

jvasseur commented Dec 26, 2019 •

edited

iamandrewluca commented Dec 26, 2019

iamandrewluca commented Dec 26, 2019 •

edited

jvasseur commented Dec 26, 2019

iamandrewluca commented Dec 26, 2019 •

edited

alexander-akait left a comment

iamandrewluca commented Dec 26, 2019 •

edited

jvasseur commented Dec 26, 2019

iamandrewluca commented Dec 26, 2019

jvasseur commented Dec 26, 2019

wizardofhogwarts left a comment

hiroppy commented Dec 29, 2019

iamandrewluca commented Jan 31, 2020

	if (req.method !== 'GET') {
	if (req.method !== 'GET' && req.method !== 'HEAD') {

fix(server): fallthrough non GET and HEAD request to routes defined in after option #2374

fix(server): fallthrough non GET and HEAD request to routes defined in after option #2374

Conversation

jvasseur commented Dec 26, 2019 • edited

For Bugs and Features; did you add new tests?

Motivation / Use-Case

Breaking Changes

Additional Info

jsf-clabot commented Dec 26, 2019 • edited

codecov bot commented Dec 26, 2019 • edited

Codecov Report

alexander-akait Dec 26, 2019

Choose a reason for hiding this comment

jvasseur Dec 26, 2019

Choose a reason for hiding this comment

iamandrewluca Dec 26, 2019

Choose a reason for hiding this comment

alexander-akait left a comment

Choose a reason for hiding this comment

jvasseur commented Dec 26, 2019 • edited

iamandrewluca commented Dec 26, 2019

iamandrewluca commented Dec 26, 2019 • edited

jvasseur commented Dec 26, 2019

iamandrewluca commented Dec 26, 2019 • edited

alexander-akait left a comment

Choose a reason for hiding this comment

iamandrewluca commented Dec 26, 2019 • edited

jvasseur commented Dec 26, 2019

iamandrewluca commented Dec 26, 2019

jvasseur commented Dec 26, 2019

wizardofhogwarts left a comment

Choose a reason for hiding this comment

hiroppy commented Dec 29, 2019

iamandrewluca commented Jan 31, 2020

fix(server): fallthrough non `GET` and `HEAD` request to routes defined in after option #2374

fix(server): fallthrough non `GET` and `HEAD` request to routes defined in after option #2374

jvasseur commented Dec 26, 2019 •

edited

jsf-clabot commented Dec 26, 2019 •

edited

codecov bot commented Dec 26, 2019 •

edited

jvasseur commented Dec 26, 2019 •

edited

iamandrewluca commented Dec 26, 2019 •

edited

iamandrewluca commented Dec 26, 2019 •

edited

iamandrewluca commented Dec 26, 2019 •

edited