Fixed of losing context existing headers in response

[skokalin]

It fixes losing context in response in case when here 3 or more Set-Cookie headers.
Closes: #1621

Proposed changes

Bugfix of the existing functionality

Checklist

Before creating a PR, run through this checklist and mark each as complete:

• [✔️] I have read the contributing guidelines.
• [✔️] I have signed the F5 Contributor License Agreement (CLA).
• [✔️] If applicable, I have added tests that prove my fix is effective or that my feature works.
• [✔️] If applicable, I have checked that any relevant tests pass after adding my changes.
• [✔️] I have updated any relevant documentation (README.md and/or CHANGELOG.md).

Unfortunately I did manage to run any unit tests, because it requires a lot of things to be installed
Would be great if someone could help me with it.

[ac000]

Hi, thanks for the pull-request.

Could you expand on the following please?

It fixes losing context ...

[skokalin]

Hi, sure!

The reproduce steps are located here #1621

Because of Set-Cookie header in the response might be repeated several times. Bug appears when is the request already set an array of the same headers (it's not related only for Set-Cookie header):

import express from 'express'
const app = express();

app.get('/', (req, res) => {
    // res.cookie('first', 'cookie', { maxAge: 900000, httpOnly: true })
    // res.cookie('second', 'cookie', { maxAge: 900000, httpOnly: true })
    // res.cookie('third', 'cookie', { maxAge: 900000, httpOnly: true })
    // ^^ here the bug, because at this moment we already have in the response an
    // array of already set cookies "first=cookie" and "second=cookie"

    res.set('X-Powered-By', ['Unit', 'Unit2', 'Unit3']);
    res.set('X-Powered-By', ['Unit4', 'Unit5']);
    // ^^^ here the bug, the same issue
    // in order to set new header with the same name we have to remove prev header
    // and set a new one, but because of usage function
    res.send('Hello, Express on Unit!')
})
app.listen(8080, () => {
    console.log(`Node server is listening on port 8080!`);
});

In order to update array of headers with the same name we have to remove previos and add a new one with updated values
Because in calculation of headers length related to field in the class it tries to find this.headers_len variable in boundaries of the function:
file http_server.js

Now let's change it to the arrow function:
file http_server.js

It also might be replaced to for cycle instead of arrow function in order to be more similar to code base

Also, I've managed how to add tests for that case, may I update the pr?
Implementation of that place also will be replaced to

        for (let i = 0; i < value.length; i++){
            this.headers_len -= Buffer.byteLength(value[i] + "", 'latin1');
        }

for cycle is faster than foreach

And thank you for so quick response!
Best regards

[ac000]

Thanks for the further explanation.

Also, I've managed how to add tests for that case, may I update the pr?

Sure, you can force push up changes.

While you're at it could you expand on the commit message with a summary of what you wrote above?

If you're updating the pytests, could you do that as a separate commit?

Thanks!

[ac000]

Great, thanks!

I'll take a look over these.

[ac000]

Just so that I'm 100% with this...

Added test for nodejs when request contains more than 2 headers with the same name

It's really any number of duplicate headers, right? 2, 3, 4 etc...

[skokalin]

correct. You might reproduce the issue by using the test case from second commit without my changes in the http_server.js file

[ac000]

Tweaked commit messages...

$ git range-diff 7183b921...eb1fc49f
1:  f27762ff ! 1:  9886d2a1 Fixed of losing context of existing headers in response
    @@ Metadata
     Author: skokalin <[email protected]>
     
      ## Commit message ##
    -    Fixed of losing context of existing headers in response
    +    node.js: Fixed issue with duplicate headers in response
    +
    +    It fixes losing context in response in cases when there are 2 or more
    +    headers with the same name.  The prev implementation used to use foreach
    +    function which uses local lexical environment and did not find
    +    this.headers_len locally, which causes crash of the http server module.
    +
    +    It was replaced with a for loop in order to make access for this.headers_len
    +    variable and improve performance of calculation.
     
    -    It fixes losing context in response in case when here 3 or more headers with the same name.
    -    The prev implementation used to use foreach function which uses local lexical environment and did not find this.headers_len localy, which causes crash of the http server module. It was replaced to for loop in order to make access for this.headers_len variable and improve performance of calculation
         Closes: https://github.com/nginx/unit/issues/1621
    +    Signed-off-by: Andrew Clayton <[email protected]>
     
      ## src/nodejs/unit-http/http_server.js ##
     @@ src/nodejs/unit-http/http_server.js: ServerResponse.prototype._removeHeader = function _removeHeader(lc_name) {
2:  7183b921 ! 2:  eb1fc49f Added test for nodejs when request contains more than 2 headers with the same name
    @@ Metadata
     Author: skokalin <[email protected]>
     
      ## Commit message ##
    -    Added test for nodejs when request contains more than 2 headers with the same name
    +    tests: nodejs: Added test for responses with duplicate headers
    +
    +    Signed-off-by: Andrew Clayton <[email protected]>
     
      ## test/node/set_header_array_with_override/app.js (new) ##
     @@

[ac000]

Rebased with master

$ git range-diff eb1fc49f...a9071e11
-:  -------- > 1:  4fcbe9ca ci: clang-ast: Update to openjdk-21-jdk
-:  -------- > 2:  39f91213 Use NULL instead of 0 as null pointer constant
1:  9886d2a1 = 3:  9ae10673 node.js: Fixed issue with duplicate headers in response
2:  eb1fc49f = 4:  a9071e11 tests: nodejs: Added test for responses with duplicate headers

[ac000]

Thanks @skokalin