createdAt field

[mquandalle]

In a Collection2 model I have defined a required field named createdAt wich is a simple Date object. How can I create an object on the client with autoform, add the createdAt field with the current date on the client, and then overwrite this field on the server to get the trust-able date value?

Maybe extract and modify the schema:

var schema = collection.simpleSchema().schema();
delete schema.createdAt;

Give this schema to an autoform and then run a Meteor.method on both the client and the server to add the createdAt attribute?

[aldeed]

I think you would have to use a submit button with data-meteor-method attribute pointing to a custom server method. I would personally make createdAt optional in the schema instead of required. Yes, you want it to be there, but if you're always doing the insert on the server, then it's pretty safe to define it as optional.

The method would be something like this:

Meteor.methods({
  doInsert: function(doc) {
    doc.createdAt = (new Date());
    return MyCollection2.insert(doc);
  }
});

Autoform validates on the client and the server before ever calling your method, so you know doc will be valid. If you'd rather not make createdAt optional, then you can set it on the client before calling the method by using the beforeMethod function, and then your server method would just be overwriting it.

MyCollection2.beforeMethod = function (doc, method) {
  if (method === "doInsert") {
    doc.createdAt = (new Date());
  }
  return doc;
}

One thing I'm wondering is whether setting it on the server is necessary. I thought Meteor had some built-in date saving magic that made sure local dates set on the client were saved in proper UTC on the server. I'm not sure I ever fully understood how they're doing the dates.

[mquandalle]

I think we must clearly distinguish Form and Schema objects.

• A Form is just a UI to create/update a javascript object in memory. One could open the browser console and create his own object without using the form. Forms objects shouldn't be trusted and don't need to be validated on the server. On the client, the validation is only here to improve the UI (errors messages), but not for security reasons.
• A Schema is a kind of extension of the Meteor build-in Match API. Before writing the database, or before executing a Meteor.method, we must check that the object we get as a parameter matches a predefined schema. Because client code can never be trusted, a schema must be defined on both the client and the server.

Of course, those two objects are linked. The Form constructor function can take a schema as a parameter.

So let's consider again the createdAt example. When I insert/update a doc in the collection, I really want to be sure that it has the createdAt field of type Date. That's why I define a schema wherein this field is required.
But in my form UI I don't want the user to populate this field and I create a form that doesn't have this field. So the Meteor doInsert method get a doc that doesn't match my schema. That's not a problem because the schema validation is done with the MyCollection2.insert call, no needs before. Actually doInsert doesn't need to be run on the server, so it doesn't need to be a Meteor method and can simply be a javascript function.

var doInsert = function(doc) {
    // doc came from the form UI but may have been manually created in the browser console
    // check(doc, Match.Any);
    doc.createdAt = (new Date());
    MyCollection2.insert(doc); // This is where the "check" is done
}

Collection methods (insert/update/delete) are safe. What about Meteor methods?
Here is what autoform documentation says:

You do not need to call check() in the method.
The equivalent of this is done for you on both the client and the server.

And here is the implementation (autoform-server.js):

Meteor.methods({
    _autoFormCheckFirst: function(method, objName, doc) {
        var obj = global[objName];
        if (!obj) {
            throw new Error("No object exists on the server with that name.");
        }
        checkSchema(doc, obj.simpleSchema());
        return Meteor.call(method, doc);
    }
});

Now, imagine that my methodA expect a doc that match schemaA as a parameter. As the documentation says I don't need to call check, this is done by a kind a proxy method. The point is that _autoFormCheckFirst get the objName as a parameter. So one could:

Meteor.call("_autoFormCheckFirst", "methodA", "schemaB", docThatMatchSchemaB);

and then my methodA will get a doc that doesn't match schemaA. The only way to fix this security issue is to hard code the check (or checkSchema) in the methodA.

To put it in a nutshell:

• Forms doesn't need server-side validation
• insert/update/delete a document in a collection2 is safe
• Meteor methods must always check (or checkSchema) their arguments
• insert/update/delete a document in a collection1 must be done via a safe Meteor method call
• Forms should be defined in Template binding. They may be reactive (add/remove fields if a user connect/disconnect).
• Schemas need to be defined in a global or "sub-global" scope, for instance in a Schemas global object. This need to be done on the server (for security) and the client (for latency compensation).

