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

traitsisgiorgos · 2024-04-14T10:20:33Z

I added the check to ensure the search service is on in the "testSqlInjectionSearchAccountRequestsInWholeSystem()" and i moved the method to the AccountRequestSearchIT.java.
fix for #12901

…rvice is on and i moved the method to the AccountRequestSearchIT.java

github-actions · 2024-04-14T10:20:44Z

Hi @traitsisgiorgos, thank you for your interest in contributing to TEAMMATES!
However, your PR does not appear to follow our contribution guidelines:

Title must start with the issue number the PR is fixing in square brackets, e.g. [#<issue-number>]
Description must reference the issue number the PR is fixing, e.g. Fixes #<issue-number> (or Part of #<issue-number> if the PR does not address the issue fully)

Please address the above before we proceed to review your PR.

github-actions · 2024-04-14T10:37:41Z

Hi @traitsisgiorgos, thank you for your interest in contributing to TEAMMATES!
However, your PR does not appear to follow our contribution guidelines:

Description must reference the issue number the PR is fixing, e.g. Fixes #<issue-number> (or Part of #<issue-number> if the PR does not address the issue fully)

Please address the above before we proceed to review your PR.

mingyuanc

Logic looks good but could you fix the lint errors? You can run ./gradlew lint to test the lint locally

mingyuanc · 2024-04-15T05:23:33Z

src/it/java/teammates/it/storage/sqlsearch/AccountRequestSearchIT.java

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


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.

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

jayasting98

A few changes may be necessary. In addition, like @mingyuanc said, we may want to run lint and fix the errors. Also, let's fix the merge conflicts with the master branch.

jayasting98 · 2024-05-08T06:50:57Z

src/it/java/teammates/it/storage/sqlsearch/AccountRequestSearchIT.java

+
+        String searchInjection = "institute'; DROP TABLE account_requests; --";
+        List<AccountRequest> actualInjection = accountRequestsDb.searchAccountRequestsInWholeSystem(searchInjection);
+        assertEquals(typicalBundle.accountRequests.size(), actualInjection.size());


Shouldn't the expected size still be zero though? I understand that it was changed because the assertion was failing. However, I feel like this search term should not result in some account request being returned, much less all of them. In other words, I feel like the assertion was failing because the actual value might be the one that is wrong, not the initial expected value of zero which seems correct to me.

Still, I am not very familiar with the search functionality. Perhaps another reviewer who is more familiar can chime in.

jayasting98 · 2024-05-08T06:53:41Z

src/it/java/teammates/it/storage/sqlsearch/AccountRequestSearchIT.java

+
+
+
+


These empty lines seem like they were not added intentionally. Similarly for the other added empty lines which can be found elsewhere. Can we have this removed?

jayasting98 · 2024-05-08T06:55:00Z

src/it/java/teammates/it/storage/sqlsearch/AccountRequestSearchIT.java

@@ -15,6 +15,7 @@
 import teammates.test.AssertHelper;
 import teammates.test.TestProperties;

+


Suggested change

jayasting98 · 2024-05-08T06:55:07Z

src/it/java/teammates/it/storage/sqlapi/AccountRequestsDbIT.java

@@ -10,6 +10,7 @@
 import teammates.storage.sqlapi.AccountRequestsDb;
 import teammates.storage.sqlentity.AccountRequest;

+


Suggested change

jayasting98 · 2024-05-08T07:15:42Z

src/it/java/teammates/it/storage/sqlsearch/AccountRequestSearchIT.java

+        ______TS("SQL Injection test in searchAccountRequestsInWholeSystem");
+
+        if (!TestProperties.isSearchServiceActive()) { 
+            return; 
+        } 


We should probably have the test segment divider after the check. If the search service is not active, we would not want it to display the string when the test is not going to execute anyway.

Also, we should not have any trailing spaces.

Suggested change

______TS("SQL Injection test in searchAccountRequestsInWholeSystem");

if (!TestProperties.isSearchServiceActive()) {

return;

}

if (!TestProperties.isSearchServiceActive()) {

return;

}

______TS("SQL Injection test in searchAccountRequestsInWholeSystem");

…searchAccountRequests method to escape special characters in the query string. Also expected value has been set to 0

jayasting98

Please update your branch. Also, here are some more comments.

jayasting98 · 2024-05-08T11:22:29Z

src/it/java/teammates/it/storage/sqlapi/AccountRequestsDbIT.java

-        assertEquals(accountRequest, actual);
-    }
-}
+}


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

