Session: Bad Request causes broken JSON response

[G-Adams]

When querying an invalid column name the JSON response is missing the last curly brace only when the store is instantiated for the session. If session store is not used this issue does not present. This occurs only with bad requests (400).

with store:
{ "message": "Bad Request"

without store:
{ "message": "Bad Request" }

Similar issue here. The workaround posited does fix the problem, but I'm unsure why implementing the store causes the workaround to be necessary.

Express/Session are most recent version as of this post.

Code:
```js
const bodyParser = require('body-parser');
const fs = require('fs');
const https = require('https');

const app = express();
const session = require('express-session');
const MySQLStore = require('express-mysql-session')(session);
const { serverError } = require('./utils/response');
const { log } = require('./utils');
const prototypes = require('./utils/prototypes');
const db = require('./utils/db/mysql');

db.connect();
const mysqlConn = db.pool;
const storeOptions = {
host: process.env.DB_HOST,
port: process.env.DB_PORT,
user: process.env.DB_USER,
password: process.env.DB_PASSWORD,
database: process.env.DB_DATABASE,
};
const sessionStore = new MySQLStore(storeOptions, mysqlConn);
// load all prototypes on initial startup
for (const func of Object.values(prototypes)) {
func();
}

// Tell express to use these middleware functions for every request
app.use(bodyParser.json());
app.use(bodyParser.urlencoded({
extended: false,
}));
let useSecure = true;
if (process.env.SECURE) {
useSecure = process.env.SECURE === 'true';
}

app.use(session({
secret: process.env.SESSION_SECRET,
resave: false,
saveUninitialized: false,
rolling: true,
store: sessionStore, // Commenting out this line fixes the bad request issue but does not instantiate store
cookie: {
sameSite: 'lax',
secure: !!process.env.SECURE_CONTEXT,
// 24 minutes - should use env variable
maxAge: parseInt(process.env.SESSION_LENGTH, 10) || 1440000,
},
}));

[dougwilson]

Hi @G-Adams thank you for the report! What request needs to be made to your example application above to see the behavior reproduced?

[G-Adams]

Hi @dougwilson thanks for the speedy response. Any kind of request that queries a table inclusive of a column name that does not exist on that table. For example, a GET request on a cars table that looks for motorcycleID.

[dougwilson]

Any kind of request that queries a table inclusive of a column name that does not exist on that table. For example, a GET request on a cars table that looks for motorcycleID.

Ok. Sorry, but it's not obvious what that would be. Like... GET /cars ? Can you show what the URL would be? The app itself seems incomplete to run, as running it above doesn't actually result in a running server, so not sure where to start on how to reproduce it. Perhaps you can provide step-by-step specific instructions. Alternatively, you're always welcome to submit a pull request with a fix, but if you're looking for someone from the project to create a fix, you need to provide clear reproduction steps. I look forward to hearing your instructions.

[ghinks]

Hello @G-Adams as mentioned, can you confirm that any query to a table with a column name that does not exist produces an issue for you.

If so could you please give as a simple example of a route that does such so we can try to reproduce this?

[G-Adams]

Hi @ghinks, I can confirm that this is the case.

An example of the URL request would be GET /cars?MotorcycleID="1".

The cars table would have a CarID column not a MotorcycleID column

The steps to reproduce are:

1. Instantiate ExpressSession with default SessionStore
2. Create a valid session
3. Make bad GET request to API (e.g. query a table with a where clause for a column known to not exist)
4. See JSON response with missing closing curly brace. { "message": "Bad Request"

Everything else works completely fine, there is only an issue specifically with Bad Requests responses.

[ghinks]

hello, checking now ..., this is the only session store I did not have a docker setup for. I'm also noting that the instructions for express-mysql-session work for mysql 5 but there are login issues on mysql 8.
Give me till the end of the day to get a working example going for us to examine :)

[dougwilson]

Hey @ghinks is there enough code above to actually diagnose anything, though? For examples it references multiple files not provided and there are no routes declared, so calling any URL on that app is just going to 404 Not Found back...

[G-Adams]

@dougwilson The posted code was just a snippet to show that the express & related packages are properly instantiated, I assumed that this could be checked within whichever test environment you might have to develop this package with by making similar calls to reproduce the issue. It would take me some time to make a working copy as the project that this applies to robust but I can do that sometime in the next week or two if necessary.

[ghinks]

This is correct @dougwilson. @G-Adams it would be best to have a working example for your issue. To do this correctly we need a simple route with a function and a call to the DB table.

This would show us the npm module client you use to call the DB etc. So @G-Adams , just a smidgen of code demonstrating would be required.

Likewise for us setting up environments. If you give me the client library that would be a starting point.

[dougwilson]

I mean, our test suite does not suffer from the reported issue, so I would say that would be evidence that we cannot easily reproduce it. It is possible that the store you are using could be the cause, though you can check that by seeing if it still reproduces with the MemoryStore (just remove the store parameter to use it). If it does reproduce for you still, the difference between your reproduction and ours would lie somewhere in the code differences provided.

[ghinks]

As I had not done a mysql server store before and it was next on my list. I came up with this. But I do not get the error being described.

All I see is the error logged as

error ER_BAD_FIELD_ERROR: Unknown column 'bad_column' in 'field list'

So we will need to get a small and working example of this in order to proceed. @G-Adams we can help, but maybe only if you can re-produce it.

const fs = require("fs");
const express = require("express");
const session = require("express-session");
const path = require("path");
const cookieParse = require("cookie-parser");
const mysql = require("mysql");

const fsp = fs.promises;
const app = express();
const MySqlStore = require("express-mysql-session")(session);
const https = require("https");

const secret = "keyboard cat";
const APP_PORT = 3000;
const mySqlOptions = {
  host: "localhost",
  port: 3306,
  user: "root",
  password: "password",
  database: "sessions"
};
const connection = mysql.createConnection(mySqlOptions);
connection.connect();

const sessionStore = new MySqlStore(mySqlOptions);
app.use(cookieParse(secret));
app.use(
  session({
    cookie: {
      secure: true,
      path: "/bar",
      maxAge: 60000,
      httpOnly: true,
      domain: "localhost"
    },
    store: sessionStore,
    secret,
    resave: false,
    name: "test-integration-sid-mysql",
    proxy: true,
    saveUninitialized: true
  })
);

app.get("/bar", (req, res) => {
  console.log("/bar");
  try {
    connection.query("select session_id, expires, data, bad_column from sessions", function(
      error,
      results,
      fields
    ) {
      if (error) {
        console.error(`error ${error.message}`);
      }
      console.log(
        `results are ${JSON.stringify(results)}, fields ${JSON.stringify(
          fields
        )}`
      );
    });
  } catch (e) {
    console.log(e.message);
  }
  res.send("bar");
});

const getCertificates = async () => {
  const cert = await fsp.readFile(
    path.join(__dirname, "../certificates/selfsigned.crt"),
    "utf-8"
  );
  const key = await fsp.readFile(
    path.join(__dirname, "../certificates/selfsigned.key"),
    "utf-8"
  );
  return {
    cert,
    key
  };
};

getCertificates()
  .then(({ cert, key }) => {
    https
      .createServer(
        {
          key,
          cert
        },
        app
      )
      .listen(APP_PORT);
  })
  .catch(e => {
    console.error("bad ...things"); // eslint-disable-line no-console
    console.log(e.message); // eslint-disable-line no-console
  });

[G-Adams]

@ghinks @dougwilson here is a repository for an environment in which you can see the issue, and steps to replicate. If I can provide any further information that you would find helpful I'd be happy to oblige.

[dougwilson]

Hi @G-Adams thanks for that repo. I believe the issue here is in your code. The reason it happens when you add this module is just because it adds a delay that reveals your code issue is all. The issue is that you are _writing to the response twice_ in the case you outlined.

Ultimately it boils down to in your code you are doing

res.status(400).json({'badRequest': 'truncated'});
next(err);

The issue is that when you start a response writing, when Express sees an error (next(err)) while it is in the middle of writing out the respond body, it assumes you have made a critical mistake and just destroys the response object, killing the TCP connection at that point.

You can read more about this in the Express documentation http://expressjs.com/en/guide/error-handling.html

If you call next() with an error after you have started writing the response (for example, if you encounter an error while streaming the response to the client) the Express default error handler closes the connection and fails the request.

[dougwilson]

To add to the above , I would suggest two different ways to handle the errors:

1. Just keep your try catch, send response pattern and remove the next(e)
2. Move out the response logic from the route handler and instead have it call next(e) on an error and create a separate error handler

There are no _wrong_ answers, just different ways to approach it.

[G-Adams]

Thank you for your time, this solution worked! I'm unsure as to why this issue only presents when using the session store, do you have any ideas?

Also: !githubgold, this has been plaguing me for a couple weeks

[dougwilson]

Yes, it exists when you add session store, as it seems it was the first complex middleware you added to your project. It would have likely happened as you began to add more middleware of a different kind as well. The response is just taking slightly longer to write out, long enough that the error routine now has the opportunity to kill the response before it finishing writing. It would have also revealed itself if your error response was larger, making it take a bit to write it out, giving that opportunity for the error to propagate during the response writing.