Tapestry 5
  1. Tapestry 5
  2. TAP5-879

404 is never raised automatically if the application has an index page.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: 5.1.0.5
    • Fix Version/s: None
    • Component/s: tapestry-core
    • Labels:
      None

      Description

      The default behavior of PageRenderDispatcher when a user access to a URL like 'http://localhost/demo/blah' (where 'demo' is the application context and 'blah' is a page that does not exist) is to translate to 'http://localhost/demo/index/blah' if an index page exists even if it has no activation method.

      It could be a better solution to check if a the index page has an activation method with the corresponding parameter number and type, and automatically raise a http 404 if not.

      1. TAP5-879.txt
        18 kB
        Christophe Cordenier
      2. TAP5-986.txt
        25 kB
        Christophe Cordenier

        Issue Links

          Activity

          Hide
          Massimo Lusetti added a comment -

          A new issue has been created to reflect what I would like to achieve.

          Here we are talking only about Index pages and I want all pages to behave the same so I ask you to follow TAP5-2070 which will implement this plus more.

          I you feel strong about this particular issue, please reopen.

          Show
          Massimo Lusetti added a comment - A new issue has been created to reflect what I would like to achieve. Here we are talking only about Index pages and I want all pages to behave the same so I ask you to follow TAP5-2070 which will implement this plus more. I you feel strong about this particular issue, please reopen.
          Hide
          Massimo Lusetti added a comment -

          I take this just to be sure it will not be closed in the bulk operation while discussion is ongoing on the dev list.

          Show
          Massimo Lusetti added a comment - I take this just to be sure it will not be closed in the bulk operation while discussion is ongoing on the dev list.
          Hide
          Ulrich Stärk added a comment -

          This issue has been last updated more than 1.5 years ago, has no assignee, affects an old version of Tapestry that is not actively developed anymore, and is therefore prone to be bulk-closed in the near future.

          If the issue still persists with the most recent development preview of Tapestry, please update it as soon as possible. In the case of a feature request, please discuss it with the Tapestry developer community on the dev@tapestry.apache.org mailing list first.

          Show
          Ulrich Stärk added a comment - This issue has been last updated more than 1.5 years ago, has no assignee, affects an old version of Tapestry that is not actively developed anymore, and is therefore prone to be bulk-closed in the near future. If the issue still persists with the most recent development preview of Tapestry, please update it as soon as possible. In the case of a feature request, please discuss it with the Tapestry developer community on the dev@tapestry.apache.org mailing list first.
          Hide
          Christophe Cordenier added a comment -

          Actually, i have implemented it successfully by overriding the OnEventWorker...

          Now i think that modify Render et ComponentEvent handlers is maybe the most simple.

          The strategy i used : if the URL has been shorten (Index page become "") and if no activation method has not been executed and the context contain parameter values, then this is surely an error...

          Show
          Christophe Cordenier added a comment - Actually, i have implemented it successfully by overriding the OnEventWorker... Now i think that modify Render et ComponentEvent handlers is maybe the most simple. The strategy i used : if the URL has been shorten (Index page become "") and if no activation method has not been executed and the context contain parameter values, then this is surely an error...
          Hide
          Josh Canfield added a comment -

          Instead of using a request filter, couldn't we just add a DefaultOnActivateWorker that comes in after OnEventWorker and checks for these conditions? (maybe it goes in OnEventWorker but that seems like a separate concern)

          I'd propose that an onActivate(EventContext) gets added if one of the "all parameter" forms doesn't exist, if it exists then we don't add. The method will get built with the knowledge of the other activate handlers and use that to decide what to do.

          What the right logic to put here is a little debatable. I don't know if we should test that the incoming parameters match exactly. I have an image component that accepts 1 parameter but is always passed 2 since I append a friendly file name for SEO. I don't implement an onActivate handler that accepts that second parameter, but maybe I should?

          Returning 404 for type mismatch is also an interesting idea that could be put into the injected default activate method.

          Show
          Josh Canfield added a comment - Instead of using a request filter, couldn't we just add a DefaultOnActivateWorker that comes in after OnEventWorker and checks for these conditions? (maybe it goes in OnEventWorker but that seems like a separate concern) I'd propose that an onActivate(EventContext) gets added if one of the "all parameter" forms doesn't exist, if it exists then we don't add. The method will get built with the knowledge of the other activate handlers and use that to decide what to do. What the right logic to put here is a little debatable. I don't know if we should test that the incoming parameters match exactly. I have an image component that accepts 1 parameter but is always passed 2 since I append a friendly file name for SEO. I don't implement an onActivate handler that accepts that second parameter, but maybe I should? Returning 404 for type mismatch is also an interesting idea that could be put into the injected default activate method.
          Hide
          Christophe Cordenier added a comment -

          New patch considering TAP5-986

          Show
          Christophe Cordenier added a comment - New patch considering TAP5-986
          Hide
          Christophe Cordenier added a comment -

          Hi,

          Finally i came up to this solution :

          1. Add a component request filter to check activation context parameters
          2. i didn't modify the matches method of ComponentEvent class to be less intrusive
          3. i didn't change the behavior at the OnEventWorker because i guess it's too late for security checking

          The algorithm is :

          1. If no activation method then no parameter is authorized
          2. If activation methods exist then verify that at least that one method matches with the activation context parameter count and type (this induces type coercion)
          3. If an activation method exists with EventContext, or List or Object[] param then it is flaged as not secured

          Remark : The patch provided contains some code from the OnEventWorker and does introspection everytime the class is invalidated and then keep metadatas in a cache.

          Aslo ,during test i have seen that this kind of code is not handled by Tapestry

          onActivate(Long number)

          {...}

          onActivate(String message) {...}

          Calling this page with a string activation context parameter generate a coercion exception, i guess this was a decision in design.

          Best Regards,
          Christophe.

          Show
          Christophe Cordenier added a comment - Hi, Finally i came up to this solution : 1. Add a component request filter to check activation context parameters 2. i didn't modify the matches method of ComponentEvent class to be less intrusive 3. i didn't change the behavior at the OnEventWorker because i guess it's too late for security checking The algorithm is : 1. If no activation method then no parameter is authorized 2. If activation methods exist then verify that at least that one method matches with the activation context parameter count and type (this induces type coercion) 3. If an activation method exists with EventContext, or List or Object[] param then it is flaged as not secured Remark : The patch provided contains some code from the OnEventWorker and does introspection everytime the class is invalidated and then keep metadatas in a cache. Aslo ,during test i have seen that this kind of code is not handled by Tapestry onActivate(Long number) {...} onActivate(String message) {...} Calling this page with a string activation context parameter generate a coercion exception, i guess this was a decision in design. Best Regards, Christophe.
          Hide
          Christophe Cordenier added a comment -

          Hello

          I have started to implement a solution , it still need to be tested, but i would like to have your opinion on the implementation choice

          1. In the PageRenderRequestHandlerImpl and ComponentEventRequestHandlerImpl

          a. Verify that the activate event has been handled if the request contains activation parameters
          b. If yes, then continue
          c. If no, then add an attribute in the request and return

          2. In the dispatcher level

          a. Each time a request handler (page render or component event) is called, check the request if the attribute is not present
          b. If yes then return false
          c. If no then return true

          Regards,
          Christophe.

          Show
          Christophe Cordenier added a comment - Hello I have started to implement a solution , it still need to be tested, but i would like to have your opinion on the implementation choice 1. In the PageRenderRequestHandlerImpl and ComponentEventRequestHandlerImpl a. Verify that the activate event has been handled if the request contains activation parameters b. If yes, then continue c. If no, then add an attribute in the request and return 2. In the dispatcher level a. Each time a request handler (page render or component event) is called, check the request if the attribute is not present b. If yes then return false c. If no then return true Regards, Christophe.
          Hide
          Christophe Cordenier added a comment -

          I am 'humbly' trying to implement a patch for this but i 'need to pick your brain' (not sure of this expression, i assume my dictionnary is right)

          To let the request pass through TapestryFilter seems to be the best solution (let the servlet container handle 404), actually as far as i know, to achieve i have to return false in the PageRenderDispatcher. My problem is how to verify if the target page implements the activation method since the only way i have found to achieve this is to check the return value of triggerEventContext ?

          Another way (that i don't really like) is to send an error in the PageRenderRequestHandler after page activation ?

          Have you any other ideas i can explore ?

          Show
          Christophe Cordenier added a comment - I am 'humbly' trying to implement a patch for this but i 'need to pick your brain' (not sure of this expression, i assume my dictionnary is right) To let the request pass through TapestryFilter seems to be the best solution (let the servlet container handle 404), actually as far as i know, to achieve i have to return false in the PageRenderDispatcher. My problem is how to verify if the target page implements the activation method since the only way i have found to achieve this is to check the return value of triggerEventContext ? Another way (that i don't really like) is to send an error in the PageRenderRequestHandler after page activation ? Have you any other ideas i can explore ?
          Hide
          Howard M. Lewis Ship added a comment -

          True, if there's an activation context, but there's no activation event handler, we should probably send a 404, i.e., let the servlet container handle the request, which amounts to the same thing.

          Show
          Howard M. Lewis Ship added a comment - True, if there's an activation context, but there's no activation event handler, we should probably send a 404, i.e., let the servlet container handle the request, which amounts to the same thing.
          Hide
          Thiago H. de Paula Figueiredo added a comment -

          Using an activation context or not is something that is decided per page, so a strict check configuration could only affect Index pages (or even only the top one). We can easily create a mixin that sends a 404 response when the activation context at least one parameter.

          Show
          Thiago H. de Paula Figueiredo added a comment - Using an activation context or not is something that is decided per page, so a strict check configuration could only affect Index pages (or even only the top one). We can easily create a mixin that sends a 404 response when the activation context at least one parameter.
          Hide
          Christophe Cordenier added a comment -

          The question is : more generally, Is this a good solution to ignore a request if the request has activation parameter and the page has not the corresponding method ? And especially if the application has an index page that is used by default.

          In the case of an Event request, an exception is thrown that make sense from a development view.
          In a Render request, ignoring the request should be a solution.

          What about a parameter for this kind of feature ? (Strict activation context checking)

          Show
          Christophe Cordenier added a comment - The question is : more generally, Is this a good solution to ignore a request if the request has activation parameter and the page has not the corresponding method ? And especially if the application has an index page that is used by default. In the case of an Event request, an exception is thrown that make sense from a development view. In a Render request, ignoring the request should be a solution. What about a parameter for this kind of feature ? (Strict activation context checking)
          Hide
          Robin Komiwes added a comment -

          Forwarding to start page is not a solution.
          HTTP 404 return code is a standard for an unavailable resource. This error code may be used for monitoring, logging ...
          With the current behavior, you are not able to distinguish a page that doesn't exist

          Show
          Robin Komiwes added a comment - Forwarding to start page is not a solution. HTTP 404 return code is a standard for an unavailable resource. This error code may be used for monitoring, logging ... With the current behavior, you are not able to distinguish a page that doesn't exist
          Hide
          Massimo Lusetti added a comment -

          I have to say that this is a strange behavior which i stumbled on quite often. It really seems that Tapestry could do better in this case as returning a 404 seems more natural.

          Show
          Massimo Lusetti added a comment - I have to say that this is a strange behavior which i stumbled on quite often. It really seems that Tapestry could do better in this case as returning a 404 seems more natural.
          Hide
          Kalle Korhonen added a comment -

          Use a Start page instead. There's a different between Start and Index page. Start page handles a request to context path only, but Index handles is sort of a catch all (try it: if you have a Start page, your application will return 404s for non-existent urls). Personally, I use a Start page for the root of the context, but Index pages for sub-folders.

          Show
          Kalle Korhonen added a comment - Use a Start page instead. There's a different between Start and Index page. Start page handles a request to context path only, but Index handles is sort of a catch all (try it: if you have a Start page, your application will return 404s for non-existent urls). Personally, I use a Start page for the root of the context, but Index pages for sub-folders.

            People

            • Assignee:
              Massimo Lusetti
              Reporter:
              Christophe Cordenier
            • Votes:
              5 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development