[BUG] Model deserialization fails if class has an initializing constructor

[mfuqua3]

Information

• Version: 6.114.17
• Packages:
  @tsed/json-mapper

The goal is to support the use of C#-esque "object initializer" syntax within domain TsED models. Currently this type of constructor will break the deserializer

Example

Steps to Reproduce:

Bug Test Controller

import { CollectionOf, getJsonSchema, Groups, Property, Put, Required, Returns, Summary } from "@tsed/schema";
import { BodyParams, PathParams } from "@tsed/platform-params";
import { deserialize } from "@tsed/json-mapper";
import { Controller } from "@tsed/di";
export class TestChildModel {
  id: number;
  name: string;
}
export class TestArrayModel {
  id: number;
  name: string;
}
@Groups<TestModel>({
  test: ["includeMe", "includedObject", "includedArray"],
  test2: ["excludeMe", "excludedArray"],
  test3: ["includeMe", "includedArray", "includedObject", "excludeMe", "excludedArray", "excludedObject"],
})
export class TestModel {
  /**
   * This constructor allows C#-esque object initialization syntax
   * example: const model = new TestModel({includeMe: "included"});
   * @param init
   */
  public constructor(init?: Partial<TestModel>) {
    Object.assign(this, init);
  }

  @Required()
  @Property(String)
  includeMe: string;
  @Required()
  @Property(String)
  excludeMe: string;
  @Required()
  @CollectionOf(TestArrayModel)
  includedArray: TestArrayModel[];
  @Required()
  @CollectionOf(TestArrayModel)
  excludedArray: TestArrayModel[];
  @Required()
  @Property(TestChildModel)
  includedObject: TestChildModel;
  @Required()
  @Property(TestChildModel)
  excludedObject: TestChildModel;
}

@Controller("api/bug-test")
export class BugTestController {
  @Put("/:pathString")
  @Summary("Test a Bug")
  @Returns(204)
  async bugTest(
    @PathParams("pathString") pathString: string,
    @BodyParams() @Groups("test") model: TestModel,
  ): Promise<void> {
    console.log(model);
    //check if schema is wrong?
    const schema = getJsonSchema(TestModel, { groups: ["test"] });
    console.log(schema);
    //check if manual deserialize works?
    const request = deserialize(model, { type: TestModel, groups: ["test"] });
    console.log(request);
  }
}

Test the endpoint

curl --location --request PUT 'http://localhost:3000/api/bug-test/someString' \
--header 'Content-Type: application/json' \
--data-raw '{
    "includeMe":"included",
    "excludeMe":"should have been excluded",
    "includedArray":[
        {
            "id":1,
            "name":"included array item 1"
        }
    ],
    "excludedArray":[
        {
            "id":2,
            "name":"should have been excluded"
        }
    ],
    "includedObject": {
        "id": 3,
        "name":"included child object"
    },
    "excludedObject": {
        "id": 4,
        "name":"should have been excluded"
    }
}'

Result

JSON Schema:

{
  type: 'object',
  properties: {
    includeMe: { type: 'string', minLength: 1 },
    includedArray: { type: 'array', items: [Object] },
    includedObject: { '$ref': '#/definitions/TestChildModel' }
  },
  required: [ 'includeMe', 'includedArray', 'includedObject' ],
  definitions: {
    TestArrayModel: { type: 'object' },
    TestChildModel: { type: 'object' }
  }
}

Deserialized Model:

TestModel {
  includeMe: 'included',
  excludeMe: 'should have been excluded',
  includedArray: [ TestArrayModel { id: 1, name: 'included array item 1' } ],
  excludedArray: [ { id: 2, name: 'should have been excluded' } ],
  includedObject: TestChildModel { id: 3, name: 'included child object' },
  excludedObject: { id: 4, name: 'should have been excluded' }
}

Acceptance criteria

• Deserializer operates as expected for a model with a "object init" constructor:
  Constructor example:

public constructor(init?: Partial<TestModel>) {
    Object.assign(this, init);
  }

Additional Context

Apparent root cause of the issue is line 121 of deserialize.ts (plainObjectToClass function).
const out: any = new type(src);

From what I can tell, passing src as a constructor parameter to type is unnecessary to the operation of this function but creates the object in a pre-populated state in this specific use case. The deserializer does not expect the object to be pre-populated and does not sanitize the extra properties.

[Romakita]

Hello @mfuqua3
For me it’s not a bug. The deserialiser give the raw object to the class constructor to allow custom mapping. I won’t change this behavior because it’s totally intented. Your constructor allow a wrong object assignment. It’s not secure to use Object.assign() (prototype pollution).

Changing that will cause a potential breaking change.

See you

[Romakita]

Also I’m not sure to understand why it broke the deserializer, excepted about the potential class properties pollution (introduced by your Object.assign()). All properties with annotation will be deserialized correctly. It doesn’t throw error.

[mfuqua3]

Hey @Romakita , thank you for the response.

I believe this constructor signature caused some unexpected behavior in the deserializer because the plainObjectToClass function invokes the class model constructor using the raw request body as a parameter:

