[OWASP-ESAPI] Java Code Revew

Jim Manico jim.manico at owasp.org
Tue Dec 8 22:35:47 EST 2009


Ken,

This is solid work, but is a lot of feedback to process. Would you be
interested in getting access to our Google Code repository so you can
create specific bugs for us there?

http://code.google.com/p/owasp-esapi-java/issues/list

Also, when did you download this code? Do you know the exact SVN entry #?

All articulate feedback is helpful, so thanks for your time Ken. We
would be lucky to get just little more of it. :)

Regards,
Jim
> First I wish this wasn't an email... it is just too easy to misunderstand intent.  I want it known that I love the purpose of ESAPI and my intentions are good. I'm not here just to complain.  I am going to offer up my time to work on this project.  I could see how some of my comments below could be taken as being harsh... that really isn't my intention.  On my flight back from The Rich Web Experience, where I talked at length about ESAPI, I finally got a chance to do a significant code review of the Java code base.  Below is my notes and some of the feedback I got from attendees.  It is my intention to evangelize OWASP and ESAPI more next year...  
>
> My biggest reason to share this information is to:
> 1. Provide feedback.
> 2. Get feedback on... hey we knew that... or ken your an idiot or.. yeah, that is on our roadmap.
>
> It should be noted that my biggest hangup was on the unit tests.  One of the best reviews an organization can do is test reviews.   Additionally, one of the best ways that a new comer or user of the system can understand the intent of a system is by reviewing the tests (I refer to unit tests as "Executable intent")
>
> OK... the review... remember... I'm on your side.
>
> 1. It is generally viewed that the filters are not useable out of the box... they represent a coding sample of what someone should do... in particular I'm looking at you ESAPIFilter.  People like the idea that they can create their own, but want something useable out of the box as well (OOTB).
> 2. WEB-INF/index.jsp as a hard coded value in ESAPIFilter doesn't make sense.
> 3. In general there are tons of System.out.printlns... Chris captured this already and has posted an issue.
> 4. The tests in general look like they run on one guys machine... with references to user.home, etc.
> 5. There are a number of main(...) methods throughout the code.
> 6. comments regarding SimlpleDateFormat issues... these should be over come if possible
> 7. There is a lot of commented out code through out.
> 8. There is a significant number of empty tests... or tests that have commented out code.
> 9. It appears that many of the tests are designed for human observation... and not machine validation.  The output is for humans.
> 10. All defaults are windows... probably not where this code will run in production.  It would be great to provide platform independent options where possible and multiple properties files for several OS.
> 11. ESAPI in general seems to not be aligned to the Industry Java direction...   JSR 303 Validation annotations went final Nov 2009.  It would make sense to align to that.  Also, I would expect security concerns to align well with AOP or Spring... perhaps it is outside the scope of this toolkit.  I could understand that.  It just seems there is still significant work for the developer after choosing ESAPI.  To me it reminds me of trying to create a Swing application... Sun provided like 20% of what was needed. (that might be an exaggeration :)
> 12. It would have made more sense to use groovy or jython as extension scripts vs. bean shell. (just based on momentum)
> 13. There is a significant amount of knowledge that a developer needs to understand about the internal details in order to use the toolkit... examples:
> 	a) ThreadLocal and the order of method execution
> 	b) Anonymous user and all of its runtime exceptions... a little strange.
> 14. testEncodeForJavascript has a lot of commented out code... does it not work?  what changed?  
> 15. testNonAttacktAfterVirtualPatch seems to be misspelled
> 16. I couldn't find any kind words for SecurityConfigurationTest... what exactly is it testing?  
> 17. SafeFileTest and EncryptedPropertiesTest both use "user.home"
> 18. It is unclear if some of the code really should be mark as deprecated...  there are clues in the comments but no annotations or other documentation of understanding...
> It would be great to see these tests actually assert an expected value... It certainly will be tough for a random number... but that situation is probably less than 10% of the test cases.
>
> I should add I'm a big fan of WAF... 
>
> Keep the faith!  
>
> Ken Sipe | kensipe at gmail.com | blog: http://kensipe.blogspot.com
>
> _______________________________________________
> OWASP-ESAPI mailing list
> OWASP-ESAPI at lists.owasp.org
> https://lists.owasp.org/mailman/listinfo/owasp-esapi
>   


-- 

- Jim Manico
OWASP ESAPI Project Manager
http://www.owasp.org/index.php/Category:OWASP_Enterprise_Security_API

OWASP Podcast Host/Producer
http://www.owasp.org/index.php/OWASP_Podcast



More information about the OWASP-ESAPI mailing list