Repository navigation
fix(resources): return 400 when path or source is missing - #2856
gtsteffaniak merged 1 commit into
Conversation
checkPermissions rejected an empty path or source with a bare fmt.Errorf, which ErrToStatus has no case for, so the default arm turned a malformed request into a 500. GET /api/resources?source=x&path= and the same call with no path at all both surfaced as a server error, leaving clients unable to tell a bad request from a real failure. Wrap both guards in ErrInvalidRequestParams, which ErrToStatus already maps to 400. One shared guard covers every caller: the resource handlers, the directory listing path, and WebDAV.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughMissing ChangesInvalid parameter handling
Estimated code review effort: 2 (Simple) | ~5 minutes Merge Risk: ⚪ Minimal · up to Malformed resource requests now return 400 instead of 500 without changing authorization or adding runtime dependencies. No actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes address issue
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
im not seeing any actual changes here... just adjusting the error text? |
No no the actual change is the both guards were returning a bare case errors.Is(err, libErrors.ErrInvalidRequestParams):
return http.StatusBadRequestso GET /api/resources with no path goes 500 -> 400. body was already fine ("path not provided"), it was just the status that was wrong which is what #2801 was about. |
|
ok that makes sense, I see that from the description now, I was just looking at the code and confused. This makes sense. While this is a behavior change for the API, I think its a useful bugfix that belongs in v2.0.3-beta so I will target that. |
Anytime, happy weekend :) |
Fixes #2801 —
GET /api/resourcesreturns 500 whenpathis empty or omitted.Problem
An empty or omitted resource
pathreached the shared permission guard, which returned a plain error.ErrToStatuscould not classify it and fell back to HTTP 500. A missingsourcefollowed the same path.Fix
Wrap both missing-parameter errors with the existing
ErrInvalidRequestParamssentinel. Every resource caller still uses the same guard, whileErrToStatusnow maps these malformed requests to HTTP 400.Deliberately not done
The issue's separate observations about users, cached permissions, and event logging are unrelated and remain unchanged.
User impact
API clients now receive a client-error status for malformed resource requests instead of mistaking them for server failures.
Verification
go test ./internal/web -run '^TestCheckPermissionsMissingParamsStatus$' -count=1— passed.GitHub Actions — all 22 repository checks passed, including backend/frontend tests and Playwright across the configured authentication modes.
git diff --check— passed.A clear description of why it was opened.
A short title that best describes the change.
Must pass unit and integration tests, which can be run checked locally prior to opening a PR.
Any additional details for functionality not covered by tests.