diff options
author | Ed Tanous <edtanous@google.com> | 2021-02-09 22:43:14 +0300 |
---|---|---|
committer | Ed Tanous <ed@tanous.net> | 2021-02-12 22:04:06 +0300 |
commit | ae6595ef5349a290bd36d5da967190aa4a461355 (patch) | |
tree | fbf9a36485d46608844bd378620c44fe9154d8a7 /COMMON_ERRORS.md | |
parent | ae34c8e888aba0108042acddbcf155a5c1a48992 (diff) | |
download | bmcweb-ae6595ef5349a290bd36d5da967190aa4a461355.tar.xz |
Add 404 handling to COMMON_ERRORS.md
At least 50% of all patchsets I see adding a collection handler seem to
get this wrong, despite a small comment in the developing doc, lets add
a concrete example so that we can be sure this gets handled in the
future, and we have something to point at in code review.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I98c9e235019472d3e39a2c142b5a5aec4bca8f4e
Diffstat (limited to 'COMMON_ERRORS.md')
-rw-r--r-- | COMMON_ERRORS.md | 58 |
1 files changed, 58 insertions, 0 deletions
diff --git a/COMMON_ERRORS.md b/COMMON_ERRORS.md index 16c7908aa7..054d38c6f7 100644 --- a/COMMON_ERRORS.md +++ b/COMMON_ERRORS.md @@ -180,3 +180,61 @@ accuracy. While these processes are largely manual, they can mostly be conducted by a simple grep statement to search for urls in question. Doing the above makes the route handlers no longer greppable, and complicates bmcweb patchsets as a whole. + +### 11. Not responding to 404 +```C++ +BMCWEB_ROUTE("/myendpoint/<str>", + [](Request& req, Response& res, const std::string& id){ + crow::connections::systemBus->async_method_call( + [asyncResp](const boost::system::error_code ec, + const std::string& myProperty) { + if (ec) + { + messages::internalError(asyncResp->res); + return; + } + ... handle code + }, + "xyz.openbmc_project.Logging", + "/xyz/openbmc_project/mypath/" + id, + "xyz.MyInterface", "GetAll", ""); +}); +``` +All bmcweb routes should handle 404 (not found) properly, and return it where +appropriate. 500 internal error is not a substitute for this, and should be +only used if there isn't a more appropriate error code that can be returned. +This is important, because a number of vulnerability scanners attempt injection +attacks in the form of /myendpoint/foobar, or /myendpoint/#$*(%)&#%$)(*& in an +attempt to circumvent security. If the server returns 500 to any of these +requests, the security scanner logs it as an error for followup. While in +general these errors are benign, and not actually a real security threat, having +a clean security run allows maintainers to minimize the amount of time spent +triaging issues reported from these scanning tools. + +A inplementation of the above that handles 404 would look like: +```C++ +BMCWEB_ROUTE("/myendpoint/<str>", + [](Request& req, Response& res, const std::string& id){ + crow::connections::systemBus->async_method_call( + [asyncResp](const boost::system::error_code ec, + const std::string& myProperty) { + if (ec == <error code that gets returned by not found>){ + messages::resourceNotFound(res); + return; + } + if (ec) + { + messages::internalError(asyncResp->res); + return; + } + ... handle code + }, + "xyz.openbmc_project.Logging", + "/xyz/openbmc_project/mypath/" + id, + "xyz.MyInterface", "GetAll", ""); +}); +``` + +Note: A more general form of this rule is that no handler should ever return 500 +on a working system, and any cases where 500 is found, can immediately be +assumed to be [a bug in either the system, or bmcweb.](https://github.com/openbmc/bmcweb/blob/master/DEVELOPING.md#error-handling) |