nestjs / nest

A progressive Node.js framework for building efficient, scalable, and enterprise-grade server-side applications with TypeScript/JavaScript 🚀
https://nestjs.com
MIT License
67.74k stars 7.63k forks source link

URIError thrown instead of BadRequestException when visiting a URL with invalid percent encoding (Express) #8915

Closed StevenGBrown closed 2 years ago

StevenGBrown commented 2 years ago

Is there an existing issue for this?

Current behavior

Given a NestJS app using Express and any endpoint with a path parameter:

import { Controller, Get } from '@nestjs/common';

@Controller()
export class AppController {
  @Get(':param')
  get(): string {
    return 'Hello World!';
  }
}

Call the endpoint and provide an invalid percent encoding for the path parameter.
e.g. GET http://localhost:3000/%FF

Anything above 7F is not valid ASCII, therefore the endpoint responds with a 400 (bad request) code as expected.

However the exception filters receive a URIError instead of a BadRequestException. And as a result, the default exception filter logs the error, even though it doesn't usually log client errors (4xx).

Minimum reproduction code

https://stackblitz.com/edit/nestjs-typescript-starter-nhv5qm?devtoolsheight=33&file=src/app.controller.ts

Steps to reproduce

  1. npm start
  2. Visit a URL with an invalid percent encoding in the path parameter, e.g. https://nestjs-typescript-starter-nhv5qm--3000.local.webcontainer.io/%FF.
  3. The endpoint responds with a 400 (bad request) code as expected: {"statusCode":400,"message":"Failed to decode param '%FF'"}.
  4. However, a URIError is logged to the terminal:
    [Nest] 19  - 13/01/2022, 17:20:00   ERROR [ExceptionsHandler] Failed to decode param '%FF'
    URIError: Failed to decode param '%FF'
       at decodeURIComponent (<anonymous>)
       at decode_param (/home/projects/nestjs-typescript-starter-nhv5qm/node_modules/express/lib/router/layer.js:172:12)
       at Layer.match (/home/projects/nestjs-typescript-starter-nhv5qm/node_modules/express/lib/router/layer.js:148:15)
       at matchLayer (/home/projects/nestjs-typescript-starter-nhv5qm/node_modules/express/lib/router/index.js:574:18)
       at next (/home/projects/nestjs-typescript-starter-nhv5qm/node_modules/express/lib/router/index.js:220:15)
       at urlencodedParser (/home/projects/nestjs-typescript-starter-nhv5qm/node_modules/body-parser/lib/types/urlencoded.js:100:7)
       at Layer.handle [as handle_request] (/home/projects/nestjs-typescript-starter-nhv5qm/node_modules/express/lib/router/layer.js:95:5)
       at trim_prefix (/home/projects/nestjs-typescript-starter-nhv5qm/node_modules/express/lib/router/index.js:317:13)
       at eval (/home/projects/nestjs-typescript-starter-nhv5qm/node_modules/express/lib/router/index.js:284:7)
       at Function.process_params (/home/projects/nestjs-typescript-starter- nhv5qm/node_modules/express/lib/router/index.js:335:12)

Expected behavior

Package

Other package

No response

NestJS version

8.1.1, 8.2.4

Packages versions

[System Information]
OS Version     : macOS Monterey
NodeJS Version : v14.17.0
NPM Version    : 8.1.4 

[Nest CLI]
Nest CLI Version : 8.1.8 

[Nest Platform Information]
platform-express version : 8.2.4
schematics version       : 8.0.5
testing version          : 8.2.4
common version           : 8.2.4
core version             : 8.2.4
cli version              : 8.1.8
[System Information]
OS Version     : Linux
NodeJS Version : v14.16.0
NPM Version    : 7.17.0

[Nest CLI]
Nest CLI Version : 8.1.3 

[Nest Platform Information]
platform-express version : 8.1.1
schematics version       : 8.0.4
testing version          : 8.1.1
common version           : 8.1.1
core version             : 8.1.1
cli version              : 8.1.3

Node.js version

14.16.0, 14.17.0

In which operating systems have you tested?

Other

Workaround

I've been using this exception filter as a workaround:

import { ArgumentsHost, BadRequestException, Catch, HttpStatus } from '@nestjs/common'
import { BaseExceptionFilter } from '@nestjs/core'

@Catch()
export class ExpressExceptionFilter extends BaseExceptionFilter {
  catch(exception: unknown, host: ArgumentsHost): void {
    super.catch(this.replaceException(exception), host)
  }

  private replaceException<T>(err: T): T | BadRequestException {
    const { message, statusCode } = (err || {}) as { message?: unknown; statusCode?: unknown }
    if (err instanceof URIError && typeof message === 'string' && statusCode === HttpStatus.BAD_REQUEST) {
      return new BadRequestException(message)
    }
    return err
  }
}

And installing it as a global filter on the Nest application:

app.useGlobalFilters(new ExpressExceptionFilter(app.get(HttpAdapterHost).httpAdapter))

Fastify

Fastify is not affected because it responds to these requests within the library instead of throwing an error.

https://github.com/fastify/fastify/blob/d320417a2fb8d715cbdc0c729a1a334090d5ae82/fastify.js#L595

kamilmysliwiec commented 2 years ago

Let's track this here https://github.com/nestjs/nest/pull/8916/files