Skip to content

fix: hasRequestDecorator/hasReplyDecorator misses constructor-assigned built-in properties#6753

Open
LeSingh1 wants to merge 1 commit into
fastify:mainfrom
LeSingh1:fix/decorator-instance-property-check
Open

fix: hasRequestDecorator/hasReplyDecorator misses constructor-assigned built-in properties#6753
LeSingh1 wants to merge 1 commit into
fastify:mainfrom
LeSingh1:fix/decorator-instance-property-check

Conversation

@LeSingh1

@LeSingh1 LeSingh1 commented Jun 4, 2026

Copy link
Copy Markdown

What

hasRequestDecorator and hasReplyDecorator only check the constructor prototype and the decorator props list. But several core properties — req.id, req.params, req.raw, req.query, req.log, req.body, reply.raw, reply.request, reply.log — are assigned directly in the Request and Reply constructors, not on their prototypes.

This means:

fastify.hasRequestDecorator('id')   // returns false (wrong)
fastify.decorateRequest('id', null) // silently succeeds (wrong)
// then at request time:
// TypeError: Cannot set property id... or req.id is null

Root cause

checkRequestExistence and decorateConstructor in lib/decorate.js use Object.hasOwn(prototype, name) and name in prototype. Properties set in the constructor body are never on the prototype, so these checks miss them.

Fix

Add an instanceProperties set to Request and Reply listing their constructor-assigned string-keyed properties. decorateConstructor and the checkRequest/ReplyExistence helpers now also check this set (walking the parent chain for derived constructors).

After the fix:

fastify.hasRequestDecorator('id')     // true
fastify.hasReplyDecorator('raw')      // true
fastify.decorateRequest('id', null)   // throws FST_ERR_DEC_ALREADY_PRESENT
fastify.decorateReply('raw', null)    // throws FST_ERR_DEC_ALREADY_PRESENT
fastify.decorateRequest('myExtra', null) // still works fine

All 52 existing decorator tests pass. Six new node:test tests cover the bug and the fix.

…-assigned built-in properties

Properties like req.id, req.params, req.body, reply.raw, and reply.log
are set directly in the Request and Reply constructors rather than on
their prototypes. Because of this, hasRequestDecorator('id') returned
false and decorateRequest('id', null) silently succeeded, overwriting
the built-in value and causing a TypeError at request time.

This adds an instanceProperties set to Request and Reply listing those
constructor-assigned string-keyed properties, and teaches
decorateConstructor and the existence helpers to check that set.
hasRequestDecorator('id') now returns true, and decorateRequest('id')
now throws FST_ERR_DEC_ALREADY_PRESENT as expected.
Comment thread lib/decorate.js
if (Object.hasOwn(instance, name) || hasKey(konstructor, name)) {
if (Object.hasOwn(instance, name) || hasKey(konstructor, name) || hasInstanceProperty(konstructor, name)) {
throw new FST_ERR_DEC_ALREADY_PRESENT(name)
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wonder if this should be named coreProperty rather than instanceProperty, since instanceProperty sounds a bit too general for the issue we are trying to address.

I would also recommend throwing a specific error when users try to override core properties. This would be more educational and would prevent them from searching for decorators they never set. For example, they might otherwise wonder where in their code they decorated request with id.

Example error code: FST_ERR_DEC_CORE_OVERRIDE.

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants