Skip to content

Fix another batch of Sonar-identified issues #3148

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

Merged
merged 2 commits into from
May 26, 2020

Conversation

idodeclare
Copy link
Contributor

  • Sanitize several user-supplied strings.
  • Fix some scope issues.
  • Prohibit XML external access.

- Sanitize several user-supplied strings.
- Fix some scope issues.
- Prohibit XML external access.
@coveralls
Copy link

coveralls commented May 9, 2020

Pull Request Test Coverage Report for Build 5175

  • 72 of 98 (73.47%) changed or added relevant lines in 15 files are covered.
  • 4 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.02%) to 73.363%

Changes Missing Coverage Covered Lines Changed/Added Lines %
opengrok-indexer/src/main/java/org/opengrok/indexer/history/ClearCaseRepository.java 0 1 0.0%
opengrok-web/src/main/java/org/opengrok/web/api/v1/controller/SuggesterController.java 4 5 80.0%
opengrok-indexer/src/main/java/org/opengrok/indexer/search/QueryBuilder.java 2 4 50.0%
opengrok-indexer/src/main/java/org/opengrok/indexer/web/Laundromat.java 31 33 93.94%
suggester/src/main/java/org/opengrok/suggest/query/SuggesterPhraseQuery.java 1 3 33.33%
opengrok-indexer/src/main/java/org/opengrok/indexer/web/PageConfig.java 4 11 36.36%
opengrok-web/src/main/java/org/opengrok/web/AuthorizationFilter.java 0 11 0.0%
Files with Coverage Reduction New Missed Lines %
opengrok-web/src/main/java/org/opengrok/web/api/v1/controller/FileController.java 4 53.49%
Totals Coverage Status
Change from base Build 5169: 0.02%
Covered Lines: 37843
Relevant Lines: 51583

💛 - Coveralls

@@ -561,7 +554,6 @@ private void findFilelessChildren(SkeletonDirs skels, File directory) {

/**
* Counts segments arising from {@code File.separatorChar} or '\\'.
* @param path
Copy link
Member

Choose a reason for hiding this comment

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

why is this deleted ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Restored with a comment

if (value == null) {
return null;
}
return value.replaceAll("[\\n\\r\\t\\f]", "_");
Copy link
Member

Choose a reason for hiding this comment

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

reuse the String literal ?

Copy link
Member

Choose a reason for hiding this comment

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

also, userInput() and luceneQuery() can be refactored into using common underlying method ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes done


HashMap<String, String[]> safes = new HashMap<>();
for (Map.Entry<String, String[]> entry : map.entrySet()) {
String k = logging(entry.getKey());
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
String k = logging(entry.getKey());
String key = logging(entry.getKey());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK done

* Represents a container for sanitizing methods for avoiding classifications as
* taint bugs.
*/
public class LaunderUtil {
Copy link
Member

Choose a reason for hiding this comment

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

perhaps this class should be called Laundromat and have the launder as a method ? :-)

Copy link
Member

Choose a reason for hiding this comment

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

also, add a simple test ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You got it

@tarzanek
Copy link
Contributor

I could argue about naming of Laundromat and its methods, but I guess I'll just keep smiling
feel free to merge from my side

@idodeclare
Copy link
Contributor Author

Thank you for taking a look, Lubos 😁

@tarzanek
Copy link
Contributor

merging

@tarzanek tarzanek merged commit 0826ee0 into oracle:master May 26, 2020
@idodeclare
Copy link
Contributor Author

Thank you, Lubos

@idodeclare idodeclare deleted the bugfix/sonar_bugs branch May 27, 2020 02:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants