ronantakizawa/codereview-bench huggingface dataset:
Repository: denoland/fresh
File: src/middlewares/cors.ts (TypeScript)

Code before:
        status: 204,
        statusText: "No Content",
        headers: responseHeaders,
      });
    }

    // For non-OPTIONS requests
    const actualResponse = await ctx.next();

    // Apply common CORS headers (ACAO, Credentials, ExposeHeaders)
    responseHeaders.forEach((value, key) => {
      // Do not overwrite Vary if it was set by downstream, merge instead
      if (key.toLowerCase() !== "vary") {
        actualResponse.headers.set(key, value);
      }
    });

    // Merge our calculated varyValues with any existing Vary from downstream response
    const existingVary = actualResponse.headers.get("Vary");
    if (existingVary) {
      existingVary.split(/\s*,\s*/).forEach((v) => varyValues.add(v));
    }

    if (varyValues.size > 0) {
      actualResponse.headers.set("Vary", Array.from(varyValues).join(", "));
    }
    // If varyValues is empty and no existingVary, Vary header remains absent or as set by downstream.

    return actualResponse;
  };
}


Review comment (suggestion):
In the case where the downstream response has a `Vary` header, but we don't want to add any additional values in the cors middleware, we're still going to split it and set it back to the exact same value here. We should avoid that.

Code after:
      statusText: "No Content",
      headers,
    });
  };

  return async (ctx: FreshContext): Promise<Response> => {
    const requestOrigin = ctx.req.headers.get("origin") || "";
    const allowOrigin = findAllowOrigin(requestOrigin, ctx);

    const varyValues = new Set<string>();
    // Add 'Origin' to Vary if a specific origin is allowed, not '*'
    if (opts.origin !== "*" && allowOrigin && allowOrigin !== "*") {
      varyValues.add("Origin");
    }

    if (ctx.req.method === "OPTIONS") {
      return OptionsResponse(ctx, allowOrigin, opts, varyValues);
    }

    // For non-OPTIONS requests
    const res = await ctx.next();

    addHeaderProperties(
      res.headers,
      allowOrigin,
      opts,
    );

    // Merge our calculated varyValues with any existing Vary from downstream response
    if (varyValues.size > 0) {
      const existingVary = res.headers.get("Vary");
      if (existingVary) {
        existingVary.split(/\s*,\s*/).forEach((v) => varyValues.add(v));
      }

      res.headers.set("Vary", Array.from(varyValues).join(", "));
    }

    return res;
  };
}
Repository: denoland/fresh
File: src/middlewares/csrf.ts (TypeScript)

Code before:
import type { FreshContext } from "../context.ts";

type IsAllowedOriginHandler = (
  origin: string,
  context: FreshContext,
) => boolean;
export interface CSRFOptions {
  origin?: string | string[] | IsAllowedOriginHandler;
}

const isSafeMethodRe = /^(GET|HEAD)$/;
const isRequestedByFormElementRe =
  /^\b(application\/x-www-form-urlencoded|multipart\/form-data|text\/plain)\b/i;

/**
 * CSRF Protection Middleware for Fresh.
 * Adapted from Hono's CORS middleware.
 * Would like to express my gratitude to all the developers of Hono.
 *
 * @param {CSRFOptions} [options] - The options for the CSRF protection middleware.
 * @param {string|string[]|(origin: string, context: Context) => boolean} [options.origin] - Specify origins.
 * @returns {MiddlewareHandler} The middleware handler function.
 *
 * @example
 * ```ts
 * const app = new App<State>()
 *
 * app.use(csrf())

Review comment (bug):
This type is part of the public API through `CSRFOptions`. We need to export this too otherwise publishing will fail because of a private interface.

Code after:
import type { Context } from "../context.ts";
import { HttpError } from "../error.ts";
import type { Middleware } from "./mod.ts";

/** Options for {@linkcode csrf}. **/
// deno-lint-ignore no-explicit-any
export interface CsrfOptions<State = any> {
  /**
   * origin - Specifies the allowed origin(s) for requests.
   *  - string: A single allowed origin.
   *  - string[]: static allowed origins.
   *  - function: A function to determine if an origin is allowed.
   */
  origin?:
    | string
    | string[]
    | ((origin: string, context: Context<State>) => boolean);
}

