Proposal: No auto-escaping for appdata

[Reinoud Elhorst]

This thread is a branchoff of "Allow JavaScript objects to be stored
in AppData" ( http://groups.google.com/group/opensocial-and-gadgets-spec/browse_thread/thread/4ed634f97029f0cd?hl=en
), focussing on whether or not appdata should have some form of auto-
escaping.

The proposal is that the appdata store works like "whatever you put in
is what you get out", and, consequently, "whatever a hacker puts in,
is what you get out". Any gadget developer should take this into
account.

I'm not going to risc summarizing the opinions on this issue -- and
most likely get them wrong (see the other thread for that).

It seems like in favour of this proposal:
Graham Spencer, Zhen Wang, Reinoud Elhorst, Chris Chabot, Lane
LiaBraaten?
against this proposal
Brian Eaton

[Reinoud Elhorst]

Perhaps to get the deadlock in this thread out of the way, it would be wise to have an idea on the current state of things:

1) Currently, the strings stored to appdata are returned escaped. This is defined in http://code.google.com/apis/opensocial/docs/0.7/spec.html#persistence . At the same, I can't seem to find any other reference in the spec to escaping, so I'm assuming that things like names are not escaped. If I were to call myself Reinoud<script>alert('hello')</script>, developers would still need to escape that part of the data manually.
2) Many opensocial gadgets out there are assuming that data is being escaped at the moment. If opensocial 0.8 would change the escaping rules, they might not realize this and expose themselves to js-injection when upgrading.
3) Many developers will want to work with non-escaped data, and escaping/unescaping data all the time seems like a lot of unnecessary overhead.

Since there are a lot of unescaped strings in the application anyway that need to be escaped (e.g. name), I (still) suggest not to do any "magic/automatic" escaping of any data. However, 2) poses a problem here. I can see three possible solutions for this.

a) Use some sort of global flag to indicate that no escaping should take place on the newFetchPersonAppDataRequest (solves 1, 2 and 3)
b) Add an extra opt_params field to the newFetchPersonAppDataRequest, that may be used to indicate not to do any escaping (solves 1, 2 and 3)
c) Create a new newFetchRawPersonAppDataRequest method (solves 1, 2 and 3)

In addition there are three other options

d) Change the behaviour of newFetchPersonAppDataRequest to just never escape (solves 1 and 3)
e) Leave everything as it is (solves 2)
f) Change the spec to escape everything, not just the appdata (solves 1 and 2)

My personal preference here is c). This will allow developers to work with data the way they want. In addition, if it turns out 90% of developers use the non-escaped version, we won't be stuck with some flag that always need to be set to true for legacy reasons.

[Brian Eaton]

On Tue, Apr 22, 2008 at 8:57 AM, Reinoud Elhorst <rei...@hyves.nl> wrote:
> 1) Currently, the strings stored to appdata are returned escaped. This is
> defined in
> http://code.google.com/apis/opensocial/docs/0.7/spec.html#persistence . At
> the same, I can't seem to find any other reference in the spec to escaping,
> so I'm assuming that things like names are not escaped. If I were to call
> myself Reinoud<script>alert('hello')</script>, developers would still need
> to escape that part of the data manually.

Orkut is escaping most person fields, and if they didn't then most of
the gadgets out there would render script tags verbatim. I haven't
seen a single gadget that treats a username as untrusted data.

[Zhen Wang]

I'm in favor of option (c), too. The solution gives character-escaping
control to developers and involves minimal changes to the spec.

On Tue, Apr 22, 2008 at 8:57 AM, Reinoud Elhorst <rei...@hyves.nl> wrote:

[Reinoud Elhorst]

It would be interesting to get some feedback from the other containers on whether they escape user data.

[Reinoud Elhorst]

Brian, since you were the only -1 on the original thread, would you object to solution (c): Create a new newFetchRawPersonAppDataRequest method (and leaving the old, escaping, newFetchPersonAppDataRequest, intact)?

I think it's very good to get clear on whether other data in OpenSocial should be escaped, but with the deadline for spec-changes being today, I'd like to get this proposal in.

[Cassie]

I think adding a new parallel method is sorta ugly. Could we instead add a param to the original method that says whether or not to escape the data? (And by default it would be true?)

so...

opensocial.DataRequest.prototype.newFetchPersonAppDataRequest = function(idSpec,
    keys, opt_params) {}

plus a new enum...