This post is related to issues #6 and #10 as well.

[aldeed]

Interesting. Good catch on the security issue. I can think of another way to fix it, though. When you create a new AutoForm or Collection2 object, there can be a "methods" option that allows you to specify which methods can be called with that schema.

MyFormA = AutoForm(schemaAObj, {methods: ["methodA"]});
MyFormB = AutoForm(schemaBObj, {methods: ["methodB"]});

Then _autoFormCheckFirst would be modified to make sure the method name is in this list before calling it on the server, after validating.

    _autoFormCheckFirst: function(method, objName, doc) {
        var obj = global[objName];
        if (!obj) {
            throw new Error("No object exists on the server with that name.");
        }
        if (!obj.allowsMethod(method)) {
            throw new Error("That method may not be called with the given schema.");
        }
        checkSchema(doc, obj.simpleSchema());
        return Meteor.call(method, doc);
    }

Now this would work:

Meteor.call("_autoFormCheckFirst", "methodA", "MyFormA", docThatMatchSchemaA);

But this would not:

Meteor.call("_autoFormCheckFirst", "methodA", "MyFormB", docThatMatchSchemaB);

As for the form vs. collection vs. schema aspect, I agree that these are separate ideas, which is why I decided to make three separate related packages instead of one, but I also wanted to make things as simple as possible, which is why there is an AutoForm object that closely resembles the Collection2 object. In truth, AutoForm adds very little to the SimpleSchema object, so I generally think of them as the same thing. Despite it's name AutoForm is really just a schema that is specific to one or more forms. Maybe it should be called FormSchema instead. The reason for having it instead of using the SimpleSchema object directly is to provide additional form-specific options, such as my proposal above for defining allowed methods, which is not something that would make sense to do directly on SimpleSchema.

So other than fixing that security/data integrity issue with _autoFormCheckFirst and attempting to remove the global scope requirement on the client, it seems like no other changes are necessary. If I'm missing any other point you're making, though, please let me know.

[mquandalle]

• Forms shouldn't be used outside the templating engine (that means, forms shouldn't be define on the server until shark)
• If myForm accept an array of methods, let's say ['methodA', 'methodB'], one could continue to do
  Meteor.call("_autoFormCheckFirst", "methodA", "schemaB", docThatMatchSchemaB);
• That's even worst than that. If I don't do some check validation on my method, one can simply call this method with any object he wants bypassing the _autoFormCheckFirst "proxy-method"
  Meteor.call("methodA", {name: 'hacker', isAdmin: true});

