Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add process.env.NODE_TLS_WARN_ON_UNAUTHORIZED to silence https warning #40587

Conversation

martinmunillas
Copy link

I would like a way to silence that warning as I'm totally aware of all the security issues that this might bring but I'm doing it only to simplify the development.

I couldn't find where to add tests or add documentation so if anyone could tell me where it would be awesome.

I would appreciate any kind of feedback :D

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Oct 24, 2021
@martinmunillas martinmunillas changed the title feat: add process.env.NODE_TLS_WARN_ON_UNAUTHORIZED to silence warning feat: add process.env.NODE_TLS_WARN_ON_UNAUTHORIZED to silence https warning Oct 24, 2021
@cjihrig
Copy link
Contributor

cjihrig commented Oct 24, 2021

You can use this instead - https://nodejs.org/api/cli.html#node_tls_reject_unauthorizedvalue

EDIT: Oops, that still warns in the first case. Can you use --no-warnings?

@martinmunillas
Copy link
Author

martinmunillas commented Oct 24, 2021

You can use this instead - https://nodejs.org/api/cli.html#node_tls_reject_unauthorizedvalue

EDIT: Oops, that still warns in the first case. Can you use --no-warnings?

I would like to only silence that warning, any other warning might be useful, maybe there is another way to silence just some warnings, or proxy them or something?

@cjihrig
Copy link
Contributor

cjihrig commented Oct 24, 2021

There is an issue somewhere that talks about making --no-warnings filter certain types of warnings. I think that would be a better approach than special casing this one warning.

@martinmunillas
Copy link
Author

There is an issue somewhere that talks about making --no-warnings filter certain types of warnings. I think that would be a better approach than special casing this one warning.

I agree, how can I help with that?

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welcome, @martinmunillas, and thanks for the pull request. I think this makes the default behavior insecure and undesirable, no? Regardless, this would also need a test, although I can understand waiting to see how folks react to the idea first.

lib/internal/options.js Show resolved Hide resolved
@cjihrig
Copy link
Contributor

cjihrig commented Oct 24, 2021

I agree, how can I help with that?

It looks like this PR has stalled out. You could try to get it going again, or see if the author minds if you take it over.

@martinmunillas
Copy link
Author

martinmunillas commented Oct 24, 2021

I agree, how can I help with that?

It looks like this PR has stalled out. You could try to get it going again, or see if the author minds if you take it over.

as it was for deprecated warnings i decided to solve it like this

const emitWarning = process.emitWarning;
process.emitWarning = (warning: string, ...values: any[]) => {
  if (warning.includes("NODE_TLS_REJECT_UNAUTHORIZED")) {
    return;
  }

  emitWarning(warning, ...values);
};

still I think there should be a way to handle this natively, what do you think?

@cjihrig
Copy link
Contributor

cjihrig commented Oct 24, 2021

as it was for deprecated warnings i decided to solve it like this

Check out some of the comments later in that thread. The idea would be to extend --no-warnings to support filtering by error code.

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think disabling this particular warning is dangerous. The warning should never occur in production and it should only be emitted once per process during development.

As others have suggested, a more generic approach that doesn't focus on a security critical warning might be better.

@martinmunillas
Copy link
Author

i will close this pr, thanks for the feedback

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants