[OWASP-ESAPI] Java Code Revew

Chris Schmidt chrisisbeef at gmail.com
Wed Dec 9 00:49:49 EST 2009


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).

I am currently working on an OOTB filter in the Reference Implementation
called ESAPISecurityFilter which will provide a configurable filter that
allows users of the RI to harness some *Instant Gratification*

2. WEB-INF/index.jsp as a hard coded value in ESAPIFilter doesn't make
sense.

Agreed - there is an issue for this, however, since the ESAPIFilter itself
is meant to be a reference and not used, I would argue that these classes be
dropped altogether from the API and included in a seperate "examples" path
in the distribution zip.

3. In general there are tons of System.out.printlns... Chris captured this
already and has posted an issue.

This is resolved as of tonight. In addition I also removed several
Exception.printStackTrace() calls

5. There are a number of main(...) methods throughout the code.

I think that most of us are getting rid of these and turning them into test
cases when we see them. I know I am - did that very thing tonight with the
PreparedString class, there is now a PreparedStringTest

7. There is a lot of commented out code through out.

I agree. I used to do this a lot myself, but it is important for us all
(developers) to remember that SVN is our friend. If we are commenting
something out because we think it needs to go away - it is better to remove
the code altogether and if it needs to be added back in it can be snagged
from a previous revision of the file. I have generally been removing
commented code as I run across it if it is obvious that the code is not
appllcable anymore.

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 :)

Haha! That made me laugh. As someone who adamently protests all things Swing
(I can write a webapp 100x faster than a Swing App) I agree with you here to
a certain extent.

To clarify, I think that it is important that ESAPI be simple to interface
with popular industry standard security libraries (JaaS, Spring, etc.)
However, it is important to note that to get the best integration, it is
important to have involvement from the community of those libbraries, if not
have them provide the integration code themselves.

My take on this is that some of us who are vested in the project and working
on it daily should be reaching out to the communities of the industry
standard libraries and trying to get them involved. I would love nothing
more than to be on the Spring download page downloading the newest latest
greatest Spring and see that there is an optional download that is a
Spring/ESAPI integration jar.

The architecture of the ESAPI makes this pretty easy for the developers to
do.



On Tue, Dec 8, 2009 at 4:01 PM, Ken Sipe <kensipe at gmail.com> wrote:

>
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: https://lists.owasp.org/pipermail/owasp-esapi/attachments/20091208/82623e8b/attachment.html 


More information about the OWASP-ESAPI mailing list