Hi Thiago! a specialized Exception sounds good, maybe something like "TapestryDispatchException" with an HTTP status and an optional message and even a redirect URL. This way, it would be more flexible than just handling status 400.
Even though the problem only occurs if @ActivationRequestParameter is used, I think it's still a good overall addition, allowing for a flexible request flow interruption mechanism with a sensible HTTP status. I think about it a little bit more and create an issue for it. Cheers, Ben On Mon, Aug 23, 2021 at 4:07 PM Thiago H. de Paula Figueiredo < thiag...@gmail.com> wrote: > Hello, Ben! > > That's an interesting question with no clear answer (i.e. one that stands > out much better than the others). > > I'd go with option 2. We could create a new exception time for time and > have PageRenderDispatcher and ComponentEventDispatcher catch it and return > a proper 400 response. In addition, if we need more flexibility, we could > create a service to define what to do with bad Tapestry URL encoding and > have the default being a 400 response. > > On Fri, Aug 20, 2021 at 9:41 AM Ben Weidig <b...@netzgut.net> wrote: > > > Hi, > > > > we have a problem with the Tapestry URLEncoder and how invalid characters > > are handled in the ActivationRequestParameterWorker. > > > > If a component request URL contains unsafe characters, an > > IllegalArgumentException is thrown: > > > > > https://github.com/apache/tapestry-5/blob/b347e650ebf1204b87220166c7d126129b5ee460/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/URLEncoderImpl.java#L144 > > > > Our logging setup sends a mail for every unhandled exception, and we > > receive quite a lot of these exceptions due to dumb bots trying random > > URLS. > > > > The strictness of the URLEncoder was discussed before, and the ticket is > > still open: https://issues.apache.org/jira/browse/TAP5-1803 > > The reasoning behind using a custom URLEncode and not the Java URLEncoder > > is still sound IMO and shouldn't be changed. > > > > We internally discussed how we could mitigate the problem, and the > > following ideas came to mind: > > > > Idea 1: @ActivationRequestParameter should have "emptyOnError" with a > > default value of false. > > If the decode fails, this option would set the decoded String to empty > > String. > > The value encoder would create a valid value for the component, and no > 500 > > error would occur. > > There shouldn't be any backward compatibility issues due to the default > > value "false". > > But the new option would allow us to handle invalid values by actually > > receiving a value in any case. > > > > The idea sounded good until I tested it, and an empty string isn't > leading > > to valid values from the ValueEncoder. > > For example, in the case of numeric values, it would just trade the > > IllegalArgumentException with NumberFormatException. > > > > > > Idea 2: Stop the request and return an HTTP 400. > > Why does an unencodable URL have to lead to an HTTP 500 status? > > From an HTTP workflow point of view, the server receives a request it > can't > > handle, so an HTTP 400 would be a better option because the request is > > faulty. > > Catching any IllegalArgumentException in > > > > > https://github.com/apache/tapestry-5/blob/b347e650ebf1204b87220166c7d126129b5ee460/tapestry-core/src/main/java/org/apache/tapestry5/internal/transform/ActivationRequestParameterWorker.java#L136 > > and stop the request and return status 400. > > > > The first problem we thought about is how to interrupt the request at > that > > particular point. > > The crash, in our case, is on page activation in > > ComponentEventRequestHandlerImpl. > > There should be mechanism added to allow the page activation to tell the > > ComponentEventRequestHandlerImpl that the activation failed due to "bad > > request", and not a server error. > > > > > > What do you think? > > Do you have similar issues with bots and invalid requests? > > Are we overthinking it, and there's a more straightforward way to deal > with > > the problem? > > > > Cheers > > Ben > > > > > -- > Thiago >
