Catch possible ID mismatch when trying to update a view#26715
Conversation
| @Context SearchUser searchUser) { | ||
| // the ID from the path always has to match the id from the request body for an update. | ||
| // currently, the FE always uses it like that, so mismatches should only occur when using the API directly | ||
| // possible improvement could be removing the ID from the path and only have it in the body |
There was a problem hiding this comment.
possible improvement could be removing the ID from the path and only have it in the body
That would go against good REST principles for an update (a PUT call). For that the id has to be in the URL.
I recommend the other way around: Ignoring an id that might have been sent in the body.
There was a problem hiding this comment.
Ok, fair point. I removed the line with the possible improvement.... The body's id is only tested now to make sure that the request is really for the intended entity - but ignored otherwise.
|
Forgot to mention: This PR should have a test. |
kroepke
left a comment
kroepke
left a comment
There was a problem hiding this comment.
makes sense, i'll add a follow-on task for us to think about how to improve these types of checks to make it less likely to miss in the future.
noted & added |
5 participants
Description
This PR fixes an oversight in #26344
which we got notified by https://github.com/Graylog2/graylog2-server/security/advisories/GHSA-w997-cmqr-xrx6
We are now also making sure that for update, the ID from the entity matches the ID from the path. The ID is not enforced by the
@Validon the view entity as it's not necessary for creation. But for an update on an existing view, it is always present and our FE always puts it in the request.Motivation and Context
How Has This Been Tested?
manual testing, added a unit test
Screenshots (if appropriate):
Types of changes
Checklist: