Grails
  1. Grails
  2. GRAILS-7912

bindData with empty include list includes everything

    Details

    • Testcase included:
      yes

      Description

      When bindData() is given an empty list of properties to include, it binds everything (but nothing should be bound).

      class FooController {
          def index = {
              def foo = new Foo()
              bindData(foo, params, [include: []])
              foo
          }
      }
      
      class Foo {
          String bar
      }
      
      class FooControllerIntegrationTest extends GroovyTestCase {
          void testBindDataIncludeNothing() {
              def controller = new FooController()
              controller.params << [ bar: 'bad' ]
              assert controller.index().bar == null
          }
      }
      

      fails with

      Assertion failed: 
      
      assert controller.index().bar == null
             |          |       |   |
             |          |       bad false
             |          Foo@1b4a4fd2
             FooController@2bbe71fd
      

      However, if the include list is not empty (and doesn't include 'bar'), then bar is not bound.

        Activity

        Hide
        Jeff Scott Brown added a comment -

        The original description here says "but nothing should be bound". Is that really correct? That isn't how the Spring DataBinder class works. It says that if allowedFields is empty then a field is allowed as long as it isn't included in the explicit excludes.

        DataBinder.java
        
                ...
        
        	protected boolean isAllowed(String field) {
        		String[] allowed = getAllowedFields();
        		String[] disallowed = getDisallowedFields();
        		return ((ObjectUtils.isEmpty(allowed) || PatternMatchUtils.simpleMatch(allowed, field)) &&
        				(ObjectUtils.isEmpty(disallowed) || !PatternMatchUtils.simpleMatch(disallowed, field)));
        	}
        
                ...
        
        

        We can circumvent this by adding logic of our own in GrailsDataBinder but I am not sure that is the right thing to do.

        Should we change this behavior or leave it as is?

        Show
        Jeff Scott Brown added a comment - The original description here says "but nothing should be bound". Is that really correct? That isn't how the Spring DataBinder class works. It says that if allowedFields is empty then a field is allowed as long as it isn't included in the explicit excludes. DataBinder.java ... protected boolean isAllowed( String field) { String [] allowed = getAllowedFields(); String [] disallowed = getDisallowedFields(); return ((ObjectUtils.isEmpty(allowed) || PatternMatchUtils.simpleMatch(allowed, field)) && (ObjectUtils.isEmpty(disallowed) || !PatternMatchUtils.simpleMatch(disallowed, field))); } ... We can circumvent this by adding logic of our own in GrailsDataBinder but I am not sure that is the right thing to do. Should we change this behavior or leave it as is?
        Hide
        J. David Beutel added a comment -

        The "nothing should be bound" is my naive expectation when, e.g., reducing the list from one property to no properties. I'm surprised if Spring DataBinder is supposed to work that way, although I see that's how it's implemented (using isEmpty() instead of allowed == null). Nevertheless, I don't see anything in its javadoc that suggests that setting the allowed fields to an array of no fields is the same as setting it to the default of all fields. (On the other hand, I'd expect that setting it back to null would be the default, but the javadoc doesn't mention null either.)

        	/**
        	 * Register fields that should be allowed for binding. Default is all
        	 * fields. Restrict this for example to avoid unwanted modifications
        	 * by malicious users when binding HTTP request parameters.
        	 * <p>Supports "xxx*", "*xxx" and "*xxx*" patterns. More sophisticated matching
        	 * can be implemented by overriding the <code>isAllowed</code> method.
        	 * <p>Alternatively, specify a list of <i>disallowed</i> fields.
        	 * @param allowedFields array of field names
        	 * @see #setDisallowedFields
        	 * @see #isAllowed(String)
        	 * @see org.springframework.web.bind.ServletRequestDataBinder
        	 */
        	public void setAllowedFields(String... allowedFields) {
        		this.allowedFields = PropertyAccessorUtils.canonicalPropertyNames(allowedFields);
        	}
        
        	/**
        	 * Return the fields that should be allowed for binding.
        	 * @return array of field names
        	 */
        	public String[] getAllowedFields() {
        		return this.allowedFields;
        	}
        

        Anyway, regardless of how Spring is supposed to work, I think that Grails would be more secure and less surprising if, when given a list of properties to bind, the list is not ignored when its size goes down to 0.

        Show
        J. David Beutel added a comment - The "nothing should be bound" is my naive expectation when, e.g., reducing the list from one property to no properties. I'm surprised if Spring DataBinder is supposed to work that way, although I see that's how it's implemented (using isEmpty() instead of allowed == null). Nevertheless, I don't see anything in its javadoc that suggests that setting the allowed fields to an array of no fields is the same as setting it to the default of all fields. (On the other hand, I'd expect that setting it back to null would be the default, but the javadoc doesn't mention null either.) /** * Register fields that should be allowed for binding. Default is all * fields. Restrict this for example to avoid unwanted modifications * by malicious users when binding HTTP request parameters. * <p>Supports "xxx*" , "*xxx" and "*xxx*" patterns. More sophisticated matching * can be implemented by overriding the <code>isAllowed</code> method. * <p>Alternatively, specify a list of <i>disallowed</i> fields. * @param allowedFields array of field names * @see #setDisallowedFields * @see #isAllowed( String ) * @see org.springframework.web.bind.ServletRequestDataBinder */ public void setAllowedFields( String ... allowedFields) { this .allowedFields = PropertyAccessorUtils.canonicalPropertyNames(allowedFields); } /** * Return the fields that should be allowed for binding. * @ return array of field names */ public String [] getAllowedFields() { return this .allowedFields; } Anyway, regardless of how Spring is supposed to work, I think that Grails would be more secure and less surprising if, when given a list of properties to bind, the list is not ignored when its size goes down to 0.
        Hide
        J. David Beutel added a comment -

        By the way, isEmpty() is correct for disallowed. I think it's just a bug in DataBinder to use isEmpty() for allowed too.

        Show
        J. David Beutel added a comment - By the way, isEmpty() is correct for disallowed. I think it's just a bug in DataBinder to use isEmpty() for allowed too.
        Hide
        J. David Beutel added a comment -

        Note that canonicalPropertyNames(String[]) preserves the distinction between null and the empty array.

        Show
        J. David Beutel added a comment - Note that canonicalPropertyNames(String[]) preserves the distinction between null and the empty array.
        Hide
        Peter Ledbrook added a comment -

        It seems a little perverse to want to include no properties during the binding. Effectively the binding doesn't happen. However, I agree with the logic that an empty list means don't include any properties. If someone passes in an empty list, we should honour that even if it seems odd.

        On the other hand, it would be a breaking change that could adversely affect existing applications. Is the current behaviour going to affect enough people to warrant a breaking change? I'm not sure.

        I'm currently leaning towards leaving it as is and documenting this corner case. Perhaps if data binding undergoes a significant overhaul, then we can revisit this.

        Show
        Peter Ledbrook added a comment - It seems a little perverse to want to include no properties during the binding. Effectively the binding doesn't happen. However, I agree with the logic that an empty list means don't include any properties. If someone passes in an empty list, we should honour that even if it seems odd. On the other hand, it would be a breaking change that could adversely affect existing applications. Is the current behaviour going to affect enough people to warrant a breaking change? I'm not sure. I'm currently leaning towards leaving it as is and documenting this corner case. Perhaps if data binding undergoes a significant overhaul, then we can revisit this.
        Hide
        J. David Beutel added a comment -

        My usage was a list of property names filtered by authorization. So this issue is the case where the user isn't authorized to update any of the fields on that part of the domain instance.

        Of course, now that I have discovered that the empty list is a special case, it's easy for me to work around it. Regarding a breaking change in 2.0, I would weigh that against how many security holes it would fix, for people who were not aware that this is a special case.

        Show
        J. David Beutel added a comment - My usage was a list of property names filtered by authorization. So this issue is the case where the user isn't authorized to update any of the fields on that part of the domain instance. Of course, now that I have discovered that the empty list is a special case, it's easy for me to work around it. Regarding a breaking change in 2.0, I would weigh that against how many security holes it would fix, for people who were not aware that this is a special case.
        Hide
        Jeff Scott Brown added a comment -

        The behavior has been left alone and The User Guide has been updated to clarify expectations.

        Show
        Jeff Scott Brown added a comment - The behavior has been left alone and The User Guide has been updated to clarify expectations.

          People

          • Assignee:
            Jeff Scott Brown
            Reporter:
            J. David Beutel
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development