Repository: MaskingTechnology/jitar (ts)

Issue:
Add path validation to requesting assets
Currently, when requesting an assets outside the root folder (the cache location), the path is updated to stay within the root path. This is an non transparent way of preventing path traversal.

The next step should be to remove this non transparent way and throw an error when path traversal is tried. We want to throw a BadRequest exception when a path outside of the root is requested.
Add fqn validation
Currently, the fqn specified in the rpc call is directly used for the request. The module loader does take care of this, but we also want to check this on the server before calling the module loader.

In case we have an invalid fqn, we will throw a BadRequest exception.
Add validation for the request version
Currently, the `version` query parameter that is passed to the rpc controller is not completely validated. Due to Express, we need to check the type of the input to be sure it's a String.

Pull request description:
feat(server): improved server-side validations
Fixes #402
Closes #403
Closes #405
Closes #415

Changes proposed in this pull request:
- added validations for versions, fqn, assets, segments
- added definitions for headers and content types
- added default header to prevent content sniffing
- check for path traversal in file manager

@MaskingTechnology/jitar


Patch:
diff --git a/packages/runtime/src/definitions/Files.ts b/packages/runtime/src/definitions/Files.ts
index ef0c8ba..b288321 100644
--- a/packages/runtime/src/definitions/Files.ts
+++ b/packages/runtime/src/definitions/Files.ts
@@ -9,14 +9,16 @@ const Files =
 
 Object.freeze(Files);
 
+const EXTENSION_PATTERN = /\.js$/;
+
 function convertToLocalFilename(filename: string): string
 {
-    return filename.replace('.js', '.local.js');
+    return filename.replace(EXTENSION_PATTERN, '.local.js');
 }
 
 function convertToRemoteFilename(filename: string): string
 {
-    return filename.replace('.js', '.remote.js');
+    return filename.replace(EXTENSION_PATTERN, '.remote.js');
 }
 
 function createNodeFilename(name: string): string
diff --git a/packages/runtime/src/errors/FileNotFound.ts b/packages/runtime/src/errors/FileNotFound.ts
index a22ccee..2a101b4 100644
--- a/packages/runtime/src/errors/FileNotFound.ts
+++ b/packages/runtime/src/errors/FileNotFound.ts
@@ -9,7 +9,7 @@ export default class FileNotFound extends NotFound
 
     constructor(filename: string)
     {
-        super(`The file '${filename} could not be found'`);
+        super(`The file '${filename}' could not be found`);
 
         this.#filename = filename;
     }
diff --git a/packages/runtime/src/utils/VersionParser.ts b/packages/runtime/src/utils/VersionParser.ts
index f64459b..ebb995e 100644
--- a/packages/runtime/src/utils/VersionParser.ts
+++ b/packages/runtime/src/utils/VersionParser.ts
@@ -3,6 +3,8 @@ import InvalidVersionNumber from '../errors/InvalidVersionNumber.js';
 
 import Version from '../models/Version.js';
 
+const VERSION_EXPRESSION = /^\d+(?:\.\d+){0,2}$/;
+
 export default class VersionParser
 {
     static parse(number: string): Version
@@ -12,6 +14,11 @@ export default class VersionParser
             return Version.DEFAULT;
         }
 
+        if (VERSION_EXPRESSION.test(number) === false)
+        {
+            throw new InvalidVersionNumber(number);
+        }
+
         const parts = number.split('.');
 
         switch (parts.length)
@@ -19,7 +26,7 @@ export default class VersionParser
             case 1: return new Version(Number.parseInt(parts[0]));
             case 2: return new Version(Number.parseInt(parts[0]), Number.parseInt(parts[1]));
             case 3: return new Version(Number.parseInt(parts[0]), Number.parseInt(parts[1]), Number.parseInt(parts[2]));
-            default: throw new InvalidVersionNumber(number);
+            default: return new Version(Number.parseInt(parts[0]), Number.parseInt(parts[1]), Number.parseInt(parts[2]));
         }
     }
 }
diff --git a/packages/server-nodejs/src/JitarServer.ts b/packages/server-nodejs/src/JitarServer.ts
index 8f47734..07d81cf 100644
--- a/packages/server-nodejs/src/JitarServer.ts
+++ b/packages/server-nodejs/src/JitarServer.ts
@@ -26,6 +26,7 @@ import RuntimeDefaults from './definitions/RuntimeDefaults.js';
 
 import RuntimeNotAvailable from './errors/RuntimeNotAvailable.js';
 import LogBuilder from './utils/LogBuilder.js';
