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

[#12048] SQL injection test for FeedbackQuestionsDbIT #12847

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

EuniceSim142
Copy link
Contributor

Part of #12048. Test for SQL injections in FeedbackQuestions database methods.


Tests not created for the following db methods as they do not accept string params (so they're type-checked at compile time):

  • getFeedbackQuestion
  • getFeedbackQuestionsForSession
  • getFeedbackQuestionsForGiverType (query uses feedbackSession's id which is UUID and giverType which is FeedbackParticipantType)
  • deleteFeedbackQuestion

@EuniceSim142 EuniceSim142 added the s.ToReview The PR is waiting for review(s) label Feb 26, 2024
Copy link
Contributor

@jayasting98 jayasting98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the other methods? Like getFeedbackQuestionsForSession, getFeedbackQuestionsForGiverType, etc.

Copy link
Contributor

@weiquu weiquu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@EuniceSim142 shall we test for the getFeedbackQuestionsForGiverType method as well? Just pass in a FeedbackSession that has injection strings in name / email / etc. I think the other methods that use UUID don't need to be tested. Do also fix the lint checks

@jayasting98 jayasting98 added this to the V9.0.0-beta.0 milestone Feb 29, 2024
@jayasting98 jayasting98 added the c.Task Other non-user-facing works, e.g. refactoring, adding tests label Feb 29, 2024
@EuniceSim142
Copy link
Contributor Author

@EuniceSim142 shall we test for the getFeedbackQuestionsForGiverType method as well? Just pass in a FeedbackSession that has injection strings in name / email / etc. I think the other methods that use UUID don't need to be tested. Do also fix the lint checks

I don't think this works as having the sqli string in name and email of feedback session won't inject it into the sql query:

cq.select(root)
                .where(cb.and(
                        cb.equal(fqJoin.get("id"), feedbackSession.getId()),
                        cb.equal(root.get("giverType"), giverType)));

the query only uses giver type and id which are typed, and name and email which contain the sqli are not used in the where clause.

Copy link
Contributor

@jayasting98 jayasting98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, in that case, I think it's fine then.

@weiquu weiquu removed this from the V9.0.0-beta.0 milestone Mar 10, 2024
Copy link
Contributor

@weiquu weiquu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jayasting98
Copy link
Contributor

@EuniceSim142 Let's get the checks to pass and resolve the merge conflict before we merge.

@jayasting98 jayasting98 added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels Mar 19, 2024
@jayasting98 jayasting98 added s.DoNotMerge The pull request may be ready to merge but should not be, pending some event and removed s.FinalReview The PR is ready for final review labels Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.Task Other non-user-facing works, e.g. refactoring, adding tests s.DoNotMerge The pull request may be ready to merge but should not be, pending some event
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants