Repository: MaskingTechnology/jitar (ts)

Issue:
Secure the module loading system
Currently the module loader allows any URL, even URLs that are outside the project folder. To avoid misuse we need to check if the URL stays within the project folder. If not, we need to throw a forbidden error.

Pull request:
bug(security): secure module loading
Fixes #242

Changes proposed in this pull request:
- Check if the URL stays within the base URL (project folder)

@MaskingTechnology/jitar


Patch:
diff --git a/packages/runtime/src/errors/ModuleNotAccessible.ts b/packages/runtime/src/errors/ModuleNotAccessible.ts
new file mode 100644
index 0000000..9504060
--- /dev/null
+++ b/packages/runtime/src/errors/ModuleNotAccessible.ts
@@ -0,0 +1,20 @@
+
+import { Loadable } from '@jitar/serialization';
+
+import Forbidden from './generic/Forbidden.js';
+
+export default class ModuleNotAccessible extends Forbidden
+{
+    #url: string;
+
+    constructor(url: string)
+    {
+        super(`Module '${url}' is not accessible`);
+
+        this.#url = url;
+    }
+
+    get url() { return this.#url; }
+}
+
+(ModuleNotAccessible as Loadable).source = '/jitar-runtime/errors/ModuleNotAccessible.js';
diff --git a/packages/runtime/src/utils/ModuleLoader.ts b/packages/runtime/src/utils/ModuleLoader.ts
index 142faf0..aa0ac11 100644
--- a/packages/runtime/src/utils/ModuleLoader.ts
+++ b/packages/runtime/src/utils/ModuleLoader.ts
@@ -1,4 +1,5 @@
 
+import ModuleNotAccessible from '../errors/ModuleNotAccessible.js';
 import ModuleNotLoaded from '../errors/ModuleNotLoaded.js';
 import Module from '../types/Module.js';
 import ModuleImporter from '../types/ModuleImporter.js';
@@ -20,8 +21,10 @@ export default class ModuleLoader
         _import = importer;
     }
 
-    static async load(url: string): Promise<Module>
+    static async load(specifier: string): Promise<Module>
     {
+        let url = specifier;
+        
         if (url.startsWith('/jitar'))
         {
             url = `../..${url}`;
@@ -30,22 +33,32 @@ export default class ModuleLoader
         if (_baseUrl !== undefined && url.startsWith(_baseUrl) === false)
         {
             url = UrlRewriter.addBase(url, _baseUrl);
+
+            if (url.startsWith(_baseUrl) === false)
+            {
+                throw new ModuleNotAccessible(specifier);
+            }
         }
 
-        return this.import(url) as Promise<Module>;
+        return this.#import(url, specifier) as Promise<Module>;
+    }
+
+    static async import(specifier: string): Promise<Module>
+    {
+        return this.#import(specifier, specifier);
     }
 
-    static async import(specifier: string)
+    static async #import(absolute: string, relative: string)
     {
         try
         {
-            return await _import(specifier);
+            return await _import(absolute);
         }
         catch (error: unknown)
         {
             const message = error instanceof Error ? error.message : String(error);
 
-            throw new ModuleNotLoaded(specifier, message);
+            throw new ModuleNotLoaded(relative, message);
         }
     }
 }
diff --git a/packages/runtime/src/utils/UrlRewriter.ts b/packages/runtime/src/utils/UrlRewriter.ts
index 6cc4758..84c2b9c 100644
--- a/packages/runtime/src/utils/UrlRewriter.ts
+++ b/packages/runtime/src/utils/UrlRewriter.ts
@@ -40,7 +40,7 @@ export default class UrlRewriter
         {
             const part = parts[index].trim();
 
-            if (part === '.' || part === '')
+            if (part === '.')
             {
                 continue;
             }
diff --git a/packages/server-nodejs/src/controllers/RPCController.ts b/packages/server-nodejs/src/controllers/RPCController.ts
index 23ba03b..48946e2 100644
--- a/packages/server-nodejs/src/controllers/RPCController.ts
+++ b/packages/server-nodejs/src/controllers/RPCController.ts
@@ -91,9 +91,9 @@ export default class RPCController
         return request.query.serialize === 'true';
     }
 
-    #extractQueryArguments(request: Request): Map<string, unknown>
+    #extractQueryArguments(request: Request): Record<string, unknown>
     {
-        const args = new Map<string, unknown>();
+        const args: Record<string, unknown> = {};
 
         for (const [key, value] of Object.entries(request.query))
         {
@@ -105,19 +105,15 @@ export default class RPCController
                 continue;
             }
 
-            args.set(key, value);
+            args[key] = value;
         }
 
         return args;
     }
 
