[OWASP-ESAPI] Java Code Revew

Jeff Williams jeff.williams at owasp.org
Wed Dec 9 02:35:33 EST 2009


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

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

This is exactly the intent. Ideally, the frameworks will use the security
controls in ESAPI to make it *even easier* for developers. There are a
number of things that they could do automatically (like CSRF protection for
example) using the ESAPI controls.  But we don't want to build a new
framework - there are plenty already. We'll just focus on making the basic
security building blocks.  Anyway, putting together a Spring/ESAPI
integration jar seems like the right approach to me.

--Jeff

 

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/20091209/954b6678/attachment.html 


More information about the OWASP-ESAPI mailing list