Shopping list addItem does not validate ingredient belongs to household #12

New Issue

marcel · 2026-04-02T11:21:22+02:00

marcel commented

2026-04-02 11:21:22 +02:00

Problem

In ShoppingService.addItem(), when a user provides an ingredientId, it is looked up by ID only — without checking that the ingredient belongs to the user's household. This allows cross-household data references.

Affected files

ShoppingService.java:131-134

ingredient = ingredientRepository.findById(request.ingredientId())
    .orElseThrow(...);  // no household check

Attack scenario

A user guesses or obtains an ingredient UUID from another household and adds it to their shopping list, linking cross-tenant data.

Recommended fix

Use ingredientRepository.findByIdAndHouseholdId(request.ingredientId(), householdId) instead of findById.

Severity

Medium — cross-household data leakage via IDOR on ingredient references.

## Problem In `ShoppingService.addItem()`, when a user provides an `ingredientId`, it is looked up by ID only — without checking that the ingredient belongs to the user's household. This allows cross-household data references. ## Affected files - `ShoppingService.java:131-134` ```java ingredient = ingredientRepository.findById(request.ingredientId()) .orElseThrow(...); // no household check ``` ## Attack scenario A user guesses or obtains an ingredient UUID from another household and adds it to their shopping list, linking cross-tenant data. ## Recommended fix Use `ingredientRepository.findByIdAndHouseholdId(request.ingredientId(), householdId)` instead of `findById`. ## Severity Medium — cross-household data leakage via IDOR on ingredient references.

marcel added the kind/security priority/medium labels 2026-04-02 11:21:48 +02:00

marcel referenced this issue

2026-04-03 09:25:26 +02:00

No household-level DB enforcement — isolation relies solely on application code #13

marcel commented

2026-04-03 09:26:18 +02:00

👨‍💻 Kai — Frontend Engineer

This is a pure backend bug, but I want to flag the frontend surface that interacts with it.

How this manifests in the UI

The shopping list (D1) likely has an "add item" flow where a user can select an existing ingredient from their household's ingredient list. If the frontend fetches ingredients via a household-scoped endpoint, the UUID it sends back should always be from the current household — so in normal usage, the IDOR isn't reachable through my UI.
However, if the ingredient selection is ever driven by a search or autocomplete that could theoretically return cross-household results (e.g., a future "search all ingredients" endpoint), the risk compounds.

Questions for me to answer

Does the add-item flow in D1 send an ingredientId explicitly, or does it send ingredient data (name, unit) and let the backend look it up or create it? The spec on this would help me understand what I'm passing to POST /shopping/items.
Is there a client-side ingredient picker component that fetches from a household-scoped ingredient list? If so, that component is implicitly safe — but I should confirm the endpoint it calls is itself household-scoped.

What I'd want from the fix

No API contract changes expected — the fix is entirely server-side. But if the endpoint starts returning 404 (or 403?) for a mismatched ingredient, my error handling in +page.server.ts needs to handle that case and surface a meaningful message to the user rather than a generic failure.
Confirm: will the fixed endpoint return 404 ("ingredient not found in this household") or 403 ("access denied")? The UX message differs.

## 👨‍💻 Kai — Frontend Engineer This is a pure backend bug, but I want to flag the frontend surface that interacts with it. **How this manifests in the UI** - The shopping list (D1) likely has an "add item" flow where a user can select an existing ingredient from their household's ingredient list. If the frontend fetches ingredients via a household-scoped endpoint, the UUID it sends back should always be from the current household — so in normal usage, the IDOR isn't reachable through my UI. - However, if the ingredient selection is ever driven by a search or autocomplete that could theoretically return cross-household results (e.g., a future "search all ingredients" endpoint), the risk compounds. **Questions for me to answer** - Does the add-item flow in D1 send an `ingredientId` explicitly, or does it send ingredient data (name, unit) and let the backend look it up or create it? The spec on this would help me understand what I'm passing to `POST /shopping/items`. - Is there a client-side ingredient picker component that fetches from a household-scoped ingredient list? If so, that component is implicitly safe — but I should confirm the endpoint it calls is itself household-scoped. **What I'd want from the fix** - No API contract changes expected — the fix is entirely server-side. But if the endpoint starts returning 404 (or 403?) for a mismatched ingredient, my error handling in `+page.server.ts` needs to handle that case and surface a meaningful message to the user rather than a generic failure. - Confirm: will the fixed endpoint return 404 ("ingredient not found in this household") or 403 ("access denied")? The UX message differs.