+import Headers from './definitions/Headers.js';
 
 const STARTUP_MESSAGE = `
        ██ ██ ████████  █████  ██████  
@@ -59,6 +60,7 @@ export default class JitarServer
 
         this.#app.use(express.json());
         this.#app.use(express.urlencoded({ extended: true }));
+        this.#app.use((request, response, next) => this.#addDefaultHeaders(request, response, next));
 
         this.#app.disable('x-powered-by');
 
@@ -257,4 +259,12 @@ export default class JitarServer
 
         this.#logger.info('Registered RPC entries', procedureNames);
     }
+
+    // eslint-disable-next-line @typescript-eslint/no-unused-vars
+    #addDefaultHeaders(request: express.Request, response: express.Response, next: express.NextFunction): void
+    {
+        response.setHeader(Headers.CONTENT_TYPE_OPTIONS, 'nosniff');
+
+        next();
+    }
 }
diff --git a/packages/server-nodejs/src/controllers/AssetsController.ts b/packages/server-nodejs/src/controllers/AssetsController.ts
index c2c6893..e911a33 100644
--- a/packages/server-nodejs/src/controllers/AssetsController.ts
+++ b/packages/server-nodejs/src/controllers/AssetsController.ts
@@ -4,6 +4,9 @@ import { Logger } from 'tslog';
 
 import { LocalRepository, Standalone, FileNotFound } from '@jitar/runtime';
 
+import Headers from '../definitions/Headers.js';
+import ContentTypes from '../definitions/ContentTypes.js';
+
 export default class AssetsController
 {
     #repository: LocalRepository | Standalone;
@@ -21,24 +24,29 @@ export default class AssetsController
 
     async #getContent(request: Request, response: Response): Promise<void>
     {
-        this.#logger.info(`Got asset -> '${request.path}'`);
-
-        const path = request.path.substring(1).trim();
-        const filename = path.length === 0 ? this.#indexFile : path;
-
         try
         {
+            const path = request.path.substring(1).trim();
+            const decodedPath = decodeURIComponent(path);
+            const filename = decodedPath.length === 0 ? this.#indexFile : decodedPath;
+
             const file = await this.#repository.readAsset(filename);
 
-            response.set('Content-Type', file.type);
-            response.set('Content-Length', String(file.size));
+            this.#logger.info(`Got asset -> '${request.path}'`);
+
+            if (file.type === ContentTypes.HTML)
+            {
+                response.setHeader(Headers.FRAME_OPTIONS, 'DENY');
+            }
+
+            response.setHeader(Headers.CONTENT_TYPE, file.type);
             response.status(200).send(file.content);
         }
         catch (error: unknown)
         {
             if (error instanceof FileNotFound)
             {
-                this.#logger.warn(`Failed to get asset -> '${filename}' | ${error.message}`);
+                this.#logger.warn(`Failed to get asset ->  ${error.message}`);
 
                 response.status(404).send(error.message);
 
@@ -47,7 +55,7 @@ export default class AssetsController
 
             const message = error instanceof Error ? error.message : String(error);
 
-            this.#logger.error(`Failed to get file content -> '${filename}' | ${message}`);
+            this.#logger.error(`Failed to get file content -> ${message}`);
 
             response.status(500).send(message);
         }
diff --git a/packages/server-nodejs/src/controllers/HealthController.ts b/packages/server-nodejs/src/controllers/HealthController.ts
index f64d059..1519a36 100644
--- a/packages/server-nodejs/src/controllers/HealthController.ts
+++ b/packages/server-nodejs/src/controllers/HealthController.ts
@@ -3,6 +3,8 @@ import express, { Request, Response } from 'express';
 import { Logger } from 'tslog';
 
 import { LocalNode, Standalone } from '@jitar/runtime';
+import Headers from '../definitions/Headers';
+import ContentTypes from '../definitions/ContentTypes';
 
 export default class HealthController
 {
@@ -34,7 +36,7 @@ export default class HealthController
 
         this.#logger.debug('Got health status');
 
-        response.setHeader('Content-Type', 'text/plain');
+        response.setHeader(Headers.CONTENT_TYPE, ContentTypes.TEXT);
 
         return response.status(200).send(healthy);
     }
diff --git a/packages/server-nodejs/src/controllers/ModulesController.ts b/packages/server-nodejs/src/controllers/ModulesController.ts
index d0b6b45..2a17e52 100644
--- a/packages/server-nodejs/src/controllers/ModulesController.ts
+++ b/packages/server-nodejs/src/controllers/ModulesController.ts
@@ -5,6 +5,9 @@ import { Logger } from 'tslog';
 import { ClientIdHelper, LocalRepository, Standalone } from '@jitar/runtime';
 import { Serializer } from '@jitar/serialization';
 
+import Headers from '../definitions/Headers';
+import ContentTypes from '../definitions/ContentTypes';
+
 const clientIdHelper = new ClientIdHelper();
 
 export default class ModulesController
@@ -34,10 +37,20 @@ export default class ModulesController
 
         const segmentFiles = request.body as string[];
 
+        for (const segmentFile of segmentFiles)
+        {
+            if (typeof segmentFile !== 'string')
+            {
+                return response.status(400).send('Invalid segment file list.');
+            }
+        }
+
         const clientId = await this.#repository.registerClient(segmentFiles);
 
         this.#logger.info(`Registered client -> ${clientId} [${segmentFiles.join(',')}]`);
 
+        response.setHeader(Headers.CONTENT_TYPE, ContentTypes.TEXT);
+
         return response.status(200).send(clientId);
     }
 
@@ -62,7 +75,7 @@ export default class ModulesController
 
             this.#logger.info(`Got module -> '${filename}' (${clientId})`);
 
-            response.set('Content-Type', file.type);
+            response.setHeader(Headers.CONTENT_TYPE, file.type);
 
             return response.status(200).send(file.content);
         }
@@ -74,7 +87,7 @@ export default class ModulesController
 
             const data = this.#serializer.serialize(error);
 
-            response.setHeader('Content-Type', 'application/json');
+            response.setHeader(Headers.CONTENT_TYPE, ContentTypes.JSON);
 
             return response.status(500).send(data);
         }
diff --git a/packages/server-nodejs/src/controllers/NodesController.ts b/packages/server-nodejs/src/controllers/NodesController.ts
index 0b2e870..c39459b 100644
--- a/packages/server-nodejs/src/controllers/NodesController.ts
+++ b/packages/server-nodejs/src/controllers/NodesController.ts
@@ -6,6 +6,8 @@ import { LocalGateway, RemoteNode } from '@jitar/runtime';
 
 import NodeDto, { nodeDtoSchema } from '../models/NodeDto.js';
 import DataConverter from '../utils/DataConverter.js';
+import Headers from '../definitions/Headers.js';
+import ContentTypes from '../definitions/ContentTypes.js';
 
 export default class NodesController
 {
@@ -27,6 +29,8 @@ export default class NodesController
 
         this.#logger.info('Got nodes');
 
+        response.setHeader(Headers.CONTENT_TYPE, ContentTypes.JSON);
+
         return response.status(200).send(nodes);
     }
 
@@ -52,6 +56,8 @@ export default class NodesController
 
             this.#logger.error(`Failed to add node | ${message}`);
 
+            response.setHeader(Headers.CONTENT_TYPE, ContentTypes.TEXT);
+
             return response.status(status).send(error);
         }
     }
diff --git a/packages/server-nodejs/src/controllers/ProceduresController.ts b/packages/server-nodejs/src/controllers/ProceduresController.ts
index 7d40831..ce2d26c 100644
--- a/packages/server-nodejs/src/controllers/ProceduresController.ts
+++ b/packages/server-nodejs/src/controllers/ProceduresController.ts
@@ -4,6 +4,9 @@ import { Logger } from 'tslog';
 
 import { LocalGateway, LocalNode, Standalone } from '@jitar/runtime';
 
+import Headers from '../definitions/Headers';
+import ContentTypes from '../definitions/ContentTypes';
+
 export default class ProceduresController
 {
     #runtime: LocalGateway | LocalNode | Standalone;
@@ -23,6 +26,8 @@ export default class ProceduresController
 
         this.#logger.info('Got procedure names');
 
+        response.setHeader(Headers.CONTENT_TYPE, ContentTypes.JSON);
+
         return response.status(200).send(names);
     }
 }
diff --git a/packages/server-nodejs/src/controllers/RPCController.ts b/packages/server-nodejs/src/controllers/RPCController.ts
index fe596d1..d0153ba 100644
--- a/packages/server-nodejs/src/controllers/RPCController.ts
+++ b/packages/server-nodejs/src/controllers/RPCController.ts
@@ -6,6 +6,8 @@ import { Request as JitarRequest, Version, VersionParser, ProcedureRuntime, BadR
 import { Serializer } from '@jitar/serialization';
 
 import CorsMiddleware from '../middleware/CorsMiddleware.js';
+import Headers from '../definitions/Headers.js';
+import ContentTypes from '../definitions/ContentTypes.js';
 
 const RPC_PARAMETERS = ['version', 'serialize'];
 const IGNORED_HEADER_KEYS = ['host', 'connection', 'content-length', 'accept-encoding', 'user-agent'];
@@ -39,24 +41,50 @@ export default class RPCController
 
     async runGet(request: ExpressRequest, response: ExpressResponse): Promise<ExpressResponse>
     {
-        const fqn = this.#extractFqn(request);
-        const version = this.#extractVersion(request);
-        const args = this.#extractQueryArguments(request);
-        const headers = this.#extractHeaders(request);
-        const serialize = this.#extractSerialize(request);
+        try
+        {
+            const fqn = this.#extractFqn(request);
+            const version = this.#extractVersion(request);
+            const args = this.#extractQueryArguments(request);
+            const headers = this.#extractHeaders(request);
+            const serialize = this.#extractSerialize(request);
+
+            return this.#run(fqn, version, args, headers, response, serialize);
+        }
+        catch (error: unknown)
+        {
+            const message = error instanceof Error ? error.message : String(error);
+
+            this.#logger.warn(`Invalid request -> ${message}`);
 
-        return this.#run(fqn, version, args, headers, response, serialize);
+            response.setHeader(Headers.CONTENT_TYPE, ContentTypes.TEXT);
+
+            return response.status(400).send(`Invalid request -> ${message}`);
+        }
     }
 
     async runPost(request: ExpressRequest, response: ExpressResponse): Promise<ExpressResponse>
     {
-        const fqn = this.#extractFqn(request);
-        const version = this.#extractVersion(request);
-        const args = this.#extractBodyArguments(request);
-        const headers = this.#extractHeaders(request);
-        const serialize = this.#extractSerialize(request);
+        try
+        {
+            const fqn = this.#extractFqn(request);
+            const version = this.#extractVersion(request);
+            const args = this.#extractBodyArguments(request);
+            const headers = this.#extractHeaders(request);
+            const serialize = this.#extractSerialize(request);
+
+            return this.#run(fqn, version, args, headers, response, serialize);
+        }
+        catch (error: unknown)
+        {
+            const message = error instanceof Error ? error.message : String(error);
+
+            this.#logger.warn(`Invalid request -> ${message}`);
 
-        return this.#run(fqn, version, args, headers, response, serialize);
+            response.setHeader(Headers.CONTENT_TYPE, ContentTypes.TEXT);
+
+            return response.status(400).send(`Invalid request -> ${message}`);
+        }
     }
 
     async runOptions(request: ExpressRequest, response: ExpressResponse): Promise<ExpressResponse>
@@ -66,14 +94,34 @@ export default class RPCController
 
     #extractFqn(request: ExpressRequest): string
     {
-        return request.path.substring(5);
+        const decodedFqn = decodeURIComponent(request.path.trim());
+        const fqn = decodedFqn.substring(5).trim();
+
+        if (fqn.length === 0)
+        {
+            throw new BadRequest('Missing procedure name');
+        }
+
+        if (fqn.includes('..'))
+        {
+            throw new BadRequest('Invalid procedure name');
+        }
+
+        return fqn;
     }
 
     #extractVersion(request: ExpressRequest): Version
     {
-        return request.query.version !== undefined
-            ? VersionParser.parse(request.query.version.toString())
-            : Version.DEFAULT;
+        const version = request.query.version !== undefined
+            ? request.query.version
+            : '';
+
+        if (typeof version !== 'string')
+        {
+            throw new BadRequest('Invalid version number');
+        }
+
+        return VersionParser.parse(version);
     }
 
     #extractSerialize(request: ExpressRequest): boolean
@@ -136,6 +184,8 @@ export default class RPCController
         if (this.#runtime.hasProcedure(fqn) === false)
         {
             // We need this check to make sure we won't run an private procedure.
+            response.setHeader(Headers.CONTENT_TYPE, ContentTypes.TEXT);
+
             return response.status(404).send(`Procedure not found -> ${fqn}`);
         }
 
@@ -188,9 +238,9 @@ export default class RPCController
     {
         const content = await this.#createResponseContent(result, serialize);
         const contentType = this.#createResponseContentType(content);
-        const responseContent = contentType === 'text/plain' ? String(content) : content;
+        const responseContent = contentType === ContentTypes.TEXT ? String(content) : content;
 
-        response.setHeader('Content-Type', contentType);
+        response.setHeader(Headers.CONTENT_TYPE, contentType);
 
         return response.status(200).send(responseContent);
     }
@@ -201,7 +251,7 @@ export default class RPCController
         const contentType = this.#createResponseContentType(content);
         const statusCode = this.#createResponseStatusCode(error);
 
-        response.setHeader('Content-Type', contentType);
+        response.setHeader(Headers.CONTENT_TYPE, contentType);
 
         return response.status(statusCode).send(content);
     }
@@ -216,8 +266,8 @@ export default class RPCController
     #createResponseContentType(content: unknown): string
     {
         return typeof content === 'object'
-            ? 'application/json'
-            : 'text/plain';
+            ? ContentTypes.JSON
+            : ContentTypes.TEXT;
     }
 
     #setResponseHeaders(response: ExpressResponse, headers: Map<string, string>): void
diff --git a/packages/server-nodejs/src/definitions/ContentTypes.ts b/packages/server-nodejs/src/definitions/ContentTypes.ts
new file mode 100644
index 0000000..95737fb
--- /dev/null
+++ b/packages/server-nodejs/src/definitions/ContentTypes.ts
@@ -0,0 +1,10 @@
+
+const ContentTypes = {
+    JSON: 'application/json',
+    TEXT: 'text/plain',
+    HTML: 'text/html',
+} as const;
+
+Object.freeze(ContentTypes);
+
+export default ContentTypes;
diff --git a/packages/server-nodejs/src/definitions/Headers.ts b/packages/server-nodejs/src/definitions/Headers.ts
new file mode 100644
index 0000000..138ef9d
--- /dev/null
+++ b/packages/server-nodejs/src/definitions/Headers.ts
@@ -0,0 +1,11 @@
+
+const Headers =
+{
+    CONTENT_TYPE: 'Content-Type',
+    CONTENT_TYPE_OPTIONS: 'X-Content-Type-Options',
+    FRAME_OPTIONS : 'X-Frame-Options'
+} as const;
+
+Object.freeze(Headers);
+
+export default Headers;
diff --git a/packages/server-nodejs/src/definitions/RuntimeDefaults.ts b/packages/server-nodejs/src/definitions/RuntimeDefaults.ts
index 9071a4a..ae63877 100644
--- a/packages/server-nodejs/src/definitions/RuntimeDefaults.ts
+++ b/packages/server-nodejs/src/definitions/RuntimeDefaults.ts
@@ -1,10 +1,12 @@
 
-enum RuntimeDefaults
+const RuntimeDefaults =
 {
-    URL = 'http://localhost:3000',
-    SOURCE = './dist',
-    CACHE = './.jitar',
-    INDEX = 'index.html'
-}
+    URL: 'http://localhost:3000',
+    SOURCE: './dist',
+    CACHE: './.jitar',
+    INDEX: 'index.html'
+} as const;
+
+Object.freeze(RuntimeDefaults);
 
 export default RuntimeDefaults;
diff --git a/packages/server-nodejs/src/utils/LocalFileManager.ts b/packages/server-nodejs/src/utils/LocalFileManager.ts
index 4e2864d..f6dc0a5 100644
--- a/packages/server-nodejs/src/utils/LocalFileManager.ts
+++ b/packages/server-nodejs/src/utils/LocalFileManager.ts
@@ -55,8 +55,16 @@ export default class LocalFileManager implements FileManager
 
     async read(filename: string): Promise<File>
     {
-        const type = await this.getType(filename);
-        const content = await this.getContent(filename);
+        const rootPath = this.getRootLocation();
+        const absoluteFilename = this.getAbsoluteLocation(filename);
+
+        if (absoluteFilename.startsWith(rootPath) === false)
+        {
+            throw new FileNotFound(filename);
+        }
+
+        const type = await this.getType(absoluteFilename);
+        const content = await this.getContent(absoluteFilename);
 
         return new File(filename, type, content);
     }


Test patch:
diff --git a/packages/runtime/test/utils/VersionParser.spec.ts b/packages/runtime/test/utils/VersionParser.spec.ts
index b487734..bd8d589 100644
--- a/packages/runtime/test/utils/VersionParser.spec.ts
+++ b/packages/runtime/test/utils/VersionParser.spec.ts
@@ -44,5 +44,12 @@ describe('utils/VersionParser', () =>
 
             expect(run).toThrow(new InvalidVersionNumber('1.2.3.4'));
         });
+
+        it('should not parse an invalid number', () =>
+        {
+            const run = () => VersionParser.parse('1.2.a');
+
+            expect(run).toThrow(new InvalidVersionNumber('1.2.a'));
+        });
     });
 });