Content disposition modified signature #287
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe controller’s file download method was refactored to build an attachment response with URL-encoded filenames, explicit UTF-8 handling, robust MIME type parsing with fallbacks, accurate content length, and revised error propagation. An unused dependency was removed. No public method signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Controller as EmployeeSignatureController
participant MIME as MIME Parser
participant HTTP as HTTP Response
Client->>Controller: GET /employee/signature/{id}
activate Controller
Controller->>Controller: Retrieve signature bytes, filename, fileType
alt Parse MIME type
Controller->>MIME: parse(fileType)
MIME-->>Controller: MediaType
else Invalid/Null fileType
Controller->>Controller: Use application/octet-stream
end
Controller->>Controller: URL-encode filename (UTF-8)
Controller->>HTTP: Build headers (Content-Type, Content-Length, Content-Disposition: attachment)
deactivate Controller
HTTP-->>Client: 200 OK + bytes
opt Error
Client->>Controller: GET /employee/signature/{id}
activate Controller
Controller->>Controller: Exception occurs
Controller-->>Client: Propagated exception (non-200)
deactivate Controller
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches🧪 Generate unit tests
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/java/com/iemr/common/controller/users/EmployeeSignatureController.java (2)
71-77: Enhance MIME fallback using filename; keep octet-stream as last resortCurrent fallback is fine; optionally also infer from filename for better accuracy.
-MediaType mediaType; -try { - mediaType = MediaType.parseMediaType(userSignID.getFileType()); -} catch (InvalidMediaTypeException | NullPointerException e) { - mediaType = MediaType.APPLICATION_OCTET_STREAM; -} +MediaType mediaType = null; +try { + mediaType = MediaType.parseMediaType(userSignID.getFileType()); +} catch (InvalidMediaTypeException | NullPointerException ignored) { } +if (mediaType == null) { + mediaType = org.springframework.http.MediaTypeFactory + .getMediaType(originalName) + .orElse(MediaType.APPLICATION_OCTET_STREAM); +}
24-26: Refactor: use Spring’s ContentDisposition builder instead of URLEncoder Project is on Java 17 and Spring Boot 3.2.2 (so Charset-overload compiles and ContentDisposition is available). RemoveURLEncoder, switch toContentDisposition cd = ContentDisposition.builder("attachment") .filename(userSignID.getFileName(), StandardCharsets.UTF_8) .build(); headers.setContentDisposition(cd);and add imports for
org.springframework.http.ContentDisposition,org.springframework.http.CacheControl,org.springframework.http.HttpStatus, andorg.springframework.web.server.ResponseStatusException.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/main/java/com/iemr/common/controller/users/EmployeeSignatureController.java(2 hunks)
🔇 Additional comments (3)
src/main/java/com/iemr/common/controller/users/EmployeeSignatureController.java (3)
32-32: LGTM: import for robust MIME parsingImporting InvalidMediaTypeException is appropriate for safe parsing fallback.
80-81: LGTM: correct headers, content type, and lengthGood use of contentType and contentLength with byte[] body.
83-86: Don’t throw generic Exception; map to HTTP with causeUse ResponseStatusException to preserve the stack and set status explicitly.
-} catch (Exception e) { - logger.error("File download for userID failed with exception " + e.getMessage(), e); - throw new Exception("Error while downloading file. Please contact administrator.."); -} +} catch (Exception e) { + logger.error("File download for userID failed with exception " + e.getMessage(), e); + throw new ResponseStatusException( + HttpStatus.INTERNAL_SERVER_ERROR, + "Error while downloading file. Please contact administrator.", + e); +}⛔ Skipped due to learnings
Learnt from: helenKaryamsetty PR: PSMRI/Common-API#123 File: src/main/java/com/iemr/common/controller/abdmfacility/AbdmFacilityController.java:41-45 Timestamp: 2024-11-20T07:23:22.514Z Learning: In the `Common-API` codebase, exception handling within controller classes like `AbdmFacilityController.java` is managed using the `OutputResponse` class, and it is not required to catch specific exceptions separately. General exception handling is sufficient in this context.
| HttpHeaders responseHeaders = new HttpHeaders(); | ||
| String fileName = URLEncoder.encode(userSignID.getFileName(), StandardCharsets.UTF_8); | ||
| responseHeaders.set(HttpHeaders.CONTENT_DISPOSITION, | ||
| "attachment; filename=\"" + fileName + "\"; filename*=UTF-8''" + fileName); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Build RFC 6266/5987-compliant Content-Disposition and prevent sensitive caching
Use Spring’s builder to correctly emit filename and filename* (UTF-8, percent-encoded), avoid '+' issues, and add no-store headers since signatures are PII.
-HttpHeaders responseHeaders = new HttpHeaders();
-String fileName = URLEncoder.encode(userSignID.getFileName(), StandardCharsets.UTF_8);
-responseHeaders.set(HttpHeaders.CONTENT_DISPOSITION,
- "attachment; filename=\"" + fileName + "\"; filename*=UTF-8''" + fileName);
+String originalName = userSignID.getFileName();
+if (originalName == null || originalName.isBlank()) {
+ originalName = "signature";
+}
+HttpHeaders responseHeaders = new HttpHeaders();
+responseHeaders.setContentDisposition(
+ ContentDisposition.attachment().filename(originalName, StandardCharsets.UTF_8).build());
+responseHeaders.setCacheControl(CacheControl.noStore());
+responseHeaders.add(HttpHeaders.PRAGMA, "no-cache");
+responseHeaders.setExpires(0);📝 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.
| HttpHeaders responseHeaders = new HttpHeaders(); | |
| String fileName = URLEncoder.encode(userSignID.getFileName(), StandardCharsets.UTF_8); | |
| responseHeaders.set(HttpHeaders.CONTENT_DISPOSITION, | |
| "attachment; filename=\"" + fileName + "\"; filename*=UTF-8''" + fileName); | |
| String originalName = userSignID.getFileName(); | |
| if (originalName == null || originalName.isBlank()) { | |
| originalName = "signature"; | |
| } | |
| HttpHeaders responseHeaders = new HttpHeaders(); | |
| responseHeaders.setContentDisposition( | |
| ContentDisposition.attachment().filename(originalName, StandardCharsets.UTF_8).build()); | |
| responseHeaders.setCacheControl(CacheControl.noStore()); | |
| responseHeaders.add(HttpHeaders.PRAGMA, "no-cache"); | |
| responseHeaders.setExpires(0); |
🤖 Prompt for AI Agents
In
src/main/java/com/iemr/common/controller/users/EmployeeSignatureController.java
around lines 66 to 70, the code manually constructs a Content-Disposition header
with a URL-encoded filename (which can produce '+' for spaces and doesn't fully
follow RFC6266/5987) and lacks anti-caching headers for sensitive PII; replace
the manual header with Spring's ContentDisposition builder
(ContentDisposition.builder("attachment").filename(originalFileName,
StandardCharsets.UTF_8).build()) and use HttpHeaders.setContentDisposition(...)
so Spring emits both filename and filename* correctly (percent-encoded UTF-8),
and add response headers to prevent caching (Cache-Control: no-store, Pragma:
no-cache, Expires: 0).
| byte[] fileBytes = userSignID.getSignature(); // MUST be byte[] | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against missing payload; return 404 for absent signature
Avoid NPE on length and emit a clear 404 when no bytes are present.
-byte[] fileBytes = userSignID.getSignature(); // MUST be byte[]
+byte[] fileBytes = userSignID.getSignature(); // MUST be byte[]
+if (fileBytes == null || fileBytes.length == 0) {
+ throw new ResponseStatusException(HttpStatus.NOT_FOUND,
+ "Signature not found for userID: " + userID);
+}📝 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.
| byte[] fileBytes = userSignID.getSignature(); // MUST be byte[] | |
| byte[] fileBytes = userSignID.getSignature(); // MUST be byte[] | |
| if (fileBytes == null || fileBytes.length == 0) { | |
| throw new ResponseStatusException(HttpStatus.NOT_FOUND, | |
| "Signature not found for userID: " + userID); | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/com/iemr/common/controller/users/EmployeeSignatureController.java
around lines 78-79, the code assumes userSignID.getSignature() is non-null and
accesses its length, which risks an NPE; add a null/empty check before using the
byte[] and return a 404 when the signature is absent. Specifically, first verify
userSignID != null and signatureBytes != null && signatureBytes.length > 0, and
if that fails respond with a 404 (e.g., ResponseEntity.notFound().build() or
throw new ResponseStatusException(HttpStatus.NOT_FOUND)); only then proceed to
write or return the byte[].
|
📋 Description
JIRA ID:
Please provide a summary of the change and the motivation behind it. Include relevant context and details.
✅ Type of Change
ℹ️ Additional Information
Please describe how the changes were tested, and include any relevant screenshots, logs, or other information that provides additional context.
Summary by CodeRabbit