Micol/file upload #1386

micolgiannelli2 · 2026-01-23T13:36:19Z

Refactor: Make document upload service configurable per form

Summary

Refactored the document upload service to support form-specific upload endpoints, making it reusable across different forms rather than being tied to a single implementation.

Changes

1. Schema Updates (`model/src/schema/schema.ts`)

Type Definition:

documentUploadApiUrl?: string | undefined;

Validation:

documentUploadApiUrl: joi.string().optional()

Forms can now optionally specify a custom document upload API URL.

2. Upload Service Refactoring (`runner/src/server/services/upload/uploadService.ts`)

Key improvements:

Extracted upload request logic into smaller, testable methods:
- buildFormData() - Creates FormData from file streams
- getUploadUrl() - Resolves the upload URL with fallback logic
- validateUploadUrl() - Validates URL is configured before upload
- buildUploadRequest() - Constructs request with security headers

URL Resolution:

const baseUrl = model?.def?.documentUploadApiUrl ?? config.documentUploadApiUrl;

Falls back to global config if form-specific URL is not provided.

Validation:
Now throws a clear error if no upload URL is configured (either in form definition or global config), preventing silent failures.

Removed:

MockUploadService class - mocking strategy didn't follow separation of concerns principles
Removed logic to spin up MockUploadService - Note this was happening silently and hence a possible risk section in the code

3. Additional Changes and Bug Fixes

Fixed incorrect log message in validateContentType() for .msg files (previously said "RIS file")
Applied formatting on the following file that was missing a blank line and hence raising an error inthe linter runner/src/server/plugins/engine/models/FormModel.ts
Reformatted KLS forms documents which previously had incorrect json spacing

Impact

✅ Non-breaking: Existing forms continue to work with global config.documentUploadApiUrl
✅ New capability: Forms can specify custom upload endpoints via documentUploadApiUrl
⚠️ Required: config.documentUploadApiUrl must be set in default.js or all file uploads will fail

Testing

Manual testing completed:

✅ kls-enquiries form with staging endpoint
✅ Error handling when URL not configured
The following testing check list demonstrates different scenarios:

Testing Checklist

Scenario 1: File upload service works as expected when documentUploadApiUrl is set in default.js
- Expected: Files upload successfully using the global config URL
Scenario 2: File upload service fails gracefully when no documentUploadApiUrl is set in default.js and no form-specific URL is configured
- Expected: Clear error message indicating upload URL is not configured
- The following error exposing the porblem is printed to the console
  Document upload API URL is not configured. Please set documentUploadApiUrl in config or model definition
Scenario 3: File upload service correctly retrieves the form-specific URL when no documentUploadApiUrl is set in default.js but one is configured in the form definition
- Expected: Files upload successfully using the form-specific URL
- The file uploads successfully
Scenario 4: File upload service prioritizes form-specific URL when both default.js and form definition have documentUploadApiUrl configured
- Expected: Files upload to the form-specific URL (not the global config URL)
  Verified:
Removed mock service code is not referenced elsewhere
Existing upload flows remain functional

Follow-up Work

the following to can be found in a conversation in the comments below
the reason I didn't make this directly is because it would involve changing environment variables in production please view conversation with @kitttang below

Consider making endpoint path (/v1/files) configurable if needed

jenbutongit · 2026-01-26T22:25:17Z

runner/src/server/services/upload/uploadService.ts

+    if (!isValid && filename?.endsWith(".ris")) {
+      this.logger.warn("UPLOAD_WARNING", {
+        reason: "RIS file had generic content type",
+        filename,
+        contentType,
+      });
+      isValid = true;
+    }
+
+    // Fallback: allow .msg files with 'application/octet-stream'
+    if (!isValid && filename?.endsWith(".msg")) {
+      this.logger.warn("UPLOAD_WARNING", {
+        reason: "MSG file had generic content type",
+        filename,
+        contentType,
+      });
+      isValid = true;
+    }


I'm not sure this is needed - you can add custom accepted mime types via the form json. Does this work for your use case?

e.g.

{ "name": "applicationForm", "options": { "required": true, "accept": "image/jpeg,image/png,application/pdf,application/vnd.oasis.opendocument.text", "customValidationMessages": { "string.empty": "Upload your application form" } }, "type": "FileUploadField", "title": "Application form", "schema": {} },

Hi Jen will look into this, I didn;t actually write this bit and the onw below part of this PR was written by Suilman but will look into this

runner/src/server/services/upload/uploadService.ts

jenbutongit · 2026-01-26T22:28:08Z

runner/src/server/utils/hmac.ts

