Skip to content

Commit 57be05c

Browse files
authored
fix(backend): prevent IDOR in user collection and request endpoints (hoppscotch#5902)
1 parent 02b3dbc commit 57be05c

10 files changed

Lines changed: 329 additions & 230 deletions

File tree

packages/hoppscotch-backend/src/errors.ts

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -787,15 +787,13 @@ export const INFRA_CONFIG_OPERATION_NOT_ALLOWED =
787787
* Error message for when the onboarding status fetch fails
788788
* (InfraConfigService)
789789
*/
790-
export const INFRA_CONFIG_FETCH_FAILED =
791-
'infra_config/fetch_failed' as const;
790+
export const INFRA_CONFIG_FETCH_FAILED = 'infra_config/fetch_failed' as const;
792791

793792
/**
794793
* Onboarding has already been completed and cannot be re-run
795794
* (OnboardingController)
796795
*/
797-
export const ONBOARDING_CANNOT_BE_RERUN =
798-
'onboarding/cannot_be_rerun' as const;
796+
export const ONBOARDING_CANNOT_BE_RERUN = 'onboarding/cannot_be_rerun' as const;
799797

800798
/**
801799
* Error message for when the database table does not exist

packages/hoppscotch-backend/src/team-collection/team-collection.service.spec.ts

Lines changed: 10 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -893,7 +893,10 @@ describe('deleteCollection', () => {
893893
.spyOn(teamCollectionService, 'getCollection')
894894
.mockResolvedValueOnce(E.right(rootTeamCollection));
895895
jest
896-
.spyOn(teamCollectionService as any, 'deleteCollectionAndUpdateSiblingsOrderIndex')
896+
.spyOn(
897+
teamCollectionService as any,
898+
'deleteCollectionAndUpdateSiblingsOrderIndex',
899+
)
897900
.mockResolvedValueOnce(E.left(TEAM_COL_REORDERING_FAILED));
898901

899902
const result = await teamCollectionService.deleteCollection(
@@ -1667,23 +1670,6 @@ describe('FIX: updateMany queries now include teamID filter for root collections
16671670
mockReset(mockPrisma);
16681671
});
16691672

1670-
const team2: Team = {
1671-
id: 'team_2',
1672-
name: 'Team 2',
1673-
};
1674-
1675-
// Team 2's root collection - should NOT be affected by Team 1's operations
1676-
const team2RootCollection: DBTeamCollection = {
1677-
id: 'team2-root-coll',
1678-
orderIndex: 1,
1679-
parentID: null,
1680-
title: 'Team 2 Root Collection',
1681-
teamID: team2.id,
1682-
data: {},
1683-
createdOn: currentTime,
1684-
updatedOn: currentTime,
1685-
};
1686-
16871673
test('FIX: deleteCollection - updateMany now correctly filters by teamID for root collections', async () => {
16881674
/**
16891675
* Scenario: Team 1 deletes a root collection
@@ -1717,7 +1703,8 @@ describe('FIX: updateMany queries now include teamID filter for root collections
17171703
await teamCollectionService.deleteCollection(team1RootToDelete.id);
17181704

17191705
// Get the updateMany call from the transaction
1720-
const updateManyCall = mockPrisma.teamCollection.updateMany.mock.calls[0][0];
1706+
const updateManyCall =
1707+
mockPrisma.teamCollection.updateMany.mock.calls[0][0];
17211708

17221709
// FIX VERIFICATION: The query now correctly includes teamID
17231710
// This ensures only Team 1's root collections are affected
@@ -1769,7 +1756,8 @@ describe('FIX: updateMany queries now include teamID filter for root collections
17691756
);
17701757

17711758
// Get the actual updateMany call arguments
1772-
const updateManyCall = mockPrisma.teamCollection.updateMany.mock.calls[0][0];
1759+
const updateManyCall =
1760+
mockPrisma.teamCollection.updateMany.mock.calls[0][0];
17731761

17741762
// FIX VERIFICATION: The query now correctly includes teamID
17751763
expect(updateManyCall.where).toEqual({
@@ -1830,7 +1818,8 @@ describe('FIX: updateMany queries now include teamID filter for root collections
18301818
);
18311819

18321820
// Get the actual updateMany call arguments
1833-
const updateManyCall = mockPrisma.teamCollection.updateMany.mock.calls[0][0];
1821+
const updateManyCall =
1822+
mockPrisma.teamCollection.updateMany.mock.calls[0][0];
18341823

18351824
// FIX VERIFICATION: The query now correctly includes teamID
18361825
expect(updateManyCall.where).toEqual({

packages/hoppscotch-backend/src/team-collection/team-collection.service.ts

Lines changed: 72 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -216,7 +216,11 @@ export class TeamCollectionService {
216216
await this.prisma.$transaction(async (tx) => {
217217
try {
218218
// lock the rows
219-
await this.prisma.lockTeamCollectionByTeamAndParent(tx, teamID, parentID);
219+
await this.prisma.lockTeamCollectionByTeamAndParent(
220+
tx,
221+
teamID,
222+
parentID,
223+
);
220224

221225
// Get the last order index
222226
const lastEntry = await tx.teamCollection.findFirst({
@@ -397,15 +401,18 @@ export class TeamCollectionService {
397401
* @param collectionID The collection ID
398402
* @returns An Either of the Collection details
399403
*/
400-
async getCollection(collectionID: string, tx: Prisma.TransactionClient | null = null) {
404+
async getCollection(
405+
collectionID: string,
406+
tx: Prisma.TransactionClient | null = null,
407+
) {
401408
try {
402-
const teamCollection = await (tx || this.prisma).teamCollection.findUniqueOrThrow(
403-
{
404-
where: {
405-
id: collectionID,
406-
},
409+
const teamCollection = await (
410+
tx || this.prisma
411+
).teamCollection.findUniqueOrThrow({
412+
where: {
413+
id: collectionID,
407414
},
408-
);
415+
});
409416
return E.right(teamCollection);
410417
} catch (error) {
411418
return E.left(TEAM_COLL_NOT_FOUND);
@@ -469,7 +476,11 @@ export class TeamCollectionService {
469476
teamCollection = await this.prisma.$transaction(async (tx) => {
470477
try {
471478
// lock the rows
472-
await this.prisma.lockTeamCollectionByTeamAndParent(tx, teamID, parentID);
479+
await this.prisma.lockTeamCollectionByTeamAndParent(
480+
tx,
481+
teamID,
482+
parentID,
483+
);
473484

474485
// fetch last collection
475486
const lastCollection = await tx.teamCollection.findFirst({
@@ -552,15 +563,19 @@ export class TeamCollectionService {
552563
await this.prisma.$transaction(async (tx) => {
553564
try {
554565
// lock the rows
555-
await this.prisma.lockTeamCollectionByTeamAndParent(tx, collection.teamID, collection.parentID);
566+
await this.prisma.lockTeamCollectionByTeamAndParent(
567+
tx,
568+
collection.teamID,
569+
collection.parentID,
570+
);
556571

557572
const deletedCollection = await tx.teamCollection.delete({
558573
where: { id: collection.id },
559574
});
560-
575+
561576
// if collection is deleted, update siblings orderIndexes
562577
// if collection was deleted before the transaction started (race condition), do not update siblings orderIndexes
563-
if (deletedCollection) {
578+
if (deletedCollection) {
564579
// update siblings orderIndexes
565580
await tx.teamCollection.updateMany({
566581
where: {
@@ -623,7 +638,7 @@ export class TeamCollectionService {
623638
);
624639

625640
return E.right(true);
626-
}
641+
}
627642

628643
/**
629644
* Change parentID of TeamCollection's
@@ -638,11 +653,10 @@ export class TeamCollectionService {
638653
newParentID: string | null,
639654
) {
640655
// fetch last collection
641-
const lastCollectionUnderNewParent =
642-
await tx.teamCollection.findFirst({
643-
where: { teamID: collection.teamID, parentID: newParentID },
644-
orderBy: { orderIndex: 'desc' },
645-
});
656+
const lastCollectionUnderNewParent = await tx.teamCollection.findFirst({
657+
where: { teamID: collection.teamID, parentID: newParentID },
658+
orderBy: { orderIndex: 'desc' },
659+
});
646660

647661
// decrement orderIndex of all next sibling collections from original collection
648662
await tx.teamCollection.updateMany({
@@ -736,47 +750,52 @@ export class TeamCollectionService {
736750
const collection = await this.getCollection(collectionID, tx);
737751
if (E.isLeft(collection)) return E.left(collection.left);
738752
// lock the rows of the collection and its siblings
739-
await this.prisma.lockTeamCollectionByTeamAndParent(tx, collection.right.teamID, collection.right.parentID);
753+
await this.prisma.lockTeamCollectionByTeamAndParent(
754+
tx,
755+
collection.right.teamID,
756+
collection.right.parentID,
757+
);
740758
// destCollectionID == null i.e move collection to root
741759
if (!destCollectionID) {
742760
if (!collection.right.parentID) {
743761
// collection is a root collection
744762
// Throw error if collection is already a root collection
745763
return E.left(TEAM_COL_ALREADY_ROOT);
746764
}
747-
765+
748766
// Change parent from child to root i.e child collection becomes a root collection
749767
// Move child collection into root and update orderIndexes for root teamCollections
750768
const updatedCollection = await this.changeParentAndUpdateOrderIndex(
751769
tx,
752770
collection.right,
753771
null,
754772
);
755-
if (E.isLeft(updatedCollection)) return E.left(updatedCollection.left);
756-
773+
if (E.isLeft(updatedCollection))
774+
return E.left(updatedCollection.left);
775+
757776
this.pubsub.publish(
758777
`team_coll/${collection.right.teamID}/coll_moved`,
759778
updatedCollection.right,
760779
);
761-
780+
762781
return E.right(updatedCollection.right);
763782
}
764-
783+
765784
// destCollectionID != null i.e move into another collection
766785
if (collectionID === destCollectionID) {
767786
// Throw error if collectionID and destCollectionID are the same
768787
return E.left(TEAM_COLL_DEST_SAME);
769788
}
770-
789+
771790
// Get collection details of destCollectionID
772791
const destCollection = await this.getCollection(destCollectionID, tx);
773792
if (E.isLeft(destCollection)) return E.left(TEAM_COLL_NOT_FOUND);
774-
793+
775794
// Check if collection and destCollection belong to the same user account
776795
if (collection.right.teamID !== destCollection.right.teamID) {
777796
return E.left(TEAM_COLL_NOT_SAME_TEAM);
778797
}
779-
798+
780799
// Check if collection is present on the parent tree for destCollection
781800
const checkIfParent = await this.isParent(
782801
collection.right,
@@ -788,8 +807,12 @@ export class TeamCollectionService {
788807
}
789808

790809
// lock the rows of the destination collection and its siblings
791-
await this.prisma.lockTeamCollectionByTeamAndParent(tx, destCollection.right.teamID, destCollection.right.parentID);
792-
810+
await this.prisma.lockTeamCollectionByTeamAndParent(
811+
tx,
812+
destCollection.right.teamID,
813+
destCollection.right.parentID,
814+
);
815+
793816
// Change parent from null to teamCollection i.e collection becomes a child collection
794817
// Move root/child collection into another child collection and update orderIndexes of the previous parent
795818
const updatedCollection = await this.changeParentAndUpdateOrderIndex(
@@ -807,11 +830,7 @@ export class TeamCollectionService {
807830
return E.right(updatedCollection.right);
808831
});
809832
} catch (error) {
810-
811-
console.error(
812-
'Error from TeamCollectionService.moveCollection',
813-
error,
814-
);
833+
console.error('Error from TeamCollectionService.moveCollection', error);
815834
return E.left(TEAM_COL_REORDERING_FAILED);
816835
}
817836
}
@@ -823,7 +842,11 @@ export class TeamCollectionService {
823842
* @param teamID The Team ID (required when collectionID is null for root collections)
824843
* @returns Number of collections
825844
*/
826-
getCollectionCount(collectionID: string, teamID: string, tx: Prisma.TransactionClient | null = null): Promise<number> {
845+
getCollectionCount(
846+
collectionID: string,
847+
teamID: string,
848+
tx: Prisma.TransactionClient | null = null,
849+
): Promise<number> {
827850
return (tx || this.prisma).teamCollection.count({
828851
where: { parentID: collectionID, teamID: teamID },
829852
});
@@ -867,7 +890,7 @@ export class TeamCollectionService {
867890

868891
// if collection is found, update orderIndexes of siblings
869892
// if collection was deleted before the transaction started (race condition), do not update siblings orderIndexes
870-
if(collectionInTx) {
893+
if (collectionInTx) {
871894
// Step 1: Decrement orderIndex of all items that come after collection.orderIndex till end of list of items
872895
await tx.teamCollection.updateMany({
873896
where: {
@@ -881,7 +904,7 @@ export class TeamCollectionService {
881904
orderIndex: { decrement: 1 },
882905
},
883906
});
884-
907+
885908
// Step 2: Update orderIndex of collection to length of list
886909
await tx.teamCollection.update({
887910
where: { id: collection.right.id },
@@ -894,7 +917,6 @@ export class TeamCollectionService {
894917
},
895918
});
896919
}
897-
898920
} catch (error) {
899921
throw new ConflictException(error);
900922
}
@@ -927,7 +949,11 @@ export class TeamCollectionService {
927949
await this.prisma.$transaction(async (tx) => {
928950
try {
929951
// Step 0: lock the rows
930-
await this.prisma.lockTeamCollectionByTeamAndParent(tx, collection.right.teamID, collection.right.parentID);
952+
await this.prisma.lockTeamCollectionByTeamAndParent(
953+
tx,
954+
collection.right.teamID,
955+
collection.right.parentID,
956+
);
931957

932958
const collectionInTx = await tx.teamCollection.findFirst({
933959
where: { id: collectionID },
@@ -940,10 +966,10 @@ export class TeamCollectionService {
940966

941967
// if collection and subsequentCollection are found, update orderIndexes of siblings
942968
// if collection or subsequentCollection was deleted before the transaction started (race condition), do not update siblings orderIndexes
943-
if(collectionInTx && subsequentCollectionInTx) {
969+
if (collectionInTx && subsequentCollectionInTx) {
944970
// Step 1: Determine if we are moving collection up or down the list
945971
const isMovingUp =
946-
subsequentCollectionInTx.orderIndex < collectionInTx.orderIndex;
972+
subsequentCollectionInTx.orderIndex < collectionInTx.orderIndex;
947973

948974
// Step 2: Update OrderIndex of items in list depending on moving up or down
949975
const updateFrom = isMovingUp
@@ -1490,7 +1516,11 @@ export class TeamCollectionService {
14901516

14911517
try {
14921518
await this.prisma.$transaction(async (tx) => {
1493-
await this.prisma.lockTeamCollectionByTeamAndParent(tx, teamID, parentID);
1519+
await this.prisma.lockTeamCollectionByTeamAndParent(
1520+
tx,
1521+
teamID,
1522+
parentID,
1523+
);
14941524

14951525
const collections = await tx.teamCollection.findMany({
14961526
where: { teamID, parentID },

0 commit comments

Comments
 (0)