Fixed external links in the viewer iframe #944
Conversation
Before this fix clicking an external link in the viewer iframe had no effect (other than an error being reported in the browser dev tools console) because the attempt to navigate the top browser context was suppressed due to sandboxing.
|
@veloman-yunkan Seems to work fine on FF 111, but reopening the problem with _top target is not acceptable IMO |
|
I agree with @kelson42. I have the feeling that this PR negates the initial purpose of sandboxing. It was never to add safety but to avoid zim files breaking navigation by changing |
|
See https://caniuse.com/?search=allow-top-navigation-by-user-activation. The one browser you might be concerned by is Safari on iOS (e.g. iPad), where this workaround wouldn't work. It also wouldn't work with IE11, but I guess that's less of a concern nowadays. |
| } else { | ||
| target.setAttribute("target", "content_iframe"); |
There was a problem hiding this comment.
This change is nice.
I even wonder if you would have done this at first instead of introducing complex sandboxing.
There was a problem hiding this comment.
Well, the sandboxing was necessary to prevent scripts in the ZIM accessing external resources ("phoning home") -- a real issue with Zimit ZIMs -- and also to prevent top-level navigation. We tried lots of combinations...
There was a problem hiding this comment.
Yeah yeah, but I think is better for us to don't count the time we have spend on this 😉
Maybe (as almost all the time), the best would have been to fix the root cause and fix the generated zims.
After all, the SW embedded in the zim files is able to catch a request to www.foo.com/bar and do a request to /A/www.foo.com/bar instead. We could (and can) make the SW refuse request to unhanded url.
There was a problem hiding this comment.
Sandboxing was needed to prevent programmatic escape from the iframe (see kiwix/kiwix-tools#604). Unfortunately this trick doesn't work perfectly in Chromium
I tried to fix the concern with The attached ZIM file demonstrates the problem. There are two pages. Either page contains a link to the other page with target set to Neither page can break out of the iframe when loaded in the viewer by specifying its URL in the address bar (e.g. http://localhost:8080/viewer#naughty/naughty.html or http://localhost:8080/viewer#naughty/naughty2.html). Clicking the links also prevents the escape attempt by the links themselves. However the link from the first page to the second page enables the javascript breakout attempt (with short delay) to succeed! Note again that such behaviour is specific to Chromium (at least v90) and not to Firefox (at least v112). |
@Jaifroid Thanks for pointing out this shortcoming of this approach. I found another one before seeing your comment. It looks like we have to abandon this PR but at least it was worth a try. |
|
Replaced by #946 |
Fixes #943
Before this fix clicking an external link in the viewer iframe had no effect (other than an error being reported in the browser dev tools console) because the attempt to navigate the top browser context was suppressed due to sandboxing.
One downside with this solution is that it again opens the possibility of losing the viewer toolbar if an internal link in a ZIM file has its
targetattribute set to_top(however, such links can be detected and corrected by the viewer). I cannot think of other downsides.