Develop -> Master (28 Oct)

[HarshP4585]

Describe your changes

• Merge develop into master (28 Oct)

Write your issue number after "Fixes "

Enter the corresponding issue number after "Fixes #"

Please ensure all items are checked off before requesting a review:

• I deployed the code locally.
• I have performed a self-review of my code.
• I have included the issue # in the PR.
• I have labelled the PR correctly.
• The issue I am working on is assigned to me.
• I have avoided using hardcoded values to ensure scalability and maintain consistency across the application.
• I have ensured that font sizes, color choices, and other UI elements are referenced from the theme.
• My pull request is focused and addresses a single, specific feature.
• If there are UI changes, I have attached a screenshot or video to this PR.

To fix this issue, we need to rewrite the regular expression to make the character class explicit and unambiguous. Specifically, in [a-z+.-], the dash (-) should be either escaped (\-) or placed at the beginning or end of the character class to remove ambiguity; likewise, for [a-z+.-:], every character meant to be matched individually should be listed explicitly (not as part of a range).

In file Clients/src/presentation/components/PlateEditor.tsx, change line 96 so that ALLOWED_URI_REGEXP uses an explicit character class for +, ., and -. The correct fix is to replace [a-z+.-] with [a-z+.\-] and similar in all relevant places in the pattern, so that only the intended characters are matched. No new imports or methods are needed.

---

To fix the problem, the regular expression should only allow the explicitly intended characters (., +, -, and :) rather than the range .-:, which includes many unintended characters. Specifically, replace [a-z+.-:] with [a-z.+-:] to individually enumerate only the characters allowed.

Detailed steps:

• Edit file Clients/src/presentation/components/Policies/PolicyDetailsModal.tsx.
• At line 452, change a-z+.-: to a-z.+-:.
• No new imports are needed; just modify this string in-place.
• No changes are needed elsewhere.

---

To make the regular expression character class unambiguous and safe, each of the special characters intended to be allowed (such as +, ., and -) should be listed explicitly and not as a range. Specifically:

• In [a-z+.-], the dash (-) should be either escaped (\-) or moved to the start or end of the character class so it is not interpreted as a range. This avoids creating the unintended .-: range.
• The other usages should also be checked (in this regex and elsewhere), but only code snippets provided should be changed.

For this file, edit line 73 of Clients/temp/test-xss-protection.js so that inside the regex, the dash - is placed at the start or end of the character class or escaped as \-, so that [a-z+.-] becomes [a-z+.\-] or [-a-z+.]. This change does not alter the intended matches or break existing functionality, but it prevents the ambiguous and dangerous character range. No new imports or dependencies are needed; only the regex itself needs to be rewritten.

---

The problem is that tempFilePath, which is derived from file.path (coming from potentially uncontrolled user input), is passed directly to fs.promises.unlink to delete a file. To fix this securely, we must restrict deletion to files that reside inside a designated temporary directory, preventing the possibility of path traversal or deletion of unauthorized files due to malicious input. The fix involves:

1. Define a safe temp files directory (Multer usually provides this, but it must be explicit in code).
2. Resolve and normalize tempFilePath with path.resolve(); then, use fs.realpathSync to handle symlinks.
3. Verify that the resolved path starts with the safe temp directory absolute path before proceeding with deletion.
4. If the check fails, log a warning and skip deletion.

All places in the snippet where fs.promises.unlink(tempFilePath) is called must be updated to include this check.

Additional steps:

• Add a constant for the temporary upload directory, if not already present (for example, /tmp/uploads or wherever Multer stores files).
• Add path safety logic before every call to fs.promises.unlink(tempFilePath).

No new npm libraries are required for path checking; use Node's built-in path and fs modules.

---

To fix this problem, we need to ensure that only files within a trusted temporary uploads directory are subject to deletion by the controller. This can be accomplished by:

• Defining a constant with the absolute path to the server’s temp upload directory (e.g., where multer writes to).
• Before calling unlink, normalize and resolve the tempFilePath, and ensure that the resolved path is actually within the trusted temp directory.
• Only proceed with deletion if this validation passes; otherwise, skip unlink and potentially log an error.

Changes needed:

• Add a constant (e.g., UPLOAD_TEMP_DIR) for the upload temp directory path (this can be configurable or set statically).
• Update uses of fs.promises.unlink(tempFilePath) to first resolve tempFilePath and compare against the upload directory.
• If the resolved path is not within the upload directory, skip unlink and log a warning.
• Add necessary imports (path) and definitions within the provided code block only.

