-
-
Notifications
You must be signed in to change notification settings - Fork 151
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support multiple browser tabs #119
Comments
Ignore the MDN link - thats just for brower add-ons :/ |
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 :/ |
@dscrobonia any thoughts on this? |
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. |
FYI I'm looking into this and will aim to share a doc later today.. |
I think there are three main problems:
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. |
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 ;) |
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 ***@***.***> 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
<psiinon#119 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFlDbcQFVmSMe-bRIGzm6Ur-e4xdvaeMks5ukEAmgaJpZM4URO9N>
.
|
@dvas0004 my biggest concern here is security. |
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 ***@***.***> 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
<psiinon#119 (comment)>,
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. |
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 ***@***.***> 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
<psiinon#119 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFlDbTr9PGYaqdp3iBpyrir2jxYEVKdlks5ulcS9gaJpZM4URO9N>
.
|
👍 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:
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:
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.
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 ***@***.***> 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
<psiinon#119 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFlDbZr4lccrdlPfNI-Oh0JOcYu8nvXxks5ulsUngaJpZM4URO9N>
.
|
@dscrobonia this is fixed isnt it? |
I think this was kept around to verify that the serviceworker changes in firefox were working. I still haven't done that. |
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 psiinon#112 (comment)) and associate them with the tab too.
The text was updated successfully, but these errors were encountered: