From 9fa0bcc705292fbb9ab84d9b717c3935a46b08f6 Mon Sep 17 00:00:00 2001 From: ree Date: Mon, 30 Oct 2023 09:23:30 +0000 Subject: [PATCH] Properly re-assemble all data in http requests before handling it (!162) Remove unneeded HttpBufferHandler ----------- The old code processed each chunk of data as an entire request, which is not correct. It was observed split data after ~14600 bytes (on a 1 gig lan connection). I think it was worse on remote connections. This was the cause of the "unknown compression method" and invalid json parse errors when saving the profile. Co-authored-by: Decoy Reviewed-on: https://dev.sp-tarkov.com/SPT-AKI/Server/pulls/162 Reviewed-by: Terkoiz Co-authored-by: ree Co-committed-by: ree --- project/src/servers/http/AkiHttpListener.ts | 76 +++++++++---------- project/src/servers/http/HttpBufferHandler.ts | 36 --------- 2 files changed, 35 insertions(+), 77 deletions(-) delete mode 100644 project/src/servers/http/HttpBufferHandler.ts diff --git a/project/src/servers/http/AkiHttpListener.ts b/project/src/servers/http/AkiHttpListener.ts index efb950e9..3e619bb9 100644 --- a/project/src/servers/http/AkiHttpListener.ts +++ b/project/src/servers/http/AkiHttpListener.ts @@ -5,7 +5,6 @@ import { inject, injectAll, injectable } from "tsyringe"; import { Serializer } from "@spt-aki/di/Serializer"; import { ILogger } from "@spt-aki/models/spt/utils/ILogger"; import { HttpRouter } from "@spt-aki/routers/HttpRouter"; -import { HttpBufferHandler } from "@spt-aki/servers/http/HttpBufferHandler"; import { IHttpListener } from "@spt-aki/servers/http/IHttpListener"; import { LocalisationService } from "@spt-aki/services/LocalisationService"; import { HttpResponseUtil } from "@spt-aki/utils/HttpResponseUtil"; @@ -22,20 +21,18 @@ export class AkiHttpListener implements IHttpListener @inject("RequestsLogger") protected requestsLogger: ILogger, @inject("JsonUtil") protected jsonUtil: JsonUtil, @inject("HttpResponseUtil") protected httpResponse: HttpResponseUtil, - @inject("LocalisationService") protected localisationService: LocalisationService, - @inject("HttpBufferHandler") protected httpBufferHandler: HttpBufferHandler + @inject("LocalisationService") protected localisationService: LocalisationService ) { } - public canHandle(_: string, req: IncomingMessage): boolean + public canHandle(_: string, req: IncomingMessage): boolean { return req.method === "GET" || req.method === "PUT" || req.method === "POST"; } public handle(sessionId: string, req: IncomingMessage, resp: ServerResponse): void { - // TODO: cleanup into interface IVerbHandler switch (req.method) { case "GET": @@ -44,51 +41,48 @@ export class AkiHttpListener implements IHttpListener this.sendResponse(sessionId, req, resp, null, response); break; } + + // these are handled almost identically. case "POST": - { - req.on("data", (data: any) => - { - const value = (req.headers["debug"] === "1") ? data.toString() : zlib.inflateSync(data); - const response = this.getResponse(sessionId, req, value); - this.sendResponse(sessionId, req, resp, value, response); - }); - break; - } case "PUT": { - req.on("data", (data) => - { - // receive data - if ("expect" in req.headers) - { - const requestLength = parseInt(req.headers["content-length"]); - - if (!this.httpBufferHandler.putInBuffer(req.headers.sessionid, data, requestLength)) - { - resp.writeContinue(); - } - } + // Data can come in chunks. Notably, if someone saves their profile (which can be + // kinda big), on a slow connection. We need to re-assemble the entire http payload + // before processing it. + + const requestLength = parseInt(req.headers["content-length"]); + const buffer = Buffer.alloc(requestLength); + let written = 0; + + req.on("data", (data: any) => { + data.copy(buffer, written, 0); + written += data.length; }); - - req.on("end", async () => + + req.on("end", () => { - const data = this.httpBufferHandler.getFromBuffer(sessionId); - this.httpBufferHandler.resetBuffer(sessionId); - - let value = zlib.inflateSync(data); - if (!value) + // Contrary to reasonable expectations, the content-encoding is _not_ actually used to + // determine if the payload is compressed. All PUT requests are, and POST requests without + // debug = 1 are as well. This should be fixed. + // let compressed = req.headers["content-encoding"] === "deflate"; + let compressed = req.method === "PUT" || req.headers["debug"] !== "1"; + + const value = compressed ? zlib.inflateSync(buffer) : buffer; + if (req.headers["debug"] === "1") { - value = data; + console.log(value.toString()); } + const response = this.getResponse(sessionId, req, value); this.sendResponse(sessionId, req, resp, value, response); }); + break; } + default: { - - this.logger.warning(this.localisationService.getText("unknown_request")); + this.logger.warning(this.localisationService.getText("unknown_request") + ": " + req.method); break; } } @@ -100,7 +94,7 @@ export class AkiHttpListener implements IHttpListener let handled = false; // Check if this is a debug request, if so just send the raw response without transformation. - if (req.headers["debug"] === "1") + if (req.headers["debug"] === "1") { this.sendJson(resp, output, sessionID); } @@ -127,7 +121,7 @@ export class AkiHttpListener implements IHttpListener this.requestsLogger.info(`RESPONSE=${this.jsonUtil.serialize(log)}`); } } - + public getResponse(sessionID: string, req: IncomingMessage, body: Buffer): string { const info = this.getBodyInfo(body, req.url); @@ -141,7 +135,7 @@ export class AkiHttpListener implements IHttpListener const log = new Request(req.method, new RequestData(req.url, req.headers, data)); this.requestsLogger.info(`REQUEST=${this.jsonUtil.serialize(log)}`); } - + let output = this.httpRouter.getResponse(req, info, sessionID); /* route doesn't exist or response is not properly set up */ if (!output) @@ -192,10 +186,10 @@ class Request public type: string, public req: RequestData ) - {} + {} } -class Response +class Response { constructor( public type: string, diff --git a/project/src/servers/http/HttpBufferHandler.ts b/project/src/servers/http/HttpBufferHandler.ts deleted file mode 100644 index 93f1906e..00000000 --- a/project/src/servers/http/HttpBufferHandler.ts +++ /dev/null @@ -1,36 +0,0 @@ -import { injectable } from "tsyringe"; - -@injectable() -export class HttpBufferHandler -{ - - protected buffers = {}; - - public resetBuffer(sessionID: string): void - { - this.buffers[sessionID] = undefined; - } - - public putInBuffer(sessionID: any, data: any, bufLength: number): boolean - { - if (this.buffers[sessionID] === undefined || this.buffers[sessionID].allocated !== bufLength) - { - this.buffers[sessionID] = { - written: 0, - allocated: bufLength, - buffer: Buffer.alloc(bufLength) - }; - } - - const buf = this.buffers[sessionID]; - - data.copy(buf.buffer, buf.written, 0); - buf.written += data.length; - return buf.written === buf.allocated; - } - - public getFromBuffer(sessionID: string): any - { - return this.buffers[sessionID].buffer; - } -} \ No newline at end of file