Validator added every time a step is submitted

[joefitter]

https://github.com/UKHomeOffice/passports-form-controller/blob/master/lib/validation/index.js#L62-L67

This appears to be an issue with any step that contains a field with options (radio, typeahead etc). An additional validator is added to the list of validators (created if empty) to check the value is in the list of options. The problem being that it doesn't check if the validator has already been added, and mutates the original array (reference to original stored, not cloned).

More worryingly is that this is persisted for the life of the instance, and across all users, each user that completes that step causes another identical validator to be added to the validators config, and this is persisted server-wide causing a potentially large memory leak.

@gxxm has been working on a fix, but probably worth pointing out to @easternbloc @JoeChapman @daniel-ac-martin and @gavboulton as will likely be affecting live systems.

This likely wasn't picked up before as the same validator is added many times, so the reduce below just calls the validator function over and over again yielding the same result (this should be replaced with _.any for efficiency), @gxxm noticed it when altering the options dynamically based on the results of a postcode lookup - multiple validators are added with different options and validation always fails

[daniel-ac-martin]

Nice catch guys!
I doubt this will be the only place where we have this bug.
In this case it is easy to fix as we can just test whether the validator already exists.
But, there is a wider question of how can we patch the validators whilst limiting the scope of the patch to a single request. Ultimately, I think we should be performing a deep copy of the fields object (which starts its life as configuration, which we tend to think of as immutable, but isn't really "because JavaScript") on every request. Then we can patch it on a pre-request basis.

[joefitter]

yeah a deep clone would fix this case but as you said this may not be the only place the bug occurs. We should be ideally making config immutable - could use something like https://facebook.github.io/immutable-js/ or https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Object/freeze. Obviously @gxxm's current method of setting options on the fly would need rethinking slightly, could perhaps unfreeze, amend the re-freeze?

[joefitter]

actually there isn't a way to unfreeze but still think we should consider locking config down

[JoeChapman]

+1 for making config immutable, unfortunately Object.freeze means that in most cases, in strict mode, trying to modify a "frozen" object will throw a TypeError so we might need to change more than just how we create the config.

[easternbloc]

Nice spot! More debug around this might have made it visible earlier. It's something I'm keen to add more to our modules UKHomeOffice-attic/hof#90

[joefitter]

Yes, it will need some thought but will hopefully pinpoint any other places in the code where we are directly modifying config

[daniel-ac-martin]

@joefitter: A deep copy of fields on each request should fix all the related bugs as well shouldn't it?

Obviously code should still check that a validator doesn't already exist when adding it to the list.

[joefitter]

@daniel-ac-martin yes it would but it wouldn't stop us from unknowingly altering the fields object again somewhere further down the line, or steps, or any other config we may wish to pass. Object.freeze should be used on any config objects to ensure this doesn't happen in future, but a deep copy on each request should fix the current issue

[daniel-ac-martin]

In case it isn't obvious, I'd like to say that I think this bug should have quite a high priority (though I'm biased). - We do need to patch the validators on a per request basis and this bug can potentially allow a malicious user to render the forms un-completable for other users. (My new date-controller suffers from this but cannot really be fixed until it is fixed here.)

Of course, I also think this will be a pain to fix. Anyone feeling energetic?

[JoeChapman]

You start Dan.

[daniel-ac-martin]

@JoeChapman: Well I'm sure I will at some point, but I don't really get any time in my working day to do HOF stuff. (Been mainly writing Scala recently.) So really me being able to do this all hinges on me having no life. ;-)
Just wanted to put this out there in case someone has the time to work on fixing miscellaneous bugs in the HMPO form stack.

[oliverweighell]

Thanks @joefitter and @gxxm for flagging this!

The Passport renewals service has now gone public, so keen to plug the memory leak as an initial step - #47

[oliverweighell]

Cheers @daniel-ac-martin for merging.

In terms of the wider discussion, I agree about immutable config. However, not so convinced about patching validators per request (quite possibly due to not really knowing the specifics of what you are doing).

What if validators (formatters, etc...) could instead ask for the data they need? Rough example: a1ebdec

Is that viable, and would it give enough flexibility to satisfy the needs of, for example, the date controller?

[daniel-ac-martin]

@oli-weighell2: Yes I've been coming around to that point of view as well. It does mean that we will need something in the fields config to indicate that a field should be treated as an input-date mixin. (Support for the input-date mixin is the reason I thought we would need this.)

I'd favour introducing a new concept (processors?) rather than expanding the scope of the validators and formatters. - They are quite neat little things as thing stand.

I have a few ideas, but they will require PRs on the HMPO stack rather than simply being a controller that can sit in its own module.

[daniel-ac-martin]

Updated HMPO/hmpo-form-wizard#35 to discuss.

[daniel-ac-martin]

Incidentally, deep copy is a PITA in JavaScript in any case.

[joefitter]

@daniel-ac-martin we use https://lodash.com/docs#cloneDeep

[joefitter]

however I dont think underscore has an equivalent

[easternbloc]

It does not. We should move all the things to lodash imho

[daniel-ac-martin]

@easternbloc I started to do just that in a branch. Stuff broke and I ran out of time. :-(
(Though I think the hmpo modules might use an old version of underscore which might be the cause of the breakage.)

[HughePaul]

fixed by deep cloning options a while back