From bd69c29023554c5dcf8cb361227ba4ebf0d1ac08 Mon Sep 17 00:00:00 2001 From: seaerchin Date: Fri, 31 Mar 2023 12:04:09 +0800 Subject: [PATCH 1/3] fix(review): return 200 for unmigrated sites chore(review): remove check on POST route tests(review): fixed tests tests(integration): fixed review tests chore(review): update function name fix(reviews): update spy fix(review): check for migrated site but not site members --- src/integration/Reviews.spec.ts | 9 +- .../v2/authenticated/__tests__/review.spec.ts | 12 +- src/routes/v2/authenticated/review.ts | 106 +++++++++++++++++- src/server.js | 3 +- 4 files changed, 122 insertions(+), 8 deletions(-) diff --git a/src/integration/Reviews.spec.ts b/src/integration/Reviews.spec.ts index 5aa024117..cfbb48e10 100644 --- a/src/integration/Reviews.spec.ts +++ b/src/integration/Reviews.spec.ts @@ -113,13 +113,17 @@ const ReviewsRouter = new _ReviewsRouter( usersService, sitesService, collaboratorsService, - notificationsService + notificationsService, + gitHubService ) const reviewsSubrouter = ReviewsRouter.getRouter() const subrouter = express() subrouter.use("/:siteName", reviewsSubrouter) const mockGenericAxios = mockAxios.create() +const migrateSpy = jest + .spyOn(ReviewsRouter, "checkIfSiteIsUnmigrated") + .mockResolvedValue(true) describe("Review Requests Integration Tests", () => { beforeAll(async () => { @@ -477,6 +481,7 @@ describe("Review Requests Integration Tests", () => { MOCK_USER_SESSION_DATA_ONE, MOCK_REPO_NAME_TWO ) + migrateSpy.mockResolvedValueOnce(false) // Act const actual = await request(app).get(`/${MOCK_REPO_NAME_TWO}/summary`) @@ -722,6 +727,7 @@ describe("Review Requests Integration Tests", () => { MOCK_USER_SESSION_DATA_ONE, MOCK_REPO_NAME_TWO ) + migrateSpy.mockResolvedValueOnce(false) // Act const actual = await request(app).get( @@ -1617,6 +1623,7 @@ describe("Review Requests Integration Tests", () => { MOCK_USER_SESSION_DATA_TWO, MOCK_REPO_NAME_TWO ) + migrateSpy.mockResolvedValueOnce(false) // Act const actual = await request(app).get( diff --git a/src/routes/v2/authenticated/__tests__/review.spec.ts b/src/routes/v2/authenticated/__tests__/review.spec.ts index ba3cea684..6d9cecf53 100644 --- a/src/routes/v2/authenticated/__tests__/review.spec.ts +++ b/src/routes/v2/authenticated/__tests__/review.spec.ts @@ -11,6 +11,7 @@ import { generateRouterForDefaultUserWithSite } from "@fixtures/app" import { mockUserId } from "@fixtures/identity" import { MOCK_USER_EMAIL_ONE, MOCK_USER_EMAIL_TWO } from "@fixtures/users" import { CollaboratorRoles, ReviewRequestStatus } from "@root/constants" +import { GitHubService } from "@root/services/db/GitHubService" import CollaboratorsService from "@services/identity/CollaboratorsService" import NotificationsService from "@services/identity/NotificationsService" import SitesService from "@services/identity/SitesService" @@ -18,6 +19,9 @@ import UsersService from "@services/identity/UsersService" import ReviewRequestService from "@services/review/ReviewRequestService" describe("Review Requests Router", () => { + const mockGithubService = { + getRepoInfo: jest.fn().mockResolvedValue(true), + } const mockReviewRequestService = { approveReviewRequest: jest.fn(), closeReviewRequest: jest.fn(), @@ -60,7 +64,8 @@ describe("Review Requests Router", () => { (mockIdentityUsersService as unknown) as UsersService, (mockSitesService as unknown) as SitesService, (mockCollaboratorsService as unknown) as CollaboratorsService, - (mockNotificationsService as unknown) as NotificationsService + (mockNotificationsService as unknown) as NotificationsService, + (mockGithubService as unknown) as GitHubService ) const subrouter = express() @@ -138,6 +143,7 @@ describe("Review Requests Router", () => { mockReviewRequestService.compareDiff.mockResolvedValueOnce( mockFilesChanged ) + mockSitesService.getBySiteName.mockResolvedValueOnce(true) // Act const response = await request(app).get("/mockSite/review/compare") @@ -152,6 +158,7 @@ describe("Review Requests Router", () => { it("should return 404 if user is not a site member", async () => { // Arrange mockIdentityUsersService.getSiteMember.mockResolvedValueOnce(null) + mockSitesService.getBySiteName.mockResolvedValueOnce(true) // Act const response = await request(app).get("/mockSite/review/compare") @@ -335,6 +342,7 @@ describe("Review Requests Router", () => { it("should return 404 if the site does not exist", async () => { // Arrange mockSitesService.getBySiteName.mockResolvedValueOnce(null) + mockGithubService.getRepoInfo.mockRejectedValueOnce(false) // Act const response = await request(app).get("/mockSite/review/summary") @@ -519,6 +527,7 @@ describe("Review Requests Router", () => { it("should return 404 if the site does not exist", async () => { // Arrange mockSitesService.getBySiteName.mockResolvedValueOnce(null) + mockGithubService.getRepoInfo.mockRejectedValueOnce(false) // Act const response = await request(app).get(`/mockSite/review/12345`) @@ -913,6 +922,7 @@ describe("Review Requests Router", () => { it("should return 404 if the site does not exist", async () => { // Arrange mockSitesService.getBySiteName.mockResolvedValueOnce(null) + mockGithubService.getRepoInfo.mockRejectedValueOnce(false) // Act const response = await request(app).get(`/mockSite/review/12345/comments`) diff --git a/src/routes/v2/authenticated/review.ts b/src/routes/v2/authenticated/review.ts index ab69a363a..1ae3f98d3 100644 --- a/src/routes/v2/authenticated/review.ts +++ b/src/routes/v2/authenticated/review.ts @@ -14,6 +14,7 @@ import UserWithSiteSessionData from "@classes/UserWithSiteSessionData" import { CollaboratorRoles, ReviewRequestStatus } from "@root/constants" import { SiteMember, User } from "@root/database/models" +import { GitHubService } from "@root/services/db/GitHubService" import CollaboratorsService from "@root/services/identity/CollaboratorsService" import NotificationsService from "@root/services/identity/NotificationsService" import SitesService from "@root/services/identity/SitesService" @@ -41,25 +42,50 @@ export class ReviewsRouter { private readonly notificationsService + private readonly githubService + constructor( reviewRequestService: ReviewRequestService, identityUsersService: UsersService, sitesService: SitesService, collaboratorsService: CollaboratorsService, - notificationsService: NotificationsService + notificationsService: NotificationsService, + githubService: GitHubService ) { this.reviewRequestService = reviewRequestService this.identityUsersService = identityUsersService this.sitesService = sitesService this.collaboratorsService = collaboratorsService this.notificationsService = notificationsService + this.githubService = githubService autoBind(this) } + // NOTE: This is required because we want to distinguish + // between unmigrated sites that don't exist in our db + // vs sites that truly don't exist. + // This is to avoid flooding the frontend with 404s + // and triggering false alarms + // TODO (IS-58): Remove this function and related calls when + // all sites are migrated over to our db + checkIfSiteIsUnmigrated = async ( + sessionData: UserWithSiteSessionData + ): Promise => { + try { + // Site exists on github but isn't migrated + await this.githubService.getRepoInfo(sessionData) + return true + } catch { + // If there are any errors, we failed to fetch the site + // and the site is assumed to not exist. + return false + } + } + compareDiff: RequestHandler< { siteName: string }, - { items: EditedItemDto[] }, + { items: EditedItemDto[] } | ResponseErrorBody, unknown, unknown, { userWithSiteSessionData: UserWithSiteSessionData } @@ -70,6 +96,21 @@ export class ReviewsRouter { const { userWithSiteSessionData } = res.locals const { siteName } = req.params + const site = await this.sitesService.getBySiteName(siteName) + const doesUnmigratedSiteExist = await this.checkIfSiteIsUnmigrated( + userWithSiteSessionData + ) + + if (!site && doesUnmigratedSiteExist) + return res.status(200).json({ message: "Unmigrated site" }) + + const siteMembers = await this.collaboratorsService.list(siteName) + // NOTE: This is an initial migrated site but + // we haven't migrated the users. + if (siteMembers.length === 0 && site) { + return res.status(200).json({ message: "Unmigrated users" }) + } + // Check if they have access to site const possibleSiteMember = await this.identityUsersService.getSiteMember( userWithSiteSessionData.isomerUserId, @@ -77,7 +118,7 @@ export class ReviewsRouter { ) if (!possibleSiteMember) { - return res.status(404).send() + return res.status(404).json({ message: "No site members found" }) } const files = await this.reviewRequestService.compareDiff( @@ -226,6 +267,19 @@ export class ReviewsRouter { const { siteName } = req.params const site = await this.sitesService.getBySiteName(siteName) const { userWithSiteSessionData } = res.locals + const doesUnmigratedSiteExist = await this.checkIfSiteIsUnmigrated( + userWithSiteSessionData + ) + + if (!site && doesUnmigratedSiteExist) + return res.status(200).json({ message: "Unmigrated site" }) + + const siteMembers = await this.collaboratorsService.list(siteName) + // NOTE: This is an initial migrated site but + // we haven't migrated the users. + if (siteMembers.length === 0 && site) { + return res.status(200).json({ message: "Unmigrated users" }) + } if (!site) { logger.error({ @@ -395,6 +449,20 @@ export class ReviewsRouter { const { siteName, requestId } = req.params const { userWithSiteSessionData } = res.locals const site = await this.sitesService.getBySiteName(siteName) + const doesUnmigratedSiteExist = await this.checkIfSiteIsUnmigrated( + userWithSiteSessionData + ) + + if (!site && doesUnmigratedSiteExist) { + return res.status(200).json({ message: "Unmigrated site" }) + } + + const siteMembers = await this.collaboratorsService.list(siteName) + // NOTE: This is an initial migrated site but + // we haven't migrated the users. + if (siteMembers.length === 0 && site) { + return res.status(200).json({ message: "Unmigrated users" }) + } if (!site) { logger.error({ @@ -760,6 +828,21 @@ export class ReviewsRouter { const { userWithSiteSessionData } = res.locals // Step 1: Check that the site exists const site = await this.sitesService.getBySiteName(siteName) + const doesUnmigratedSiteExist = await this.checkIfSiteIsUnmigrated( + userWithSiteSessionData + ) + + if (!site && doesUnmigratedSiteExist) { + return res.status(200).json({ message: "Unmigrated site" }) + } + + const siteMembers = await this.collaboratorsService.list(siteName) + // NOTE: This is an initial migrated site but + // we haven't migrated the users. + if (siteMembers.length === 0 && site) { + return res.status(200).json({ message: "Unmigrated users" }) + } + if (!site) { logger.error({ message: "Invalid site requested", @@ -932,11 +1015,10 @@ export class ReviewsRouter { }) } - // Step 2: Check that user exists. + // Step 2. Check that user exists. // Having session data is proof that this user exists // as otherwise, they would be rejected by our middleware const { userWithSiteSessionData } = res.locals - // Check if they are a collaborator const role = await this.collaboratorsService.getRole( siteName, @@ -1155,6 +1237,20 @@ export class ReviewsRouter { const { path } = req.query const { userWithSiteSessionData } = res.locals const site = await this.sitesService.getBySiteName(siteName) + const doesUnmigratedSiteExist = await this.checkIfSiteIsUnmigrated( + userWithSiteSessionData + ) + + if (!site && doesUnmigratedSiteExist) { + return res.status(200).json({ message: "Unmigrated site" }) + } + + const siteMembers = await this.collaboratorsService.list(siteName) + // NOTE: This is an initial migrated site but + // we haven't migrated the users. + if (siteMembers.length === 0 && site) { + return res.status(200).json({ message: "Unmigrated users" }) + } if (!site) { logger.error({ diff --git a/src/server.js b/src/server.js index f8e80b693..cd9ec6aba 100644 --- a/src/server.js +++ b/src/server.js @@ -198,7 +198,8 @@ const reviewRouter = new ReviewsRouter( usersService, sitesService, collaboratorsService, - notificationsService + notificationsService, + gitHubService ) const authenticatedSubrouterV1 = getAuthenticatedSubrouterV1({ authenticationMiddleware, From c25f448813e1addce67d0209375ec35ef9cb6c7b Mon Sep 17 00:00:00 2001 From: seaerchin Date: Fri, 31 Mar 2023 15:50:48 +0800 Subject: [PATCH 2/3] fix(sanitize): use same setup for dompurify as FE fix(markdown): correct .split --- src/services/utilServices/Sanitizer.ts | 42 ++++++++++++++++++++++++++ src/utils/file-upload-utils.js | 11 ++++--- src/utils/markdown-utils.js | 12 ++++---- src/utils/yaml-utils.ts | 7 +++-- 4 files changed, 59 insertions(+), 13 deletions(-) create mode 100644 src/services/utilServices/Sanitizer.ts diff --git a/src/services/utilServices/Sanitizer.ts b/src/services/utilServices/Sanitizer.ts new file mode 100644 index 000000000..741306547 --- /dev/null +++ b/src/services/utilServices/Sanitizer.ts @@ -0,0 +1,42 @@ +import DOMPurify from "isomorphic-dompurify" + +DOMPurify.setConfig({ + ADD_TAGS: ["iframe", "#comment", "script"], + ADD_ATTR: [ + "allow", + "allowfullscreen", + "frameborder", + "scrolling", + "marginheight", + "marginwidth", + "target", + "async", + ], + // required in case