-    async #extractBodyArguments(request: Request): Promise<Map<string, unknown>>
+    async #extractBodyArguments(request: Request): Promise<Record<string, unknown>>
     {
-        const args = this.#useSerializer
-            ? await this.#serializer.deserialize(request.body) as unknown
-            : request.body;
-
-        return new Map<string, unknown>(Object.entries(args));
+        return request.body;
     }
 
     #extractHeaders(request: Request): Map<string, string>
@@ -145,7 +141,7 @@ export default class RPCController
         return headers;
     }
 
-    async #run(fqn: string, version: Version, args: Map<string, unknown>, headers: Map<string, string>, response: Response, serialize: boolean): Promise<Response>
+    async #run(fqn: string, version: Version, args: Record<string, unknown>, headers: Map<string, string>, response: Response, serialize: boolean): Promise<Response>
     {
         if (this.#runtime.hasProcedure(fqn) === false)
         {
@@ -155,7 +151,14 @@ export default class RPCController
 
         try
         {
-            const result = await this.#runtime.handle(fqn, version, args, headers);
+            if (this.#useSerializer)
+            {
+                args = await this.#serializer.deserialize(args) as Record<string, unknown>;
+            }
+
+            const argsMap = new Map<string, unknown>(Object.entries(args));
+
+            const result = await this.#runtime.handle(fqn, version, argsMap, headers);
 
             this.#logger.info(`Ran procedure -> ${fqn} (${version.toString()})`);
 


Test patch:
diff --git a/packages/runtime/test/_fixtures/utils/ModuleLoader.fixture.ts b/packages/runtime/test/_fixtures/utils/ModuleLoader.fixture.ts
new file mode 100644
index 0000000..334f6a7
--- /dev/null
+++ b/packages/runtime/test/_fixtures/utils/ModuleLoader.fixture.ts
@@ -0,0 +1,13 @@
+
+const moduleImporter = async (specifier: string) =>
+{
+    if (specifier === '/root/app/public/app.js')
+    {
+        // eslint-disable-next-line @typescript-eslint/no-empty-function
+        return { default: function app() {} };
+    }
+
+    throw Error('Not found');
+};
+
+export { moduleImporter };
diff --git a/packages/runtime/test/utils/ModuleLoader.spec.ts b/packages/runtime/test/utils/ModuleLoader.spec.ts
new file mode 100644
index 0000000..17ff65d
--- /dev/null
+++ b/packages/runtime/test/utils/ModuleLoader.spec.ts
@@ -0,0 +1,55 @@
+
+import { describe, expect, it } from 'vitest';
+
+import ModuleNotAccessible from '../../src/errors/ModuleNotAccessible';
+import ModuleNotLoaded from '../../src/errors/ModuleNotLoaded';
+import ModuleLoader from '../../src/utils/ModuleLoader';
+
+import { moduleImporter } from '../_fixtures/utils/ModuleLoader.fixture';
+
+ModuleLoader.setImporter(moduleImporter);
+ModuleLoader.setBaseUrl('/root/app/');
+
+describe('utils/ModuleLoader', () =>
+{
+    describe('.load(specifier)', () =>
+    {
+        it('should load an existing specifier from the base URL', async () =>
+        {
+            const module = await ModuleLoader.load('public/app.js');
+
+            expect(module.default).toBeInstanceOf(Function);
+        });
+
+        it('should not load non-existing specifier from the base URL', () =>
+        {
+            const run = () => ModuleLoader.load('public/non-existing.js');
+
+            expect(run).rejects.toEqual(new ModuleNotLoaded('public/non-existing.js', 'Not found'));
+        });
+
+        it('should not allow URLs outside the base URL', () =>
+        {
+            const run = () => ModuleLoader.load('../secrets.js');
+
+            expect(run).rejects.toEqual(new ModuleNotAccessible('../secrets.js'));
+        });
+    });
+
+    describe('.import(specifier)', () =>
+    {
+        it('should import an existing specifier', async () =>
+        {
+            const module = await ModuleLoader.import('/root/app/public/app.js');
+
+            expect(module.default).toBeInstanceOf(Function);
+        });
+
+        it('should not import a non-existing specifier', () =>
+        {
+            const run = () => ModuleLoader.load('/root/app/public/non-existing.js');
+
+            expect(run).rejects.toEqual(new ModuleNotLoaded('/root/app/public/non-existing.js', 'Not found'));
+        });
+    });
+});