zap-hud icon indicating copy to clipboard operation
zap-hud copied to clipboard

:construction: WIP Integrate parcel and refactor to more pure Vue components

Open jaywon opened this issue 5 years ago • 24 comments

Very preliminary non-working POC of refactor to use parcel bundler and refactor needed for integrating more front-end tooling for HUD. Still a lot of work to do but major improvements include:

  • Front-end environment variable build support (less API string interpolation on proxy)
  • More modular and maintainable component based architecture following Vue Single Page Component pattern
  • Easier dependency management with npm
  • Full support of ES6+ features w/ ES5 build support
  • Vue specific linter support
  • Theme-able and global styles with SCSS integration
  • Component level styles
  • Minified/obfuscated production builds for front-end assets
  • Cache invalidation on build
  • Multiple entrypoint builds
  • Maaayyybe hot module replacement and other front-end developer tool/integration compatibility

To install parcel:

npm install -g parcel-bundler

Create .env:

cp .env.example .env

To build bundle:

npm run build

Sample build files and real bundle sizes for Display, Panel, and Management and Drawer entrypoints and bundled components/css:

Screen Shot 2019-03-24 at 11 36 32 PM

I'll try to update this with more info, I'm sure I'm missing some info and still a lot of work to do but the general framework is there and feedback more than welcome!

jaywon avatar Mar 20 '19 07:03 jaywon

This pull request introduces 5 alerts when merging bcde54ea6bf9d8b0c016deb780e33f754862270e into 3393ec0f9cbb86d6f88086b563cdae6758ba555e - view on LGTM.com

new alerts:

  • 3 for Unused variable, import, function or class
  • 2 for Missing variable declaration

Comment posted by LGTM.com

psiinon avatar Mar 20 '19 07:03 psiinon

wow wow wow wow wow. This is such incredible work. Beautifully linted code, modular and reusable components, concise descriptive commits. This is absolutely incredible. Looks great so far. @jaywon please be sure to lmk if there is anyway I can help! You are killing it!

dscrobonia avatar Mar 26 '19 15:03 dscrobonia

Current Status/Requested Java HUD API changes

With the integration of Parcel, we now have Parcel acting as a transpliation/bundler to take development files with distinct syntax for variable interpolation at build time as well as path resolution. Upon build Parcel will do the following:

  • Minify HTML
  • Transpile Vue/ES6 into pure JavaScript files
  • Transpile SCSS templates into pure CSS files
  • Replace variables with {{{ ZAP_HUD_FILES }}} with their associated .env values as a replacement
    • This replaces the need for Java to interpolate these values at the time that assets are requested/served.
    • The following variables can now be set in development and compiled at build IF NEEDED on the client side at build time
      • ZAP_HUD_FILES=https://zap
      • ZAP_HUD_URL=https://targetdomain.com (ASSUMING STILL NEEDED IN JAVA SIDE)
      • ZAP_HUD_API=https://zap/api
      • ZAP_HUD_WS=https://zap/websocket
      • ZAP_HUD_TOOLS=http://zap/to/some/tool
      • ZAP_SHARED_SECRET=sometestsecret (ASSUMING STILL NEEDED IN JAVA SIDE)
      • DEV_MODE= (ASSUMING STILL NEEDED IN JAVA SIDE)
      • TUTORIAL_URL= (NOT SURE ABOUT THIS, DIDN'T RESEARCH
      • ZAP_LOCALE=en_GB

The list of transpiled files so far looks like this and are built to the dist directory under zapHomeFiles for gradle to copy over:

Screen Shot 2019-04-30 at 7 36 38 PM

Ultimately I think it would be preferred for the HUD Java proxy to just serve most of the static assets as fully qualified URLs on the https://zap domain rather than be requested via the ?name= query string parameter and right now that's the MAIN blocker in terms of moving forward on the front end to test and finalize the PR.

Proposed Solution

What I would like is for the following main entrypoint(s) and associated bundled assets to be served directly from the proxy as they are requested and removing the need for them to be requested with the ?name= query string parameter and have the proxy simply serve as a proxy.

Screen Shot 2019-04-30 at 8 14 00 PM

Old: https://zap/?name=display.html New: https://zap/dist/Panel/panel.html

These HTML files are then automatically updated at bundle time to reference a fingerprinted version of the .js and .css files so we need to serve those as well with fully qualified URLs w/o query string parameters.

Screen Shot 2019-04-30 at 9 44 24 PM Screen Shot 2019-04-30 at 9 44 34 PM

What I am not entirely clear on is the full extent of why the randomly generated string is needed in the URL other than some comments about detecting if the HUD is in use from a security point of view, but I don't understand the full scope of that.

It's possible that the Java proxy could detect requests for the direct static assets with a 302 redirect to a URL with the random string and then validate that way to serve the contents of the file. But i'm making suggestions without full clarity on the current solution.

Any feedback is appreciated and I'll do my best to answer and have dialogue in a timely manner. It took me a while to just have the time to document this clearly of what the blocker was. Please let me know if I can clarify or other considerations that I can propose solutions for.

cc: @psiinon @thc202 @dscrobonia

jaywon avatar May 01 '19 07:05 jaywon

Thanks @jaywon :) I'll have a good look through your proposal and give a more detailed response, but I'll give an immediate response now.

