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

FST_ERR_DEC_ALREADY_PRESENT error when set trustProxy to true #50

Closed
Dafyh opened this issue Sep 13, 2024 · 8 comments
Closed

FST_ERR_DEC_ALREADY_PRESENT error when set trustProxy to true #50

Dafyh opened this issue Sep 13, 2024 · 8 comments
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@Dafyh
Copy link
Contributor

Dafyh commented Sep 13, 2024

Describe the bug
If the trustProxy option is set to true, the following error occurs: FST_ERR_DEC_ALREADY_PRESENT.

To Reproduce
Steps to reproduce the behavior:

'use strict'
const tap = require('tap')
const fastify = require('fastify')
const plugin = require('../')

tap.test('Plugin#Decoration', scope => {
  scope.test('Throw a FST_ERR_DEC_ALREADY_PRESENT error', async t => {
    await fastify({ trustProxy: true }).register(plugin).ready()
  })
})

Expected behavior
Should not return any errors.

Screenshots
image

@metcoder95 metcoder95 added the bug Something isn't working label Sep 15, 2024
@metcoder95
Copy link
Owner

Thanks for the report!

We can provide two properties at the same time, if the request.ip has been already set, we can just skip it.

The second one can always be set, something like realIp or inferredIp (or similar).

Would you like to submit a PR?

@metcoder95 metcoder95 added the good first issue Good for newcomers label Sep 15, 2024
@Dafyh
Copy link
Contributor Author

Dafyh commented Sep 16, 2024

Yes, I can do a PR.

However, we could redefine the getter ip in the hook preHandler instead of adding a property in the case where trustProxy is true (ip, ips, hostname and protocol are present) ?

@Dafyh
Copy link
Contributor Author

Dafyh commented Sep 16, 2024

For example :

const isTrustProxyOn = [
  'ip',
  'ips',
  'hostname',
  'protocol'
].every((key) => instance.hasRequestDecorator(key))

if (isTrustProxyOn) {
  async function redefineIpDecorator (request) {
    Object.defineProperty(request, 'ip', {
      get () {
        return ipDecorator.getter.apply(request)
      }
    })
  }

  instance.addHook('preHandler', redefineIpDecorator)
} else {
  instance.decorateRequest('ip', ipDecorator)
}

Where ipDecorator is the object with the getter.

@metcoder95
Copy link
Owner

sgtm. i’d just recommend to not re define the property but rather just assign it.

let’s also document this in the documentation

@Dafyh
Copy link
Contributor Author

Dafyh commented Sep 16, 2024

Unfortunately, I can't assign a value as the setter is not implemented (TypeError). We could also propose a PR on fastify, but the simplest solution would be to redefine the property into fastify-ip 😊.

I will do a PR in any cases.

see

@metcoder95
Copy link
Owner

ah good callout, then that's fine 👍

@fraxken
Copy link

fraxken commented Sep 17, 2024

Thanks for the fast answer and the release ❤️ 🚀

@Dafyh
Copy link
Contributor Author

Dafyh commented Sep 18, 2024

Thanks 👍.

@Dafyh Dafyh closed this as completed Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants