On Tue, May 3, 2011 at 09:17, <[email protected]> wrote:
> On 2011/05/03 16:11:37, xtof wrote:
>
>
> http://gwt-code-reviews.appspot.com/1380806/diff/28033/user/src/com/google/gwt/safehtml/shared/SafeUri.java
>
>> File user/src/com/google/gwt/safehtml/shared/SafeUri.java (right):
>>
>
>
>
> http://gwt-code-reviews.appspot.com/1380806/diff/28033/user/src/com/google/gwt/safehtml/shared/SafeUri.java#newcode41
>
>> user/src/com/google/gwt/safehtml/shared/SafeUri.java:41: * context in
>>
> which the
>
>> URL is used matters too (link {@code href} vs. image
>> On 2011/05/02 18:57:13, skybrian wrote:
>> > This sounds somewhat dangerous, actually. It seems like if context
>>
> matters,
>
>> > either each context should have its own type, or creators of SafeUrl
>>
> instances
>
>> > should stick to the lowest common denominator.
>> >
>> > The reason is that the code creating a SafeUrl instance might be far
>>
> removed
>
>> > from the code that uses it. If the creator believes that a URL will
>>
> be used in
>
>> > iframe context, but actually it isn't, then reviewers cannot find
>>
> the problem
>
>> > without having an end-to-end understanding of the program's data
>>
> flow, and any
>
>> > refactoring of intermediate code has the potential to introduce a
>>
> bug without
>
>> a
>> > type error and without the reviewers seeing anything wrong.
>> >
>> > It seems like the whole point of having safe types with clear
>>
> contracts is
>
>> make
>> > sure that local reviews are sufficient to guarantee safety?
>> >
>> > I hate the complexity this is likely to introduce, but on the other
>>
> hand, a
>
>> > SafeUrl type that isn't actually safe doesn't sound so great either.
>>
>
> I think I agree with Brian on this one. Can we specify the contract
>>
> such that a
>
>> SafeUri is safe to use in any of these contexts (at the expense of
>>
> being overly
>
>> restrictive, for e.g. img src URIs)? If not, I think we need to
>>
> introduces
>
>> separate types. Otherwise we'll loose the local revieability benefit
>>
> of the
>
>> SafeXyz types.
>>
>
> If that's the only point left, can you make the edit yourself before
> submitting? (as, while I agree, I don't know what to say exactly here)
> Otherwise, if there are other comments requiring a new patchset, then
> I'll try to address this one at the same time.
>
>
My apologies dropping the response on this thread (I'd missed the last
question and had assumed this was good to submit).
Also, I didn't realize that I actually need to fetch and submit this patch
(I'm not part of GWT team proper and wasn't familiar with the process for
changes from external developers). I've now grabbed your latest patch set
and will prepare it for commit (I'm making a few minor fixes to make
extended presubmit tests pass). I'll post an updated patch set for your
final review shortly.
With respect to the question about the SafeUri docs: I've changed the
relevant paragraphs to,
* <p>
* All implementing classes must maintain the class invariant (by design and
* implementation and/or convention of use), that invoking {@link
#asString()}
* on any instance will return a string that is safe to assign to a
URL-typed
* DOM or CSS property in a browser (or to use similarly in a "URL
context"), in
* the sense that doing so must not cause unintended execution of script in
the
* browser.
*
* <p>
* In determining safety of a URL both the value itself as well as its
* provenance matter. An arbitrary URI, including e.g. a
* <code>javascript:</code> URI, can be deemed safe in the sense of this
type's
* contract if it is entirely under the program's control (e.g., a string
* literal, {@see UriUtils#fromSafeConstant}).
Sound good?
Note: UriUtils#fromSafeConstant doesn't exist in the current patch, but I
think it should; analogous to SafeHtmlUtils#fromSafeConstant. This would
allow developers to concisely write,
SafeUri voidJs = UriUtils.fromSafeConstant("javascript:void(0)");
and similar.
Unless there are objections I'll add it; the implementation of
fromSafeConstant would be the exact same as fromTrustedString; however its
use signifies that its value is supposed to be a compile time constant,
whereas fromTrustedString may be passed a value that depends on user input,
but is somehow (though not trivially) known to be safe. We shouldn't be
worried to see many "fromSafeConstant(<literal>)" uses in a program, but
should be worried if we see a lot of fromTrustedString ones (which would
have to be carefully manually reviewed, and if there's lots of uses it may
be an indication that a special case sanitizer is needed.
Again, sorry for letting this sit for several weeks :/
--xtof
> TIA
>
>
> http://gwt-code-reviews.appspot.com/1380806/
>
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
