Fix spoofing vulnerability (Resolves #15)#75
Open
felipemelozx wants to merge 98 commits intoomatheusmesmo:developfrom
Open
Fix spoofing vulnerability (Resolves #15)#75felipemelozx wants to merge 98 commits intoomatheusmesmo:developfrom
felipemelozx wants to merge 98 commits intoomatheusmesmo:developfrom
Conversation
… caminho relevantes
Co-authored-by: Matheus Oliveira <99829531+omatheusmesmo@users.noreply.github.com>
This exception will be thrown when a user attempts to access or modify resources they don't own, preventing ID spoofing attacks.
Returns HTTP 403 Forbidden when ResourceOwnershipException is thrown, providing clear feedback for unauthorized access attempts.
The owner ID is now obtained from the authenticated user context instead of the request body to prevent ID spoofing vulnerabilities.
Make User entity implement Spring Security's UserDetails interface to enable direct use as authentication principal. This allows @AuthenticationPrincipal to inject the User entity directly in controllers without additional conversion.
Mapper now receives the authenticated user as a parameter instead of extracting from DTO, ensuring the owner is set from security context.
Add findByOwnerIdAndDeletedFalse method to filter lists by owner, supporting ownership-based access control at the data layer.
- Inject authenticated user via @AuthenticationPrincipal annotation - Pass authenticated user to service layer for ownership verification - Prevent ID spoofing by removing reliance on request body IDs
Ensure User entity is properly loaded as UserDetails for @AuthenticationPrincipal injection in controllers.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Owner
|
@felipemelozx , hi! I made a huge change in the develop branch and there is more conflicts, could you please fix again? Thanks. |
Owner
|
Hi @felipemelozx ! A better approach would be update your remote repository and then run the following commands inside your local feature branch. |
Contributor
Author
|
Hi @omatheusmesmo ! I ran the commands but nothing changed in my branch. Do you think it would be better to create a new branch from the latest develop and move the changes there? It might be easier to work from a clean branch. |
Owner
@felipemelozx did you update your remote branch first? |
Contributor
Author
|
Yes, I updated it. |
Owner
Oh, that's weird if 'git status' doesn't show commits to push. You don't need to create a new branch on the remote. You can delete your local branch and recreate it from the updated main. Just be sure to stash your changes to this feature before deleting the branch. In the end, you can run 'git push -f' to update the remote branch. Hope works fine. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/src/main/java/com/omatheusmesmo/shoppmate/auth/configs/RsaKeyConfig.java (1)
26-33:⚠️ Potential issue | 🟠 MajorRestore resource validation and fix resource handling for JWT key loading
Lines 26 and 38 load keys from externally configured
Resourcevalues without validating existence, readability, or scheme. This allows unsafe key origins via environment variable override (e.g.,JWT_PRIVATE_KEY_PATH=https://attacker.com/key.pem), weakening trust boundaries for signing/verification keys. Additionally, theInputStreamcreated bygetInputStream()is never explicitly closed, andnew String(byte[])lacks an explicit charset.🔧 Proposed fix
@@ import org.springframework.beans.factory.annotation.Value; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.core.io.Resource; +import java.nio.charset.StandardCharsets; import java.security.KeyFactory; import java.security.interfaces.RSAPrivateKey; import java.security.interfaces.RSAPublicKey; import java.security.spec.PKCS8EncodedKeySpec; import java.security.spec.X509EncodedKeySpec; import java.util.Base64; @@ `@Bean` public RSAPrivateKey privateKey() throws Exception { + validateResource(privateKeyResource, "jwt.private-key"); - String key = new String(privateKeyResource.getInputStream().readAllBytes()); + String key = new String(privateKeyResource.getInputStream().readAllBytes(), StandardCharsets.UTF_8); key = key.replace("-----BEGIN PRIVATE KEY-----", "").replace("-----END PRIVATE KEY-----", "").replaceAll("\\s", ""); @@ `@Bean` public RSAPublicKey publicKey() throws Exception { + validateResource(publicKeyResource, "jwt.public-key"); - String key = new String(publicKeyResource.getInputStream().readAllBytes()); + String key = new String(publicKeyResource.getInputStream().readAllBytes(), StandardCharsets.UTF_8); key = key.replace("-----BEGIN PUBLIC KEY-----", "").replace("-----END PUBLIC KEY-----", "").replaceAll("\\s", ""); @@ + private void validateResource(Resource resource, String propertyName) throws Exception { + if (!resource.exists() || !resource.isReadable()) { + throw new IllegalStateException("Invalid key resource for " + propertyName); + } + URI uri = resource.getURI(); + String scheme = uri.getScheme(); + if (scheme != null && !scheme.equals("file") && !scheme.equals("classpath")) { + throw new IllegalStateException("Unsupported key resource scheme for " + propertyName + ": " + scheme); + } + } }Also applies to: 38-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/com/omatheusmesmo/shoppmate/auth/configs/RsaKeyConfig.java` around lines 26 - 33, In RsaKeyConfig, validate and safely load the RSA keys: before reading privateKeyResource (and likewise publicKeyResource), check resource.exists() and resource.isReadable(), and reject or throw if the resource URL has an untrusted remote scheme (e.g., only allow "file" or "classpath" schemes) to prevent loading from attacker-controlled URLs; read the stream using a try-with-resources (or explicitly close the InputStream) and construct the String with an explicit charset (StandardCharsets.UTF_8) instead of new String(byte[]), then proceed to Base64-decode and build the PKCS8EncodedKeySpec; apply the same validation/stream-closing/charset fixes to the public key loading path as well.README.md (1)
74-100:⚠️ Potential issue | 🟡 MinorDuplicate and mislabeled sections in manual setup instructions.
There appear to be merge artifacts or copy-paste errors:
- Lines 74-79: Labeled "Frontend" but contain Java/Maven backend commands (should likely be removed or correctly labeled)
- Lines 92-100: Duplicate the npm instructions already present in lines 82-91
📝 Proposed fix to remove duplicates and fix mislabeling
-#### Frontend -2. Ensure you have Java 17 installed. -3. Run with: - ```bash - ./mvnw spring-boot:run - ``` - #### Frontend 1. Navigate to `frontend/`. 2. Install dependencies: ```bash npm install ``` 3. Run with: ```bash npm start ``` 4. Access at [http://localhost:4200](http://localhost:4200). -2. Install dependencies: - ```bash - npm install - ``` -3. Run with: - ```bash - npm start - ``` -4. Access at [http://localhost:4200](http://localhost:4200).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 74 - 100, Remove the duplicate and mislabelled sections in the README by deleting the Maven/backend snippet that is incorrectly under the "Frontend" heading and removing the repeated npm install/start block; ensure there is a single correctly labeled "Backend" or "Server" section containing the Java 17 and ./mvnw spring-boot:run instructions and a single "Frontend" section that lists Navigate to `frontend/`, npm install, npm start, and the http://localhost:4200 URL so the README no longer contains duplicated npm instructions or the misplaced Maven commands.
🧹 Nitpick comments (7)
.github/workflows/gemini-triage.yml (1)
62-62: Consider pinningrun-gemini-clito a specific commit SHA.The action uses
@v0which is a floating tag that may change unexpectedly. While the# ratchet:excludecomment indicates this is intentional, floating tags can introduce breaking changes or security issues without notice.As per coding guidelines: "Use specific action versions (commit SHA or tagged versions)." Consider pinning to a commit SHA once the action stabilizes, or document the rationale for using the floating tag.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/gemini-triage.yml at line 62, The workflow currently references the floating tag 'google-github-actions/run-gemini-cli@v0' (with '# ratchet:exclude') which can change; update the workflow to pin the action to a specific commit SHA or stable tag (or add a short justification comment if you must keep the floating tag) so CI behavior is deterministic; locate the uses line referencing 'google-github-actions/run-gemini-cli@v0' and replace '@v0' with a concrete '@<commit-sha>' or '@<tag>' (or add an explicit rationale near the '# ratchet:exclude' comment).backend/src/test/java/com/omatheusmesmo/shoppmate/service/ItemServiceTest.java (1)
109-127: Please make sure the real query is exercised outside Mockito.These changes only swap the mocked repository method, so this suite still won't catch a broken
findByIdWithRelations(...)implementation or regressions in thedeleted = falsebehavior. If that coverage does not already exist elsewhere, add a repository/integration test for the new lookup.Also applies to: 136-186
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/test/java/com/omatheusmesmo/shoppmate/service/ItemServiceTest.java` around lines 109 - 127, The test ItemServiceTest currently only mocks itemRepository.findByIdWithRelations(...) so it doesn't exercise the real repository query or the deleted = false filter; add an integration/repository test that uses the actual ItemRepository (not Mockito) to persist an Item with deleted=false and another with deleted=true, then call the real repository method findByIdWithRelations(id) (or the JPA query backing it) to assert the deleted=true item is not returned and deleted=false is returned; ensure the test class boots Spring context or uses `@DataJpaTest` and saves entities via ItemRepository before invoking findByIdWithRelations to verify the real query behavior and prevent regressions.backend/src/main/java/com/omatheusmesmo/shoppmate/item/service/ItemService.java (1)
55-57: Separate the read lookup from the write precheck.This method is now doing double duty: read paths need the richer load, but
removeItem()/editItem()only use it to confirm existence, andfindItem(Item)still follows a different lookup path. Splitting those responsibilities would keep lookup semantics consistent and avoid extra work on mutation flows. As per coding guidelines, "You are a Senior Java Architect and Spring Boot expert. You focus on writing high-performance, secure, and maintainable microservices using modern Java 17 patterns."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/com/omatheusmesmo/shoppmate/item/service/ItemService.java` around lines 55 - 57, Split the responsibilities of ItemService.findById into a rich-read method and a lightweight existence check: keep a new read method (e.g., findByIdWithRelations or keep findById but ensure it uses itemRepository.findByIdWithRelations(id) and throws NoSuchElementException) for read paths that need full relations, and add a separate precheck method (e.g., ensureExistsById or existsByIdCheck) that calls a lightweight repository lookup (itemRepository.existsById(id) or itemRepository.findById(id) without loading relations) and throws the same NoSuchElementException for mutation flows; then update mutation methods like removeItem and editItem to call the new precheck method instead of the heavy findByIdWithRelations call so edits/removals don't incur the extra relation loading.backend/src/main/java/com/omatheusmesmo/shoppmate/list/controller/ShoppingListController.java (1)
22-22: Remove stale TODO comment.This TODO has been addressed by the changes in this PR—
getAllShoppingLists(line 39) now callsservice.findAllByUser(user), which returns only lists owned by or shared with the authenticated user.♻️ Proposed fix
-// TODO: return only lists owned by the user or that are shared with him `@RestController` `@RequestMapping`("/lists")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/com/omatheusmesmo/shoppmate/list/controller/ShoppingListController.java` at line 22, Remove the stale TODO comment in ShoppingListController.java; the functionality is already implemented as getAllShoppingLists now calls service.findAllByUser(user) which returns only lists owned by or shared with the authenticated user, so delete the "// TODO: return only lists owned by the user or that are shared with him" line to avoid confusion.backend/src/main/java/com/omatheusmesmo/shoppmate/list/service/ListPermissionService.java (3)
22-30: Consider making injected dependenciesfinal.The coding guidelines specify constructor injection with
finalfields. These fields are assigned only in the constructor and should be markedfinalto ensure immutability and clearly express intent.♻️ Proposed fix
- private final ListPermissionRepository listPermissionRepository; + private final ListPermissionRepository listPermissionRepository; - private final ShoppingListService shoppingListService; + private final ShoppingListService shoppingListService; - private final UserService userService; + private final UserService userService; - private final AuditService auditService; + private final AuditService auditService; - private final ListPermissionMapper listPermissionMapper; + private final ListPermissionMapper listPermissionMapper;Note: The fields are already
final. No change needed here.On closer inspection, the fields are already
final. LGTM on dependency injection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/com/omatheusmesmo/shoppmate/list/service/ListPermissionService.java` around lines 22 - 30, The injected dependencies in ListPermissionService (listPermissionRepository, shoppingListService, userService, auditService, listPermissionMapper) are already declared final, so no code changes are needed; simply confirm the constructor performs proper constructor injection and leave the final fields as-is.
45-48: Extract ownership verification to a helper method.The ownership check pattern is repeated across
addListPermission,findListUserPermissionById, andfindAllPermissionsByListId. Extracting this to a private helper would improve maintainability and provide a single place for potential null-safety improvements.♻️ Proposed helper method
private void verifyOwnership(ShoppingList list, User user, String message) { if (!list.getOwner().getId().equals(user.getId())) { throw new ResourceOwnershipException(message); } }Usage example:
-if (!shoppingList.getOwner().getId().equals(requester.getId())) { - throw new ResourceOwnershipException( - "User does not have permission to grant access to this list. Only the list owner can grant permissions."); -} +verifyOwnership(shoppingList, requester, + "User does not have permission to grant access to this list. Only the list owner can grant permissions.");Also applies to: 78-80, 103-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/com/omatheusmesmo/shoppmate/list/service/ListPermissionService.java` around lines 45 - 48, Extract the repeated ownership check in ListPermissionService into a private helper method verifyOwnership(ShoppingList list, User user, String message) that throws ResourceOwnershipException when list.getOwner().getId().equals(user.getId()) is false; replace the inline checks in addListPermission, findListUserPermissionById, and findAllPermissionsByListId with calls to verifyOwnership(shoppingList, requester, "<same message as original>") to centralize null-safety and error messaging (ensure you call getOwner() and getId() safely as needed inside the helper).
100-108: Consider simplifying by removing redundant access check.
findAndVerifyAccess(line 101) verifies the user has access (owner OR shared), but lines 103-105 then reject non-owners anyway. Since only owners can proceed, the initial access check is redundant—you could call a simplefindListByIdinstead.However, if
findAndVerifyAccessalso verifies the list exists and isn't deleted, keeping it might be intentional for consistency.♻️ Potential simplification
public List<ListPermission> findAllPermissionsByListId(Long id, User user) { - ShoppingList list = shoppingListService.findAndVerifyAccess(id, user); - - if (!list.getOwner().getId().equals(user.getId())) { - throw new ResourceOwnershipException("Only the list owner can view permissions"); - } + ShoppingList list = shoppingListService.findListById(id); + verifyOwnership(list, user, "Only the list owner can view permissions"); return listPermissionRepository.findByShoppingListIdAndDeletedFalse(id); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/com/omatheusmesmo/shoppmate/list/service/ListPermissionService.java` around lines 100 - 108, The method findAllPermissionsByListId currently calls shoppingListService.findAndVerifyAccess and then enforces owner-only access again, which is redundant; fix by either (A) replace the call to findAndVerifyAccess with a simpler existence check such as shoppingListService.findListById (or equivalent findById that verifies existence/deleted) and keep the owner-only check and the ResourceOwnershipException, or (B) keep the call to findAndVerifyAccess and remove the subsequent owner-only check lines (the throw) so ownership isn’t redundantly enforced. Update findAllPermissionsByListId in ListPermissionService and adjust the call to shoppingListService accordingly (use findListById or remove the extra check) so behavior is consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/gemini-dispatch.yml:
- Around line 125-165: The review and invoke jobs reference missing reusable
workflow files causing runtime failures; either add the two workflows named ./
.github/workflows/gemini-review.yml and ./.github/workflows/gemini-invoke.yml
implementing the expected inputs/permissions/outputs used by the review and
invoke jobs, or update the review and invoke job 'uses' values to point to
existing reusable workflows; locate the job definitions named review and invoke
in the gemini-dispatch.yml and ensure the referenced workflow filenames and
required input/secret contracts match the created workflows (also verify triage
uses ./ .github/workflows/gemini-triage.yml if intended).
In @.github/workflows/gemini-triage.yml:
- Around line 23-25: The selected_labels output is empty because
env.SELECTED_LABELS is never set; update the triage job to extract labels from
the Gemini CLI result (e.g., steps.gemini_analysis.outputs.summary or
steps.get_labels.outputs.*) and set a step output or env var that the job
outputs can reference; specifically, add a step after run-gemini-cli that parses
steps.gemini_analysis.outputs.summary into a step output like selected_labels
(or sets an env var SELECTED_LABELS) and then change outputs.selected_labels to
reference that step output (instead of env.SELECTED_LABELS) so the downstream
label job condition can evaluate correctly.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/auth/configs/RsaKeyConfig.java`:
- Around line 26-27: The PEM loading currently calls
privateKeyResource.getInputStream().readAllBytes() (and similarly
publicKeyResource.getInputStream()) without closing the stream and converts
bytes to String using the platform default charset; change both to use
try-with-resources to open the InputStream and readAllBytes inside the try
block, and convert bytes to String with StandardCharsets.US_ASCII (e.g. new
String(bytes, StandardCharsets.US_ASCII)) before stripping "-----BEGIN/END
PRIVATE/ PUBLIC KEY-----" and whitespace so streams are closed deterministically
and parsing uses an explicit charset; update the code paths that build the key
String (the variables computed from privateKeyResource and publicKeyResource)
accordingly.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/auth/service/CustomUserDetailsService.java`:
- Around line 20-21: The current orElseThrow in CustomUserDetailsService returns
a UsernameNotFoundException that echoes the submitted email; change the thrown
message to a generic, non-PII string (e.g., "User not found" or "Invalid
credentials") so the repository lookup in userRepository.findByEmail(email) no
longer leaks the email; update the exception construction in the method (the
lambda passed to orElseThrow) to use the generic message while keeping the same
exception type UsernameNotFoundException.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/list/controller/ListItemController.java`:
- Around line 37-40: The controller currently ignores the path-level listId for
single-item operations; update ListItemController handlers (getListItemById,
createListItem, updateListItem, deleteListItem) to accept `@PathVariable` Long
listId and pass it to service methods (e.g., change calls to
service.findListItemById(id, user) -> service.findListItemById(listId, id, user)
or add new service overloads that accept listId) and validate that
requestDTO.listId() (for POST/PUT) matches the path listId, returning a 400/403
on mismatch; also update the service implementations to enforce the parent
listId check and add a regression test that sends a request with mismatched path
listId vs body listId to ensure the controller rejects it.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/list/controller/ListPermissionController.java`:
- Around line 47-49: Ensure the controller enforces the nested resource by
accepting the path variable listId in ListPermissionController methods (e.g.,
addListPermission and the other permission mutation endpoints) and passing that
listId into the service (e.g., service.addListPermission(..., listId)); inside
the controller validate that requestDTO.idList() (or the target permission's
list id for PUT/DELETE) matches the path listId and reject mismatches with an
appropriate 4xx response, and update the corresponding service signatures to
accept the explicit listId so the service can also enforce/validate parent/child
consistency.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/list/service/ShoppingListService.java`:
- Around line 74-76: The editList method currently uses the generic
findAndVerifyAccess(ShoppingList.getId(), currentLoggedUser) which allows
READ-only collaborators to perform mutations; change this to a mutation-specific
authorization check (e.g., findAndVerifyWriteAccess or verifyOwner) before
performing any save or delete operations in editList and the similar blocks at
lines 92-95; update or add a new helper (referenced by name in ListItemService
usage) that asserts WRITE or OWNER permissions and call that helper instead of
the generic read check so only users with write/owner rights can rename lists or
modify items.
In `@backend/src/main/java/com/omatheusmesmo/shoppmate/user/entity/User.java`:
- Around line 15-16: In User.java replace the use of
Collections.singletonList(...) with List.of(...) where authorities are created
(e.g., in the getAuthorities / getRoles logic that currently calls
Collections.singletonList(new SimpleGrantedAuthority(role))); update that
expression to List.of(new SimpleGrantedAuthority(role)) and ensure the import
for java.util.List is present; optionally, if you intend to use role-based
checks later, prepend "ROLE_" to the authority string when constructing
SimpleGrantedAuthority (e.g., "ROLE_" + role) so future hasRole(...) checks work
as expected.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/utils/exception/GlobalExceptionHandler.java`:
- Around line 105-109: The handler handleResourceOwnership currently logs and
returns ex.getMessage(), which may expose PII; change it to avoid returning or
logging raw exception details: update the log.warn call to a generic message
like "Resource ownership violation detected" (optionally log a non-sensitive
error id) and construct the ApiError with a generic message (e.g., "Access
Denied" or "You do not have permission to access this resource") without
including ex.getMessage(); keep ResourceOwnershipException for control flow but
do not propagate its message to logs or the ResponseEntity.
In
`@backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ListPermissionControllerSecurityIntegrationTest.java`:
- Around line 149-156: The test duplicates the "forbidden" scenario by posting
to ownerList with tokenUserA; instead exercise the owner-grant path: in
ListPermissionControllerSecurityIntegrationTest update the request to
authenticate as the list owner (use the owner token variable, e.g., tokenOwner
or whatever represents the owner) when posting maliciousDTO for ownerList and
change the expectation from status().isForbidden() to the successful owner
outcome (e.g., status().isOk() and appropriate json assertions) so the
owner-can-grant-on-their-own-list path is covered.
In `@frontend/src/app/shared/components/list-item/list-item.component.html`:
- Around line 113-116: The template renders the same subtitle twice; remove the
duplicated interpolation block that repeats "{{ listItem.quantity }} {{
listItem.item.unit.symbol }} x {{ listItem.unitPrice | currency }} = {{
listItem.totalPrice | currency }} | Category: {{ listItem.item.category.name }}"
so only the original single instance remains in list-item.component.html
(referencing the listItem template variable within the ListItemComponent).
Delete the redundant block (the second occurrence) and verify screen-reader
output still conveys the same info or add an ARIA attribute on the remaining
element if needed to preserve accessibility.
In `@frontend/src/app/shared/services/shopping-list.service.ts`:
- Around line 12-15: Remove the duplicated import of ListItemRequestDTO and
ListItemResponseDTO in shopping-list.service.ts by deleting the redundant import
block so only the original import remains; ensure the remaining import statement
(the one that imports ListItemRequestDTO and ListItemResponseDTO) is kept and
properly grouped with other imports to avoid TypeScript/lint errors.
---
Outside diff comments:
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/auth/configs/RsaKeyConfig.java`:
- Around line 26-33: In RsaKeyConfig, validate and safely load the RSA keys:
before reading privateKeyResource (and likewise publicKeyResource), check
resource.exists() and resource.isReadable(), and reject or throw if the resource
URL has an untrusted remote scheme (e.g., only allow "file" or "classpath"
schemes) to prevent loading from attacker-controlled URLs; read the stream using
a try-with-resources (or explicitly close the InputStream) and construct the
String with an explicit charset (StandardCharsets.UTF_8) instead of new
String(byte[]), then proceed to Base64-decode and build the PKCS8EncodedKeySpec;
apply the same validation/stream-closing/charset fixes to the public key loading
path as well.
In `@README.md`:
- Around line 74-100: Remove the duplicate and mislabelled sections in the
README by deleting the Maven/backend snippet that is incorrectly under the
"Frontend" heading and removing the repeated npm install/start block; ensure
there is a single correctly labeled "Backend" or "Server" section containing the
Java 17 and ./mvnw spring-boot:run instructions and a single "Frontend" section
that lists Navigate to `frontend/`, npm install, npm start, and the
http://localhost:4200 URL so the README no longer contains duplicated npm
instructions or the misplaced Maven commands.
---
Nitpick comments:
In @.github/workflows/gemini-triage.yml:
- Line 62: The workflow currently references the floating tag
'google-github-actions/run-gemini-cli@v0' (with '# ratchet:exclude') which can
change; update the workflow to pin the action to a specific commit SHA or stable
tag (or add a short justification comment if you must keep the floating tag) so
CI behavior is deterministic; locate the uses line referencing
'google-github-actions/run-gemini-cli@v0' and replace '@v0' with a concrete
'@<commit-sha>' or '@<tag>' (or add an explicit rationale near the '#
ratchet:exclude' comment).
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/item/service/ItemService.java`:
- Around line 55-57: Split the responsibilities of ItemService.findById into a
rich-read method and a lightweight existence check: keep a new read method
(e.g., findByIdWithRelations or keep findById but ensure it uses
itemRepository.findByIdWithRelations(id) and throws NoSuchElementException) for
read paths that need full relations, and add a separate precheck method (e.g.,
ensureExistsById or existsByIdCheck) that calls a lightweight repository lookup
(itemRepository.existsById(id) or itemRepository.findById(id) without loading
relations) and throws the same NoSuchElementException for mutation flows; then
update mutation methods like removeItem and editItem to call the new precheck
method instead of the heavy findByIdWithRelations call so edits/removals don't
incur the extra relation loading.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/list/controller/ShoppingListController.java`:
- Line 22: Remove the stale TODO comment in ShoppingListController.java; the
functionality is already implemented as getAllShoppingLists now calls
service.findAllByUser(user) which returns only lists owned by or shared with the
authenticated user, so delete the "// TODO: return only lists owned by the user
or that are shared with him" line to avoid confusion.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/list/service/ListPermissionService.java`:
- Around line 22-30: The injected dependencies in ListPermissionService
(listPermissionRepository, shoppingListService, userService, auditService,
listPermissionMapper) are already declared final, so no code changes are needed;
simply confirm the constructor performs proper constructor injection and leave
the final fields as-is.
- Around line 45-48: Extract the repeated ownership check in
ListPermissionService into a private helper method verifyOwnership(ShoppingList
list, User user, String message) that throws ResourceOwnershipException when
list.getOwner().getId().equals(user.getId()) is false; replace the inline checks
in addListPermission, findListUserPermissionById, and findAllPermissionsByListId
with calls to verifyOwnership(shoppingList, requester, "<same message as
original>") to centralize null-safety and error messaging (ensure you call
getOwner() and getId() safely as needed inside the helper).
- Around line 100-108: The method findAllPermissionsByListId currently calls
shoppingListService.findAndVerifyAccess and then enforces owner-only access
again, which is redundant; fix by either (A) replace the call to
findAndVerifyAccess with a simpler existence check such as
shoppingListService.findListById (or equivalent findById that verifies
existence/deleted) and keep the owner-only check and the
ResourceOwnershipException, or (B) keep the call to findAndVerifyAccess and
remove the subsequent owner-only check lines (the throw) so ownership isn’t
redundantly enforced. Update findAllPermissionsByListId in ListPermissionService
and adjust the call to shoppingListService accordingly (use findListById or
remove the extra check) so behavior is consistent.
In
`@backend/src/test/java/com/omatheusmesmo/shoppmate/service/ItemServiceTest.java`:
- Around line 109-127: The test ItemServiceTest currently only mocks
itemRepository.findByIdWithRelations(...) so it doesn't exercise the real
repository query or the deleted = false filter; add an integration/repository
test that uses the actual ItemRepository (not Mockito) to persist an Item with
deleted=false and another with deleted=true, then call the real repository
method findByIdWithRelations(id) (or the JPA query backing it) to assert the
deleted=true item is not returned and deleted=false is returned; ensure the test
class boots Spring context or uses `@DataJpaTest` and saves entities via
ItemRepository before invoking findByIdWithRelations to verify the real query
behavior and prevent regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e817ed5f-9043-40d2-8db7-7fbd3a03d7fb
📒 Files selected for processing (36)
.github/workflows/gemini-dispatch.yml.github/workflows/gemini-triage.ymlREADME.mdbackend/src/main/java/com/omatheusmesmo/shoppmate/auth/configs/RsaKeyConfig.javabackend/src/main/java/com/omatheusmesmo/shoppmate/auth/service/CustomUserDetailsService.javabackend/src/main/java/com/omatheusmesmo/shoppmate/item/repository/ItemRepository.javabackend/src/main/java/com/omatheusmesmo/shoppmate/item/service/ItemService.javabackend/src/main/java/com/omatheusmesmo/shoppmate/list/controller/ListItemController.javabackend/src/main/java/com/omatheusmesmo/shoppmate/list/controller/ListPermissionController.javabackend/src/main/java/com/omatheusmesmo/shoppmate/list/controller/ShoppingListController.javabackend/src/main/java/com/omatheusmesmo/shoppmate/list/dtos/ShoppingListRequestDTO.javabackend/src/main/java/com/omatheusmesmo/shoppmate/list/entity/ShoppingList.javabackend/src/main/java/com/omatheusmesmo/shoppmate/list/mapper/ListItemMapper.javabackend/src/main/java/com/omatheusmesmo/shoppmate/list/mapper/ListMapper.javabackend/src/main/java/com/omatheusmesmo/shoppmate/list/repository/ShoppingListRepository.javabackend/src/main/java/com/omatheusmesmo/shoppmate/list/service/ListItemService.javabackend/src/main/java/com/omatheusmesmo/shoppmate/list/service/ListPermissionService.javabackend/src/main/java/com/omatheusmesmo/shoppmate/list/service/ShoppingListService.javabackend/src/main/java/com/omatheusmesmo/shoppmate/user/entity/User.javabackend/src/main/java/com/omatheusmesmo/shoppmate/utils/exception/GlobalExceptionHandler.javabackend/src/main/java/com/omatheusmesmo/shoppmate/utils/exception/ResourceOwnershipException.javabackend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ListItemControllerSecurityIntegrationTest.javabackend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ListPermissionControllerSecurityIntegrationTest.javabackend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ShoppingListControllerSecurityIntegrationTest.javabackend/src/test/java/com/omatheusmesmo/shoppmate/list/service/ListItemServiceTest.javabackend/src/test/java/com/omatheusmesmo/shoppmate/list/service/ListPermissionServiceTest.javabackend/src/test/java/com/omatheusmesmo/shoppmate/service/ItemServiceTest.javafrontend/src/app/shared/components/list-item/list-item.component.htmlfrontend/src/app/shared/components/unit/unit.component.tsfrontend/src/app/shared/mocks/mock-services.tsfrontend/src/app/shared/services/category.service.tsfrontend/src/app/shared/services/item.service.tsfrontend/src/app/shared/services/list-permission.service.tsfrontend/src/app/shared/services/shopping-list.service.tsfrontend/src/app/shared/services/unit.service.tsfrontend/src/app/shared/services/user.service.ts
💤 Files with no reviewable changes (2)
- frontend/src/app/shared/mocks/mock-services.ts
- frontend/src/app/shared/components/unit/unit.component.ts
.github/workflows/gemini-triage.yml
Outdated
Comment on lines
+23
to
+25
| outputs: | ||
| available_labels: '${{ steps.get_labels.outputs.available_labels }}' | ||
| selected_labels: '${{ env.SELECTED_LABELS }}' |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
google-github-actions/run-gemini-cli outputs environment variables
💡 Result:
google-github-actions/run-gemini-cli does not “output environment variables” as a built-in feature. The action defines only two outputs:
summary(maps to the captured Gemini CLI stdout / parsed response)error(maps to the captured Gemini CLI stderr / parsed error) [1][2]
Internally, the action writes those two values to GITHUB_OUTPUT (gemini_response and gemini_errors)—it is not dumping env wholesale. [2]
That said, the action does pass various values via environment variables to the step that runs Gemini CLI (including auth-related values), and it also writes a Job Summary that includes the prompt and (when available) the response/error—so if the Gemini CLI/model is induced to print secrets (e.g., via prompt injection), those could end up in the action outputs/logs/summary because they’re derived from stdout/stderr. [2]
Sources:
[1] run-gemini-cli README / docs (Outputs section) (github.com)
[2] action.yml (outputs mapping + GITHUB_OUTPUT writes + job summary generation) (github.com)
Citations:
- 1: https://github.com/google-github-actions/run-gemini-cli
- 2: https://github.com/google-github-actions/run-gemini-cli/blob/main/action.yml
🏁 Script executed:
# Read the workflow file to verify the review comment
cat -n .github/workflows/gemini-triage.yml | head -150Repository: omatheusmesmo/ShoppMate
Length of output: 7000
selected_labels output is always empty—SELECTED_LABELS environment variable is never set.
The output on line 25 references ${{ env.SELECTED_LABELS }}, but this environment variable is never set in the triage job steps. The gemini_analysis step uses google-github-actions/run-gemini-cli, which only exports two fixed outputs (summary and error)—not arbitrary environment variables. As a result, the label job condition on line 105 will never be satisfied.
To fix this, capture the selected labels from the Gemini CLI output (via steps.gemini_analysis.outputs.summary or similar) and pass them explicitly to the output, or parse them as a separate step before the job output.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/gemini-triage.yml around lines 23 - 25, The
selected_labels output is empty because env.SELECTED_LABELS is never set; update
the triage job to extract labels from the Gemini CLI result (e.g.,
steps.gemini_analysis.outputs.summary or steps.get_labels.outputs.*) and set a
step output or env var that the job outputs can reference; specifically, add a
step after run-gemini-cli that parses steps.gemini_analysis.outputs.summary into
a step output like selected_labels (or sets an env var SELECTED_LABELS) and then
change outputs.selected_labels to reference that step output (instead of
env.SELECTED_LABELS) so the downstream label job condition can evaluate
correctly.
Comment on lines
26
to
27
| String key = new String(privateKeyResource.getInputStream().readAllBytes()); | ||
| key = key.replace("-----BEGIN PRIVATE KEY-----", "").replace("-----END PRIVATE KEY-----", "").replaceAll("\\s", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n backend/src/main/java/com/omatheusmesmo/shoppmate/auth/configs/RsaKeyConfig.javaRepository: omatheusmesmo/ShoppMate
Length of output: 2205
Use try-with-resources and explicit charset for PEM loading
Lines 26 and 38 open streams without explicit closing and rely on platform default charset via new String(byte[]). The stream obtained from getInputStream() is not closed—readAllBytes() does not auto-close streams. Additionally, using the platform default charset makes charset handling non-deterministic. Use try-with-resources with StandardCharsets.US_ASCII for proper resource management and deterministic parsing.
♻️ Proposed fix
@@
import java.security.KeyFactory;
@@
import java.util.Base64;
+import java.nio.charset.StandardCharsets;
@@
public RSAPrivateKey privateKey() throws Exception {
- String key = new String(privateKeyResource.getInputStream().readAllBytes());
+ String key;
+ try (var in = privateKeyResource.getInputStream()) {
+ key = new String(in.readAllBytes(), StandardCharsets.US_ASCII);
+ }
@@
public RSAPublicKey publicKey() throws Exception {
- String key = new String(publicKeyResource.getInputStream().readAllBytes());
+ String key;
+ try (var in = publicKeyResource.getInputStream()) {
+ key = new String(in.readAllBytes(), StandardCharsets.US_ASCII);
+ }Also applies to: 38-40
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/auth/configs/RsaKeyConfig.java`
around lines 26 - 27, The PEM loading currently calls
privateKeyResource.getInputStream().readAllBytes() (and similarly
publicKeyResource.getInputStream()) without closing the stream and converts
bytes to String using the platform default charset; change both to use
try-with-resources to open the InputStream and readAllBytes inside the try
block, and convert bytes to String with StandardCharsets.US_ASCII (e.g. new
String(bytes, StandardCharsets.US_ASCII)) before stripping "-----BEGIN/END
PRIVATE/ PUBLIC KEY-----" and whitespace so streams are closed deterministically
and parsing uses an explicit charset; update the code paths that build the key
String (the variables computed from privateKeyResource and publicKeyResource)
accordingly.
Comment on lines
+20
to
+21
| return userRepository.findByEmail(email) | ||
| .orElseThrow(() -> new UsernameNotFoundException("User not found with email: " + email)); |
There was a problem hiding this comment.
Avoid echoing the submitted email in UsernameNotFoundException.
backend/src/main/java/com/omatheusmesmo/shoppmate/utils/exception/GlobalExceptionHandler.java Lines 91-95 currently copy ex.getMessage() into the response body. With the new message here, every miss leaks the submitted email and makes account-enumeration responses more specific than they need to be.
🔒 Proposed fix
return userRepository.findByEmail(email)
- .orElseThrow(() -> new UsernameNotFoundException("User not found with email: " + email));
+ .orElseThrow(() -> new UsernameNotFoundException("User not found"));As per coding guidelines, "Never log sensitive data (PII, tokens, keys)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/auth/service/CustomUserDetailsService.java`
around lines 20 - 21, The current orElseThrow in CustomUserDetailsService
returns a UsernameNotFoundException that echoes the submitted email; change the
thrown message to a generic, non-PII string (e.g., "User not found" or "Invalid
credentials") so the repository lookup in userRepository.findByEmail(email) no
longer leaks the email; update the exception construction in the method (the
lambda passed to orElseThrow) to use the generic message while keeping the same
exception type UsernameNotFoundException.
backend/src/main/java/com/omatheusmesmo/shoppmate/list/controller/ListItemController.java
Outdated
Show resolved
Hide resolved
Comment on lines
+15
to
16
| import java.util.Collections; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 --type=java '@PreAuthorize|@Secured|hasRole\(|hasAuthority\(|ROLE_' backendRepository: omatheusmesmo/ShoppMate
Length of output: 49
🏁 Script executed:
cat backend/src/main/java/com/omatheusmesmo/shoppmate/user/entity/User.javaRepository: omatheusmesmo/ShoppMate
Length of output: 1698
Replace Collections.singletonList() with List.of() per Java 17 guidelines.
At line 40, Collections.singletonList(new SimpleGrantedAuthority(role)) should use List.of(...) instead. Authority format is currently "USER" without the "ROLE_" prefix; while this works with direct authority checks like hasAuthority("USER"), consider prefixing with "ROLE_" if the codebase adopts role-based authorization in the future.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/main/java/com/omatheusmesmo/shoppmate/user/entity/User.java`
around lines 15 - 16, In User.java replace the use of
Collections.singletonList(...) with List.of(...) where authorities are created
(e.g., in the getAuthorities / getRoles logic that currently calls
Collections.singletonList(new SimpleGrantedAuthority(role))); update that
expression to List.of(new SimpleGrantedAuthority(role)) and ensure the import
for java.util.List is present; optionally, if you intend to use role-based
checks later, prepend "ROLE_" to the authority string when constructing
SimpleGrantedAuthority (e.g., "ROLE_" + role) so future hasRole(...) checks work
as expected.
Comment on lines
+105
to
+109
| @ExceptionHandler(ResourceOwnershipException.class) | ||
| public ResponseEntity<ApiError> handleResourceOwnership(ResourceOwnershipException ex) { | ||
| log.warn("Resource ownership violation: {}", ex.getMessage()); | ||
| ApiError apiError = new ApiError(HttpStatus.FORBIDDEN, "Access Denied", ex.getMessage()); | ||
| return new ResponseEntity<>(apiError, HttpStatus.FORBIDDEN); |
There was a problem hiding this comment.
Don't return raw ownership-failure details from the exception.
This handler sends ex.getMessage() to both the logs and the client. If service messages include user emails or list IDs, forbidden responses start leaking internal identifiers. Keep the 403 payload generic here.
🔒 Proposed fix
`@ExceptionHandler`(ResourceOwnershipException.class)
public ResponseEntity<ApiError> handleResourceOwnership(ResourceOwnershipException ex) {
- log.warn("Resource ownership violation: {}", ex.getMessage());
- ApiError apiError = new ApiError(HttpStatus.FORBIDDEN, "Access Denied", ex.getMessage());
+ log.warn("Resource ownership violation");
+ ApiError apiError = new ApiError(
+ HttpStatus.FORBIDDEN,
+ "Access Denied",
+ "You are not authorized to access this resource");
return new ResponseEntity<>(apiError, HttpStatus.FORBIDDEN);
}As per coding guidelines, "Never log sensitive data (PII, tokens, keys)."
📝 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
| @ExceptionHandler(ResourceOwnershipException.class) | |
| public ResponseEntity<ApiError> handleResourceOwnership(ResourceOwnershipException ex) { | |
| log.warn("Resource ownership violation: {}", ex.getMessage()); | |
| ApiError apiError = new ApiError(HttpStatus.FORBIDDEN, "Access Denied", ex.getMessage()); | |
| return new ResponseEntity<>(apiError, HttpStatus.FORBIDDEN); | |
| `@ExceptionHandler`(ResourceOwnershipException.class) | |
| public ResponseEntity<ApiError> handleResourceOwnership(ResourceOwnershipException ex) { | |
| log.warn("Resource ownership violation"); | |
| ApiError apiError = new ApiError( | |
| HttpStatus.FORBIDDEN, | |
| "Access Denied", | |
| "You are not authorized to access this resource"); | |
| return new ResponseEntity<>(apiError, HttpStatus.FORBIDDEN); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/utils/exception/GlobalExceptionHandler.java`
around lines 105 - 109, The handler handleResourceOwnership currently logs and
returns ex.getMessage(), which may expose PII; change it to avoid returning or
logging raw exception details: update the log.warn call to a generic message
like "Resource ownership violation detected" (optionally log a non-sensitive
error id) and construct the ApiError with a generic message (e.g., "Access
Denied" or "You do not have permission to access this resource") without
including ex.getMessage(); keep ResourceOwnershipException for control flow but
do not propagate its message to logs or the ResponseEntity.
Comment on lines
+149
to
+156
| void testNonOwnerCannotGrantPermissionOnOwnListToAnotherUser() throws Exception { | ||
| ListPermissionRequestDTO maliciousDTO = new ListPermissionRequestDTO(ownerList.getId(), userB.getId(), | ||
| Permission.WRITE); | ||
|
|
||
| mockMvc.perform(post("/lists/" + ownerList.getId() + "/permissions").with(csrf()) | ||
| .header("Authorization", "Bearer " + tokenUserA).contentType(MediaType.APPLICATION_JSON) | ||
| .content(objectMapper.writeValueAsString(maliciousDTO))).andExpect(status().isForbidden()) | ||
| .andExpect(jsonPath("$.message").exists()); |
There was a problem hiding this comment.
This is a copy of the previous forbidden scenario.
The body still posts to ownerList with tokenUserA, so it does not exercise the "own list" case the method name promises. That leaves the owner-can-grant-on-their-own-list path untested.
🧪 Proposed fix
- void testNonOwnerCannotGrantPermissionOnOwnListToAnotherUser() throws Exception {
- ListPermissionRequestDTO maliciousDTO = new ListPermissionRequestDTO(ownerList.getId(), userB.getId(),
- Permission.WRITE);
-
- mockMvc.perform(post("/lists/" + ownerList.getId() + "/permissions").with(csrf())
- .header("Authorization", "Bearer " + tokenUserA).contentType(MediaType.APPLICATION_JSON)
- .content(objectMapper.writeValueAsString(maliciousDTO))).andExpect(status().isForbidden())
- .andExpect(jsonPath("$.message").exists());
+ void testOwnerCanGrantPermissionOnOwnListToAnotherUser() throws Exception {
+ ListPermissionRequestDTO permissionDTO = new ListPermissionRequestDTO(userAList.getId(), userB.getId(),
+ Permission.WRITE);
+
+ mockMvc.perform(post("/lists/" + userAList.getId() + "/permissions").with(csrf())
+ .header("Authorization", "Bearer " + tokenUserA).contentType(MediaType.APPLICATION_JSON)
+ .content(objectMapper.writeValueAsString(permissionDTO))).andExpect(status().isCreated())
+ .andExpect(jsonPath("$.permission").value("WRITE"));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ListPermissionControllerSecurityIntegrationTest.java`
around lines 149 - 156, The test duplicates the "forbidden" scenario by posting
to ownerList with tokenUserA; instead exercise the owner-grant path: in
ListPermissionControllerSecurityIntegrationTest update the request to
authenticate as the list owner (use the owner token variable, e.g., tokenOwner
or whatever represents the owner) when posting maliciousDTO for ownerList and
change the expectation from status().isForbidden() to the successful owner
outcome (e.g., status().isOk() and appropriate json assertions) so the
owner-can-grant-on-their-own-list path is covered.
frontend/src/app/shared/components/list-item/list-item.component.html
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
backend/src/test/java/com/omatheusmesmo/shoppmate/list/service/ShoppingListServiceTest.java (2)
95-100: Rename test to match asserted behavior.Line 98 asserts
ResourceOwnershipException, but the method name saysWhenNotFound. Rename for clarity to avoid future confusion in failure triage.Proposed rename
- void testRemoveListThrowsExceptionWhenNotFound() { + void testRemoveListThrowsExceptionWhenUserDoesNotOwnResource() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/test/java/com/omatheusmesmo/shoppmate/list/service/ShoppingListServiceTest.java` around lines 95 - 100, Rename the test method testRemoveListThrowsExceptionWhenNotFound to a name that reflects the asserted exception and scenario (e.g., testRemoveListThrowsResourceOwnershipExceptionWhenNotOwnedByUser) so it matches the assertion of ResourceOwnershipException; update the method declaration to the new name and keep the body invoking shoppingListService.removeList(999L, testUser) with the same repository mock (findByIdAndUserId returning Optional.empty()) to preserve behavior.
112-117: Add explicit edit ownership-denied test case.This segment validates only the success path for
editList. Add a negative ownership test to lock in the 403/ownership contract for updates too.Proposed additional test
+ `@Test` + void testEditListThrowsExceptionWhenUserDoesNotOwnResource() { + when(shoppingListRepository.findByIdAndUserId(1L, testUser.getId())) + .thenReturn(Optional.empty()); + + assertThrows(com.omatheusmesmo.shoppmate.utils.exception.ResourceOwnershipException.class, + () -> shoppingListService.editList(testList, testUser)); + + verify(shoppingListRepository, never()).save(any(ShoppingList.class)); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/test/java/com/omatheusmesmo/shoppmate/list/service/ShoppingListServiceTest.java` around lines 112 - 117, Add a new negative test alongside testEditListSuccess that verifies ownership is enforced: create a test method (e.g., testEditListOwnershipDenied) that tries to call shoppingListService.editList(testList, testUser) but mock shoppingListRepository.findByIdAndUserId(1L, testUser.getId()) to return Optional.empty() (or otherwise indicate the list does not belong to that user) and assert that editList throws the expected ownership/403 exception (e.g., AccessDeniedException or the service's ResponseStatusException for 403) instead of succeeding; use the same testList and testUser setup and mirror the success test structure but assert the exception is thrown.backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ShoppingListControllerSecurityIntegrationTest.java (1)
36-61: Refactor DI to constructor injection with immutable fields.Please switch
@Autowiredfields to constructor-injectedfinaldependencies to match repository standards.As per coding guidelines, "Dependency Injection: Always use constructor injection with final fields."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ShoppingListControllerSecurityIntegrationTest.java` around lines 36 - 61, Replace field injection in ShoppingListControllerSecurityIntegrationTest by making the dependencies private final fields (mockMvc, objectMapper, shoppingListRepository, userRepository, passwordEncoder, jwtService, listMapper, shoppingListService, listPermissionRepository) and injecting them via a constructor; remove the `@Autowired` annotations from the fields, add a single constructor that accepts these types and assigns them to the final fields (you can annotate the constructor with `@Autowired` or omit the annotation if there is only one constructor), and update any usages to reference the final fields.backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ListItemControllerSecurityIntegrationTest.java (1)
45-85: Use constructor injection withfinalfields in this test class.Current field injection works, but it violates the project DI rule and makes dependencies mutable.
Refactor sketch
class ListItemControllerSecurityIntegrationTest { - `@Autowired` - private MockMvc mockMvc; + private final MockMvc mockMvc; + private final ObjectMapper objectMapper; + // ...other final dependencies + `@Autowired` + ListItemControllerSecurityIntegrationTest( + MockMvc mockMvc, + ObjectMapper objectMapper, + ListItemRepository listItemRepository, + ShoppingListRepository shoppingListRepository, + ShoppingListService shoppingListService, + ListItemService listItemService, + UserRepository userRepository, + ItemRepository itemRepository, + CategoryRepository categoryRepository, + UnitRepository unitRepository, + PasswordEncoder passwordEncoder, + JwtService jwtService, + ListMapper listMapper, + ListPermissionRepository listPermissionRepository) { + this.mockMvc = mockMvc; + this.objectMapper = objectMapper; + // ...assign all fields + }As per coding guidelines, "Dependency Injection: Always use constructor injection with final fields."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ListItemControllerSecurityIntegrationTest.java` around lines 45 - 85, The test class ListItemControllerSecurityIntegrationTest currently uses mutable field injection for many beans (mockMvc, objectMapper, listItemRepository, shoppingListRepository, shoppingListService, listItemService, userRepository, itemRepository, categoryRepository, unitRepository, passwordEncoder, jwtService, listMapper, listPermissionRepository); replace these with constructor injection by declaring each as private final, adding a single `@Autowired` constructor that accepts all those types as parameters and assigns them to the final fields, and remove the individual `@Autowired` annotations from the fields so dependencies are immutable and comply with the project's DI rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ListItemControllerSecurityIntegrationTest.java`:
- Around line 30-43: Add the explicit test profile annotation to the test class:
annotate ListItemControllerSecurityIntegrationTest with `@ActiveProfiles`("test")
placed above the class declaration and keep the existing import for
ActiveProfiles; remove any truly unused imports in the file (or other test
classes) to avoid warnings. Ensure the annotation is applied to the class named
ListItemControllerSecurityIntegrationTest so the test uses
application-test.properties and follows the suite convention.
In
`@backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ShoppingListControllerSecurityIntegrationTest.java`:
- Around line 23-34: The test class
ShoppingListControllerSecurityIntegrationTest imports ActiveProfiles but doesn't
apply it, so add the `@ActiveProfiles`("test") annotation to the
ShoppingListControllerSecurityIntegrationTest class declaration to ensure the
test profile (application-test.properties, H2 config, test logging) is loaded
when running integration tests; locate the class
ShoppingListControllerSecurityIntegrationTest and annotate it with
`@ActiveProfiles`("test").
---
Nitpick comments:
In
`@backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ListItemControllerSecurityIntegrationTest.java`:
- Around line 45-85: The test class ListItemControllerSecurityIntegrationTest
currently uses mutable field injection for many beans (mockMvc, objectMapper,
listItemRepository, shoppingListRepository, shoppingListService,
listItemService, userRepository, itemRepository, categoryRepository,
unitRepository, passwordEncoder, jwtService, listMapper,
listPermissionRepository); replace these with constructor injection by declaring
each as private final, adding a single `@Autowired` constructor that accepts all
those types as parameters and assigns them to the final fields, and remove the
individual `@Autowired` annotations from the fields so dependencies are immutable
and comply with the project's DI rule.
In
`@backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ShoppingListControllerSecurityIntegrationTest.java`:
- Around line 36-61: Replace field injection in
ShoppingListControllerSecurityIntegrationTest by making the dependencies private
final fields (mockMvc, objectMapper, shoppingListRepository, userRepository,
passwordEncoder, jwtService, listMapper, shoppingListService,
listPermissionRepository) and injecting them via a constructor; remove the
`@Autowired` annotations from the fields, add a single constructor that accepts
these types and assigns them to the final fields (you can annotate the
constructor with `@Autowired` or omit the annotation if there is only one
constructor), and update any usages to reference the final fields.
In
`@backend/src/test/java/com/omatheusmesmo/shoppmate/list/service/ShoppingListServiceTest.java`:
- Around line 95-100: Rename the test method
testRemoveListThrowsExceptionWhenNotFound to a name that reflects the asserted
exception and scenario (e.g.,
testRemoveListThrowsResourceOwnershipExceptionWhenNotOwnedByUser) so it matches
the assertion of ResourceOwnershipException; update the method declaration to
the new name and keep the body invoking shoppingListService.removeList(999L,
testUser) with the same repository mock (findByIdAndUserId returning
Optional.empty()) to preserve behavior.
- Around line 112-117: Add a new negative test alongside testEditListSuccess
that verifies ownership is enforced: create a test method (e.g.,
testEditListOwnershipDenied) that tries to call
shoppingListService.editList(testList, testUser) but mock
shoppingListRepository.findByIdAndUserId(1L, testUser.getId()) to return
Optional.empty() (or otherwise indicate the list does not belong to that user)
and assert that editList throws the expected ownership/403 exception (e.g.,
AccessDeniedException or the service's ResponseStatusException for 403) instead
of succeeding; use the same testList and testUser setup and mirror the success
test structure but assert the exception is thrown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 05671a8e-7055-47e7-9ddc-85c37b9c7422
📒 Files selected for processing (7)
backend/src/main/java/com/omatheusmesmo/shoppmate/list/entity/ListPermission.javabackend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ListItemControllerSecurityIntegrationTest.javabackend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ListPermissionControllerSecurityIntegrationTest.javabackend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ShoppingListControllerSecurityIntegrationTest.javabackend/src/test/java/com/omatheusmesmo/shoppmate/list/service/ShoppingListServiceTest.javafrontend/src/app/shared/components/unit/unit.component.tsfrontend/src/app/shared/services/shopping-list.service.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ListPermissionControllerSecurityIntegrationTest.java
- frontend/src/app/shared/services/shopping-list.service.ts
- frontend/src/app/shared/components/unit/unit.component.ts
Comment on lines
+30
to
+43
| import org.springframework.test.context.ActiveProfiles; | ||
| import org.springframework.test.web.servlet.MockMvc; | ||
| import com.omatheusmesmo.shoppmate.list.repository.ShoppingListRepository; | ||
| import com.omatheusmesmo.shoppmate.list.repository.ListPermissionRepository; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.*; | ||
| import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.csrf; | ||
| import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.*; | ||
| import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; | ||
| import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; | ||
|
|
||
| @SpringBootTest | ||
| @AutoConfigureMockMvc | ||
| class ListItemControllerSecurityIntegrationTest { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether a dedicated test profile is configured and used.
rg -n --type=java '@ActiveProfiles' backend/src/test/java
fd 'application-test.*' backend/src/test/resources src/test/resources
rg -n --type=properties --type=yaml 'spring\.datasource|spring\.profiles|spring\.jpa' backend/src/test/resources src/test/resourcesRepository: omatheusmesmo/ShoppMate
Length of output: 238
🏁 Script executed:
#!/bin/bash
# Check if the class has `@ActiveProfiles` annotation
echo "=== Checking ListItemControllerSecurityIntegrationTest class ==="
rg -n '@ActiveProfiles|@SpringBootTest|@AutoConfigureMockMvc' backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ListItemControllerSecurityIntegrationTest.java | head -20
# Check content of application-test.properties
echo -e "\n=== Checking application-test.properties content ==="
cat backend/src/test/resources/application-test.properties
# Find other test classes and their profile usage
echo -e "\n=== Other test classes using `@ActiveProfiles` ==="
rg -l '@ActiveProfiles' backend/src/test/java | head -10
# Check for `@SpringBootTest` usage patterns
echo -e "\n=== Sample `@SpringBootTest` classes (first 5) ==="
rg -B2 '@SpringBootTest' backend/src/test/java | head -30Repository: omatheusmesmo/ShoppMate
Length of output: 3480
🏁 Script executed:
#!/bin/bash
# Check AbstractIntegrationTest for profile configuration
echo "=== Checking AbstractIntegrationTest class ==="
cat backend/src/test/java/com/omatheusmesmo/shoppmate/shared/testcontainers/AbstractIntegrationTest.java
# Check Maven pom.xml for test profile configuration
echo -e "\n=== Checking Maven pom.xml test configuration ==="
rg -A10 '<profiles>|<profile>|<activeByDefault>|maven.test' pom.xml 2>/dev/null | head -40
# Check if there's a gradle.properties or gradle build file
echo -e "\n=== Looking for Gradle configuration ==="
fd '(gradle|pom)' . -maxdepth 2 | head -10Repository: omatheusmesmo/ShoppMate
Length of output: 2507
🏁 Script executed:
#!/bin/bash
# Check first 50 lines of ListItemControllerSecurityIntegrationTest to see full class structure
echo "=== First 100 lines of ListItemControllerSecurityIntegrationTest ==="
head -100 backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ListItemControllerSecurityIntegrationTest.java
# Check if Spring Boot automatically loads test profile during test execution
echo -e "\n=== Checking pom.xml for test configuration ==="
cat pom.xml | grep -A20 '<maven.test'Repository: omatheusmesmo/ShoppMate
Length of output: 3921
🏁 Script executed:
#!/bin/bash
# Check if ListItemControllerSecurityIntegrationTest extends AbstractIntegrationTest
echo "=== Checking class hierarchy ==="
head -50 backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ListItemControllerSecurityIntegrationTest.java | grep -E 'class|extends'
# Check another similar security test class to see pattern
echo -e "\n=== Checking ShoppingListControllerSecurityIntegrationTest pattern ==="
head -50 backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ShoppingListControllerSecurityIntegrationTest.java | tail -20
# Verify Spring Boot default behavior - check if application.properties exists
echo -e "\n=== Checking for application.properties ==="
ls -la backend/src/test/resources/application* 2>/dev/null
# Check Spring Boot documentation pattern
echo -e "\n=== Check Spring Boot test config files ==="
find backend/src/test/resources -type f -name "application*" 2>/dev/nullRepository: omatheusmesmo/ShoppMate
Length of output: 858
Add explicit test profile annotation and remove unused import.
The class imports ActiveProfiles at line 30 but never uses it. While application-test.properties exists and is properly configured, explicitly annotating the class with @ActiveProfiles("test") makes the test isolation intent clear and follows Spring Boot conventions. This pattern is currently missing from all test classes in the suite.
Proposed fix
import org.springframework.test.context.ActiveProfiles;
import org.springframework.test.web.servlet.MockMvc;
import com.omatheusmesmo.shoppmate.list.repository.ShoppingListRepository;
import com.omatheusmesmo.shoppmate.list.repository.ListPermissionRepository;
import static org.junit.jupiter.api.Assertions.*;
import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.csrf;
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.*;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;
`@SpringBootTest`
`@AutoConfigureMockMvc`
+@ActiveProfiles("test")
class ListItemControllerSecurityIntegrationTest {Also applies to: 98-107, 155-164, and similar test classes in the suite.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ListItemControllerSecurityIntegrationTest.java`
around lines 30 - 43, Add the explicit test profile annotation to the test
class: annotate ListItemControllerSecurityIntegrationTest with
`@ActiveProfiles`("test") placed above the class declaration and keep the existing
import for ActiveProfiles; remove any truly unused imports in the file (or other
test classes) to avoid warnings. Ensure the annotation is applied to the class
named ListItemControllerSecurityIntegrationTest so the test uses
application-test.properties and follows the suite convention.
Comment on lines
+23
to
+34
| import org.springframework.test.context.ActiveProfiles; | ||
| import org.springframework.test.web.servlet.MockMvc; | ||
| import org.springframework.test.web.servlet.MvcResult; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.*; | ||
| import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.csrf; | ||
| import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.*; | ||
| import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.*; | ||
|
|
||
| @SpringBootTest | ||
| @AutoConfigureMockMvc | ||
| class ShoppingListControllerSecurityIntegrationTest { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "ShoppingListControllerSecurityIntegrationTest.java"Repository: omatheusmesmo/ShoppMate
Length of output: 185
🏁 Script executed:
head -100 backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ShoppingListControllerSecurityIntegrationTest.javaRepository: omatheusmesmo/ShoppMate
Length of output: 3863
🏁 Script executed:
sed -n '65,115p' backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ShoppingListControllerSecurityIntegrationTest.javaRepository: omatheusmesmo/ShoppMate
Length of output: 1830
🏁 Script executed:
grep -n "@ActiveProfiles" backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/*.javaRepository: omatheusmesmo/ShoppMate
Length of output: 49
🏁 Script executed:
# Check if there's a test profile configured and what it does
find . -type f -name "application*.yml" -o -name "application*.properties" | xargs grep -l "^spring.profiles" 2>/dev/nullRepository: omatheusmesmo/ShoppMate
Length of output: 51
🏁 Script executed:
# Check the full class to see if `@ActiveProfiles` exists elsewhere
grep -n "@ActiveProfiles\|@SpringBootTest\|@AutoConfigureMockMvc" backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ShoppingListControllerSecurityIntegrationTest.javaRepository: omatheusmesmo/ShoppMate
Length of output: 110
🏁 Script executed:
# Check what application-test.yml or application-test.properties contains
find . -type f \( -name "application-test.yml" -o -name "application-test.properties" \)Repository: omatheusmesmo/ShoppMate
Length of output: 123
🏁 Script executed:
# Look for another integration test file to compare
find backend/src/test/java -type f -name "*IntegrationTest.java" | head -5Repository: omatheusmesmo/ShoppMate
Length of output: 633
🏁 Script executed:
# Check lines 71-76 and 103-108 in the test file
sed -n '71,76p;103,108p' backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ShoppingListControllerSecurityIntegrationTest.javaRepository: omatheusmesmo/ShoppMate
Length of output: 397
🏁 Script executed:
# Check one of the other integration tests to see the pattern
head -40 backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ListPermissionControllerSecurityIntegrationTest.java | grep -E "@SpringBootTest|@AutoConfigureMockMvc|@ActiveProfiles"Repository: omatheusmesmo/ShoppMate
Length of output: 104
🏁 Script executed:
# Check what's in application-test.properties
cat backend/src/test/resources/application-test.propertiesRepository: omatheusmesmo/ShoppMate
Length of output: 525
🏁 Script executed:
# Check complete class declaration for ShoppingListControllerSecurityIntegrationTest
sed -n '32,36p' backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ShoppingListControllerSecurityIntegrationTest.javaRepository: omatheusmesmo/ShoppMate
Length of output: 174
🏁 Script executed:
# Check if any other integration test files have `@ActiveProfiles`
grep -r "@ActiveProfiles" backend/src/test/java --include="*.java" -lRepository: omatheusmesmo/ShoppMate
Length of output: 49
🏁 Script executed:
# Check Spring Boot test auto-configuration behavior - does it auto-activate test profile?
# Let's see what happens without `@ActiveProfiles` in a `@SpringBootTest`
cat backend/src/test/java/com/omatheusmesmo/shoppmate/category/controller/CategoryControllerWithIntegrationTest.java | head -40Repository: omatheusmesmo/ShoppMate
Length of output: 1840
🏁 Script executed:
# Verify Spring Boot's default behavior regarding test profile activation
# Check if application.properties exists in main resources
find backend/src/main -name "application.properties" -o -name "application.yml"Repository: omatheusmesmo/ShoppMate
Length of output: 116
🏁 Script executed:
# Check if there are any comments or documentation about test profile activation
grep -r "test profile\|ActiveProfiles\|application-test" backend/src/test --include="*.java" -B 2 -A 2 | head -30Repository: omatheusmesmo/ShoppMate
Length of output: 2611
🏁 Script executed:
# Confirm the complete class declaration including all annotations
sed -n '32,40p' backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ShoppingListControllerSecurityIntegrationTest.javaRepository: omatheusmesmo/ShoppMate
Length of output: 258
🏁 Script executed:
# Check if ListItemControllerSecurityIntegrationTest has `@ActiveProfiles` annotation applied
sed -n '32,40p' backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ListItemControllerSecurityIntegrationTest.javaRepository: omatheusmesmo/ShoppMate
Length of output: 644
🏁 Script executed:
# Check if ListPermissionControllerSecurityIntegrationTest has `@ActiveProfiles` annotation applied
sed -n '32,40p' backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ListPermissionControllerSecurityIntegrationTest.javaRepository: omatheusmesmo/ShoppMate
Length of output: 295
🏁 Script executed:
# Check the main application.properties to understand the default configuration
head -20 backend/src/main/resources/application.propertiesRepository: omatheusmesmo/ShoppMate
Length of output: 687
Add @ActiveProfiles("test") annotation to activate test configuration.
The ActiveProfiles import is present but the annotation is not applied to the class. Without it, Spring Boot will not load application-test.properties, and the test configuration (H2 database, logging levels) will not be activated. Add the annotation to ensure tests use the test profile.
Proposed fix
`@SpringBootTest`
`@AutoConfigureMockMvc`
+@ActiveProfiles("test")
class ShoppingListControllerSecurityIntegrationTest {📝 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
| import org.springframework.test.context.ActiveProfiles; | |
| import org.springframework.test.web.servlet.MockMvc; | |
| import org.springframework.test.web.servlet.MvcResult; | |
| import static org.junit.jupiter.api.Assertions.*; | |
| import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.csrf; | |
| import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.*; | |
| import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.*; | |
| @SpringBootTest | |
| @AutoConfigureMockMvc | |
| class ShoppingListControllerSecurityIntegrationTest { | |
| import org.springframework.test.context.ActiveProfiles; | |
| import org.springframework.test.web.servlet.MockMvc; | |
| import org.springframework.test.web.servlet.MvcResult; | |
| import static org.junit.jupiter.api.Assertions.*; | |
| import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.csrf; | |
| import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.*; | |
| import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.*; | |
| `@SpringBootTest` | |
| `@AutoConfigureMockMvc` | |
| `@ActiveProfiles`("test") | |
| class ShoppingListControllerSecurityIntegrationTest { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ShoppingListControllerSecurityIntegrationTest.java`
around lines 23 - 34, The test class
ShoppingListControllerSecurityIntegrationTest imports ActiveProfiles but doesn't
apply it, so add the `@ActiveProfiles`("test") annotation to the
ShoppingListControllerSecurityIntegrationTest class declaration to ensure the
test profile (application-test.properties, H2 config, test logging) is loaded
when running integration tests; locate the class
ShoppingListControllerSecurityIntegrationTest and annotate it with
`@ActiveProfiles`("test").
Owner
|
Hello @felipemelozx ,you can just ignore coderabbit if the file isn't related to your feature. ASAP I'll take a look at your pr. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ListItemControllerSecurityIntegrationTest.java (2)
200-207: Prefer Stream API for counting items.Manual iteration can be simplified using streams per Java 17 best practices. Alternatively, consider adding a
countByShoppListId(Long listId)method to the repository for more efficient queries.♻️ Proposed stream-based refactor
- Iterable<ListItem> userBItems = listItemRepository.findAll(); - long userBItemCount = 0; - for (ListItem li : userBItems) { - if (li.getShoppList().getId().equals(userBList.getId())) { - userBItemCount++; - } - } + long userBItemCount = StreamSupport.stream(listItemRepository.findAll().spliterator(), false) + .filter(li -> li.getShoppList().getId().equals(userBList.getId())) + .count();Add import:
import java.util.stream.StreamSupport;The same pattern applies to lines 233-240.
As per coding guidelines: "Use Stream API & Collections" for cleaner and more idiomatic Java 17 code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ListItemControllerSecurityIntegrationTest.java` around lines 200 - 207, Replace the manual loop used to count items in ListItemControllerSecurityIntegrationTest with a Stream-based or repository-count solution: replace the iteration over listItemRepository.findAll() that increments userBItemCount for entries whose li.getShoppList().getId().equals(userBList.getId()) with either a StreamSupport.stream(...).filter(...) .count() call or, better, add and use a repository method like countByShoppListId(Long id) on listItemRepository and assert that it returns 1 for userBList.getId(); update the assertions at both the current block and the similar block around lines 233-240 to use the new approach.
41-43: Consider adding@ActiveProfiles("test")for explicit test profile activation.The class follows the same pattern as
ShoppingListControllerSecurityIntegrationTestandListPermissionControllerSecurityIntegrationTest, which also lack this annotation. While Spring Boot typically auto-detectsapplication-test.properties, adding the explicit annotation makes test isolation intent clearer. This could be addressed across all security integration tests in a separate cleanup task.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ListItemControllerSecurityIntegrationTest.java` around lines 41 - 43, Add the explicit test profile annotation to the class declaration of ListItemControllerSecurityIntegrationTest: annotate the class ListItemControllerSecurityIntegrationTest with `@ActiveProfiles`("test") and add the required import (org.springframework.test.context.ActiveProfiles) so the test runs with the explicit "test" profile; apply the same change to other security integration test classes in a follow-up cleanup if desired.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ListItemControllerSecurityIntegrationTest.java`:
- Around line 200-207: Replace the manual loop used to count items in
ListItemControllerSecurityIntegrationTest with a Stream-based or
repository-count solution: replace the iteration over
listItemRepository.findAll() that increments userBItemCount for entries whose
li.getShoppList().getId().equals(userBList.getId()) with either a
StreamSupport.stream(...).filter(...) .count() call or, better, add and use a
repository method like countByShoppListId(Long id) on listItemRepository and
assert that it returns 1 for userBList.getId(); update the assertions at both
the current block and the similar block around lines 233-240 to use the new
approach.
- Around line 41-43: Add the explicit test profile annotation to the class
declaration of ListItemControllerSecurityIntegrationTest: annotate the class
ListItemControllerSecurityIntegrationTest with `@ActiveProfiles`("test") and add
the required import (org.springframework.test.context.ActiveProfiles) so the
test runs with the explicit "test" profile; apply the same change to other
security integration test classes in a follow-up cleanup if desired.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 58b6f51c-ea7c-4bdf-874e-4a993aa9b27f
📒 Files selected for processing (3)
backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ListItemControllerSecurityIntegrationTest.javabackend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ListPermissionControllerSecurityIntegrationTest.javabackend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ShoppingListControllerSecurityIntegrationTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ListPermissionControllerSecurityIntegrationTest.java
- backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ShoppingListControllerSecurityIntegrationTest.java
omatheusmesmo
requested changes
Mar 16, 2026
Owner
omatheusmesmo
left a comment
omatheusmesmo
left a comment
There was a problem hiding this comment.
Hello @felipemelozx ,
This report details the inconsistencies and regressions identified in PR #75, primarily resulting from manual conflict resolutions that compromised the integrity of the develop branch.
1. 🚨 Frontend UI Errors
The interpolation in the list item card was accidentally duplicated in the template, causing direct visual redundancy for the user.
Evidence (Diff):
--- a/frontend/src/app/shared/components/list-item/list-item.component.html
+++ b/frontend/src/app/shared/components/list-item/list-item.component.html
@@ -110,6 +110,10 @@
{{ listItem.unitPrice | currency }} =
{{ listItem.totalPrice | currency }} | Category:
{{ listItem.item.category.name }}
+ {{ listItem.quantity }} {{ listItem.item.unit.symbol }} x
+ {{ listItem.unitPrice | currency }} =
+ {{ listItem.totalPrice | currency }} | Category:
+ {{ listItem.item.category.name }}
</mat-card-subtitle>2. 📝 Critical Inconsistencies in README.md
The README.md file suffered severe degradation during the merge, including duplicated instructions and Backend (Java/Maven) commands being listed under the "Frontend" heading.
Evidence (Diff):
--- a/README.md
+++ b/README.md
@@ -71,6 +71,13 @@ docker compose up postgres -d
./mvnw spring-boot:run
```
+#### Frontend
+2. Ensure you have Java 17 installed.
+3. Run with:
+ ```bash
+ ./mvnw spring-boot:run
+ ```
+
#### Frontend
1. Navigate to `frontend/`.
2. Install dependencies:3. 🔒 Backend Security Regression
The RSA key loading validation in RsaKeyConfig.java, which prevented the use of internal keys (inside the jar or classpath) in favor of configurable external files, was completely removed.
Evidence (Diff):
--- a/backend/src/main/java/com/omatheusmesmo/shoppmate/auth/configs/RsaKeyConfig.java
+++ b/backend/src/main/java/com/omatheusmesmo/shoppmate/auth/configs/RsaKeyConfig.java
@@ -24,8 +23,7 @@ public class RsaKeyConfig {
@Bean
public RSAPrivateKey privateKey() throws Exception {
- validateResource(privateKeyResource, "private-key");
- String key = new String(Files.readAllBytes(privateKeyResource.getFile().toPath()));
+ String key = new String(privateKeyResource.getInputStream().readAllBytes());4. 🤖 Gemini Workflows (Removal Required)
Gemini workflows were improperly modified in this PR. Since these are infrastructure files and are not part of the ID Spoofing fix scope (Issue #15), the changes in these files should be reverted or the files removed from this PR to avoid CI conflicts.
5. 🛠️ Feature Implementation Feedback (Performance & Clean Code)
While the security fix is robust and correctly prevents IDOR attacks, some areas for optimization and cleanup were identified:
- Redundant User Lookups: In
ShoppingListService.saveList, theisListValid->getOwnerIdflow performs an extra database call to fetch the User by ID. Since theUserobject is already injected via@AuthenticationPrincipaland passed through the mapper, this extra lookup is unnecessary and impacts performance. - Stale Documentation:
ShoppingListController.javastill contains a// TODO(line 22) about returning only owned/shared lists. This has already been implemented in this PR, so the comment should be removed to keep the code clean. - Duplicated Access Verification: The
updateShoppingListendpoint in the Controller callsservice.findAndVerifyAccess, but the subsequent call toservice.editListperforms the exact same verification. One of these calls should be removed to streamline the request flow. - Error Handling Robustness:
getOwnerIdinShoppingListServicemay trigger anInternal Server Error (500)if the owner ID is unexpectedly null. It is safer to rely directly on the authenticatedUserobject provided by the security context.
6. ✅ Conclusion
The ID Spoofing vulnerability fix is functionally correct and secure. The use of @AuthenticationPrincipal and service-level ownership checks effectively blocks unauthorized access. However, the PR cannot be approved in its current state due to the security regressions, UI bugs, and documentation corruption introduced during conflict resolution.
Recommendations:
- Revert changes to
README.mdand fix the formatting. - Restore
validateResourceinRsaKeyConfig.java. - Clean up the duplication in
list-item.component.html. - Remove Gemini workflows from the scope of this PR.
- Address the performance redundancies mentioned in Section 5.
🛠️ Suggested Recovery Commands
To restore files that are not related to the ID Spoofing feature to their original state in develop, you can run:
# Restore workflows
git checkout origin/develop -- .github/workflows/gemini-dispatch.yml
git checkout origin/develop -- .github/workflows/gemini-triage.yml
# Restore README
git checkout origin/develop -- README.md
# Restore security validation in RSA config
git checkout origin/develop -- backend/src/main/java/com/omatheusmesmo/shoppmate/auth/configs/RsaKeyConfig.javaThis review was made with ❤️ by @omatheusmesmo and @gemini-cli
Owner
There was a problem hiding this comment.
This file was removed in develop/main
…zx/ShoppMate into fix/id-spoofing-vulnerability
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/main/java/com/omatheusmesmo/shoppmate/list/service/ListItemService.java (1)
99-120:⚠️ Potential issue | 🟠 MajorRe-run the quantity invariant on updates.
checkQuantity(...)is only enforced on create. BotheditList(...)overloads now write the new quantity straight to the entity, soquantity <= 0can be persisted through service callers. Revalidate after applying the DTO.Suggested fix (apply to both overloads)
existingListItem.setQuantity(listItemUpdateRequestDTO.quantity()); existingListItem.setPurchased(listItemUpdateRequestDTO.purchased()); existingListItem.setUnitPrice(listItemUpdateRequestDTO.unitPrice()); + checkQuantity(existingListItem); auditService.setAuditData(existingListItem, false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/com/omatheusmesmo/shoppmate/list/service/ListItemService.java` around lines 99 - 120, Both editList overloads (editList(Long id, ...) and editList(Long listId, Long id, ...)) write the DTO quantity directly to the entity without re-running the quantity invariant; call the existing checkQuantity(...) validation after setting quantity (and before auditService.setAuditData / ListItemRepository.save) to ensure quantity > 0 is enforced; update both methods to invoke checkQuantity(existingListItem.getQuantity(), ...) or the appropriate checkQuantity signature and only proceed to auditService.setAuditData(existingListItem, false) and ListItemRepository.save(existingListItem) if validation passes.
♻️ Duplicate comments (1)
backend/src/main/java/com/omatheusmesmo/shoppmate/list/service/ShoppingListService.java (1)
69-73:⚠️ Potential issue | 🟠 MajorRequire mutation-specific authorization before saves.
editList(...)is still guarded by the samefindAndVerifyAccess(...)helper used by read flows. WhateverfindByIdAndUserId(...)treats as merely accessible now becomes sufficient for a save, andListItemServicereuses this helper for add/edit/delete as well. Please split read vs. write checks here and call a write/owner-specific helper from every mutation path.Also applies to: 91-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/com/omatheusmesmo/shoppmate/list/service/ShoppingListService.java` around lines 69 - 73, The editList(...) path uses the read-oriented findAndVerifyAccess(...) helper which treats shared/read access as sufficient for mutations; replace that with a mutation/owner-only check (e.g., add or call a new verifyOwnerAccess/findAndVerifyOwnerAccess helper) before calling editListWithoutVerification(ShoppingList) and update every mutation path (including the ListItemService calls and the analogous block at lines 91-95) to call the owner-specific verifier rather than the read verifier so only owners can perform saves/edits/deletes.
🧹 Nitpick comments (4)
README.md (2)
38-41: Consider adding Frontend prerequisites and backend connection details.The Frontend instructions are standard, but could be enhanced with:
- Prerequisites: Node.js and npm versions
- Backend connection: Whether environment configuration is needed to connect to the backend API (e.g., environment files, proxy configuration)
- Development server: Mention default port (typically 4200)
📝 Example expanded Frontend section
### Frontend +**Prerequisites**: Node.js 18+, npm 9+ + 1. Navigate to `frontend/`. 2. Install dependencies: `npm install`. -3. Run with `ng serve`. +3. Configure backend API URL in `src/environments/environment.ts` if needed. +4. Run with `ng serve` (defaults to http://localhost:4200).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 38 - 41, Update the Frontend section under "Frontend" in README.md to include prerequisites (required Node.js and npm versions), how to configure backend connection (describe which environment files or proxy config to edit and whether an API_BASE_URL or similar env var is needed), and the development server details (default port used by `ng serve`, typically 4200, and how to change it). Reference the existing instructions (the frontend/ folder and the `npm install` + `ng serve` commands) and add a short example of which file to edit (e.g., environment.ts or proxy.conf.json) and where to put the backend URL so developers can run the frontend against the backend out of the box.
33-36: Expand Backend setup instructions with configuration details and prerequisites.The Backend setup is quite minimal and may leave new developers uncertain about required steps. Consider adding:
- Prerequisites: Java 17, Maven, PostgreSQL (with version)
- Configuration specifics: Path to
application.properties(e.g.,src/main/resources/), required properties (spring.datasource.url,spring.datasource.username,spring.datasource.password, JWT secret)- Database setup: Whether Flyway migrations run automatically, or if manual steps are needed
- Authentication setup: Given this PR adds JWT authentication and ownership validation, consider documenting how to create test users or seed initial data
📝 Example expanded Backend section
### Backend +**Prerequisites**: Java 17, Maven, PostgreSQL 14+ + 1. Navigate to `backend/`. -2. Configure your PostgreSQL database in `application.properties`. +2. Configure your PostgreSQL database in `src/main/resources/application.properties`: + ```properties + spring.datasource.url=jdbc:postgresql://localhost:5432/shoppmate + spring.datasource.username=your_user + spring.datasource.password=your_password + jwt.secret=your_secret_key + ``` 3. Run with `./mvnw spring-boot:run`. +4. Flyway migrations will run automatically on startup. +5. Use the registration endpoint to create your first user.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 33 - 36, Update the Backend section in README.md to include prerequisites (Java 17, Maven, PostgreSQL version), explicit configuration details pointing to src/main/resources/application.properties and listing required properties (spring.datasource.url, spring.datasource.username, spring.datasource.password, jwt.secret), note whether Flyway migrations run automatically on startup, and add a short instruction for creating initial test users (e.g., use the registration endpoint) so developers can seed accounts for JWT-authenticated flows and ownership validation..github/workflows/gemini-dispatch.yml (1)
139-139: Prefer explicit secret mapping instead ofsecrets: inherit.
secrets: inheritforwards all available secrets into reusable workflows. Narrowing this to only required secrets improves least-privilege and auditability.Also applies to: 153-153, 167-167
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/gemini-dispatch.yml at line 139, Replace usages of the reusable workflow input "secrets: 'inherit'" with explicit secret mappings: identify the required secrets for the called reusable workflows and replace each occurrence of secrets: 'inherit' with a secrets: mapping that explicitly lists those secrets (e.g., secrets: { SECRET_NAME: ${{ secrets.SECRET_NAME }}, ... }); update all occurrences noted (the entries at the same key on lines referenced) so only the minimal required secrets are forwarded and auditability/least-privilege is preserved.backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ListItemControllerSecurityIntegrationTest.java (1)
171-373: Add collaborator and cross-user mismatch regressions.The suite covers owners vs unrelated users, plus a mismatch between two lists the same user owns. It still does not exercise a READ collaborator or a request that combines
userAListwithuserBListItem, which are the cases that catch "read access can still mutate" and "foreign item IDs are distinguishable" regressions inbackend/src/main/java/com/omatheusmesmo/shoppmate/list/service/ListItemService.java.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ListItemControllerSecurityIntegrationTest.java` around lines 171 - 373, Add tests that exercise two missing cases: (1) a collaborator with READ permission accessing another user's list items to ensure ListItemService enforces read-only collaborator rules and does not allow state mutations, and (2) a request combining userAList path with userBListItem id to ensure ListItemService verifies that the item actually belongs to the path list (rejecting when item.list.id != path listId). Update or add test methods that call the controller endpoints (GET/POST/PUT/DELETE) using token for a collaborator user and a mismatched path/listItem pair, asserting Forbidden/BadRequest as appropriate and that repository state is unchanged; target behavior should be enforced by ListItemService methods that validate permissions and ownership.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/gemini-dispatch.yml:
- Line 197: The GITHUB_TOKEN environment precedence currently prefers a
long-lived secret (secrets.GEMINI_PAT) first, increasing blast radius; change
the precedence so short-lived tokens are chosen first by reordering the
expression to prefer steps.mint_identity_token.outputs.token and github.token
before any long-lived secrets (secrets.GEMINI_PAT or secrets.GITHUB_TOKEN), i.e.
update the GITHUB_TOKEN assignment that references secrets.GEMINI_PAT,
steps.mint_identity_token.outputs.token, secrets.GITHUB_TOKEN, and github.token
so the mint_identity_token output and github.token are evaluated before the
GEMINI_PAT secret.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/list/controller/ShoppingListController.java`:
- Around line 79-85: Controller is performing access check and then calling the
bypass save helper (findAndVerifyAccess -> listMapper.updateEntityFromDto ->
editListWithoutVerification), which splits the authorization boundary; instead,
keep the write-check and save inside the service API by removing public calls to
editListWithoutVerification from ShoppingListController and invoking a single
service method that performs the access verification, mapping/save, and returns
the updated ShoppingList (e.g., add or use ShoppingListService.editList(...)
that accepts the request DTO or entity and does mapping + save internally); make
editListWithoutVerification non-public (private/package-private) so only service
internals can call it, and update ShoppingListController.updateShoppingList to
call the service method that enforces authorization and persists the changes.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/list/service/ListItemService.java`:
- Around line 65-85: The current helpers (ListItemService.findListItemById(Long
id, User user) and findListItemById(Long listId, Long id, User user)) fetch by
global id first and then check access, leaking existence; change the flow to
verify access first and scope the DB query to the list before materializing the
entity: call shoppingListService.findAndVerifyAccess(...) before repository
lookup, and replace usages of ListItemRepository.findByIdAndDeletedFalse(...)
and ListItemRepository.findByIdAndDeletedFalseFetchShoppList(...) with a
repository method that constrains by shoppList id (e.g.
findByIdAndShoppListIdAndDeletedFalse* or add
findByIdAndShoppListIdAndDeletedFalseFetchShoppList) so the repo lookup fails if
the item does not belong to the authorized list; keep the same exception
semantics (NoSuchElementException / IllegalArgumentException) and remove the
pre-check that inspects list membership after an unconstrained fetch.
---
Outside diff comments:
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/list/service/ListItemService.java`:
- Around line 99-120: Both editList overloads (editList(Long id, ...) and
editList(Long listId, Long id, ...)) write the DTO quantity directly to the
entity without re-running the quantity invariant; call the existing
checkQuantity(...) validation after setting quantity (and before
auditService.setAuditData / ListItemRepository.save) to ensure quantity > 0 is
enforced; update both methods to invoke
checkQuantity(existingListItem.getQuantity(), ...) or the appropriate
checkQuantity signature and only proceed to
auditService.setAuditData(existingListItem, false) and
ListItemRepository.save(existingListItem) if validation passes.
---
Duplicate comments:
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/list/service/ShoppingListService.java`:
- Around line 69-73: The editList(...) path uses the read-oriented
findAndVerifyAccess(...) helper which treats shared/read access as sufficient
for mutations; replace that with a mutation/owner-only check (e.g., add or call
a new verifyOwnerAccess/findAndVerifyOwnerAccess helper) before calling
editListWithoutVerification(ShoppingList) and update every mutation path
(including the ListItemService calls and the analogous block at lines 91-95) to
call the owner-specific verifier rather than the read verifier so only owners
can perform saves/edits/deletes.
---
Nitpick comments:
In @.github/workflows/gemini-dispatch.yml:
- Line 139: Replace usages of the reusable workflow input "secrets: 'inherit'"
with explicit secret mappings: identify the required secrets for the called
reusable workflows and replace each occurrence of secrets: 'inherit' with a
secrets: mapping that explicitly lists those secrets (e.g., secrets: {
SECRET_NAME: ${{ secrets.SECRET_NAME }}, ... }); update all occurrences noted
(the entries at the same key on lines referenced) so only the minimal required
secrets are forwarded and auditability/least-privilege is preserved.
In
`@backend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ListItemControllerSecurityIntegrationTest.java`:
- Around line 171-373: Add tests that exercise two missing cases: (1) a
collaborator with READ permission accessing another user's list items to ensure
ListItemService enforces read-only collaborator rules and does not allow state
mutations, and (2) a request combining userAList path with userBListItem id to
ensure ListItemService verifies that the item actually belongs to the path list
(rejecting when item.list.id != path listId). Update or add test methods that
call the controller endpoints (GET/POST/PUT/DELETE) using token for a
collaborator user and a mismatched path/listItem pair, asserting
Forbidden/BadRequest as appropriate and that repository state is unchanged;
target behavior should be enforced by ListItemService methods that validate
permissions and ownership.
In `@README.md`:
- Around line 38-41: Update the Frontend section under "Frontend" in README.md
to include prerequisites (required Node.js and npm versions), how to configure
backend connection (describe which environment files or proxy config to edit and
whether an API_BASE_URL or similar env var is needed), and the development
server details (default port used by `ng serve`, typically 4200, and how to
change it). Reference the existing instructions (the frontend/ folder and the
`npm install` + `ng serve` commands) and add a short example of which file to
edit (e.g., environment.ts or proxy.conf.json) and where to put the backend URL
so developers can run the frontend against the backend out of the box.
- Around line 33-36: Update the Backend section in README.md to include
prerequisites (Java 17, Maven, PostgreSQL version), explicit configuration
details pointing to src/main/resources/application.properties and listing
required properties (spring.datasource.url, spring.datasource.username,
spring.datasource.password, jwt.secret), note whether Flyway migrations run
automatically on startup, and add a short instruction for creating initial test
users (e.g., use the registration endpoint) so developers can seed accounts for
JWT-authenticated flows and ownership validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 46775526-d3f7-4dcd-adc0-cfa3fcb762e4
📒 Files selected for processing (9)
.github/workflows/gemini-dispatch.ymlREADME.mdbackend/src/main/java/com/omatheusmesmo/shoppmate/list/controller/ListItemController.javabackend/src/main/java/com/omatheusmesmo/shoppmate/list/controller/ShoppingListController.javabackend/src/main/java/com/omatheusmesmo/shoppmate/list/repository/ListItemRepository.javabackend/src/main/java/com/omatheusmesmo/shoppmate/list/service/ListItemService.javabackend/src/main/java/com/omatheusmesmo/shoppmate/list/service/ShoppingListService.javabackend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ListItemControllerSecurityIntegrationTest.javafrontend/src/app/shared/components/list-item/list-item.component.html
✅ Files skipped from review due to trivial changes (1)
- frontend/src/app/shared/components/list-item/list-item.component.html
|
|
||
| - name: 'Send failure comment' | ||
| env: | ||
| GITHUB_TOKEN: '${{ secrets.GEMINI_PAT || steps.mint_identity_token.outputs.token || secrets.GITHUB_TOKEN || github.token }}' |
There was a problem hiding this comment.
Failure path prefers a long-lived PAT over short-lived tokens.
At Line 197, secrets.GEMINI_PAT is chosen first, so a broader token is used even when an app token exists. Prefer short-lived app token / github.token first to reduce blast radius.
Suggested hardening
- GITHUB_TOKEN: '${{ secrets.GEMINI_PAT || steps.mint_identity_token.outputs.token || secrets.GITHUB_TOKEN || github.token }}'
+ GITHUB_TOKEN: '${{ steps.mint_identity_token.outputs.token || github.token || secrets.GITHUB_TOKEN || secrets.GEMINI_PAT }}'📝 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
| GITHUB_TOKEN: '${{ secrets.GEMINI_PAT || steps.mint_identity_token.outputs.token || secrets.GITHUB_TOKEN || github.token }}' | |
| GITHUB_TOKEN: '${{ steps.mint_identity_token.outputs.token || github.token || secrets.GITHUB_TOKEN || secrets.GEMINI_PAT }}' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/gemini-dispatch.yml at line 197, The GITHUB_TOKEN
environment precedence currently prefers a long-lived secret
(secrets.GEMINI_PAT) first, increasing blast radius; change the precedence so
short-lived tokens are chosen first by reordering the expression to prefer
steps.mint_identity_token.outputs.token and github.token before any long-lived
secrets (secrets.GEMINI_PAT or secrets.GITHUB_TOKEN), i.e. update the
GITHUB_TOKEN assignment that references secrets.GEMINI_PAT,
steps.mint_identity_token.outputs.token, secrets.GITHUB_TOKEN, and github.token
so the mint_identity_token output and github.token are evaluated before the
GEMINI_PAT secret.
backend/src/main/java/com/omatheusmesmo/shoppmate/list/controller/ShoppingListController.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/omatheusmesmo/shoppmate/list/service/ListItemService.java
Outdated
Show resolved
Hide resolved
felipemelozx
force-pushed
the
fix/id-spoofing-vulnerability
branch
from
6effb7c to
b4c09a1
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/test/java/com/omatheusmesmo/shoppmate/list/service/ListItemServiceTest.java (1)
202-212:⚠️ Potential issue | 🟡 MinorMissing mock for access verification.
The
findAllmethod inListItemServicecallsshoppingListService.findAndVerifyAccess(idList, user)before fetching items, but this test doesn't mock that call. This will cause the test to fail or throw an unexpected exception whenfindAndVerifyAccessreturnsnullor throws.🐛 Proposed fix
`@Test` void findAll() { + when(shoppingListService.findAndVerifyAccess(1L, user)).thenReturn(shoppingList); when(listItemRepository.findByShoppListIdAndDeletedFalse(1L)).thenReturn(List.of(listItem)); List<ListItem> result = service.findAll(1L, user); assertNotNull(result); assertEquals(1, result.size()); + verify(shoppingListService, times(1)).findAndVerifyAccess(1L, user); verify(listItemRepository, times(1)).findByShoppListIdAndDeletedFalse(1L); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/test/java/com/omatheusmesmo/shoppmate/list/service/ListItemServiceTest.java` around lines 202 - 212, The test misses mocking the access check: before stubbing listItemRepository, mock shoppingListService.findAndVerifyAccess(idList, user) to return a valid non-null ShoppingList (or appropriate DTO) so ListItemService.findAll(1L, user) can proceed; specifically add a when(shoppingListService.findAndVerifyAccess(1L, user)).thenReturn(existingShoppingList) (or equivalent) and optionally verify(shoppingListService).findAndVerifyAccess(1L, user) alongside the existing verify of listItemRepository.findByShoppListIdAndDeletedFalse.
🧹 Nitpick comments (2)
backend/src/test/java/com/omatheusmesmo/shoppmate/list/service/ListItemServiceTest.java (1)
107-135: Remove duplicate test method.
findListItem(lines 107-120) andfindListItemById(lines 122-135) are identical in their mocking setup, service method invocation, and assertions. Consider removing one to avoid maintenance overhead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/test/java/com/omatheusmesmo/shoppmate/list/service/ListItemServiceTest.java` around lines 107 - 135, Two test methods (findListItem and findListItemById) are identical; remove the duplicate to avoid maintenance overhead by deleting one of the methods (either findListItem or findListItemById) from ListItemServiceTest.java and keep the other to cover the behavior of service.findListItemById; ensure the remaining test still mocks shoppingListService.findAndVerifyAccess(...) and listItemRepository.findByIdAndShoppListIdAndDeletedFalseFetchShoppList(...), invokes service.findListItemById(shoppingList.getId(), listItem.getId(), user), and retains the assertNotNull and verify(...) assertions.backend/src/main/java/com/omatheusmesmo/shoppmate/list/service/ListItemService.java (1)
21-21: Field name should use camelCase.The field
ListItemRepositoryuses PascalCase (class name style) instead of the standard Java camelCase convention (listItemRepository). This inconsistency can cause confusion and deviates from Java naming conventions.♻️ Proposed fix
- private final ListItemRepository ListItemRepository; + private final ListItemRepository listItemRepository;Also update all usages (lines 33, 48, 70, 77, 88, 94) to use
listItemRepository.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/com/omatheusmesmo/shoppmate/list/service/ListItemService.java` at line 21, Rename the field `ListItemRepository` to follow camelCase (e.g., `listItemRepository`) and update all references to it inside the class (constructor parameter, assignments, and calls in methods such as usages at the locations currently referring to `ListItemRepository`) so the repository field, constructor injection, and method calls consistently use `listItemRepository`; ensure constructor parameter and any getters/uses (e.g., in save/update/find/remove methods) are renamed accordingly to avoid compilation errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@backend/src/test/java/com/omatheusmesmo/shoppmate/list/service/ListItemServiceTest.java`:
- Around line 202-212: The test misses mocking the access check: before stubbing
listItemRepository, mock shoppingListService.findAndVerifyAccess(idList, user)
to return a valid non-null ShoppingList (or appropriate DTO) so
ListItemService.findAll(1L, user) can proceed; specifically add a
when(shoppingListService.findAndVerifyAccess(1L,
user)).thenReturn(existingShoppingList) (or equivalent) and optionally
verify(shoppingListService).findAndVerifyAccess(1L, user) alongside the existing
verify of listItemRepository.findByShoppListIdAndDeletedFalse.
---
Nitpick comments:
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/list/service/ListItemService.java`:
- Line 21: Rename the field `ListItemRepository` to follow camelCase (e.g.,
`listItemRepository`) and update all references to it inside the class
(constructor parameter, assignments, and calls in methods such as usages at the
locations currently referring to `ListItemRepository`) so the repository field,
constructor injection, and method calls consistently use `listItemRepository`;
ensure constructor parameter and any getters/uses (e.g., in
save/update/find/remove methods) are renamed accordingly to avoid compilation
errors.
In
`@backend/src/test/java/com/omatheusmesmo/shoppmate/list/service/ListItemServiceTest.java`:
- Around line 107-135: Two test methods (findListItem and findListItemById) are
identical; remove the duplicate to avoid maintenance overhead by deleting one of
the methods (either findListItem or findListItemById) from
ListItemServiceTest.java and keep the other to cover the behavior of
service.findListItemById; ensure the remaining test still mocks
shoppingListService.findAndVerifyAccess(...) and
listItemRepository.findByIdAndShoppListIdAndDeletedFalseFetchShoppList(...),
invokes service.findListItemById(shoppingList.getId(), listItem.getId(), user),
and retains the assertNotNull and verify(...) assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5335224d-4b98-40d6-89c6-17811ff8c642
📒 Files selected for processing (6)
backend/src/main/java/com/omatheusmesmo/shoppmate/list/controller/ShoppingListController.javabackend/src/main/java/com/omatheusmesmo/shoppmate/list/repository/ListItemRepository.javabackend/src/main/java/com/omatheusmesmo/shoppmate/list/service/ListItemService.javabackend/src/main/java/com/omatheusmesmo/shoppmate/list/service/ShoppingListService.javabackend/src/test/java/com/omatheusmesmo/shoppmate/list/controller/ListItemControllerSecurityIntegrationTest.javabackend/src/test/java/com/omatheusmesmo/shoppmate/list/service/ListItemServiceTest.java
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
8 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.
Pull Request Description
This PR implements a complete fix for an ID Spoofing (IDOR) vulnerability that allowed authenticated users to access or modify resources belonging to other users by manipulating user IDs in request payloads.
Related Task
Resolves #15
What was done?
@AuthenticationPrincipal, allowing the authenticated user to be automatically injected from theSecurityContextHolder.Userentity to implementUserDetails, enabling Spring Security to manage it directly after authentication through the JWT filter.ResourceOwnershipExceptionis thrown and an HTTP 403 Forbidden response is returned.idUserfield from request DTOs to eliminate the spoofing vector.Tests
How to Test
Useful Links
@AuthenticationPrincipaldocumentationSummary by CodeRabbit
New Features
Documentation
Tests