search

expressjs / body-parser

Node.js body parsing middleware
MIT License
5.44k stars 728 forks source link

'Misspelled' booleans as value returns HTML body with my directory path - Throws no error. #539

Closed Cinnamals closed 4 weeks ago

Cinnamals commented 1 month ago

Hi! I have a question about the way the body-parser reacts when passing a "misspelled" boolean as a value.
I am importing the express JSON-body parser, which gives me an unexpected behaviour in this specific situation (code sample to run is pasted below). The logs tells me it is the JSON body parser failing because of an unexpected token, which is absolutely fair.

However I am not expecting an HTML body back as a response, which happens here in my case.

Say one were to use this curl command:

curl --header "Content-Type: application/json" \ 
  --request PATCH \
  --data '{"read": false}' \
  http://localhost:5000/messages/66e

Then our response looks great! 

 {"read":false}

The false value here is not encapsulated with quotation marks and is being interpeted correctly. However if we introduce a spelling error, 'false' => 'falsre', I get a HTML body back and no error is actually thrown when the parser fails due to an unexpected token.

So let's do the misspelled boolean value:

 curl --header "Content-Type: application/json" \
  --request PATCH \
  --data '{"read": falsre}' \
  http://localhost:5000/messages/66e

Then the response is:

<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8">
<title>Error</title>
</head>
SyntaxError: Unexpected token &#39;r&#39;, &quot;{&quot;read&quot;: falsre}&quot; is not valid JSON<br> &nbsp; &nbsp;at JSON.parse (&lt;anonymous&gt;)<br> &nbsp; &nbsp;at parse (/home/user/maybe-bug/node_modules/body-parser/lib/types/json.js:92:19)<br> &nbsp; &nbsp;at /home/user/maybe-bug/node_modules/body-parser/lib/read.js:128:18<br> &nbsp; &nbsp;at AsyncResource.runInAsyncScope (node:async_hooks:206:9)<br> &nbsp; &nbsp;at invokeCallback (/home/user/maybe-bug/node_modules/raw-body/index.js:238:16)<br> &nbsp; &nbsp;at done (/home/user/maybe-bug/node_modules/raw-body/index.js:227:7)<br> &nbsp; &nbsp;at IncomingMessage.onEnd (/home/user/maybe-bug/node_modules/raw-body/index.js:287:7)<br> &nbsp; &nbsp;at IncomingMessage.emit (node:events:518:28)<br>
 &nbsp; &nbsp;at endReadableNT (node:internal/streams/readable:1696:12)<br> &nbsp; &nbsp;at process.processTicksAndRejections (node:internal/process/task_queues:82:21)</pre>

The console then logs this error:

SyntaxError: Unexpected token 'r', "{"read": falsre}" is not valid JSON
    at JSON.parse (<anonymous>)
    at parse (/home/user/maybe-bug/node_modules/body-parser/lib/types/json.js:92:19)
    at /home/user/maybe-bug/node_modules/body-parser/lib/read.js:128:18
    at AsyncResource.runInAsyncScope (node:async_hooks:206:9)
    at invokeCallback (/home/user/maybe-bug/node_modules/raw-body/index.js:238:16)
    at done (/home/user/maybe-bug/node_modules/raw-body/index.js:227:7)
    at IncomingMessage.onEnd (/home/user/maybe-bug/node_modules/raw-body/index.js:287:7)
    at IncomingMessage.emit (node:events:518:28)
    at endReadableNT (node:internal/streams/readable:1696:12)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21)
SyntaxError: Unexpected token 'r', "{"read": falsre}" is not valid JSON
    at JSON.parse (<anonymous>)
    at parse (/home/user/maybe-bug/node_modules/body-parser/lib/types/json.js:92:19)
    at /home/user/maybe-bug/node_modules/body-parser/lib/read.js:128:18
    at AsyncResource.runInAsyncScope (node:async_hooks:206:9)
    at invokeCallback (/home/user/maybe-bug/node_modules/raw-body/index.js:238:16)
    at done (/home/user/maybe-bug/node_modules/raw-body/index.js:227:7)
    at IncomingMessage.onEnd (/home/user/maybe-bug/node_modules/raw-body/index.js:287:7)
    at IncomingMessage.emit (node:events:518:28)
    at endReadableNT (node:internal/streams/readable:1696:12)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21)

