Refactored code to align with Vanilla JS / REST-api#96
Merged
Conversation
|
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 41 minutes and 50 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughReplaced server-side page controller with static .html pages, updated SecurityConfig to protect Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DocumentController
participant DocumentService
participant MinioService
participant Repository
Client->>DocumentController: GET /api/documents/{fileKey}/download
DocumentController->>DocumentService: getByFileKey(fileKey)
DocumentService->>Repository: findByFileKey(fileKey)
Repository-->>DocumentService: Document or empty
alt Document found
DocumentService->>MinioService: getFile(fileKey)
MinioService-->>DocumentService: InputStream (file)
DocumentController-->>Client: 200 OK + stream (Content-Disposition)
else Not found
DocumentController-->>Client: 404 Not Found
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
src/main/resources/static/admin.html (1)
778-782:⚠️ Potential issue | 🟠 MajorResidual Thymeleaf in the navbar won't render in a static file.
th:text="${user.name ...}"andth:text="${role}"are inert here sinceadmin.htmlis served as a static resource. The navbar will display the literal fallback text ("User", "ADMIN") for every session. Fetch the current user (e.g./api/users/me) in JS and populate.user-name/.role-badgefrom the response, same pattern the rest of the page already uses.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/admin.html` around lines 778 - 782, The navbar still contains Thymeleaf attributes that won't be processed in this static admin.html; replace the inert th:text usage by adding client-side code to fetch the current user (GET /api/users/me) and then set the DOM textContent for the .user-name element (use user.name || user.githubLogin) and the .role-badge element (use user.role or response.role), keeping the existing logout() button behavior; locate the .user-name and .role-badge nodes in the script that initializes the page (or add a small init function) and update them after the fetch, handling errors by leaving the fallback text unchanged or logging to console.src/main/resources/static/incidents.html (1)
173-199:⚠️ Potential issue | 🔴 Critical
roleis always unset, so the page always shows "Unable to load incidents".
document.body.dataset.roleis sourced fromth:attr="data-role=${role}"(line 173). Since the file is now static, thedata-roleattribute is never written —roleisundefinedat line 185 — and the dispatch at lines 191-199 falls into theelsebranch that renders "Unable to load incidents".Fetch the role client-side (e.g. via a
/api/users/meor/api/auth/roleendpoint) before branching, or attach the role via a small cookie/header set by the backend during authentication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/incidents.html` around lines 173 - 199, The page reads role from document.body.dataset.role (set via th:attr="data-role=${role}") but since the HTML is now static role is undefined, causing loadIncidents() to always hit the unknown-role branch; update loadIncidents (or an init function called before it) to retrieve the current user's role from the backend (e.g., call a /api/users/me or /api/auth/role endpoint) and set a local variable (role) before the existing branching logic, or alternatively read role from a backend-set cookie/header and use that value instead of document.body.dataset.role; ensure you reference and update the same symbol loadIncidents and remove reliance on document.body.dataset.role so the ADMIN/HANDLER/RESIDENT branches work correctly.src/main/resources/static/profile.html (2)
213-218:⚠️ Potential issue | 🟡 MinorTwo navigation links missed the
.htmlrename.Lines 215 and 217 still point to the extensionless routes, so they bypass the
/profile.html//admin.htmlmatchers added inSecurityConfigand will 404 oncePageControlleris gone.Proposed fix
<a href="/dashboard.html">Home</a> <a th:if="${role != 'PENDING'}" href="/incidents.html">Incidents</a> - <a href="/profile">Profile</a> + <a href="/profile.html">Profile</a> <div th:if="${role == 'ADMIN'}"> - <a href="/admin">Admin</a> + <a href="/admin.html">Admin</a> </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/profile.html` around lines 213 - 218, Update the hardcoded anchor hrefs in profile.html so they point to the new .html pages: change the Incidents link (the <a th:if="${role != 'PENDING'}" href="/incidents.html">) and the Profile link (currently href="/profile") to use /incidents.html and /profile.html respectively, and also update the Admin link inside the <div th:if="${role == 'ADMIN'}"> to href="/admin.html" so these anchors match the SecurityConfig .html matchers and avoid 404s; keep the existing th:if conditions unchanged.
1-295:⚠️ Potential issue | 🔴 CriticalThymeleaf directives throughout won't be processed in a static file.
profile.htmlnow lives undersrc/main/resources/static/but is still built around server-side rendering:th:if,th:text,th:classappend,th:src,th:attr,xmlns:that lines 2, 214, 216, 220–221, 231–236, 242, 247, 252, 257, 260. Served statically these are emitted verbatim, so:
- User identity/name/email/avatar never populate (the page shows literal placeholders "User", "user@example.com", "K", "-").
- The PENDING role pending-note and ADMIN-only Admin link never render conditionally.
accountCreateddatum is read fromdata-created-atat line 287 — but that attribute is only set viath:attr, so the date won't render either.Convert to vanilla JS consistent with the PR's direction: fetch user data from a REST endpoint (e.g.
/api/users/me) inDOMContentLoadedand render into plain DOM nodes; use a data API instead of${role}for the admin link visibility. Same pattern asdashboard.html/admin.html/incidents.html/viewincident.html.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/profile.html` around lines 1 - 295, The file still uses Thymeleaf directives but now lives in static/—replace server-side rendering with client-side rendering: in the existing DOMContentLoaded handler (document.addEventListener("DOMContentLoaded", ...)) fetch the current user from /api/users/me, then populate DOM nodes by id/class: set .user-name text, .info-value nodes for display name/email/githubLogin, set the profile-avatar img src or create the avatar-placeholder text, set .role-badge text and add the role class (role.toLowerCase()), show/hide the Admin nav link and Incidents link based on role, and write the formatted createdAt string into `#accountCreated` using the existing formatDate function; ensure logout() stays intact and remove/ignore all th:* attributes so the page works purely from the fetched JSON.src/main/resources/static/createincident.html (1)
93-139:⚠️ Potential issue | 🔴 CriticalForm is non-functional: Thymeleaf in a static file + stale action.
This file is served from
src/main/resources/static/and is therefore delivered as a static resource — Spring does not run Thymeleaf against static resources. Every directive (th:action,th:object,th:field,th:if,th:errors,th:name,th:value,th:text) is rendered as literal attributes, so:
- The
actionattribute resolves to nothing useful;th:action="@{/create-incident}"also no longer matches any controller afterPageControllerremoval.- The hidden CSRF input (
th:name="${_csrf.parameterName}") is never populated → POSTs will be rejected by the CSRF filter.th:field="*{subject}",th:field="*{description}", andth:field="*{incidentCategory}"never become realname/valueattributes, so the form submits nothing the backend can bind.- Validation blocks (
#fields.hasErrors(...)) render as static markup with no conditional.This whole page needs to be converted to vanilla JS (matching the rest of the PR): add explicit
nameattributes, manually fetch the CSRF token (or add/api/**tocsrf.ignoringRequestMatchersappropriately), andfetch()the relevant REST endpoints (e.g.POST /api/incidentsthenPOST /api/documents/upload/{incidentId}as a follow-up upload), handling errors in JS rather than Thymeleaf#fields.The same Thymeleaf-in-static issue applies to
profile.html,admin.html,dashboard.html,viewincident.html, andincidents.html— called out individually there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/createincident.html` around lines 93 - 139, The form in createincident.html is broken because it contains Thymeleaf attributes that are not processed in static resources; replace the template directives by converting the form to plain HTML with explicit name attributes (e.g., inputs for subject, description, incidentCategory, files) and remove th:* attributes, then implement a small JS module that (1) retrieves a CSRF token (via a meta tag or a GET to /api/csrf), (2) posts JSON to the REST create endpoint (POST /api/incidents) to create the incident, (3) on success uploads files via multipart to POST /api/documents/upload/{incidentId}, and (4) displays validation/server errors in the DOM instead of using `#fields.hasErrors`; apply the same conversion pattern to profile.html, admin.html, dashboard.html, viewincident.html, and incidents.html so no th:* attributes remain in static files.src/main/resources/static/dashboard.html (2)
693-712:⚠️ Potential issue | 🟡 MinorSeveral navigation links missed the
.htmlrename.These four still point at the removed
PageControllerroutes and will 404 (and bypass the newSecurityConfigrole matchers):Proposed fix
- <a href="/admin" class="btn btn-outline btn-sm">Manage all →</a> + <a href="/admin.html" class="btn btn-outline btn-sm">Manage all →</a> ... - <a th:if="${role == 'RESIDENT'}" href="/create-incident" class="btn btn-primary">Create new incident</a> - <a th:if="${role == 'HANDLER'}" href="/incidents" class="btn btn-handler">Manage incidents</a> - <a th:if="${role == 'ADMIN'}" href="/admin" class="btn btn-admin">Go to Admin Panel</a> + <a th:if="${role == 'RESIDENT'}" href="/createincident.html" class="btn btn-primary">Create new incident</a> + <a th:if="${role == 'HANDLER'}" href="/incidents.html" class="btn btn-handler">Manage incidents</a> + <a th:if="${role == 'ADMIN'}" href="/admin.html" class="btn btn-admin">Go to Admin Panel</a>(The
th:ifon these will remain non-functional until the Thymeleaf-in-static issue above is fixed, but thehrefvalues should be consistent with the rest of the file regardless.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/dashboard.html` around lines 693 - 712, Update the hard-coded hrefs to use the .html routes so they match the new pages and SecurityConfig matchers: change the anchor with text "Manage all →" (inside the pending users card), and the three action buttons "Create new incident", "Manage incidents", and "Go to Admin Panel" (inside the div with id "actionButtons") to point to /admin.html, /create-incident.html, /incidents.html, and /admin.html respectively; leave the th:if expressions as-is.
594-714:⚠️ Potential issue | 🔴 CriticalEntire dashboard depends on Thymeleaf that won't run in a static file.
dashboard.htmlis served fromsrc/main/resources/static/, so noth:*attribute is ever processed. In particular:
- Line 594:
data-role,data-user-id,data-user-activenever get written →userRole,userId,userActiveat lines 718-720 are allundefined/"false". The DOMContentLoaded handler at lines 987-994 then skipsloadIncidents()andloadPendingUsers()on every page load, so stats/lists stay at their placeholder text.- Lines 645-646: user name and role-badge never populate.
- Lines 657, 665, 672, 690, 703, 709-711, 603: every
th:ifbranch is ignored, so the "Account Deactivated" card, "pending approval" card, and role-scoped action buttons are either all rendered together as static markup or all missing, regardless of the user.This page (and the other static HTML files flagged) needs to fetch user/role via a REST endpoint on load, render the right branch client-side, and set the role/userId on
document.body.datasetitself.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/dashboard.html` around lines 594 - 714, The dashboard currently relies on Thymeleaf-only attributes (th:*) in static/dashboard.html so client-side code never gets role/user info; replace that by fetching the current user from your REST user endpoint (e.g., GET /api/me) in the DOMContentLoaded handler, then set document.body.dataset.role, dataset.userId and dataset.userActive from the JSON response and update DOM nodes (querySelector('.user-name'), querySelector('.role-badge'), `#totalIncidents`, `#openIncidents`, `#resolvedIncidents`, `#pendingUsersCard`, `#incidentsList`, `#actionButtons`, `#dropdownUsersList`, etc.) to show/hide content based on role/active state; finally ensure after setting these datasets you call loadIncidents() and loadPendingUsers() as needed so stats/lists populate.src/main/resources/static/viewincident.html (2)
215-243:⚠️ Potential issue | 🟠 MajorDocument links use
/documents/...but the controller is now under/api/documents.
DocumentControllerwas remapped to@RequestMapping("/api/documents"), so these links need/api/documents/as the base. As written, clicking an image/file opens a 404.Proposed fix
- const fileUrl = "/documents/" + encodeURIComponent(doc.fileKey); + const fileUrl = "/api/documents/" + encodeURIComponent(doc.fileKey);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/viewincident.html` around lines 215 - 243, The document links are using "/documents/" but the controller was remapped to "/api/documents"; update the construction of fileUrl in the incident.documents.map block (where fileUrl = "/documents/" + encodeURIComponent(doc.fileKey)) to use "/api/documents/" as the base so both the anchor href and img src point to "/api/documents/{encodedKey}"; keep the existing encodeURIComponent, isImage detection, and escapeHtml(doc.fileName) logic intact so only the base path is changed to match DocumentController's `@RequestMapping`("/api/documents").
145-183:⚠️ Potential issue | 🔴 CriticalPage is unreachable at the URL the app navigates to, and Thymeleaf inputs render as literals.
getIncidentIdFromUrl()(line 186) assumes the page is served at a path like/incidents.html/{id}, butincidents.htmlandviewincident.htmlare both static files. Requests to/incidents.html/5don't match any static resource or controller — they 404. Either name this file to match the URL and add a proper controller (or static resource mapping) for/incidents/{id}, or switch the navigation to use a query string (e.g.viewincident.html?id=5) and updategetIncidentIdFromUrl()to readURLSearchParams.body th:attr="data-role=${role}", theth:name/th:valueCSRF inputs (lines 163-164), andth:value="${userId}"(line 165) are not processed fromstatic/, so the hiddenincidentId/userId/CSRF fields submit empty. The comment form will fail CSRF and the backend user/incident binding.- Line 162 still posts to
/comments(the extensionless Thymeleaf form-redirect controller). With the page being vanilla JS elsewhere, this should go through a REST endpoint with a JSON body and explicit CSRF handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/viewincident.html` around lines 145 - 183, The page is unreachable and Thymeleaf expressions are not processed because viewincident.html is served as a static file and getIncidentIdFromUrl() assumes a path-style ID; either move this file into Thymeleaf templates and expose a controller for /incidents/{id} so body th:attr="data-role=${role}" and the hidden CSRF/userId inputs are rendered, or keep it static and remove all th:* usages and instead populate data-role, incidentIdInput, userId and CSRF token in JS at runtime (e.g., read a meta tag or an authenticated API). Also stop posting the form directly to the Thymeleaf controller at "/comments": change the form to submit via fetch to a REST endpoint (e.g., /api/comments) with a JSON body and include the CSRF token in the header; update getIncidentIdFromUrl() to read the ID from URLSearchParams if you switch to query-string navigation (viewincident.html?id=5), or keep path routing only if you add the controller. Ensure references: getIncidentIdFromUrl(), body th:attr="data-role=${role}", the hidden inputs (incidentIdInput, userId, CSRF parameterName), and the form action "/comments" are updated accordingly.src/main/java/org/example/team6backend/document/controller/DocumentController.java (1)
79-93:⚠️ Potential issue | 🔴 CriticalMissing authorization check — IDOR on file download.
Unlike
getFile(lines 48-51),downloadFileresolves the document byfileKeyand streams it without verifying that the requesting user has access to the parent incident. Any authenticated principal (includingPENDINGusers, and anyone who learns or guesses a fileKey) can retrieve any stored document. This is a direct-object-reference authorization bypass.🔒 Proposed fix — mirror the incident access check from getFile
`@GetMapping`("/{fileKey}/download") public ResponseEntity<InputStreamResource> downloadFile(`@PathVariable` String fileKey, `@AuthenticationPrincipal` CustomUserDetails userDetails) { AppUser user = userDetails.getUser(); Document document = documentService.getByFileKey(fileKey) .orElseThrow(() -> new ResponseStatusException(HttpStatus.NOT_FOUND)); + Incident incident = incidentService.getById(document.getIncident().getId(), user); + if (incident == null) { + throw new ResponseStatusException(HttpStatus.NOT_FOUND); + } + InputStream stream = minioService.getFile(fileKey); return ResponseEntity.ok() .header(HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=\"" + document.getFileName() + "\"") .contentType(MediaType.parseMediaType(document.getContentType())).body(new InputStreamResource(stream)); }Also consider extracting the document-resolve + incident-authorize logic into a shared private helper since both endpoints need it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/team6backend/document/controller/DocumentController.java` around lines 79 - 93, The downloadFile endpoint currently streams a file without verifying the requesting user's access; update downloadFile to mirror the incident-access check used by getFile: after resolving the Document via documentService.getByFileKey(fileKey) and before calling minioService.getFile(fileKey), verify that userDetails.getUser() is authorized for the Document's parent incident (the same logic or service call used in getFile) and throw a ResponseStatusException(HttpStatus.FORBIDDEN) when unauthorized; to avoid duplication, extract the "resolve document + authorize incident" sequence into a shared private helper (used by both getFile and downloadFile).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/team6backend/config/SecurityConfig.java`:
- Around line 23-33: SecurityConfig: update the URL matchers and CSRF ignores to
cover the missing subpaths and document upload API and ensure extensionless
routes are protected; change exact matchers like
requestMatchers("/incidents.html", "/profile.html") to include subpaths (e.g.
"/incidents.html/**", "/admin.html/**" where applicable) so requests like
"/incidents.html/{id}" and admin subpaths are role-restricted, add
"/api/documents/**" to the csrf(...).ignoringRequestMatchers list to avoid CSRF
failures for POST /api/documents/upload/{incidentId}, and either add equivalent
requestMatchers for the extensionless routes ("/dashboard", "/incidents",
"/admin", "/create-incident") with the same role rules or coordinate callers to
use the .html paths so role checks remain consistent.
In `@src/main/resources/static/incidents.html`:
- Around line 345-347: The client-side navigation in function viewIncident uses
a path `/incidents.html/${id}` that 404s; update viewIncident to navigate to the
static detail page with a query param (e.g., `viewincident.html?id=${id}`) and
then read the id via URLSearchParams in viewincident.html, and apply the same
change to the two identical navigations in dashboard.html (the occurrences
around the functions that call viewIncident/navigation) so the detail page is
reachable; alternatively, if you prefer server-side routing, add a controller
mapping `/incidents/{id}` → forward to the static `viewincident.html` and keep
the original `/incidents/{id}` navigation, but pick one approach and make the
corresponding code changes to the function viewIncident and the dashboard
navigation callers.
---
Outside diff comments:
In
`@src/main/java/org/example/team6backend/document/controller/DocumentController.java`:
- Around line 79-93: The downloadFile endpoint currently streams a file without
verifying the requesting user's access; update downloadFile to mirror the
incident-access check used by getFile: after resolving the Document via
documentService.getByFileKey(fileKey) and before calling
minioService.getFile(fileKey), verify that userDetails.getUser() is authorized
for the Document's parent incident (the same logic or service call used in
getFile) and throw a ResponseStatusException(HttpStatus.FORBIDDEN) when
unauthorized; to avoid duplication, extract the "resolve document + authorize
incident" sequence into a shared private helper (used by both getFile and
downloadFile).
In `@src/main/resources/static/admin.html`:
- Around line 778-782: The navbar still contains Thymeleaf attributes that won't
be processed in this static admin.html; replace the inert th:text usage by
adding client-side code to fetch the current user (GET /api/users/me) and then
set the DOM textContent for the .user-name element (use user.name ||
user.githubLogin) and the .role-badge element (use user.role or response.role),
keeping the existing logout() button behavior; locate the .user-name and
.role-badge nodes in the script that initializes the page (or add a small init
function) and update them after the fetch, handling errors by leaving the
fallback text unchanged or logging to console.
In `@src/main/resources/static/createincident.html`:
- Around line 93-139: The form in createincident.html is broken because it
contains Thymeleaf attributes that are not processed in static resources;
replace the template directives by converting the form to plain HTML with
explicit name attributes (e.g., inputs for subject, description,
incidentCategory, files) and remove th:* attributes, then implement a small JS
module that (1) retrieves a CSRF token (via a meta tag or a GET to /api/csrf),
(2) posts JSON to the REST create endpoint (POST /api/incidents) to create the
incident, (3) on success uploads files via multipart to POST
/api/documents/upload/{incidentId}, and (4) displays validation/server errors in
the DOM instead of using `#fields.hasErrors`; apply the same conversion pattern to
profile.html, admin.html, dashboard.html, viewincident.html, and incidents.html
so no th:* attributes remain in static files.
In `@src/main/resources/static/dashboard.html`:
- Around line 693-712: Update the hard-coded hrefs to use the .html routes so
they match the new pages and SecurityConfig matchers: change the anchor with
text "Manage all →" (inside the pending users card), and the three action
buttons "Create new incident", "Manage incidents", and "Go to Admin Panel"
(inside the div with id "actionButtons") to point to /admin.html,
/create-incident.html, /incidents.html, and /admin.html respectively; leave the
th:if expressions as-is.
- Around line 594-714: The dashboard currently relies on Thymeleaf-only
attributes (th:*) in static/dashboard.html so client-side code never gets
role/user info; replace that by fetching the current user from your REST user
endpoint (e.g., GET /api/me) in the DOMContentLoaded handler, then set
document.body.dataset.role, dataset.userId and dataset.userActive from the JSON
response and update DOM nodes (querySelector('.user-name'),
querySelector('.role-badge'), `#totalIncidents`, `#openIncidents`,
`#resolvedIncidents`, `#pendingUsersCard`, `#incidentsList`, `#actionButtons`,
`#dropdownUsersList`, etc.) to show/hide content based on role/active state;
finally ensure after setting these datasets you call loadIncidents() and
loadPendingUsers() as needed so stats/lists populate.
In `@src/main/resources/static/incidents.html`:
- Around line 173-199: The page reads role from document.body.dataset.role (set
via th:attr="data-role=${role}") but since the HTML is now static role is
undefined, causing loadIncidents() to always hit the unknown-role branch; update
loadIncidents (or an init function called before it) to retrieve the current
user's role from the backend (e.g., call a /api/users/me or /api/auth/role
endpoint) and set a local variable (role) before the existing branching logic,
or alternatively read role from a backend-set cookie/header and use that value
instead of document.body.dataset.role; ensure you reference and update the same
symbol loadIncidents and remove reliance on document.body.dataset.role so the
ADMIN/HANDLER/RESIDENT branches work correctly.
In `@src/main/resources/static/profile.html`:
- Around line 213-218: Update the hardcoded anchor hrefs in profile.html so they
point to the new .html pages: change the Incidents link (the <a th:if="${role !=
'PENDING'}" href="/incidents.html">) and the Profile link (currently
href="/profile") to use /incidents.html and /profile.html respectively, and also
update the Admin link inside the <div th:if="${role == 'ADMIN'}"> to
href="/admin.html" so these anchors match the SecurityConfig .html matchers and
avoid 404s; keep the existing th:if conditions unchanged.
- Around line 1-295: The file still uses Thymeleaf directives but now lives in
static/—replace server-side rendering with client-side rendering: in the
existing DOMContentLoaded handler (document.addEventListener("DOMContentLoaded",
...)) fetch the current user from /api/users/me, then populate DOM nodes by
id/class: set .user-name text, .info-value nodes for display
name/email/githubLogin, set the profile-avatar img src or create the
avatar-placeholder text, set .role-badge text and add the role class
(role.toLowerCase()), show/hide the Admin nav link and Incidents link based on
role, and write the formatted createdAt string into `#accountCreated` using the
existing formatDate function; ensure logout() stays intact and remove/ignore all
th:* attributes so the page works purely from the fetched JSON.
In `@src/main/resources/static/viewincident.html`:
- Around line 215-243: The document links are using "/documents/" but the
controller was remapped to "/api/documents"; update the construction of fileUrl
in the incident.documents.map block (where fileUrl = "/documents/" +
encodeURIComponent(doc.fileKey)) to use "/api/documents/" as the base so both
the anchor href and img src point to "/api/documents/{encodedKey}"; keep the
existing encodeURIComponent, isImage detection, and escapeHtml(doc.fileName)
logic intact so only the base path is changed to match DocumentController's
`@RequestMapping`("/api/documents").
- Around line 145-183: The page is unreachable and Thymeleaf expressions are not
processed because viewincident.html is served as a static file and
getIncidentIdFromUrl() assumes a path-style ID; either move this file into
Thymeleaf templates and expose a controller for /incidents/{id} so body
th:attr="data-role=${role}" and the hidden CSRF/userId inputs are rendered, or
keep it static and remove all th:* usages and instead populate data-role,
incidentIdInput, userId and CSRF token in JS at runtime (e.g., read a meta tag
or an authenticated API). Also stop posting the form directly to the Thymeleaf
controller at "/comments": change the form to submit via fetch to a REST
endpoint (e.g., /api/comments) with a JSON body and include the CSRF token in
the header; update getIncidentIdFromUrl() to read the ID from URLSearchParams if
you switch to query-string navigation (viewincident.html?id=5), or keep path
routing only if you add the controller. Ensure references:
getIncidentIdFromUrl(), body th:attr="data-role=${role}", the hidden inputs
(incidentIdInput, userId, CSRF parameterName), and the form action "/comments"
are updated accordingly.
🪄 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: 2d177e49-de86-42ba-a7d9-156cad111eab
📒 Files selected for processing (10)
src/main/java/org/example/team6backend/config/SecurityConfig.javasrc/main/java/org/example/team6backend/document/controller/DocumentController.javasrc/main/java/org/example/team6backend/page/PageController.javasrc/main/resources/static/admin.htmlsrc/main/resources/static/createincident.htmlsrc/main/resources/static/dashboard.htmlsrc/main/resources/static/incidents.htmlsrc/main/resources/static/index.htmlsrc/main/resources/static/profile.htmlsrc/main/resources/static/viewincident.html
💤 Files with no reviewable changes (1)
- src/main/java/org/example/team6backend/page/PageController.java
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/java/org/example/team6backend/document/controller/DocumentController.java (2)
68-81:⚠️ Potential issue | 🟡 Minor
uploadFilemissing null-check onuserDetails.
getFilewas hardened against a missing principal (lines 43‑45), butuploadFilestill callsuserDetails.getUser()directly on line 72. If Spring Security ever resolves an anonymous/no principal here (e.g., an endpoint-level permit-all rule, or a misconfiguration), this produces an opaque 500 NPE instead of a clean 401. Mirror the check fromgetFilefor consistency.🛠️ Suggested change
public ResponseEntity<Void> uploadFile(`@PathVariable` Long incidentId, `@RequestParam`("files") List<MultipartFile> files, `@AuthenticationPrincipal` CustomUserDetails userDetails) { + if (userDetails == null) { + throw new ResponseStatusException(HttpStatus.UNAUTHORIZED); + } AppUser user = userDetails.getUser();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/team6backend/document/controller/DocumentController.java` around lines 68 - 81, The uploadFile method calls userDetails.getUser() without checking for a null principal; mirror the null-check used in getFile by first verifying CustomUserDetails userDetails is not null and returning a 401/Unauthorized (or throwing the same AuthenticationException used in getFile) when it is null, before calling userDetails.getUser() and proceeding to call incidentService.getById and documentService.uploadFile.
83-97:⚠️ Potential issue | 🔴 CriticalAuthorization bypass:
/{fileKey}/downloaddoesn't verify the caller owns the incident.Compared to
getFile(lines 49‑55), this endpoint:
- Does not null-check
userDetails(will NPE atuserDetails.getUser()if the Security filter is ever relaxed for this route).- Fetches
useron line 86 but then never uses it to validate access — there is noincidentService.getById(document.getIncident().getId(), user)check. Any authenticated user who knows (or guesses) afileKeycan download any document belonging to any incident. This is an authorization bypass that contradicts the authorization model used by the sibling inline endpoint.MediaType.parseMediaType(document.getContentType())will throwInvalidMediaTypeExceptionifcontentTypeis null —getFilealready guards this (lines 59‑61) butdownloadFiledoes not.Given the two handlers only differ by
Content-Disposition(inlinevsattachment), consider collapsing them or extracting a private helper so the authorization and null-safety logic can’t drift again.🛠️ Suggested minimum fix
`@GetMapping`("/{fileKey}/download") public ResponseEntity<InputStreamResource> downloadFile(`@PathVariable` String fileKey, `@AuthenticationPrincipal` CustomUserDetails userDetails) { - AppUser user = userDetails.getUser(); - - Document document = documentService.getByFileKey(fileKey) - .orElseThrow(() -> new ResponseStatusException(HttpStatus.NOT_FOUND)); - - InputStream stream = minioService.getFile(fileKey); - - return ResponseEntity.ok() - .header(HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=\"" + document.getFileName() + "\"") - .contentType(MediaType.parseMediaType(document.getContentType())).body(new InputStreamResource(stream)); - + if (userDetails == null) { + throw new ResponseStatusException(HttpStatus.UNAUTHORIZED); + } + AppUser user = userDetails.getUser(); + + Document document = documentService.getByFileKey(fileKey) + .orElseThrow(() -> new ResponseStatusException(HttpStatus.NOT_FOUND)); + + Incident incident = incidentService.getById(document.getIncident().getId(), user); + if (incident == null) { + throw new ResponseStatusException(HttpStatus.NOT_FOUND); + } + + MediaType mediaType = document.getContentType() != null + ? MediaType.parseMediaType(document.getContentType()) + : MediaType.APPLICATION_OCTET_STREAM; + + InputStream stream = minioService.getFile(fileKey); + return ResponseEntity.ok() + .header(HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=\"" + document.getFileName() + "\"") + .contentType(mediaType) + .body(new InputStreamResource(stream)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/team6backend/document/controller/DocumentController.java` around lines 83 - 97, downloadFile in DocumentController currently dereferences userDetails without null-check, never validates the caller owns the incident, and can pass a null contentType into MediaType.parseMediaType; update downloadFile to mirror getFile: null-check userDetails before calling userDetails.getUser(), fetch the document via documentService.getByFileKey(fileKey), then call incidentService.getById(document.getIncident().getId(), user) (or the same ownership-check helper used by getFile) to enforce authorization, and guard against null document.getContentType() before calling MediaType.parseMediaType (use a default or omit contentType header if null). Consider extracting a private helper used by getFile and downloadFile to return the validated Document/MediaType/InputStreamResource to avoid divergence.src/main/java/org/example/team6backend/document/service/MinioService.java (1)
56-62:⚠️ Potential issue | 🟡 MinorUse a proper logger and reconsider silently swallowing MinIO deletion failures.
Two concerns on the new behavior:
System.err.printlnshould be replaced with an SLF4J logger (the rest of the codebase uses@Slf4j, e.g.DocumentService).System.errbypasses log formatting, levels, MDC, and log aggregation.- Unconditionally suppressing the exception turns a previously loud failure into a silent one. The caller
IncidentService(lines 94‑102 ofIncidentService.java) already wrapsdeleteFilein try/catch expecting exceptions during rollback cleanup — now that branch will never log anything useful and rollback failures become invisible. Consider keeping the exception-throwing contract (and letting callers decide), or at minimum log atWARN/ERRORvia SLF4J with the stack trace.🛠️ Suggested change
+import lombok.extern.slf4j.Slf4j; ... +@Slf4j `@Service` `@RequiredArgsConstructor` public class MinioService { ... public void deleteFile(String fileKey) { try { minioClient.removeObject(RemoveObjectArgs.builder().bucket(bucketName).object(fileKey).build()); } catch (Exception e) { - System.err.println("MinIO delete failed for " + fileKey + ": " + e.getMessage()); + log.warn("MinIO delete failed for fileKey={}", fileKey, e); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/team6backend/document/service/MinioService.java` around lines 56 - 62, The deleteFile method currently swallows exceptions using System.err; replace that with SLF4J logging and preserve the exception contract so callers (like IncidentService) can handle rollback failures. Add/ensure `@Slf4j` on MinioService, change the catch to log.error("MinIO delete failed for {}", fileKey, e) (include the stack trace) and then rethrow the exception (or wrap and throw a RuntimeException) instead of silently returning; alternatively, remove the try/catch so the original exception propagates.
🧹 Nitpick comments (1)
src/test/java/org/example/team6backend/document/controller/DocumentControllerTest.java (1)
62-88: Consider adding coverage for the new/{fileKey}/downloadendpoint and the 401 path.The existing tests exercise
GET /api/documents/{fileKey}(inline response) and the 404 path, which align with the controller changes. However, the PR introduces:
- A new
GET /api/documents/{fileKey}/downloadendpoint (attachment) which is completely untested.- An explicit
ResponseStatusException(HttpStatus.UNAUTHORIZED)whenuserDetails == nullingetFile, which isn’t exercised (the@BeforeEachalways installs an authenticated principal).Adding tests for both would lock in the new REST semantics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/team6backend/document/controller/DocumentControllerTest.java` around lines 62 - 88, Add two tests in DocumentControllerTest: one named like getFile_download_shouldReturnAttachment that calls GET "/api/documents/abc/download" and verifies the mocked Document returned by documentService.getByFileKey("abc"), incidentService.getById(1L, ..) and minioService.getFile("abc") produce status 200 and header Content-Disposition equals "attachment; filename=\"test.pdf\"" (mirror the existing getFile test but expect "attachment" instead of "inline"); and another named getFile_shouldReturn401_whenUnauthenticated that invokes the existing GET "/api/documents/abc" without the authenticated principal (use SecurityMockMvcRequestPostProcessors.anonymous() or omit the default authenticated setup used in `@BeforeEach`) and assert status is 401, ensuring documentService.getByFileKey and minioService are not required for that test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/org/example/team6backend/document/controller/DocumentController.java`:
- Around line 24-25: Update viewincident.html to reference the new endpoint by
replacing the hardcoded "/documents/" file URL with "/api/documents/" so
document retrieval uses the new REST path; then in SecurityConfig (the
HttpSecurity configuration method where "/api/incidents/**" is authorized) add
an explicit authorization rule for "/api/documents/**" with the same
role(s)/hasAuthority settings used for "/api/incidents/**" so the endpoint is
matched before anyRequest().authenticated().
In
`@src/main/java/org/example/team6backend/document/service/DocumentService.java`:
- Around line 61-69: The current DocumentService.deleteFile method catches
Throwable and swallows repository failures; change it so the DB delete
(documentRepository.delete(document)) is invoked without a blanket catch so
repository exceptions propagate (retain the `@Transactional` behavior), then call
minioService.deleteFile(document.getFileKey()) inside a separate try/catch that
catches Exception (not Throwable) and logs the MinIO failure; reference
DocumentService.deleteFile, documentRepository.delete, and
minioService.deleteFile, and note that if MinioService.deleteFile already
swallows exceptions you can remove the inner try/catch entirely.
---
Outside diff comments:
In
`@src/main/java/org/example/team6backend/document/controller/DocumentController.java`:
- Around line 68-81: The uploadFile method calls userDetails.getUser() without
checking for a null principal; mirror the null-check used in getFile by first
verifying CustomUserDetails userDetails is not null and returning a
401/Unauthorized (or throwing the same AuthenticationException used in getFile)
when it is null, before calling userDetails.getUser() and proceeding to call
incidentService.getById and documentService.uploadFile.
- Around line 83-97: downloadFile in DocumentController currently dereferences
userDetails without null-check, never validates the caller owns the incident,
and can pass a null contentType into MediaType.parseMediaType; update
downloadFile to mirror getFile: null-check userDetails before calling
userDetails.getUser(), fetch the document via
documentService.getByFileKey(fileKey), then call
incidentService.getById(document.getIncident().getId(), user) (or the same
ownership-check helper used by getFile) to enforce authorization, and guard
against null document.getContentType() before calling MediaType.parseMediaType
(use a default or omit contentType header if null). Consider extracting a
private helper used by getFile and downloadFile to return the validated
Document/MediaType/InputStreamResource to avoid divergence.
In `@src/main/java/org/example/team6backend/document/service/MinioService.java`:
- Around line 56-62: The deleteFile method currently swallows exceptions using
System.err; replace that with SLF4J logging and preserve the exception contract
so callers (like IncidentService) can handle rollback failures. Add/ensure
`@Slf4j` on MinioService, change the catch to log.error("MinIO delete failed for
{}", fileKey, e) (include the stack trace) and then rethrow the exception (or
wrap and throw a RuntimeException) instead of silently returning; alternatively,
remove the try/catch so the original exception propagates.
---
Nitpick comments:
In
`@src/test/java/org/example/team6backend/document/controller/DocumentControllerTest.java`:
- Around line 62-88: Add two tests in DocumentControllerTest: one named like
getFile_download_shouldReturnAttachment that calls GET
"/api/documents/abc/download" and verifies the mocked Document returned by
documentService.getByFileKey("abc"), incidentService.getById(1L, ..) and
minioService.getFile("abc") produce status 200 and header Content-Disposition
equals "attachment; filename=\"test.pdf\"" (mirror the existing getFile test but
expect "attachment" instead of "inline"); and another named
getFile_shouldReturn401_whenUnauthenticated that invokes the existing GET
"/api/documents/abc" without the authenticated principal (use
SecurityMockMvcRequestPostProcessors.anonymous() or omit the default
authenticated setup used in `@BeforeEach`) and assert status is 401, ensuring
documentService.getByFileKey and minioService are not required for that test.
🪄 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: aa73fcd5-5883-4e0a-a578-7bd0f3a63ea3
📒 Files selected for processing (4)
src/main/java/org/example/team6backend/document/controller/DocumentController.javasrc/main/java/org/example/team6backend/document/service/DocumentService.javasrc/main/java/org/example/team6backend/document/service/MinioService.javasrc/test/java/org/example/team6backend/document/controller/DocumentControllerTest.java
This was referenced Apr 23, 2026
Merged
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 by CodeRabbit
New Features
Bug Fixes & Improvements
Removed