Skip to content

Change request id header default value to false#4194

Merged
mcollina merged 9 commits intofastify:nextfrom
philippviereck:change-requestIdHeader-default-value-to-false
Jul 12, 2023
Merged

Change request id header default value to false#4194
mcollina merged 9 commits intofastify:nextfrom
philippviereck:change-requestIdHeader-default-value-to-false

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

Philipp Viereck added 5 commits August 11, 2022 00:27
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)

@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
@philippviereck philippviereck deleted the change-requestIdHeader-default-value-to-false branch July 18, 2023 12:37
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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.

6 participants