http: Expose http.validateHeaderName/Value #33119

osher · 2020-04-28T09:35:50Z

The use-case is for any framework that provides user mw a response replacement, that collects the desired response state, and sends it only on conclusion.

As such a framework, I'd want to validate the header names and values as soon as the user-code provides them.
This - to

eliminate errors on response-send time,
provide developer stack trace that contains the line that submits the offending values

Doing it this will allow to:

const { validateHeaderName, validateHeaderValue } = require('http');

and use them.

Checklist

added docs
added tests
commit message follows commit guidelines

osher · 2020-04-28T09:44:38Z

BTW - My current work-around is a hack, roughly in the spirit below.
I'd be more than happy to just use the validators as proposed...

(req, res, next) => {
   const headers = {}
   //TRICKY: have to do fool around the sybmol here because of a
   //      "[kOutHeaders]  === null" check in `setHeader`
   //        - even though cautious code initiates it when it's not found!!
   //      (classic case of a strict check that defeats the purpose)
   //      anyway - this is fragile in case the symbol form changes ... :stuck_out_tongue: 
   const headersProp = Object.getOwnPropertySymbols(res).find(
      s => s.toString().includes('kOutHeaders')
   );
   const { setHeader } = res
   //.... more stuff

   const reply = {
      //... more stuff
      setHeader: (k, v) => {
          try {
               setHeader.call({ [headersProp] : null }, k, v)
          } catch (e) {
               throw augmentWithFrameworkContextInfo(e)
          }
      },
      //... more stuff
   }

   controller[method]( req, reply).then(finishIt).catch(next)
}

mscdex · 2020-04-28T11:23:45Z

-1 It's not clear to me why this is required. Besides, if the internals change where other public API methods end up using additional validation functions, those would need to be exported too and trying to keep up with that would be a nightmare.

If the end goal here is to mutate the error object thrown by setHeader(), there are less brittle solutions without needing to expose and keep on top of these internal functions:

Use a custom response class via ServerResponse: MyServerResponseConstructor in a config object passed to http(s).createServer(). This avoids having to monkey-patch methods every time for every request/response.
If the purpose of the custom reply is to restrict access to res methods/properties, you could do:

   const reply = {
      //... more stuff
      setHeader: (...args) => {
          try {
               setHeader.call(this, ...args)
          } catch (e) {
               throw augmentWithFrameworkContextInfo(e)
          }
      },
      //... more stuff
   }
   OutgoingMessage.call(reply);

If restricting access to res methods/properties is not a goal, you can simply just monkey-patch setHeader directly (although option 1 would be better for this instead):

  res.setHeader = (...args) => {
    try {
      setHeader.call(this, ...args)
    } catch (e) {
      throw augmentWithFrameworkContextInfo(e)
    }
  }

osher · 2020-04-28T12:28:52Z

@mscdex - thanks for your quick response.

My personal goal - as in, the specific use-case I'm currently at now - is indeed to mediate and restrict access to res, but I was not thinking only about myself but trying to think generically.
I want to be able to reuse the same validators. The fact that I'm building a mediator is a private case.
It could be a proofer, a code analysis tool, a server-code generator, a network analyzer, a VS/Atom editor-plugin, anything that might want to assist with http-headers.

It's a purely functional logic with no implications of state... 😉

Keeping this logic hidden basically results in code duplications in user codes...
Examples of code duplications from the open-source word - without searching a lot I saw this:
https://www.npmjs.com/package/http-headers-validation
That's only because of the obviousness of the relevance implied by the name.
I'm afraid we can be sure it's possible to find more.

Anyway - the example I brought here is very simplified, I have more going on (potentially proprietary).

I admit that what you proposed in your points will work - but IMHO - it is by far more cumbersome than just using the validator functions.
in cumbersome - I mean:

it needs more lines of code
reading them explains less the goal, and will require a lot of stories in comments and lore passed in code-reviews.

For the work-around - I consider replacing my workaround with option 2 just until such change can be accepted - however - that's basically replacing one specific monkey patch with another, both of which are workaround hacks tailored for this use-case and do not solve the fact that this logic should be reusable and it is not.
I'm considering option 2 because indeed it's less fragile - so as a start - sincere thanks here 😄