ZAP has a plugin architecture. The API endpoint is maintained by the core. The callback mechanism (with the random path element) was created well before we used it in the HUD, to support faking on domain content. The HUD is a ZAP add-on. We currently do not provide a mechanism for add-ons to register to handle arbitrary paths on the main host:port that ZAP is listening on. That's not to say that we cant do that, but it will require core changes, and we'll need to be very careful to make sure that the HUD (or any other add-on that uses them) does not introduce vulnerabilities into ZAP. Would you be able to cope with the existing 'random' path elements or will it break everything you're doing? I am wary of making significant core changes this close to the 2.8.0 release...

psiinon avatar May 01 '19 08:05 psiinon

Not sure exactly what you mean by "randomly generated strings in the url", but in my experience this is usually there for cache-busting in-case the source-map / source file changes. This is imperative when using web-worker threads / PWA because otherwise you would have to manually clear the cache which doesn't work everywhere. Maybe I don't understand enough about the HUD yet though and you are talking about something else.

nothingismagick avatar May 01 '19 08:05 nothingismagick

@nothingismagick when the HUD communicates with ZAP it uses URLs that have a random component. These are the 'callback' URLs that ZAP supports for faking on domain content. They only change when you restart ZAP so should have no impact on browser caching.

psiinon avatar May 01 '19 08:05 psiinon

The new transpiled Vue files will have their own cache busting uniquely generated id's as seen in the image above. I guess I don't understand the concept of faking on domain content...

