[OWASP-ESAPI] Java Code Revew
chrisisbeef at gmail.com
Tue Dec 8 23:08:02 EST 2009
FWIW - I will be checking in all the files with the System.out removed
tonight. I elected to focus on the API code itself with this round and
circle back around to take care of the test cases if we decide that it would
be worthwhile to nuke them there as well.
On Tue, Dec 8, 2009 at 8:35 PM, Jim Manico <jim.manico at owasp.org> wrote:
> 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?
> 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. :)
> > 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
> > 2. WEB-INF/index.jsp as a hard coded value in ESAPIFilter doesn't make
> > 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...
> > a) ThreadLocal and the order of method execution
> > b) Anonymous user and all of its runtime exceptions... a little
> 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
> OWASP Podcast Host/Producer
> OWASP-ESAPI mailing list
> OWASP-ESAPI at lists.owasp.org
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the OWASP-ESAPI