Fix/connot set trust proxy #654
Conversation
| @@ -23,13 +23,14 @@ const DEFAULT_ARGUMENTS = { | |||
|
|
|||
| module.exports = function parseArgs (args) { | |||
| dotenv.config() | |||
| const TRUST_PROXY = process.env.TRUST_PROXY || '' | |||
There was a problem hiding this comment.
We use the FASTIFY_ prefix everywhere, I would keep it
| const commandLineArguments = argv(args, { | ||
| configuration: { | ||
| 'populate--': true | ||
| }, | ||
| number: ['port', 'inspect-port', 'body-limit', 'plugin-timeout', 'close-grace-delay'], | ||
| string: ['log-level', 'address', 'socket', 'prefix', 'ignore-watch', 'logging-module', 'debug-host', 'lang', 'require', 'config', 'method'], | ||
| boolean: ['pretty-logs', 'options', 'watch', 'verbose-watch', 'debug', 'standardlint', 'common-prefix', 'include-hooks'], | ||
| boolean: ['pretty-logs', 'options', 'trustProxy', 'watch', 'verbose-watch', 'debug', 'standardlint', 'common-prefix', 'include-hooks'], |
There was a problem hiding this comment.
it has multiple shapes.
fwis, string and boolean are the most used so I would support it or I would rename it to FASTIFY_ENABLE_TRUST_PROXY
| const isNumber = isNaN(Number(TRUST_PROXY)) === false | ||
| const isTrue = TRUST_PROXY.toLowerCase() === 'true' | ||
| const trustProxy = isNumber ? Number(TRUST_PROXY) : isTrue |
There was a problem hiding this comment.
this is very hard to read and it does too much logic IMHO
regardless this code, the parsedArgs will win if the user set a boolean.. I would hardly understand the behaviour here.
KIS approach:
- one env per boolean
- one env per trust proxy hosts
- one env per trust hop
the priority is the list i wrote when user set multiple env vars
|
@Eomm it seems this PR was closed, we use Am I correct in reading this that for this more flexible |
Checklist
npm run testandnpm run benchmarkand the Code of conduct