marcel commented

2026-04-03 09:26:28 +02:00

🛠️ Backend Engineer

Classic IDOR — simple to fix, important to catch. The fix itself is one line, but there are a few things to think through carefully.

The fix
Change:

ingredient = ingredientRepository.findById(request.ingredientId())
    .orElseThrow(...);

To:

ingredient = ingredientRepository.findByIdAndHouseholdId(request.ingredientId(), householdId)
    .orElseThrow(() -> new ResourceNotFoundException("Ingredient not found"));

This should throw a 404, not a 403. Returning 404 is the right choice here — it avoids confirming to an attacker that the UUID exists in another household. A 403 would be an information leak ("yes, that ID exists, but you can't access it").

Repository method

If findByIdAndHouseholdId doesn't exist yet, it's a one-liner Spring Data method: Optional<Ingredient> findByIdAndHouseholdId(UUID id, UUID householdId). Spring Data will generate the query automatically — no custom @Query needed.

Questions before implementing

Is Ingredient a shared global entity (e.g., a product catalog) or a household-scoped entity with its own household_id column? The fix assumes household-scoped. If ingredients can be shared across households (e.g., a global ingredient library), the model is different and the fix needs more thought.
Does ShoppingService.addItem() also create new ingredients inline (without an ingredientId)? If so, does that creation path also correctly associate the new ingredient with the current household?
Are there other places in the codebase where ingredientRepository.findById() is called without a household check? This might not be the only instance.

Broader audit

This pattern — findById without household scope — is worth a codebase-wide grep. Any repository's findById call that isn't paired with a household check is a potential IDOR. Related to #13.

## 🛠️ Backend Engineer Classic IDOR — simple to fix, important to catch. The fix itself is one line, but there are a few things to think through carefully. **The fix** Change: ```java ingredient = ingredientRepository.findById(request.ingredientId()) .orElseThrow(...); ``` To: ```java ingredient = ingredientRepository.findByIdAndHouseholdId(request.ingredientId(), householdId) .orElseThrow(() -> new ResourceNotFoundException("Ingredient not found")); ``` This should throw a 404, not a 403. Returning 404 is the right choice here — it avoids confirming to an attacker that the UUID exists in another household. A 403 would be an information leak ("yes, that ID exists, but you can't access it"). **Repository method** - If `findByIdAndHouseholdId` doesn't exist yet, it's a one-liner Spring Data method: `Optional<Ingredient> findByIdAndHouseholdId(UUID id, UUID householdId)`. Spring Data will generate the query automatically — no custom `@Query` needed. **Questions before implementing** - Is `Ingredient` a shared global entity (e.g., a product catalog) or a household-scoped entity with its own `household_id` column? The fix assumes household-scoped. If ingredients can be shared across households (e.g., a global ingredient library), the model is different and the fix needs more thought. - Does `ShoppingService.addItem()` also create new ingredients inline (without an `ingredientId`)? If so, does that creation path also correctly associate the new ingredient with the current household? - Are there other places in the codebase where `ingredientRepository.findById()` is called without a household check? This might not be the only instance. **Broader audit** - This pattern — `findById` without household scope — is worth a codebase-wide grep. Any repository's `findById` call that isn't paired with a household check is a potential IDOR. Related to #13.

marcel commented

2026-04-03 09:26:39 +02:00

🧪 QA Engineer

This is a textbook IDOR that needs a direct regression test. The fix is small but the test is what guarantees it never regresses.

Tests I'd add for this fix

