added two new options to the forced-browse add-on. #1534

KajanM · 2018-04-09T13:02:03Z

as part of #173 following options in DirBuster are brought to Forced Browse add-on.
HTML Parsing Options > File extensions to not process
Scan Options > Fail Case String

kingthorin

Haven’t tested it yet, here’s a few things.

kingthorin · 2018-04-09T14:06:32Z

src/org/zaproxy/zap/extension/bruteforce/BruteForceParam.java

+	    List<String> extensionsToMissList = new ArrayList<>();
+	    for (String extensionToMiss: extensionsToMiss.replaceAll("\\s", 
+	    		EMPTY_STRING).split(",")) {
+	    	if (!extensionToMiss.equals(EMPTY_STRING)) {


How about !extensionToMiss.isEmpty()

Actually there’s also probably a way to do this without a loop and using addAll. Should the code handle duplicates? If it was all put in a set I believe that’d ensure unique entries...

kingthorin · 2018-04-09T15:11:01Z

src/org/zaproxy/zap/extension/bruteforce/BruteForceParam.java

+		if (failCaseString == null) {			
+			throw new IllegalArgumentException("failCaseString is null");
+		}
+		if (!"".equals(failCaseString)) {


Should use isEmpty as mentioned above.

kingthorin · 2018-04-09T15:14:25Z

src/org/zaproxy/zap/extension/bruteforce/resources/Messages.properties

@@ -13,6 +13,8 @@ bruteforce.options.button.addfile	= Select File...
 bruteforce.options.label.addfile	= Add custom Forced Browse file:
 bruteforce.options.label.browsefiles = Force Browse files
 bruteforce.options.label.defaultfile = Default file:
+bruteforce.options.label.extensionsToMiss = File extensions to not process (separated by ,):


I think “ignore” might be more clear than ‘not process’.

KajanM · 2018-04-10T05:37:53Z

Suggestions are addressed and ready for the review.

psiinon · 2018-04-11T08:51:04Z

@KajanM could you update https://github.com/zaproxy/zap-extensions/blob/beta/src/org/zaproxy/zap/extension/bruteforce/resources/help/contents/options.html to add the new options?

thc202 · 2018-04-11T10:48:54Z

src/org/zaproxy/zap/extension/bruteforce/BruteForce.java

@@ -58,7 +58,8 @@
 	private boolean recursive = BruteForceParam.DEFAULT_RECURSIVE;
 	private DirBusterManager manager = null;
 	private List<String> extensions = null;
-
+	private Vector<String> extsToMissVector = null;
+


A space was accidentally added.

thc202 · 2018-04-11T10:49:02Z

src/org/zaproxy/zap/extension/bruteforce/BruteForceParam.java


 	public static final int DEFAULT_THREAD_PER_SCAN = 10;
 	public static final int MAXIMUM_THREADS_PER_SCAN = 200;
 	public static final boolean DEFAULT_RECURSIVE = true;
 	public static final boolean DEFAULT_BROWSE_FILES = false;
 	public static final String EMPTY_STRING = "";
+	public static final String DEFAULT_EXTENSIONS_TO_MISS = "jpg, gif, jpeg, ico, tiff, png, bmp";
+	public static final String DEFAULT_FAIL_CASE_STRING = Config.failCaseString;
+	public static final String WHITESPACE = "\\s";


This should be private.

@thc202 DEFAULT_EXTENSIONS_TO_MISS, DEFAULT_FAIL_CASE_STRING are also used in OptionsBruteForcePanel.java. Should I provide getter method or leave it public?

Those are fine, I just mean WHITESPACE and COMMA (though it applies to EMPTY_STRING as well), we don't really want other classes to depend on this class just because of these generic constants.

Sorry for the misunderstanding.
As of this comment and this thread, I removed WHITESPACE and COMMA constants.

No problem.
Sounds good.

thc202 · 2018-04-11T10:49:11Z

src/org/zaproxy/zap/extension/bruteforce/BruteForceParam.java

+	public static final String DEFAULT_EXTENSIONS_TO_MISS = "jpg, gif, jpeg, ico, tiff, png, bmp";
+	public static final String DEFAULT_FAIL_CASE_STRING = Config.failCaseString;
+	public static final String WHITESPACE = "\\s";
+	public static final String COMMA = ",";


Same here, also do we really need a constant for this? (If so, it should probably have a more meaningful name, like EXTENSION_SEPARATOR.)

thc202 · 2018-04-11T10:49:18Z

src/org/zaproxy/zap/extension/bruteforce/BruteForceParam.java

+
+	/**
+	 * @return {@code String} of comma-separated file-extensions that are not to be processed.
+	 * {@code "jpg, gif, jpeg, ico, tiff, png, bmp"} is returned by default


Probably better to link to the constant instead, otherwise this will have to be changed if we happen to add/remove an extension.

thc202 · 2018-04-11T10:49:21Z

src/org/zaproxy/zap/extension/bruteforce/BruteForceParam.java

+
+	/**
+	 * Define a {@code String} of comma-separated file-extensions for 
+	 * resources not to be processed


This could also use "ignore".

thc202 · 2018-04-11T10:49:44Z

src/org/zaproxy/zap/extension/bruteforce/BruteForceParam.java

+	 * {@code "jpg", "gif", "jpeg", "ico", "tiff", "png", "bmp"}
+	 *  
+	 */
+	public Vector<String> getExtensionsToMissVector() {


This could return the Set, let BruteForce convert it to Vector (actually BruteForce could just add them to the existing extsToMiss).

thc202 · 2018-04-11T10:49:46Z

src/org/zaproxy/zap/extension/bruteforce/BruteForceParam.java

+	 *  
+	 */
+	public Vector<String> getExtensionsToMissVector() {
+		if (extensionsToMiss.trim().equals(EMPTY_STRING)) {


isEmpty()

thc202 · 2018-04-11T10:49:57Z

src/org/zaproxy/zap/extension/bruteforce/BruteForceParam.java

+		if (failCaseString == null) {			
+			throw new IllegalArgumentException("failCaseString is null");
+		}
+		if (!failCaseString.isEmpty()) {


If the empty string is not acceptable we should throw an exception instead (like above).
Also, this should be validated in OptionsBruteForcePanel to inform the user that it should not be left empty.

thc202 · 2018-04-11T10:51:23Z

I'm not sure this fixes the issue, we might want check if there's any other useful option (or functionality) that needs to be exposed (it does not need to be done in this PR though).

kingthorin · 2018-04-11T10:59:36Z

Ya the issue has comments about showing all traffic, following redirects, Files/dirs/both, recursion (done I think), request limits, etc

So the commit and PR messages should probably be changed.

KajanM · 2018-04-11T15:29:30Z

@thc202 @kingthorin

I'm not sure this fixes the issue, ...

I will create a separate PR for other missing options. As I am new to open-source development I wanted to get some insights from the review before putting all the changes.

kingthorin · 2018-04-11T19:21:18Z

No problem

KajanM · 2018-04-13T08:01:42Z

Thank you for the review. Suggestions are addressed.

thc202 · 2018-04-13T10:27:51Z

src/org/zaproxy/zap/extension/bruteforce/BruteForce.java

@@ -99,6 +99,7 @@ public BruteForce (ScanTarget target, File file, BruteForceListenner listenner,
 	    } else {
 	    	extensions = Collections.emptyList();
 	    }
+	    extsToMissVector = new Vector<>(bruteForceParam.getExtensionsToMissSet());


I was suggesting change the extsToMiss directly, e.g.: manager.extsToMiss.addAll(bruteForceParam.getExtensionsToMissSet()) (which avoids changing setupManager).

Wow, it is beautiful.

thc202 · 2018-04-13T10:28:06Z

src/org/zaproxy/zap/extension/bruteforce/BruteForceParam.java

+	public void setFailCaseString(String failCaseString) {
+		if (failCaseString == null || failCaseString.isEmpty()) {
+	    	throw new IllegalArgumentException(
+	    			Constant.messages.getString("bruteforce.options.error.failCaseString.invalid"));


For exception messages not show directly in the UI we keep them in English.
The failCaseString is not being set to the configuration file.

thc202 · 2018-04-13T10:28:18Z

src/org/zaproxy/zap/extension/bruteforce/resources/Messages.properties

@@ -10,9 +10,12 @@ bruteforce.desc                     = Forced browsing of files and directories u
 bruteforce.dir.popup                = Forced Browse directory
 bruteforce.dir.and.children.popup   = Forced Browse directory (and children)
 bruteforce.options.button.addfile	= Select File...
+bruteforce.options.error.failCaseString.invalid = Invalid Fail Case String.


We should tell why it's invalid, better say something like "The Fail Case String should not be empty."

Is it possible to provide null value to the JTextField from the UI?
Is it enough to validate only for empty value?

No, it just returns an empty string. Yes.

KajanM · 2018-04-13T16:19:58Z

Done.

thc202 · 2018-04-13T17:10:23Z

LGTM, there's just one thing missing, the ZapAddOn.xml needs to be updated, the version should be 8 and the changes replaced with a short description mentioning the options added.

KajanM · 2018-04-13T19:46:08Z

Updated the ZapAddOn.xml and made setFailCaseString, getFailCaseString, getExtensionsToMissSet, setExtensionsToMiss, getExtensionsToMiss methods in BruteForceParam.java, package private.

thc202 · 2018-04-13T19:54:10Z

Note that the version was not updated. It was fine to leave them public also fine with package accessibility :) (+1 to the removal of the unused import.)

following options in DirBuster are brought to Forced Browse add-on. HTML Parsing Options > File extensions to not process Scan Options > Fail Case String

KajanM · 2018-04-13T20:02:49Z

Version updated. Thank you for the time :)

thc202 · 2018-04-13T20:10:39Z

Thank you!

thc202 · 2018-04-16T10:24:21Z

@kingthorin looks good?

kingthorin

Yup, looks good to me.

kingthorin · 2018-04-16T10:34:02Z

Squashed and merged.

thc202 · 2018-04-16T12:27:54Z

@KajanM how would you like to be credited?
https://github.com/zaproxy/zap-core-help/wiki/HelpCredits

KajanM · 2018-04-16T12:44:33Z

Kajan Mohanagandhirasa(@GM_K4J4N)

From zaproxy/zap-extensions#1534.

KajanM · 2018-04-16T13:00:21Z

Thank you :)

minor refactorization to improve code readability

129f65d

KajanM changed the title ~~Tighter DirBuster Integration~~ Fix zaproxy/zaproxy #173 Tighter DirBuster Integration Apr 9, 2018

kingthorin reviewed Apr 9, 2018

View reviewed changes

thc202 reviewed Apr 11, 2018

View reviewed changes

KajanM changed the title ~~Fix zaproxy/zaproxy #173 Tighter DirBuster Integration~~ added file-extensions-to-ignore, fail-case-string options to forced-browse add-on. Apr 11, 2018

KajanM changed the title ~~added file-extensions-to-ignore, fail-case-string options to forced-browse add-on.~~ added two new options to the forced-browse add-on. Apr 13, 2018

thc202 reviewed Apr 13, 2018

View reviewed changes

new options for forced-browse add-on

d35a8d3

following options in DirBuster are brought to Forced Browse add-on. HTML Parsing Options > File extensions to not process Scan Options > Fail Case String

thc202 approved these changes Apr 13, 2018

View reviewed changes

kingthorin approved these changes Apr 16, 2018

View reviewed changes

kingthorin merged commit 0b1b852 into zaproxy:beta Apr 16, 2018

KajanM deleted the beta branch April 16, 2018 11:18

thc202 added a commit to thc202/zap-core-help that referenced this pull request Apr 16, 2018

Add Kajan Mohanagandhirasa to credits

e7d5b87

From zaproxy/zap-extensions#1534.

thc202 mentioned this pull request Apr 16, 2018

Add Kajan Mohanagandhirasa to credits zaproxy/zap-core-help#185

Merged

added two new options to the forced-browse add-on. #1534

added two new options to the forced-browse add-on. #1534

Conversation

KajanM commented Apr 9, 2018 • edited

kingthorin 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

KajanM commented Apr 10, 2018

psiinon commented Apr 11, 2018

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

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

thc202 commented Apr 11, 2018

kingthorin commented Apr 11, 2018 • edited

KajanM commented Apr 11, 2018 • edited

kingthorin commented Apr 11, 2018

KajanM commented Apr 13, 2018

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

KajanM commented Apr 13, 2018

thc202 commented Apr 13, 2018

KajanM commented Apr 13, 2018

thc202 commented Apr 13, 2018

KajanM commented Apr 13, 2018

thc202 commented Apr 13, 2018

thc202 commented Apr 16, 2018

kingthorin left a comment

Choose a reason for hiding this comment

kingthorin commented Apr 16, 2018

thc202 commented Apr 16, 2018

KajanM commented Apr 16, 2018

KajanM commented Apr 16, 2018

KajanM commented Apr 9, 2018 •

edited

kingthorin commented Apr 11, 2018 •

edited

KajanM commented Apr 11, 2018 •

edited