I understand my use-case is not the intention, but I could not figure out what purpose this invocation serves.

Edit: I misread your comment and I understand the purpose.

Here is additional context about my architecture and how I encountered this problem. I may need to determine an alternate way to what I'm going for.

---

Due to the addition of Groups, my team has been able to consolidate many of our request bodies into a small number of DTO models. However, we have chosen not to use these Model types within our Service functions but would instead rather use dedicated interfaces that apply to the specific use case.

This has also allowed me to encapsulate deserialization/serialization within our presentation layer, by limiting model interactions to within our controllers.

Consider this example, which closely reflects the patterns of my system architecture:

Data Contracts

Interfaces defining the request parameters and expected response from my business layer

export interface CreateUserRequest {
  name: string;
  email: string;
}

export interface User {
  id: number;
  name: string;
  email: string;
}

Business Layer Interface

Service component abstraction to be provided by the Ts.ED injector:

export interface IUserService {
  createUser(request: CreateUserRequest): Promise<User>;
}

export const IUserService: unique symbol = Symbol.for("IUserService");

TsED Model

Implements data contracts, defines JSON serialization behavior, validation, and schema for my OpenAPI spec.

@Groups<UserModel>({
  create: ["name", "email"],
  info: ["id", "name", "email"],
})
export class UserModel implements CreateUserRequest, User {
  constructor(init?: Partial<UserModel>) {
    Object.assign(this, init);
  }
  @Required()
  @Integer()
  id: number;
  @Required()
  @Email()
  @Property(String)
  email: string;
  @Required()
  @Property(String)
  name: string;
}

Controller

Ties all of the pieces together

@Controller("/api/users")
export class UsersController {
  constructor(@Inject(IUserService) private readonly userService: IUserService) {
  }

  @Post("/")
  @Summary("Create a New User")
  @Returns(201, UserModel).Groups("info")
  async createUser(@BodyParams() @Groups("create") request: UserModel): Promise<UserModel> {
    const createdUser = await this.userService.createUser(request);
    return new UserModel(createdUser); //<-- Object init constructor is used to assign response to TsED model
  }
}

This is my current architecture and the constructor in the model (used by the final step of my controller) is my current issue.

[Romakita]

Hello @mfuqua3

Sorry, I thought I answered you on the last comment but it seems that the message got lost.

I see your issue. But I cannot change the json-mapper behavior.

But you can simplify your code. When you us @Returns on an endpoint, the moel will be used by the json-mapper to serialize you object (plain object or instance class) to generate the response. So you don't need to create a new UserModel in your controller.

@Controller("/api/users")
export class UsersController {
  constructor(@Inject(IUserService) private readonly userService: IUserService) {
  }

  @Post("/")
  @Summary("Create a New User")
  @Returns(201, UserModel).Groups("info")
  async createUser(@BodyParams() @Groups("create") request: UserModel): Promise<UserModel> {
    return this.userService.createUser(request);
  }
}

This feature works with Returns(201, UserModel).Groups("info") and Returns(200, Array).Of( UserModel).Groups("info").

So it should solve partially your problem.

To avoid prototype pollution you have to spread object and pick each property value before assignment:

export class UserModel implements CreateUserRequest, User {
  constructor({id, name, email}?: Partial<UserModel> = {}) {
    Object.assign(this, {id, name, email});
  }
}

But this code is similar to:

import {deserialize} from "@tsed/json-mapper";

const instance = deserialize<UserModel>(obj, {type: UserModel});

See you
Romain

[mfuqua3]

@Romakita

Thank you, this is enormously helpful feedback. I will adjust our application per your suggestions, thank you for your time and all of the work you do on this framework.

We can consider this issue closed.

[github-actions]

🎉 Are you happy?

If you appreciated the support, know that it is free and is carried out on personal time ;)

A support, even a little bit makes a difference for me and continues to bring you answers!

[Romakita]

I'll consider your issue for the next major version. Your constructor example give me a potential security issue ^^.
Thanks for that ;)

[Romakita]

Ok I'll introduce a new configuration option to disable this behavior in v6:

@Constructor({
  jsonMapper: {
    disableUnsecureConstructor: true
  }
})

v7 will set this option to true by default.

[Romakita]

PR created: #2059

[github-actions]

🎉 Are you happy?

If you appreciated the support, know that it is free and is carried out on personal time ;)

A support, even a little bit makes a difference for me and continues to bring you answers!

[Romakita]

🎉 This issue has been resolved in version 6.128.9 🎉

The release is available on:

• v6.128.9
• GitHub release

Your semantic-release bot 📦🚀

[Romakita]

🎉 This issue has been resolved in version 7.0.0-rc.4 🎉

The release is available on:

• v7.0.0-rc.4
• GitHub release

Your semantic-release bot 📦🚀

[Romakita]

🎉 This issue has been resolved in version 7.0.0-rc.6 🎉

The release is available on:

• v7.0.0-rc.6
• GitHub release

Your semantic-release bot 📦🚀

[Romakita]

🎉 This issue has been resolved in version 7.0.0 🎉

The release is available on:

• v7.0.0
• GitHub release

Your semantic-release bot 📦🚀