Programming exercises: Fix image export in combination with tooltip text #9879
Conversation
github-actions
bot
added
server
WalkthroughThe changes introduce a new regex pattern, Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 pmd (7.7.0)src/main/java/de/tum/cit/aet/artemis/exercise/service/ExerciseWithSubmissionsExportService.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/exercise/service/ExerciseWithSubmissionsExportService.java (1)
134-142: Consider improving robustness of path extractionWhile the basic functionality is correct, consider these improvements:
- Cache the Pattern compilation
- Add validation for the extracted path
- Handle escaped quotes in tooltip text
private static final Pattern HOVER_TEXT_PATTERN = Pattern.compile(EMBEDDED_FILE_MARKDOWN_WITH_HOVERTEXT); private void copyFilesEmbeddedWithMarkdownSyntax(...) { for (String embeddedFile : embeddedFilesWithMarkdownSyntax) { String lastPartOfMatchedString = embeddedFile.substring(embeddedFile.lastIndexOf("]") + 1); String filePath; - if (Pattern.compile(EMBEDDED_FILE_MARKDOWN_WITH_HOVERTEXT).matcher(lastPartOfMatchedString).matches()) { + if (HOVER_TEXT_PATTERN.matcher(lastPartOfMatchedString).matches()) { filePath = lastPartOfMatchedString.substring(lastPartOfMatchedString.indexOf("(") + 1, lastPartOfMatchedString.indexOf(" ")); } else { filePath = lastPartOfMatchedString.substring(lastPartOfMatchedString.indexOf("(") + 1, lastPartOfMatchedString.indexOf(")")); } + if (!isValidMarkdownPath(filePath)) { + log.warn("Invalid markdown file path: {}", filePath); + continue; + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/main/java/de/tum/cit/aet/artemis/exercise/service/ExerciseWithSubmissionsExportService.java(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/de/tum/cit/aet/artemis/exercise/service/ExerciseWithSubmissionsExportService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
🔇 Additional comments (2)
src/main/java/de/tum/cit/aet/artemis/exercise/service/ExerciseWithSubmissionsExportService.java (2)
46-47: LGTM: Well-defined regex constant for hover text extraction
The new regex pattern correctly matches markdown image syntax with tooltip text, following the format .
134-142: Verify path extraction security
The current implementation might be vulnerable to path traversal if the input is not properly sanitized. Please verify that:
- The path is properly sanitized before being used
- The path cannot escape the intended directory
- Input validation is performed on the markdown content
✅ Verification successful
Let me gather more information about how the file paths are handled in the codebase, particularly focusing on the class that contains the code under review.
Let me check how the extracted file paths are used and if there's any validation before their usage.
Let me check one final aspect about how the paths are handled after extraction, particularly in the constructFilenameAndCopyFile method and the FilePathService usage.
Let me check one final aspect about the path validation and conversion between public and actual paths in FilePathService.
Path extraction is properly secured through multiple layers of validation
The implementation is secure against path traversal for the following reasons:
- The extracted file path is only used to get the filename component (
filePath.replace(API_MARKDOWN_FILE_PATH, "")) - The actual file path is constructed using
FilePathService.getMarkdownFilePath().resolve(fileName), which ensures files can only be accessed within the markdown directory - The
FilePathServiceincludes path validation:- Normalizes paths to remove
../and./ - Validates paths against known allowed directories
- Throws exceptions for unknown or invalid paths
- Normalizes paths to remove
- The
constructFilenameAndCopyFilemethod uses safe file operations within the designated directories
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for path traversal prevention in the codebase
# Look for path sanitization
rg -A 5 "sanitize.*(?i)path|(?i)path.*sanitize"
# Look for path traversal prevention
rg -A 5 "normalize.*(?i)path|(?i)path.*normalize"
# Look for FilePathService usage
ast-grep --pattern 'FilePathService.$_'
Length of output: 31431
Script:
#!/bin/bash
# Check the implementation of ExerciseWithSubmissionsExportService and related path handling
ast-grep --pattern 'class ExerciseWithSubmissionsExportService {
$$$
}'
# Look for the pattern constant definition
rg "EMBEDDED_FILE_MARKDOWN_WITH_HOVERTEXT" -A 2
# Check for any path validation or sanitization in the same file
rg -g "ExerciseWithSubmissionsExportService.java" "sanitize|validate|normalize|check.*path"
Length of output: 2603
Script:
#!/bin/bash
# Check how the extracted filePath is used in ExerciseWithSubmissionsExportService
rg -g "ExerciseWithSubmissionsExportService.java" -A 5 -B 5 "filePath ="
# Look for FileService usage and path validation in the service
ast-grep --pattern 'class FileService {
$$$
sanitizeByCheckingIfPathStartsWithSubPathElseThrow($_, $_) {
$$$
}
$$$
}'
# Check if there's any validation after path extraction
rg -g "ExerciseWithSubmissionsExportService.java" "FileService\.sanitize|FilePathService\."
Length of output: 4868
Script:
#!/bin/bash
# Check the implementation of constructFilenameAndCopyFile and related path handling
ast-grep --pattern 'constructFilenameAndCopyFile($$$) {
$$$
}'
# Look for FilePathService implementation and its path validation
ast-grep --pattern 'class FilePathService {
$$$
}'
# Check for any path validation in the markdown file handling
rg -g "FilePathService.java" "getMarkdownFilePath|markdown.*path" -A 5
Length of output: 1531
Script:
#!/bin/bash
# Check for path validation in FilePathService
rg -g "FilePathService.java" "actualPathForPublicPath|publicPathForActualPath" -A 10
# Look for the actual implementation of constructFilenameAndCopyFile in ExerciseWithSubmissionsExportService
rg -g "ExerciseWithSubmissionsExportService.java" -A 15 "private void constructFilenameAndCopyFile"
# Check for any security-related path validation in FileService
rg -g "FileService.java" "sanitize.*path|validate.*path|check.*path" -A 5
Length of output: 9365
HawKhiem
added
deploy:artemis-test4
and removed
deploy:artemis-test5
deploy:artemis-test4
labels
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
|
Please unstale, I will recreate the PR in the near future |
HawKhiem
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
Cathy0123456789
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
sawys777
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
Checklist
General
Server
Changes affecting Programming Exercises
Motivation and Context
Fixes #9823
Description
With this PR, the Artemis Server now correctly extracts uploaded images from the markdown of exercises to include them in an exercise export, even when a tooltip text is specified. To achieve this, the code differentiates between two types of image URLs: those with tooltip text and those without. Using RegEx, it adjusts the corresponding substring operations based on the URL type.
Steps for Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Performance Review
Code Review
Manual Tests
Exam Mode Test
Performance Tests
Summary by CodeRabbit
New Features
Bug Fixes