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

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

This is a bugfix
This is a feature
This is a code refactor
This is a test update
This is a docs update
This is a metadata update

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

 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

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

I'm taking a look into serve-index implementation

lib/Server.js

unrevised6419 · 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.

unrevised6419 · 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

unrevised6419 · 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

unrevised6419 · 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!

unrevised6419 · 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

unrevised6419 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

unrevised6419 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 unrevised6419 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

unrevised6419 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

unrevised6419 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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

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

Uh oh!

jvasseur commented Dec 26, 2019 •

edited

Loading

jsf-clabot commented Dec 26, 2019 •

edited

Loading

codecov bot commented Dec 26, 2019 •

edited

Loading

alexander-akait Dec 26, 2019

jvasseur Dec 26, 2019

unrevised6419 Dec 26, 2019

alexander-akait left a comment

jvasseur commented Dec 26, 2019 •

edited

Loading

unrevised6419 commented Dec 26, 2019

Uh oh!

unrevised6419 commented Dec 26, 2019 •

edited

Loading

jvasseur commented Dec 26, 2019

unrevised6419 commented Dec 26, 2019 •

edited

Loading

alexander-akait left a comment

unrevised6419 commented Dec 26, 2019 •

edited

Loading

jvasseur commented Dec 26, 2019

unrevised6419 commented Dec 26, 2019

jvasseur commented Dec 26, 2019

wizardofhogwarts left a comment

hiroppy commented Dec 29, 2019

unrevised6419 commented Jan 31, 2020

Labels

9 participants

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

Uh oh!

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

Uh oh!

Conversation

jvasseur commented Dec 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

For Bugs and Features; did you add new tests?

Motivation / Use-Case

Breaking Changes

Additional Info

jsf-clabot commented Dec 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

codecov bot commented Dec 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

unrevised6419 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 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

unrevised6419 commented Dec 26, 2019

Uh oh!

unrevised6419 commented Dec 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

jvasseur commented Dec 26, 2019

unrevised6419 commented Dec 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

alexander-akait left a comment

Choose a reason for hiding this comment

unrevised6419 commented Dec 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

jvasseur commented Dec 26, 2019

unrevised6419 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

unrevised6419 commented Jan 31, 2020

Labels

9 participants

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

Loading

jsf-clabot commented Dec 26, 2019 •

edited

Loading

codecov bot commented Dec 26, 2019 •

edited

Loading

jvasseur commented Dec 26, 2019 •

edited

Loading

unrevised6419 commented Dec 26, 2019 •

edited

Loading

unrevised6419 commented Dec 26, 2019 •

edited

Loading

unrevised6419 commented Dec 26, 2019 •

edited

Loading