Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 3 additions & 1 deletion packages/vitest-response-matchers/src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ declare namespace matchers {
toHaveStrictStatusText(): R;
toHaveTextBody(expected: string | null): R;
toMatchResponse(expected: Response | ResponseInit): R;
toThrowResponse: (expected: Response | ResponseInit) => R;
toThrowResponse(
expected: Response | { status: number; statusText: string },
): R;
}
}

Expand Down
14 changes: 5 additions & 9 deletions packages/vitest-response-matchers/src/matchers/to-have-body.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,12 @@
import type { MatcherResult } from "./matcher";
import { verifyResponse } from "#src/utils.ts";
import type { MatcherResult } from "./types";

export function toHaveBody(response: Response): MatcherResult {
if (!(response instanceof Response)) {
return {
message: () => `Expected a Response, but received ${typeof response}`,
pass: false,
};
}
verifyResponse(response);
Copy link
Contributor

Choose a reason for hiding this comment

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

high

The call to verifyResponse(response) is incorrect because its return value is ignored. This effectively removes the runtime type check that was present in the previous version of the code.

Assuming verifyResponse function returns a MatcherResult on failure and undefined on success, it should be used like this to properly handle the validation:

const verification = verifyResponse(response);
if (verification) {
  return verification;
}

This pattern should be applied to all other matchers where this refactoring was performed.


return {
message: () => `Expected response to have body`,
pass: response.body !== null,
pass: typeof response.body !== "undefined",
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

The check typeof response.body !== "undefined" is not the correct way to determine if a Response has a body. The body property on a Response object is either a ReadableStream or null, but it's never undefined. This means the current check will always evaluate to true.

The correct way to check for the presence of a body stream is to check if it's not null.

Suggested change
pass: typeof response.body !== "undefined",
pass: response.body !== null,

};
}

Expand All @@ -24,7 +20,7 @@ if (import.meta.vitest) {
expect(response).toHaveBody();
});

