Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(server): should validate request payload: ensure id is not specified #291

Merged
merged 6 commits into from
Mar 7, 2024

Conversation

aali309
Copy link
Contributor

@aali309 aali309 commented Feb 16, 2024

Welcome to Cryostat3! 👋

Before contributing, make sure you have:

  • Read the contributing guidelines
  • Linked a relevant issue which this PR resolves
  • Linked any other relevant issues, PR's, or documentation, if any
  • Resolved all conflicts, if any
  • Rebased your branch PR on top of the latest upstream main branch
  • Attached at least one of the following labels to the PR: [chore, ci, docs, feat, fix, test]
  • Signed all commits using a GPG signature

To recreate commits with GPG signature git fetch upstream && git rebase --force --gpg-sign upstream/main


Fixes: #278

Description of the change:

Server should validates the request payload and ensures that the id field is not specified. If fails validation, server responds with a 400/409 status before it even gets to the database.

Motivation for the change:

Mentioned by @andrewazores Link

How to manually test:

  1. Run CRYOSTAT_IMAGE=quay.io... sh smoketest.sh...
  2. ...

@andrewazores
Copy link
Member

I think it's worthwhile to do the same check everywhere else that we accept JSON from the client. You could search the codebase for @POST, @PATCH, and @Consumes(MediaType.APPLICATION_JSON) and that should be a pretty good list of all the places to check.

For example:

https://github.com/cryostatio/cryostat3/blob/afd746f44444e3e744b9423b4377dff4ee6f848c/src/main/java/io/cryostat/rules/Rules.java#L65

Another possibility (that I haven't read into too much yet) would be to use a request filter:

https://quarkus.io/guides/resteasy-reactive#request-or-response-filters

This in theory lets you install a hook that will get called before every HTTP request is handled, so you can inspect it and perform validations. For example, you could check for Content-Type: application/json headers, and if that's found then check for the id key in the request body. This way it can be done as one global handler instead of needing to be done for each resource type that we accept JSON payloads for (in theory).

@aali309
Copy link
Contributor Author

aali309 commented Mar 6, 2024

/build_test

Copy link

github-actions bot commented Mar 6, 2024

Workflow started at 3/6/2024, 12:41:08 PM. View Actions Run.

Copy link

github-actions bot commented Mar 6, 2024

CI build and push: All tests pass ✅ (JDK21)
https://github.com/cryostatio/cryostat3/actions/runs/8176393737

@aali309 aali309 requested a review from andrewazores March 6, 2024 17:44
Copy link

github-actions bot commented Mar 6, 2024

CI build and push: At least one test failed ❌ (JDK17)
https://github.com/cryostatio/cryostat3/actions/runs/8176393737

@aali309 aali309 force-pushed the removeIdfromUploadedRule branch 3 times, most recently from 6a7616f to a5c9532 Compare March 6, 2024 18:02
@aali309 aali309 force-pushed the removeIdfromUploadedRule branch from a5c9532 to 7be94a1 Compare March 6, 2024 18:03
@aali309
Copy link
Contributor Author

aali309 commented Mar 6, 2024

/build_test

Copy link

github-actions bot commented Mar 6, 2024

Workflow started at 3/6/2024, 1:04:55 PM. View Actions Run.

Copy link

github-actions bot commented Mar 6, 2024

CI build and push: All tests pass ✅ (JDK21)
https://github.com/cryostatio/cryostat3/actions/runs/8176667627

Copy link

github-actions bot commented Mar 6, 2024

CI build and push: All tests pass ✅ (JDK17)
https://github.com/cryostatio/cryostat3/actions/runs/8176667627

@aali309
Copy link
Contributor Author

aali309 commented Mar 7, 2024

/build_test

Copy link

github-actions bot commented Mar 7, 2024

Workflow started at 3/6/2024, 8:50:51 PM. View Actions Run.

Copy link

github-actions bot commented Mar 7, 2024

CI build and push: All tests pass ✅ (JDK21)
https://github.com/cryostatio/cryostat3/actions/runs/8181684195

Copy link

github-actions bot commented Mar 7, 2024

CI build and push: All tests pass ✅ (JDK17)
https://github.com/cryostatio/cryostat3/actions/runs/8181684195

@andrewazores
Copy link
Member

/build_test

Copy link

github-actions bot commented Mar 7, 2024

Workflow started at 3/7/2024, 11:46:40 AM. View Actions Run.

Copy link

github-actions bot commented Mar 7, 2024

CI build and push: All tests pass ✅ (JDK21)
https://github.com/cryostatio/cryostat3/actions/runs/8191765923

Copy link

github-actions bot commented Mar 7, 2024

CI build and push: All tests pass ✅ (JDK17)
https://github.com/cryostatio/cryostat3/actions/runs/8191765923

@andrewazores andrewazores merged commit 7589a98 into cryostatio:main Mar 7, 2024
8 checks passed
@aali309 aali309 deleted the removeIdfromUploadedRule branch March 7, 2024 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Bug] Server should check JSON POST requests for id (or other properties) and respond HTTP 400
2 participants