-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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 AccountRequestsDbIT #12788
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, other than 1 thing.
src/it/java/teammates/it/storage/sqlapi/AccountRequestsDbIT.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be testing the UPDATE
and DELETE
queries too? I mean like using the updateAccountRequest
and deleteAccountRequest
methods.
src/it/java/teammates/it/storage/sqlapi/AccountRequestsDbIT.java
Outdated
Show resolved
Hide resolved
Updated with these tests! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems okay to me. Thanks!
String emailInjection = "email'/**/OR/**/1=1/**/@gmail.com"; | ||
String nameInjection = "name'; DROP TABLE account_requests; --"; | ||
String instituteInjection = "institute'; DROP TABLE account_requests; --"; | ||
AccountRequest accountRequestInjection = new AccountRequest(emailInjection, nameInjection, instituteInjection); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Actually, I should have combined everything in one shot like you did.
accountRequestDb.createAccountRequest(accountRequest); | ||
|
||
String searchInjection = "institute'; DROP TABLE account_requests; --"; | ||
List<AccountRequest> actualInjection = accountRequestDb.searchAccountRequestsInWholeSystem(searchInjection); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this use SQL? I thought it's Solr. Still, I guess there's no harm, and it's possible the implementation may change to use SQL, but I'm not sure it's likely, even if PostgreSQL can do full-text search.
Part of #12048