core: services: bag of holding: protect against bad API calls

[ES-Alexander]

Current behaviour

It's possible to break the bag by putting in a poorly formed set API call, e.g. by having a trailing slash on the path (e.g. .../bag/v1.0/set/path/), which results in things like

  "path": {
    "": {
      "variable": 9
    }
  }

(and causes failed subsequent get requests), instead of the desired .../bag/v1.0/set/path ->

  "path": {
    "variable": 9
  }

Expected or desired behaviour

1. Where possible, bag should protect itself from API calls that can break its usage for subsequent calls
2. If practical bag should try to fix broken calls into something more functional (e.g. by stripping trailing slashes), or at least reject them so they don't break it for the other services

Prerequisites

• [X] I have checked to make sure that a similar request has not already been filed or fixed.

[rafaellehmkuhl]

I have reached the same problem.

In one of my Cockpit syncs, somewhat happened and the bag-of-holdings json file got broke, which caused the service to die till I stopped it and deleted it's cache.

broke_bag.json

This seems like a critical problem to me. Any service relying in the bag of holdings can and will be affected if anything goes wrong, and there's no user-friendly way to fix it.

[ES-Alexander]

Your posted json seems to be cut off early, so perhaps failed partway through writing or something?

As discussed in the meeting, it would be nice if the service keeps a temporary "always valid" backup file of the current state during request processing, and checks whether each attempted change to the state fails to be read back after saving, in which case it can roll back to the backup and reject the request.

If it restarts while writing or something then when it attempts to start up again it can load the backup file if it fails to load the primary file.

This seems like a critical problem to me.

I definitely agree - several other services rely on the bag, so having simple ways to break it is problematic.

[Williangalvani]

I believe this was fixed by https://github.com/bluerobotics/BlueOS/pull/2761

[ES-Alexander]

@Williangalvani does that actually prevent poisoning the current settings with an API call, or is it just a band-aid that fails more gracefully when the settings have become invalid (e.g. by restoring to default settings or something)?

[Williangalvani]

@Williangalvani does that actually prevent poisoning the current settings with an API call, or is it just a band-aid that fails more gracefully when the settings have become invalid (e.g. by restoring to default settings or something)?

the json.loads() call will fail and raise an exception if our new data in invalid, which will prevent saving it and cause the http request to fail