opensocial.DataRequest.DataRequestFields = {
  /**
   * A boolean specifying whether or not to automatically
   * escape all of the data returned. Defaults to true.
   *
   * @member opensocial.DataRequest.PeopleRequestFields
   */
  ESCAPE_VALUES : 'escapeValues',
}


- Cassie

btw - I'm gathering that nobody cares that all activity and people fields are auto escaped?

[Cassie]

Oh, and I now see Reinoud's explanation for why we should have a parallel method. Here is my counter:

Unfortunately, if we want to always be backwards compatible we would never be able to get rid of the original method. So, if we are always going for backward compatibleness we would be stuck with something gross. On the other hand, opensocial doesn't strictly have to be backwards compatible all of the time because we have handy versions. So, we could break out of this if we wanted to thus making option c not any better than option b for that issue.

Also, I think that having an option to auto escape or not is a useful feature. It seems like something we may want to keep around no matter what the default is. At least this way we also have our nice opt_params bag which we can easily extend in the future and containers can customize.

lol, was I convincing at all?

- Cassie

[Reinoud Elhorst]

On Fri, Apr 25, 2008 at 3:14 PM, Cassie <do...@google.com> wrote:

I think adding a new parallel method is sorta ugly. Could we instead add a param to the original method that says whether or not to escape the data? (And by default it would be true?)

so...

opensocial.DataRequest.prototype.newFetchPersonAppDataRequest = function(idSpec,
    keys, opt_params) {}

plus a new enum...

opensocial.DataRequest.DataRequestFields = {
  /**
   * A boolean specifying whether or not to automatically
   * escape all of the data returned. Defaults to true.
   *
   * @member opensocial.DataRequest.PeopleRequestFields
   */
  ESCAPE_VALUES : 'escapeValues',
}


- Cassie

btw - I'm gathering that nobody cares that all activity and people fields are auto escaped?

Are you saying that all people and activity fields are auto-escaped according to the spec? I searched for this, but couldn't find it, so I assumed it was an implementation irregularity (and I would be against it for the very same reasons )

I would support all three of solutions (a), (b) and (c); (c) has my personal preference because of the reasons mentioned above, but if the majority would be for (a) or (b), that's fine with me.

[Cassie]

On Fri, Apr 25, 2008 at 4:00 PM, Reinoud Elhorst <rei...@hyves.nl> wrote:

On Fri, Apr 25, 2008 at 3:14 PM, Cassie <do...@google.com> wrote:

I think adding a new parallel method is sorta ugly. Could we instead add a param to the original method that says whether or not to escape the data? (And by default it would be true?)

so...

opensocial.DataRequest.prototype.newFetchPersonAppDataRequest = function(idSpec,
    keys, opt_params) {}

plus a new enum...

opensocial.DataRequest.DataRequestFields = {
  /**
   * A boolean specifying whether or not to automatically
   * escape all of the data returned. Defaults to true.
   *
   * @member opensocial.DataRequest.PeopleRequestFields
   */
  ESCAPE_VALUES : 'escapeValues',
}


- Cassie

btw - I'm gathering that nobody cares that all activity and people fields are auto escaped?

Are you saying that all people and activity fields are auto-escaped according to the spec? I searched for this, but couldn't find it, so I assumed it was an implementation irregularity (and I would be against it for the very same reasons )

Yes. Specifically, every time you call getField the data is escaped.
This is what the spec was supposed to say, I'll see if I can find it...

[Brian Eaton]

On Fri, Apr 25, 2008 at 6:07 AM, Reinoud Elhorst <rei...@hyves.nl> wrote:
> Brian, since you were the only -1 on the original thread, would you object
> to solution (c): Create a new newFetchRawPersonAppDataRequest method (and
> leaving the old, escaping, newFetchPersonAppDataRequest, intact)?

I'm fine with any solution that does escaping by default (to handle
ignorance) and provides an option to unescape (to handle people with
legitimate needs for unescaping.)

I like Cassie's proposal to add paramers to
newFetchPersonAppDataRequest. I'd lean towards an enumeration rather
than a boolean value. Maybe the following possibilities?
ESCAPE_VALUES: 'htmlEscape', // default value
ESCAPE_TYPE: 'none', // use this if you want to enable XSS worms to
attack your users.

Then we can add more types later as needed.

I should comment, however, that if you call
gadgets.util.unescapeString() on untrusted data today and then assign
that data to innerHTML, your app is vulnerable. I've yet to see an
opensocial app that does anything except assign data to innerHTML, so
I'm pretty sure that anybody who uses ESCAPE_TYPE = none is asking for
trouble.

[Graham Spencer]