jayasting98 · 2024-05-08T11:28:10Z

src/main/java/teammates/storage/sqlsearch/AccountRequestSearchManager.java

+        // Escape special characters in the query string
+        String escapedQueryString = ClientUtils.escapeQueryChars(queryString);
+
+        SolrQuery query = getBasicQuery(escapedQueryString);


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.

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

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

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

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.

traitsisgiorgos added 2 commits April 12, 2024 13:22

This is the fix for #12901. I added the check to enusre the search se…

12d1d20

…rvice is on and i moved the method to the AccountRequestSearchIT.java

Removed unused import in AccountRequestsDbIT

f19e93c

Merge branch 'master' into #branch_for_12901

ecc6bbc

traitsisgiorgos closed this Apr 14, 2024

traitsisgiorgos changed the title ~~[ #12901] Added the check for search service and moved the method to the correct file~~ [#12901] Added the check for search service and moved the method to the correct file Apr 14, 2024

traitsisgiorgos reopened this Apr 14, 2024

mingyuanc requested changes Apr 15, 2024

View reviewed changes

0 in the assertEquals changed with the correct value

1ce6ab1

traitsisgiorgos requested a review from mingyuanc April 30, 2024 12:54

jayasting98 mentioned this pull request May 7, 2024

[#12901] Check if the search service is active in account request search test #13101

Merged

jayasting98 reviewed May 8, 2024

View reviewed changes

Empty lines removed. Changes on AccountRequestSearchManager class in …

3868de8

…searchAccountRequests method to escape special characters in the query string. Also expected value has been set to 0

traitsisgiorgos requested a review from jayasting98 May 8, 2024 08:10

jayasting98 reviewed May 8, 2024

View reviewed changes

Check if the searchService check cause the failing

c807729

traitsisgiorgos requested a review from jayasting98 May 9, 2024 07:41

traitsisgiorgos closed this Jun 10, 2024

traitsisgiorgos deleted the #branch_for_12901 branch June 13, 2024 10:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

traitsisgiorgos commented Apr 14, 2024 •

edited

Loading

github-actions bot commented Apr 14, 2024

github-actions bot commented Apr 14, 2024

mingyuanc left a comment

mingyuanc Apr 15, 2024

traitsisgiorgos Apr 15, 2024

jayasting98 left a comment •

edited

Loading

jayasting98 May 8, 2024

jayasting98 May 8, 2024

jayasting98 May 8, 2024

jayasting98 May 8, 2024

jayasting98 May 8, 2024

jayasting98 left a comment

jayasting98 May 8, 2024

jayasting98 May 8, 2024

traitsisgiorgos May 8, 2024

traitsisgiorgos May 8, 2024

traitsisgiorgos May 8, 2024

jayasting98 May 10, 2024 •

edited

Loading

		@@ -15,6 +15,7 @@
		import teammates.test.AssertHelper;
		import teammates.test.TestProperties;

		@@ -10,6 +10,7 @@
		import teammates.storage.sqlapi.AccountRequestsDb;
		import teammates.storage.sqlentity.AccountRequest;

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

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

Conversation

traitsisgiorgos commented Apr 14, 2024 • edited Loading

github-actions bot commented Apr 14, 2024

github-actions bot commented Apr 14, 2024

mingyuanc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayasting98 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayasting98 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayasting98 May 10, 2024 • edited Loading

Choose a reason for hiding this comment

traitsisgiorgos commented Apr 14, 2024 •

edited

Loading

jayasting98 left a comment •

edited

Loading

jayasting98 May 10, 2024 •

edited

Loading