About the argument of needing future validations - do you feel that any future validation will not be applicable as part of validateHeaderName or validateHeaderValue?
Would there be anything else to validate except header names and values? If there were - wouldn't that be a breaking change?

I considered for a moment to add validateHeader(name, value), but strived to keep my changes minimal and not introduce new functions, because this my first PR here, and I'm yet to get tests running locally. Admittedly lame, but I'll do it in baby steps and let myself be led from here 😄

osher · 2020-04-28T16:01:06Z

@mscdex - thanks for the lead!! - FYI - I got a simpler hack:

const dummy = new OutgoingMessage()
const reply {
   //...
   setHeader: (k, v) => {
       try {
          dummy.setHeader(k, v)
       //....
   },
   //.. 
}

(never thought this constructor will work without all the wireworks...)

Still monkeying, just shorter, and does not mount lotta dirt on an Object passed to user-code.
The fact that I have to run the OutgoingMessage constructor at all is ...:thinking: ...not pretty...

So I still believe this PR has value and no risk 😉

mscdex · 2020-04-28T16:21:31Z

I still think we shouldn't expose them. If internals change and/or these functions change in one way or another (e.g. split into smaller functions or even removed entirely), we don't want to have to be maintaining these utility functions because they've now been exposed to userland.

I think the solutions outlined in this PR are fine for userland. If others happen to share this similar (and uncommon IMO) need, they can use one of those -- or develop their own validators.

osher · 2020-04-28T20:09:13Z

would you please consider for a moment that whatever any additional validations may be needed in the future, they can be implemented as part of validateHeaderName and/or validateHeaderValue?

Let us also consider that if there will be such a need for future validations, it would also mean that the code duplications of this logic will need it too... 😉

As for the 3 ways - I have played with them, and they do not fit the use-case of mediation and restricting access to res.
Way 1 - I end with working on the response object - which is exactly what I want to prevent.
Way 2 - I end up with loads of attributes created by the constructor mounted on the object I pass the user
Way 3 - works on the actual response object, and assumes that all I want is to augment the error.
Way 4 - (the one I added) - requires me to create and keep a hidden dummy instance of an OutgoingMessage, which I have doubts about its footprint. Feels like abuse. I'd do it happily on tests, not on production code.

Hmmm.
You know what? Let's ask this. I have another direction I can propose.

If you would like not to export these validators, - here's an alternative that does not expose anything that is not already exposed.

How about the following change instead. Would that be considerable?

 OutgoingMessage.prototype.setHeader = function setHeader(name, value) {
   if (this._header) {
     throw new ERR_HTTP_HEADERS_SENT('set');
   }
   validateHeaderName(name);
   validateHeaderValue(name, value);

   let headers = this[kOutHeaders];
-  if (headers === null)
+  if (!headers)
     this[kOutHeaders] = headers = ObjectCreate(null);

   headers[name.toLowerCase()] = [name, value];
 };

it could also be

-  if (headers == null)
+  if (!headers)

or

-  if (headers === null)
+  if (!Boolean(headers))

whatever fits the lint rules...

Unfortunately, this line is what prevents me from a clear and efficient code, in the spirit of:

  const { setHeader } = OutgoingMessage.prototype.setHeader;
  ...
  try {
     setHeader.call({}, k, v)

because, you see, it ensists on having this[kOutgoingHeaders] === null in order to initiate that object...
If it did not ensist on null - it would have initiated it and move on happily

What do you think?

addaleax

This seems okay to me, but it should come with tests and documentation

osher · 2020-04-30T19:12:51Z

Let`s start with tests.
well. here's a cultural shock... 🤔
I took a look at the tests directory... not what I expected to see.

As a basis - I can obviously show the validators are exposed and that they have the expected signature.
I can also show that they are usable as purely functional "static methods".
Last - I can show that they pass correct values and fail wrong values - but I'm not sure if I should - this should already be tested somewhere already, no?

Anyway - I'll appreciate a point in the right direction

So what I'm expected here is something in the spirit of /test/parallel/test-http-outgoing-header-validators.js?
does it matter if it`s one file per tested API or it can be one for both?

I'm also confused that there are no test titles... hardly comments...
Does that mean I should isolate a test-case per file? an API per file?

Thanks in advance for any guidance.
I've shied from getting involved for a long time, but here we are...

Edited

ok. I found the writing-tests.md guide and the BUILDING.md, I'm running make for the first time (in my life, ever)...

I mean to do it in one file for both validators if that's alright with ppl here
does /test/parallel/test-http-outgoing-header-validators.js sound correct?
or should it be /test/parallel/test-http-header-validators.js ?
And if so - should they be exposed as require('http').validateHeaderName|validateHeaderName as well?

osher · 2020-05-05T10:13:56Z

@addaleax
I added tests, I'm looking for the right place to add docs - I could not find any page dedicated to OutgoingMessage.

I do suspect that it's what referred in docs as ServerResponse, am I right? Is that were I should document it?

When I'm looking at how it looks from user perspective, I start to think that it will provide a better interface if they'll be exposed directly on http, so that instead:

const { OutgoingMessage: { validateHeaderName, validateHeaderValue } } = require('http')

it will be

const { validateHeaderName, validateHeaderValue } = require('http')

I was also surprized to find that some obvious validations do not happen.

E.g - this does not fail validation:

const str = "1234567890"
const obviouslyTooLong = Array.from(Array(5e6)).map(() => str) // Gives length 5e7
validateHeaderValue('x-too-long', obviouslyTooLong)

I actually got to fail on the max length of strings before I saw any error thrown for invalid length of header name or value.

I don't mind if it's not part of the infra, this length validation may indeed belong to user-land, I just have to double-check with you...

To sum up:

LMK how to proceed with docs - I'll be happy to go on
LMK about what's the policy re. validations
LMK about the final interface exposed to the user - http.validate.... or http.OutgoingMessage.validate... (or both, one a friendlier shorthand to the other)

Thanks!!

EDITED

I'm getting the feeling that OutgoingMessage is an undocumented implementation detail, and therefore the validators should not be exposed on it, but either on http or on http.ServerResponse.

Can you please confirm this or correct me so I know how to proceed?
Thanks again!!

addaleax · 2020-05-05T13:41:03Z

I'm getting the feeling that OutgoingMessage is an undocumented implementation detail, and therefore the validators should not be exposed on it, but either on http or on http.ServerResponse.

Can you please confirm this or correct me so I know how to proceed?

Yes, that’s correct – OutgoingMessage serves as the base class for client requests and server responses, but it’s not intended to be used directly by users. Providing these methods on the http module itself seems to make sense to me 👍

osher · 2020-05-05T15:34:05Z

@addaleax - Okie dokie. So here's what we've got so far: - validator methods are exposed directly on `http` (which internally pulls them from `OutgoingMessage` - I did it this way to minimize code changes, I hope it's on target). - test file named `test/parallel/test-http-validate-headers.js`, and test them from there - added docs in `docs/http.md`

…

On Tue, May 5, 2020 at 4:41 PM Anna Henningsen ***@***.***> wrote: I'm getting the feeling that OutgoingMessage is an undocumented implementation detail, and therefore the validators should not be exposed on it, but either on http or on http.ServerResponse. Can you please confirm this or correct me so I know how to proceed? Yes, that’s correct – OutgoingMessage serves as the base class for client requests and server responses, but it’s not intended to be used directly by users. Providing these methods on the http module itself seems to make sense to me 👍 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#33119 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGECHLLHK7QFEJ5D34JR63RQAJQBANCNFSM4MSVHO3Q> .

addaleax

LGTM with the linter failures fixed

@nodejs/http

doc/api/http.md

addaleax · 2020-05-05T15:41:52Z

doc/api/http.md

+try {
+ validateHeaderName('')
+} catch(err) {
+ err instanceof TypeError // --> true


I’m not sure we should document this, partly because it seems wrong to me to use TypeError here.

Suggested change

err instanceof TypeError // --> true

Please have a look at the docs for setHeader - it explicitly says TypeError, so I took it from there.
Do you still believe we should remove it?

P.S - good that you note it - because many infrastructures I've witnessed (the lionshare are proprietary, but not only) use error types to distinguish between and sort errors, often with a secondary look at the code.

I personally detest class hierarchies and consider them to be implementation details, but, you know... let them have what they say they can't live without...

addaleax · 2020-05-05T15:42:07Z

doc/api/http.md

+try {
+ validateHeaderValue('x-my-header', undefined);
+} catch(err) {
+ err instanceof TypeError; // --> true


ditto here,

Suggested change

err instanceof TypeError; // --> true

The use-case is for any framework that provides user mw a response replacement, that collects the desired response state, and applies them only on conclusion. As such a framework, I'd want to validate the header names and values as soon as the user-code provides them. This - to eliminate errors on response-send time, and provide developer stack trace that contains the line that submits the offending values.

osher · 2020-05-06T13:55:29Z

@addaleax - I adhered everything except one thing I would like to double-check -

about removing from doc examples:

   err instanceof TypeError; // --> true

Please have a look at the docs for setHeader - it explicitly says TypeError, so I took it from there.
Do you still believe we should remove it?

addaleax · 2020-05-07T23:32:28Z

@osher It’s not essential but yes, I’d prefer to remove those lines – partially for the exact reason that you mentioned, which is that if you want to check for this type of error, looking at the error code is the right thing to do 👍

nodejs-github-bot · 2020-05-07T23:34:11Z

CI: https://ci.nodejs.org/job/node-test-pull-request/31224/

The use-case is for any framework that provides user mw a response replacement, that collects the desired response state, and applies them only on conclusion. As such a framework, I'd want to validate the header names and values as soon as the user-code provides them. This - to eliminate errors on response-send time, and provide developer stack trace that contains the line that submits the offending values. PR-URL: #33119 Reviewed-By: Anna Henningsen <anna@addaleax.net>

addaleax · 2020-05-09T05:56:46Z

Landed in 96faea1, thanks for the PR!

mscdex · 2020-05-09T13:48:07Z

It seems these functions are still tacked onto OutgoingMessage instead of being added to the module.exports for lib/_http_outgoing.js?

osher · 2020-05-10T07:59:32Z

@mscdex -
they are exposed on http, see here: https://github.com/nodejs/node/blob/master/lib/http.js#L67
the fact that they stay on outgoing-message where they are primarily used is an implementation detail 😉

@addaleax - I agree that the err.code has a finer resolution, and as a consultant, when I'm asked about error handling - that's what I always recommend. However, people come to node from many different backgrounds and bring their habits with them, and they kinda expect it to work like they're used to, even if they're missing the spirit of the platform. So I thought that as long as it's not a mistake - it would be a part of making the platform welcoming for foreigners.
I see you took the PR as is - Thank you for your responsiveness and tolerance - I will be happy to work with you again 👍

mscdex · 2020-05-10T08:31:48Z

the fact that they stay on outgoing-message where they are primarily used is an implementation detail

I don't understand how it's an implementation detail. There's no need for it to be set on OutgoingMessage solely for the purposes of exporting from require('http'). The functions could have been added to the module.exports in lib/_http_outgoing.js just like OutgoingMessage is, and then those would get pulled out of there from lib/http.js.

osher · 2020-05-10T08:41:32Z

Yes. I see what you point at, that's true.
You mean in _http_outgoing

- OutgoingMessage.validateHeaderName = validateHeaderName;
- OutgoingMessage.validateHeaderValue = validateHeaderValue;

  module.exports = {
+   validateHeaderName,
+   validateHeaderValue,
    OutgoingMessage
  };

and in http:

- const { OutgoingMessage } = require('_http_outgoing');
- const { validateHeaderName, validateHeaderValue } = OutgoingMessage;
+ const { OutgoingMessage, validateHeaderName, validateHeaderValue } = require('_http_outgoing');

(plus similar effect on tests)

I can agree It looks tidier.

Would you like a PR for that?
given that the tests and docs are ready - it should be a fast one, right?

mscdex · 2020-05-10T13:10:35Z

PR would be great.

The use-case is for any framework that provides user mw a response replacement, that collects the desired response state, and applies them only on conclusion. As such a framework, I'd want to validate the header names and values as soon as the user-code provides them. This - to eliminate errors on response-send time, and provide developer stack trace that contains the line that submits the offending values. PR-URL: #33119 Reviewed-By: Anna Henningsen <anna@addaleax.net>

Notable changes: async_hooks: * (SEMVER-MINOR) move PromiseHook handler to JS (Stephen Belanger) #32891 cli: * (SEMVER-MINOR) add `--trace-atomics-wait` flag (Anna Henningsen) #33292 fs: * (SEMVER-MINOR) add .ref() and .unref() methods to watcher classes (rickyes) #33134 http: * (SEMVER-MINOR) expose http.validate-header-name/value (osher) #33119 repl: * (SEMVER-MINOR) deprecate repl._builtinLibs (Ruben Bridgewater) #33294 * (SEMVER-MINOR) remove obsolete completer variable (Ruben Bridgewater) #33294 * (SEMVER-MINOR) deprecate repl.inputStream and repl.outputStream (Ruben Bridgewater) #33294 * (SEMVER-MINOR) improve repl autocompletion for require calls (Ruben Bridgewater) #33282 * (SEMVER-MINOR) replace hard coded core module list with actual list (Ruben Bridgewater) #33282 * (SEMVER-MINOR) show reference errors during preview (Ruben Bridgewater) #33282 * (SEMVER-MINOR) improve repl preview (Ruben Bridgewater) #33282 src: * add support for TLA (Gus Caplan) #30370 test: * (SEMVER-MINOR) refactor test/parallel/test-bootstrap-modules.js (Ruben Bridgewater) #33282 PR-URL: TODO

Notable changes: async_hooks: * (SEMVER-MINOR) move PromiseHook handler to JS (Stephen Belanger) #32891 cli: * (SEMVER-MINOR) add `--trace-atomics-wait` flag (Anna Henningsen) #33292 fs: * (SEMVER-MINOR) add .ref() and .unref() methods to watcher classes (rickyes) #33134 http: * (SEMVER-MINOR) expose http.validate-header-name/value (osher) #33119 repl: * (SEMVER-MINOR) deprecate repl._builtinLibs (Ruben Bridgewater) #33294 * (SEMVER-MINOR) deprecate repl.inputStream and repl.outputStream (Ruben Bridgewater) #33294 * (SEMVER-MINOR) show reference errors during preview (Ruben Bridgewater) #33282 * (SEMVER-MINOR) improve repl preview (Ruben Bridgewater) #33282 src: * add support for TLA (Gus Caplan) #30370 PR-URL: TODO

Notable changes: async_hooks: * (SEMVER-MINOR) move PromiseHook handler to JS (Stephen Belanger) #32891 cli: * (SEMVER-MINOR) add `--trace-atomics-wait` flag (Anna Henningsen) #33292 fs: * (SEMVER-MINOR) add .ref() and .unref() methods to watcher classes (rickyes) #33134 http: * (SEMVER-MINOR) expose http.validate-header-name/value (osher) #33119 repl: * (SEMVER-MINOR) deprecate repl._builtinLibs (Ruben Bridgewater) #33294 * (SEMVER-MINOR) deprecate repl.inputStream and repl.outputStream (Ruben Bridgewater) #33294 * (SEMVER-MINOR) show reference errors during preview (Ruben Bridgewater) #33282 * (SEMVER-MINOR) improve repl preview (Ruben Bridgewater) #33282 src: * add support for TLA (Gus Caplan) #30370 PR-URL: #33452

MylesBorins · 2020-08-17T21:56:36Z

Would we want to backport this to v12.x? If so it seems like they should come with #33371

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Apr 28, 2020

osher marked this pull request as draft April 28, 2020 09:36

osher marked this pull request as ready for review April 28, 2020 11:51

addaleax reviewed Apr 29, 2020

View reviewed changes

addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 29, 2020

addaleax approved these changes May 5, 2020

View reviewed changes

osher changed the title ~~http: Expose OutgoingMessage.validateHeaderName/Value~~ http: Expose http.validateHeaderName/Value May 5, 2020

osher requested a review from addaleax May 6, 2020 13:53

addaleax approved these changes May 7, 2020

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 7, 2020

addaleax closed this May 9, 2020

osher deleted the patch-1 branch May 10, 2020 07:39

This was referenced May 12, 2020

http: tidy up exposure of header validation #33370

Closed

http: tidy up exposure of header validation #33371

Closed

codebytere mentioned this pull request May 18, 2020

v14.3.0 proposal #33452

Merged

MylesBorins added the backport-requested-v12.x label Aug 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: Expose http.validateHeaderName/Value #33119

http: Expose http.validateHeaderName/Value #33119

osher commented Apr 28, 2020 •

edited

Loading

osher commented Apr 28, 2020 •

edited

Loading

mscdex commented Apr 28, 2020 •

edited

Loading

osher commented Apr 28, 2020 •

edited

Loading

osher commented Apr 28, 2020

mscdex commented Apr 28, 2020

osher commented Apr 28, 2020

addaleax left a comment

osher commented Apr 30, 2020 •

edited

Loading

osher commented May 5, 2020 •

edited

Loading

addaleax commented May 5, 2020

osher commented May 5, 2020 via email

addaleax left a comment

addaleax May 5, 2020

osher May 5, 2020 •

edited

Loading

addaleax May 5, 2020

osher commented May 6, 2020 •

edited

Loading

addaleax commented May 7, 2020

nodejs-github-bot commented May 7, 2020

addaleax commented May 9, 2020

mscdex commented May 9, 2020

osher commented May 10, 2020 •

edited

Loading

mscdex commented May 10, 2020

osher commented May 10, 2020 •

edited

Loading

mscdex commented May 10, 2020

MylesBorins commented Aug 17, 2020

http: Expose http.validateHeaderName/Value #33119

http: Expose http.validateHeaderName/Value #33119

Conversation

osher commented Apr 28, 2020 • edited Loading

Checklist

osher commented Apr 28, 2020 • edited Loading

mscdex commented Apr 28, 2020 • edited Loading

osher commented Apr 28, 2020 • edited Loading

osher commented Apr 28, 2020

mscdex commented Apr 28, 2020

osher commented Apr 28, 2020

addaleax left a comment

Choose a reason for hiding this comment

osher commented Apr 30, 2020 • edited Loading

Edited

osher commented May 5, 2020 • edited Loading

EDITED

addaleax commented May 5, 2020

osher commented May 5, 2020 via email

addaleax left a comment

Choose a reason for hiding this comment

addaleax May 5, 2020

Choose a reason for hiding this comment

osher May 5, 2020 • edited Loading

Choose a reason for hiding this comment

addaleax May 5, 2020

Choose a reason for hiding this comment

osher commented May 6, 2020 • edited Loading

addaleax commented May 7, 2020

nodejs-github-bot commented May 7, 2020

addaleax commented May 9, 2020

mscdex commented May 9, 2020

osher commented May 10, 2020 • edited Loading

mscdex commented May 10, 2020

osher commented May 10, 2020 • edited Loading

mscdex commented May 10, 2020

MylesBorins commented Aug 17, 2020

osher commented Apr 28, 2020 •

edited

Loading

osher commented Apr 28, 2020 •

edited

Loading

mscdex commented Apr 28, 2020 •

edited

Loading

osher commented Apr 28, 2020 •

edited

Loading

osher commented Apr 30, 2020 •

edited

Loading

osher commented May 5, 2020 •

edited

Loading

osher May 5, 2020 •

edited

Loading

osher commented May 6, 2020 •

edited

Loading

osher commented May 10, 2020 •

edited

Loading

osher commented May 10, 2020 •

edited

Loading