`Development`: Improve background image input validation #7170

MarkusPaulsen · 2023-09-10T20:54:38Z

Checklist

General

I tested all changes and their related features with all corresponding user types on a test server.
Language: I followed the guidelines for inclusive, diversity-sensitive, and appreciative language.
I chose a title conforming to the naming conventions for pull requests.

Server

Important: I implemented the changes with a very good performance and prevented too many (unnecessary) database calls.
I followed the coding and design guidelines.
I added multiple integration tests (Spring) related to the features (with a high test coverage).
I documented the Java code using JavaDoc style.

Changes affecting Programming Exercises

I tested all changes and their related features with all corresponding user types on Test Server 1 (Atlassian Suite).
I tested all changes and their related features with all corresponding user types on Test Server 2 (Jenkins and Gitlab).

Motivation and Context

Several endpoints in the classes ExamResource, ExerciseGroupResource and QuizExerciseResource do not sanitise the background file path of imported quiz exercises, which can lead to path traversal when being used without sanatisation.

Description

We now:

Sanatize every background file path
Check if every background file path starts with /api/files/drag-and-drop/backgrounds

Steps for Testing

Prerequisites:

1 Instructor

Log in to Artemis
Navigate to Course Administration
Either create a new course or access an existing one
Navigate to the exercise overview
Create a quiz
Add Drag-And-Drop Question exercise with a background and a picture drag item to the quiz
Save the quiz
Navigate to the exercise overview
Click on import quiz
Import the created quiz as a new quiz
Click on Preview Quiz
Check, whether the imported exercise displays the background image and the picture drag item correctly

Exam Mode Testing

Prerequisites:

1 Instructor

Log in to Artemis
Navigate to the exam overview
Create a new exam
Navigate to the exercise group overview
Create a new exercise group
Import the previously created quiz
Navigate to the exam overview
Import the created exam as a new exam
Click on Preview Quiz
Check, if the exercise in the exported exam correctly displays the background image
Navigate to the exercise group overview
Import the exercise group from the firstly created exam.
Click on Preview Quiz
Check, if the exercise in the newly created exam correctly displays the background image

Review Progress

Performance Review

I (as a reviewer) confirm that the server changes (in particular related to database calls) are implemented with a very good performance

Code Review

Code Review 1
Code Review 2

Manual Tests

Test 1
Test 2

Exam Mode Test

Test 1
Test 2

Test Coverage

laadvo

Due to the changes, some import quiz exercise tests from QuizExerciseIntegrationTest seem to fail with Status response 500 now.
I am guessing this is because they use images with invalid paths. Additionally, could you move the two new tests into QuizExerciseIntegrationTest since quiz import is already tested there? Alternatively, you could move the existing quiz import tests into the new class, but then make sure to keep it package-private (see Codacy and failing architecture test).

…ontains("files/temp").

…ePathElseThrow method from QuizExerciseImportTest.java to QuizExerciseIntegrationTest.java.

MarkusPaulsen · 2023-09-11T10:00:05Z

Due to the changes, some import quiz exercise tests from QuizExerciseIntegrationTest seem to fail with Status response 500 now. I am guessing this is because they use images with invalid paths. Additionally, could you move the two new tests into QuizExerciseIntegrationTest since quiz import is already tested there? Alternatively, you could move the existing quiz import tests into the new class, but then make sure to keep it package-private (see Codacy and failing architecture test).

Fixed in:

jakubriegel

Works on ts1

MaximilianAnzinger

Testing Session TS1

pal03377

Tested in testing session on TS1. We tested

normal quiz creation with background image
exam quiz creation with background image
importing those quiz exercises (exam and non-exam)
importing whole exams

The images were always intact and available, also in the student view.

nityanandaz

Manual test on TS1

Drag and drop worked in all places

src/main/java/de/tum/in/www1/artemis/service/QuizExerciseImportService.java

src/test/java/de/tum/in/www1/artemis/exercise/quizexercise/QuizExerciseIntegrationTest.java

Kroko-fant

tested in a testing session everything worked so far.

pal03377

