Wicket
  1. Wicket
  2. WICKET-1166

add sanity check on form submit for request method

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.3.0-rc1
    • Fix Version/s: 1.3.2
    • Component/s: wicket
    • Labels:
      None
    • Environment:
      Safari 3

      Description

      When refreshing a frameset that includes an already POST submitted Wicket form in a frame, using the redirect to render strategy, Safari erroneously requests the form's original target by GET, rather than the location that was eventually redirected to. Therefore none of the form values are available in the request object and NPEs will occur trying to access them in places like AbstractConverter.java:55.

      Because Form allows for a particular request method to be specified, I think it should also confirm that the expected method was used instead of waiting for an NPE in validation. The outcome is the same, but the cause of the error (the client) would be more evident in server logs, etc. Patch to come...

      1. submit-method-javadoc.patch
        1 kB
        Nathan Hamblen
      2. submit-method.patch
        1 kB
        Nathan Hamblen

        Activity

        Hide
        Igor Vaynberg added a comment -

        applied javadoc patch. no processing logic was changed

        Show
        Igor Vaynberg added a comment - applied javadoc patch. no processing logic was changed
        Hide
        Nathan Hamblen added a comment -

        I'm not sure where the formal requirements would be, but I do find the method's documentation misleading for the current behavior: "Gets the method used to submit the form." As I've said I think it's a reasonable solution to just update the javadocs, so, here's a patch that does that.

        In the longer run I still think it would be better to lock down the submit process a little more. The same reasons that that is difficult are the reasons I think it should be done: with ajax and nesting, forms have become a pretty fuzzy concept in Wicket. For a user looking at the API it's not clear what will happen under various scenarios. But, that's just my opinion. Feel free to close with or without this javadoc patch.

        Show
        Nathan Hamblen added a comment - I'm not sure where the formal requirements would be, but I do find the method's documentation misleading for the current behavior: "Gets the method used to submit the form." As I've said I think it's a reasonable solution to just update the javadocs, so, here's a patch that does that. In the longer run I still think it would be better to lock down the submit process a little more. The same reasons that that is difficult are the reasons I think it should be done: with ajax and nesting, forms have become a pretty fuzzy concept in Wicket. For a user looking at the API it's not clear what will happen under various scenarios. But, that's just my opinion. Feel free to close with or without this javadoc patch.
        Hide
        Ate Douma added a comment -

        I don't think this check on the method used is correct in this situation. As Johan said, its for generating markup and I don't think it should not (need to) be enforced during the submit.
        With regards to portlets, yeah: potentially there might be an issue too, at least for JSR-168 (Portlet API 1.0).
        The current 1.0 portlet spec doesn't formally support servlet access during processAction (when a form submit is processed), but through the Apache Portals Bridge ServletContext solution it actually is possible to do so and WicketPortlet uses it as such. Now, as a servlet context doesn't formally "exist" in Portlet API 1.0, it really depends on the Portal implementation how/what the getMethod() will return.
        Most likely simply the value as provided by the underlying servlet request, as in the case of Jetspeed-2 portal, but other portals might do it differently.

        Now, for the imminent Portlet API 2.0 (JSR-286, which finally has been submitted to the JCP for validation), this problem will be solved: as it then formally allows servlet access during processAction and getMethod() will return the original value. So, exactly as the current Apache Portlet Bridge and Jetspeed-2 are doing.

        To cut it short, most likely there won't be a problem with portlets and definitely not when using JSR-286.

        Still, I don't think this specific use-case (a bug in Safari) warrants such a check which is too strict and beyond the formal requirements in my view.

        Show
        Ate Douma added a comment - I don't think this check on the method used is correct in this situation. As Johan said, its for generating markup and I don't think it should not (need to) be enforced during the submit. With regards to portlets, yeah: potentially there might be an issue too, at least for JSR-168 (Portlet API 1.0). The current 1.0 portlet spec doesn't formally support servlet access during processAction (when a form submit is processed), but through the Apache Portals Bridge ServletContext solution it actually is possible to do so and WicketPortlet uses it as such. Now, as a servlet context doesn't formally "exist" in Portlet API 1.0, it really depends on the Portal implementation how/what the getMethod() will return. Most likely simply the value as provided by the underlying servlet request, as in the case of Jetspeed-2 portal, but other portals might do it differently. Now, for the imminent Portlet API 2.0 (JSR-286, which finally has been submitted to the JCP for validation), this problem will be solved: as it then formally allows servlet access during processAction and getMethod() will return the original value. So, exactly as the current Apache Portlet Bridge and Jetspeed-2 are doing. To cut it short, most likely there won't be a problem with portlets and definitely not when using JSR-286. Still, I don't think this specific use-case (a bug in Safari) warrants such a check which is too strict and beyond the formal requirements in my view.
        Hide
        Nathan Hamblen added a comment -

        I don't know... I think ajax is a more immediate problem. The patch I uploaded throws the exception when a post form is submitted by ajax. So that's already a no-go, unless we're talking about adding some ajax detection to the logic.

        Maybe the fix is just to alter the documentation surrounding Form's "method" property to say that it's a recommendation to the markup engine and doesn't guarantee or even influence how the form is processed.

        Show
        Nathan Hamblen added a comment - I don't know... I think ajax is a more immediate problem. The patch I uploaded throws the exception when a post form is submitted by ajax. So that's already a no-go, unless we're talking about adding some ajax detection to the logic. Maybe the fix is just to alter the documentation surrounding Form's "method" property to say that it's a recommendation to the markup engine and doesn't guarantee or even influence how the form is processed.
        Hide
        Igor Vaynberg added a comment -

        is something like this going to screw with portlets at all?

        Show
        Igor Vaynberg added a comment - is something like this going to screw with portlets at all?
        Hide
        Johan Compagner added a comment -

        Its just for generating markup.
        But now in this hyrbid world other things could happen yes.

        So if a form is made for a get. But also used in ajax and then as a post that is not a problem for me
        no exception have to be thrown then.

        Show
        Johan Compagner added a comment - Its just for generating markup. But now in this hyrbid world other things could happen yes. So if a form is made for a get. But also used in ajax and then as a post that is not a problem for me no exception have to be thrown then.
        Hide
        Nathan Hamblen added a comment -

        Good catch! If you have the form with method="get", and then submit by ajax it throws an exception with this patch. I wonder if adding ajax submit buttons to a form shouldn't force a method="post", since that is the method that will be used for ajax regardless? Seems a little confusing to have a GET form that is actually posted.

        I guess it comes down to what getMethod() is supposed to mean, if it's just for the markup or if it implies how the form is expected and required to be submitted. If it's just for the markup the method description should probably not say "the method used to submit the form".

        Show
        Nathan Hamblen added a comment - Good catch! If you have the form with method="get", and then submit by ajax it throws an exception with this patch. I wonder if adding ajax submit buttons to a form shouldn't force a method="post", since that is the method that will be used for ajax regardless? Seems a little confusing to have a GET form that is actually posted. I guess it comes down to what getMethod() is supposed to mean, if it's just for the markup or if it implies how the form is expected and required to be submitted. If it's just for the markup the method description should probably not say "the method used to submit the form".
        Hide
        Igor Vaynberg added a comment -

        what about forms submitted via ajax?

        Show
        Igor Vaynberg added a comment - what about forms submitted via ajax?
        Hide
        Nathan Hamblen added a comment -

        I tested the same scenario with a GET and there was no null pointer; it acts as if the form had been resubmitted normally.

        Show
        Nathan Hamblen added a comment - I tested the same scenario with a GET and there was no null pointer; it acts as if the form had been resubmitted normally.
        Hide
        Johan Compagner added a comment -

        if the form was somehow a GET what happens then? Do you still get the nullpointer? Because then it is the method is the same ..

        Show
        Johan Compagner added a comment - if the form was somehow a GET what happens then? Do you still get the nullpointer? Because then it is the method is the same ..
        Hide
        Nathan Hamblen added a comment -

        So, anyway, here is my little patch. I doubt it will live up to the five alarm fire I set trying to upload it.

        Show
        Nathan Hamblen added a comment - So, anyway, here is my little patch. I doubt it will live up to the five alarm fire I set trying to upload it.

          People

          • Assignee:
            Igor Vaynberg
            Reporter:
            Nathan Hamblen
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development