Merge pull request #2324 from Giveth/security/xss-sanitize-project-updates

Sanitize project update HTML to prevent stored XSS

Author: ae2079

package-lock.json

@@ -65,6 +65,7 @@
         "patch-package": "^6.5.1",
         "rate-limit-redis": "^4.2.0",
         "reflect-metadata": "^0.1.13",
+        "sanitize-html": "^2.17.4",
         "siwe": "^3.0.0",
         "slugify": "^1.4.7",
         "stripe": "^8.137.0",
@@ -96,6 +97,7 @@
         "@types/mocha": "^8.2.1",
         "@types/node": "^14.14.31",
         "@types/node-cron": "^3.0.0",
+        "@types/sanitize-html": "^2.16.1",
         "@typescript-eslint/eslint-plugin": "^7.2.0",
         "@typescript-eslint/parser": "^7.2.0",
         "chai": "^4.3.0",
@@ -273,4 +275,4 @@
         }
     },
     "license": "ISC"
-}
+}

package.json

@@ -1,6 +1,7 @@
 import { Field, Float, ID, ObjectType } from 'type-graphql';
 import {
   AfterInsert,
+  AfterLoad,
   AfterUpdate,
   BaseEntity,
   BeforeInsert,
@@ -32,6 +33,7 @@ import { findUserById } from '../repositories/userRepository';
 import { EstimatedMatchingByQfRound } from '../types/qfTypes';
 import { i18n, translationErrorMessagesKeys } from '../utils/errorMessages';
 import { getHtmlTextSummary } from '../utils/utils';
+import { sanitizeProjectRichText } from '../utils/htmlSanitizer';
 import { ProjectFuturePowerView } from '../views/projectFuturePowerView';
 import { ProjectInstantPowerView } from '../views/projectInstantPowerView';
 import { ProjectPowerView } from '../views/projectPowerView';
@@ -640,6 +642,16 @@ export class Project extends BaseEntity {
     this.descriptionSummary = getHtmlTextSummary(this.description);
   }
 
+  // Defense in depth for rows persisted before sanitize-on-write was added.
+  // Sanitizing here is idempotent on already-clean content, so newly written
+  // rows pay only a small CPU cost on read.
+  @AfterLoad()
+  sanitizeProjectDescriptionOnLoad() {
+    if (this.description) {
+      this.description = sanitizeProjectRichText(this.description);
+    }
+  }
+
   // we should not expose this field to the client
   @Column('text', { nullable: true })
   fundingPoolHdPath?: string;
@@ -839,6 +851,16 @@ export class ProjectUpdate extends BaseEntity {
   setProjectUpdateContentSummary() {
     this.contentSummary = getHtmlTextSummary(this.content);
   }
+
+  // Defense in depth for rows persisted before sanitize-on-write was added
+  // (the original stored-XSS path was via featuredProjectUpdate.content).
+  // Sanitizing here is idempotent on already-clean content.
+  @AfterLoad()
+  sanitizeProjectUpdateContentOnLoad() {
+    if (this.content) {
+      this.content = sanitizeProjectRichText(this.content);
+    }
+  }
 }
 
 @ObjectType()

src/entities/project.ts

@@ -39,6 +39,11 @@ import {
   creteSlugFromProject,
   titleWithoutSpecialCharacters,
 } from '../utils/utils';
+import {
+  getRichTextPlainLength,
+  sanitizeProjectRichText,
+} from '../utils/htmlSanitizer';
+import { PROJECT_DESCRIPTION_MAX_LENGTH } from '../constants/validators';
 import { Category } from '../entities/category';
 import { Organization, ORGANIZATION_LABELS } from '../entities/organization';
 import { ProjectStatus } from '../entities/projectStatus';
@@ -262,6 +267,12 @@ export class CauseResolver {
         i18n.__(translationErrorMessagesKeys.YOU_ARE_NOT_THE_OWNER_OF_PROJECT),
       );
 
+    if (newProjectData.description) {
+      newProjectData.description = sanitizeProjectRichText(
+        newProjectData.description,
+      );
+    }
+
     for (const field in newProjectData) {
       if (field === 'addresses' || field === 'socialMedia') {
         // We will take care of addresses and relations manually
@@ -477,6 +488,16 @@ export class CauseResolver {
         throw new Error(i18n.__(translationErrorMessagesKeys.INVALID_INPUT));
       }
 
+      if (
+        getRichTextPlainLength(description) > PROJECT_DESCRIPTION_MAX_LENGTH
+      ) {
+        throw new Error(
+          i18n.__(
+            translationErrorMessagesKeys.PROJECT_DESCRIPTION_LENGTH_SIZE_EXCEEDED,
+          ),
+        );
+      }
+
       // Validate chainId is a polygon chain id
       if (typeof chainId !== 'number' || isNaN(chainId) || chainId !== 137) {
         throw new Error(i18n.__(translationErrorMessagesKeys.INVALID_CHAIN_ID));
@@ -592,7 +613,7 @@ export class CauseResolver {
 
       const causeData = {
         title: convert(title.trim()),
-        description: description.trim(),
+        description: sanitizeProjectRichText(description.trim()),
         chainId,
         slug,
         slugHistory: [],

src/resolvers/causeResolver.ts

@@ -6298,6 +6298,61 @@ function editProjectUpdateTestCases() {
       errorMessages.AUTHENTICATION_REQUIRED,
     );
   });
+  it('should strip XSS payloads from edited project update content', async () => {
+    const user = await saveUserDirectlyToDb(generateRandomEtheriumAddress());
+    const accessToken = await generateTestAccessToken(user.id);
+    const project = await saveProjectDirectlyToDb({
+      ...createProjectData(),
+      adminUserId: user.id,
+    });
+    const updateProject = await ProjectUpdate.create({
+      userId: user.id,
+      projectId: project.id,
+      content: 'safe original',
+      title: 'safe original title',
+      createdAt: new Date(),
+      isMain: false,
+    }).save();
+
+    const maliciousContent =
+      '<p>hello</p><script>alert(1)</script><img src="x" onerror="alert(2)"><a href="javascript:alert(3)">x</a>';
+
+    const result = await axios.post(
+      graphqlUrl,
+      {
+        query: editProjectUpdateQuery,
+        variables: {
+          updateId: updateProject.id,
+          content: maliciousContent,
+          title: 'still safe title',
+        },
+      },
+      {
+        headers: {
+          Authorization: `Bearer ${accessToken}`,
+        },
+      },
+    );
+
+    const returned = result.data.data.editProjectUpdate.content;
+    assert.notInclude(returned, '<script');
+    assert.notInclude(returned, 'onerror');
+    assert.notInclude(returned, 'javascript:');
+    assert.include(returned, '<p>hello</p>');
+
+    // Persistence-level check: the @AfterLoad hook also sanitizes on read,
+    // so a regression that skipped write-time sanitization could be hidden
+    // by the GraphQL response. Read the row directly from the database to
+    // verify the payload was sanitized *before* being persisted.
+    const [storedRow] = await AppDataSource.getDataSource().query(
+      'SELECT content FROM public.project_update WHERE id = $1',
+      [updateProject.id],
+    );
+    assert.notInclude(storedRow.content, '<script');
+    assert.notInclude(storedRow.content, 'onerror');
+    assert.notInclude(storedRow.content, 'javascript:');
+    assert.include(storedRow.content, '<p>hello</p>');
+  });
 }
 
 function deleteProjectUpdateTestCases() {

src/resolvers/projectResolver.test.ts

@@ -97,6 +97,10 @@ import {
 } from '../repositories/projectRepository';
 import { sortTokensByOrderAndAlphabets } from '../utils/tokenUtils';
 import { getNotificationAdapter } from '../adapters/adaptersFactory';
+import {
+  getRichTextPlainLength,
+  sanitizeProjectRichText,
+} from '../utils/htmlSanitizer';
 import { NETWORK_IDS } from '../provider';
 import { getVerificationFormStatusByProjectId } from '../repositories/projectVerificationRepository';
 import {
@@ -114,7 +118,10 @@ import { creteSlugFromProject, isSocialMediaEqual } from '../utils/utils';
 import { findCampaignBySlug } from '../repositories/campaignRepository';
 import { Campaign } from '../entities/campaign';
 import { FeaturedUpdate } from '../entities/featuredUpdate';
-import { PROJECT_UPDATE_CONTENT_MAX_LENGTH } from '../constants/validators';
+import {
+  PROJECT_DESCRIPTION_MAX_LENGTH,
+  PROJECT_UPDATE_CONTENT_MAX_LENGTH,
+} from '../constants/validators';
 import { calculateGivbackFactor } from '../services/givbackService';
 import { ProjectBySlugResponse } from './types/projectResolver';
 import { ChainType } from '../types/network';
@@ -1508,6 +1515,12 @@ export class ProjectResolver {
         i18n.__(translationErrorMessagesKeys.YOU_ARE_NOT_THE_OWNER_OF_PROJECT),
       );
 
+    if (newProjectData.description) {
+      newProjectData.description = sanitizeProjectRichText(
+        newProjectData.description,
+      );
+    }
+
     for (const field in newProjectData) {
       if (field === 'addresses' || field === 'socialMedia') {
         // We will take care of addresses and relations manually
@@ -1774,7 +1787,23 @@ export class ProjectResolver {
     @Ctx() ctx: ApolloContext,
   ): Promise<Project> {
     const user = await getLoggedInUser(ctx);
-    const { image, description } = projectInput;
+    const { image } = projectInput;
+    if (projectInput.description) {
+      if (
+        getRichTextPlainLength(projectInput.description) >
+        PROJECT_DESCRIPTION_MAX_LENGTH
+      ) {
+        throw new Error(
+          i18n.__(
+            translationErrorMessagesKeys.PROJECT_DESCRIPTION_LENGTH_SIZE_EXCEEDED,
+          ),
+        );
+      }
+      projectInput.description = sanitizeProjectRichText(
+        projectInput.description,
+      );
+    }
+    const { description } = projectInput;
 
     const dbUser = await findUserById(user.id);
 
@@ -1974,17 +2003,16 @@ export class ProjectResolver {
         i18n.__(translationErrorMessagesKeys.AUTHENTICATION_REQUIRED),
       );
 
-    if (
-      content?.replace(/<[^>]+>/g, '')?.length >
-      PROJECT_UPDATE_CONTENT_MAX_LENGTH
-    ) {
+    if (getRichTextPlainLength(content) > PROJECT_UPDATE_CONTENT_MAX_LENGTH) {
       throw new Error(
         i18n.__(
           translationErrorMessagesKeys.PROJECT_UPDATE_CONTENT_LENGTH_SIZE_EXCEEDED,
         ),
       );
     }
 
+    const sanitizedContent = sanitizeProjectRichText(content);
+
     const owner = await findUserById(user.userId);
 
     if (!owner)
@@ -2007,7 +2035,7 @@ export class ProjectResolver {
     const update = ProjectUpdate.create({
       userId: user.userId,
       projectId: project.id,
-      content,
+      content: sanitizedContent,
       title,
       createdAt: new Date(),
       isMain: false,
@@ -2039,16 +2067,11 @@ export class ProjectResolver {
       throw new Error(
         i18n.__(translationErrorMessagesKeys.AUTHENTICATION_REQUIRED),
       );
-    if (
-      content?.replace(/<[^>]+>/g, '')?.length >
-      PROJECT_UPDATE_CONTENT_MAX_LENGTH
-    ) {
-      throw new Error(
-        i18n.__(
-          translationErrorMessagesKeys.PROJECT_UPDATE_CONTENT_LENGTH_SIZE_EXCEEDED,
-        ),
-      );
-    }
+
+    // Intentionally no length check here: legacy project_update rows can
+    // exceed the current max, and rejecting on edit would block owners from
+    // fixing or trimming long content. New content is still capped via
+    // addProjectUpdate. (Per RamRamez's review on PR #2324.)
 
     const owner = await findUserById(user.userId);
 
@@ -2075,7 +2098,7 @@ export class ProjectResolver {
       );
 
     update.title = title;
-    update.content = content;
+    update.content = sanitizeProjectRichText(content);
     await update.save();
     await update.reload();
 

src/resolvers/projectResolver.ts

@@ -400,6 +400,8 @@ export const translationErrorMessagesKeys = {
     'REGISTERED_NON_PROFITS_CATEGORY_DOESNT_EXIST',
   PROJECT_UPDATE_CONTENT_LENGTH_SIZE_EXCEEDED:
     'PROJECT_UPDATE_CONTENT_LENGTH_SIZE_EXCEEDED',
+  PROJECT_DESCRIPTION_LENGTH_SIZE_EXCEEDED:
+    'PROJECT_DESCRIPTION_LENGTH_SIZE_EXCEEDED',
   DRAFT_DONATION_DISABLED: 'DRAFT_DONATION_DISABLED',
   DRAFT_RECURRING_DONATION_DISABLED: 'DRAFT_RECURRING_DONATION_DISABLED',
   EVM_SUPPORT_ONLY: 'EVM_SUPPORT_ONLY',