Code: As @julian-christl mentioned, the paths to check are actually URL paths relative to the domain, so we should not use File utils to check them. Also, I found one other place in the code where it might sense to introduce a similar check.

src/main/java/de/tum/in/www1/artemis/service/QuizExerciseImportService.java

…tService.java Co-authored-by: Julian Christl <[email protected]>

2. Rename sanitizeByCheckingIfPathContainsSubPathElseThrow to sanitizeByCheckingIfPathStartsWithSubPathElseThrow.

src/main/java/de/tum/in/www1/artemis/service/FileService.java

src/main/java/de/tum/in/www1/artemis/service/QuizExerciseImportService.java

Co-authored-by: Lucas Welscher <[email protected]>

Co-authored-by: Julian Christl <[email protected]>

julian-christl

Now that looks good. Thank you.

jakubriegel

Code LGTM 👍🏻 Change works on TS4

pal03377

The code looks really solid now! I also manually tested the upload on TS4 again.

laadvo

Code looks good to me

Improved input validation.

c73e279

MarkusPaulsen requested a review from a team as a code owner September 10, 2023 20:54

github-actions bot assigned MarkusPaulsen Sep 10, 2023

github-actions bot added tests server Pull requests that update Java code. (Added Automatically!) labels Sep 10, 2023

MarkusPaulsen changed the title ~~Improved input validation.~~ Development: Improved input validation. Sep 11, 2023

MarkusPaulsen changed the title ~~Development: Improved input validation.~~ Development: Improve background image input validation. Sep 11, 2023

laadvo reviewed Sep 11, 2023

View reviewed changes

Markus Paulsen added 2 commits September 11, 2023 10:07

Ignore cases where backgroundFilePath == null || backgroundFilePath.c…

f60b9e3

…ontains("files/temp").

Changed the location of the checkIfBackgroundFilePathIsABackgroundFil…

303e417

…ePathElseThrow method from QuizExerciseImportTest.java to QuizExerciseIntegrationTest.java.

MarkusPaulsen requested a review from julian-christl September 11, 2023 11:50

jakubriegel previously approved these changes Sep 11, 2023

View reviewed changes

MaximilianAnzinger previously approved these changes Sep 11, 2023

View reviewed changes

pal03377 approved these changes Sep 11, 2023

View reviewed changes

nityanandaz previously approved these changes Sep 11, 2023

View reviewed changes

julian-christl reviewed Sep 11, 2023

View reviewed changes

Kroko-fant previously approved these changes Sep 11, 2023

View reviewed changes

pal03377 added the deploy:artemis-test5 label Sep 11, 2023

pal03377 temporarily deployed to artemis-test5.artemis.cit.tum.de September 11, 2023 16:19 — with GitHub Actions Inactive

github-actions bot added lock:artemis-test5 and removed deploy:artemis-test5 labels Sep 11, 2023

pal03377 suggested changes Sep 11, 2023

View reviewed changes

src/main/java/de/tum/in/www1/artemis/service/QuizExerciseImportService.java Outdated Show resolved Hide resolved

pal03377 removed the lock:artemis-test5 label Sep 11, 2023

MarkusPaulsen dismissed stale reviews from Kroko-fant, nityanandaz, and MaximilianAnzinger via 963d701 September 13, 2023 08:38

Update src/main/java/de/tum/in/www1/artemis/service/QuizExerciseImpor…

963d701

…tService.java Co-authored-by: Julian Christl <[email protected]>

1. Deleted Legacy Paths.

1dd1398

2. Rename sanitizeByCheckingIfPathContainsSubPathElseThrow to sanitizeByCheckingIfPathStartsWithSubPathElseThrow.

Strohgelaender reviewed Sep 24, 2023

View reviewed changes

src/main/java/de/tum/in/www1/artemis/service/FileService.java Outdated Show resolved Hide resolved

julian-christl reviewed Sep 24, 2023

View reviewed changes

src/main/java/de/tum/in/www1/artemis/service/FileService.java Outdated Show resolved Hide resolved

src/main/java/de/tum/in/www1/artemis/service/QuizExerciseImportService.java Outdated Show resolved Hide resolved

MarkusPaulsen and others added 3 commits September 24, 2023 22:55