+/**
+ * Validates an HMAC signature
+ */
+export async function validateHmac(


Is this used? unit test if so please

jenbutongit · 2026-01-26T22:34:44Z

runner/src/server/utils/hmac.ts

+export async function createHmac(email: string, hmacKey: string) {
+  try {
+    // Get current timestamp
+    const currentTimestamp = Math.floor(Date.now() / 1000);
+
+    // Prepare the data for HMAC calculation
+    const dataToHash = email + currentTimestamp;
+
+    // Calculate the HMAC hash
+    const hmac = crypto
+      .createHmac("sha256", hmacKey)
+      .update(dataToHash)
+      .digest("hex");
+
+    const expiryTimestamp = currentTimestamp + TIME_THRESHOLD;
+    const adjustedExpiryForDisplay = applyBSTIfRequired(expiryTimestamp);
+
+    const hmacExpiryTime = formatUnixTimestamp(adjustedExpiryForDisplay);
+
+    return [hmac, currentTimestamp, hmacExpiryTime];
+  } catch (error) {
+    console.error("Error creating HMAC:", error);
+    throw error;
+  }
+}


Is this used? Unit test if so. If not, let's keep it out of the codebase

I would suggest setting your servers to UTC and validate on this - or using something like date-fns or temporal to resolve any timezone related issues.

jenbutongit · 2026-01-26T22:45:05Z

runner/src/server/services/upload/uploadService.ts

+    const [hmacSignature, requestTime] = await createHmacRaw(
+      request.yar.id,
+      hmacKey
    );

-    return this.parsedDocumentUploadResponse(responseData);
+    const securityHeaders = {
+      "X-Request-ID": request.yar.id.toString(),
+      "X-HMAC-Signature": hmacSignature.toString(),
+      "X-HMAC-Time": requestTime.toString(),
+    };


This appears to be implementation detail of your document upload api - could you wrap this section in if (hmacKey) { or if(requiresSecurityHeaders) { please? not all existing APIs need this

jenbutongit · 2026-01-26T22:46:30Z

runner/src/server/services/upload/uploadService.ts

+    const id = request.params?.id;
+    const forms = request.server?.app?.forms;
+    const model = id && forms?.[id];
+    const hmacKey = model?.def?.fileUploadHmacSharedKey;


doesn't look like this was added to the schema?

do you have a hmac key per form? or is this shared across all your forms? would consider adding this as an env var if shared across all forms

jenbutongit · 2026-01-26T22:49:25Z

runner/src/server/index.ts

-  if (!config.documentUploadApiUrl) {
-    server.registerService([
-      Schmervice.withName("uploadService", MockUploadService),
-    ]);
-  } else {
-    server.registerService([UploadService]);
-  }


This needs to stay in. MockUploadService is used when you are running the application without a document upload api configured. Unit and e2e tests use this too.

jenbutongit · 2026-01-26T22:55:48Z

runner/src/server/services/upload/uploadService.ts

+    // TODO: I didn't want to introduce a breaking change, but maybe the versioning should be handled differently
+    return `${baseUrl}`;


Agreed, versioning should be handled by the config but will require all consumers of XGovFormBuilder to change their config if they want to upgrade. Currently your implementation is a breaking change. You will need to add /v1 to base url

Ok, what it is currently is that it calls the end poin v1/files and doesn't allow you to call any other endpoint should I leave it like this an open a ticket somewhere for someone to introduce this necessary breaking change?

…passed down

…nal parameter

micolgiannelli2 added 5 commits January 23, 2026 13:16

inculded documentUmentUploadApiUrl as a prarmenter to the form

9fe6144

removed mockUploadService

fc3c2eb

new strucutre for file upload service

af53186

fixed hmac import

850306c

added local host as default end point removed default variable

44cfc94

micolgiannelli2 marked this pull request as ready for review January 23, 2026 14:32

jenbutongit requested changes Jan 26, 2026

View reviewed changes

micolgiannelli2 added 6 commits January 30, 2026 12:16

re-introduced mock functionality through function

183172f

commented out hmac

6e5db79

typscript es lint bugs resolved and included request parameter being …

2e86adb

…passed down

re introduced deprecated mock file upload made sure request was optio…

1d8151d

…nal parameter

returns mock when url is undefined

eb9f6be

added files commited by mistake

c7595af

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Micol/file upload #1386

Micol/file upload #1386

Uh oh!

micolgiannelli2 commented Jan 23, 2026 •

edited

Loading

Uh oh!

jenbutongit Jan 26, 2026

Uh oh!

micolgiannelli2 Jan 29, 2026 •

edited

Loading

Uh oh!

Uh oh!

jenbutongit Jan 26, 2026

Uh oh!

jenbutongit Jan 26, 2026

Uh oh!

jenbutongit Jan 26, 2026

Uh oh!

jenbutongit Jan 26, 2026

Uh oh!

jenbutongit Jan 26, 2026

Uh oh!

jenbutongit Jan 26, 2026

Uh oh!

micolgiannelli2 Jan 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		// TODO: I didn't want to introduce a breaking change, but maybe the versioning should be handled differently
		return `${baseUrl}`;

Micol/file upload #1386

Are you sure you want to change the base?

Micol/file upload #1386

Uh oh!

Conversation

micolgiannelli2 commented Jan 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Refactor: Make document upload service configurable per form

Summary

Changes

1. Schema Updates (model/src/schema/schema.ts)

2. Upload Service Refactoring (runner/src/server/services/upload/uploadService.ts)

3. Additional Changes and Bug Fixes

Impact

Testing

Testing Checklist

Follow-up Work

Uh oh!

Choose a reason for hiding this comment

Uh oh!

micolgiannelli2 Jan 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

micolgiannelli2 commented Jan 23, 2026 •

edited

Loading

1. Schema Updates (`model/src/schema/schema.ts`)

2. Upload Service Refactoring (`runner/src/server/services/upload/uploadService.ts`)

micolgiannelli2 Jan 29, 2026 •

edited

Loading