[4.2] FormBuilder throwing exception with binding array value to a single value form element

[garygreen]

Works as expected -- the model view data is bound to the text box correctly

<?php
// Simulate get request data from the user
Input::merge(['fruit' => 'Apple');
?>

{{ Form::model(Input::all()) }}
    {{ Form::text('fruit') }}
{{ Form::close() }}

A request is made for an array of fruit and an exception is thrown:

<?php
// Simulate get request data from the user
Input::merge(['fruit' => ['Apple']);
?>

{{ Form::model(Input::all()) }}
    {{ Form::text('fruit') }}
{{ Form::close() }}

Update: the same thing happens even when not using model binding when re-populating old session input

The issue spawns from the getValueAttribute function -- looking at the docblock comment it says it should return a string but it can return an array (in particular to support multiple select values etc). To me it looks like the function should always return either a string, array or null (to represent not adding value attribute to the form element). For those fields where only one value is allowed (text, radio, single select, etc) it should always return a string. For multiple values it should always return an array of selected values or null. Was that the desired effect? Either way the docblock is wrong.

https://github.com/laravel/framework/blob/4.2/src/Illuminate/Html/FormBuilder.php#L889

[Anahkiasen]

Well yeah, why would fruit be an array ?

[garygreen]

@Anahkiasen what do you mean?

[Anahkiasen]

Well in your second example you have Input::merge(['fruit' => ['Apple']); but fruit is a text field, why would it return an array?

[garygreen]

@Anahkiasen There are three main types of formats that can be submitted by the client: text, array, file. Of course in your system you intend for the field to be a string, the user can quite easily change the field name in the http request and throw an exception in your application.

Take for instance a website which runs on Laravel (testing for this vulnerability):

http://www.laravel-tricks.com/search?q=test --> fine
http://www.laravel-tricks.com/search?q[]=test --> malicious request, throws exception due to the same reason I suspect they are using model form binding.

The code is open source: https://github.com/CodepadME/laravel-tricks/blob/master/app/views/partials/search.blade.php

[lucasmichot]

Seems related to #5645

[garygreen]

In fact this isn't even model-form binding specific. It even occurs for re-populating old input from the session! Updating title

[garygreen]

@lucasmichot no, this is something totally different. This is not validation related.

[lucasmichot]

No this is array input related

[garygreen]

@lucasmichot please, this isn't related to validation. This is to do with how Laravel determines what value to populate form elements with; old input, the given value, or a model-binded value. It's NOT validation related at all. The docblock in the code says it returns a string, I believe that was it's intention but it can return an array, therefore it's vulnerable to throwing this exception.

[lucasmichot]

Hey Gary, this is clear to me this is not validation related. No need to repeat. As I said this is array related

[garygreen]

@lucasmichot no problem 👍

[garygreen]

@GrahamCampbell, was this closed because the form builder isn't maintained anymore? At least the docblock should be changed even if this bug isn't going to be fixed.

[GrahamCampbell]

Send a pull to illuminate/html then.