Happy path (confirm fix doesn't break valid usage):

shouldAddItemWithValidIngredientFromSameHousehold() — provide an ingredientId that belongs to the user's household, assert item is created successfully.

Security regression tests:

shouldReturn404WhenIngredientIdBelongsToADifferentHousehold() — create an ingredient in household A, authenticate as household B user, attempt to add that ingredient ID to household B's shopping list, assert 404 (not 200, not 403, not 500).
shouldReturn404WhenIngredientIdDoesNotExist() — provide a random UUID that doesn't exist in any household, assert 404. (Verifies the error path works regardless of the isolation fix.)

Edge cases worth testing

shouldReturn400WhenIngredientIdIsNotAValidUUID() — malformed UUID in the request body.
shouldAddItemWithoutIngredientId() — if the ingredientId field is optional, verify the happy path works when it's omitted entirely.

How to structure the integration test

// Arrange
UUID householdAIngredientId = createIngredientInHouseholdA();
loginAsHouseholdBUser();

// Act
POST /shopping/items with { ingredientId: householdAIngredientId, ... }

// Assert
expect 404
expect no item created in household B's shopping list

Question

Does the test infrastructure support multi-household test data seeding easily? We'll need this pattern repeatedly for the broader household isolation work (#13). Worth investing in a reusable TestHouseholdFactory if not already present.

## 🧪 QA Engineer This is a textbook IDOR that needs a direct regression test. The fix is small but the test is what guarantees it never regresses. **Tests I'd add for this fix** Happy path (confirm fix doesn't break valid usage): - `shouldAddItemWithValidIngredientFromSameHousehold()` — provide an `ingredientId` that belongs to the user's household, assert item is created successfully. Security regression tests: - `shouldReturn404WhenIngredientIdBelongsToADifferentHousehold()` — create an ingredient in household A, authenticate as household B user, attempt to add that ingredient ID to household B's shopping list, assert 404 (not 200, not 403, not 500). - `shouldReturn404WhenIngredientIdDoesNotExist()` — provide a random UUID that doesn't exist in any household, assert 404. (Verifies the error path works regardless of the isolation fix.) **Edge cases worth testing** - `shouldReturn400WhenIngredientIdIsNotAValidUUID()` — malformed UUID in the request body. - `shouldAddItemWithoutIngredientId()` — if the `ingredientId` field is optional, verify the happy path works when it's omitted entirely. **How to structure the integration test** ``` // Arrange UUID householdAIngredientId = createIngredientInHouseholdA(); loginAsHouseholdBUser(); // Act POST /shopping/items with { ingredientId: householdAIngredientId, ... } // Assert expect 404 expect no item created in household B's shopping list ``` **Question** - Does the test infrastructure support multi-household test data seeding easily? We'll need this pattern repeatedly for the broader household isolation work (#13). Worth investing in a reusable `TestHouseholdFactory` if not already present.

marcel commented

2026-04-03 09:26:51 +02:00

🔒 Sable — Security Engineer

This is a confirmed IDOR (Insecure Direct Object Reference) — OWASP Top 10 #1 (Broken Access Control). The attack scenario in the issue is realistic and low-effort.

Attack scenario, spelled out

Attacker is a legitimate user of the app in household B.
Attacker obtains or guesses a UUID belonging to an ingredient in household A. UUIDs are not secret — they may be visible in URLs, response bodies, or logs.
Attacker sends POST /shopping/items with { "ingredientId": "<household-A-UUID>" }.
The ingredient from household A is now linked to household B's shopping list — cross-tenant data reference established.

Why this matters beyond the obvious

Even if the ingredient "data" isn't directly exposed, a cross-household reference corrupts the data model. If ingredient details are later fetched through the shopping list, they could expose household A's data (ingredient name, unit, custom notes, etc.) to household B.
If ingredients ever have richer metadata (dietary restrictions, allergen flags, cost data), this becomes a more serious privacy leak.

The fix is correct — 404 not 403
Returning 404 for a mismatched household is the right call. A 403 would confirm the UUID exists somewhere in the system, which is information leakage. The caller should not be able to distinguish "this ID doesn't exist" from "this ID exists but isn't yours."

Broader audit I'd require before closing

grep -r "findById" src/ across all repository usages — flag every one that isn't paired with a household scope check. This issue caught one instance; there may be others.
Are there similar patterns in RecipeService, MealPlanService, or IngredientService where a resource is looked up by ID alone?
Does the fix cover the write path too? If updateItem() or deleteItem() in ShoppingService also look up items by ID without household scope, those are separate IDORs.

Severity note
The issue rates this Medium. I'd keep that rating — the ingredient data itself isn't highly sensitive, but the pattern is. Any similar bug on a more sensitive entity (meal plans, household members, invite tokens) would be High.

## 🔒 Sable — Security Engineer This is a confirmed IDOR (Insecure Direct Object Reference) — OWASP Top 10 #1 (Broken Access Control). The attack scenario in the issue is realistic and low-effort. **Attack scenario, spelled out** 1. Attacker is a legitimate user of the app in household B. 2. Attacker obtains or guesses a UUID belonging to an ingredient in household A. UUIDs are not secret — they may be visible in URLs, response bodies, or logs. 3. Attacker sends `POST /shopping/items` with `{ "ingredientId": "<household-A-UUID>" }`. 4. The ingredient from household A is now linked to household B's shopping list — cross-tenant data reference established. **Why this matters beyond the obvious** - Even if the ingredient "data" isn't directly exposed, a cross-household reference corrupts the data model. If ingredient details are later fetched through the shopping list, they could expose household A's data (ingredient name, unit, custom notes, etc.) to household B. - If ingredients ever have richer metadata (dietary restrictions, allergen flags, cost data), this becomes a more serious privacy leak. **The fix is correct — 404 not 403** Returning 404 for a mismatched household is the right call. A 403 would confirm the UUID exists somewhere in the system, which is information leakage. The caller should not be able to distinguish "this ID doesn't exist" from "this ID exists but isn't yours." **Broader audit I'd require before closing** - `grep -r "findById" src/` across all repository usages — flag every one that isn't paired with a household scope check. This issue caught one instance; there may be others. - Are there similar patterns in `RecipeService`, `MealPlanService`, or `IngredientService` where a resource is looked up by ID alone? - Does the fix cover the write path too? If `updateItem()` or `deleteItem()` in `ShoppingService` also look up items by ID without household scope, those are separate IDORs. **Severity note** The issue rates this Medium. I'd keep that rating — the ingredient data itself isn't highly sensitive, but the pattern is. Any similar bug on a more sensitive entity (meal plans, household members, invite tokens) would be High.

marcel commented

2026-04-03 09:27:00 +02:00

🎨 Atlas — UI/UX Designer

Backend fix, but I want to flag the user-facing consequence of the 404 response once the fix is in.

The scenario from a user's perspective
In normal usage, users will never trigger this — they select ingredients from their own household's list through the UI. But if a user somehow ends up with a stale or invalid ingredientId (e.g., an ingredient that was deleted between when they opened the add-item dialog and when they submitted), they'll now get a 404.

What does the UI show on 404?

Does the add-item form in D1 currently handle a 404 response from POST /shopping/items? Or does it only handle success?
If we show a generic "something went wrong" message, that's technically correct but not helpful. A more useful message would be: "This ingredient is no longer available. Please select a different one."
The key UX question: should the form stay open with an error, or close and show a toast notification?

Design guidance for the error state

Keep the add-item form open on failure — don't lose the user's other input (item name, quantity, unit).
Show an inline error near the ingredient selector, not a full-page error.
Use --color-error for the error text, consistent with other form validation in the design system.
The message should guide the user toward a resolution, not just report failure.

Question for Kai

How does the current add-item form handle backend validation errors today? Is there an established pattern I should design against, or is this the first error case we're defining for this form?

## 🎨 Atlas — UI/UX Designer Backend fix, but I want to flag the user-facing consequence of the 404 response once the fix is in. **The scenario from a user's perspective** In normal usage, users will never trigger this — they select ingredients from their own household's list through the UI. But if a user somehow ends up with a stale or invalid `ingredientId` (e.g., an ingredient that was deleted between when they opened the add-item dialog and when they submitted), they'll now get a 404. **What does the UI show on 404?** - Does the add-item form in D1 currently handle a 404 response from `POST /shopping/items`? Or does it only handle success? - If we show a generic "something went wrong" message, that's technically correct but not helpful. A more useful message would be: "This ingredient is no longer available. Please select a different one." - The key UX question: should the form stay open with an error, or close and show a toast notification? **Design guidance for the error state** - Keep the add-item form open on failure — don't lose the user's other input (item name, quantity, unit). - Show an inline error near the ingredient selector, not a full-page error. - Use `--color-error` for the error text, consistent with other form validation in the design system. - The message should guide the user toward a resolution, not just report failure. **Question for Kai** - How does the current add-item form handle backend validation errors today? Is there an established pattern I should design against, or is this the first error case we're defining for this form?

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/mealprep#12