Conversation
- Use Path.normalize() + startsWith() for robust file path validation in UploadService - Normalize rootLocation to absolute path in constructor for reliable path checks - Add extension validation in storeBytes to restrict to alphanumeric extensions - Add logging to UploadService for file operation failures - Remove user-controlled input from CalendarController error responses - Sanitize log inputs to prevent CRLF injection - Fix wordsToFilename() replacing space with space (no-op) to use underscores - Add least-privilege permissions blocks to all GitHub Actions workflows - Update test assertions to match improved error messages Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
WalkthroughAdded top-level permissions to multiple GitHub Actions workflows; changed client filename normalization to replace all spaces globally; sanitized calendar-controller logging and simplified 404 response; hardened upload service with path normalization, resolved-path validation, directory creation, and safer file store/load handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Service as UploadService
participant FS as FileSystem
rect rgba(0,123,255,0.5)
Client ->> Service: send bytes + metadata (store request)
Service ->> Service: compute content-hash, normalize uploadRoot, validate size
Service ->> FS: ensure upload directories exist
Service ->> FS: write file to resolved target (Files.copy)
Service -->> Client: return stored filename / identifier
end
rect rgba(40,167,69,0.5)
Client ->> Service: request file load or delete
Service ->> Service: resolve filename, validate path within uploadRoot
Service ->> FS: read file (FileSystemResource) or delete file
Service -->> Client: file stream or acknowledgement
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/src/utils/format.ts (1)
1-6:⚠️ Potential issue | 🟡 MinorUse
import typefor type-only imports.Lines 1-3: The interfaces
ILightUser,IMinimalUser,IThesis, andIApplicationare only used in type annotations and should be imported withimport typeto ensure they don't get bundled at runtime.Example fix
import type { ILightUser, IMinimalUser } from '../requests/responses/user' import type { IThesis } from '../requests/responses/thesis' import { ThesisState } from '../requests/responses/thesis' import type { IApplication } from '../requests/responses/application' import { ApplicationState } from '../requests/responses/application' // ... rest of imports🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/utils/format.ts` around lines 1 - 6, The imports at the top import interfaces that are only used as types (ILightUser, IMinimalUser, IThesis, IApplication); change those to use TypeScript's type-only imports (use "import type { ... }") while keeping value imports like ThesisState, ApplicationState and useMantineColorScheme as normal imports so runtime bundles are unchanged; update the import statements that currently reference ILightUser, IMinimalUser, IThesis, and IApplication to "import type" to avoid emitting them into runtime output.
🧹 Nitpick comments (1)
.github/workflows/prod.yml (1)
11-14: LGTM —packages: writeis correctly declared at the caller level.Per GitHub's model, GITHUB_TOKEN permissions can only be downgraded — not elevated — by called workflows. A workflow chain A → B → C where A has
package: readmeans B and C cannot havepackage: write. Declaringpackages: writehere is required to propagate that scope intobuild_docker.yml, where Docker image pushes to ghcr.io occur.Optional: for slightly tighter scoping, you could override
permissionsat the individual calling-job level (e.g., grant onlycontents: readto therun-testsjob), butrun_tests.ymlalready self-restricts tocontents: readso the practical impact is nil.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/prod.yml around lines 11 - 14, The permissions block currently declares packages: write which is required to propagate GH_TOKEN write scope into the downstream build_docker.yml workflow where ghcr.io pushes occur; leave the top-level permissions: contents: read and packages: write as-is to ensure called workflows (e.g., build_docker.yml) can perform package writes, and if you want tighter scope optionally override permissions at the individual job level (for example the run-tests job or in run_tests.yml) to restrict that job to contents: read only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@client/src/utils/format.ts`:
- Around line 1-6: The imports at the top import interfaces that are only used
as types (ILightUser, IMinimalUser, IThesis, IApplication); change those to use
TypeScript's type-only imports (use "import type { ... }") while keeping value
imports like ThesisState, ApplicationState and useMantineColorScheme as normal
imports so runtime bundles are unchanged; update the import statements that
currently reference ILightUser, IMinimalUser, IThesis, and IApplication to
"import type" to avoid emitting them into runtime output.
---
Nitpick comments:
In @.github/workflows/prod.yml:
- Around line 11-14: The permissions block currently declares packages: write
which is required to propagate GH_TOKEN write scope into the downstream
build_docker.yml workflow where ghcr.io pushes occur; leave the top-level
permissions: contents: read and packages: write as-is to ensure called workflows
(e.g., build_docker.yml) can perform package writes, and if you want tighter
scope optionally override permissions at the individual job level (for example
the run-tests job or in run_tests.yml) to restrict that job to contents: read
only.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/build_docker.yml.github/workflows/deploy_docker.yml.github/workflows/dev.yml.github/workflows/e2e_tests.yml.github/workflows/prod.yml.github/workflows/run_tests.ymlclient/src/utils/format.tsserver/src/main/java/de/tum/cit/aet/thesis/controller/CalendarController.javaserver/src/main/java/de/tum/cit/aet/thesis/service/UploadService.javaserver/src/test/java/de/tum/cit/aet/thesis/service/UploadServiceTest.java
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
These methods belong to PR #864 and were accidentally included here. Removing them to keep this PR focused and avoid merge conflicts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/src/main/java/de/tum/cit/aet/thesis/service/UploadService.java (1)
81-93:⚠️ Potential issue | 🟡 MinorNormalize extension case before allowlist check.
Uppercase extensions (e.g.,
.PNG) will currently fail the allowlist even though they’re valid. Normalizing avoids user-facing false negatives.✅ Suggested fix
import java.util.HexFormat; +import java.util.Locale; import java.util.Set; @@ -String extension = FilenameUtils.getExtension(originalFilename); +String extension = FilenameUtils.getExtension(originalFilename).toLowerCase(Locale.ROOT);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/main/java/de/tum/cit/aet/thesis/service/UploadService.java` around lines 81 - 93, Normalize the extracted extension to a consistent case before checking the allowlist: after getting originalFilename and computing extension, convert extension to lower-case (e.g., extension = extension.toLowerCase(Locale.ROOT)) and then perform the allowedExtensions check; ensure allowedExtensions comparisons are done against the normalized extension (or normalize the allowlist entries once) so uppercase inputs like ".PNG" pass, while leaving the rest of the logic (computeFileHash, filename creation, target path checks, rootLocation check) unchanged.
🧹 Nitpick comments (1)
server/src/main/java/de/tum/cit/aet/thesis/service/UploadService.java (1)
169-189: Sanitize filename before logging to avoid CRLF log injection.If
filenamecan contain CR/LF, it can forge log lines. Consider reusing the CRLF sanitizer used elsewhere or normalize here.🧹 Suggested tweak
- log.warn("Failed to delete file '{}': {}", filename, e.getMessage()); + String safeFilename = filename.replaceAll("[\\r\\n]", "_"); + log.warn("Failed to delete file '{}': {}", safeFilename, e.getMessage());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/main/java/de/tum/cit/aet/thesis/service/UploadService.java` around lines 169 - 189, The log call in deleteFile(String filename) directly logs the untrusted filename which can contain CR/LF and enable log injection; before calling Files.deleteIfExists or log.warn, sanitize the filename (e.g. reuse the project's existing CRLF sanitizer or replace '\r' and '\n' with escaped equivalents and optionally truncate) and use that sanitized value in the log.warn call; keep using deleteFile, filename, rootLocation and Files.deleteIfExists as-is but pass the sanitizedFilename into the logger.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/src/main/java/de/tum/cit/aet/thesis/service/UploadService.java`:
- Around line 129-167: The storeBytes method exposes an un-allowlisted extension
parameter; either reduce its visibility or enforce an allowlist: change
storeBytes to be private (so only internal callers use it) OR add an
UploadFileType parameter (e.g. storeBytes(byte[] bytes, UploadFileType type, int
maxSize) or storeBytes(byte[] bytes, String extension, Set<String>
allowedExtensions, int maxSize)) and validate the provided extension against
UploadFileType's allowed extensions before computing the hash and writing the
file (mirror the validation pattern used in the existing store() method),
throwing UploadException on mismatch.
---
Outside diff comments:
In `@server/src/main/java/de/tum/cit/aet/thesis/service/UploadService.java`:
- Around line 81-93: Normalize the extracted extension to a consistent case
before checking the allowlist: after getting originalFilename and computing
extension, convert extension to lower-case (e.g., extension =
extension.toLowerCase(Locale.ROOT)) and then perform the allowedExtensions
check; ensure allowedExtensions comparisons are done against the normalized
extension (or normalize the allowlist entries once) so uppercase inputs like
".PNG" pass, while leaving the rest of the logic (computeFileHash, filename
creation, target path checks, rootLocation check) unchanged.
---
Nitpick comments:
In `@server/src/main/java/de/tum/cit/aet/thesis/service/UploadService.java`:
- Around line 169-189: The log call in deleteFile(String filename) directly logs
the untrusted filename which can contain CR/LF and enable log injection; before
calling Files.deleteIfExists or log.warn, sanitize the filename (e.g. reuse the
project's existing CRLF sanitizer or replace '\r' and '\n' with escaped
equivalents and optionally truncate) and use that sanitized value in the
log.warn call; keep using deleteFile, filename, rootLocation and
Files.deleteIfExists as-is but pass the sanitizedFilename into the logger.
server/src/main/java/de/tum/cit/aet/thesis/service/UploadService.java
Outdated
Show resolved
Hide resolved
krusche
deleted the
feature/improve-input-validation-and-workflow-permissions
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
contains("..")) with robustPath.normalize()+startsWith()validation instore(),load(),storeBytes(), anddeleteFile(). NormalizerootLocationto an absolute path in the constructor so relative config values work correctly. Add alphanumeric extension validation instoreBytes(). Add@Slf4jlogging for file operation failures.text/plaincontent type. Add null-safe CRLF sanitization for log messages.wordsToFilename()which was a no-op (replacing space with space) — now correctly replaces spaces with underscores using a global regex.permissionsblocks to all 6 workflow files (contents: readfor test/deploy,contents: read+packages: writefor build/push workflows).Test plan
Summary by CodeRabbit
Bug Fixes
Improvements
Chores