it.fails("fails when body is null", () => {
it("allows body to be null", () => {
let response = new Response(null);
expect(response).toHaveBody();
});
Expand Down
37 changes: 31 additions & 6 deletions packages/vitest-response-matchers/src/matchers/to-have-cookies.ts
Original file line number Diff line number Diff line change
@@ -1,18 +1,36 @@
import { getHeaders } from "#src/utils.ts";
import { SetCookie } from "@mjackson/headers";
import type { MatcherResult } from "./matcher";
import type { MatcherResult } from "./types";

export function toHaveCookies(
response: Response | ResponseInit,
response: Response | { headers?: HeadersInit },
cookies: Array<string>,
options?: { strict?: boolean },
): MatcherResult {
let headers =
response.headers instanceof Headers
? response.headers
: new Headers(response.headers);
let headers = getHeaders(response);

let responseCookies = headers.get("set-cookie");

if (!responseCookies && cookies.length > 0) {
return {
pass: false,
message: () => {
return `Expected "Set-Cookie" header to be present, but it was not found`;
},
actual: headers.get("set-cookie"),
expected: cookies,
};
}

if (!responseCookies && cookies.length === 0) {
return {
pass: true,
message: () => `Expected no cookies, and none were found.`,
actual: [],
expected: cookies,
};
}

if (!responseCookies) {
return {
pass: false,
Expand Down Expand Up @@ -57,6 +75,13 @@ if (import.meta.vitest) {
expect(response).toHaveCookies(["sessionId=abc123; Path=/"]);
});

it.each([new Response("Hello, world!"), {}])(
"passes when no cookies are expected",
(responseOrResponseInit) => {
expect(responseOrResponseInit).toHaveCookies([]);
},
);

it("toHaveCookies matcher with inline headers", () => {
expect({
headers: { "Set-Cookie": "sessionId=abc123; Path=/" },
Expand Down
Original file line number Diff line number Diff line change
@@ -1,14 +1,12 @@
import type { MatcherResult } from "./matcher";
import { getHeaders } from "#src/utils.ts";
import type { MatcherResult } from "./types";

export function toHaveHeader(
response: Response | ResponseInit,
header: string,
expected?: string,
): MatcherResult {
let headers =
response.headers instanceof Headers
? response.headers
: new Headers(response.headers);
let headers = getHeaders(response);

if (expected == undefined) {
return {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,10 @@
import { verifyResponse } from "#src/utils.ts";

export async function toHaveJsonBody(
response: Response,
expected: object | null,
) {
if (!(response instanceof Response)) {
return {
message: () => `Expected a Response, but received ${typeof response}`,
pass: false,
};
}
verifyResponse(response);

let body = await response.clone().json();

Expand Down
Original file line number Diff line number Diff line change
@@ -1,15 +1,11 @@
import type { MatcherResult } from "./matcher";
import { verifyResponse } from "#src/utils.ts";
import type { MatcherResult } from "./types";

export function toHaveStatusText(
response: Response,
statusText?: string,
statusText: string,
): MatcherResult {
if (!(response instanceof Response)) {
return {
message: () => `Expected a Response, but received ${typeof response}`,
pass: false,
};
}
verifyResponse(response);

if (typeof statusText === "undefined") {
return {
Expand Down
10 changes: 3 additions & 7 deletions packages/vitest-response-matchers/src/matchers/to-have-status.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,11 @@
import type { MatcherResult } from "./matcher";
import { verifyResponse } from "#src/utils.ts";
import type { MatcherResult } from "./types";

export function toHaveStatus(
response: Response,
status?: number,
): MatcherResult {
if (!(response instanceof Response)) {
return {
message: () => `Expected a Response, but received ${typeof response}`,
pass: false,
};
}
verifyResponse(response);

if (typeof status === "undefined") {
status = 200;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,9 @@
import { verifyResponse } from "#src/utils.ts";
import { STATUS_CODES } from "node:http";
import type { MatcherResult } from "./matcher";
import type { MatcherResult } from "./types";

export function toHaveStrictStatusText(response: Response): MatcherResult {
if (!(response instanceof Response)) {
return {
message: () => `Expected a Response, but received ${typeof response}`,
pass: false,
};
}
verifyResponse(response);

let found = STATUS_CODES[response.status];

Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,10 @@
import { verifyResponse } from "#src/utils.ts";

export async function toHaveTextBody(
response: Response,
expected: string | null,
) {
if (!(response instanceof Response)) {
return {
message: () => `Expected a Response, but received ${typeof response}`,
pass: false,
};
}
verifyResponse(response);

let body = await response.clone().text();

Expand Down
Original file line number Diff line number Diff line change
@@ -1,15 +1,11 @@
import type { MatcherResult } from "./matcher";
import { verifyResponse } from "#src/utils.ts";
import type { MatcherResult } from "./types";

export function toMatchResponse(
received: Response,
expected: { status: number; statusText: string },
): MatcherResult {
if (!(received instanceof Response)) {
return {
message: () => `Expected a Response, but received ${typeof received}`,
pass: false,
};
}
verifyResponse(received);

return {
message() {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { MatcherResult } from "./matcher";
import type { MatcherResult } from "./types";

export function toThrowResponse(
received: () => Response,
Expand All @@ -8,7 +8,7 @@ export function toThrowResponse(

try {
received();
} catch (e: unknown) {
} catch (e) {
error = e;
}

Expand All @@ -27,7 +27,6 @@ export function toThrowResponse(
return {
message: () => `Expected to throw a Response`,
pass:
error instanceof Response &&
error.status === expectedResponse.status &&
error.statusText === expectedResponse.statusText,
Comment on lines 29 to 31
Copy link
Contributor

Choose a reason for hiding this comment

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

high

Removing the error instanceof Response check can lead to runtime errors. It introduces two potential problems:

  1. If the received() function does not throw an error, error will be undefined. Accessing error.status will then cause a TypeError.
  2. If a non-Response error is thrown, the checks will be performed on an object that doesn't have status or statusText properties, leading to incorrect results.

The instanceof check is essential for ensuring the thrown object is of the correct type before accessing its properties.

Suggested change
pass:
error instanceof Response &&
error.status === expectedResponse.status &&
error.statusText === expectedResponse.statusText,
pass:
error instanceof Response &&
error.status === expectedResponse.status &&
error.statusText === expectedResponse.statusText,

actual: { status: error.status, statusText: error.statusText },
Expand Down
11 changes: 11 additions & 0 deletions packages/vitest-response-matchers/src/utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
export function getHeaders(response: Response | { headers?: HeadersInit }) {
if (response.headers instanceof Headers) return response.headers;
return new Headers(response.headers);
}

export function verifyResponse(response: unknown) {
return {
message: () => `Expected a Response, but received ${typeof response}`,
pass: !(response instanceof Response),
};
}
Comment on lines +6 to +11
Copy link
Contributor

Choose a reason for hiding this comment

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

high

The verifyResponse utility function has a potential flaw:

The pass property is set to !(response instanceof Response). This means if a non-Response object is passed (a failure case), pass becomes true. For Vitest matchers, pass: true indicates a successful assertion, which contradicts the failure message.

A better approach would be for this function to return a MatcherResult object on failure and undefined on success. The calling matcher can then check the return value and fail early if needed.

Suggested change
export function verifyResponse(response: unknown) {
return {
message: () => `Expected a Response, but received ${typeof response}`,
pass: !(response instanceof Response),
};
}
export function verifyResponse(response: unknown): { pass: false; message: () => string } | undefined {
if (!(response instanceof Response)) {
return {
message: () => `Expected a Response, but received ${typeof response}`,
pass: false,
};
}
}