firewall: return graceful failure instead of 500 on rule not-found - #10430
firewall: return graceful failure instead of 500 on rule not-found#10430haxorton wants to merge 1 commit into
Conversation
moveRuleBefore (FilterController and FilterBaseController::moveRuleBeforeBase) and toggleRuleLogBase threw a UserException when the referenced rule uuid did not exist.
Because api.php renders every thrown exception as HTTP 500, a non-existent uuid produced a 500, whereas the inherited del/set/toggle verbs return {"result":"failed"} and these very methods already return ["status" => "error", ...] for an invalid request method.
Return that same ["status" => "error", ...] shape for the not-found case so the outcome is consistent and no longer surfaces as a server error
|
I'm not sure this is the way to go here. We were planning to increase the use of UserException() because it works on all GUI pages. Deferring to @AdSchellevis when he is back. |
|
Ah if that is the plan, then I am fine with that too :) |
|
The user exceptions are intentional indeed, but we might need to improve the error model a bit for consistency indeed. Let's keep this open for further discussion. A lot of these standard errors are silently ignored, which in most cases isn't an issue as these are hardly triggered anyway. We could consider adding some different exceptions and trapping them in the UI like the user-exception at some point for clarity. |
|
Anything I can support with? I really like opnsense and love to have it, so when I can do something please say so. |
|
@haxorton not yet, but thanks for the offer. |
|
added 99e49c5 to offer some typing for exceptions and replaced the ones in the firewall code with warnings. For consistency we might consider replacing some other callers as well. |
…ase template PR: #10430 Although we can debate about the http status codes (kept the standard one at 500, warning and info as 200), the idea is to add some glue for the frontend so it knows what we mean and can represent the dialog based on its type. (cherry picked from commit 99e49c5) (cherry picked from commit 5023639)
3 participants
Important notices
Before you submit a pull request, we ask you kindly to acknowledge the following:
If AI was used, please disclose:
Describe the problem
moveRuleBefore (FilterController and FilterBaseController::moveRuleBeforeBase) and toggleRuleLogBase threw a UserException when the referenced rule uuid did not exist.
Because api.php renders every thrown exception as HTTP 500, a non-existent uuid produced a 500, whereas the inherited del/set/toggle verbs return {"result":"failed"} and these very methods already return ["status" => "error", ...] for an invalid request method.
Describe the proposed solution
Return that same ["status" => "error", ...] shape for the not-found case so the outcome is consistent and no longer surfaces as a server error
Again, not something breaking but more a OCD topic to make it more streamlined.
Related issue
None found