Add type checks for API settings #880
Conversation
WantClue
force-pushed
the
dev-latest
branch
2 times, most recently
from
e09e1e8 to
daf1233
Compare
main/http_server/http_server.c
Outdated
| } | ||
|
|
||
| if (!result) { | ||
| ESP_LOGW(TAG, "Wrong API input type for setting '%s'", item->string); |
There was a problem hiding this comment.
This should return HTTP 400, and ideally the whole request should be ignored. I'm not 100% sure but it looks like it'll accept the values it can, and ignore the ones that are invalid.
There was a problem hiding this comment.
Yes, invalid items are ignored.
Ignoring the whole request would only work if the first item is invalid. Otherwise the whole request would have to be parsed first?
There was a problem hiding this comment.
There are now two differences with the pre-check generic loop:
- "overheat_mode" can also be set to values greater than 0. Is it better to remove it from the loop and check it manually?
- "coreVoltage" / "frequency": The generic pre-check protects against negative values (like Protect against negative frequency and voltage values #326) but the original implementation also excluded 0?
item->valueint > 0
Do we need an optional min_value/max_value check for each value?
There was a problem hiding this comment.
It's tricky and I'm not sure where to go with this. If we want to do it properly, we need to check for valid values on each item. Having the SettingsFormat struct makes this more difficult. For example, the bools only allow 0/1, even though they're stored in U16. The display field only has a specific set of strings that are valid. The rotation field of #1022 only should have 0/90/180/270. You also already pointed out voltage/frequency, and overheat_mode is a special case indeed, which can only be disabled through the API, not enabled, so 0 is the only valid option here.
Maybe the more complete way would be to first indeed to a pre-check, per field. Does the field exist, is it of the right type and is the parsed value actually valid, and if so, continue. Only after all fields have been checked, the update part is checking for the existence of the field and update the NVS store. That, or have intermediate fields, which might be even cleaner actually, as you don't have to parse it again.
There was a problem hiding this comment.
I think the latest changes address your suggestions.
- check for valid type, valid data type range and value range for each item using the generic approach
- check individual values or strings manually if necessary
- parse only once with cached values
- all errors of the request are logged and not just one
terratec
force-pushed
the
add_api_checks
branch
2 times, most recently
from
8108cd8 to
0aa2bfa
Compare
| } | ||
| break; | ||
| case NVS_TYPE_I32: | ||
| break; |
There was a problem hiding this comment.
NVS_TYPE_I32 is a valid case but the type range is the same.
Maybe I should add a check in case the json type item->valueint changes? Or at least a comment?
| // update NVS (if result is okay) and clean up | ||
| for (int i = 0; i < ARRAY_SIZE(settings); i++) { | ||
| if (settings[i].updated) { | ||
| if (result) { |
There was a problem hiding this comment.
Why not have the for loop inside the if (result)? Can't we skip the whole loop if result == false?
There was a problem hiding this comment.
For now the loop is crucial to free the string buffers (if used).
Check proper type for the API settings. Especially important for "fanspeed".
A JSON command
{"fanspeed": "50"}would be converted to 0 fan speed.Maybe the string notation is misleading in the wiki:
curl -X PATCH http://yourbitaxe/api/system \ -H "Content-Type: application/json" \ -d '{"fanspeed": "desired_speed_value"}'