/**
 * CSRF Protection Middleware for Fresh.
 *
 * @param options Options for the CSRF protection middleware.
 * @returns The middleware handler function.
 *
 * @example Basic usage (with defaults)
 * ```ts
 * const app = new App<State>()
Repository: denoland/fresh
File: src/middlewares/csrf.ts (TypeScript)

Code before:
import type { FreshContext } from "../context.ts";

export type IsAllowedOriginHandler = (
  origin: string,
  context: FreshContext,
) => boolean;
export interface CSRFOptions {
  origin?: string | string[] | IsAllowedOriginHandler;
}

const isSafeMethodRe = /^(GET|HEAD)$/;
const isRequestedByFormElementRe =
  /^\b(application\/x-www-form-urlencoded|multipart\/form-data|text\/plain)\b/i;

/**
 * CSRF Protection Middleware for Fresh.
 * Adapted from Hono's CORS middleware.
 * Would like to express my gratitude to all the developers of Hono.
 *
 * @param {CSRFOptions} [options] - The options for the CSRF protection middleware.
 * @param {string|string[]|(origin: string, context: Context) => boolean} [options.origin] - Specify origins.
 * @returns {MiddlewareHandler} The middleware handler function.
 *
 * @example
 * ```ts
 * const app = new App<State>()
 *
 * app.use(csrf())
 *
 * // Specifying origins with using `origin` option
 * // string
 * app.use(csrf({ origin: 'https://myapp.example.com' }))
 *
 * // string[]
 * app.use(
 *   csrf({
 *     origin: ['https://myapp.example.com', 'http://development.myapp.example.com'],
 *   })

Review comment (bug):
This is a personal preference but I like to avoid regexes as much as possible. They can be difficult to understand and can be buggy. Even the simple ones. I'd suggest:

```ts
function isSafeMethod(method: string): boolean {
  return method === "GET" || method === "POST";
}
```

Of even, for brevity:
```ts
function isSafeMethod(method: string): boolean {
  return ["GET", "POST"].includes(method);
}
```

You could even inline this logic. No need for separate functions.

Code after:
import type { Context } from "../context.ts";
import { HttpError } from "../error.ts";
import type { Middleware } from "./mod.ts";

/** Options for {@linkcode csrf}. **/
// deno-lint-ignore no-explicit-any
export interface CsrfOptions<State = any> {
  /**
   * origin - Specifies the allowed origin(s) for requests.
   *  - string: A single allowed origin.
   *  - string[]: static allowed origins.
   *  - function: A function to determine if an origin is allowed.
   */
  origin?:
    | string
    | string[]
    | ((origin: string, context: Context<State>) => boolean);
}