Update src/main/java/de/tum/in/www1/artemis/service/FileService.java

ed40eb5

Co-authored-by: Lucas Welscher <[email protected]>

Update src/main/java/de/tum/in/www1/artemis/service/FileService.java

2239f55

Co-authored-by: Julian Christl <[email protected]>

Fixed intended paths.

be06ceb

julian-christl approved these changes Sep 24, 2023

View reviewed changes

jakubriegel added the deploy:artemis-test4 label Sep 25, 2023

jakubriegel temporarily deployed to artemis-test4.artemis.cit.tum.de September 25, 2023 13:42 — with GitHub Actions Inactive

github-actions bot added lock:artemis-test4 and removed deploy:artemis-test4 labels Sep 25, 2023

jakubriegel approved these changes Sep 25, 2023

View reviewed changes

jakubriegel removed the lock:artemis-test4 label Sep 25, 2023

pal03377 added the deploy:artemis-test4 label Sep 27, 2023

pal03377 temporarily deployed to artemis-test4.artemis.cit.tum.de September 27, 2023 11:23 — with GitHub Actions Inactive

github-actions bot added lock:artemis-test4 and removed deploy:artemis-test4 labels Sep 27, 2023

pal03377 removed the lock:artemis-test4 label Sep 27, 2023

pal03377 approved these changes Sep 27, 2023

View reviewed changes

laadvo approved these changes Sep 28, 2023

View reviewed changes

MarkusPaulsen added 2 commits September 28, 2023 23:33

Merge branch 'develop' into bugfix/fix-background-image-input-validation

2ae4200

Merge branch 'develop' into bugfix/fix-background-image-input-validation

d61ebe4

MarkusPaulsen added ready to merge maintainer-approved The feature maintainer has approved the PR labels Oct 1, 2023

Merge branch 'develop' into bugfix/fix-background-image-input-validation

0416075

julian-christl removed the ready for review label Oct 1, 2023

krusche changed the title ~~Development: Improve background image input validation.~~ Development: Improve background image input validation Oct 3, 2023

krusche added this to the 6.5.4 milestone Oct 3, 2023

krusche merged commit fa029c1 into develop Oct 3, 2023

krusche deleted the bugfix/fix-background-image-input-validation branch October 3, 2023 07:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`Development`: Improve background image input validation #7170

`Development`: Improve background image input validation #7170

MarkusPaulsen commented Sep 10, 2023 •

edited

Loading

laadvo left a comment •

edited

Loading

MarkusPaulsen commented Sep 11, 2023

jakubriegel left a comment

MaximilianAnzinger left a comment

pal03377 left a comment

nityanandaz left a comment

Kroko-fant left a comment

pal03377 left a comment

julian-christl left a comment

jakubriegel left a comment

pal03377 left a comment

laadvo left a comment

Development: Improve background image input validation #7170

Development: Improve background image input validation #7170

Conversation

MarkusPaulsen commented Sep 10, 2023 • edited Loading

Checklist

General

Server

Changes affecting Programming Exercises

Motivation and Context

Description

Steps for Testing

Exam Mode Testing

Review Progress

Performance Review

Code Review

Manual Tests

Exam Mode Test

Test Coverage

laadvo left a comment • edited Loading

Choose a reason for hiding this comment

MarkusPaulsen commented Sep 11, 2023

jakubriegel left a comment

Choose a reason for hiding this comment

MaximilianAnzinger left a comment

Choose a reason for hiding this comment

pal03377 left a comment

Choose a reason for hiding this comment

nityanandaz left a comment

Choose a reason for hiding this comment

Kroko-fant left a comment

Choose a reason for hiding this comment

pal03377 left a comment

Choose a reason for hiding this comment

julian-christl left a comment

Choose a reason for hiding this comment

jakubriegel left a comment

Choose a reason for hiding this comment

pal03377 left a comment

Choose a reason for hiding this comment

laadvo left a comment

Choose a reason for hiding this comment

`Development`: Improve background image input validation #7170

`Development`: Improve background image input validation #7170

MarkusPaulsen commented Sep 10, 2023 •

edited

Loading

laadvo left a comment •

edited

Loading