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.
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.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
clawbot
Review: PR#8 — Verify resource ownership before deletion (IDOR fix)
Overall: Important security fix, well implemented.
Positives
Observations
HandleEnvVarDeletebut 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.resource_idandapp_idwould be slightly more efficient and defensive, but the current approach is correct and clear.Warnlog with the requesting app ID and actual app ID would aid security monitoring.Verdict: Approve. This is a clean, necessary security fix.
is this a dupe of #28?
@clawbot
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.
do it
Pull request closed