🛡️ fix: validate message feedback payloads (#14500)

* fix: validate message feedback payloads

* fix: enforce feedback rating tag consistency

api/server/routes/__tests__/messages-feedback.spec.js

@@ -0,0 +1,154 @@
+const express = require('express');
+const request = require('supertest');
+
+jest.mock('@librechat/agents', () => ({
+  sleep: jest.fn(),
+}));
+
+jest.mock('@librechat/api', () => ({
+  unescapeLaTeX: jest.fn((value) => value),
+  countTokens: jest.fn().mockResolvedValue(10),
+  sendFeedbackScore: jest.fn().mockResolvedValue(undefined),
+  traceIdForMessage: jest.fn((messageId) => `trace-${messageId}`),
+}));
+
+jest.mock('@librechat/data-schemas', () => ({
+  ...jest.requireActual('@librechat/data-schemas'),
+  logger: {
+    debug: jest.fn(),
+    info: jest.fn(),
+    warn: jest.fn(),
+    error: jest.fn(),
+  },
+}));
+
+jest.mock('librechat-data-provider', () => ({
+  ...jest.requireActual('librechat-data-provider'),
+}));
+
+jest.mock('~/models', () => ({
+  saveConvo: jest.fn(),
+  getMessage: jest.fn(),
+  saveMessage: jest.fn(),
+  getMessages: jest.fn(),
+  updateMessage: jest.fn(),
+  deleteMessages: jest.fn(),
+  getConvosQueried: jest.fn(),
+  searchMessages: jest.fn(),
+  getMessagesByCursor: jest.fn(),
+}));
+
+jest.mock('~/server/services/Artifacts/update', () => ({
+  findAllArtifacts: jest.fn(),
+  replaceArtifactContent: jest.fn(),
+}));
+
+jest.mock('~/server/middleware', () => ({
+  requireJwtAuth: (req, res, next) => next(),
+  validateMessageReq: (req, res, next) => next(),
+  configMiddleware: (req, res, next) => next(),
+  sendValidationResponse: jest.fn(),
+  prepareMessageRequestValidation: jest.fn(),
+}));
+
+jest.mock('~/db/models', () => ({
+  Message: {
+    findOne: jest.fn(),
+    find: jest.fn(),
+    meiliSearch: jest.fn(),
+  },
+}));
+
+describe('PUT /:conversationId/:messageId/feedback', () => {
+  let app;
+  const { sendFeedbackScore } = require('@librechat/api');
+  const { updateMessage } = require('~/models');
+
+  beforeAll(() => {
+    const messagesRouter = require('../messages');
+
+    app = express();
+    app.use(express.json());
+    app.use((req, res, next) => {
+      req.user = { id: 'user-1' };
+      next();
+    });
+    app.use('/api/messages', messagesRouter);
+  });
+
+  beforeEach(() => {
+    jest.clearAllMocks();
+    updateMessage.mockImplementation((userId, { messageId, feedback }) =>
+      Promise.resolve({
+        messageId,
+        conversationId: 'conversation-1',
+        endpoint: 'openAI',
+        feedback,
+      }),
+    );
+  });
+
+  it('persists validated feedback and removes unknown fields', async () => {
+    const feedback = {
+      rating: 'thumbsDown',
+      tag: 'inaccurate',
+      text: 'The answer is incorrect',
+      ignored: 'value',
+    };
+
+    const response = await request(app)
+      .put('/api/messages/conversation-1/message-1/feedback')
+      .send({ feedback });
+
+    expect(response.status).toBe(200);
+    expect(updateMessage).toHaveBeenCalledWith(
+      'user-1',
+      {
+        messageId: 'message-1',
+        feedback: {
+          rating: 'thumbsDown',
+          tag: 'inaccurate',
+          text: 'The answer is incorrect',
+        },
+      },
+      { context: 'updateFeedback' },
+    );
+    expect(sendFeedbackScore).toHaveBeenCalledWith(
+      expect.objectContaining({
+        feedback: {
+          rating: 'thumbsDown',
+          tag: 'inaccurate',
+          text: 'The answer is incorrect',
+        },
+      }),
+    );
+  });
+
+  it.each([
+    ['an object tag', { rating: 'thumbsDown', tag: { key: 'inaccurate' } }],
+    ['a tag for the opposite rating', { rating: 'thumbsUp', tag: 'inaccurate' }],
+    ['oversized text', { rating: 'thumbsDown', tag: 'other', text: 'x'.repeat(1025) }],
+  ])('rejects %s before persistence or export', async (_name, feedback) => {
+    const response = await request(app)
+      .put('/api/messages/conversation-1/message-1/feedback')
+      .send({ feedback });
+
+    expect(response.status).toBe(400);
+    expect(response.body).toEqual({ error: 'Invalid feedback' });
+    expect(updateMessage).not.toHaveBeenCalled();
+    expect(sendFeedbackScore).not.toHaveBeenCalled();
+  });
+
+  it('preserves clearing feedback', async () => {
+    const response = await request(app)
+      .put('/api/messages/conversation-1/message-1/feedback')
+      .send({});
+
+    expect(response.status).toBe(200);
+    expect(updateMessage).toHaveBeenCalledWith(
+      'user-1',
+      { messageId: 'message-1', feedback: null },
+      { context: 'updateFeedback' },
+    );
+  });
+});

api/server/routes/messages.js

@@ -1,7 +1,7 @@
 const express = require('express');
 const { v4: uuidv4 } = require('uuid');
 const { logger } = require('@librechat/data-schemas');
-const { ContentTypes, isAssistantsEndpoint } = require('librechat-data-provider');
+const { ContentTypes, feedbackSchema, isAssistantsEndpoint } = require('librechat-data-provider');
 const {
   unescapeLaTeX,
   countTokens,
@@ -431,12 +431,17 @@ router.put(
     try {
       const { conversationId, messageId } = req.params;
       const { feedback } = req.body;
+      const feedbackResult = feedback == null ? null : feedbackSchema.safeParse(feedback);
+
+      if (feedbackResult && !feedbackResult.success) {
+        return res.status(400).json({ error: 'Invalid feedback' });
+      }
 
       const updatedMessage = await db.updateMessage(
         req?.user?.id,
         {
           messageId,
-          feedback: feedback || null,
+          feedback: feedbackResult?.data ?? null,
         },
         { context: 'updateFeedback' },
       );

packages/data-provider/src/feedback.spec.ts

@@ -0,0 +1,13 @@
+import { FEEDBACK_TAGS, feedbackSchema } from './feedback';
+
+describe('feedbackSchema', () => {
+  it.each(FEEDBACK_TAGS)('accepts $key with its $direction rating', ({ direction, key }) => {
+    expect(feedbackSchema.safeParse({ rating: direction, tag: key }).success).toBe(true);
+  });
+
+  it.each(FEEDBACK_TAGS)('rejects $key with the opposite rating', ({ direction, key }) => {
+    const oppositeRating = direction === 'thumbsUp' ? 'thumbsDown' : 'thumbsUp';
+
+    expect(feedbackSchema.safeParse({ rating: oppositeRating, tag: key }).success).toBe(false);
+  });
+});

packages/data-provider/src/feedback.ts

@@ -107,11 +107,22 @@ export function getTagsForRating(rating: TFeedbackRating): TFeedbackTag[] {
 export const feedbackTagKeySchema = z.enum(FEEDBACK_REASON_KEYS);
 export const feedbackRatingSchema = z.enum(FEEDBACK_RATINGS);
 
-export const feedbackSchema = z.object({
-  rating: feedbackRatingSchema,
-  tag: feedbackTagKeySchema,
-  text: z.string().max(1024).optional(),
-});
+export const feedbackSchema = z
+  .object({
+    rating: feedbackRatingSchema,
+    tag: feedbackTagKeySchema,
+    text: z.string().max(1024).optional(),
+  })
+  .refine(
+    ({ rating, tag }) =>
+      FEEDBACK_TAGS.some(
+        (feedbackTag) => feedbackTag.key === tag && feedbackTag.direction === rating,
+      ),
+    {
+      message: 'Feedback tag does not match rating',
+      path: ['tag'],
+    },
+  );
 
 export type TMinimalFeedback = z.infer<typeof feedbackSchema>;