Hi Jesse, Thanks for identifying these problems and working on the improvements!
I've added some comments about small issues to https://github.com/apache/wicket/commit/c63989018a8fb11a3dbbc68253e74f3fd2117430 . I like the new setting used for the new "crypt." prefix used for the page expired notification. Is it good idea to use such settings/flags to be able to tell what to be encrypted (componentAndPageInfo, query strings, all) explicitly ? At the moment this is "calculated". I ask whether it is sensible to keep the current code that decides what to encrypt and what not and add settings which will define this explicitly ? Martin Grigorov Wicket Training and Consulting https://twitter.com/mtgrigorov On Thu, Oct 9, 2014 at 7:23 PM, Jesse Long <[email protected]> wrote: > Hi All, > > We have had multiple requests from users bemoaning the fact that > CryptoMapper does not encrypt request listener URLs for mounted pages and > the home page. I've been working on CryptoMapper to add support for this, > but there is some change in behavior, so I am asking for review of the > changes before I commit. > > The changes are in two commits to sandbox/cryptomapper-request-listener. > > We're talking about these URLs: /mount/path?5-1.ILinkListener-link > > In WICKET-4140, Martin provides a custom CryptoMapper, as do I in > WICKET-5326. (I was not aware of the solution in 4140). Its time to have > the solution provided by default. > > I have modified CryptoMapper to only encrypt all parts of the URL > (segments as well as query parameters) if the URL starts with "/wicket/". > This covers all wicket generated URLs, except home page and mounts. > > For mounted pages and the home page, we only encrypt the PageComponentInfo > parameter - the "5-1.ILinkListener-link" part. We dont want to encrypt all > query parameters, because people want to be able to edit some query > parameters. > > /my/mounted/page?5-1.ILinkListener-link becomes /my/mounted/page?wicket= > asdjkghasfhajksdhfjksdhfkjsdhfkjsdk. > > This means that CryptoMapper now can, and probably should, be installed > AFTER mounting pages etc. > > It also means that URLs generated for pages that are mounted before > CryptoMapper is installed will no longer be encrypted in full, only the > PageComponentInfo parameter will be encrypted. I dont think this is a > problem though, since why would you mount a page and then be upset that the > mounted URL is being used? > > I imagine that there will be times when a page is mounted after > CryptoMapper is installed, but the user would like request listener URLs > for this page to be encrypted too, and will therefore install a second > CryptoMapper. I added test to make sure we can support this. > > Issue #2: > > Sometimes users use KeyInSessionSunJceCryptFactory, which stores the > crypt key in the session, unique per session. When the session expires, and > a user attempt to open a URL encrypted by the now expired session, the URL > cannot be decrypted and he gets a 404 error. > > In the discussion around WICKET-5326, a wanted to see a > PageExpiredException in these circumstances, not 404. Makes sense to me. > > To do this I have added a setting to CryptoMapper which will prepend > "crypt." to fully encrypted URLs, to clearly mark the URL as definitely > encrypted, and definitely should be decryptable. When the URL is decrypted > later, CryptoMapper knows that this URL should definitely be decryptable > and, if not, it can throw a PageExpiredException. > > This option is off by default. > > Also included are some other small fixes for CryptoMapper, like > getCompatabilityScore() not really working, and encrypted URLs being too > long by one segment. > > Please let me know if this is all fine, then I will delete the sandbox > branch and put these change into wicket-6.x and master. > > Thanks, > Jesse >
