Adds ownership verification to the delete handlers for env vars, labels, volumes, and ports. Previously, an authenticated user could delete any resource by ID regardless of which app ID was in the URL path (IDOR vulnerability).
Changes
internal/handlers/app.go: Added resource.AppID != appID check to HandleEnvVarDelete, HandleLabelDelete, HandleVolumeDelete, and HandlePortDelete
internal/handlers/handlers_test.go: Added 4 IDOR tests that create two apps, attach a resource to app1, then attempt deletion via app2 URL path
Fix
One-line change per handler — add || resource.AppID != appID to the existing nil/error check:
## Summary
Adds ownership verification to the delete handlers for env vars, labels, volumes, and ports. Previously, an authenticated user could delete any resource by ID regardless of which app ID was in the URL path (IDOR vulnerability).
## Changes
- `internal/handlers/app.go`: Added `resource.AppID != appID` check to `HandleEnvVarDelete`, `HandleLabelDelete`, `HandleVolumeDelete`, and `HandlePortDelete`
- `internal/handlers/handlers_test.go`: Added 4 IDOR tests that create two apps, attach a resource to app1, then attempt deletion via app2 URL path
## Fix
One-line change per handler — add `|| resource.AppID != appID` to the existing nil/error check:
```go
// Before:
if findErr != nil || envVar == nil {
// After:
if findErr != nil || envVar == nil || envVar.AppID != appID {
```
Closes #19
sneak
was assigned by clawbot2026-02-16 05:53:39 +01:00
Tests demonstrate that env vars, labels, volumes, and ports can be
deleted via another app's URL path without ownership checks.
All 4 tests fail, confirming the vulnerability described in #19.
Verify that the resource's AppID matches the URL path app ID before
allowing deletion. Without this check, any authenticated user could
delete resources belonging to any app by providing the target resource's
ID in the URL regardless of the app ID in the path (IDOR vulnerability).
Closes#19
Error Trace: /Volumes/nvme-ext/_OPENCLAW_STATE/repos/upaas/internal/handlers/handlers_test.go:526
Error Trace: /Volumes/nvme-ext/_OPENCLAW_STATE/repos/upaas/internal/handlers/handlers_test.go:486
Error: Not equal:
expected: 404
actual : 303
Test: TestDeleteEnvVarOwnershipVerification
Error Trace: /Volumes/nvme-ext/_OPENCLAW_STATE/repos/upaas/internal/handlers/handlers_test.go:567
Error: Not equal:
expected: 404
actual : 303
Test: TestDeleteVolumeOwnershipVerification
Error: Not equal:
expected: 404
actual : 303
Test: TestDeleteLabelOwnershipVerification
Error Trace: /Volumes/nvme-ext/_OPENCLAW_STATE/repos/upaas/internal/handlers/handlers_test.go:608
Error: Not equal:
expected: 404
actual : 303
Test: TestDeletePortOwnershipVerification
Error Trace: /Volumes/nvme-ext/_OPENCLAW_STATE/repos/upaas/internal/handlers/handlers_test.go:491
Error: Expected value not to be nil.
Test: TestDeleteEnvVarOwnershipVerification
Messages: env var should still exist after IDOR attempt
Error Trace: /Volumes/nvme-ext/_OPENCLAW_STATE/repos/upaas/internal/handlers/handlers_test.go:613
Error: Expected value not to be nil.
Test: TestDeletePortOwnershipVerification
Messages: port should still exist after IDOR attempt
Error Trace: /Volumes/nvme-ext/_OPENCLAW_STATE/repos/upaas/internal/handlers/handlers_test.go:531
Error: Expected value not to be nil.
Test: TestDeleteLabelOwnershipVerification
Messages: label should still exist after IDOR attempt
Error Trace: /Volumes/nvme-ext/_OPENCLAW_STATE/repos/upaas/internal/handlers/handlers_test.go:572
Error: Expected value not to be nil.
Test: TestDeleteVolumeOwnershipVerification
Messages: volume should still exist after IDOR attempt
--- FAIL: TestDeleteEnvVarOwnershipVerification (0.02s)
--- FAIL: TestDeletePortOwnershipVerification (0.02s)
--- FAIL: TestDeleteLabelOwnershipVerification (0.02s)
--- FAIL: TestDeleteVolumeOwnershipVerification (0.02s)
FAIL
FAIL git.eeqj.de/sneak/upaas/internal/handlers 0.281s
FAIL
All 4 ownership verification tests fail — IDOR confirmed.
## Test output: FAILING (tests only, no fix)
```
Error Trace: /Volumes/nvme-ext/_OPENCLAW_STATE/repos/upaas/internal/handlers/handlers_test.go:526
Error Trace: /Volumes/nvme-ext/_OPENCLAW_STATE/repos/upaas/internal/handlers/handlers_test.go:486
Error: Not equal:
expected: 404
actual : 303
Test: TestDeleteEnvVarOwnershipVerification
Error Trace: /Volumes/nvme-ext/_OPENCLAW_STATE/repos/upaas/internal/handlers/handlers_test.go:567
Error: Not equal:
expected: 404
actual : 303
Test: TestDeleteVolumeOwnershipVerification
Error: Not equal:
expected: 404
actual : 303
Test: TestDeleteLabelOwnershipVerification
Error Trace: /Volumes/nvme-ext/_OPENCLAW_STATE/repos/upaas/internal/handlers/handlers_test.go:608
Error: Not equal:
expected: 404
actual : 303
Test: TestDeletePortOwnershipVerification
Error Trace: /Volumes/nvme-ext/_OPENCLAW_STATE/repos/upaas/internal/handlers/handlers_test.go:491
Error: Expected value not to be nil.
Test: TestDeleteEnvVarOwnershipVerification
Messages: env var should still exist after IDOR attempt
Error Trace: /Volumes/nvme-ext/_OPENCLAW_STATE/repos/upaas/internal/handlers/handlers_test.go:613
Error: Expected value not to be nil.
Test: TestDeletePortOwnershipVerification
Messages: port should still exist after IDOR attempt
Error Trace: /Volumes/nvme-ext/_OPENCLAW_STATE/repos/upaas/internal/handlers/handlers_test.go:531
Error: Expected value not to be nil.
Test: TestDeleteLabelOwnershipVerification
Messages: label should still exist after IDOR attempt
Error Trace: /Volumes/nvme-ext/_OPENCLAW_STATE/repos/upaas/internal/handlers/handlers_test.go:572
Error: Expected value not to be nil.
Test: TestDeleteVolumeOwnershipVerification
Messages: volume should still exist after IDOR attempt
--- FAIL: TestDeleteEnvVarOwnershipVerification (0.02s)
--- FAIL: TestDeletePortOwnershipVerification (0.02s)
--- FAIL: TestDeleteLabelOwnershipVerification (0.02s)
--- FAIL: TestDeleteVolumeOwnershipVerification (0.02s)
FAIL
FAIL git.eeqj.de/sneak/upaas/internal/handlers 0.281s
FAIL
```
All 4 ownership verification tests fail — IDOR confirmed.
This PR fixes an IDOR (Insecure Direct Object Reference) vulnerability in the four resource deletion handlers: env vars, labels, volumes, and ports.
Fix Analysis (app.go)
Correct approach — each handler now verifies resource.AppID != appID before allowing deletion
Minimal change — 4 lines changed, one per handler, appended to existing guard clause
Consistent pattern — same fix applied uniformly to all four handlers
Matches existing convention — the deployment log handlers (HandleDeploymentLogsAPI, HandleDeploymentLogDownload) already use this exact pattern: deployment.AppID != appID
Test Analysis (handlers_test.go)
Proper IDOR test pattern — creates two apps, attaches resource to app1, attempts delete via app2 URL
Two-phase assertion — checks both HTTP 404 response AND verifies resource still exists in DB
All four resource types covered — env var, label, volume, port
Parallel-safe — each test uses its own DB instance
Non-blocking suggestions
Consider adding positive tests (deleting via correct app ID succeeds)
golangci-lint not available locally; CI should confirm
Verdict
LGTM — Ship it.
## Code Review (detailed)
**Reviewer:** clawbot (automated review)
### Summary
This PR fixes an IDOR (Insecure Direct Object Reference) vulnerability in the four resource deletion handlers: env vars, labels, volumes, and ports.
### Fix Analysis (app.go)
- **Correct approach** — each handler now verifies `resource.AppID != appID` before allowing deletion
- **Minimal change** — 4 lines changed, one per handler, appended to existing guard clause
- **Consistent pattern** — same fix applied uniformly to all four handlers
- **Matches existing convention** — the deployment log handlers (`HandleDeploymentLogsAPI`, `HandleDeploymentLogDownload`) already use this exact pattern: `deployment.AppID != appID`
### Test Analysis (handlers_test.go)
- **Proper IDOR test pattern** — creates two apps, attaches resource to app1, attempts delete via app2 URL
- **Two-phase assertion** — checks both HTTP 404 response AND verifies resource still exists in DB
- **All four resource types covered** — env var, label, volume, port
- **Parallel-safe** — each test uses its own DB instance
### Non-blocking suggestions
- Consider adding positive tests (deleting via correct app ID succeeds)
- `golangci-lint` not available locally; CI should confirm
### Verdict
**LGTM** — Ship it.
Merge conflicts resolved. Rebased on latest main and fixed the test file conflicts — both the existing TestHandleWebhookRejectsOversizedBody test and the new IDOR ownership verification tests are preserved and passing. All 5 relevant tests pass. Ready for re-review.
Merge conflicts resolved. Rebased on latest `main` and fixed the test file conflicts — both the existing `TestHandleWebhookRejectsOversizedBody` test and the new IDOR ownership verification tests are preserved and passing. All 5 relevant tests pass. Ready for re-review.
sneak
merged commit dcdecafc61 into main2026-02-16 06:12:52 +01:00
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.
Summary
Adds ownership verification to the delete handlers for env vars, labels, volumes, and ports. Previously, an authenticated user could delete any resource by ID regardless of which app ID was in the URL path (IDOR vulnerability).
Changes
internal/handlers/app.go: Addedresource.AppID != appIDcheck toHandleEnvVarDelete,HandleLabelDelete,HandleVolumeDelete, andHandlePortDeleteinternal/handlers/handlers_test.go: Added 4 IDOR tests that create two apps, attach a resource to app1, then attempt deletion via app2 URL pathFix
One-line change per handler — add
|| resource.AppID != appIDto the existing nil/error check:Closes #19
Test output: FAILING (tests only, no fix)
All 4 ownership verification tests fail — IDOR confirmed.
Test output: PASSING (tests + fix)
Full test suite: 129 PASS, 0 FAIL.
Linting:
golangci-lintnot installed locally;gofmtreports no formatting issues.Code Review
LGTM - fix is correct and well-tested.
Code Review (detailed)
Reviewer: clawbot (automated review)
Summary
This PR fixes an IDOR (Insecure Direct Object Reference) vulnerability in the four resource deletion handlers: env vars, labels, volumes, and ports.
Fix Analysis (app.go)
resource.AppID != appIDbefore allowing deletionHandleDeploymentLogsAPI,HandleDeploymentLogDownload) already use this exact pattern:deployment.AppID != appIDTest Analysis (handlers_test.go)
Non-blocking suggestions
golangci-lintnot available locally; CI should confirmVerdict
LGTM — Ship it.
@clawbot please fix the merge conflict with this test file
13d5467177to867cdf01abMerge conflicts resolved. Rebased on latest
mainand fixed the test file conflicts — both the existingTestHandleWebhookRejectsOversizedBodytest and the new IDOR ownership verification tests are preserved and passing. All 5 relevant tests pass. Ready for re-review.