/**
 * CSRF Protection Middleware for Fresh.
 *
 * @param options Options for the CSRF protection middleware.
 * @returns The middleware handler function.
 *
 * @example Basic usage (with defaults)
 * ```ts
 * const app = new App<State>()
 *
 * app.use(csrf())
 * ```
 *
 * @example Specifying static origins
 * ```ts
 * app.use(csrf({ origin: 'https://myapp.example.com' }))
 *
 * // string[]
 * app.use(
Repository: denoland/fresh
File: src/middlewares/csrf.ts (TypeScript)

Code before:
import type { FreshContext } from "../context.ts";

export type IsAllowedOriginHandler = (
  origin: string,
  context: FreshContext,
) => boolean;
export interface CSRFOptions {
  origin?: string | string[] | IsAllowedOriginHandler;
}

const isSafeMethodRe = /^(GET|HEAD)$/;
const isRequestedByFormElementRe =
  /^\b(application\/x-www-form-urlencoded|multipart\/form-data|text\/plain)\b/i;

/**
 * CSRF Protection Middleware for Fresh.
 * Adapted from Hono's CORS middleware.
 * Would like to express my gratitude to all the developers of Hono.
 *
 * @param {CSRFOptions} [options] - The options for the CSRF protection middleware.
 * @param {string|string[]|(origin: string, context: Context) => boolean} [options.origin] - Specify origins.
 * @returns {MiddlewareHandler} The middleware handler function.
 *
 * @example
 * ```ts
 * const app = new App<State>()
 *
 * app.use(csrf())
 *
 * // Specifying origins with using `origin` option
 * // string
 * app.use(csrf({ origin: 'https://myapp.example.com' }))
 *
 * // string[]
 * app.use(
 *   csrf({
 *     origin: ['https://myapp.example.com', 'http://development.myapp.example.com'],
 *   })
 * )
 *
 * // Function
 * // It is strongly recommended that the protocol be verified to ensure a match to `$`.
 * // You should *never* do a forward match.
 * app.use(
 *   '*',
 *   csrf({
 *     origin: (origin) => /https:\/\/(\w+\.)?myapp\.example\.com$/.test(origin),

Review comment (security):
```suggestion
 * @param options The options for the CSRF protection middleware.
 * @returns The middleware handler function.
```
Type information isn't needed in JSDocs for TypeScript.

Code after:
import type { Context } from "../context.ts";
import { HttpError } from "../error.ts";
import type { Middleware } from "./mod.ts";

/** Options for {@linkcode csrf}. **/
// deno-lint-ignore no-explicit-any
export interface CsrfOptions<State = any> {
  /**
   * origin - Specifies the allowed origin(s) for requests.
   *  - string: A single allowed origin.
   *  - string[]: static allowed origins.
   *  - function: A function to determine if an origin is allowed.
   */
  origin?:
    | string
    | string[]
    | ((origin: string, context: Context<State>) => boolean);
}

/**
 * CSRF Protection Middleware for Fresh.
 *
 * @param options Options for the CSRF protection middleware.
 * @returns The middleware handler function.
 *
 * @example Basic usage (with defaults)
 * ```ts
 * const app = new App<State>()
 *
 * app.use(csrf())
 * ```
 *
 * @example Specifying static origins
 * ```ts
 * app.use(csrf({ origin: 'https://myapp.example.com' }))
 *
 * // string[]
 * app.use(
 *   csrf({
 *     origin: ['https://myapp.example.com', 'http://development.myapp.example.com'],
 *   })
 * )
 * ```
 *
 * @example Specifying more complex origins
 * ```ts
 * app.use(
Repository: denoland/fresh
File: src/middlewares/csrf.ts (TypeScript)

Code before:
    origin: string | undefined | null,
    ctx: FreshContext,
  ) => {
    if (origin === undefined || origin === null) {
      return false;
    }
    return handler(origin, ctx);
  };

  return async (ctx: FreshContext): Promise<Response> => {
    if (
      !isSafeMethodRe.test(ctx.req.method) &&
      isRequestedByFormElementRe.test(
        ctx.req.headers.get("content-type") || "text/plain",
      ) &&
      !isAllowedOrigin(ctx.req.headers.get("origin"), ctx)
    ) {
      return new Response(
        "Forbidden: CSRF protection failed. The request is not allowed from this origin.",
        {
          status: 403,
        },
      );
    }

    return await ctx.next();
  };
}


Review comment (bug):
```suggestion
  return (ctx: FreshContext): Promise<Response> => {
    if (
      !isSafeMethodRe.test(ctx.req.method) &&
      isRequestedByFormElementRe.test(
        ctx.req.headers.get("content-type") || "text/plain",
      ) &&
      !isAllowedOrigin(ctx.req.headers.get("origin"), ctx)
    ) {
      return new Response(
        "Forbidden: CSRF protection failed. The request is not allowed from this origin.",
        {
          status: 403,
        },
      );
    }

    return ctx.next();
```
Nit

Code after:

    if (!optsOrigin) {
      return origin === ctx.url.origin;
    }
    if (typeof optsOrigin === "string") {
      return origin === optsOrigin;
    }
    if (typeof optsOrigin === "function") {
      return optsOrigin(origin, ctx);
    }
    return Array.isArray(optsOrigin) && optsOrigin.includes(origin);
  };

  return async (ctx) => {
    const { method, headers } = ctx.req;

    // Safe methods
    if (method === "GET" || method === "HEAD" || method === "OPTIONS") {
      return await ctx.next();
    }

    const secFetchSite = headers.get("Sec-Fetch-Site");
    const origin = headers.get("origin");

    if (secFetchSite !== null) {
      if (
        secFetchSite === "same-origin" || secFetchSite === "none" ||
        isAllowedOrigin(origin, ctx)
      ) {
        return await ctx.next();
      }

      throw new HttpError(403);
    }

    // Neither `Sec-Fetch-Site` or `Origin` is set
    if (origin === null) {
      return await ctx.next();
    }

    if (isAllowedOrigin(origin, ctx)) {
      return await ctx.next();
    }

    throw new HttpError(403);
  };
}
Repository: denoland/fresh
File: src/middlewares/csrf.ts (TypeScript)

Code before:
import type { FreshContext } from "../context.ts";

/**
 * Determines whether a given origin is allowed for CSRF protection.
 *
 * @param origin - The origin of the incoming request.
 * @param context - The FreshContext object for the current request.
 * @returns A boolean indicating if the origin is allowed.
 */