All code changes will be within the file Servers/controllers/fileManager.ctrl.ts as shown.

---

To fix the problem, we must ensure that the path used for reading the file (file.path) is constrained to expected upload directories and cannot be manipulated to refer to sensitive or unintended files on the system. The typical approach here is to treat Multer's temporary directory as a "root" and check (after normalizing or resolving the path) that file.path is inside this directory before proceeding.

Specifically:

• Identify the temp uploads directory used by Multer; by default, this could be the system's temp directory, or a custom path. For maximum safety, the code should enforce that only files under a known temp uploads root are allowed.
• Before using fs.createReadStream(file.path), resolve the absolute path for file.path, and confirm it is a child of the known temp uploads root.
• Only proceed if this validation passes, otherwise throw an error.

Since the code is only shown in Servers/utils/fileManager.utils.ts, the check should be implemented in the uploadFileToManager function just before reading from file.path.

We'll add:

• Definition and calculation of the expected temp storage directory (e.g., process.env.TEMP_UPLOAD_ROOT or default to OS temp).
• Validation logic above fs.createReadStream(...).
• An error throw if the validation fails.
• Optionally, import os if os.tmpdir() is used.

---

General fix strategy:
After creating the destination file path (permanentFilePath), normalize the path with path.resolve and ensure it remains within the expected upload directory. If the resolved permanentFilePath does not start with the absolute path of uploadsDir (plus a separator), abort the operation.

Detailed fix for the provided code:

• After constructing permanentFilePath (line 53), use path.resolve on it and then confirm that the resolved value starts with the resolved uploadsDir (with the path separator to prevent prefix sneaking).
• If the check fails, delete any temp file (if present), throw or return an error, and do not continue.
• This logic should be implemented immediately before any file writing operations.

Requirements to implement:

• Add a block after lines 53 or 54 to:
  • Compute const resolvedUploadsDir = path.resolve(uploadsDir) (if not computed there already)
  • const resolvedPermanentFilePath = path.resolve(permanentFilePath)
  • If not resolvedPermanentFilePath.startsWith(resolvedUploadsDir + path.sep), throw an error.
• No new dependencies are required; only built-in path is needed.

---

To address the issue, modify how the target file path (permanentFilePath) is computed. After generating the filename and building the path, use path.resolve (or fs.realpathSync after ensuring the file exists, but for uploads, path.resolve is better) to normalize it. Then check that the resulting absolute path begins with the intended uploads directory, also resolved with path.resolve to strictify the comparison. If the path is not contained within the storage root, throw an error. This prevents any possibility of traversing outside the "uploads/file-manager/" directory. Place the check immediately after constructing permanentFilePath, before writing/moving the uploaded file.

Required changes:

• Add resolve logic and containment check after line 53 (permanentFilePath).
• If the resolved path does not begin with the intended uploads directory, throw an error.
• No new imports are necessary.

---

The best way to fix this problem is to ensure that every file path used for file system operations is guaranteed to reside inside the intended uploads root directory, regardless of any user input. This is done by (1) sanitizing the filename using a robust library (optionally, such as sanitize-filename from npm), (2) constructing the full file path using path.resolve, and (3) checking that the resolved path strictly starts with the intended root directory, aborting otherwise. This should be performed prior to every file system operation using user-controlled input, notably before calling fs.promises.unlink with permanentFilePath. For best defense-in-depth, update the code in Servers/utils/fileManager.utils.ts around the construction and deletion of the file path (permanentFilePath), using path.resolve, and add a check that it does not escape the uploads directory.

If you do not wish to introduce external dependencies, you may strengthen the sanitization logic, but the most robust solution would be to perform both path normalization and explicit root directory containment checks.

---

The best way to fix the problem is to avoid letting untrusted input appear directly in the format string passed as the first argument to console.error. Instead, use a static string with a format specifier (e.g., %s) and pass the user-controlled value as a separate argument. This ensures that the path, no matter what it contains, will be treated as a literal string by the logger.
To implement the fix:

• In Servers/utils/fileManager.utils.ts, update the problematic console.error call (line 109) by replacing the template string with a static format string and the tainted value as a separate argument:
  Change:

  console.error(`Failed to cleanup orphaned file after DB error: ${permanentFilePath}`, unlinkError);

  To:

  console.error("Failed to cleanup orphaned file after DB error: %s", permanentFilePath, unlinkError);

• No new imports or helper methods are needed.
• No other regions of the provided snippets require edits.

---

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch develop

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}