MyFaces Core
  1. MyFaces Core
  2. MYFACES-1467

Validation doesn't run for required fields if submitted value is null

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.1.5-SNAPSHOT, 1.2.0-SNAPSHOT
    • Fix Version/s: 1.1.6
    • Component/s: General
    • Labels:
      None

      Description

      A component with a required value will not fail validation as expected if the submitted value is null. This issue is not seen normally because browsers send the value for an empty text field as an empty string. That is, the POST data for an empty field1 will contain the field name but no value, like field1=&field2=something. However, if you use a man-in-the-middle proxy such as Paros to remove "fieldname=" from the POST data, the submitted value will be null. UIInput.validate() skips validation for null submitted values, but since requiredness is also part of validation, the requiredness check gets skipped, too.

      1. patch2.txt
        2 kB
        David Chandler
      2. patch.txt
        0.7 kB
        David Chandler

        Issue Links

          Activity

          Hide
          Adam Winer added a comment -

          There is no conflict in the spec. It is a Renderer's responsibility to only call setSubmittedValue(null) when there is truly no submission - disabled=true, readonly=true, etc. Otherwise, it must call setSubmittedValue() with something non-null. It is free to convert that submitted value back to null later in getConvertedValue(). A Renderer that copies getRequestParameterMap().get() over to submitted value without checking for null has a bug.

          Show
          Adam Winer added a comment - There is no conflict in the spec. It is a Renderer's responsibility to only call setSubmittedValue(null) when there is truly no submission - disabled=true, readonly=true, etc. Otherwise, it must call setSubmittedValue() with something non-null. It is free to convert that submitted value back to null later in getConvertedValue(). A Renderer that copies getRequestParameterMap().get() over to submitted value without checking for null has a bug.
          Hide
          Manfred Geiler added a comment -

          deferred until 1.1.6

          Show
          Manfred Geiler added a comment - deferred until 1.1.6
          Hide
          David Chandler added a comment -

          Has the spec already been amended to address this issue? Section 3.5.5 on validator implementation now reads

          Unless otherwise specified, components with a null local value cause the
          validation checking by this Validator to be skipped. If a component should be
          required to have a non-null value, a component attribute with the name
          required and the value true must be added to the component in order to
          enforce this rule.

          I don't remember the second sentence being there before.

          At any rate, I agree that the spec doesn't resolve the conflict between required=true and disabled/readonly=true in the case of null values. The root issue appears to be that the spec overloads the meaning of a null submitted value. At validation time, there is currently no way to distinguish between a null value from the user and a null value because the component is readonly or disabled. Besides causing the issue at hand, it masks a parameter tampering attack (changing the value of a readonly or disabled component), which is also of security interest.

          So the spec needs to
          1. Discontinue overloading the meaning of a null submitted value (perhaps by the introduction of new properties to UIInput) and
          2. Explicitly address the conflict between required and readonly / disabled for null values

          Are we agreed on this? Are there other proposals?

          In the mean time, can we leave the issue open until the spec issue is resolved? JSF security issues need to be addressed, and if users begin to depend on insecure behavior (as they have been), it will only be that much harder to fix later.

          Show
          David Chandler added a comment - Has the spec already been amended to address this issue? Section 3.5.5 on validator implementation now reads Unless otherwise specified, components with a null local value cause the validation checking by this Validator to be skipped. If a component should be required to have a non-null value, a component attribute with the name required and the value true must be added to the component in order to enforce this rule. I don't remember the second sentence being there before. At any rate, I agree that the spec doesn't resolve the conflict between required=true and disabled/readonly=true in the case of null values. The root issue appears to be that the spec overloads the meaning of a null submitted value. At validation time, there is currently no way to distinguish between a null value from the user and a null value because the component is readonly or disabled. Besides causing the issue at hand, it masks a parameter tampering attack (changing the value of a readonly or disabled component), which is also of security interest. So the spec needs to 1. Discontinue overloading the meaning of a null submitted value (perhaps by the introduction of new properties to UIInput) and 2. Explicitly address the conflict between required and readonly / disabled for null values Are we agreed on this? Are there other proposals? In the mean time, can we leave the issue open until the spec issue is resolved? JSF security issues need to be addressed, and if users begin to depend on insecure behavior (as they have been), it will only be that much harder to fix later.
          Hide
          Matthias Weßendorf added a comment -

          All,

          since the added patch (added because of security issues) causes too much side effect issues with others.
          We can't apply the second patch. That guy is adding new methods to UIInput, which we can't do, since that class is part of the SPEC.

          David, I'd like to close this issue (won't fix (or illegal)), since I see/know (or even saw ) your concerns, can you nail that bug to JSF SPEC at Java.net?
          So an issue like this needs to be addressed by the spec guys.

          sorry,
          Matthias

          Show
          Matthias Weßendorf added a comment - All, since the added patch (added because of security issues) causes too much side effect issues with others. We can't apply the second patch. That guy is adding new methods to UIInput, which we can't do, since that class is part of the SPEC. David, I'd like to close this issue (won't fix (or illegal)), since I see/know (or even saw ) your concerns, can you nail that bug to JSF SPEC at Java.net? So an issue like this needs to be addressed by the spec guys. sorry, Matthias
          Hide
          Cagatay Civici added a comment -

          This issue leads to other problems for us since it's related to core UIInput. Simply it's illegal to add checks for each of attributes like disabled, readOnly, displayValueOnly, it's hard to maintain too.

          Show
          Cagatay Civici added a comment - This issue leads to other problems for us since it's related to core UIInput. Simply it's illegal to add checks for each of attributes like disabled, readOnly, displayValueOnly, it's hard to maintain too.
          Hide
          Stefan Betermieux added a comment -

          changes in UIInput.validate() affect displayValueOnly behaviour.

          Show
          Stefan Betermieux added a comment - changes in UIInput.validate() affect displayValueOnly behaviour.
          Hide
          Stefan Betermieux added a comment -

          Hi,

          I am coming from TOMAHAWK-850 where I have reported errors regarding displayValueOnly="true" and required="true" on tomahawk UIInput components. The current svn and the latest patch (patch2.txt) will not resolve these issues. I know that this should be handled as an issue in TOMAHAWK, but fixing the problem can only begin when UIInput settled.

          Stefan

          Show
          Stefan Betermieux added a comment - Hi, I am coming from TOMAHAWK-850 where I have reported errors regarding displayValueOnly="true" and required="true" on tomahawk UIInput components. The current svn and the latest patch (patch2.txt) will not resolve these issues. I know that this should be handled as an issue in TOMAHAWK, but fixing the problem can only begin when UIInput settled. Stefan
          Hide
          David Chandler added a comment -

          Jeff, Cristi, you're absolutely right. There is no conflict between the required and disabled/readonly attributes--that's what I get for posting after a long week in another time zone. It is possible to preserve the prior security benefit as well as restore the old behavior, and I've provided patch2.txt to that end. In order to beef up the validate() method, I added isDisabled() and isReadonly() methods which read from the special component attributes map since not all UIInput components have the readonly and disabled properties.

          At first, I was concerned that parameter tampering might be possible when disabled or readonly is true; however, this is prevented by HtmlRendererUtils.decodeUIInput, which doesn't set the submitted value if the component is disabled or readonly, which is no doubt why the unamended spec said that validation implementations should skip validation if the submitted value is null. In essence, JSF has always ignored user input for disabled and readonly components, and the new patch preserves that behavior, while still preventing parameter tampering if a required value is missing from a component that is enabled for editing (i.e., not readonly or disabled).

          Please give patch2 a whirl. I confirmed that I do not get validation errors when using both required=true and disabled=true.

          Show
          David Chandler added a comment - Jeff, Cristi, you're absolutely right. There is no conflict between the required and disabled/readonly attributes--that's what I get for posting after a long week in another time zone. It is possible to preserve the prior security benefit as well as restore the old behavior, and I've provided patch2.txt to that end. In order to beef up the validate() method, I added isDisabled() and isReadonly() methods which read from the special component attributes map since not all UIInput components have the readonly and disabled properties. At first, I was concerned that parameter tampering might be possible when disabled or readonly is true; however, this is prevented by HtmlRendererUtils.decodeUIInput, which doesn't set the submitted value if the component is disabled or readonly, which is no doubt why the unamended spec said that validation implementations should skip validation if the submitted value is null. In essence, JSF has always ignored user input for disabled and readonly components, and the new patch preserves that behavior, while still preventing parameter tampering if a required value is missing from a component that is enabled for editing (i.e., not readonly or disabled). Please give patch2 a whirl. I confirmed that I do not get validation errors when using both required=true and disabled=true.
          Hide
          Jeff Bischoff added a comment -

          I have also noticed the breakage in my code that Cristi noted. For some fields, I have disabled bound to a bean property, but required hard-coded to "true". In these cases, the new patch is causing me to get validation errors where I didn't used to see them.

          Of course as a user, this problem can be avoided with something like:

          <h:inputText disabled="#

          {bean.disabled}

          " required="#

          {not bean.disabled}

          " />

          However, for those of us with large, existing applications that depend on the old behaviour, this would need to be changed in a LOT of places. IMHO, the old behaviour was rather intuitive. However, after reading this thread I think that perhaps the original way this was implemented was perhaps oversimplified. Validation should be skipped when the component is disabled or read-only, but not whenever the value is null. Is there a way we can keep the patch to fix the security hole, but yet restore the old behaviour specifically for disabled and read-only use cases?

          Jeff Bischoff

          Show
          Jeff Bischoff added a comment - I have also noticed the breakage in my code that Cristi noted. For some fields, I have disabled bound to a bean property, but required hard-coded to "true". In these cases, the new patch is causing me to get validation errors where I didn't used to see them. Of course as a user, this problem can be avoided with something like: <h:inputText disabled="# {bean.disabled} " required="# {not bean.disabled} " /> However, for those of us with large, existing applications that depend on the old behaviour, this would need to be changed in a LOT of places. IMHO, the old behaviour was rather intuitive. However, after reading this thread I think that perhaps the original way this was implemented was perhaps oversimplified. Validation should be skipped when the component is disabled or read-only, but not whenever the value is null. Is there a way we can keep the patch to fix the security hole, but yet restore the old behaviour specifically for disabled and read-only use cases? Jeff Bischoff
          Hide
          David Chandler added a comment -

          Cristi,

          Good catch. Just to clarify, you're saying what's broken is that you can no longer use required="true" and disabled="true" together, correct? I'm actually surprised this ever worked, as these two attributes would seem in conflict with each other. Conceptually, if an input component is disabled, validation can be skipped, so the conditions could be added; however, the spec does not exempt validation from occurring in the case of disabled or readonly, whereas it explicitly states that required="true" with a null value should result in an error. Therefore, a strict reading of the spec would seem to indicate that required="true" and disabled="true" should in fact result in a validation error because required="true" implicitly takes precedence.

          Having said that, I'm OK with adding conditions to skip validation for disabled or readonly components even when required="true", as this was the de facto behavior before this fix.

          I understand now how a submitted value of null is "special." Rather than put in explicit checks to call validation only when a field is editable (not disabled or readonly), the current code relies on the fact that the submitted value is null in these cases, and therefore validation is skipped. Whether this was by design or by accident, I wonder. Interestingly, UIInput.processValidators() method skips validation if the component is not rendered. Perhaps we should add checks for disabled and readonly in the same method, as validate() needn't even be called if these attributes are true.

          I guess it all depends on how strictly we interpret the spec regarding the required attribute. As for where we put the conditions, that depends on whether it ever possible for a disabled or readonly component to have a non-null submitted value. Does anyone know?

          /dmc

          Show
          David Chandler added a comment - Cristi, Good catch. Just to clarify, you're saying what's broken is that you can no longer use required="true" and disabled="true" together, correct? I'm actually surprised this ever worked, as these two attributes would seem in conflict with each other. Conceptually, if an input component is disabled, validation can be skipped, so the conditions could be added; however, the spec does not exempt validation from occurring in the case of disabled or readonly, whereas it explicitly states that required="true" with a null value should result in an error. Therefore, a strict reading of the spec would seem to indicate that required="true" and disabled="true" should in fact result in a validation error because required="true" implicitly takes precedence. Having said that, I'm OK with adding conditions to skip validation for disabled or readonly components even when required="true", as this was the de facto behavior before this fix. I understand now how a submitted value of null is "special." Rather than put in explicit checks to call validation only when a field is editable (not disabled or readonly), the current code relies on the fact that the submitted value is null in these cases, and therefore validation is skipped. Whether this was by design or by accident, I wonder. Interestingly, UIInput.processValidators() method skips validation if the component is not rendered. Perhaps we should add checks for disabled and readonly in the same method, as validate() needn't even be called if these attributes are true. I guess it all depends on how strictly we interpret the spec regarding the required attribute. As for where we put the conditions, that depends on whether it ever possible for a disabled or readonly component to have a non-null submitted value. Does anyone know? /dmc
          Hide
          Matthias Weßendorf added a comment -

          fix is missing disabled and readonly properties

          Show
          Matthias Weßendorf added a comment - fix is missing disabled and readonly properties
          Hide
          Grigoras Cristinel added a comment -

          Mathias,

          Yes the same behavior.

          Cristi

          Show
          Grigoras Cristinel added a comment - Mathias, Yes the same behavior. Cristi
          Hide
          Matthias Weßendorf added a comment -

          Cristi-

          I also expect readonly impact.
          can you test ?

          -M

          Show
          Matthias Weßendorf added a comment - Cristi- I also expect readonly impact. can you test ? -M
          Hide
          Grigoras Cristinel added a comment -

          Hi ,

          This change in code have break the disabled atribute behavior.
          How disabled will work now ? I have a field required in new record, and disabled in edit mode.
          And destroy the displayValueOnly enhancement in tomahawk.

          Cristi

          Show
          Grigoras Cristinel added a comment - Hi , This change in code have break the disabled atribute behavior. How disabled will work now ? I have a field required in new record, and disabled in edit mode. And destroy the displayValueOnly enhancement in tomahawk. Cristi
          Hide
          Thomas Spiegl added a comment -

          I applied the original patch to UIInput. In my oppinion the patch does not break the spec which says in 3.5.5 (Standard Validator Implementations): "Unless otherwise specified, components with a null local value cause the validation checking by this validator to be skipped."

          The current behavior can be described as follows: If the submitted value is null and the components required attribute is set true, then the javax.faces.component.UIInput.REQUIRED message is registered and UIInput.valid is set to false. No further validation takes place, so that all posibly registered validators are being skipped.

          The patch passed all TCK tests.

          Show
          Thomas Spiegl added a comment - I applied the original patch to UIInput. In my oppinion the patch does not break the spec which says in 3.5.5 (Standard Validator Implementations): "Unless otherwise specified, components with a null local value cause the validation checking by this validator to be skipped." The current behavior can be described as follows: If the submitted value is null and the components required attribute is set true, then the javax.faces.component.UIInput.REQUIRED message is registered and UIInput.valid is set to false. No further validation takes place, so that all posibly registered validators are being skipped. The patch passed all TCK tests.
          Hide
          David Chandler added a comment -

          I suspect there are still other issues here, but to get the immediate security problem resolved, I will proceed with Adam's suggestion to patch the decode() method of the TextInput Renderer. It turns out that HtmlRendererUtils.decodeUIInput() is logging a warning when it detects the missing client_id in the request, but is taking no further action. It seems this would be a fine place to set the submitted value to the empty string so as to trigger requiredness validation. Thoughts?

          if(paramMap.containsKey(clientId))

          { ((EditableValueHolder) component).setSubmittedValue(paramMap .get(clientId)); }

          else

          { log.warn( "There should always be a submitted value for an input if it" + " is rendered, its form is submitted, and it is not disabled" + " or read-only. Component : "+ RendererUtils.getPathToComponent(component)); }

          Thanks,
          /dmc

          Show
          David Chandler added a comment - I suspect there are still other issues here, but to get the immediate security problem resolved, I will proceed with Adam's suggestion to patch the decode() method of the TextInput Renderer. It turns out that HtmlRendererUtils.decodeUIInput() is logging a warning when it detects the missing client_id in the request, but is taking no further action. It seems this would be a fine place to set the submitted value to the empty string so as to trigger requiredness validation. Thoughts? if(paramMap.containsKey(clientId)) { ((EditableValueHolder) component).setSubmittedValue(paramMap .get(clientId)); } else { log.warn( "There should always be a submitted value for an input if it" + " is rendered, its form is submitted, and it is not disabled" + " or read-only. Component : "+ RendererUtils.getPathToComponent(component)); } Thanks, /dmc
          Hide
          David Chandler added a comment -

          Alas, I am in anguish, Adam. I do not see how the proposed patch violates the spec.

          If a submitted value is null and the field is required, the patched line will not return immediately as before, but it will proceed to validateValue(), which does indeed return for empty (null or zero-length) values without calling a validator. Perhaps the validateValue() method would be better named callValidatorIfNotEmpty(), because that's what it actually does.

          Further, the CURRENT code does NOT meet spec section 3.5.4 for the case I have demonstrated. I don't know how the TCK works, but I highly doubt it's simulating a MITM tool to test compliance with section 3.5.4. From a security point of view, the MITM case is really the only one that matters. I'm in banking, and we do this kind of penetration testing routinely, so it is important to me that MyFaces is 100% secure against MITM attacks. I have thus far been very pleased with MyFaces as this is the only vulnerability of its kind (parameter tampering) which I have found in the code to date, and I am doing my best to help maintain its quality.

          Thank you,
          /dmc

          Show
          David Chandler added a comment - Alas, I am in anguish, Adam. I do not see how the proposed patch violates the spec. If a submitted value is null and the field is required, the patched line will not return immediately as before, but it will proceed to validateValue(), which does indeed return for empty (null or zero-length) values without calling a validator. Perhaps the validateValue() method would be better named callValidatorIfNotEmpty(), because that's what it actually does. Further, the CURRENT code does NOT meet spec section 3.5.4 for the case I have demonstrated. I don't know how the TCK works, but I highly doubt it's simulating a MITM tool to test compliance with section 3.5.4. From a security point of view, the MITM case is really the only one that matters. I'm in banking, and we do this kind of penetration testing routinely, so it is important to me that MyFaces is 100% secure against MITM attacks. I have thus far been very pleased with MyFaces as this is the only vulnerability of its kind (parameter tampering) which I have found in the code to date, and I am doing my best to help maintain its quality. Thank you, /dmc
          Hide
          Adam Winer added a comment -

          It cannot be fixed in UIInput "once for all", because the spec describes how UIInput must behave. There are no options other than spec compliance.

          For the specific choice for cross-field validation, adding a component as the last one is not a good strategy, since this will break in future partial lifecycle executions (e.g., DynaFaces). Also, don't know why you'd want to make such a component a UIInput, since it really doesn't have a value of its own.

          Show
          Adam Winer added a comment - It cannot be fixed in UIInput "once for all", because the spec describes how UIInput must behave. There are no options other than spec compliance. For the specific choice for cross-field validation, adding a component as the last one is not a good strategy, since this will break in future partial lifecycle executions (e.g., DynaFaces). Also, don't know why you'd want to make such a component a UIInput, since it really doesn't have a value of its own.
          Hide
          David Chandler added a comment -

          Thanks everyone for your contributions.

          If I may summarize Craig's comments, skipping validation for a null value is a good thing, whereas skipping requiredness check for a null value is not. Correct?

          Adam, the approach of fixing it only in h:inputText renderer gives me concern because it leaves this issue hanging for any custom components people write (and possibly others of the base components). You can use an MITM proxy to wipe a submitted value for any component, not just h:inputText. Therefore, I'd really rather fix it in UIInput once for all. Can you elaborate on your concerns with this approach?

          As to a cross-field validation "framework", I have successfully used the approach of creating a component <my:formValidator validator="id or bean.method">. You put it as the last field in a form, and the component internally does a setSubmittedValue("NO_OP") so as to force the validation method specified in the attribute to run. I'd be happy to contribute my 3-line component to Tomahawk if desired

          Show
          David Chandler added a comment - Thanks everyone for your contributions. If I may summarize Craig's comments, skipping validation for a null value is a good thing, whereas skipping requiredness check for a null value is not. Correct? Adam, the approach of fixing it only in h:inputText renderer gives me concern because it leaves this issue hanging for any custom components people write (and possibly others of the base components). You can use an MITM proxy to wipe a submitted value for any component, not just h:inputText. Therefore, I'd really rather fix it in UIInput once for all. Can you elaborate on your concerns with this approach? As to a cross-field validation "framework", I have successfully used the approach of creating a component <my:formValidator validator="id or bean.method">. You put it as the last field in a form, and the component internally does a setSubmittedValue("NO_OP") so as to force the validation method specified in the attribute to run. I'd be happy to contribute my 3-line component to Tomahawk if desired
          Hide
          Martin Marinschek added a comment -

          Yes, I agree that this might be better off in an extended framework. But how to build this extended framework if no validator ever gets called when the value is null? I don't have a place to hook in the framework then (at least to existing components extending from the standard components - Trinidad has it easier here, with a completely different component hierarchy)...

          regards,

          Martin

          Show
          Martin Marinschek added a comment - Yes, I agree that this might be better off in an extended framework. But how to build this extended framework if no validator ever gets called when the value is null? I don't have a place to hook in the framework then (at least to existing components extending from the standard components - Trinidad has it easier here, with a completely different component hierarchy)... regards, Martin
          Hide
          Adam Winer added a comment -

          With regards to the original issue: the bug is not in UIInput, but is in fact in the decode() implementation for the h:inputText Renderer. If the field is expected to be submitted, a request parameter of null should get changed to the empty string, because a submitted value of null is special.

          W/regards to all Craig said, yes, the current behavior in the component space is exactly as intended.

          Show
          Adam Winer added a comment - With regards to the original issue: the bug is not in UIInput, but is in fact in the decode() implementation for the h:inputText Renderer. If the field is expected to be submitted, a request parameter of null should get changed to the empty string, because a submitted value of null is special. W/regards to all Craig said, yes, the current behavior in the component space is exactly as intended.
          Hide
          Matthias Weßendorf added a comment -

          I pretty much think that the single value validation is a good thing. JSF is a spec not a product.
          so myfaces or ri can (not must) handle this different.

          At least it (single v. valid.) protcets you from bad bad hooks of the frameworks itself.

          at least you need to write a framework on top of faces for that yourself.
          (which only you (or a community) is in charge of it)

          Show
          Matthias Weßendorf added a comment - I pretty much think that the single value validation is a good thing. JSF is a spec not a product. so myfaces or ri can (not must) handle this different. At least it (single v. valid.) protcets you from bad bad hooks of the frameworks itself. at least you need to write a framework on top of faces for that yourself. (which only you (or a community) is in charge of it)
          Hide
          Craig McClanahan added a comment -

          > what do you say to my reasoning for cases where required is either true or false, depending on the value of another component?

          I say two things:

          • JSF validation is all about single values – cross field validation
            is left to the application, or to frameworks built on top of JSF
            (i.e. it's reasonable to build a "validation framework" extending
            JSF that does this kind of thing, but it's out of scope for the JSF
            validation APIs themselves, at least for 1.0).
          • Firing validators on null values doesn't solve your use case anyway. You are going
            to need to do something application specific anyway. The current APIs
            are nowhere near rich enough to express all of the possible cross field
            scenarios that you would need to cover to be complete.

          In the short term (i.e. before you can convince some future JSF expert group to change this), the best advice might be to build a standalone validation framework that deals with all the possible cross-field type issues, rather than trying to coerce individual components to behave differently than the JSF standard ones do. Also, keep an eye on JSRs like #303 (annotations based validation rules), which will be playing in this same space.

          Show
          Craig McClanahan added a comment - > what do you say to my reasoning for cases where required is either true or false, depending on the value of another component? I say two things: JSF validation is all about single values – cross field validation is left to the application, or to frameworks built on top of JSF (i.e. it's reasonable to build a "validation framework" extending JSF that does this kind of thing, but it's out of scope for the JSF validation APIs themselves, at least for 1.0). Firing validators on null values doesn't solve your use case anyway. You are going to need to do something application specific anyway. The current APIs are nowhere near rich enough to express all of the possible cross field scenarios that you would need to cover to be complete. In the short term (i.e. before you can convince some future JSF expert group to change this), the best advice might be to build a standalone validation framework that deals with all the possible cross-field type issues, rather than trying to coerce individual components to behave differently than the JSF standard ones do. Also, keep an eye on JSRs like #303 (annotations based validation rules), which will be playing in this same space.
          Hide
          Martin Marinschek added a comment -

          Hi Craig,

          what do you say to my reasoning for cases where required is either true or false, depending on the value of another component?

          regards,

          Martin

          Show
          Martin Marinschek added a comment - Hi Craig, what do you say to my reasoning for cases where required is either true or false, depending on the value of another component? regards, Martin
          Hide
          Craig McClanahan added a comment -

          > On a sidenote - I believe that it is bad to skip validation at all if the value of a field is null.

          I haven't looked to see if this changed in 1.2, but I can tell you with certainty that this behavior is exactly what was intended for version 1.0. The reasoning was that, if there is no value, then there is nothing to be validated. Indeed, this is the entire reason that "required" exists as a property, instead of as a validator, in the first place.

          Craig

          Show
          Craig McClanahan added a comment - > On a sidenote - I believe that it is bad to skip validation at all if the value of a field is null. I haven't looked to see if this changed in 1.2, but I can tell you with certainty that this behavior is exactly what was intended for version 1.0. The reasoning was that, if there is no value, then there is nothing to be validated. Indeed, this is the entire reason that "required" exists as a property, instead of as a validator, in the first place. Craig
          Hide
          Martin Marinschek added a comment -

          Hi David, Cagatay, Matthias,

          this bug is indeed a problem in the sourcebase. On a sidenote - I believe that it is bad to skip validation at all if the value of a field is null.

          What if you want to have a field which is only required if a defined other field has a certain value? You can't just set the required-attribute (you can also not value-bind the required attribute properly, cause you don't necessarily have the converted and validated value of the other field in your backing bean model), and will need a special validator. Fact is that this special validator will never be executed, if the value is null, so effectively, you can't do the validation.

          I believe we should change this in the next version of the spec.

          regards,

          Martin

          Show
          Martin Marinschek added a comment - Hi David, Cagatay, Matthias, this bug is indeed a problem in the sourcebase. On a sidenote - I believe that it is bad to skip validation at all if the value of a field is null. What if you want to have a field which is only required if a defined other field has a certain value? You can't just set the required-attribute (you can also not value-bind the required attribute properly, cause you don't necessarily have the converted and validated value of the other field in your backing bean model), and will need a special validator. Fact is that this special validator will never be executed, if the value is null, so effectively, you can't do the validation. I believe we should change this in the next version of the spec. regards, Martin
          Hide
          David Chandler added a comment -

          Thanks for thinking this over, Cagatay and Matthias.

          The more I think about this, the less I think it's a spec issue, after all. In the case of the MITM exploit, neither MyFaces nor RI currently meet the requirement of section 3.5.4 of the spec, which states "If the value of this property [required] is true and the component has no value, the component is marked invalid and a message is added..."

          Section 3.5.5 immediately following begins with the caveat "Unless otherwise specified...." Well, section 3.5.4 does in fact "otherwise specify." Given the proximity of these sections, I find it likely that 3.5.4 is exactly what 3.5.5 has in mind, so there is no problem with the spec at all. Even if the sections were in conflict, I would think it a much more serious problem to ignore the required attribute than to call validation with a null value. One is an optimization, the other a security flaw.

          The code in UIInput.validateValue() method does exactly what 3.5.4 and 3.5.5 say it should do, but the patched line of code as it currently exists defeats the good code in validateValue() by not calling it for a null submitted value, even if the field is required. The patch simply ensures that the already-correct validateValue() method will run if the component is required.

          So, I go back to my position that the code in question is a security bug and also not compliant with the spec for this case. If we apply the patch, the only time users would ever see a difference between the RI and MyFaces is when using an MITM proxy. And of course, I'll be submitting this to RI, as well.

          Show
          David Chandler added a comment - Thanks for thinking this over, Cagatay and Matthias. The more I think about this, the less I think it's a spec issue, after all. In the case of the MITM exploit, neither MyFaces nor RI currently meet the requirement of section 3.5.4 of the spec, which states "If the value of this property [required] is true and the component has no value, the component is marked invalid and a message is added..." Section 3.5.5 immediately following begins with the caveat "Unless otherwise specified...." Well, section 3.5.4 does in fact "otherwise specify." Given the proximity of these sections, I find it likely that 3.5.4 is exactly what 3.5.5 has in mind, so there is no problem with the spec at all. Even if the sections were in conflict, I would think it a much more serious problem to ignore the required attribute than to call validation with a null value. One is an optimization, the other a security flaw. The code in UIInput.validateValue() method does exactly what 3.5.4 and 3.5.5 say it should do, but the patched line of code as it currently exists defeats the good code in validateValue() by not calling it for a null submitted value, even if the field is required. The patch simply ensures that the already-correct validateValue() method will run if the component is required. So, I go back to my position that the code in question is a security bug and also not compliant with the spec for this case. If we apply the patch, the only time users would ever see a difference between the RI and MyFaces is when using an MITM proxy. And of course, I'll be submitting this to RI, as well.
          Hide
          Matthias Weßendorf added a comment -

          Cagatay,

          it's not that much important, how the RI behaves... if there is the same bug, I am fine with that
          But... why is that bug there? Because the javadoc/spec says explicit to do it wrong, or at least partial wrong...

          David was showing me some cool stuff you are able to do w/ a man-in-the-middle tool.
          so the filed was required and we cut out the value from the submit request. so the value is null... but the field is still required.

          I like to see david open a sepc issue on that and putting the url as a ref here in.

          Show
          Matthias Weßendorf added a comment - Cagatay, it's not that much important, how the RI behaves... if there is the same bug, I am fine with that But... why is that bug there? Because the javadoc/spec says explicit to do it wrong, or at least partial wrong... David was showing me some cool stuff you are able to do w/ a man-in-the-middle tool. so the filed was required and we cut out the value from the submit request. so the value is null... but the field is still required. I like to see david open a sepc issue on that and putting the url as a ref here in.
          Hide
          Cagatay Civici added a comment -

          I agree, it's a spec issue rather than a myfaces issue.

          Show
          Cagatay Civici added a comment - I agree, it's a spec issue rather than a myfaces issue.
          Hide
          David Chandler added a comment -

          Agreed in part. It's actually a bug in the spec due to these conflicting requirements:

          Section 3.5.4 (Validation Processing) of the 1.1 spec states:
          "The reader-independent property required is a shorthand for the function of a required validator. If the value of this property is true and the component has no value, the component is marked invalid and a message is added..."

          This requirement is not met in the current impl when using a proxy tool or other mechanism to remove the name-value pair from the POST data.

          However, section 3.5.5 (Standard Validator Implementations) says:

          "Unless otherwise specified, components with a null local value cause the validation checking by this validator to be skipped."

          If you consider requiredness checking to be part of validation as the spec does, then these two sections are in conflict. The current code skips validation for null value as section 3.5.5 prescribes, but in doing so violates the requirement of 3.5.4, which results in the undesirable behavior that a required field may be omitted without detection simply by removing it altogether from the POST.

          Matthias and I worked on this together this afternoon at ApacheCon, so I expect him to post some more details shortly.

          Show
          David Chandler added a comment - Agreed in part. It's actually a bug in the spec due to these conflicting requirements: Section 3.5.4 (Validation Processing) of the 1.1 spec states: "The reader-independent property required is a shorthand for the function of a required validator. If the value of this property is true and the component has no value, the component is marked invalid and a message is added..." This requirement is not met in the current impl when using a proxy tool or other mechanism to remove the name-value pair from the POST data. However, section 3.5.5 (Standard Validator Implementations) says: "Unless otherwise specified, components with a null local value cause the validation checking by this validator to be skipped." If you consider requiredness checking to be part of validation as the spec does, then these two sections are in conflict. The current code skips validation for null value as section 3.5.5 prescribes, but in doing so violates the requirement of 3.5.4, which results in the undesirable behavior that a required field may be omitted without detection simply by removing it altogether from the POST. Matthias and I worked on this together this afternoon at ApacheCon, so I expect him to post some more details shortly.
          Hide
          Cagatay Civici added a comment -

          I've doubts about this, if submitted value is null it means the component isn't submitted at all and it does not make sense to run validation whether it is required or not. RI also behaves the same way.

          Show
          Cagatay Civici added a comment - I've doubts about this, if submitted value is null it means the component isn't submitted at all and it does not make sense to run validation whether it is required or not. RI also behaves the same way.
          Hide
          David Chandler added a comment -

          Fixes UIInput.validate to always check for a non-null value if isRequired() is true.

          Show
          David Chandler added a comment - Fixes UIInput.validate to always check for a non-null value if isRequired() is true.

            People

            • Assignee:
              Matthias Weßendorf
              Reporter:
              David Chandler
            • Votes:
              2 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development