export type IsAllowedOriginHandler = (
  origin: string,
  context: FreshContext,
) => boolean;

/**
 * Options for configuring the CSRF protection middleware.
 *
 * @property origin - Specifies the allowed origin(s) for requests. Can be a string, an array of strings, or a custom handler function.
 *   - string: A single allowed origin.
 *   - string[]: Multiple allowed origins.
 *   - IsAllowedOriginHandler: A function to determine if an origin is allowed.
 */
export interface CSRFOptions {
  origin?: string | string[] | IsAllowedOriginHandler;
}

function isSafeMethod(method: string): boolean {
  return method === "GET" || method === "HEAD";
}

function isRequestedByFormElement(contentType: string): boolean {
  const contentTypeLower = contentType.toLowerCase();

  return contentTypeLower === "application/x-www-form-urlencoded" ||
    contentTypeLower === "multipart/form-data" ||
    contentTypeLower === "text/plain";
}

/**
 * CSRF Protection Middleware for Fresh.
 * Adapted from Hono's CORS middleware.
 * Would like to express my gratitude to all the developers of Hono.
 *
 * @param options Options for the CSRF protection middleware.
 * @returns The middleware handler function.
 *
 * @example
 * ```ts

Review comment (security):
```suggestion
export interface CsrfOptions {
```
Names for public interfaces in the Deno runtime and Standard Library APIs use camelcasing, even for abbreviations. E.g. `Deno.serveHttp`, `Deno.listenTls`, etc. Ref: https://github.com/denoland/std/pull/2582

Code after:
import type { Context } from "../context.ts";
import { HttpError } from "../error.ts";
import type { Middleware } from "./mod.ts";

/** Options for {@linkcode csrf}. **/
// deno-lint-ignore no-explicit-any
export interface CsrfOptions<State = any> {
  /**
   * origin - Specifies the allowed origin(s) for requests.
   *  - string: A single allowed origin.
   *  - string[]: static allowed origins.
   *  - function: A function to determine if an origin is allowed.
   */
  origin?:
    | string
    | string[]
    | ((origin: string, context: Context<State>) => boolean);
}

