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

Should a SSE connection be closed on `UnauthorizedException`, even if the `ClassSerializerInterceptor` is used? #9810

Closed iamFIREcracker closed 2 years ago

iamFIREcracker commented 2 years ago

Is there an existing issue for this?

Current behavior

I have a @Sse handler which optionally throws an UnauthorizedException, and when it does it, the exception gets serialized and sent to the client as a server-sent event; however, shouldn't Nest simply close the connection and send the serialized exception within the body of the response? Otherwise I am afraid there migth be the risk of un-authorized requests consuming server resources.

Minimum reproduction code

https://github.com/iamFIREcracker/nestjs-sse-error-min-reproduction

Steps to reproduce

  1. npm ci
  2. npm run start:dev
  3. curl -v http://127.0.0.1:3000/sse

Expected behavior

I would expect the server to close the connection with a 401 status code:

*   Trying 127.0.0.1:3000...
* TCP_NODELAY set
* Connected to 127.0.0.1 (127.0.0.1) port 3000 (#0)
> GET /sse HTTP/1.1
> Host: 127.0.0.1:3000
> User-Agent: curl/7.68.0
> Accept: */*
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 401 Unauthorized
< X-Powered-By: Express
< Content-Type: application/json; charset=utf-8
< Content-Length: 43
< ETag: W/"2b-hGShxOkieaAVDloBubJVM+h58D8"
< Date: Mon, 20 Jun 2022 04:58:18 GMT
< Connection: keep-alive
< Keep-Alive: timeout=5
<
* Connection #0 to host 127.0.0.1 left intact
{"statusCode":401,"message":"Unauthorized"}

What happens instead, the exception gets serialized as server-sent event, and the connection left open:

*   Trying 127.0.0.1:3000...
* TCP_NODELAY set
* Connected to 127.0.0.1 (127.0.0.1) port 3000 (#0)
> GET /sse HTTP/1.1
> Host: 127.0.0.1:3000
> User-Agent: curl/7.68.0
> Accept: */*
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< x-powered-by: Express
< Content-Type: text/event-stream
< Connection: keep-alive
< Cache-Control: private, no-cache, no-store, must-revalidate, max-age=0, no-transform
< Pragma: no-cache
< Expire: 0
< X-Accel-Buffering: no
< Date: Mon, 20 Jun 2022 04:59:18 GMT
< Transfer-Encoding: chunked
<

event: error
id: 1
data: Unauthorized

* Connection #0 to host 127.0.0.1 left intact

Package

Other package

class-transformer

NestJS version

8.4.6

Packages versions

{
  "name": "nest-typescript-starter",
  "version": "1.0.0",
  "description": "Nest TypeScript starter repository",
  "license": "MIT",
  "scripts": {
    "prebuild": "rimraf dist",
    "build": "nest build",
    "format": "prettier --write \"src/**/*.ts\" \"test/**/*.ts\"",
    "start": "nest start",
    "start:dev": "nest start --watch",
    "start:debug": "nest start --debug --watch",
    "start:prod": "node dist/main",
    "lint": "eslint '{src,apps,libs,test}/**/*.ts' --fix",
    "test": "jest",
    "test:watch": "jest --watch",
    "test:cov": "jest --coverage",
    "test:debug": "node --inspect-brk -r tsconfig-paths/register -r ts-node/register node_modules/.bin/jest --runInBand",
    "test:e2e": "echo 'No e2e tests implemented yet.'"
  },
  "dependencies": {
    "@nestjs/common": "^8.4.6",
    "@nestjs/core": "^8.4.6",
    "@nestjs/platform-express": "^8.4.6",
    "class-transformer": "^0.5.1",
    "reflect-metadata": "0.1.13",
    "rimraf": "3.0.2",
    "rxjs": "7.4.0"
  },
  "devDependencies": {
    "@nestjs/cli": "8.1.5",
    "@nestjs/schematics": "8.0.8",
    "@nestjs/testing": "8.2.3",
    "@types/dotenv": "8.2.0",
    "@types/express": "4.17.13",
    "@types/jest": "27.4.1",
    "@types/node": "16.11.26",
    "@types/supertest": "2.0.11",
    "@typescript-eslint/eslint-plugin": "4.33.0",
    "@typescript-eslint/parser": "4.33.0",
    "eslint": "7.32.0",
    "eslint-config-prettier": "8.3.0",
    "eslint-plugin-import": "2.25.4",
    "jest": "27.5.1",
    "prettier": "2.5.1",
    "supertest": "6.1.6",
    "ts-jest": "27.0.7",
    "ts-loader": "9.2.8",
    "ts-node": "10.4.0",
    "tsconfig-paths": "3.11.0",
    "typescript": "4.3.5"
  },
  "jest": {
    "moduleFileExtensions": [
      "js",
      "json",
      "ts"
    ],
    "rootDir": "src",
    "testRegex": ".spec.ts$",
    "transform": {
      "^.+\\.(t|j)s$": "ts-jest"
    },
    "coverageDirectory": "../coverage",
    "testEnvironment": "node"
  }
}

Node.js version

18.1.0

In which operating systems have you tested?

Other

With version 8.2.3 the situation is even worse, given that an UnauthorizedException will cause the application to crash.

            | Class
    Nest.js | Serializer  |
    version | Interceptor | Effects
   ---------+-------------+-------------------------------------------------
   8.2.3    | No          | - Exception logged by default exception filter
            |             | - Connection closed w/ 500
   ---------+-------------+-------------------------------------------------
   8.2.3    | Yes         | - Exception **not** logged by default exception filter
            |             | - Application crashes
   ---------+-------------+-------------------------------------------------
   8.4.6    | No          | - Exception **not** logged by default exception filter
   ---------+-------------+-------------------------------------------------
   8.4.6    | Yes         | - Exception **not** logged by default exception filter
            |             | - Connection opened, and exception serialized
            |             |   as server-sent event
kamilmysliwiec commented 2 years ago

I have a @Sse handler which optionally throws an UnauthorizedException, and when it does it, the exception gets serialized and sent to the client as a server-sent event

That's the expected behavior.

Please, use our Discord channel (support) for such questions. We are using GitHub to track bugs, feature requests, and potential improvements.