fix(revisions): always check noteId when fetching a revision

This fixes a reported security vulnerability where one use could
retrieve revisions of another note where they don't have access
to. This was possible, because the URL included both the note alias
and the revision UUID, the backend then checked the user's permissions
for the note alias but fetched and returned the revision by its UUID
without checking whether the revision belongs to that note.

Credits for finding and reporting this vulnerability to:
- The Raw (https://github.com/therawdev)
- Vishal (https://github.com/shukla304)

Signed-off-by: Erik Michelson <github@erik.michelson.eu>

backend/src/api/private/notes/notes.controller.ts

@@ -176,8 +176,11 @@ export class NotesController {
   @OpenApi(200, 404)
   @RequirePermission(PermissionLevel.READ)
   @UseInterceptors(GetNoteIdInterceptor)
-  async getNoteRevision(@Param('revisionUuid') revisionUuid: string): Promise<RevisionDto> {
-    return await this.revisionsService.getRevisionDto(revisionUuid);
+  async getNoteRevision(
+    @Param('revisionUuid') revisionUuid: string,
+    @RequestNoteId() noteId: number,
+  ): Promise<RevisionDto> {
+    return await this.revisionsService.getRevisionDto(revisionUuid, noteId);
   }
 
   @Put(':noteAlias/metadata/permissions/users/:username')

backend/src/api/public/notes/notes.controller.ts

@@ -362,8 +362,11 @@ export class NotesController {
     },
     404,
   )
-  async getNoteRevision(@Param('revisionUuid') revisionUuid: string): Promise<RevisionDto> {
-    return await this.revisionsService.getRevisionDto(revisionUuid);
+  async getNoteRevision(
+    @Param('revisionUuid') revisionUuid: string,
+    @RequestNoteId() noteId: number,
+  ): Promise<RevisionDto> {
+    return await this.revisionsService.getRevisionDto(revisionUuid, noteId);
   }
 
   @UseInterceptors(GetNoteIdInterceptor)

backend/src/revisions/revisions.service.spec.ts

@@ -219,11 +219,13 @@ describe('RevisionsService', () => {
           FieldNameRevision.patch,
         ],
         TableRevision,
-        FieldNameRevision.uuid,
+        [FieldNameRevision.uuid, FieldNameRevision.noteId],
         [],
       );
-      await expect(service.getRevisionDto(mockRevisionUuid1)).rejects.toThrow(NotInDBError);
-      expectBindings(tracker, 'select', [[mockRevisionUuid1]], true);
+      await expect(service.getRevisionDto(mockRevisionUuid1, mockNoteId)).rejects.toThrow(
+        NotInDBError,
+      );
+      expectBindings(tracker, 'select', [[mockRevisionUuid1, mockNoteId]], true);
     });
 
     it('correctly returns the fetched revision', async () => {
@@ -238,7 +240,7 @@ describe('RevisionsService', () => {
           FieldNameRevision.patch,
         ],
         TableRevision,
-        FieldNameRevision.uuid,
+        [FieldNameRevision.uuid, FieldNameRevision.noteId],
         [
           {
             [FieldNameRevision.uuid]: mockRevisionUuid1,
@@ -253,7 +255,7 @@ describe('RevisionsService', () => {
           },
         ],
       );
-      const result = await service.getRevisionDto(mockRevisionUuid1);
+      const result = await service.getRevisionDto(mockRevisionUuid1, mockNoteId);
       expect(result).toStrictEqual({
         uuid: mockRevisionUuid1,
         content: mockContent1,
@@ -263,7 +265,7 @@ describe('RevisionsService', () => {
         description: mockDescription,
         patch: mockPatch,
       });
-      expectBindings(tracker, 'select', [[mockRevisionUuid1]], true);
+      expectBindings(tracker, 'select', [[mockRevisionUuid1, mockNoteId]], true);
     });
   });
 

backend/src/revisions/revisions.service.ts

@@ -203,10 +203,11 @@ export class RevisionsService {
    * Get a revision by its UUID
    *
    * @param revisionUuid The UUID of the revision to get
+   * @param noteId The id of the note this revision belongs to
    * @returns The revision DTO
    * @throws NotInDBError if the revision with the given UUID does not exist
    */
-  async getRevisionDto(revisionUuid: string): Promise<RevisionDto> {
+  async getRevisionDto(revisionUuid: string, noteId: number): Promise<RevisionDto> {
     const revision = await this.knex(TableRevision)
       .select(
         FieldNameRevision.uuid,
@@ -217,6 +218,7 @@ export class RevisionsService {
         FieldNameRevision.patch,
       )
       .where(FieldNameRevision.uuid, revisionUuid)
+      .andWhere(FieldNameRevision.noteId, noteId)
       .first();
     if (revision === undefined) {
       throw new NotInDBError(