Test code:

#!/usr/bin/env node
const express = require('express');
const app = express();

// Setup - middlewares
app.use(express.json());

// Routes
app.get('/', (req, res) => {
    console.log('At index');
    res.status(200).json();
});

app.patch('/messages/:id', updateReadStatus);

async function updateReadStatus(req, res) {
    try {
        console.log('Im in!');
        const { read } = req.body;  // An HTML body is returned here when misspelling a boolean value.
        console.log(`The read is: ${read}`);   // <-- We never get to here. And no error is thrown either to try-catch it.
        res.status(200).json(req.body);
    } catch(e) {
        console.error('Not caught as an error either: ', e);
    }
}

async function main() {
    app.listen(5000, () => {
        console.log(`Server is running on http://localhost:5000/`);
    });
}

main().catch(err => console.log(err));

Is this the expected behaviour?

gulbaki commented 1 month ago 
--data '{"read": falsre}'

The value falsre is not valid JSON. JSON requires that strings be enclosed in double quotes. Since falsre is not enclosed in quotes, the parser assumes it's a boolean or other primitive type, but because falsre isn't recognized as a valid keyword (like true or false), the parser throws an error.

--data '{"read": "falsre"}'

Here, "falsre" is properly treated as a string, making the JSON valid.

So, in this case, the error is correct because the value "falsre" needs to be wrapped in double quotes for it to be recognized as a string.

dpopp07 commented 1 month ago

So, in this case, the error is correct because the value "falsre" needs to be wrapped in double quotes for it to be recognized as a string.

I don't think the question is whether or not the "error is correct" - it's certainly a valid error - but whether or not the behavior should be automatically returning HTML in the response.

Is this the expected behaviour?

It looks like it is. If you look at the code snippet below, you see that it tries to parse the body on line 131. When that fails, the error is caught and a 400-status error response is created and passed as the callback (effectively replacing yours, it seems like).

https://github.com/expressjs/body-parser/blob/966bc9dd141cae791d5634a46af58435327b3170/lib/read.js#L131-L137

So, what it looks like to me is that your comment "// An HTML body is returned here when misspelling a boolean value." is not quite accurate. I don't think your callback is ever actually invoked. Indeed, I wouldn't expect your console log "I'm in" to be called in this scenario. The error is caught and handled while processing through the middleware, before it gets to your callback.

Looks like the middleware is annotating the req object before it gets passed to your callback. This makes the current behavior make some sense, because if it can't parse the body, how should it communicate that through the req and res objects it passes to you? So I think that's why you're seeing a default response.

That said, it would be nice to have control over how your service responds (pun intended!) to bad user input. I'm not sure if or how express allows for any customization of that behavior.

dpopp07 commented 1 month ago

Turns out, this is indeed documented behavior.

dpopp07 commented 1 month ago

@Cinnamals The way to resolve this and gain control over the error handling is to write custom error handling middleware. See the Writing Error Handlers section. If you define it after the JSON handling middleware, the error response will be given to your middleware:

app.use(express.json());

app.use((err, req, res, next) => {
  console.error(err.message); // You will get the error from the body-parser here
  res.status(500).send('Something broke!');
})
Cinnamals commented 1 month ago

@Cinnamals The way to resolve this and gain control over the error handling is to write custom error handling middleware. See the Writing Error Handlers section. If you define it after the JSON handling middleware, the error response will be given to your middleware:

app.use(express.json());

app.use((err, req, res, next) => {
  console.error(err.message); // You will get the error from the body-parser here
  res.status(500).send('Something broke!');
})

Yes, thank you. I stupidly edited out that I am aware of this.

And I should have clarified that I am curious about sending primitive data types, not wrapping the value in quotation marks as to handle it as a string, like the first reply from @gulbaki suggests.

I am happy with your first replies, @dpopp07 ! I shouldve been faster to thank you. :)

Thanks for all the replies.

dpopp07 commented 1 month ago

No worries! Glad it was helpful

dpopp07 commented 1 month ago

@UlisesGascon I think this can be closed