Feature/s3 integration - #15
Merged
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 13 minutes and 25 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds AWS S3 integration with LocalStack: Docker Compose adds LocalStack; Maven adds AWS SDK BOM and S3 dependency; Spring config supplies an S3Client; new S3Service and S3Controller provide upload/download/delete endpoints; security permits Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,230,255,0.5)
participant Client
end
rect rgba(220,255,200,0.5)
participant S3Controller
participant S3Service
end
rect rgba(255,230,200,0.5)
participant S3Client as AWS S3Client
participant LocalStack
end
Client->>S3Controller: POST /api/files/upload (MultipartFile)
S3Controller->>S3Service: uploadFile(key, bytes, contentType)
S3Service->>S3Client: putObject(PutObjectRequest, RequestBody)
S3Client->>LocalStack: HTTP PUT to S3 endpoint
LocalStack-->>S3Client: PutObjectResponse
S3Client-->>S3Service: return
S3Service-->>S3Controller: return
S3Controller-->>Client: 200 OK (success message)
sequenceDiagram
rect rgba(200,230,255,0.5)
participant Client
end
rect rgba(220,255,200,0.5)
participant S3Controller
participant S3Service
end
rect rgba(255,230,200,0.5)
participant S3Client as AWS S3Client
participant LocalStack
end
Client->>S3Controller: GET /api/files/download/{key}
S3Controller->>S3Service: downloadFile(key)
S3Service->>S3Client: getObject(GetObjectRequest)
S3Client->>LocalStack: HTTP GET to S3 endpoint
LocalStack-->>S3Client: ResponseInputStream (object data)
S3Client-->>S3Service: return InputStream
S3Service-->>S3Controller: return InputStream
S3Controller->>S3Controller: readAllBytes() -> byte[]
S3Controller-->>Client: 200 OK (file bytes + Content-Disposition)
sequenceDiagram
rect rgba(200,230,255,0.5)
participant Client
end
rect rgba(220,255,200,0.5)
participant S3Controller
participant S3Service
end
rect rgba(255,230,200,0.5)
participant S3Client as AWS S3Client
participant LocalStack
end
Client->>S3Controller: DELETE /api/files/delete/{key}
S3Controller->>S3Service: deleteFile(key)
S3Service->>S3Client: deleteObject(DeleteObjectRequest)
S3Client->>LocalStack: HTTP DELETE to S3 endpoint
LocalStack-->>S3Client: DeleteObjectResponse
S3Client-->>S3Service: return
S3Service-->>S3Controller: return
S3Controller-->>Client: 200 OK (deletion success)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
src/main/java/backendlab/team4you/config/S3Config.java (1)
28-37: Consider closing S3Client on application shutdown.
S3ClientimplementsSdkAutoCloseableand holds HTTP connections. While the connection pool may eventually be garbage collected, explicitly closing the client on shutdown is best practice to release resources promptly.♻️ Proposed fix to ensure S3Client cleanup
`@Bean` + `@PreDestroy` public S3Client s3Client() { AwsBasicCredentials credentials = AwsBasicCredentials.create(accessKey, secretKey); return S3Client.builder() .region(Region.of(region)) .credentialsProvider(StaticCredentialsProvider.create(credentials)) .endpointOverride(URI.create(endpointUrl)) // Points to LocalStack instead of real AWS .forcePathStyle(true) // Required for LocalStack .build(); } + + `@Bean`(destroyMethod = "close") + public S3Client s3Client() { ... }Alternatively, annotate the bean with
destroyMethod:`@Bean`(destroyMethod = "close") public S3Client s3Client() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/config/S3Config.java` around lines 28 - 37, The S3Client bean (s3Client method) isn't being closed on JVM shutdown; update the bean definition to ensure the S3Client is closed by the Spring container (e.g., declare the `@Bean` with destroyMethod="close" or implement a lifecycle hook that calls close on the S3Client) so the SdkAutoCloseable connections are released when the application stops. Ensure the change targets the s3Client() bean and uses the S3Client.close() method as the destroy action.src/main/java/backendlab/team4you/s3/S3Service.java (2)
27-36: Consider validating the key parameter.If
keyis null or empty, the S3 SDK will throw an exception. Adding validation provides clearer error messages.♻️ Proposed validation
public void uploadFile(String key, byte[] data, String contentType) { + if (key == null || key.isBlank()) { + throw new IllegalArgumentException("File key cannot be null or empty"); + } s3Client.putObject(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/s3/S3Service.java` around lines 27 - 36, The uploadFile method in S3Service should validate the key parameter before calling s3Client.putObject: check that key is not null and not blank (e.g., after trimming) and if invalid throw an IllegalArgumentException with a clear message like "S3 key must not be null or empty"; update the uploadFile(String key, byte[] data, String contentType) method to perform this validation and only call s3Client.putObject (and use bucketName) when the key passes validation.
38-46: Document or address InputStream lifecycle.The returned
InputStreamis aResponseInputStreamfrom AWS SDK that holds an open HTTP connection. Callers must close it to avoid resource leaks. Consider either:
- Documenting this contract in Javadoc
- Returning
byte[]directly to simplify resource management for callers♻️ Option: Return byte[] to simplify resource management
- // Download a file from S3 - public InputStream downloadFile(String key) { - return s3Client.getObject( + // Download a file from S3 + public byte[] downloadFile(String key) throws IOException { + try (InputStream stream = s3Client.getObject( GetObjectRequest.builder() .bucket(bucketName) .key(key) .build() - ); + )) { + return stream.readAllBytes(); + } }This moves the responsibility of closing the stream to the service layer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/s3/S3Service.java` around lines 38 - 46, The downloadFile method currently returns the raw ResponseInputStream (open HTTP connection) which callers must close; change downloadFile(String key) to return a byte[] instead and inside the method call s3Client.getObject(GetObjectRequest.builder().bucket(bucketName).key(key).build()) within a try-with-resources block (capturing the ResponseInputStream<GetObjectResponse>), read all bytes (e.g., response.readAllBytes()) and return the byte[] so the service closes the stream itself; alternatively if you keep the InputStream signature, add Javadoc to downloadFile(String key) noting it returns a ResponseInputStream that the caller MUST close and update callers accordingly.src/test/java/backendlab/team4you/s3/S3ServiceTest.java (1)
50-62: Unchecked cast warning from mocking generic type.The pipeline warning about "unchecked or unsafe operations" comes from
mock(ResponseInputStream.class)without type parameters. This is a known Mockito limitation with generics.♻️ Suppress the warning if needed
`@Test` + `@SuppressWarnings`("unchecked") void downloadFile_shouldCallGetObject() { // Arrange ResponseInputStream<GetObjectResponse> mockStream = mock(ResponseInputStream.class);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/backendlab/team4you/s3/S3ServiceTest.java` around lines 50 - 62, The unchecked generic-mock warning in S3ServiceTest.downloadFile_shouldCallGetObject comes from mocking ResponseInputStream without type parameters; fix it by performing an explicit cast to ResponseInputStream<GetObjectResponse> and suppressing the unchecked warning for that statement or the test method (e.g., add `@SuppressWarnings`("unchecked") and write ResponseInputStream<GetObjectResponse> mockStream = (ResponseInputStream<GetObjectResponse>) mock(ResponseInputStream.class)); keep the verify(s3Client, times(1)).getObject(...) assertion unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker-compose.yaml`:
- Around line 14-26: LocalStack currently starts without creating the S3 bucket
so S3 operations fail; add an init hook to create the bucket on startup by
adding an init script (e.g., init-localstack.sh) that runs awslocal s3 mb
s3://${AWS_BUCKET_NAME:-my-bucket} and wire it into the localstack service
configuration (or enable EAGER_SERVICE_LOADING and an init hook) so the bucket
named by AWS_BUCKET_NAME is created before the app runs; update the
docker-compose localstack service (service name "localstack") to execute this
initialization.
In `@src/main/java/backendlab/team4you/config/SecurityConfig.java`:
- Line 28: Current code in SecurityConfig uses .csrf(csrf -> csrf.disable()),
which disables CSRF globally; change this to disable CSRF only for API endpoints
by replacing the global disable with a scoped configuration that ignores or
disables CSRF for requests matching your REST API paths (e.g., "/api/**")
instead; locate the .csrf(csrf -> csrf.disable()) call in SecurityConfig and
update it to use csrf(...).ignoringRequestMatchers or an equivalent
request-matcher-based API so non-API (form-based) endpoints keep CSRF
protection.
- Around line 35-36: SecurityConfig currently permits all access to
"/api/files/**", which is unsafe; update the HTTP security rules to require
authentication for file-mutating endpoints (e.g., change the matcher for
POST/PUT/DELETE to .authenticated() and optionally allow GET for public
downloads) by using method-specific requestMatchers (HttpMethod.POST/PUT/DELETE
with "/api/files/**") instead of a blanket .permitAll(), and/or add method-level
security annotations in S3Controller on the mutating handlers (annotate
uploadFile, deleteFile, updateFile methods with
`@PreAuthorize`("isAuthenticated()" or a role check) and enable
`@EnableMethodSecurity` if not already configured. Ensure the SecurityConfig
ordering places these matchers before any broader permits so the authenticated
rules are enforced.
In `@src/main/java/backendlab/team4you/s3/S3Controller.java`:
- Around line 34-41: The InputStream from s3Service.downloadFile(key) in the
controller method downloadFile(`@PathVariable` String key) is not closed; wrap the
call to s3Service.downloadFile(key) in a try-with-resources so the
ResponseInputStream is closed after calling readAllBytes(), then build and
return the ResponseEntity<byte[]> from the bytes read; keep the method signature
(throws IOException) and ensure no open stream is left holding the S3 HTTP
connection.
- Line 38: The Content-Disposition header currently inserts the raw key into
HttpHeaders.CONTENT_DISPOSITION which can allow header injection; update the
S3Controller (where the response header is built using
HttpHeaders.CONTENT_DISPOSITION and the variable key) to build a safe header
value via org.springframework.http.ContentDisposition (e.g.,
ContentDisposition.builder("attachment").filename(...).build().toString()) and
ensure the filename is encoded/normalized using
java.nio.charset.StandardCharsets (or set both filename and filename* with
UTF-8) so quotes/newlines are handled and the header value is sanitized.
- Around line 26-30: uploadFile currently uses
MultipartFile.getOriginalFilename() which may return null; validate the filename
in the uploadFile method and reject or generate a safe key: if
getOriginalFilename() is null or blank, return
ResponseEntity.badRequest().body("Missing filename") (or alternatively generate
a safe key such as UUID.randomUUID().toString()); then call
s3Service.uploadFile(...) with the validated/sanitized key and the file
bytes/content type. Ensure you reference uploadFile and s3Service.uploadFile
when making the change and avoid passing a null key to the S3 SDK.
In `@src/test/java/backendlab/team4you/s3/S3ServiceTest.java`:
- Around line 19-23: The tests don't inject S3Service.bucketName (annotated with
`@Value`) so it's null; in S3ServiceTest set the private field on the injected
S3Service before running tests (e.g., in a `@BeforeEach` setup) using
ReflectionTestUtils.setField(s3Service, "bucketName", "expected-bucket") so
PutObjectRequest/GetObjectRequest/DeleteObjectRequest use a real bucket; after
setting the field, replace broad any() bucket matchers in verifications with
explicit checks (ArgumentCaptor or eq("expected-bucket")) to assert the correct
bucket is used.
---
Nitpick comments:
In `@src/main/java/backendlab/team4you/config/S3Config.java`:
- Around line 28-37: The S3Client bean (s3Client method) isn't being closed on
JVM shutdown; update the bean definition to ensure the S3Client is closed by the
Spring container (e.g., declare the `@Bean` with destroyMethod="close" or
implement a lifecycle hook that calls close on the S3Client) so the
SdkAutoCloseable connections are released when the application stops. Ensure the
change targets the s3Client() bean and uses the S3Client.close() method as the
destroy action.
In `@src/main/java/backendlab/team4you/s3/S3Service.java`:
- Around line 27-36: The uploadFile method in S3Service should validate the key
parameter before calling s3Client.putObject: check that key is not null and not
blank (e.g., after trimming) and if invalid throw an IllegalArgumentException
with a clear message like "S3 key must not be null or empty"; update the
uploadFile(String key, byte[] data, String contentType) method to perform this
validation and only call s3Client.putObject (and use bucketName) when the key
passes validation.
- Around line 38-46: The downloadFile method currently returns the raw
ResponseInputStream (open HTTP connection) which callers must close; change
downloadFile(String key) to return a byte[] instead and inside the method call
s3Client.getObject(GetObjectRequest.builder().bucket(bucketName).key(key).build())
within a try-with-resources block (capturing the
ResponseInputStream<GetObjectResponse>), read all bytes (e.g.,
response.readAllBytes()) and return the byte[] so the service closes the stream
itself; alternatively if you keep the InputStream signature, add Javadoc to
downloadFile(String key) noting it returns a ResponseInputStream that the caller
MUST close and update callers accordingly.
In `@src/test/java/backendlab/team4you/s3/S3ServiceTest.java`:
- Around line 50-62: The unchecked generic-mock warning in
S3ServiceTest.downloadFile_shouldCallGetObject comes from mocking
ResponseInputStream without type parameters; fix it by performing an explicit
cast to ResponseInputStream<GetObjectResponse> and suppressing the unchecked
warning for that statement or the test method (e.g., add
`@SuppressWarnings`("unchecked") and write ResponseInputStream<GetObjectResponse>
mockStream = (ResponseInputStream<GetObjectResponse>)
mock(ResponseInputStream.class)); keep the verify(s3Client,
times(1)).getObject(...) assertion unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a6adc669-f748-4b4c-85cc-ce8515561134
📒 Files selected for processing (9)
docker-compose.yamlpom.xmlsrc/main/java/backendlab/team4you/config/S3Config.javasrc/main/java/backendlab/team4you/config/SecurityConfig.javasrc/main/java/backendlab/team4you/s3/S3Controller.javasrc/main/java/backendlab/team4you/s3/S3Service.javasrc/main/java/backendlab/team4you/s3/s3-test.httpsrc/main/resources/application.propertiessrc/test/java/backendlab/team4you/s3/S3ServiceTest.java
| CustomAuthenticationSuccessHandler successHandler) throws Exception { | ||
|
|
||
| return http | ||
| .csrf(csrf -> csrf.disable()) |
Contributor
There was a problem hiding this comment.
Disabling CSRF globally is overly broad.
CSRF is disabled for the entire application, which exposes all form-based endpoints (login, profile, etc.) to CSRF attacks. If CSRF needs to be disabled for the REST API endpoints, consider a more targeted approach using request matchers.
🔒 Proposed fix to scope CSRF disable to API endpoints only
return http
- .csrf(csrf -> csrf.disable())
+ .csrf(csrf -> csrf
+ .ignoringRequestMatchers("/api/**"))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .csrf(csrf -> csrf.disable()) | |
| .csrf(csrf -> csrf | |
| .ignoringRequestMatchers("/api/**")) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/backendlab/team4you/config/SecurityConfig.java` at line 28,
Current code in SecurityConfig uses .csrf(csrf -> csrf.disable()), which
disables CSRF globally; change this to disable CSRF only for API endpoints by
replacing the global disable with a scoped configuration that ignores or
disables CSRF for requests matching your REST API paths (e.g., "/api/**")
instead; locate the .csrf(csrf -> csrf.disable()) call in SecurityConfig and
update it to use csrf(...).ignoringRequestMatchers or an equivalent
request-matcher-based API so non-API (form-based) endpoints keep CSRF
protection.
Comment on lines
+35
to
+36
| .requestMatchers("/api/files/**").permitAll() | ||
|
|
Contributor
There was a problem hiding this comment.
Unrestricted access to file operations is a security risk.
Permitting unauthenticated access to /api/files/** allows anyone to upload, download, and delete files without authentication. This could lead to:
- Unauthorized data exfiltration via download
- Storage abuse via unlimited uploads
- Data loss via anonymous deletions
Consider restricting at least the upload and delete operations to authenticated users, or add method-level security annotations (@PreAuthorize) in S3Controller.java.
🔒 Proposed fix to require authentication for file operations
- .requestMatchers("/api/files/**").permitAll()
+ .requestMatchers("/api/files/download/**").permitAll()
+ .requestMatchers("/api/files/upload", "/api/files/delete/**").hasAnyRole("USER", "ADMIN")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .requestMatchers("/api/files/**").permitAll() | |
| .requestMatchers("/api/files/download/**").permitAll() | |
| .requestMatchers("/api/files/upload", "/api/files/delete/**").hasAnyRole("USER", "ADMIN") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/backendlab/team4you/config/SecurityConfig.java` around lines 35
- 36, SecurityConfig currently permits all access to "/api/files/**", which is
unsafe; update the HTTP security rules to require authentication for
file-mutating endpoints (e.g., change the matcher for POST/PUT/DELETE to
.authenticated() and optionally allow GET for public downloads) by using
method-specific requestMatchers (HttpMethod.POST/PUT/DELETE with
"/api/files/**") instead of a blanket .permitAll(), and/or add method-level
security annotations in S3Controller on the mutating handlers (annotate
uploadFile, deleteFile, updateFile methods with
`@PreAuthorize`("isAuthenticated()" or a role check) and enable
`@EnableMethodSecurity` if not already configured. Ensure the SecurityConfig
ordering places these matchers before any broader permits so the authenticated
rules are enforced.
| InputStream stream = s3Service.downloadFile(key); | ||
| byte[] bytes = stream.readAllBytes(); | ||
| return ResponseEntity.ok() | ||
| .header(HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=\"" + key + "\"") |
Contributor
There was a problem hiding this comment.
Sanitize filename in Content-Disposition header.
The key is used directly in the Content-Disposition header. If the key contains characters like " or newlines, it could cause header injection or malformed responses.
🛡️ Proposed fix to sanitize the filename
- .header(HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=\"" + key + "\"")
+ .header(HttpHeaders.CONTENT_DISPOSITION,
+ ContentDisposition.attachment()
+ .filename(key, StandardCharsets.UTF_8)
+ .build()
+ .toString())Add import:
import org.springframework.http.ContentDisposition;
import java.nio.charset.StandardCharsets;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .header(HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=\"" + key + "\"") | |
| .header(HttpHeaders.CONTENT_DISPOSITION, | |
| ContentDisposition.attachment() | |
| .filename(key, StandardCharsets.UTF_8) | |
| .build() | |
| .toString()) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/backendlab/team4you/s3/S3Controller.java` at line 38, The
Content-Disposition header currently inserts the raw key into
HttpHeaders.CONTENT_DISPOSITION which can allow header injection; update the
S3Controller (where the response header is built using
HttpHeaders.CONTENT_DISPOSITION and the variable key) to build a safe header
value via org.springframework.http.ContentDisposition (e.g.,
ContentDisposition.builder("attachment").filename(...).build().toString()) and
ensure the filename is encoded/normalized using
java.nio.charset.StandardCharsets (or set both filename and filename* with
UTF-8) so quotes/newlines are handled and the header value is sanitized.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/main/java/backendlab/team4you/s3/S3Controller.java (1)
41-41:⚠️ Potential issue | 🟠 MajorBuild
Content-Dispositionsafely from untrustedkey.Line [41] inserts raw
keyinto a header value. UseContentDispositionbuilder to avoid malformed/header-injection values.💡 Proposed fix
import org.springframework.http.HttpHeaders; import org.springframework.http.MediaType; +import org.springframework.http.ContentDisposition; import org.springframework.http.ResponseEntity; +import java.nio.charset.StandardCharsets; @@ - .header(HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=\"" + key + "\"") + .header(HttpHeaders.CONTENT_DISPOSITION, + ContentDisposition.attachment() + .filename(key, StandardCharsets.UTF_8) + .build() + .toString())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/s3/S3Controller.java` at line 41, The Content-Disposition header in S3Controller is built by concatenating the untrusted key into the header value (HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=\"" + key + "\""), which risks header injection; fix it by using Spring's ContentDisposition.builder() to construct a safe disposition from the key (e.g., setType("attachment") and filename(key) or filenameEncoded if needed) and then pass contentDisposition.toString() (or ContentDisposition header string) to the header() call so the framework safely escapes/encodes the filename.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@init-localstack.sh`:
- Line 2: Make the bucket creation idempotent and safely quoted: instead of
unconditionally running awslocal s3 mb s3://${AWS_BUCKET_NAME:-team4you-files},
check for the bucket first (e.g., using awslocal s3api head-bucket or
list-buckets) and only call awslocal s3 mb if the check indicates the bucket
does not exist, and always quote the bucket name variable (use
"${AWS_BUCKET_NAME:-team4you-files}") in the check and create commands; update
the invocation in init-localstack.sh where awslocal s3 mb is used.
In `@src/main/java/backendlab/team4you/s3/S3Controller.java`:
- Around line 13-16: The S3 endpoints are publicly accessible; remove the
permissive rule and require authenticated access: update SecurityConfig to
remove permitAll() for the "/api/files/**" matcher (so it requires
authentication) and/or replace it with an .authenticated() or role-based rule
(e.g., hasRole("ADMIN")), then add method-level protection in S3Controller by
annotating the upload and delete handlers (the POST "/api/files/upload" method
and DELETE "/api/files/delete/{key}" method) with `@PreAuthorize` or `@Secured` as
appropriate (e.g., `@PreAuthorize`("isAuthenticated()" or
`@PreAuthorize`("hasRole('ADMIN')")) to ensure only authorized users can perform
upload/delete. Ensure imports for `@PreAuthorize/`@Secured are added and Spring
Security global method security is enabled if not already.
- Around line 37-44: The downloadFile method in S3Controller currently reads the
whole InputStream into memory via stream.readAllBytes(); change it to return a
ResponseEntity<StreamingResponseBody> that obtains the InputStream from
s3Service.downloadFile(key) and writes it to the response using
StreamingResponseBody with InputStream.transferTo(outputStream), preserving the
Content-Disposition and contentType headers; ensure the InputStream is closed
after transfer (use try-with-resources inside the StreamingResponseBody lambda)
and propagate IO errors as appropriate.
---
Duplicate comments:
In `@src/main/java/backendlab/team4you/s3/S3Controller.java`:
- Line 41: The Content-Disposition header in S3Controller is built by
concatenating the untrusted key into the header value
(HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=\"" + key + "\""), which
risks header injection; fix it by using Spring's ContentDisposition.builder() to
construct a safe disposition from the key (e.g., setType("attachment") and
filename(key) or filenameEncoded if needed) and then pass
contentDisposition.toString() (or ContentDisposition header string) to the
header() call so the framework safely escapes/encodes the filename.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f2d1b71d-dd7f-433e-9ac0-03b6c32dd266
📒 Files selected for processing (4)
docker-compose.yamlinit-localstack.shsrc/main/java/backendlab/team4you/s3/S3Controller.javasrc/test/java/backendlab/team4you/s3/S3ServiceTest.java
✅ Files skipped from review due to trivial changes (1)
- src/test/java/backendlab/team4you/s3/S3ServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- docker-compose.yaml
Comment on lines
+13
to
+16
| @RestController | ||
| // All endpoints start with /api/files | ||
| @RequestMapping("/api/files") | ||
| public class S3Controller { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 --type=java 'requestMatchers\("/api/files/\*\*"\)\.permitAll\(\)|@RequestMapping\("/api/files"\)|@PostMapping\("/upload"\)|@DeleteMapping\("/delete/\{key\}"\)'Repository: ithsjava25/project-backend-team4you
Length of output: 2123
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the full S3Controller to check for method-level security annotations
echo "=== S3Controller.java (full) ==="
cat -n src/main/java/backendlab/team4you/s3/S3Controller.java
echo -e "\n=== SecurityConfig.java (full) ==="
cat -n src/main/java/backendlab/team4you/config/SecurityConfig.javaRepository: ithsjava25/project-backend-team4you
Length of output: 7312
Restrict /api/files/** endpoints to authenticated users; currently allows anonymous upload/delete.
The endpoints at /api/files/upload (POST) and /api/files/delete/{key} (DELETE) are destructive operations but are publicly accessible. SecurityConfig.java line 35 permits /api/files/** to all users without authentication. The S3Controller methods have no @PreAuthorize, @Secured, or internal authorization checks. Remove permitAll() from line 35 and require authentication; optionally restrict to specific roles if only certain users should manage files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/backendlab/team4you/s3/S3Controller.java` around lines 13 - 16,
The S3 endpoints are publicly accessible; remove the permissive rule and require
authenticated access: update SecurityConfig to remove permitAll() for the
"/api/files/**" matcher (so it requires authentication) and/or replace it with
an .authenticated() or role-based rule (e.g., hasRole("ADMIN")), then add
method-level protection in S3Controller by annotating the upload and delete
handlers (the POST "/api/files/upload" method and DELETE
"/api/files/delete/{key}" method) with `@PreAuthorize` or `@Secured` as appropriate
(e.g., `@PreAuthorize`("isAuthenticated()" or `@PreAuthorize`("hasRole('ADMIN')"))
to ensure only authorized users can perform upload/delete. Ensure imports for
`@PreAuthorize/`@Secured are added and Spring Security global method security is
enabled if not already.
Comment on lines
+37
to
+44
| public ResponseEntity<byte[]> downloadFile(@PathVariable String key) throws IOException { | ||
| try (InputStream stream = s3Service.downloadFile(key)) { | ||
| byte[] bytes = stream.readAllBytes(); | ||
| return ResponseEntity.ok() | ||
| .header(HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=\"" + key + "\"") | ||
| .contentType(MediaType.APPLICATION_OCTET_STREAM) | ||
| .body(bytes); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "S3Controller.java" -o -name "*.java" | head -20Repository: ithsjava25/project-backend-team4you
Length of output: 61
🏁 Script executed:
find . -path "*/s3/*" -name "*.java" | head -20Repository: ithsjava25/project-backend-team4you
Length of output: 247
🏁 Script executed:
find . -name "S3Controller.java"Repository: ithsjava25/project-backend-team4you
Length of output: 135
🏁 Script executed:
cat -n ./src/main/java/backendlab/team4you/s3/S3Controller.javaRepository: ithsjava25/project-backend-team4you
Length of output: 2442
🏁 Script executed:
cat -n ./src/main/java/backendlab/team4you/s3/S3Service.javaRepository: ithsjava25/project-backend-team4you
Length of output: 2330
🏁 Script executed:
find . -name "pom.xml" -o -name "build.gradle"Repository: ithsjava25/project-backend-team4you
Length of output: 88
🏁 Script executed:
cat -n ./pom.xmlRepository: ithsjava25/project-backend-team4you
Length of output: 5111
Stream S3 downloads to prevent memory spikes with large files.
The current implementation at line 39 uses readAllBytes() which loads the entire S3 object into heap memory. For large files, this causes memory spikes and risks OOM. Use StreamingResponseBody with transferTo() to stream the response instead.
💡 Proposed fix
import org.springframework.http.HttpHeaders;
import org.springframework.http.MediaType;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.*;
import org.springframework.web.multipart.MultipartFile;
+import org.springframework.web.servlet.mvc.method.annotation.StreamingResponseBody;
@@
- public ResponseEntity<byte[]> downloadFile(`@PathVariable` String key) throws IOException {
- try (InputStream stream = s3Service.downloadFile(key)) {
- byte[] bytes = stream.readAllBytes();
- return ResponseEntity.ok()
- .header(HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=\"" + key + "\"")
- .contentType(MediaType.APPLICATION_OCTET_STREAM)
- .body(bytes);
- }
+ public ResponseEntity<StreamingResponseBody> downloadFile(`@PathVariable` String key) {
+ StreamingResponseBody body = outputStream -> {
+ try (InputStream stream = s3Service.downloadFile(key)) {
+ stream.transferTo(outputStream);
+ }
+ };
+ return ResponseEntity.ok()
+ .header(HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=\"" + key + "\"")
+ .contentType(MediaType.APPLICATION_OCTET_STREAM)
+ .body(body);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/backendlab/team4you/s3/S3Controller.java` around lines 37 - 44,
The downloadFile method in S3Controller currently reads the whole InputStream
into memory via stream.readAllBytes(); change it to return a
ResponseEntity<StreamingResponseBody> that obtains the InputStream from
s3Service.downloadFile(key) and writes it to the response using
StreamingResponseBody with InputStream.transferTo(outputStream), preserving the
Content-Disposition and contentType headers; ensure the InputStream is closed
after transfer (use try-with-resources inside the StreamingResponseBody lambda)
and propagate IO errors as appropriate.
MartinStenhagen
approved these changes
Apr 15, 2026
Closed
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
2 participants
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.
Added S3 file handling pipeline using AWS SDK and LocalStack 3.8.1 for local development.
Includes upload, download and delete functionality exposed via REST API:
Unit tested with Mockito. No real AWS credentials required anymore.
Summary by CodeRabbit
New Features
Tests
Chores