ensure that res.status is not called multiple times

NEW
Unassigned

Status

Taskcluster
Platform Libraries
14 days ago
19 hours ago

People

(Reporter: dustin, Unassigned, Mentored)

Tracking

(Blocks: 1 bug)

Details

Simple errors like a forgotten await can end up with an API method calling `res.send` twice with different results.  This should raise alarm bells that would cause tests to fail and tracebacks to go to sentry.
Blocks: 1436000
This fix would be in the taskcluster-lib-api library.
Mentor: dustin@mozilla.com

Comment 2

5 days ago
Hey, Can I work on this bug ? Some guidance on how to approach it would be appreciated. 
Thanks !
Sure!

Have a look at https://github.com/taskcluster/taskcluster-lib-api.  You'll want to get familiar with Express.js too.

A good way to start would be to add a test case to taskcluster-lib-api that calls res.status(200) twice in the same request.  We want the second time to be an error, but right now it's not.

The next step will be to figure out if Express can be configured to detect this and throw an exception.  If not, then we'll need to figure out how to throw that exception ourselves.
Summary: ensure that res.send is not called multiple times → ensure that res.status is not called multiple times

Comment 4

5 days ago
Thank you for the guidance Dustin!
I will start working on it.

Comment 5

4 days ago
Hello Dustin,

Checking the value of res.headerSent can be used to determine whether res.send is being called the second time. Have I understood it correctly ?
Yes, that sounds about right.  It is probably worth looking at the Express source code to see if res.send checks res.headerSent.  It seems like it does not!

Comment 7

4 days ago
We can throw an exception when res.send is called the 2nd time, as -

`if (res.headersSent) throw new Error('Multiple responses from same request');`

Is this correct ?
That sounds good, yes :)

Comment 9

3 days ago
I assume the test cases can be added in test/auth_test.js file. How do I run the tests ? I am new to Express.js, So a little help on how to test whether my changes are working would be appreciated.
Thanks!
It's probably best to add a new test file for this, patterned after auth_test.js.

The test files have this format:

suite(
  // declare an API
  // setup
  // teardown
  // tests
)

So I think you could start by declaring an API with one method that calls res.status(200) twice.  Then add a test that calls the API method and checks that it returns a 500 status, not 200 :)

...actually, that won't work.  The first call to res.status(200) will succeed and send the 200 response before the second call happens.  Ugh.

Brian, do you have an idea how to test this, based on your recent work with req.authorize?

Akshatha, please make a pull request with your changes, that passes all of the existing tests.  We can do some review on that while we figure out how to write the tests :)
Flags: needinfo?(bstack)
Before we write a test for this, we should either land or reject something I just put up for review [0]. It is just a change to test/auth_test.js in an attempt to clean it up and make it easier to understand.

Once we decide the status of that, we can add a test that uses side-effects to test. We pass a mock lib-monitor instance into api methods and then assert certain things about the mock are true afterward in a few other places [1]. You can see the mock implementation in [2]. This may be a good place to assert that reportError was called on lib-monitor in the tests rather than asserting return codes. I will be doing a similar thing to check for regressions in authorize().


[0] https://github.com/taskcluster/taskcluster-lib-api/pull/78
[1] https://github.com/taskcluster/taskcluster-lib-api/blob/d4aadf0405a0051db523390468184f7e4e3c30d4/test/responsetimer_test.js#L69-L78
[2] https://github.com/taskcluster/taskcluster-lib-monitor/blob/master/src/mockmonitor.js#L16
Flags: needinfo?(bstack)

Comment 12

2 days ago
I am getting the following error when I try to run the tests -

akshatha@akshatha-HP-15-Notebook-PC:~/taskcluster-lib-api/test$ mocha auth_test.js
/home/akshatha/taskcluster-lib-api/test/auth_test.js:1
(function (exports, require, module, __filename, __dirname) { suite('api/auth', function() {
                                                              ^
ReferenceError: suite is not defined
    at Object.<anonymous> (/home/akshatha/taskcluster-lib-api/test/auth_test.js:1:63)
    at Module._compile (module.js:660:30)
    at Object.Module._extensions..js (module.js:671:10)
    at Module.load (module.js:573:32)
    at tryModuleLoad (module.js:513:12)
    at Function.Module._load (module.js:505:3)
    at Module.require (module.js:604:17)
    at require (internal/module.js:11:18)
    at /usr/local/lib/node_modules/mocha/lib/mocha.js:231:27
    at Array.forEach (<anonymous>)
    at Mocha.loadFiles (/usr/local/lib/node_modules/mocha/lib/mocha.js:228:14)
    at Mocha.run (/usr/local/lib/node_modules/mocha/lib/mocha.js:536:10)
    at Object.<anonymous> (/usr/local/lib/node_modules/mocha/bin/_mocha:582:18)
    at Module._compile (module.js:660:30)
    at Object.Module._extensions..js (module.js:671:10)
    at Module.load (module.js:573:32)
    at tryModuleLoad (module.js:513:12)
    at Function.Module._load (module.js:505:3)
    at Function.Module.runMain (module.js:701:10)
    at startup (bootstrap_node.js:190:16)
    at bootstrap_node.js:662:3

Initially I was getting this error when trying to run `node auth_test.js`
Some links on the Internet suggested mocha needs to be installed and added to dependencies. But even after running the tests with mocha, it still doesn't seem to solve the issue. Please help and let me know if I am doing something wrong. Thanks !
If you run `yarn` and then `yarn test` does that still happen? We use https://yarnpkg.com/ to manage dependencies.

Comment 14

19 hours ago
Thanks Brian. That fixed the issue.

I have made a pull request with the changes I discussed in comment 7.
https://github.com/taskcluster/taskcluster-lib-api/pull/80

Please review. 
Thanks!
You need to log in before you can comment on or make changes to this bug.