Real-World Bugs: Uncontrolled Object Mapping & Spread Syntax

Hi there! When building an API or BFF Gateway, you often need to convert data from input/output ports to domain classes. There are two common ways to create mapping layers: using mapper libraries or manually mapping one object to another. Each approach has its advantages and disadvantages. Personally, I prefer using auto-mapper libraries when working with strongly typed languages like Java. However, when developing in Javascript or Typescript, I usually implement mapper methods and call them explicitly. It is also important to validate all input before mapping them.

In this article, I will demonstrate a bug that is caused by uncontrolled mapping and insecure usage of the spread operator in Javascript.

Context

A BFF Gateway fetches product information from a domain service, enhances it with the merchant data, and sends it back to the clients. The technology stack includes NodeJS, Typescript, and Express.

Proposed Solution

The application fetches data from two distinct sources. The code sample is a simplified version of both the product service client and the merchant service client.

export interface ProductServiceResponse {
    id: string;
    name: string;
    description: string;
    price: number;
    merchantId: string;
}

export interface MerchantServiceResponse {
    id: string;
    name: string;
    score: number;
    communicationChannel: {
        address: string;
        taxAddress: string;
    }
}

export class ProductServiceClient {
    public async getProduct(id: string): Promise<Optional<ProductServiceResponse>> {
        // http call and type assertions...
    }
}


export class MerchantServiceClient {
    public async getMerchant(id: string): Promise<Optional<MerchantServiceResponse>> {
        // http call and type assertions...
    }
}

Business logic is straightforward. If the product cannot be found, return an empty result (optional.empty); otherwise, return the product with or without the merchant data. Let us take a closer look at the implementation and try to spot the bug.

// Domain representation of a Merchant
export interface Merchant {
    name: string;
    rating: number;
    address: string;
    taxAddress: string;
}

// Domain representation of a Product
export interface Product {
    id: string;
    title: string;
    metaData: string;
    price: number;
    merchant?: Merchant;
}

export class ProductService {
    private readonly productClient: ProductServiceClient;
    private readonly merchantClient: MerchantServiceClient;

    public constructor(productClient: ProductServiceClient, merchantClient: MerchantServiceClient) {
        this.productClient = productClient;
        this.merchantClient = merchantClient;
    }

    public async getProduct(id: string): Promise<Optional<Product>> {
        const maybeProductResponse = await this.productClient.getProduct(id);

        if (maybeProductResponse.isEmpty()) {
            return Optional.empty();
        }

        const productResponse = maybeProductResponse.get();

        const response: Product = {
            id: productResponse.id,
            title: productResponse.name,
            metaData: productResponse.description,
            price: productResponse.price,
        }

        const maybeMerchantResponse = await this.merchantClient.getMerchant(productResponse.merchantId);

        if (maybeMerchantResponse.isEmpty()) {
            return Optional.ofNonNull(response);
        }

        const merchantResponse = maybeMerchantResponse.get();

        response.merchant = {
            name: merchantResponse.name,
            rating: merchantResponse.score,
            ...merchantResponse.communicationChannel
        };

        return Optional.ofNonNull(response);
    }
}

Implementation is clear and easy to test. However, there is a severe problem in the mapping section.

Analysis

In this section, the Spread operator merges the communicationChannel object into the merchant object.

You can use spread syntax to merge multiple objects into one new object. (Spread Operator)

response.merchant = {
    name: merchantResponse.name,
    rating: merchantResponse.score,
    ...merchantResponse.communicationChannel
};

This is an uncontrolled object mapping. So, what is the problem here?

After a few weeks, the merchant service introduced a new field called phone number under the communicationChannel object. This information is sensitive and should not have been exposed. However, due to the nature of the Spread syntax, it has been exposed. It was very difficult to discover that error because that modification did not break the integration and contract tests.

Improvements

In some cases, solving a bug is easier than finding it. To solve this bug, we can map all fields explicitly.

response.merchant = {
    name: merchantResponse.name,
    rating: merchantResponse.score,
    address: merchantResponse.communicationChannel.address,
    taxAddress: merchantResponse.communicationChannel.taxAddress,
};

Conclusion

It is worth remembering that such errors frequently occur in javascript and typescript-based applications.

Disclaimer

These are my opinions, and all feedback is welcomed!

Happy refactorings! 🚀 🎉 🥳