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 18, 2024
1 parent 2e960a0 commit 5b199c9
Show file tree
Hide file tree
Showing 98 changed files with 245 additions and 520 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
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,7 @@ public void scan() {
Constant.messages.getString(
MESSAGE_PREFIX + "desc",
step2numberOfNodes - 1 + silentProxySet.size()))
.setAttack(getAttack())
.setAttack(Constant.messages.getString(MESSAGE_PREFIX + "attack"))
.setOtherInfo(extraInfo)
.setMessage(getBaseMsg())
.raise();
Expand All @@ -765,18 +765,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
Original file line number Diff line number Diff line change
Expand Up @@ -448,7 +448,7 @@ private boolean isEmptyOrTooSimilar(HttpMessage msg, int matchPercentage) {
* @param fileExtension
* @return
*/
private boolean dataMatchesExtension(byte[] data, String fileExtension) {
private static boolean dataMatchesExtension(byte[] data, String fileExtension) {
if (fileExtension != null) {
if (fileExtension.equals("JSP")) {
if (PATTERN_JSP.matcher(new String(data)).find()) return true;
Expand Down Expand Up @@ -502,7 +502,7 @@ public Map<String, String> getAlertTags() {
* @param b
* @return
*/
private int calcLengthMatchPercentage(int a, int b) {
private static int calcLengthMatchPercentage(int a, int b) {
if (a == 0 && b == 0) return 100;
if (a == 0 || b == 0) return 0;

Expand Down
Loading

0 comments on commit 5b199c9

Please sign in to comment.