Form state is shared between different requests #13

Closed
powmedia opened this Issue Oct 12, 2011 · 7 comments

Projects

None yet

3 participants

@powmedia

Using: NodeJS 0.4.12, Express: 2.4.7

//controller.js, imported into app.js

var forms = require('../lib/forms'),
    fields = forms.fields,
    validators = forms.validators;

var signupForm = forms.create({

    first_name: fields.string({ required: true }),
    last_name: fields.string({ required: true }),
    email: fields.email({ required: true }),
    password: fields.password({ required: true }),
    password_confirm: fields.password({
        required: true,
        validators: validators.matchField('password')
    })

});

module.exports = function(app) {

    app.get('/signup', function(req, res, next) {
        res.render('signup', {
            signupForm: form.toHTML()
        });
    });

    app.post('/signup', function(req, res, next) {
        signupForm.handle(req.body, {
            success: function(form) {
                res.redirect('/app');
            },
            other: function(form) {
                res.render('signup', {
                    form: form.toHTML()
                });
            }
        });
    });
};

I've been seeing some strange behaviour, here's the steps to reproduce:

  1. Create form
  2. Open one browser and fill in the form, so it fails validation.
  3. Open a different browser, visit the '/signup' route and the form will be pre populated, showing data that the other browser (user) entered. Potentially displaying sensitive user info to another user.

Seems like the original signupForm object is being modified when bound to a request's form data?

@ljharb
Collaborator
ljharb commented Apr 2, 2012

This is kind of a huge problem, what with security and privacy. Does anyone know how it can be fixed?

@powmedia
powmedia commented Apr 2, 2012

I haven't used it in a while but I think this fork fixes the problem: https://github.com/tdryer/forms

On 2 Apr 2012, at 06:59, Jordan Harband wrote:

This is kind of a huge problem, what with security and privacy. Does anyone know how it can be fixed?


Reply to this email directly or view it on GitHub:
#13 (comment)

@johngeorgewright

This is a scary problem... however, looking at your code I think can see why it's happening. You're using the same object between different routes. Your signupForm will only ever be constructed once. Although I haven't tested this, I believe re-factoring your code like so should fix your problem:

//controller.js, imported into app.js

var forms = require('../lib/forms'),
    fields = forms.fields,
    validators = forms.validators;

var signupForm = function() {
    return forms.create({

        first_name: fields.string({ required: true }),
        last_name: fields.string({ required: true }),
        email: fields.email({ required: true }),
        password: fields.password({ required: true }),
        password_confirm: fields.password({
            required: true,
            validators: validators.matchField('password')
        })

    });
};

module.exports = function(app) {

    app.get('/signup', function(req, res, next) {
        res.render('signup', {
            signupForm: signupForm().toHTML()
        });
    });

    app.post('/signup', function(req, res, next) {
        signupForm().handle(req.body, {
            success: function(form) {
                res.redirect('/app');
            },
            other: function(form) {
                res.render('signup', {
                    form: form.toHTML()
                });
            }
        });
    });
};
@ljharb
Collaborator
ljharb commented Apr 3, 2012

The form should only need to be created once. However, its value should not persist beyond a single request, and refactoring our code to remove the benefit of caching seems inappropriate. It seems that tdryer@86f354d fixes the problem, so it just needs to be merged into the main project.

@johngeorgewright

Ahha... agreed. Much better way of go about it.

@ljharb ljharb was assigned Apr 30, 2012
@ljharb
Collaborator
ljharb commented May 4, 2012

Please see the comments on #16 for an update on this issue

@ljharb
Collaborator
ljharb commented Jul 11, 2012

Fixed by #16

@ljharb ljharb closed this Jul 11, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment