Skip to content

Feature/transaction itens user validation #108

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Feature/transaction itens user validation #108

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions app/Http/Controllers/API/v1/TransactionController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use Illuminate\Http\JsonResponse;
use Illuminate\Http\Request;
use Illuminate\Support\Facades\DB;
use Illuminate\Validation\Rule;
use OpenApi\Attributes as OA;
use Symfony\Component\HttpKernel\Exception\HttpException;
use Throwable;
Expand Down Expand Up @@ -205,15 +206,23 @@ public function store(Request $request): JsonResponse
'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); })],

'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); })],

'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); })],

'wallet_id' => 'nullable|integer|exists:wallets,id',
'group_id' => ['nullable', 'integer', Rule::exists('groups', 'id')->where(function ($query) {
$query->where('user_id', auth()->user()->id);
})],
'party_id' => ['nullable', 'integer', Rule::exists('parties', 'id')->where(function ($query) {
$query->where('user_id', auth()->user()->id);
})],
'wallet_id' => ['nullable', 'integer', Rule::exists('wallets', 'id')->where(function ($query) {
$query->where('user_id', auth()->user()->id);
})],
'categories' => 'nullable|array',
'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); })],

'categories.*' => 'integer|exists:categories,id',
'categories.*' => ['integer', Rule::exists('categories', 'id')->where(function ($query) {
$query->where('user_id', auth()->user()->id);
})],
'files' => 'nullable|array',
'files.*' => 'file|mimes:jpg,jpeg,png,pdf|max:1240',
]);
Expand Down
Loading