🚧 WIP Integrate parcel and refactor to more pure Vue components #459
Conversation
…riable substition
* Refactor panel.html/css/js to standard Vue Single File Component format * Use JS ES6 import statements for referencing other components/libs. This will help use bundlers and dependencies more effectively * Implement SASS/SCSS and scoped styles for standardizing front-end architecture * Move re-usable button templates to a components directory * WIP: Clean up old code once validating functionality
* Migrate management.html to a single entrypoint for build tool capability * Use ES6 import statements to reference dependencies to enable build tool dependency management and decoupling of components * Implement SASS/SCSS scoped style management for theming * WIP: Clean up commented code once verifying functionality
and sass dev dependency
…'s own entrypoint
… it's own entrypoint
* Remove localforage script include and use ES6 import * Clean up dead code and add addtional TODO reminders
|
This pull request introduces 5 alerts when merging bcde54e into 3393ec0 - view on LGTM.com new alerts:
Comment posted by LGTM.com |
…or transpilation at build time
- Install alertify as node module - Upgrade Vue version - Upgrade Vue utils - Remove webpack related build packages, handled by parcel now
| import VueI18n from "vue-i18n"; | ||
|
|
||
| const messages = { | ||
| en_GB: { |
There was a problem hiding this comment.
I have a lot of comments about this i18n approach.
- It will not scale if you suddenly have dozens of languages and unnecessarily bloats the Vue object.
- There is no-lazy loading possible with this approach.
- JS to store strings opens up direct "false friend" translation injection attacks, aka
eval()/constructor.constructor()etc. - including HTML requires using v-html, which is a very well known XSS gateway.
- if you MUST use v-html, use freeze to harden these strings, because at runtime they will never change
Kazupon's lib does a great job of protecting Vue from attack vectors (even better than Vue itself, I know - I helped him harden it), but since translations are generally not as closely audited as code blocks, it is trivial to smuggle in <img> and <object> tags that can do really bad things. With the intended audience this is not as big an issue as it would be if your toddler was using the interface on an iPad, but best practice should be followed here in all cases.
| @@ -0,0 +1,2 @@ | |||
| import Vue from 'vue'; | |||
| export const EventBus = new Vue(); | |||
There was a problem hiding this comment.
Because the events are registered in this external bus, they all need to be individually torn down:
e.g.
beforeDestroy () {
eventBus.$off('showSiteTreeModal')
}
You could create a global mixin here, but I think that the difficulty you would face is that you will have to contort yourself to know which component is being destroyed. Probably better to tear them down in the individual components, because that way later contributors can SEE that they are being unregistered.
| common_turn_on: "Turn On", | ||
| error_invalid_html_header: "Invalid HTML header", | ||
| error_with_message: | ||
| "Error: {0}.<br>See console log and zap.log for more details.", |
There was a problem hiding this comment.
This is an example of HTML in the object. Just don't.
There was a problem hiding this comment.
@nothingismagick: Is that a complaint about the <br>?
| }, | ||
| created() { | ||
| let self = this; | ||
|
|
There was a problem hiding this comment.
I haven't tried building the code from your PR yet, but I wonder a little why the clone of 'this'. Shouldn't the fat arrow function of the EventBus pass the scope natively? And even if this is merely an assignment instead of a deep copy, it does cost some cycles. Considering that this is all being run simultaneously in one browser window I would look for all opportunities to shave performance hits.
| }; | ||
| </script> | ||
|
|
||
| <style lang="scss" scoped> |
There was a problem hiding this comment.
Every block you aren't using costs time in the development IDE (especially scoped style blocks). I have to recommend removing EVERYTHING that is not needed. End users won't thank you for it, but devs certainly will.
| @@ -0,0 +1,58 @@ | |||
| <template id="dialog-modal-template"> | |||
There was a problem hiding this comment.
Tabs / spaces ??? Which is it?
| const ZAP_SHARED_SECRET = process.env.ZAP_SHARED_SECRET; | ||
|
|
||
| import Vue from "vue"; | ||
| import localforage from "localforage"; |
There was a problem hiding this comment.
Leaving this here, because I see a number of problems with the implementation of localforage - I think that for the reasons that follow you would do well to create an export similar to the one for the EventBus. That way you could configure it, and potentially extend the interface (if you are so inclined).
For one, it seems to me that you are really expecting that localstorage is a safe place to store things. It isn't. The localForage serializer does not protect, just convert to and from base64. Should a developer's localhost (for example) get poisoned with appropriately base64'd functions, you would have no way of stopping them from reeking havoc. I think the only way to harden this interface is to actually pass it through e.g. DOMPurify.
Furthermore, there are a number of places in the code where you aren't catching errors. What happens if I don't have any more storage space available? localForage will pass that as an error, but how will this interface react?
Current Status/Requested Java HUD API changesWith 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:
The list of transpiled files so far looks like this and are built to the
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 Proposed SolutionWhat 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
Old: These HTML files are then automatically updated at bundle time to reference a fingerprinted version of the
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. |
|
Thanks @jaywon :) ZAP has a plugin architecture. |
|
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 when the HUD communicates with ZAP it uses URLs that have a random component. |
|
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:
And converts it to something like this at build time:
The prefix is customizable at
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. |
|
Other example is the CSS. Parcel allows us to just include
And converts it to this at build time with no reference at all in the development files:
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 so the
Would that work? |
|
Nice! We could easily do something like prefix the hud files like If we could prefix all the files with a URL qualifier like the We would actually still want this for |
|
'images' can be a sub folder of the hud top level dir. |
|
I think that could work. If I change the build option
I get build paths like this, which the
|
|
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: |
|
Have you looked at quasar-framework, by the way? All this stuff is figured
out already.
… |
|
@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 that would be great, we really need this :) |
|
Re #459 (comment) 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. |
|
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? |
|
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 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. |
| </script> | ||
|
|
||
| <style lang="scss" scoped> | ||
| </style> |
There was a problem hiding this comment.
Can you add a new-line at eof?
There was a problem hiding this comment.
@jsoref Can you clarify the need or value add of adding a new-line? I'm just curious if that's compliance w/ a tool or some other purpose.
There was a problem hiding this comment.
git diff (and every other diff) get really pissy about this.
Basically all tools that work on whole lines (newline terminated) make optimizations based on this. So, whenever it's violated, they have to do a lot of work to undo this assumption.
And standard text editors (e.g. vi) will by default insert the newline. This means that if I (or someone else) use such an editor to make a change to a file which is missing the newline, the editor will add it, and git (or hg or ...) will blame us for changing that last line -- even if our change is in fact nowhere near. Once our change is merged, anyone who uses a blame tool will see us as having made the change to that last line, and if further lines are added below, it'll look like we changed that specific line.
So, as a general rule, the best policy is to always include the newline.
There was a problem hiding this comment.
Gotcha, thanks for the clarification. Seems like something a linter should be responsible for enforcing ultimately.
| common_turn_on: "Turn On", | ||
| error_invalid_html_header: "Invalid HTML header", | ||
| error_with_message: | ||
| "Error: {0}.<br>See console log and zap.log for more details.", |
There was a problem hiding this comment.
@nothingismagick: Is that a complaint about the <br>?
|
@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! |
|
Hi @jaywon any plans to continue work on HUD? I think this PR holds huge potential if translated into smaller ones |
|
@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. |
Very preliminary non-working POC of refactor to use
parcelbundler and refactor needed for integrating more front-end tooling for HUD. Still a lot of work to do but major improvements include:npmTo install parcel:
Create
.env:To build bundle:
Sample build files and real bundle sizes for Display, Panel, and Management and Drawer entrypoints and bundled components/css:
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!