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.

Sign up for GitHub

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

fix: stop leaking optional query params and headers across subsequent… #51

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: stop leaking optional query params and headers across subsequent… #51

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

| Statements | Branches | Functions | Lines |
| ---------------------------------------- | ---------------------------------------- | ---------------------------------------- | ----------------------------------- |
| ![Statements](https://img.shields.io/badge/Coverage-93.98%25-brightgreen.svg "Make me better!") | ![Branches](https://img.shields.io/badge/Coverage-79.42%25-red.svg "Make me better!") | ![Functions](https://img.shields.io/badge/Coverage-87.58%25-yellow.svg "Make me better!") | ![Lines](https://img.shields.io/badge/Coverage-95.32%25-brightgreen.svg "Make me better!") |
| ![Statements](https://img.shields.io/badge/Coverage-93.98%25-brightgreen.svg "Make me better!") | ![Branches](https://img.shields.io/badge/Coverage-80.14%25-yellow.svg "Make me better!") | ![Functions](https://img.shields.io/badge/Coverage-87.58%25-yellow.svg "Make me better!") | ![Lines](https://img.shields.io/badge/Coverage-95.32%25-brightgreen.svg "Make me better!") |

> A declarative and [axios](https://github.com/axios/axios) based retrofit implementation for JavaScript and TypeScript.

Expand Down
12 changes: 6 additions & 6 deletions src/baseService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,17 +243,17 @@ export class BaseService {
@nonHTTPRequestMethod
private _resolveHeaders(methodName: string, args: any[]): any {
const meta = this.__meta__;
const headers = meta[methodName].headers || {};
const headers = { ...meta[methodName].headers };
const headerParams = meta[methodName].headerParams;
for (const pos in headerParams) {
if (headerParams[pos]) {
if (headerParams[pos] && args[pos] !== undefined) {
headers[headerParams[pos]] = args[pos];
}
}
const headerMapIndex = meta[methodName].headerMapIndex;
if (headerMapIndex >= 0) {
for (const key in args[headerMapIndex]) {
if (args[headerMapIndex][key]) {
if (args[headerMapIndex][key] !== undefined) {
headers[key] = args[headerMapIndex][key];
}
}
Expand All @@ -264,17 +264,17 @@ export class BaseService {
@nonHTTPRequestMethod
private _resolveQuery(methodName: string, args: any[]): any {
const meta = this.__meta__;
const query = meta[methodName].query || {};
Copy link
Author

@alechill alechill Dec 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Issue 1 - leaking across requests

If method has an @Queries decorator, this.__meta__[methodName].query exists and is pulled in by reference, so when appending new params to the query object these end up being persisted on the instance and so undesirably get pulled into the next request via this.__meta__[methodName].query

this.__meta__[methodName].query is only safe for and meant for static query params, cloning the query object before manipulation breaks the reference and fixes this issue

const query = { ...meta[methodName].query };
const queryParams = meta[methodName].queryParams;
for (const pos in queryParams) {
if (queryParams[pos]) {
if (queryParams[pos] && args[pos] !== undefined) {
Copy link
Author

@alechill alechill Dec 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Issue 2 - sending undefined as value

Once leak was fixed, optional params with no value specified would always be present as keys in query config object with value of undefined...

{ 
    myoptionalparam: undefined, 
    myrequiredparam: 'hello' 
}

This isn't desirable as given the query string /something?myoptionalparam=undefined a server would interpret this as a string with value of "undefined"

For headers this effect was worse as errored due to trying to call setHeader(undefined) which is not allowed

Avoiding adding unspecified optional query and header args to config avoids this, so they don't get sent with the request at all

query[queryParams[pos]] = args[pos];
}
}
const queryMapIndex = meta[methodName].queryMapIndex;
if (queryMapIndex >= 0) {
for (const key in args[queryMapIndex]) {
if (args[queryMapIndex][key]) {
if (args[queryMapIndex][key] !== undefined) {
Copy link
Author

Choose a reason for hiding this comment

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

Issue 3 - discarding legitimate falsy values

While this bit technically was already preventing undefined values being added to the config object, what it actually does is filter out all falsy values. Therefore empty strings "", number 0, bolean false etc which can all be legitimate values would be discarded. Using an explicit test for undefined allows legit values to pass through

query[key] = args[queryMapIndex][key];
}
}
Expand Down
28 changes: 28 additions & 0 deletions test/fixture/fixtures.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,34 @@ export class PostService extends BaseService {
@QueryArrayFormat('comma')
async getPostsWithQueryArrayFormatComma(@Query('groups') groups: string[]): Promise<Response> { return <Response>{} };

@GET("/posts")
@Queries({
page: 1,
size: 20,
sort: "createdAt:desc",
})
async getPostsWithOptionalQuery(@Query('since') since?: string): Promise<Response> { return <Response>{} };

@GET("/posts")
@Queries({
page: 1,
size: 20,
sort: "createdAt:desc",
})
async getPostsWithOptionalQueryMap(@QueryMap filters?: SearchQuery): Promise<Response> { return <Response>{} };

@GET("/posts")
@Headers({
'Cache-Control': 'no-cache'
})
async getPostsWithOptionalHeader(@Header('X-Correlation-Id') correlationId?: string): Promise<Response> { return <Response>{} };

@GET("/posts")
@Headers({
'Cache-Control': 'no-cache'
})
async getPostsWithOptionalHeaderMap(@HeaderMap headers?: any): Promise<Response> { return <Response>{} };

@POST("/posts")
@FormUrlEncoded
async createPost(@Field("title") title: string, @Field("content") content: string): Promise<Response> { return <Response>{} };
Expand Down
94 changes: 94 additions & 0 deletions test/ts-retrofit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,100 @@ describe("Test ts-retrofit.", () => {
expect(response.config.params).toMatchObject(query);
});

test("Test `@Query` decorator should not persist optional param across requests.", async () => {
const postsService = new ServiceBuilder()
.setEndpoint(TEST_SERVER_ENDPOINT)
.build(PostService);
const since1 = "2022-12-01"
const since3 = "2020-10-31"
const response1 = await postsService.getPostsWithOptionalQuery(since1);
const response2 = await postsService.getPostsWithOptionalQuery();
const response3 = await postsService.getPostsWithOptionalQuery(since3);
console.log(response1.config.params)
console.log(response2.config.params)
console.log(response3.config.params)
expect(response1.config.params.since).toEqual(since1);

expect(response2.config.params.since).toBeUndefined();
expect(Object.keys(response2.config.params)).not.toContain('since');

expect(response3.config.params.since).toEqual(since3);
});

test("Test `@QueryMap` decorator should not persist optional params across requests.", async () => {
const postsService = new ServiceBuilder()
.setEndpoint(TEST_SERVER_ENDPOINT)
.build(PostService);
const query1: SearchQuery = {
title: "TypeScript",
};
const query3: SearchQuery = {
author: "John Doe",
};
const response1 = await postsService.getPostsWithOptionalQueryMap(query1);
const response2 = await postsService.getPostsWithOptionalQueryMap();
const response3 = await postsService.getPostsWithOptionalQueryMap(query3);

expect(response1.config.params.title).toEqual(query1.title);
expect(response1.config.params.author).toBeUndefined();
expect(Object.keys(response1.config.params)).not.toContain('author');

expect(response2.config.params.title).toBeUndefined();
expect(Object.keys(response2.config.params)).not.toContain('title');
expect(response2.config.params.author).toBeUndefined();
expect(Object.keys(response2.config.params)).not.toContain('author');

expect(response3.config.params.title).toBeUndefined();
expect(Object.keys(response3.config.params)).not.toContain('title');
expect(response3.config.params.author).toEqual(query3.author);
});

test("Test `@Header` decorator should not persist optional header across requests.", async () => {
const postsService = new ServiceBuilder()
.setEndpoint(TEST_SERVER_ENDPOINT)
.build(PostService);
const correlationId1 = "1234abcd"
const correlationId3 = "9876zyxw"
const response1 = await postsService.getPostsWithOptionalHeader(correlationId1);
const response2 = await postsService.getPostsWithOptionalHeader();
const response3 = await postsService.getPostsWithOptionalHeader(correlationId3);

expect(response1.config.headers['X-Correlation-Id']).toEqual(correlationId1);

expect(response2.config.headers['X-Correlation-Id']).toBeUndefined();
expect(Object.keys(response2.config.headers)).not.toContain('X-Correlation-Id');

expect(response3.config.headers['X-Correlation-Id']).toEqual(correlationId3);
});

test("Test `@HeaderMap` decorator should not persist optional headers across requests.", async () => {
const postsService = new ServiceBuilder()
.setEndpoint(TEST_SERVER_ENDPOINT)
.build(PostService);
const headers1 = {
'X-Session-Id': "1234abcd"
};
const headers3 = {
'X-Correlation-Id': "9876zyxw",
};
const response1 = await postsService.getPostsWithOptionalHeaderMap(headers1);
const response2 = await postsService.getPostsWithOptionalHeaderMap();
const response3 = await postsService.getPostsWithOptionalHeaderMap(headers3);

expect(response1.config.headers['X-Session-Id']).toEqual(headers1['X-Session-Id']);
expect(response1.config.headers['X-Correlation-Id']).toBeUndefined();
expect(Object.keys(response1.config.headers)).not.toContain('X-Correlation-Id');

expect(response2.config.headers['X-Session-Id']).toBeUndefined();
expect(Object.keys(response2.config.headers)).not.toContain('X-Session-Id');
expect(response2.config.headers['X-Correlation-Id']).toBeUndefined();
expect(Object.keys(response2.config.headers)).not.toContain('X-Correlation-Id');

expect(response3.config.headers['X-Session-Id']).toBeUndefined();
expect(Object.keys(response3.config.headers)).not.toContain('X-Session-Id');
expect(response3.config.headers['X-Correlation-Id']).toEqual(headers3['X-Correlation-Id']);
});

test("Test `@FormUrlEncoded` decorator.", async () => {
const postService = new ServiceBuilder()
.setEndpoint(TEST_SERVER_ENDPOINT)
Expand Down