Skip to content

Commit

Permalink
🛡️ fix: Minor Vulnerabilities (#4543)
Browse files Browse the repository at this point in the history
* fix: ReDoS in ChatGPT Import

* ci: should correctly process citations from real ChatGPT data

* ci: Add ReDoS vulnerability test for processAssistantMessage

* refactor: Update thread management and citation handling

* refactor(validateImageRequest): robust validation

* refactor(Prompt.js): update name search regex to escape special characters

* refactor(Preset): exclude user from preset update to prevent mass assignment

* refactor(files.js): Improve file deletion process

* ci: updated validateImageRequest.spec.js

* a11y: plugin pagination

* refactor(CreatePromptForm.tsx): Improve input field styling

* chore(Prompts): typing and accessibility

* fix: prompt creation access role check

* chore: remove duplicate jsdocs
  • Loading branch information
danny-avila authored Oct 24, 2024
1 parent 094a40d commit 3f3b592
Show file tree
Hide file tree
Showing 15 changed files with 698 additions and 53 deletions.
3 changes: 2 additions & 1 deletion api/models/Preset.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ module.exports = {
savePreset: async (user, { presetId, newPresetId, defaultPreset, ...preset }) => {
try {
const setter = { $set: {} };
const update = { presetId, ...preset };
const { user: _, ...cleanPreset } = preset;
const update = { presetId, ...cleanPreset };
if (preset.tools && Array.isArray(preset.tools)) {
update.tools =
preset.tools
Expand Down
5 changes: 3 additions & 2 deletions api/models/Prompt.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const {
removeGroupFromAllProjects,
} = require('./Project');
const { Prompt, PromptGroup } = require('./schema/promptSchema');
const { escapeRegExp } = require('~/server/utils');
const { logger } = require('~/config');

/**
Expand Down Expand Up @@ -106,7 +107,7 @@ const getAllPromptGroups = async (req, filter) => {
let searchShared = true;
let searchSharedOnly = false;
if (name) {
query.name = new RegExp(name, 'i');
query.name = new RegExp(escapeRegExp(name), 'i');
}
if (!query.category) {
delete query.category;
Expand Down Expand Up @@ -159,7 +160,7 @@ const getPromptGroups = async (req, filter) => {
let searchShared = true;
let searchSharedOnly = false;
if (name) {
query.name = new RegExp(name, 'i');
query.name = new RegExp(escapeRegExp(name), 'i');
}
if (!query.category) {
delete query.category;
Expand Down
37 changes: 25 additions & 12 deletions api/server/middleware/spec/validateImages.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ const validateImageRequest = require('~/server/middleware/validateImageRequest')

describe('validateImageRequest middleware', () => {
let req, res, next;
const validObjectId = '65cfb246f7ecadb8b1e8036b';

beforeEach(() => {
req = {
Expand Down Expand Up @@ -43,7 +44,7 @@ describe('validateImageRequest middleware', () => {

test('should return 403 if refresh token is expired', () => {
const expiredToken = jwt.sign(
{ id: '123', exp: Math.floor(Date.now() / 1000) - 3600 },
{ id: validObjectId, exp: Math.floor(Date.now() / 1000) - 3600 },
process.env.JWT_REFRESH_SECRET,
);
req.headers.cookie = `refreshToken=${expiredToken}`;
Expand All @@ -54,22 +55,34 @@ describe('validateImageRequest middleware', () => {

test('should call next() for valid image path', () => {
const validToken = jwt.sign(
{ id: '123', exp: Math.floor(Date.now() / 1000) + 3600 },
{ id: validObjectId, exp: Math.floor(Date.now() / 1000) + 3600 },
process.env.JWT_REFRESH_SECRET,
);
req.headers.cookie = `refreshToken=${validToken}`;
req.originalUrl = '/images/123/example.jpg';
req.originalUrl = `/images/${validObjectId}/example.jpg`;
validateImageRequest(req, res, next);
expect(next).toHaveBeenCalled();
});

test('should return 403 for invalid image path', () => {
const validToken = jwt.sign(
{ id: '123', exp: Math.floor(Date.now() / 1000) + 3600 },
{ id: validObjectId, exp: Math.floor(Date.now() / 1000) + 3600 },
process.env.JWT_REFRESH_SECRET,
);
req.headers.cookie = `refreshToken=${validToken}`;
req.originalUrl = '/images/456/example.jpg';
req.originalUrl = '/images/65cfb246f7ecadb8b1e8036c/example.jpg'; // Different ObjectId
validateImageRequest(req, res, next);
expect(res.status).toHaveBeenCalledWith(403);
expect(res.send).toHaveBeenCalledWith('Access Denied');
});

test('should return 403 for invalid ObjectId format', () => {
const validToken = jwt.sign(
{ id: validObjectId, exp: Math.floor(Date.now() / 1000) + 3600 },
process.env.JWT_REFRESH_SECRET,
);
req.headers.cookie = `refreshToken=${validToken}`;
req.originalUrl = '/images/123/example.jpg'; // Invalid ObjectId
validateImageRequest(req, res, next);
expect(res.status).toHaveBeenCalledWith(403);
expect(res.send).toHaveBeenCalledWith('Access Denied');
Expand All @@ -78,16 +91,16 @@ describe('validateImageRequest middleware', () => {
// File traversal tests
test('should prevent file traversal attempts', () => {
const validToken = jwt.sign(
{ id: '123', exp: Math.floor(Date.now() / 1000) + 3600 },
{ id: validObjectId, exp: Math.floor(Date.now() / 1000) + 3600 },
process.env.JWT_REFRESH_SECRET,
);
req.headers.cookie = `refreshToken=${validToken}`;

const traversalAttempts = [
'/images/123/../../../etc/passwd',
'/images/123/..%2F..%2F..%2Fetc%2Fpasswd',
'/images/123/image.jpg/../../../etc/passwd',
'/images/123/%2e%2e%2f%2e%2e%2f%2e%2e%2fetc%2fpasswd',
`/images/${validObjectId}/../../../etc/passwd`,
`/images/${validObjectId}/..%2F..%2F..%2Fetc%2Fpasswd`,
`/images/${validObjectId}/image.jpg/../../../etc/passwd`,
`/images/${validObjectId}/%2e%2e%2f%2e%2e%2f%2e%2e%2fetc%2fpasswd`,
];

traversalAttempts.forEach((attempt) => {
Expand All @@ -101,11 +114,11 @@ describe('validateImageRequest middleware', () => {

test('should handle URL encoded characters in valid paths', () => {
const validToken = jwt.sign(
{ id: '123', exp: Math.floor(Date.now() / 1000) + 3600 },
{ id: validObjectId, exp: Math.floor(Date.now() / 1000) + 3600 },
process.env.JWT_REFRESH_SECRET,
);
req.headers.cookie = `refreshToken=${validToken}`;
req.originalUrl = '/images/123/image%20with%20spaces.jpg';
req.originalUrl = `/images/${validObjectId}/image%20with%20spaces.jpg`;
validateImageRequest(req, res, next);
expect(next).toHaveBeenCalled();
});
Expand Down
23 changes: 23 additions & 0 deletions api/server/middleware/validateImageRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,24 @@ const cookies = require('cookie');
const jwt = require('jsonwebtoken');
const { logger } = require('~/config');

const OBJECT_ID_LENGTH = 24;
const OBJECT_ID_PATTERN = /^[0-9a-f]{24}$/i;

/**
* Validates if a string is a valid MongoDB ObjectId
* @param {string} id - String to validate
* @returns {boolean} - Whether string is a valid ObjectId format
*/
function isValidObjectId(id) {
if (typeof id !== 'string') {
return false;
}
if (id.length !== OBJECT_ID_LENGTH) {
return false;
}
return OBJECT_ID_PATTERN.test(id);
}

/**
* Middleware to validate image request.
* Must be set by `secureImageLinks` via custom config file.
Expand All @@ -25,6 +43,11 @@ function validateImageRequest(req, res, next) {
return res.status(403).send('Access Denied');
}

if (!isValidObjectId(payload.id)) {
logger.warn('[validateImageRequest] Invalid User ID');
return res.status(403).send('Access Denied');
}

const currentTimeInSeconds = Math.floor(Date.now() / 1000);
if (payload.exp < currentTimeInSeconds) {
logger.warn('[validateImageRequest] Refresh token expired');
Expand Down
8 changes: 7 additions & 1 deletion api/server/routes/files/files.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,13 @@ router.delete('/', async (req, res) => {
return;
}

await processDeleteRequest({ req, files });
const fileIds = files.map((file) => file.file_id);
const userFiles = await getFiles({ file_id: { $in: fileIds }, user: req.user.id });
if (userFiles.length !== files.length) {
return res.status(403).json({ message: 'You can only delete your own files' });
}

await processDeleteRequest({ req, files: userFiles });

logger.debug(
`[/files] Files deleted successfully: ${files
Expand Down
2 changes: 1 addition & 1 deletion api/server/routes/prompts.js
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ const createPrompt = async (req, res) => {
}
};

router.post('/', createPrompt);
router.post('/', checkPromptCreate, createPrompt);

/**
* Updates a prompt group
Expand Down
10 changes: 1 addition & 9 deletions api/server/services/Threads/manage.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ const {
} = require('librechat-data-provider');
const { retrieveAndProcessFile } = require('~/server/services/Files/process');
const { recordMessage, getMessages } = require('~/models/Message');
const { countTokens, escapeRegExp } = require('~/server/utils');
const { spendTokens } = require('~/models/spendTokens');
const { saveConvo } = require('~/models/Conversation');
const { countTokens } = require('~/server/utils');

/**
* Initializes a new thread or adds messages to an existing thread.
Expand Down Expand Up @@ -518,14 +518,6 @@ const recordUsage = async ({
const uniqueCitationStart = '^====||===';
const uniqueCitationEnd = '==|||||^';

/** Helper function to escape special characters in regex
* @param {string} string - The string to escape.
* @returns {string} The escaped string.
*/
function escapeRegExp(string) {
return string.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
}

/**
* Sorts, processes, and flattens messages to a single string.
*
Expand Down
10 changes: 9 additions & 1 deletion api/server/utils/citations.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,14 @@
const citationRegex = /\[\^\d+?\^\]/g;
const regex = / \[.*?]\(.*?\)/g;

/** Helper function to escape special characters in regex
* @param {string} string - The string to escape.
* @returns {string} The escaped string.
*/
function escapeRegExp(string) {
return string.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
}

const getCitations = (res) => {
const adaptiveCards = res.details.adaptiveCards;
const textBlocks = adaptiveCards && adaptiveCards[0].body;
Expand Down Expand Up @@ -47,4 +55,4 @@ const citeText = (res, noLinks = false) => {
return result;
};

module.exports = { getCitations, citeText };
module.exports = { getCitations, citeText, escapeRegExp };
Loading

0 comments on commit 3f3b592

Please sign in to comment.