zap-hud
zap-hud copied to clipboard
zaproxy
Support multiple browser tabs
Open 2 browser tabs with the HUD running. Open any of the alert dialogs in the first tab and the dialogs open in the second :/
Proposal - we associate a unique id with each tab (maybe one already exists in the tabs API?) and then include this in any post messages that are tab specific.
As part of this we'll need to remove any global variables (eg as per https://github.com/psiinon/zap-hud/pull/112#discussion_r191291024) and associate them with the tab too.
Ignore the MDN link - thats just for brower add-ons :/ https://stackoverflow.com/questions/11896160/any-way-to-identify-browser-tab-in-javascript suggests assigning a random number (guid would be better;) via session storage although duplicating tabs breaks this. Can anyone suggest any alternative options? @dscrobonia @Pamplemousse ?
Before I start diving too far into it, I do believe that we could ultimately set up an architecture where each inject script creates a unique identifier for that target page (and its corresponding HUD iframes) that could be used to send messages to and from specific tabs to the service worker.
That being said I'd suggest we design how supporting multiple tabs would fit into the entire HUD architecture first before tackling one aspect of it. And unless we expect multiple tabs for the Alpha release (which I don't belive we currently do) I'd reccommend we don't worry about it until August.
I'm afraid this feels a bit like a fundamental design issue to me :/ People use multiple tabs all of the time, and to say the HUD only supports one tab feels quite restrictive. I'm also concerned that retro fitting this could be a lot more painful than getting it right now. How about we try to design a solution for this and then decide if we actually implement it for the Alpha release based on how much work we think will be required?
@dscrobonia any thoughts on this? As I understand it the tools are all maintaining their state via the serviceworker (in indexDb) which has no concept of (or direct access to) tabs. I've been trying to work out a way to support multiple tabs for the alerts revamp and I just dont think its possible with the current architecture. I think lack of support for multiple tabs could kill the HUD. I think people are so used to using multiple tabs that they wont take the HUD seriously if it doesnt support them. We also want this release to encourage people to start hacking on the HUD. If they do actually do that then wont they have to completely rework any code they've written if we have to re-architect to add multiple tab support? Sorry, but I think this could be a blocker for the alpha release. Thoughts anyone?
Agreed, it's pretty common to use multiple tabs at the same time and it's surprising that actions done in one tab show up in another.
I think there are three main problems:
- Dialogs only open in the latest tab
- Growl alerts only open in the latest tab
- Tool data is global and not per domain / frame
This hack ensures growl alerts appear on all tabs. Ideally they should only appear on tabs on the relevant domain, but that shouldnt be too hard to do. Will update with thoughts (or maybe progress) on the other later...
Looking forward to diving back into this soon! Will update with work steps and progress when I get back into it.
What if we move from 3 separate iframes (one for drawer, left + right panel), to a single iframe that contains the entire HUD floating above the target page? That would allow us to use VueJS events rather than control everything through the service worker, and would allow us to move a lot of the global variables into a local scope. The service worker should be in charge of just receiving the websocket messages and distributing to the client windows.
That architecture would solve "Dialogs only open in the latest tab", and would probably make "tool data is global" easier to tackle
@dvas0004 so that was my first plan, when I started the HUD way back in 2016 ;) However I couldnt get the click events to reliably get through to the target site when not interacting with the HUD. But that was then, and I knew even less about client side code than I do now ;) If you can get that working then it would really simplify things!
Didn't think of that - d'oh!
But doesn't matter, since we can inject whatever we want into the page we can use the same concept but just use an injected div instead. I made a quick demo here:
https://codepen.io/anon/pen/QZgXeo
I left the background a bit greyish on purpose so you can see the "overlay". So the idea would be to:
- Inject a div which transparently overlays the entire screen
- use "pointer-events" to make sure events pass down into the target page, while making sure injected buttons actually do listen to events
The main challenge then would be the service worker. The serviceworker would now be living in the target domain which can cause some problems especially if the target already has it's own SW. The way around this IMHO would be to inject a tiny 0x0 iframe in which our SW lives, and it passes messages to the target page via the normal postMessage ( https://davidwalsh.name/window-iframe)
What do you think?
On Fri, 12 Oct 2018 at 10:09, Simon Bennetts [email protected] wrote:
@dvas0004 https://github.com/dvas0004 so that was my first plan, when I started the HUD way back in 2016 ;) However I couldnt get the click events to reliably get through to the target site when not interacting with the HUD. But that was then, and I knew even less about client side code than I do now ;) If you can get that working then it would really simplify things!
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/psiinon/zap-hud/issues/119#issuecomment-429227358, or mute the thread https://github.com/notifications/unsubscribe-auth/AFlDbcQFVmSMe-bRIGzm6Ur-e4xdvaeMks5ukEAmgaJpZM4URO9N .
@dvas0004 my biggest concern here is security. Right now we only ever call ZAP from our HUD domain, either from the UI pages or the service worker. We can send messages from the target domain to the HUD domain but thats using shared keys and the plan is to allow this to be switched off completely. If the target site can interact directly with ZAP then its game over - the target site will (probably) be able to compromise the users machine. I think we have a pretty good security model right now (which I need to document fully!) so I'd be very wary of making significant changes to it at this stage unless we were really confident that it was secure and made our lives much easier. TBH I'd have though it would be better to look at converting the HUD to a cross browser extension. This is something we've talked about, but we decided it would be too much of a distraction. Our focus right now is getting the HUD released. Otherwise we could just keep on reworking it in-perpetuity ;)
You're completely right with respect to security. The current architecture is definitely impacting a couple of issues. It's not to say that we can't work around them (except in some issue where the bug comes from the browser, like issue #204) - it just makes it a bit harder. On the other hand, we can make things a bit easier with the div method I illustrated, but to be completely secure we'd need to ensure that only one-way communication is allowed (from ZAP -> HUD), and not vice-versa. That of course means that we are forgoing the possibility of features like starting an active scan directly from HUD and other features that require HUD->ZAP communication. If those features are definitely required, then we'd need to be extra careful (shared keys, limited API endpoints, etc, etc). The age old security vs usability debate :(
On Tue, 16 Oct 2018 at 14:10, Simon Bennetts [email protected] wrote:
@dvas0004 https://github.com/dvas0004 my biggest concern here is security. Right now we only ever call ZAP from our HUD domain, either from the UI pages or the service worker. We can send messages from the target domain to the HUD domain but thats using shared keys and the plan is to allow this to be switched off completely. If the target site can interact directly with ZAP then its game over - the target site will (probably) be able to compromise the users machine. I think we have a pretty good security model right now (which I need to document fully!) so I'd be very wary of making significant changes to it at this stage unless we were really confident that it was secure and made our lives much easier. TBH I'd have though it would be better to look at converting the HUD to a cross browser extension. This is something we've talked about, but we decided it would be too much of a distraction. Our focus right now is getting the HUD released. Otherwise we could just keep on reworking it in-perpetuity ;)
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/psiinon/zap-hud/issues/119#issuecomment-430196960, or mute the thread https://github.com/notifications/unsubscribe-auth/AFlDbfR_jRGi6cVpRfTZwH2pwyEf5ZgGks5ulb6IgaJpZM4URO9N .
We absolutely need to be able to invoke ZAP features from the HUD - its way too limited without that. I also dont want to limit the API endpoints we can use, the idea is that this could become the main interface for ZAP. So if the div method doesnt allow that then its a non starter from my point of view :)
Fair enough! Then i'd say the div method is only viable if we do allow the target website to communicate with ZAP (given that the div would be injected into the target site) with all the security concerns that brings. Maybe we should stick to the current architecture and research some changes to the get the issues described here resolved
On Tue, 16 Oct 2018 at 14:36, Simon Bennetts [email protected] wrote:
We absolutely need to be able to invoke ZAP features from the HUD - its way too limited without that. I also dont want to limit the API endpoints we can use, the idea is that this could become the main interface for ZAP. So if the div method doesnt allow that then its a non starter from my point of view :)
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/psiinon/zap-hud/issues/119#issuecomment-430203867, or mute the thread https://github.com/notifications/unsubscribe-auth/AFlDbTr9PGYaqdp3iBpyrir2jxYEVKdlks5ulcS9gaJpZM4URO9N .
:+1: re keeping to the current architecture but doing some more research into alternatives
Woah! just caught up on this thread. @dvas0004 I love the creative new approaches. We've obviously spent lots of time trying to figure out the best architecture for the HUD, weighing the pros and cons, but we're aware that the route we chose might not be the best. I'm always interested in hearing different ideas and approaches to this challenge.
To piggy back on some of what @psiinon has already said, the iframe route provides:
- domain isolation, which is great for security purposes. We have much stronger controls on how we interact with our target site
- code isolation, which allows us to keep the site we're trying to scan, clean from the code we inject. Ideally we'd like to keep as low a footprint as possible on the target site. (we don't want to inject code that we then scan as vulnerable :p) ((we're hoping to add some more clientside ZAP tools in the future that would look for things like this, so it wouldn't just be the proxy doing the scanning))
- consistency, by loading in an iframe we know the environment our UI will be running in, by injecting them directly into the target site we have to deal with the target's js globals, imports, libraries, css, etc...
I'll also say that I am not the most technically fluent on client side technologies so if there is a solution that addresses these that would be very cool. I also think there are large limitations with the service worker model, and we acknowledge that we're for sure hacking around that spec.
In the past we've looked at models without the service worker, one where actually take the target page and inject it into a ZAP hosted domain that wraps around it, and we've considered the browser extension model (may need to look back at our pros/cons here, we may have misjudged them)...
All that being said if you've got some ideas for how to rearchitect I'd say create a working doc (google docs or something) that we can all comment on the various aspects of it. Because there might be something really powerful that you're thinking of that we're overlooking. :) lmk if you have any other thouthts on it!
@dscrobonia thanks for summarizing up the requirements! Easy to see how the div idea would never have worked trying to meet them. In any case I'd rather stick to an architecture that's already been setup at least until we get something out the door :)
Back to this particular issue, i'm trying to find a way around "Dialogs only open in the latest tab". At least in chrome, this is happening because we're storing the latest client ID in indexedDB, which we filter against when sending a postMessage. So all the events then end up being "consumed" by only the the latest client opened. Here's what I'm thinking of as a solution - if you guys think this is a good way forward then i'll create a new issue dealing with this problem specifically and code a PR against it:
-
Modify the rightPanel and leftPanel javascript to insert a random UUID into sessionStorage. (I've tested that even though they are in the same domain, each tab keeps a different sessionStorage).
-
When we click on a tool that presents a dialog, in the background we're firing off a "buttonClicked" event. We'll modify the Vue code to make sure that it reads the UUID from sessionStorage and includes this UUID in the event
-
The event is sent to all clients (in my tests I simply added a new postMessage instruction to all clients in addition to what was already there to make sure I dont break anything - this worked fine though in reality I will need to code some more error checking)
-
The clients would then check if the event's UUID matches their own, and only display the dialog box if there's a match
I know it's a roundabout way but with the current architecture this looks like the best shot. I may run into some issues I didn't think off when actually coding this but so far my tests look promising. Anyways if you guys agree i'll try out the above in a separate PR :)
ahh!! very cool! Yeah you're talking about creating some sort of ID for each of the panels on a specific tab! That is a very similar idea to what I'd been working on. ;) Take a look at some notes I wrote up on how I think we can support multiple tabs. (Somehow this doc link went missing from this this issue note. It covers that "tab id" concept along with a lot of the little details that you'd likely have run into: (https://docs.google.com/document/d/1JXua4gAigMfB7nS6PaiCDaqNJYW0QG8UYcLXb2QlsCc/edit?usp=sharing
Its been my one big thing I need to get done lately, and I haven't made the progress I need to.
- My first obstacle being the difference in the way chrome and firefox support serviceworkers across tabs. ( see #199 ) There should be a single serviceworker process in the firefox browser even across multiple tabs, but there is actually a process/tab which is not how chrome operates. haha.
- The second obstacle being that we're a little wishy washy with our "data model". We haven't quite landed on how/where we want to store specific types of data.
The second obstacle is less of an issue and I'd likely just figure it out as I went along, but seeing as the first obstacle might be fixed soon in firefox I was going to wait until then, so I didn't have to create a new solution to hack around it.
Lemme know what you think of my notes and let me know if it fits into what you were thinking!!
This is awesome. Your thinking is in fact a superset of mine so I would definitely agree with this approach. I'll comment a bit further within the document itself - fantastic work.
On Wed, 17 Oct 2018 at 08:50, David Scrobonia [email protected] wrote:
ahh!! very cool! Yeah you're talking about creating some sort of ID for each of the panels on a specific tab! That is a very similar idea to what I'd been working on. ;) Take a look at some notes I wrote up on how I think we can support multiple tabs. (Somehow this doc link went missing from this this issue note. It covers that "tab id" concept along with a lot of the little details that you'd likely have run into: ( https://docs.google.com/document/d/1JXua4gAigMfB7nS6PaiCDaqNJYW0QG8UYcLXb2QlsCc/edit?usp=sharing
Its been my one big thing I need to get done lately, and I haven't made the progress I need to.
- My first obstacle being the difference in the way chrome and firefox support serviceworkers across tabs. ( see #199 https://github.com/psiinon/zap-hud/issues/199 ) There should be a single serviceworker process in the firefox browser even across multiple tabs, but there is actually a process/tab which is not how chrome operates. haha.
- The second obstacle being that we're a little wishy washy with our "data model" https://github.com/psiinon/zap-hud/wiki/Data-Model. We haven't quite landed on how/where we want to store specific types of data.
The second obstacle is less of an issue and I'd likely just figure it out as I went along, but seeing as the first obstacle might be fixed soon in firefox I was going to wait until then, so I didn't have to create a new solution to hack around it.
Lemme know what you think of my notes and let me know if it fits into what you were thinking!!
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/psiinon/zap-hud/issues/119#issuecomment-430498191, or mute the thread https://github.com/notifications/unsubscribe-auth/AFlDbZr4lccrdlPfNI-Oh0JOcYu8nvXxks5ulsUngaJpZM4URO9N .
I think this was kept around to verify that the serviceworker changes in firefox were working. I still haven't done that.