Node: Migration from process.binding

[jdalton]

With #22004 landed to doc deprecate process.binding I was encouraged to open an issue related to my use of it. Now is a great time to starting examining what from process.binding can be exposed in a user-friendly way.

In my own experience I've attempted to use _(if available and there were no user-facing options)_ these bindings. My usage fits into these categories

1. chrome inspector wiring
2. command-line checks
3. custom error stack decoration
4. custom inspection

The API run down as follows:

• process.binding("config")

  • experimentalREPLAwait _(command line checks)_
  • experimentalWorker _(command line checks)_
  • exposeInternals _(command line checks)_
  • preserveSymlinks _(command line checks)_
  • preserveSymlinksMain _(command line checks)_

• process.binding("inspector")

  • callAndPauseOnStart _(inspector wiring; other utils like ndb_ use this too)
  • consoleCall _(inspector wiring)_
  • open _(only existence checks, much like Node does)_

• process.binding("util")

  • decorated_private_symbol _(error stack decoration)_
  • getProxyDetails _(custom inspection)_
  • setHiddenValue _(error stack decoration)_
  • safeGetenv

For chrome inspector wiring there has been some work to expose things like originalConsole (#21659), but more in this area is needed.

For command-line checks the answer may be to use process.execArgv, but a more config like object form would be handy.

For custom error stack decoration there is early feelers in #21958.

Custom inspection helpers like getProxyDetails/getPromiseDetails and safeGetenv have nothing simmering at the moment.

Updated:

I crossed through API that have user-facing options.

Update:

Replaced process.binding("inspector").open inferences with process.config.variables.v8_enable_inspector checks.

[devsnek]

i have reservations about exposing anything on process.binding('util') publicly... stack decoration shouldn't have a node-specific solution and promise and proxy details can't be equally represented by the engines that might want to run node.

taking a step back, i think we should look at how the things these apis enabled would be implemented if these apis had never existed, instead of straight-up replacements.

[jdalton]

@devsnek

stack decoration shouldn't have a node-specific solution

Cool. As long as there's some way, it would be nice. _(#21958 is a step to providing a way)_

and promise and proxy details can't be equally represented by the engines that might want to run node.

Right, Chakra had to do extra work to support those.

taking a step back, i think we should look at how the things these apis enabled would be implemented if these apis had never existed, instead of straight-up replacements.

👏 That's a great exercise!

[mcollina]

consoleCall (inspector wiring)

@jdalton have you seen https://nodejs.org/api/inspector.html#inspector_inspector_console to print messages into the inspector console?

[jdalton]

@mcollina

have you seen https://nodejs.org/api/inspector.html#inspector_inspector_console to print messages into the inspector console?

Yes. I believe you're saying I can create my own form of consoleCall that pipes to Node's console and the inspector console with the exposed originalConsole. 👍

[mcollina]

It would be great if you could confirm that, so we can rule out one item from that list.

[jdalton]

@mcollina

It would be great if you could confirm that, so we can rule out one item from that list.

Confirmed. Looks like consoleCall could be replaced with a plain JS function.

[ak239]

I am not sure that I am not missing some context, but the vital part of console.log behavior at least on inspector side - it is providing stack trace that points to the place where console.log was called. I am worried that with plain JS function we will get JS stack frame that contains location inside this function.

[jdalton]

@ak239

I am worried that with plain JS function we will get JS stack frame that contains location inside this function.

Would you be up for providing a small repro case so I can test it out and see what's what?

[devsnek]

@jdalton

https://github.com/nodejs/node/blob/ce98e2e989ec52f6aa205c8b340ff214aba5f060/lib/internal/bootstrap/node.js#L441-L444

right here the two functions are wrapped to be called by a native function (consoleCall is c++) https://github.com/nodejs/node/blob/ce98e2e989ec52f6aa205c8b340ff214aba5f060/src/inspector_js_api.cc#L157-L181

[jdalton]

@devsnek I wasn't asking the whereabouts of consoleCall in Node core. I was asking for @ak239 to provide an example that showed the potential issue.

[devsnek]

@jdalton you can try replacing the c++ consoleCall with this if you wanna play around with it.

// because this is written in js it will be added as an additional frame
// which doesn't show up in the c++ version
function consoleCall(inspector_method, node_method, config_object, ...args) {
  const in_call_key = 'in_call';
  if (!(in_call_key in config_object)) {
    config_object[in_call_key] = true;
    inspector_method(...args);
    delete config_object[in_call_key];
  }
  node_method(...args);
}

[jdalton]

@devsnek

you can try replacing the c++ consoleCall with this if you wanna play around with it.

Thanks! I've got something on my end as well. I'm just looking for a test case to run it against is all.

[TimothyGu]

@jdalton

Test script:

'use strict';

const { Console } = require('console');
const originalConsole = require('inspector').console;

const config = {};

// Pretty straightforward port of InspectorConsoleCall() to JavaScript.
function consoleCall(inspectorMethod, nodeMethod, config, ...args) {
    // Assume inspector is always on.
    if (!('in_call' in config)) {
        config.in_call = true;
        inspectorMethod.apply(this, args);
    }
    delete config.in_call;

    nodeMethod.apply(this, args);
}

const jsWrappedConsole = new Console(process.stdout);
jsWrappedConsole.error = consoleCall.bind(jsWrappedConsole, originalConsole.error, jsWrappedConsole.error, config);

function usesCPPWrapped() {
    console.error('bleh');
}

function usesJSWrapped() {
    jsWrappedConsole.error('bleh');
}

usesCPPWrapped();
usesJSWrapped();

Note how console.error's stack is at the right place, but jsWrappedConsole.error's error symbol is always on the inspectorMethod.apply line.

Hope this helps.

[jdalton]

Ah yep okay, since console.error doesn't actually throw I can't get creative with Error.captureStackTrace either.

[devsnek]

@jdalton you can do builtinLibs.includes('inspector') for that open method which would be much more idiomatic anyway.

if my changes to error stack decoration ever land there will be no need for decorated_private_symbol, setHiddenValue, and getHiddenValue.

i would recommend creating os.getenv and os.setenv as a replacement for safeGetenv.

i would strongly disagree with exposing proxy inspection as an api and i will to the best of my ability block exposing promise inspection

[ak239]

Thanks @TimothyGu !

@jdalton, you can check the same thing inside ndb, just run it and evaluate console.trace in console, top frame should point to anonymous script, not to some line inside internal scripts.

BTW, regardless inspector binding that I use inside ndb. In ideal world I would like to extend inspector.open(...) API in a non trivial way. For my use case it would be nice to have callback as last argument of this function. If inspector.open is called with wait=true then this callback is called right before waiting and gets address of started inspector websocket as argument and if this callback returns true then node will break at first line of user script. From implementation point of view it should be easy to implement, I just need to find some time.

[jdalton]

@devsnek

i will to the best of my ability block exposing promise inspection

Let's hold off on elevating tone and focused on something more productive like following up on your previous comments...

taking a step back, i think we should look at how the things these apis enabled would be implemented if these apis had never existed, instead of straight-up replacements.

@ak239

Thank you for the feedback!

[devsnek]

@jdalton sorry i didn't mean to elevate, i was just trying to "check off" the items or so to speak

[mcollina]

@TimothyGu Maybe I'm not understading the use case for https://github.com/nodejs/node/issues/22064#issuecomment-410091767.
Why someone would need to wrap it? I completely understand the reason why we are binding it in C++ in Node.js, which is to redirect our console to two places.

Is that a common use case for that API?

[TimothyGu]

@mcollina

Why someone would need to wrap it?

I don't know why someone would need to do the same thing we are doing for console, but this thread is a conversation around the APIs we expose through process.binding() – whether useful for userland or not.

I'm just demonstrating how the function must be written in C++, and could not be written in JavaScript with the same effect, which was @jdalton's question.

[devsnek]

again i think what we need to do with this is just ask "what would people be doing with the console if these apis never existed"

[jasnell]

@jdalton ... be careful relying on process.config. The fact that it is mutable and the fact that there are modules in use in the ecosystem that completely overwrite it makes it unreliable... which is why internally we switched to using process.binding('config') in the first place.

[jdalton]

Tracking issue at https://github.com/nodejs/node/issues/22160.

[gajus]

This threat seems to have no mention of http_parser.

I am facing an issue where a website responds with an invalid header.

https://gist.github.com/gajus/45e37f8e46e400f377baa8a8dd39c92a

In earlier Node.js versions, it was possible to workaround this by overriding process.binding('http_parser').HTTPParser, e.g. with http-parser-js. However, currently attempting to override has no effect. It seems that "http_parser" is only used when --http-parser=legacy flag is present.

So aside from using --http-parser=legacy, what would be the correct way now to handle invalid HTTP response given that there seems to be no way to override the http_parser_llhttp binding?

Attempt to process.binding('http_parser_llhttp').HTTPParser = {} throws Error: No such module: http_parser_llhttp.

[gajus]

I have worked around this specific issue by using --http-parser=legacy and stripping the offending headers.

/**
 * ___utmv* headers when server by Incapsula CDN may include invalid chracters
 * that are causing 'Parse Error: Invalid header value char'.
 * We work around the Parse Error by using `--http-parser=legacy`.
 * However, we also strip the offending headers to avoid passing them
 * down if the request is being proxied.
 * @see https://gist.github.com/gajus/d02aa2c6c705fb8c3256a0108e4e03ff
 */
const sanitizeHeaders = (headers) => {
  const append = {};

  if (headers['set-cookie']) {
    append['set-cookie'] = [];

    for (const value of headers['set-cookie']) {
      if (!value.startsWith('___utmv')) {
        append['set-cookie'].push(value);
      }
    }
  }

  return {
    ...headers,
    ...append,
  };
};

const main = async () => {
  const got = require('got');

  const response = await got('https://entradas.cinesa.es/compra/?performanceCode=185560&s=192');

  console.log(sanitizeHeaders(response.headers));
};

main();

However, this means that I would not be able to workaround the issue if it was not possible to override http_parser or use --http-parser=legacy.

[stevenvachon]

Is this going to get fixed? I'd really prefer not to litter my codebase(s) with a fix for this.

[devsnek]

cc @nodejs/http ^

[jasnell]

The legacy http parser has been removed from master as of https://github.com/nodejs/node/commit/ac59dc42edb721ede2e5ddc6d1e4945ee2bf1e9c (https://github.com/nodejs/node/pull/29589) and the above workaround will not work from Node.js 13+ forward. As @bnoordhuis points out in the thread in https://github.com/nodejs/node/issues/21509, the current behavior is intentional as to avoid a very real security vulnerability, and it is working as intended.

[stevenvachon]

Is https://github.com/nodejs/node/issues/3591 related to this at all? It still has not been resolved.

[jasnell]

To be honest, I don't really know @stevenvachon, it's been a while since I've looked that issue over.

[gajus]

As @bnoordhuis points out in the thread in #21509, the current behavior is intentional as to avoid a very real security vulnerability, and it is working as intended.

What is the security vulnerability? He doesn't state in that comment the implications.

What is the workaround for when a remote service returns invalid response, such as in a scenario I describe in https://github.com/nodejs/node/issues/22064#issuecomment-519578895 ?

[jasnell]

Strict handling of invalid characters is necessary to prevent a variety of request smuggling and response splitting style attacks.

The correct workaround is to try to get the broken remote service fixed. Not sure what else we can do now that the legacy parser has been removed.

[devsnek]

was the question here about loosening how llhttp works or allowing the parser to be overridden? the latter seems like a reasonable request to me.

[gajus]

The correct workaround is to try to get the broken remote service fixed. Not sure what else we can do now that the legacy parser has been removed.

That is an insane position to take.

I run a large data aggregation network. There are literally hundreds of websites misbehaving even within our relatively restricted domain operation.

This change literally makes Node.js not usable for us.

[addaleax]

@gajus Although I see the relationship, I feel like this is turning into a separate discussion from what the issue is about, and I’d suggest opening a new issue, ideally with a description of what exact “features” you need from the newer parser that the old one had.

[bnoordhuis]

allowing the parser to be overridden

This has been discussed in the past (repeatedly and at length) and the answer has always been 'no' because it turns too many implementation details into frozen-forever public API.

Custom JS parsers exist (link). You could even WASM-ify http-parser if you want bug-for-bug compatibility. I'm its maintainer and I'd be open to that.

[gajus]

@devsnek overriding process.binding('http_parser').HTTPParser doesn't work in Node v12, though – does it?

[gajus]

The HTTPParser override does not work in v12 and above.

I have raised a new issue https://github.com/nodejs/node/issues/30573.

[jasnell]

For all internal uses, we have migrated away from using process.binding(). I believe we can close this issue now, although there is the follow on question of when/how we can deprecate process.binding()