ensure that res.status is not called multiple times

NEW
Unassigned

Status

11 months ago
2 months ago

People

(Reporter: dustin, Unassigned, Mentored, NeedInfo)

Tracking

(Blocks: 1 bug, {good-first-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.
This fix would be in the taskcluster-lib-api library.
Mentor: dustin

Comment 2

11 months 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

11 months ago
Thank you for the guidance Dustin!
I will start working on it.

Comment 5

11 months 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

11 months 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

11 months 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

11 months 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

11 months 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!
Assignee: nobody → akshanayaki
Akshatha appears to have stopped working on this..
Assignee: akshanayaki → nobody

Comment 16

10 months ago
Hi Dustin. Can I take this task up ?
Assignee: nobody → tuhinatwyla
Please do!
Assignee: tuhinatwyla → nobody

Updated

6 months ago
Keywords: good-first-bug

Comment 18

5 months ago
Hey! Can I take up this bug? I am new so I would like to do this as my good first bug and I would also appreciate any help offered.
Sure!  Hopefully there's enough context above to get started?
Assignee: nobody → rishabh.budhiraja

Comment 20

5 months ago
Yes there is! I'll not hesitate to ask if I get stuck anywhere!

Comment 21

5 months ago
Hey Dustin! I could write a test case which would check if res.sent() is being sent twice or not. I wanted to ask you, that out of the 49 tests that were before I added mine, 38 were passing, 1 is pending and 10 were failing. Was it like that or did I mess something up or am I missing something here?
Regarding the test cases, the one pending makes sense.  However, the rest should all pass.  What errors are you seeing?

And just to make sure we're thinking of the same thing: what we need to do is make sure that any code *using* this library fails if it calls res.send() twice.  So just adding something under tests/ in taskcluster-lib-api won't be enough (since that code isn't run by users of the library).
Rishabh, how is this going?
Flags: needinfo?(rishabh.budhiraja)
Assignee: rishabh.budhiraja → nobody
You need to log in before you can comment on or make changes to this bug.