Skip to content

Conversation

Denjiwe
Copy link

@Denjiwe Denjiwe commented Oct 8, 2025

This change resolves #99 by using a Illuminate\Validation\Rule validation, checking whether the given group, party, wallet or categories belong to the authenticated user.

Copy link

sourceant bot commented Oct 8, 2025

Code Review Summary

The pull request primarily focuses on enhancing the store method within TransactionController.php by implementing more robust security validation for related resources. Specifically, it updates the group_id, party_id, wallet_id, and categories.* validation rules to ensure that users can only associate transactions with resources they own. This is a positive step towards improving data integrity and preventing unauthorized data manipulation.

🚀 Key Improvements

  • Enhanced security and data integrity in the store method by scoping resource validation to the authenticated user's user_id.

💡 Minor Suggestions

  • Consider extracting the repeated Rule::exists(...)->where(user_id) logic into a reusable custom rule or helper function to improve DRYness and maintainability for future enhancements.

🚨 Critical Issues

  • The update method in app/Http/Controllers/API/v1/TransactionController.php is missing the same user-scoped validation introduced in the store method for party_id, wallet_id, group_id, and categories.*. This constitutes a critical security vulnerability, allowing users to potentially link transactions to resources belonging to other users. This issue must be resolved before merging.

Copy link

@sourceant sourceant bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review complete. See the overview comment for a summary.

'description' => 'nullable|string',
'datetime' => ['nullable', new Iso8601DateTime],
'created_at' => ['nullable', new Iso8601DateTime],
'group_id' => 'nullable|integer|exists:groups,id',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The party_id validation in the update method currently uses a simple exists rule, which means any party_id existing in the database would be considered valid. This could allow a user to link a transaction to a party belonging to another user, leading to a security vulnerability and data integrity issue. This should mirror the more secure validation implemented in the store method.

Suggested change
'group_id' => 'nullable|integer|exists:groups,id',
'party_id' => ['nullable', 'integer', Rule::exists('parties', 'id')->where(function ($query) { $query->where('user_id', auth()->user()->id); })],

'datetime' => ['nullable', new Iso8601DateTime],
'created_at' => ['nullable', new Iso8601DateTime],
'group_id' => 'nullable|integer|exists:groups,id',
'party_id' => 'nullable|integer|exists:parties,id',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to party_id, the wallet_id validation in the update method lacks user-specific scope. This could enable a user to associate a transaction with a wallet that does not belong to them. It is crucial to apply the same secure exists validation with a user_id scope as introduced in the store method to prevent unauthorized data manipulation.

Suggested change
'party_id' => 'nullable|integer|exists:parties,id',
'wallet_id' => ['nullable', 'integer', Rule::exists('wallets', 'id')->where(function ($query) { $query->where('user_id', auth()->user()->id); })],

'type' => 'required|string|in:income,expense',
'description' => 'nullable|string',
'datetime' => ['nullable', new Iso8601DateTime],
'created_at' => ['nullable', new Iso8601DateTime],
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The group_id validation in the update method suffers from the same vulnerability as party_id and wallet_id. Without scoping the exists rule to the authenticated user's groups, a malicious user could link their transaction to another user's group. Update this validation rule to include the user_id check for enhanced security.

Suggested change
'created_at' => ['nullable', new Iso8601DateTime],
'group_id' => ['nullable', 'integer', Rule::exists('groups', 'id')->where(function ($query) { $query->where('user_id', auth()->user()->id); })],

'is_recurring' => 'nullable|boolean',
'recurrence_period' => 'nullable|string|in:daily,weekly,monthly,yearly',
'recurrence_interval' => 'nullable|integer|min:1',
'recurrence_ends_at' => ['nullable', 'date', 'after:today', new Iso8601DateTime],
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The validation for categories.* in the update method also needs to be updated to ensure that users can only associate transactions with categories they own. Failing to do so opens a potential security loophole where a user could link to categories belonging to other users. Apply the Rule::exists with the user scope, consistent with the store method's implementation.

Suggested change
'recurrence_ends_at' => ['nullable', 'date', 'after:today', new Iso8601DateTime],
'categories.*' => ['integer', Rule::exists('categories', 'id')->where(function ($query) { $query->where('user_id', auth()->user()->id); })],

@kofimokome kofimokome self-requested a review October 8, 2025 10:19
Copy link
Collaborator

@kofimokome kofimokome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Denjiwe thanks for submitting a PR. We appreciate your contributions.
Please kindly address the following issues:

  1. Run php artisan test locally and ensure all the tests pass, then update the PR
  2. We follow the Conventional Commits Specification for our commit messages. Please rename your commits to follow the rules outlined here

Feel free to reply here if you need any help.

@kofimokome kofimokome requested a review from nfebe October 8, 2025 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ensure groups, categories and parties belong to the user.

2 participants