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

Jump to bottom

Inconsistent "400 bad request" response, for GET request with a body #4026

Closed
marcopeg opened this issue Aug 12, 2019 · 11 comments
Closed

Inconsistent "400 bad request" response, for GET request with a body #4026

marcopeg opened this issue Aug 12, 2019 · 11 comments
Assignees
@dougwilson

Comments

@marcopeg
Copy link

Hello, I came across a weird behavior when it comes to GET requests:

Here is a GET request performed through the fetch API.
It is wrong because GET should NOT have a body.

await fetch('http://localhost:8080', {
  body: JSON.stringify({})
})

Nevertheless, if my Express handler looks like the following code, it works just fine:

app.get('/', (req, res) => res.send('ok'))

It looks like Express simply ignores the body part of the request.

BUT if I add a middleware that awaits for a Promise, then Express will fail my request with a 400 - Bad Request answer:

app.get('/', [
  (req, res, next) => {
    const foo = new Promise(r => r())
    foo.then(next)
  }
], (req, res) => res.send('ok'))

This piece of code will cause Express to "validate" the request payload and drop the request due to the existing (and correctly unexpected) body.

Now, wouldn't it be better if the behavior of either failing the request or ignoring the body was consistent with or without promises in the handling chain?

Thanks guys!
Awesome work :-)
@dougwilson
Copy link
Contributor

dougwilson commented Aug 12, 2019 •
edited

Interesting, we will investigate. There isn't really anything in Express that would 400 from something like that, and awaiting a promise should not change behavior.

Is there any stack trace printing to your console when this happens? If not, it is likely Node.js HTTP module itself that is sending the 400.

@marcopeg
Copy link
Author

marcopeg commented Aug 12, 2019 via email

@dougwilson
Copy link
Contributor

Unfortunately I tried to create an app based on your description and could not reproduce the 400. Can you provide a reproduction case?

@marcopeg
Copy link
Author

marcopeg commented Aug 12, 2019 via email

@marcopeg
Copy link
Author

Here you go:
https://github.com/marcopeg/express-weird-behavior

This will reproduce the problem using Docker, so it should be quite safe to reproduce it.

Marco

@dougwilson
Copy link
Contributor

dougwilson commented Aug 13, 2019 •
edited

Thank you, that helps a lot! I was able to reproduce it with your example, and it looks like the reason I couldn't reproduce the issue is because it seems specific to the client you are using. Using cURL as a client, for example, shows the 200 OK coming back from the body test where your client gets a 400 Bad Request:

$ curl -v http://127.0.0.1:5555/d -H'content-type: application/json' -d'{}' -XGET
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to 127.0.0.1 (127.0.0.1) port 5555 (#0)
> GET /d HTTP/1.1
> Host: 127.0.0.1:5555
> User-Agent: curl/7.54.0
> Accept: */*
> content-type: application/json
> Content-Length: 2
> 
* upload completely sent off: 2 out of 2 bytes
< HTTP/1.1 200 OK
< X-Powered-By: Express
< Content-Type: text/html; charset=utf-8
< Content-Length: 4
< ETag: W/"4-cM5NkoAgYFvRntn1zvlbeUF4oB8"
< Date: Tue, 13 Aug 2019 13:47:19 GMT
< Connection: keep-alive
< 
* Connection #0 to host 127.0.0.1 left intact
ok/d

The Bad Request is coming from Node.js itself, though, so before Express.js can even see the request. Changing the server to just pure Node.js reproduces the issue without Express, at which case you can file an issue with Node.js or your client library, which ever you think is more appropriate (probably your client library, since cURL has no issue making the same requests that you get a 400 for):

const http = require('http')

const sleepTimeout = duration => (next) => setTimeout(next, duration)
const sleepPromise = duration => (next) => (new Promise(r => setTimeout(r, duration))).then(next)
const sleepSync = duration => (next) => {
    for (let i = 0; i < duration; i++) console.log({ i })
    next()
}

const app = (req, res) => {
    switch (req.url) {
        case '/a':
            res.end('ok/a')
            break
        case '/b':
            sleepSync(9999)(() => res.end('ok/b'))
            break
        case '/c':
            sleepTimeout(500)(() => res.end('ok/c'))
            break
        case '/d':
            sleepPromise(500)(() => res.end('ok/d'))
            break
    }
}

http.createServer(app).listen(8080, () => console.log('App1 is running...'))

@dougwilson dougwilson self-assigned this Aug 13, 2019
@marcopeg
Copy link
Author

marcopeg commented Aug 13, 2019 via email

@raijinsetsu
Copy link

@marcopeg
I bumped into this same issue and then looked at the NodeJS issue tracker. It does not look like you opened an issue over there, nor did Doug. Did you find a work-around for this issue or did someone else submit the issue? I searched through the NodeJS issues but could not find any that seemed to match.

@dougwilson
Copy link
Contributor

Hi @raijinsetsu I did not make an issue with Node.js, as noted above, I believe it was the client library at use that was the issue and not Node.js. I left filing up to OP since they would have the most background and eagerness to work to get it fixed upstream with the client library.

@raijinsetsu
Copy link

raijinsetsu commented Feb 4, 2020 via email

@TBG-FR
Copy link

TBG-FR commented Apr 20, 2022

I missed that detail. I thought you were saying it was the NodeJS server. We happen to be using the NodeJS native Http client which means that, it you're correct, it's still an issue in NodeJS. I'll followup with some testing and open an issue.
…
On Tue, Feb 4, 2020, 10:03 Douglas Wilson @.***> wrote: Hi @raijinsetsu https://github.com/raijinsetsu I did not make an issue with Node.js, as noted above, I believe it was the client library at use that was the issue and not Node.js. I left filing up to OP since they would have the most background and eagerness to work to get it fixed upstream with the client library. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4026?email_source=notifications&email_token=ABT6LNQ7MK5YOYVZKIBARJDRBF7STA5CNFSM4IK7FE42YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKX6BIQ#issuecomment-581951650>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABT6LNQV3RT37ZTSHEH4HJDRBF7STANCNFSM4IK7FE4Q .

Did you open an issue about this ? I'm facing the same behaviour, using native NodeJS too

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

@dougwilson dougwilson

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@dougwilson @marcopeg @raijinsetsu @TBG-FR