search

cushon / issues-import

0 stars 0 forks source link

GWT SafeHtml check #26

Open cushon opened 9 years ago

cushon commented 9 years ago

Original issue created by eaftan@google.com on 2012-07-19 at 08:35 PM

GWT has an API called "SafeHtml" that has certain documented restrictions on how you're supposed to use it, and assuming developers use it correctly they should be largely protected against XSS vulnerabilities. Using it correctly amounts to two requirements:

  1. Some methods like SafeHtmlUtils.fromSafeConstant(String) are documented as requiring a "safe" string literal as the argument. Safe is defined here as a string that correctly parses as a sequence of complete HTML tags and leaves the parser in standard HTML context. (E.g., "" is okay; "<a href='" and "<script>" are not.)
  2. GWT has legacy methods like Element.setInnerHTML(String) that predate the introduction of SafeHtml APIs and subvert the protections offered, so they should be avoided in favor of the SafeHtml-variants.

My FindBugs works by recognizing the following expressions as safe:

  1. String literals that return true for com.google.gwt.safehtml.shared.SafeHtmlHostedModeUtils.isCompleHtml(String).
  2. String values returned from a call to SafeHtml.asString().
  3. A concatenation of string expressions recognized as safe (e.g., " + safeHtmlValue.asString() + "" is still safe).

It then warns about calls to methods like SafeHtmlUtils.fromSafeConstant(String) or Element.setInnerHTML(String) that aren't using safe arguments. Recognizing "safe" expressions this way is intentionally lenient to avoid needless code churn due to obviously safe (and incredibly common) code constructs like element.setInnerHTML("").

(It's complicated somewhat further because there are some methods that have an interface like setTextOrHTML(String text, boolean isHtml), and I further only warn on these methods if I can't statically determine the isHtml boolean argument is always false.)

cushon commented 9 years ago

Original comment posted by eaftan@google.com on 2013-02-11 at 07:50 PM

Matthew, are you still working on this, or should we close the bug?

cushon commented 9 years ago

Original comment posted by mdempsky@google.com on 2013-02-11 at 07:54 PM

I haven't been actively working on it in the past few months. We're in the middle of working on a GWT 2.5.1 release, and after that I'm hoping to finish it this quarter.

I've lowered the priority at least to reflect that, but we can close it and reopen later instead if you'd prefer.

Labels: -Priority-Medium, Priority-Low

cushon commented 9 years ago

Original comment posted by eaftan@google.com on 2013-02-12 at 04:22 AM

Let's leave it open, it's fine this way. Thanks!

cushon commented 9 years ago

Original comment posted by supertri@google.com on 2013-11-20 at 01:24 AM

What is the current status of this check?

cushon commented 9 years ago

Original comment posted by mdempsky@google.com on 2013-11-20 at 01:26 AM

I have an internal implementation of the check, but it's still disabled. I plan to productionalize it internally and work out any remaining kinks, and then look into open sourcing it.

cushon commented 9 years ago

Original comment posted by eaftan@google.com on 2014-03-04 at 12:29 AM

Friendly ping; what is the status of this check?

cushon commented 9 years ago

Original comment posted by mdempsky@google.com on 2014-03-04 at 12:35 AM

Martynas has been working on this most recently and probably knows the current state.

CC: martynas@google.com