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
