Skip to content

Commit

Permalink
scan rules: Clean code tweaks
Browse files Browse the repository at this point in the history
- Add static modifier where applicable.
- CHANGELOG > Add maintenance note (if there wasn't already one
present).
- pscanrules > Made resource message methods private again where example
alerts have been implemented, or removed them where there was only a
single usage (inlining the Contstant resource message usage).
  • Loading branch information
kingthorin committed Aug 20, 2024
1 parent 2e960a0 commit 97eff58
Show file tree
Hide file tree
Showing 100 changed files with 245 additions and 521 deletions.
3 changes: 2 additions & 1 deletion addOns/ascanrules/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ All notable changes to this add-on will be documented in this file.
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).

## Unreleased

### Changed
- Maintenance changes.

## [67] - 2024-07-22

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ public int getWascId() {
return 7;
}

private String randomCharacterString(int length) {
private static String randomCharacterString(int length) {
StringBuilder sb1 = new StringBuilder(length + 1);
int counter = 0;
int character = 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,7 @@ public int getRisk() {
return Alert.RISK_HIGH;
}

private String getOtherInfo(TestType testType, String testValue) {
private static String getOtherInfo(TestType testType, String testValue) {
return Constant.messages.getString(
MESSAGE_PREFIX + "otherinfo." + testType.getNameKey(), testValue);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ public String getReference() {
return Constant.messages.getString(MESSAGE_PREFIX + "refs");
}

private void checkIfDirectory(HttpMessage msg) throws URIException {
private static void checkIfDirectory(HttpMessage msg) throws URIException {

URI uri = msg.getRequestHeader().getURI();
uri.setQuery(null);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,7 @@ private static boolean isRedirectHost(String value, boolean escaped) throws URIE
* @param msg the current message where reflected redirection should be check into
* @return get back the redirection type if exists
*/
private int isRedirected(String payload, HttpMessage msg) {
private static int isRedirected(String payload, HttpMessage msg) {

// (1) Check if redirection by "Location" header
// http://en.wikipedia.org/wiki/HTTP_location
Expand Down Expand Up @@ -471,7 +471,7 @@ private static boolean isRedirectPresent(Pattern pattern, String value) {
* @param type the redirection type
* @return a string representing the reason of this redirection
*/
private String getRedirectionReason(int type) {
private static String getRedirectionReason(int type) {
switch (type) {
case REDIRECT_LOCATION_HEADER:
return Constant.messages.getString(MESSAGE_PREFIX + "reason.location.header");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ public String getReference() {
return Constant.messages.getString(MESSAGE_PREFIX + "refs");
}

private String getError(char c) {
private static String getError(char c) {
return Constant.messages.getString(MESSAGE_PREFIX + "error" + c);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ private String getEmptyValueResponse(String paramName) throws IOException {
* @param value the value that need to be checked
* @return true if it seems to be encrypted
*/
private boolean isEncrypted(byte[] value) {
private static boolean isEncrypted(byte[] value) {

// Make sure we have a reasonable sized string
// (encrypted strings tend to be long, and short strings tend to break our numbers)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -608,7 +608,7 @@ private boolean sendAndCheckPayload(
return false;
}

private String getContentsToMatch(HttpMessage message) {
private static String getContentsToMatch(HttpMessage message) {
return message.getResponseHeader().isHtml()
? StringEscapeUtils.unescapeHtml4(message.getResponseBody().toString())
: message.getResponseHeader().toString() + message.getResponseBody().toString();
Expand Down Expand Up @@ -700,7 +700,7 @@ public String match(String contents) {
return matchWinDirectories(contents);
}

private String matchNixDirectories(String contents) {
private static String matchNixDirectories(String contents) {
Pattern procPattern =
Pattern.compile("(?:^|\\W)proc(?:\\W|$)", Pattern.CASE_INSENSITIVE);
Pattern etcPattern = Pattern.compile("(?:^|\\W)etc(?:\\W|$)", Pattern.CASE_INSENSITIVE);
Expand All @@ -727,7 +727,7 @@ private String matchNixDirectories(String contents) {
return null;
}

private String matchWinDirectories(String contents) {
private static String matchWinDirectories(String contents) {
if (contents.contains("Windows")
&& Pattern.compile("Program\\sFiles").matcher(contents).find()) {
return "Windows";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ private HttpMessage createHttpMessage(URI uri) throws HttpMalformedHeaderExcepti
* @return
* @throws URIException
*/
private URI getClassURI(URI hostURI, String classname) throws URIException {
private static URI getClassURI(URI hostURI, String classname) throws URIException {
return new URI(
hostURI.getScheme()
+ "://"
Expand All @@ -288,7 +288,7 @@ private URI getClassURI(URI hostURI, String classname) throws URIException {
false);
}

private URI getPropsFileURI(URI hostURI, String propsfilename) throws URIException {
private static URI getPropsFileURI(URI hostURI, String propsfilename) throws URIException {
return new URI(
hostURI.getScheme()
+ "://"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,11 +76,11 @@ public String getDescription() {
return Constant.messages.getString("ascanrules.spring4shell.desc");
}

private boolean is400Response(HttpMessage msg) {
private static boolean is400Response(HttpMessage msg) {
return !msg.getResponseHeader().isEmpty() && msg.getResponseHeader().getStatusCode() == 400;
}

private void setGetPayload(HttpMessage msg, String payload) throws URIException {
private static void setGetPayload(HttpMessage msg, String payload) throws URIException {
msg.getRequestHeader().setMethod("GET");
URI uri = msg.getRequestHeader().getURI();
String query = uri.getEscapedQuery();
Expand All @@ -92,7 +92,7 @@ private void setGetPayload(HttpMessage msg, String payload) throws URIException
uri.setEscapedQuery(query);
}

private void setPostPayload(HttpMessage msg, String payload) {
private static void setPostPayload(HttpMessage msg, String payload) {
msg.getRequestHeader().setMethod("POST");
String body = msg.getRequestBody().toString();
if (body.isEmpty()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,11 @@ private enum PayloadHandling {
CONCAT_PATH
};

private NanoServerHandler createHttpRedirectHandler(String path, String header) {
private static NanoServerHandler createHttpRedirectHandler(String path, String header) {
return createHttpRedirectHandler(path, header, PayloadHandling.NEITHER);
}

private NanoServerHandler createHttpRedirectHandler(
private static NanoServerHandler createHttpRedirectHandler(
String path, String header, PayloadHandling payloadHandling) {
return new NanoServerHandler(path) {
@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ void checkNoPathsHaveLeadingSlash() {
}
}

private void assertNoLeadingSlash(String message, String path) {
private static void assertNoLeadingSlash(String message, String path) {
assertThat(message.replace(REPLACE_TOKEN, path), !path.startsWith("/"), is(true));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,7 @@ void shouldAlertOnlyIfCertainTagValuesArePresent()
assertThat(alert.getConfidence(), equalTo(Alert.CONFIDENCE_MEDIUM));
}

private NanoServerHandler createNanoHandler(
private static NanoServerHandler createNanoHandler(
String path, NanoHTTPD.Response.IStatus status, String responseBody) {
return new NanoServerHandler(path) {
@Override
Expand Down
1 change: 1 addition & 0 deletions addOns/ascanrulesAlpha/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
## Unreleased
### Changed
- Update minimum ZAP version to 2.15.0.
- Maintenance changes.

### Fixed
- Alert text for various rules has been updated to more consistently use periods and spaces in a uniform manner.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,6 @@ public String getDescription() {
return Constant.messages.getString(MESSAGE_PREFIX + "desc");
}

private String getOtherInfo() {
return Constant.messages.getString(MESSAGE_PREFIX + "other");
}

@Override
public String getSolution() {
return Constant.messages.getString(MESSAGE_PREFIX + "soln");
Expand Down Expand Up @@ -159,7 +155,7 @@ public void scan(HttpMessage msg, String param, String value) {
.setConfidence(Alert.CONFIDENCE_MEDIUM)
.setParam(param)
.setAttack(attack)
.setOtherInfo(getOtherInfo())
.setOtherInfo(Constant.messages.getString(MESSAGE_PREFIX + "other"))
.setEvidence(evidence)
.setMessage(testMsg)
.raise();
Expand Down Expand Up @@ -194,7 +190,7 @@ private String doesResponseContainString(HttpBody body, String str) {
return null;
}

private List<String> loadFile(String file) {
private static List<String> loadFile(String file) {
/*
* ZAP will have already extracted the file from the add-on and put it underneath the 'ZAP home' directory
*/
Expand Down
1 change: 1 addition & 0 deletions addOns/ascanrulesBeta/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
- The following scan rules now include example alert functionality for documentation generation purposes (Issue 6119):
- Expression Language Injection
- Cookie Slack Detector
- Removed misleading "Attack" from the Proxy Disclosure scan rule alerts (Issue 8556).

### Fixed
- Potential false positives in the Source Code Disclosure - File Inclusion scan rule when responses are empty or the original message resulted in an error to start with (Issue 8517).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -426,7 +426,7 @@ public List<Alert> getExampleAlerts() {
.build());
}

private boolean isEmptyResponse(byte[] response) {
private static boolean isEmptyResponse(byte[] response) {
return response.length == 0;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ public TreeSet<HtmlParameter> getParams(Source s, List<Element> inputTags) {
* @param url found in the body of the targeted page
* @return a hashmap of the query string
*/
private Map<String, List<String>> getUrlParameters(String url) {
private static Map<String, List<String>> getUrlParameters(String url) {
Map<String, List<String>> params = new HashMap<>();

if (url != null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ public String getReference() {
return Constant.messages.getString(MESSAGE_PREFIX + "refs");
}

private String getError(char c) {
private static String getError(char c) {
return Constant.messages.getString(MESSAGE_PREFIX + "error" + c);
}

Expand Down Expand Up @@ -145,7 +145,7 @@ public Map<String, String> getAlertTags() {
return ALERT_TAGS;
}

private String randomIntegerString(int length) {
private static String randomIntegerString(int length) {

int numbercounter = 0;
int character = 0;
Expand All @@ -169,7 +169,7 @@ private String randomIntegerString(int length) {
return sb1.toString();
}

private String singleString(int length, char c) // Single Character String
private static String singleString(int length, char c) // Single Character String
{

int numbercounter = 0;
Expand Down Expand Up @@ -241,7 +241,7 @@ private AlertBuilder buildAlert(
.setUri(url)
.setParam(param)
.setAttack(attack)
.setOtherInfo(this.getError(type))
.setOtherInfo(IntegerOverflowScanRule.getError(type))
.setEvidence(evidence);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -752,7 +752,6 @@ public void scan() {
Constant.messages.getString(
MESSAGE_PREFIX + "desc",
step2numberOfNodes - 1 + silentProxySet.size()))
.setAttack(getAttack())
.setOtherInfo(extraInfo)
.setMessage(getBaseMsg())
.raise();
Expand All @@ -765,18 +764,14 @@ public void scan() {
}
}

private String getPath(URI uri) {
private static String getPath(URI uri) {
String path = uri.getEscapedPath();
if (path != null) {
return path;
}
return "/";
}

private String getAttack() {
return Constant.messages.getString(MESSAGE_PREFIX + "attack");
}

@Override
public int getRisk() {
return Alert.RISK_MEDIUM;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -645,7 +645,7 @@ private AlertBuilder buildAlert(String attack, String otherInfo, String evidence
.setEvidence(evidence);
}

private Matcher matchStyles(String body) {
private static Matcher matchStyles(String body) {
// remove all " and ' for proper matching url('somefile.png')
String styleBody = body.replaceAll("['\"]", "");
return STYLE_URL_LOAD.matcher(styleBody);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1317,7 +1317,7 @@ private static void logSessionFixation(
* @param cookieName
* @return the HtmlParameter representing the cookie, or null if no matching cookie was found
*/
private HtmlParameter getResponseCookie(HttpMessage message, String cookieName) {
private static HtmlParameter getResponseCookie(HttpMessage message, String cookieName) {
TreeSet<HtmlParameter> cookieBackParams = message.getResponseHeader().getCookieParams();
if (cookieBackParams.isEmpty()) {
// no cookies
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ private AlertBuilder createAlert(int risk, String otherInfo) {
return newAlert().setRisk(risk).setConfidence(Alert.CONFIDENCE_LOW).setOtherInfo(otherInfo);
}

private StringBuilder createOtherInfoText(
private static StringBuilder createOtherInfoText(
Set<String> cookiesThatMakeADifference, Set<String> cookiesThatDoNOTMakeADifference) {

StringBuilder otherInfoBuff =
Expand All @@ -228,15 +228,15 @@ private StringBuilder createOtherInfoText(
return otherInfoBuff;
}

private void listCookies(Set<String> cookieSet, StringBuilder otherInfoBuff) {
private static void listCookies(Set<String> cookieSet, StringBuilder otherInfoBuff) {
Iterator<String> itYes = cookieSet.iterator();
while (itYes.hasNext()) {
formatCookiesList(otherInfoBuff, itYes);
}
otherInfoBuff.append(getEOL());
}

private int calculateRisk(
private static int calculateRisk(
Set<String> cookiesThatDoNOTMakeADifference, StringBuilder otherInfoBuff) {
int riskLevel = Alert.RISK_INFO;
for (String cookie : cookiesThatDoNOTMakeADifference) {
Expand All @@ -252,35 +252,36 @@ private int calculateRisk(
return riskLevel;
}

private String getSessionDestroyedText(String cookie) {
private static String getSessionDestroyedText(String cookie) {
return Constant.messages.getString("ascanbeta.cookieslack.session.destroyed", cookie);
}

private String getAffectResponseYes() {
private static String getAffectResponseYes() {
return Constant.messages.getString("ascanbeta.cookieslack.affect.response.yes");
}

private String getAffectResponseNo() {
private static String getAffectResponseNo() {
return Constant.messages.getString("ascanbeta.cookieslack.affect.response.no");
}

private String getSeparator() {
private static String getSeparator() {
return Constant.messages.getString("ascanbeta.cookieslack.separator");
}

private String getEOL() {
private static String getEOL() {
return Constant.messages.getString("ascanbeta.cookieslack.endline");
}

private void formatCookiesList(StringBuilder otherInfoBuff, Iterator<String> cookieIterator) {
private static void formatCookiesList(
StringBuilder otherInfoBuff, Iterator<String> cookieIterator) {

otherInfoBuff.append(cookieIterator.next());
if (cookieIterator.hasNext()) {
otherInfoBuff.append(getSeparator());
}
}

private String getSessionCookieWarning(String cookie) {
private static String getSessionCookieWarning(String cookie) {
return Constant.messages.getString("ascanbeta.cookieslack.session.warning", cookie);
}

Expand Down
Loading

0 comments on commit 97eff58

Please sign in to comment.