/**
 * CSRF Protection Middleware for Fresh.
 *
 * @param options Options for the CSRF protection middleware.
 * @returns The middleware handler function.
 *
 * @example Basic usage (with defaults)
 * ```ts
 * const app = new App<State>()
 *
 * app.use(csrf())
 * ```
 *
 * @example Specifying static origins
 * ```ts
 * app.use(csrf({ origin: 'https://myapp.example.com' }))
 *
 * // string[]
 * app.use(
 *   csrf({
 *     origin: ['https://myapp.example.com', 'http://development.myapp.example.com'],
 *   })
 * )
 * ```
 *
 * @example Specifying more complex origins
 * ```ts
 * app.use(
 *   '*',
Repository: denoland/fresh
File: src/middlewares/csrf.ts (TypeScript)

Code before:
 */
export function csrf(
  options?: CSRFOptions,
): (ctx: FreshContext) => Promise<Response> {
  const handler: IsAllowedOriginHandler = ((optsOrigin) => {
    if (!optsOrigin) {
      return (origin, ctx) => origin === new URL(ctx.req.url).origin;
    } else if (typeof optsOrigin === "string") {
      return (origin) => origin === optsOrigin;
    } else if (typeof optsOrigin === "function") {
      return optsOrigin;
    } else {
      return (origin) => optsOrigin.includes(origin);
    }
  })(options?.origin);
  const isAllowedOrigin = (
    origin: string | undefined | null,
    ctx: FreshContext,
  ) => {
    if (origin === undefined || origin === null) {
      return false;
    }
    return handler(origin, ctx);
  };

  return async (ctx: FreshContext): Promise<Response> => {
    if (
      !isSafeMethod(ctx.req.method) &&
      isRequestedByFormElement(
        ctx.req.headers.get("content-type") || "text/plain",
      ) &&
      !isAllowedOrigin(ctx.req.headers.get("origin"), ctx)
    ) {
      return new Response(
        "Forbidden: CSRF protection failed. The request is not allowed from this origin.",
        {
          status: 403,
        },
      );
    }

    return await ctx.next();
  };
}


Review comment (suggestion):
```suggestion
  return (ctx: FreshContext): Promise<Response> => {
```
Again, doesn't seem to need to be async here.

Code after:
    if (typeof optsOrigin === "function") {
      return optsOrigin(origin, ctx);
    }
    return Array.isArray(optsOrigin) && optsOrigin.includes(origin);
  };

  return async (ctx) => {
    const { method, headers } = ctx.req;

    // Safe methods
    if (method === "GET" || method === "HEAD" || method === "OPTIONS") {
      return await ctx.next();
    }

    const secFetchSite = headers.get("Sec-Fetch-Site");
    const origin = headers.get("origin");

    if (secFetchSite !== null) {
      if (
        secFetchSite === "same-origin" || secFetchSite === "none" ||
        isAllowedOrigin(origin, ctx)
      ) {
        return await ctx.next();
      }

      throw new HttpError(403);
    }

    // Neither `Sec-Fetch-Site` or `Origin` is set
    if (origin === null) {
      return await ctx.next();
    }

    if (isAllowedOrigin(origin, ctx)) {
      return await ctx.next();
    }

    throw new HttpError(403);
  };
}
Repository: denoland/fresh
File: src/middlewares/csrf.ts (TypeScript)

Code before:
    }
  })(options?.origin);
  const isAllowedOrigin = (
    origin: string | undefined | null,
    ctx: FreshContext,
  ) => {
    if (origin === undefined || origin === null) {
      return false;
    }
    return handler(origin, ctx);
  };

  return async (ctx: FreshContext): Promise<Response> => {
    if (
      !isSafeMethod(ctx.req.method) &&
      isRequestedByFormElement(
        ctx.req.headers.get("content-type") || "text/plain",
      ) &&
      !isAllowedOrigin(ctx.req.headers.get("origin"), ctx)
    ) {
      return new Response(
        "Forbidden: CSRF protection failed. The request is not allowed from this origin.",
        {
          status: 403,
        },
      );
    }

    return await ctx.next();
  };
}


Review comment (bug):
```suggestion
      throw new HttpError(403, "CSRF protection failed. The request is not allowed from this origin.");
```
Allows the user to handle the error in their own way.

Code after:

    const secFetchSite = headers.get("Sec-Fetch-Site");
    const origin = headers.get("origin");

    if (secFetchSite !== null) {
      if (
        secFetchSite === "same-origin" || secFetchSite === "none" ||
        isAllowedOrigin(origin, ctx)
      ) {
        return await ctx.next();
      }

      throw new HttpError(403);
    }

    // Neither `Sec-Fetch-Site` or `Origin` is set
    if (origin === null) {
      return await ctx.next();
    }

    if (isAllowedOrigin(origin, ctx)) {
      return await ctx.next();
    }

    throw new HttpError(403);
  };
}
Repository: denoland/fresh
File: src/middlewares/csrf.ts (TypeScript)

Code before:
 * )
 * ```
 */
export function csrf(
  options?: CSRFOptions,
): (ctx: FreshContext) => Promise<Response> {
  const handler: IsAllowedOriginHandler = ((optsOrigin) => {
    if (!optsOrigin) {
      return (origin, ctx) => origin === new URL(ctx.req.url).origin;
    } else if (typeof optsOrigin === "string") {
      return (origin) => origin === optsOrigin;
    } else if (typeof optsOrigin === "function") {
      return optsOrigin;
    } else {
      return (origin) => optsOrigin.includes(origin);
    }
  })(options?.origin);
  const isAllowedOrigin = (
    origin: string | undefined | null,
    ctx: FreshContext,
  ) => {
    if (origin === undefined || origin === null) {
      return false;
    }
    return handler(origin, ctx);
  };

  return async (ctx: FreshContext): Promise<Response> => {
    if (
      !isSafeMethod(ctx.req.method) &&
      isRequestedByFormElement(
        ctx.req.headers.get("content-type") || "text/plain",
      ) &&
      !isAllowedOrigin(ctx.req.headers.get("origin"), ctx)
    ) {
      return new Response(
        "Forbidden: CSRF protection failed. The request is not allowed from this origin.",
        {
          status: 403,
        },
      );
    }

    return await ctx.next();
  };
}


Review comment (suggestion):
This is hard to follow, to me, at least. Would this work?
```suggestion
  const isAllowedOrigin = (
    origin: string | undefined | null,
    ctx: FreshContext,
  ) => {
    if (!origin) return false;
    const optsOrigin = options?.origin;
    if (!optsOrigin) {
      return origin === new URL(ctx.req.url).origin;
    }
    if (typeof optsOrigin === "string") {
      return origin === optsOrigin;
    }
    if (typeof optsOrigin === "function") {
      return optsOrigin(origin, ctx);
    }
    return Array.isArray(optsOrigin) && optsOrigin.includes(origin);
  };
```

Code after:
      return origin === optsOrigin;
    }
    if (typeof optsOrigin === "function") {
      return optsOrigin(origin, ctx);
    }
    return Array.isArray(optsOrigin) && optsOrigin.includes(origin);
  };

  return async (ctx) => {
    const { method, headers } = ctx.req;

    // Safe methods
    if (method === "GET" || method === "HEAD" || method === "OPTIONS") {
      return await ctx.next();
    }

    const secFetchSite = headers.get("Sec-Fetch-Site");
    const origin = headers.get("origin");

    if (secFetchSite !== null) {
      if (
        secFetchSite === "same-origin" || secFetchSite === "none" ||
        isAllowedOrigin(origin, ctx)
      ) {
        return await ctx.next();
      }

      throw new HttpError(403);
    }

    // Neither `Sec-Fetch-Site` or `Origin` is set
    if (origin === null) {
      return await ctx.next();
    }

    if (isAllowedOrigin(origin, ctx)) {
      return await ctx.next();
    }

    throw new HttpError(403);
  };
}