That makes sense about the plugin architecture. I had seen that there was a specific HudFileProxy.java` class and was thinking that was something that could be modified specifically for HUD compatibility without interfering with ZAP core?

The challenge is with the bundler is that it takes something like this in development:

Screen Shot 2019-04-30 at 11 13 54 PM

And converts it to something like this at build time:

Screen Shot 2019-04-30 at 11 14 09 PM

The prefix is customizable at build via the --public-url attribute but is meant to be a domain/path prefix and doesn't seem to be accept a fully qualified domain and open attribute like https://zap/?name=.

Screen Shot 2019-04-30 at 11 15 22 PM

I could look further into what Parcel supports in terms of control of path interpolation at build time if we can't make changes to ZAP core. It's built to be extensible, so I'll look into that in the meantime.

It's just kind of an odd state is that all the front-end build tools make an assumption that files are served in a traditional web server sense.

Also thanks for the GREAT commentary @nothingismagick! I'm trying to get this unblocked as a first priority (with limited time), but very much looking forward to addressing your feedback and unblocking you to execute on all of these ideas.

jaywon avatar May 01 '19 09:05 jaywon

Other example is the CSS. Parcel allows us to just include <style> blocks in the .vue components with no reference in the HTML:

Screen Shot 2019-04-30 at 11 23 13 PM

And converts it to this at build time with no reference at all in the development files:

Screen Shot 2019-04-30 at 11 22 38 PM

These are all great improvements as it relates to theming, build time interpolation, support of ES6+ features, Vue best practices/developer experience, dependency management, and such but I'm sure we'll figure something out 😃

jaywon avatar May 01 '19 09:05 jaywon

@jaywon so the ?name= path element is under the HUDs control :) Its handled here: https://github.com/zaproxy/zap-hud/blob/develop/src/main/java/org/zaproxy/zap/extension/hud/HudFileProxy.java#L107 We can easily change that part, but we will need to know the difference between files and images as they are held in different directories: https://github.com/zaproxy/zap-hud/blob/develop/src/main/java/org/zaproxy/zap/extension/hud/HudFileProxy.java#L139 We could just have:

  • https://zap/<random part>/display.<blah>.js
  • https://zap/<random part>/tools/attack.<blah>.js
  • https://zap/<random part>/images/hud.png

Would that work?

psiinon avatar May 01 '19 09:05 psiinon

Nice! We could easily do something like prefix the hud files like https://zap/hud and https://zap/images. Or.. do they need to be in separate directories? The images path could be a child directory of the https://zap/hud.

If we could prefix all the files with a URL qualifier like the https://zap/hud and whitelist files in that path. All other files for any other ZAP plugins would still need to use the ?name= if needed?

We would actually still want this for inject.js actually because that's completely outside the Vue architecture. So that would still be https://zap/?name=inject.js unless decided otherwise, that's how it currently works. We only refactored Vue stuff but that's really just an iframe loader.

jaywon avatar May 01 '19 09:05 jaywon

'images' can be a sub folder of the hud top level dir. Are you ok with the top level random path element? If not we'll need core ZAP changes (and some more thinking;)

psiinon avatar May 01 '19 09:05 psiinon

I think that could work. If I change the build option --public-url=https://zap/hud/RANDOM_TOKEN like this:

Screen Shot 2019-05-01 at 12 19 23 AM

I get build paths like this, which the RANDOM_TOKEN could still be modified via string interpolation on first request when the HUD starts as it works now:

Screen Shot 2019-05-01 at 12 22 31 AM

jaywon avatar May 01 '19 10:05 jaywon

If that doesn't work out it looks like there's an option to customize the Parcel build routine to keep things the way they currently work but would take a bit of figuring out:

https://parceljs.org/api.html

jaywon avatar May 01 '19 10:05 jaywon

Have you looked at quasar-framework, by the way? All this stuff is figured out already.

nothingismagick avatar May 01 '19 10:05 nothingismagick

@psiinon I'd like to pick this back up as it seems to be blocking a lot of other conversations/progress 😞 Has there been any progress of updating the API to allow for serving bundled assets?

jaywon avatar Aug 01 '19 00:08 jaywon

@jaywon that would be great, we really need this :) TBH I'd forgotten that you were waiting on API changes. Am I right in thinking you needed the URLs to be changed to use path elements for the file names rather than parameters? I'll look at that asap.

psiinon avatar Aug 01 '19 07:08 psiinon

Re https://github.com/zaproxy/zap-hud/pull/459#issuecomment-488229956 we will definitely need to set some variables at run time. We will also need to be able to dynamically add content, eg to support optional add-ons that want to add HUD functionality. Let me know if you need more details of these requirements.

psiinon avatar Aug 01 '19 07:08 psiinon

Another thought - would it be possible to split this up into multiple PRs, eg closer to one of each of the issues you mention in the first comment? Its ok if you cant, but it would make it much easier to review (and therefore merge) and that would probably cause less disruption to other PRs.

psiinon avatar Aug 02 '19 07:08 psiinon

Let me rethink this. I've been watching PRs for front-end changes and know that there's commits that would need to get merged into this PR regardless. I think I started this PR w/ the intent of getting bundling and dependency management to work first, and it cascaded into the other refactoring, but now that the refactoring is clear maybe I can take a reverse approach to refactoring first w/ the end goal being the bundled assets and dependency management.

When #459 gets merged i can at least test that any of this is going to work and then map out a much clearer path than before. 🤔

jaywon avatar Aug 06 '19 20:08 jaywon

@jaywon let me know if I can help out at all. Probably a lot here that's a one person job, but if even just sitting down and working through some small tasks and dropping inspirational quotes while you crank through the refactor helps lmk. Would be happy to get online at the same time and try to burn through some code with ya.

dscrobonia avatar Aug 07 '19 02:08 dscrobonia

@jsoref Thank you for all the notes. It's unlikely that this PR will ever get merged (too large, outdated) as much as serve as a reference for a smaller, more up to date, series of commits. If I'm able to start that I'll take all of this feedback into consideration!

jaywon avatar Dec 09 '19 20:12 jaywon

Hi @jaywon any plans to continue work on HUD? I think this PR holds huge potential if translated into smaller ones

njmulsqb avatar Oct 14 '23 13:10 njmulsqb

@njmulsqb This is so old I'd have to refresh myself on it. I wouldn't mind but I believe the HUD project overall has stalled out?

jaywon avatar Oct 14 '23 15:10 jaywon

@njmulsqb This is so old I'd have to refresh myself on it. I wouldn't mind but I believe the HUD project overall has stalled out?

Nop. HUD is looking for contributors but ZAP team lacks JS expertise, I have been trying to understand the codebase and contribute but since you've done so much work it will be a great help if you jump in.

njmulsqb avatar Oct 14 '23 15:10 njmulsqb