So, again, Meteor methods must always check (or checkSchema) their arguments (that's why the audit-argument-checks package makes sense), and Autoforms doesn't need server-side validation (validation is only here to improve the UI)

[aldeed]

1. Forms are not defined on the server. The AutoForm object is, but as I said, that's really just a form schema, and it's standard practice to validate form submission on both client and server for security, which means that developers must define AutoForm on both client and server (in common js).
2. I don't agree that there's still a security hole through _autoFormCheckFirst because if the developer specified a method that doesn't actually expect that schema, that would be developer error, not a problem with this package. But...
3. You're definitely right about being able to Meteor.call the method directly. I'm not sure why I thought that couldn't happen. I guess I was thinking the server method would only be callable from the server, which obviously doesn't make sense. So I guess you're correct that developers will have to call checkSchema() themselves. Maybe meteor methods feature could be enhanced to allow you to define methods callable only from the server, and then the current way would work.

So I will:

• update the readme to indicate that checkSchema() must be called in each method
• remove the call to _autoFormCheckFirst and call the user method directly

I think that's it to fix this issue. I can address the scoping issue separately.

[mquandalle]

Ok for [2] and [3].

For [1], validate forms on the server is useless, it doesn't add security at all.
Check arguments on Meteor methods, and use collection2 insert/update/delete are the right place to do the security checks.

In others words, Meteor methods arguments are never trusted (because one can manually do a Meteor.call on a DDP client) and that's why we don't need to (and we can't) be sure that the form object that we get has the good schema (that's why we don't need server side form validation).

A form is just a UI to get an object in client JavaScript memory. And client objects can never be trusted.

[aldeed]

You're not really validating the form on the server, you're validating the object that was passed in against the schema. That's precisely what you're arguing must be done. It's no different than calling check(). In fact, I originally implemented it as an extension of the check() function, but 0.6.5 made that stop working, so that's why it's called checkSchema() now.

It's true that you don't need to do it if your method is working with a collection2, but what about the example of a contact form?

The AutoForm object:

ContactForm = new AutoForm({
    name: {
        type: String,
        label: "Your name",
        max: 50
    },
    email: {
        type: String,
        regEx: SchemaRegEx.Email,
        label: "E-mail address"
    },
    message: {
        type: String,
        label: "Message",
        max: 1000
    }
});

The HTML:

{{#autoForm schema="ContactForm" id="contactForm"}}
<fieldset>
    <legend>Contact Us</legend>
    <div class="form-group{{#if afFieldIsInvalid 'name'}} has-error{{/if}}">
        {{afFieldLabel "name" class="control-label"}}
        {{afFieldInput "name"}}
        {{#if afFieldIsInvalid "name"}}
        <span class="help-block">{{afFieldMessage "name"}}</span>
        {{/if}}
    </div>
    <div class="form-group{{#if afFieldIsInvalid 'email'}} has-error{{/if}}">
        {{afFieldLabel "email" class="control-label"}}
        {{afFieldInput "email"}}
        {{#if afFieldIsInvalid "email"}}
        <span class="help-block">{{afFieldMessage "email"}}</span>
        {{/if}}
    </div>
    <div class="form-group{{#if afFieldIsInvalid 'message'}} has-error{{/if}}">
        {{afFieldLabel "message" class="control-label"}}
        {{afFieldInput "message" rows="10"}}
        {{#if afFieldIsInvalid "message"}}
        <span class="help-block">{{afFieldMessage "message"}}</span>
        {{/if}}
    </div>
    <div>
        <button type="button" data-meteor-method="sendEmail" class="btn btn-primary">Submit</button>
        <button type="reset" class="btn btn-default">Reset</button>
    </div>
</fieldset>
{{/autoForm}}

The Meteor method:

Meteor.methods({
    sendEmail: function(doc) {
        checkSchema(doc, ContactForm.simpleSchema());
        var text = "Name: " + doc.name + "\n\n"
                + "Email: " + doc.email + "\n\n\n\n"
                + doc.message;

        this.unblock();

        Email.send({
            to: "test@example.com",
            from: doc.email,
            subject: "Website Contact Form - Message From " + doc.name,
            text: text
        });
    }
});

In the Meteor method, the call to ContactForm.simpleSchema() requires ContactForm to be defined on the server.

[aldeed]

Now you've got me thinking. I don't think collection2 methods are actually secure either. Both client and server just check the doc against the schema and then call the normal collection insert/update/remove. This means that one could insert into the wrapped collection1 and the insert would happen on the server, too, without validation. The client could be made secure by calling a proxy meteor method that does the insert, but it would also have to disallow client side modification of the collection.

I think I'll need to include some schema checks in the allow/deny for the wrapped collection1 when c2 creates it. I'm going to have to give this a lot more thought.

[aldeed]

Regarding the C2 security issue, see issue Meteor-Community-Packages/meteor-collection2#5. I figured out a fix.

[mquandalle]

Here is my API proposition for the same contact form: https://gist.github.com/mquandalle/6284027

[aldeed]

OK, that's helpful. I see what you're proposing. I think that should all be fine. I will probably continue to support passing in the string name of a global AutoForm object for backwards compatibility, but there is nothing preventing handlebar binding now that the automatic server check is gone.

[mquandalle]

That's great!

Here is the same proposal binding code for "two forms in one" issue #6:

Template.tplName.myForm = function() {
    new AutoForm(
        _.extend(
            CollectionA.simpleSchema().schema(),
            CollectionB.simpleSchema().schema()
        )
    );
};

I agree that you can easily support backwards compatibility by checking if the first argument is a string or not.