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.

Sign up for GitHub

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

Jump to bottom

[#12901] Added the check for search service and moved the method to the correct file #13021

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

[#12901] Added the check for search service and moved the method to the correct file #13021

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
12d1d20
This is the fix for #12901. I added the check to enusre the search se…
traitsisgiorgos Apr 12, 2024
f19e93c
Removed unused import in AccountRequestsDbIT
traitsisgiorgos Apr 13, 2024
ecc6bbc
Merge branch 'master' into #branch_for_12901
traitsisgiorgos Apr 14, 2024
1ce6ab1
0 in the assertEquals changed with the correct value
traitsisgiorgos Apr 30, 2024
3868de8
Empty lines removed. Changes on AccountRequestSearchManager class in …
traitsisgiorgos May 8, 2024
c807729
Check if the searchService check cause the failing
traitsisgiorgos May 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 1 addition & 16 deletions src/it/java/teammates/it/storage/sqlapi/AccountRequestsDbIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,19 +192,4 @@ public void testSqlInjectionInDeleteAccountRequest() throws Exception {
AccountRequest actual = accountRequestDb.getAccountRequest(accountRequest.getEmail(), accountRequest.getInstitute());
assertEquals(accountRequest, actual);
}

@Test
public void testSqlInjectionSearchAccountRequestsInWholeSystem() throws Exception {
______TS("SQL Injection test in searchAccountRequestsInWholeSystem");

AccountRequest accountRequest = new AccountRequest("test@gmail.com", "name", "institute");
accountRequestDb.createAccountRequest(accountRequest);

String searchInjection = "institute'; DROP TABLE account_requests; --";
List<AccountRequest> actualInjection = accountRequestDb.searchAccountRequestsInWholeSystem(searchInjection);
assertEquals(0, actualInjection.size());

AccountRequest actual = accountRequestDb.getAccountRequest("test@gmail.com", "institute");
assertEquals(accountRequest, actual);
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's ensure the files have trailing newlines. That means also for the AccountRequestSearchIT.java file.

21 changes: 20 additions & 1 deletion src/it/java/teammates/it/storage/sqlsearch/AccountRequestSearchIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,4 +164,23 @@ private static void verifySearchResults(List<AccountRequest> actual,
AssertHelper.assertSameContentIgnoreOrder(Arrays.asList(expected), actual);
}

}
@Test
public void testSqlInjectionSearchAccountRequestsInWholeSystem() throws Exception {

if (!TestProperties.isSearchServiceActive()) {
return;
}

______TS("SQL Injection test in searchAccountRequestsInWholeSystem");

AccountRequest accountRequest = new AccountRequest("test@gmail.com", "name", "institute");
accountRequestsDb.createAccountRequest(accountRequest);

String searchInjection = "institute'; DROP TABLE account_requests; --";
List<AccountRequest> actualInjection = accountRequestsDb.searchAccountRequestsInWholeSystem(searchInjection);
assertEquals(0, actualInjection.size());
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the component test is failing here where 0 is expected whereas actual is 11, maybe you could check why is it returning such a value.

Copy link
Author

Choose a reason for hiding this comment

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

yes i am trying to fix it. Thanks for the feedback


AccountRequest actual = accountRequestsDb.getAccountRequest("test@gmail.com", "institute");
assertEquals(accountRequest, actual);
}
}
6 changes: 5 additions & 1 deletion src/main/java/teammates/storage/sqlsearch/AccountRequestSearchManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import org.apache.solr.client.solrj.SolrQuery;
import org.apache.solr.client.solrj.response.QueryResponse;
import org.apache.solr.client.solrj.util.ClientUtils;
import org.apache.solr.common.SolrDocument;

import teammates.common.exception.SearchServiceException;
Expand Down Expand Up @@ -50,7 +51,10 @@ void sortResult(List<AccountRequest> result) {
* Searches for account requests.
*/
public List<AccountRequest> searchAccountRequests(String queryString) throws SearchServiceException {
SolrQuery query = getBasicQuery(queryString);
// Escape special characters in the query string
String escapedQueryString = ClientUtils.escapeQueryChars(queryString);

SolrQuery query = getBasicQuery(escapedQueryString);
Comment on lines +54 to +57
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an interesting solution. From what I know, the only special characters here are the two - characters at the end (the SQL comment), so it is weird that it does anything to escape them. Does this actually get the tests to pass? I cannot tell because the GitHub Actions do not seem to be running. I suspect this is because of the conflicting files. Let's update this branch to the latest commit on the master branch and handle the conflicting files.

Regardless, I am also not sure if this can be accepted. Wouldn't this prevent a user from using those special characters when using the search functionality? We may want to see if we can find an alternative way to solve this instead.

Copy link
Author

Choose a reason for hiding this comment

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

you are right, it isn't passing the tests. if I update it all the changes will be lost

Copy link
Author

Choose a reason for hiding this comment

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

Since the comment have been escaped, it shouldn't return 0 responses?

Copy link
Author

Choose a reason for hiding this comment

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

if the response is an error the list size should be 0, but what if in our case we don't check it and it accepts the error as a response? That would cause a non-empty-list

Copy link
Contributor

Choose a reason for hiding this comment

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

if I update it all the changes will be lost

We cannot merge this into the master branch unless this is updated and there are no more conflicts. If the changes are lost when it is updated, then the changes should be made again after updating. Please update your branch by merging the master branch (up to the most recent commit) into your branch.


QueryResponse response = performQuery(query);
return convertDocumentToEntities(response.getResults());
Expand Down