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

Change request id header default value to false #4194

Conversation

philippviereck
Copy link
Contributor

Builds on #4193

This is a breaking change.

I believe the requestIdHeader should be opt-in instead of opt-out.

NOTE: when user sets requestIdHeader to
true the behaviour is the same as if it was set to false
because req.headers[true] || genReqId(req)
is equivalent to genReqId(req) (req.headers[true] is undefined, therefore genReqId will be used).
In other words requestIdHeader must be a string, else it will be 'ignored'.

Solution: Provide a default value for requestIdHeader when true or maybe throw.

Checklist

generated by running 'node build/build-validation.js'
after change on 'build-validation.js'
Changes default from 'request-id' to `false`.
Making it opt-in instead of opt-out.

NOTE: when user sets requestIdHeader to
`true` the behavior is the same as if it was set to `false`
because `req.headers[true] || genReqId(req)`
 is equivalent to `genReqId(req)` (req.headers[true] is undefined).
TL;DR requestIdHeader must be a string, else it will be 'ignored'.
Solution: Provide a default value for requestIdHeader when `true` or throw.
Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

I don't understand what this gives us other than API churn.

@philippviereck
Copy link
Contributor Author

Assuming #4194 gets accepted.
There will be an option to opt-out of the requestIdHeader functionality.
But still, by default the 'request-id' header will be enabled and overwrite the reqId if present.
Meaning any request can set its own reqId, essentially being able to 'pollute' the logs with duplicate IDs. This will make investigating an incident harder I'd assume. Though not impossible so I don't consider it a vulnerability.

I just feel like the default config should be locked down and you can 'open up' instead of you having to 'lock down'.
So I think this default behaviour is an unexpected behaviour and therefore should be opt-in.

BUT I understand that this might break more things than fix. I can live with the opt-out, just if you don't know about it you might get surprised.

@mcollina
Copy link
Member

This could land in v5 whenever we would ship it (likely spring/summer 2023)

lib/reqIdGenFactory.js Outdated Show resolved Hide resolved
@Eomm Eomm changed the base branch from main to next September 2, 2022 19:26
@Eomm Eomm added the semver-major Issue or PR that should land as semver major label Sep 25, 2022
@mcollina mcollina added this to the v5.0.0 milestone Nov 14, 2022
@Uzlopak
Copy link
Contributor

Uzlopak commented Jul 8, 2023

I investigated this. I kind of agree. Currently fastify will check for the request-id header and take that.

Imho it should be opt-out, as @philippviereck suggested.

@Uzlopak
Copy link
Contributor

Uzlopak commented Jul 8, 2023

@jsumners
@mcollina
@Eomm

PTAL

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm, we should do this for v5

@Uzlopak
Copy link
Contributor

Uzlopak commented Jul 9, 2023

@mcollina it is targetting next branch ;)

@Uzlopak Uzlopak requested a review from jsumners July 9, 2023 13:51
@mcollina mcollina merged commit 7381195 into fastify:next Jul 12, 2023
24 checks passed
@philippviereck philippviereck deleted the change-requestIdHeader-default-value-to-false branch July 18, 2023 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major Issue or PR that should land as semver major
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants