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

Fix/connot set trust proxy #654

Closed
wants to merge 8 commits into from
10 changes: 7 additions & 3 deletions args.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,14 @@ const DEFAULT_ARGUMENTS = {

module.exports = function parseArgs (args) {
dotenv.config()
const TRUST_PROXY = process.env.TRUST_PROXY || ''
Copy link
Member

Choose a reason for hiding this comment

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

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'],
Copy link
Member

Choose a reason for hiding this comment

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

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

envPrefix: 'FASTIFY_',
alias: {
port: ['p'],
Expand Down Expand Up @@ -65,7 +66,9 @@ module.exports = function parseArgs (args) {

// Merge objects from lower to higher priority
const parsedArgs = { ...DEFAULT_ARGUMENTS, ...configFileOptions, ...commandLineArguments }

const isNumber = isNaN(Number(TRUST_PROXY)) === false
const isTrue = TRUST_PROXY.toLowerCase() === 'true'
const trustProxy = isNumber ? Number(TRUST_PROXY) : isTrue
Comment on lines +69 to +71
Copy link
Member

Choose a reason for hiding this comment

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

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

return {
_: parsedArgs._,
'--': additionalArgs,
Expand All @@ -91,6 +94,7 @@ module.exports = function parseArgs (args) {
lang: parsedArgs.lang,
method: parsedArgs.method,
commonPrefix: parsedArgs.commonPrefix,
includeHooks: parsedArgs.includeHooks
includeHooks: parsedArgs.includeHooks,
trustProxy: parsedArgs.trustProxy || Boolean(trustProxy)
}
}
4 changes: 3 additions & 1 deletion start.js
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,9 @@ async function runFastify (args, additionalOptions, serverOptions) {
if (opts.options && file.options) {
options = deepmerge(options, file.options)
}

if (opts.trustProxy) {
options.trustProxy = opts.trustProxy
}
const fastify = Fastify(options)

if (opts.prefix) {
Expand Down
18 changes: 12 additions & 6 deletions test/args.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ test('should parse args correctly', t => {
lang: 'js',
method: undefined,
commonPrefix: false,
includeHooks: undefined
includeHooks: undefined,
trustProxy: false
})
})

Expand Down Expand Up @@ -108,7 +109,8 @@ test('should parse args with = assignment correctly', t => {
lang: 'js',
method: undefined,
commonPrefix: false,
includeHooks: undefined
includeHooks: undefined,
trustProxy: false
})
})

Expand Down Expand Up @@ -181,7 +183,8 @@ test('should parse env vars correctly', t => {
lang: 'js',
method: undefined,
commonPrefix: false,
includeHooks: undefined
includeHooks: undefined,
trustProxy: false
})
})

Expand Down Expand Up @@ -272,7 +275,8 @@ test('should parse custom plugin options', t => {
lang: 'js',
method: undefined,
commonPrefix: false,
includeHooks: undefined
includeHooks: undefined,
trustProxy: false
})
})

Expand Down Expand Up @@ -310,7 +314,8 @@ test('should parse config file correctly and prefer config values over default o
lang: 'js',
method: undefined,
commonPrefix: false,
includeHooks: undefined
includeHooks: undefined,
trustProxy: true
})
})

Expand Down Expand Up @@ -352,6 +357,7 @@ test('should prefer command line args over config file options', t => {
lang: 'js',
method: undefined,
commonPrefix: false,
includeHooks: undefined
includeHooks: undefined,
trustProxy: true
})
})
3 changes: 2 additions & 1 deletion test/data/custom-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,6 @@ module.exports = {
prettyLogs: true,
debugPort: 4000,
pluginTimeout: 9 * 1000,
closeGraceDelay: 1000
closeGraceDelay: 1000,
trustProxy: true
}
Loading