Verify resource ownership before deletion to prevent IDOR (closes #3) #8

clawbot · 2026-02-08T21:05:30+01:00

clawbot commented

2026-02-08 21:05:30 +01:00

No description provided.

sneak was assigned by clawbot

2026-02-08 21:05:30 +01:00

clawbot added 1 commit 2026-02-08 21:05:30 +01:00

fix: verify resource ownership before deletion to prevent IDOR (closes #3 ) 839d83ab8b

clawbot reviewed 2026-02-16 05:55:29 +01:00

clawbot left a comment

Review: PR#8 — Verify resource ownership before deletion (IDOR fix)

Overall: Important security fix, well implemented.

Positives

Fixes a real IDOR vulnerability — authenticated users could delete resources belonging to other apps by manipulating URL parameters.
Consistent pattern across all four resource types (env vars, labels, volumes, ports).
Returns 404 (not 403) for mismatched ownership — correct choice to avoid information leakage.
Good test coverage: the env var IDOR test creates two apps and verifies cross-app deletion is blocked.

Observations

Test covers only env vars: The test validates HandleEnvVarDelete but not labels, volumes, or ports. Since the fix is the same pattern repeated four times, this is acceptable but adding at least one more (e.g. ports) would catch copy-paste errors.
Lookup by ID across apps: The resource is first fetched by its own ID (not scoped to the app), then checked. This means the resource is still loaded from DB even on mismatch. A query scoped to both resource_id and app_id would be slightly more efficient and defensive, but the current approach is correct and clear.
No logging on IDOR attempt: Consider logging when ownership verification fails — it could indicate malicious activity. A Warn log with the requesting app ID and actual app ID would aid security monitoring.

Verdict: Approve. This is a clean, necessary security fix.

## Review: PR#8 — Verify resource ownership before deletion (IDOR fix) **Overall: Important security fix, well implemented.** ### Positives - Fixes a real IDOR vulnerability — authenticated users could delete resources belonging to other apps by manipulating URL parameters. - Consistent pattern across all four resource types (env vars, labels, volumes, ports). - Returns 404 (not 403) for mismatched ownership — correct choice to avoid information leakage. - Good test coverage: the env var IDOR test creates two apps and verifies cross-app deletion is blocked. ### Observations 1. **Test covers only env vars**: The test validates `HandleEnvVarDelete` but not labels, volumes, or ports. Since the fix is the same pattern repeated four times, this is acceptable but adding at least one more (e.g. ports) would catch copy-paste errors. 2. **Lookup by ID across apps**: The resource is first fetched by its own ID (not scoped to the app), then checked. This means the resource is still loaded from DB even on mismatch. A query scoped to both `resource_id` and `app_id` would be slightly more efficient and defensive, but the current approach is correct and clear. 3. **No logging on IDOR attempt**: Consider logging when ownership verification fails — it could indicate malicious activity. A `Warn` log with the requesting app ID and actual app ID would aid security monitoring. **Verdict: Approve.** This is a clean, necessary security fix.

sneak commented

2026-02-16 05:59:48 +01:00

is this a dupe of #28?

sneak commented

2026-02-16 05:59:53 +01:00

@clawbot

sneak removed their assignment 2026-02-16 05:59:59 +01:00

clawbot commented

2026-02-16 06:00:16 +01:00

Yes — #3 and #19 describe the same IDOR bug (missing ownership verification on env var/label/volume/port deletion). PR #28 (closes #19) is the newer version with updated tests against current main.

Recommendation: close this PR (#8) in favor of PR #28, and close #3 as duplicate of #19.

Yes — #3 and #19 describe the same IDOR bug (missing ownership verification on env var/label/volume/port deletion). PR #28 (closes #19) is the newer version with updated tests against current main. Recommendation: close this PR (#8) in favor of PR #28, and close #3 as duplicate of #19.

sneak commented

2026-02-16 06:28:21 +01:00

do it

sneak closed this pull request

2026-02-16 06:28:21 +01:00