I think allowing developers to turn off escaping is a good compromise. +1 overall, and I like (b) as the mechanism (optional flag on method).

--g

[Arne Roomann-Kurrik]

On Fri, Apr 25, 2008 at 9:35 AM, Brian Eaton <bea...@google.com> wrote:

I should comment, however, that if you call

gadgets.util.unescapeString() on untrusted data today and then assign
that data to innerHTML, your app is vulnerable.  I've yet to see an
opensocial app that does anything except assign data to innerHTML, so
I'm pretty sure that anybody who uses ESCAPE_TYPE = none is asking for
trouble.

I've seen some Flash-based games render app data / profile data.  The escaping in this case wound up causing some ugly output that would not have been vulnerable to XSS.  That's probably the best use case here (aside from unescaping JSON encoded structures, but we have another proposal dealing with that).

+1 to Cassie's proposal.

[Cassie]

Okay, so it seems like an optional flag to the method sounds good to everyone. (except Zhen who hasn't commented if the new spec change is okay. +zhen so he can comment)

The enum sounds like a very flexible option so this will go into 0.8 unless anyone has any objections.
Just reply if there are any problems with this.

Thanks.

- Cassie

[Zhen Wang]

An optional flag sounds good to me.

[Reinoud Elhorst]

Cassie, did you check on whether the current spec demands escaping of getField? If not it might be wise to add that to this proposal and introduce the optional field for getField as well (not sure whether that last part was passed as well as part of this thread). I don't expect anyone will have objections there, since the first part is just fixing the spec (putting it the way it was meant to be, I think), and the second part is basically what we discussed for newFetchPersonAppDataRequest, on which we could all agree.

[Arne Roomann-Kurrik]

+1 to Reinoud's proposal, for the record.

~Arne

--
OpenSocial IRC - irc://irc.freenode.net/opensocial

[Brian Eaton]

Can someone summarize the proposal we are voting on?

[Arne Roomann-Kurrik]

Sorry, I meant his most recent proposal-

1.) Change the description of getField (for Person and Activity objects) to indicate that data returned will be escaped by default
2.) Add an opt_param to getField (for Person and Activity objects) allowing a parameter to be passed to disable escaping on the returned value.

To me, this seems like a logical extension of the enum value for newFetchPersonAppDataRequest that was approved in this thread.

~Arne

[Brian Eaton]

I'd be happier with the proposal to escape all the fields on the JSON
object, but this sounds reasonable as well.

[Arne Roomann-Kurrik]

I don't think that they're mutually exclusive - it looks like the other JSON object thread is leaning toward having data types, which means that the escaping behavior could be determined by the data type- for a string, the entire data is escaped, for an object, individual fields are escaped.  This proposal is more about controlling whether the auto-escape happens or not and documenting the default in the spec.

~Arne

[Brian Eaton]

OK, so we're talking about two modifications to the same API in
parallel in different threads. Classy. =)

If the end result of this is:
1) JSON is handled in such a way that developers don't need to call
unescape to deal with it.
2) The API gets a new enumerated type describing how the developer
wants the returned data formatted.
3) The default handling is HTML escaping.

... then that's +1 from me.

[Arne Roomann-Kurrik]

On Tue, Apr 29, 2008 at 2:53 PM, Brian Eaton <bea...@google.com> wrote:

OK, so we're talking about two modifications to the same API in
parallel in different threads.  Classy. =)

Yeah, but Cassie is the mutex, so it's OK ;)

~Arne
 

[Zhen Wang]

On Tue, Apr 29, 2008 at 2:53 PM, Brian Eaton <bea...@google.com> wrote:
>
> OK, so we're talking about two modifications to the same API in
> parallel in different threads. Classy. =)
>
> If the end result of this is:
> 1) JSON is handled in such a way that developers don't need to call
> unescape to deal with it.

+1

> 2) The API gets a new enumerated type describing how the developer
> wants the returned data formatted.

+1 -- though I'd much prefer to have a new function to retrieve raw
data (i.e. getRawField) because I have a hard time swallowing the
verbosity of "opensocial.DataRequest.DataRequestFields.RAW_VALUE".

> 3) The default handling is HTML escaping.

+1

[Brian Eaton]

On Tue, Apr 29, 2008 at 3:06 PM, Zhen Wang <wa...@google.com> wrote:
> > 2) The API gets a new enumerated type describing how the developer
> > wants the returned data formatted.
>
> +1 -- though I'd much prefer to have a new function to retrieve raw
> data (i.e. getRawField) because I have a hard time swallowing the
> verbosity of "opensocial.DataRequest.DataRequestFields.RAW_VALUE".

