fix(api): validate transaction category, merchant and tag ownership#2823
fix(api): validate transaction category, merchant and tag ownership#2823erkdgn wants to merge 4 commits into
Conversation
The transactions API assigned category_id, merchant_id and tag_ids straight onto the record without checking them, which failed three different ways: - A malformed UUID was cast to NULL by ActiveRecord, so the request returned 201 with the association silently dropped. This is what issue we-promise#2781 reports: the category is sent, the call succeeds, and the transaction comes back uncategorized with no error to explain why. - An unknown but well-formed UUID tripped the foreign key, and the blanket rescue turned it into a 500 internal_server_error. - A UUID belonging to another family passed the foreign key and was accepted, letting a caller attach another family's category, merchant or tag to their own transaction. Validate all three against the caller's own family before assigning them and return 422 otherwise, matching the scoping already applied in trades_controller and recurring_transactions_controller. Also spell out in the OpenAPI descriptions that these ids must be UUIDs from the caller's own family. Fixes we-promise#2781 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughTransaction creation and updates now reject malformed or cross-family category, merchant, and tag IDs before processing. Merchant listings expose family and provider merchants assignable through the API, with tests and API schemas covering the updated behavior. ChangesTransaction association scoping
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant APIClient
participant TransactionsController
participant Family
participant Transaction
APIClient->>TransactionsController: Create or update transaction
TransactionsController->>Family: Validate category, merchant, and tag ownership
Family-->>TransactionsController: Return validation result
TransactionsController->>Transaction: Process mutation when valid
TransactionsController-->>APIClient: Return transaction or validation error
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4912ada6a1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| end | ||
|
|
||
| merchant_id = transaction_params[:merchant_id] | ||
| if merchant_id.present? && !family_owns?(family.merchants, merchant_id) |
There was a problem hiding this comment.
When a family has provider-synced transactions, Api::V1::MerchantsController#index intentionally returns their accessible ProviderMerchant records, but this check searches only family.merchants, whose association is restricted to FamilyMerchant. Consequently, passing a merchant ID returned by GET /api/v1/merchants to transaction create or update now incorrectly returns 422; validate against the same accessible merchant scope used by the merchants API.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 84f1df8 — thank you, this was a real regression.
Verified against the dev container: a ProviderMerchant attached to a transaction on an accessible account is returned by GET /api/v1/merchants, and passing that id to POST /api/v1/transactions returned 422 with my guard in place.
Rather than duplicate the index's scope, I extracted it into Family#api_assignable_merchants_for(user) and now use it from both MerchantsController#index and the transactions validation, so the two cannot drift apart again. Added a regression test that asserts the merchants index lists the provider merchant and that creating a transaction with it succeeds.
| return | ||
| end | ||
|
|
||
| return unless validate_family_associations |
There was a problem hiding this comment.
When PATCH supplies an invalid or foreign category, merchant, or tag, this new guard returns 422, but the PATCH operation in spec/requests/api/v1/transactions_spec.rb still documents only 200/404 responses and does not describe the ownership constraints; the generated OpenAPI YAML mirrors that omission. Update the rswag PATCH operation and regenerate the docs so clients receive the newly introduced contract.
AGENTS.md reference: AGENTS.md:L39-L46
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in 84f1df8. The PATCH operation in spec/requests/api/v1/transactions_spec.rb now documents the family-ownership requirement on category_id, merchant_id and tag_ids (keeping the existing omit-vs-[] tag semantics) and adds the 422 response, and docs/api/openapi.yaml is regenerated.
One note on the regenerated file: rswag:specs:swaggerize also emits pre-existing drift unrelated to this PR (the merchants CSV import endpoint, MerchantImportResult, and the transfer fee fields), from specs already on main that were never regenerated. I left that out to keep this diff reviewable — it is still outstanding on main and probably worth a separate sync.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/controllers/api/v1/transactions_controller.rb`:
- Around line 334-350: Update the transaction parameter validation around
tags_provided? and the tag_ids handling to inspect the raw request value before
permit casts it. Reject tag_ids unless it is an array containing only strings,
including rejecting scalar, object, and blank-string elements, with the existing
422 validation response before create or update; only then normalize,
deduplicate, and apply tag IDs without clearing existing tags.
In `@spec/requests/api/v1/transactions_spec.rb`:
- Around line 174-179: Update the PATCH request schema in the transaction API
spec to document category_id, merchant_id, and tag_ids family-ownership
requirements consistently with POST, preserving the existing tag omission versus
[] semantics; then regenerate docs/api/openapi.yaml and ensure request coverage
reflects the same 422 validation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e4d4591c-c498-4447-ade0-3f21d4fd8528
📒 Files selected for processing (4)
app/controllers/api/v1/transactions_controller.rbdocs/api/openapi.yamlspec/requests/api/v1/transactions_spec.rbtest/controllers/api/v1/transactions_controller_test.rb
Addresses review feedback on the association scoping guard. GET /api/v1/merchants returns a family's own merchants plus the provider merchants attached to transactions in accounts the user can reach, but Family#merchants is scoped to FamilyMerchant alone. Validating against it meant a merchant id taken straight from the merchants index was rejected with a 422 on create and update. Extract the scope the merchants index already uses into Family#api_assignable_merchants_for and validate against that, so the two endpoints cannot drift apart again. tag_ids needs checking before strong parameters see it. permit(tag_ids: []) drops any value that is not an array of scalars, so a string or an object arrived as nil and then cleared the transaction's tags on update while reporting 200. Reject a non-array shape with a 422. Blank entries stay allowed and are simply skipped in the ownership check, because a form-encoded empty array arrives as [""] and that is how a caller clears tags. Also document the family ownership requirement and the 422 response on the PATCH operation, which previously described only 200 and 404. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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)
spec/requests/api/v1/transactions_spec.rb (1)
174-175: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDocument provider merchants as assignable.
api_assignable_merchants_forand the new controller test permit provider merchants surfaced byGET /api/v1/merchants, but these descriptions say a merchant must belong to the caller’s family. Describemerchant_idas an ID returned by that endpoint instead.
spec/requests/api/v1/transactions_spec.rb#L174-L175: Correct the POST merchant description.spec/requests/api/v1/transactions_spec.rb#L316-L317: Correct the PATCH merchant description.docs/api/openapi.yaml#L7358-L7362: Regenerate/update the POST OpenAPI description.docs/api/openapi.yaml#L7478-L7482: Regenerate/update the PATCH OpenAPI description.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spec/requests/api/v1/transactions_spec.rb` around lines 174 - 175, Update the merchant_id descriptions for both POST and PATCH transaction schemas to state that the ID must be returned by GET /api/v1/merchants and may refer to provider merchants, rather than requiring membership in the caller’s family. Apply the matching description changes in spec/requests/api/v1/transactions_spec.rb lines 174-175 and 316-317, and docs/api/openapi.yaml lines 7358-7362 and 7478-7482.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@spec/requests/api/v1/transactions_spec.rb`:
- Around line 174-175: Update the merchant_id descriptions for both POST and
PATCH transaction schemas to state that the ID must be returned by GET
/api/v1/merchants and may refer to provider merchants, rather than requiring
membership in the caller’s family. Apply the matching description changes in
spec/requests/api/v1/transactions_spec.rb lines 174-175 and 316-317, and
docs/api/openapi.yaml lines 7358-7362 and 7478-7482.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f04a7fa9-f538-41a3-a2fb-0552ad0eaf1e
📒 Files selected for processing (6)
app/controllers/api/v1/merchants_controller.rbapp/controllers/api/v1/transactions_controller.rbapp/models/family.rbdocs/api/openapi.yamlspec/requests/api/v1/transactions_spec.rbtest/controllers/api/v1/transactions_controller_test.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- app/controllers/api/v1/transactions_controller.rb
84f1df8 widened transaction create/update to accept the same provider merchants GET /api/v1/merchants surfaces, but the merchant_id description still said it had to belong to the caller's family. Word it the way the validation actually behaves. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Confirmed and fixed in 751799a. The |
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 (2)
spec/requests/api/v1/transactions_spec.rb (1)
174-179: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winBroaden the POST 422 response documentation.
These field descriptions correctly add family-scoped association requirements, but the POST response documentation still identifies 422 only as a missing-required-fields error. Include invalid, unknown, and cross-family association IDs, then regenerate
docs/api/openapi.yaml.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spec/requests/api/v1/transactions_spec.rb` around lines 174 - 179, Update the POST transaction 422 response documentation in the surrounding request schema to describe missing required fields plus invalid, unknown, and cross-family association IDs, including category, merchant, and tag references. Then regenerate docs/api/openapi.yaml so the committed OpenAPI specification matches the updated documentation.docs/api/openapi.yaml (1)
7356-7386: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument POST association-validation failures in the 422 response.
The request fields now correctly state family-scoped IDs, but the POST response still describes 422 only as “missing required fields.” The endpoint also returns 422 for malformed, unknown, and cross-family category, merchant, and tag IDs. Broaden the response description in the request spec and regenerate this document.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/api/openapi.yaml` around lines 7356 - 7386, Broaden the POST endpoint’s 422 response description from missing required fields to include malformed, unknown, and cross-family category, merchant, and tag ID association-validation failures. Update the request specification source for this endpoint, then regenerate docs/api/openapi.yaml so the generated document reflects the expanded validation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/api/openapi.yaml`:
- Around line 7356-7386: Broaden the POST endpoint’s 422 response description
from missing required fields to include malformed, unknown, and cross-family
category, merchant, and tag ID association-validation failures. Update the
request specification source for this endpoint, then regenerate
docs/api/openapi.yaml so the generated document reflects the expanded validation
behavior.
In `@spec/requests/api/v1/transactions_spec.rb`:
- Around line 174-179: Update the POST transaction 422 response documentation in
the surrounding request schema to describe missing required fields plus invalid,
unknown, and cross-family association IDs, including category, merchant, and tag
references. Then regenerate docs/api/openapi.yaml so the committed OpenAPI
specification matches the updated documentation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5df70b3c-fa8b-4603-80e5-c5214ed08dd9
📒 Files selected for processing (2)
docs/api/openapi.yamlspec/requests/api/v1/transactions_spec.rb
751799a fixed the merchant_id wording but missed that the POST 422 documentation never covered the family-scoping validation added in 4912ada -- only the PATCH operation had it. The two existing POST 422 examples ("missing account_id", "missing required fields") don't demonstrate an invalid category_id, so the generated doc undersold what the endpoint actually rejects with. rswag's rake task runs with --order defined (lib in rswag-specs' swaggerize.rake), so when multiple response '422' blocks exist on the same operation, only the last-declared one's description survives the merge -- there is no way to represent multiple distinct 422 causes as separate entries. Rather than depend on which one happens to be declared last, gave all three POST 422 examples on this operation the same description covering every case, and added a third example that actually exercises an invalid category_id so the scenario has coverage, matching the PATCH operation's existing example. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Both outside-diff findings from the latest review (on 751799a) confirmed and fixed in fdb4178. The POST 422 documentation only ever showed "missing account_id" or "missing required fields" — the association-validation failures added in 4912ada were never represented for POST, only for PATCH. While fixing it I found the underlying reason this kept happening: rswag's Fixed by giving all three POST 422 examples the same comprehensive description, and adding a third example that actually exercises an invalid |
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
Solid fix for a real correctness/security gap — the three failure modes in the PR description (malformed UUID silently dropped, unknown UUID → 500, cross-family id accepted) all check out against the diff, and transactions_controller.rb was indeed the outlier vs. trades_controller.rb/recurring_transactions_controller.rb, which already scope by family.
A few things I liked:
- Extracting
Family#api_assignable_merchants_forout of the merchants controller and reusing it for merchant ownership checks keeps the "assignable to this family" definition in one place instead of duplicating the provider-merchant logic. validate_tag_idsreading from the rawparams[:transaction][:tag_ids]rather than the permitted params, with the comment explaining why (permit(tag_ids: [])would silently coerce a bad value toniland wipe existing tags on update) — that's a subtle trap and worth documenting the way it's documented.- Test coverage is thorough: malformed/unknown/cross-family × category/merchant/tag × create/update, plus the tag-array-vs-scalar edge case and the empty-array-clears-tags case.
Only note (already called out in the PR description, not a blocker): the GET /api/v1/transactions?category_id=... index filter still silently returns an empty set for an unusable id rather than a 4xx — different code path, fine to leave for a follow-up.
Generated by Claude Code
2 participants
Fixes #2781
The problem
Api::V1::TransactionsControllerassignedcategory_id,merchant_idandtag_idsstraight onto the record without validating them. That failed three different ways, verified against a dev container:"18bsy6-")"category": nullinternal_server_errorThe first row is what #2781 reports. ActiveRecord casts an invalid UUID to
NULLfor auuidcolumn, so the API accepts the request, reports success, and returns the transaction uncategorized — with nothing indicating what went wrong. The reporter reasonably concluded the field type must be wrong.The second row happens because the unknown id reaches the
transactions → categoriesforeign key, and the action's blanketrescue => ereports that as a generic 500.The third row is a cross-family issue: a valid UUID from another family satisfies the foreign key, so a caller could attach another family's category, merchant or tag to their own transaction.
merchant_idandtag_idsbehave identically tocategory_idon all three, andupdatesilently ignores an unusablecategory_idthe same waycreatedoes.The fix
Validate all three references against the caller's own family before assigning them, returning 422 when they don't resolve. This is the same scoping already used by
trades_controller.rb(category_id) andrecurring_transactions_controller.rb(merchant_id) —transactions_controller.rbwas the one place missing it.The OpenAPI descriptions for
category_id,merchant_idandtag_idsnow state that the value must be a UUID from the caller's own family, which is the documentation half of the confusion in the issue.Notes for reviewers
category_idskips validation as before.GET /api/v1/transactions?category_id=...) still returns an empty result set for an unusable id rather than a 4xx. Same class of silent failure, but different code path and different semantics, so it is left out of this PR.docs/api/openapi.yamlwas regenerated withrswag:specs:swaggerize. Regeneration also surfaces pre-existing drift unrelated to this change (merchants CSV import endpoint, transfer fee fields), which I deliberately excluded to keep this diff focused — that drift is still outstanding onmain.Verification
Run inside the dev container against this branch:
Nine tests were added to
test/controllers/api/v1/transactions_controller_test.rbcovering the malformed, unknown and cross-family cases for category, merchant and tag on bothcreateandupdate, plus the happy path. Eight of them fail onmainand pass here.🤖 Generated with Claude Code
Summary by CodeRabbit
category_id,merchant_id, andtag_idsare from the caller’s family before saving.422 Unprocessable Entity.tag_idsare rejected; an explicit empty array clears tags.category_id,merchant_id, andtag_ids, including tag assignment vs clearing.