Closed mwangggg closed 7 months ago
I think this should really be done on the server side also, and this is actually a broader change that affects more than just rule definitions.
Here we have an example of a POST endpoint that expects a form submission (URL-encoded or multipart) for creating a custom target. There is no @RestForm
for the entity's id
, so there is no way for a client to hit this endpoint and supply an ID that the server will try to (re)use. This is good - the ID should only ever be assigned by the server, or really by the database. In any case the client should not be allowed to specify an ID for a created entity.
Here we have a problem. This is an equivalent endpoint that expects a JSON payload body. This means the client is free to submit a JSON that includes { "id": 1234 }
and the API definition accepts it. This will be deserialized into a Target
instance that really has that field with that value set. The server will then try to persist that into the database. This should not be allowed. Either the server should silently scrub and ignore the id
field and allow it to be assigned by the database, or the server should refuse the request outright and respond with a 400/409 status before it even gets to the database. IMO the 400/409 response is the better, cleaner way.
For user convenience I think it's OK and a good idea to drop id
fields on the client UI side like this, since the user probably does not know or care that downloaded rules contain an id
. The user just wants to do the workflow we were doing when we discovered this bug - download a rule, then upload it to reuse later. If that fails because of an id
field that I don't care about it's annoying, and I have to go edit the JSON file and remove it myself. This way the primary user workflow is nice and smooth and the UI helps the user avoid doing silly things that result in a 400 response.
However, we shouldn't rely on our UI being the only client, so the server does need to be more proactive about checking requests.
Looks good. Please double check if there are any other UI workflows that perform a JSON POST like this and might also need to have
id
fields stripped off.
This should be the only user upload where stripping the id applies.
Welcome to Cryostat! 👋
Before contributing, make sure you have:
main
branch[chore, ci, docs, feat, fix, test]
To recreate commits with GPG signature
git fetch upstream && git rebase --force --gpg-sign upstream/main
Fixes: https://github.com/cryostatio/cryostat3/issues/261