I think I'm the one who suggested an enumerated type originally, but
I'm fairly neutral as to the specifics of function name vs parameter.

[Louis Ryan]

Ok Im +1 here too.

I agree with Zhen that we need to resolve our syntactic sugar issues with enums. I'd prefer to do that wholesale in 0.9.

Zhen if youre so inclined can you send out a proposal?

[Zhen Wang]

Yes, I'll revisit the syntactic sugar issue when we move on to 0.9.

[Cassie]

Okay, so the opt_param to the fetchPersonAppData was approved already.
It looks we want to add the same param to all of the getField methods. That sounds fine.

Just fyi, here is the code change for fetchPersonAppData (minus comments so it is shorter):

opensocial.EscapeType = {HTML_ESCAPE : 'htmlEscape', NONE : 'none'};   <---- on opensocial. so we can reuse for all model objects
opensocial.DataRequest.DataRequestFields = {ESCAPE_TYPE : 'escapeType'};
opensocial.DataRequest.prototype.newFetchPersonAppDataRequest = function(idSpec, keys, opt_params) {};

Here is what the getField addition would be:

getField(name, opt_params);

Usage:
getField('name')
   or
getField('name', {'escapeType' : 'none'})   <--- not super horrible... but we can definitely add sugar in 0.9

- Cassie

[Cassie]

On Tue, Apr 29, 2008 at 11:56 PM, Arne Roomann-Kurrik <kur...@google.com> wrote:

On Tue, Apr 29, 2008 at 2:53 PM, Brian Eaton <bea...@google.com> wrote:

OK, so we're talking about two modifications to the same API in
parallel in different threads.  Classy. =)

Yeah, but Cassie is the mutex, so it's OK ;)

ps - that is so awesome
 

[Zhen Wang]

On Wed, Apr 30, 2008 at 11:16 AM, Cassie <do...@google.com> wrote:
> Okay, so the opt_param to the fetchPersonAppData was approved already.
> It looks we want to add the same param to all of the getField methods. That
> sounds fine.
>
> Just fyi, here is the code change for fetchPersonAppData (minus comments so
> it is shorter):
>
> opensocial.EscapeType = {HTML_ESCAPE : 'htmlEscape', NONE : 'none'}; <----
> on opensocial. so we can reuse for all model objects
> opensocial.DataRequest.DataRequestFields = {ESCAPE_TYPE : 'escapeType'};
> opensocial.DataRequest.prototype.newFetchPersonAppDataRequest =
> function(idSpec, keys, opt_params) {};
>
> Here is what the getField addition would be:
>
> getField(name, opt_params);
>
> Usage:
> getField('name')
> or
> getField('name', {'escapeType' : 'none'}) <--- not super horrible... but
> we can definitely add sugar in 0.9

I'm confused. If we're encouraging the use of plain strings, what is
the point of these new enum definitions?

[Cassie]

On Wed, Apr 30, 2008 at 8:22 PM, Zhen Wang <wa...@google.com> wrote:

On Wed, Apr 30, 2008 at 11:16 AM, Cassie <do...@google.com> wrote:
> Okay, so the opt_param to the fetchPersonAppData was approved already.
> It looks we want to add the same param to all of the getField methods. That
> sounds fine.
>
> Just fyi, here is the code change for fetchPersonAppData (minus comments so
> it is shorter):
>
> opensocial.EscapeType = {HTML_ESCAPE : 'htmlEscape', NONE : 'none'};   <----
> on opensocial. so we can reuse for all model objects
> opensocial.DataRequest.DataRequestFields = {ESCAPE_TYPE : 'escapeType'};
>  opensocial.DataRequest.prototype.newFetchPersonAppDataRequest =
> function(idSpec, keys, opt_params) {};
>
> Here is what the getField addition would be:
>
> getField(name, opt_params);
>
> Usage:
> getField('name')
>     or
> getField('name', {'escapeType' : 'none'})   <--- not super horrible... but
> we can definitely add sugar in 0.9

I'm confused. If we're encouraging the use of plain strings, what is
the point of these new enum definitions?

plain strings or enums are fine - depending on what your js coding preference is
enums are for clear docing purposes. they are also good for "compiler" errors (as in you'll get a slightly more meaningful js error)

but you can't force javascript users to use enums as they don't really exist, right?
thus - i used them in my example for clarity :)

i hope that helps.

- cassie
 

[Zhen Wang]

Thanks for the clarification, Cassie "the Mutex" :)