Skip to content
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.

Sign up for GitHub

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

Jump to bottom

webuipoc: add react based UI #5443

Merged
merged 25 commits into from
May 23, 2024
Merged

webuipoc: add react based UI #5443

merged 25 commits into from
May 23, 2024

Conversation

Moeez905
Copy link
Contributor

@Moeez905 Moeez905 commented May 11, 2024 •
edited

No description provided.

njmulsqb and others added 5 commits May 10, 2024 18:41
@njmulsqb
add react boilerplate
380c0a2 
Signed-off-by: Najam Ul Saqib <najamulsaqib@tutamail.com>
@njmulsqb
add npm build script
3de312c 
Signed-off-by: Najam Ul Saqib <najamulsaqib@tutamail.com>
@njmulsqb
fix path of build
c7701fc
@njmulsqb
update if condition
7570e34 
Signed-off-by: Najam Ul Saqib <najamulsaqib@tutamail.com>
@Moeez905
tailwind CSS integrate
7b3a9d1
@github-actions GitHub Actions
Copy link

github-actions bot commented May 11, 2024 •
edited

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@Moeez905
Copy link
Contributor Author

Moeez905 commented May 11, 2024 •
edited

Tailwind-CSS integrated in this React App

@Moeez905 Moeez905 marked this pull request as ready for review May 11, 2024 14:07
zapbot added a commit to zaproxy/cla that referenced this pull request May 11, 2024
@zapbot
@Moeez905 has signed the CLA in zaproxy/zap-extensions#5443
84c6c30
@thc202
Copy link
Member

thc202 commented May 12, 2024

Please create a pull request from a personal repo, otherwise we aren't able to push.

@njmulsqb
Copy link
Contributor

Please create a pull request from a personal repo, otherwise we aren't able to push.

Earlier I have got PRs merged from Tecvity's fork; is this a recent change?

@thc202
Copy link
Member

thc202 commented May 12, 2024

It's not about merging the PR but pushing to the PR's branch.

@njmulsqb
Copy link
Contributor

I see, are you guys also willing to contribute to Web GUI?

@thc202
Copy link
Member

thc202 commented May 13, 2024

Yes not only contribute but maintain too.

@thc202 thc202 changed the title Web UI webuipoc: add react based UI May 13, 2024
@Moeez905
Copy link
Contributor Author

Please create a pull request from a personal repo, otherwise we aren't able to push.

I have made a new fork on personal repository, now should I make another PR from new fork after closing this PR?

@thc202
Copy link
Member

thc202 commented May 13, 2024

Since this is near ready I'd keep as is and for following PRs use the new fork.

@thc202
Copy link
Member

thc202 commented May 14, 2024

Should be updated to pick/use #5446.

@njmulsqb
Copy link
Contributor

Should be updated to pick/use #5446.

Done

@psiinon
Copy link
Member

psiinon commented May 14, 2024

Does it still need to disable the CSP, if so, are the errors as before?
Ideally I'd like to see at least one working ZAP API call, even if it is just to get the top level domains from the sites tree.

@njmulsqb
run spotlessApply
5e54ec2 
Signed-off-by: Najam Ul Saqib <najamulsaqib@tutamail.com>
njmulsqb and others added 2 commits May 14, 2024 03:43
@njmulsqb @ricekot
Update addOns/webuipoc/src/main/pocs/web-ui/.gitignore
8cb3e8b 
Co-authored-by: Akshath Kothari <akshath.kothari@gmail.com>
Signed-off-by: Najam Ul Saqib <najamulsaqib@tutamail.com>
@njmulsqb
remove dist
45a7e1b 
Signed-off-by: Najam Ul Saqib <najamulsaqib@tutamail.com>
psiinon
psiinon reviewed May 15, 2024
View reviewed changes
addOns/webuipoc/src/main/pocs/web-ui/src/App.js Outdated
const [childNode,SetChildNode] = useState(null);
const WebClickZAP = async () => {
try {
const response = await axios.get('http://localhost:1337/JSON/core/view/childNodes/');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldnt need to specify http://localhost:1337/ - the API should be on the same host:port
If its not then it will fail anyway..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While developing locally, the React development environment runs on http://localhost:8000 so we need to specify the URL. How can it be ruled out during development?
Yes, I agree once build is deployed, specifying the domain isn't required as it will be on same host:port but thats not the case while developing (even we've to disable CORS temporarily for cross communication)

@Moeez905
Copy link
Contributor Author

Moeez905 commented May 15, 2024 •
edited

We've prepared a React app. It calls the API endpoint JSON/core/view/childNodes and prints data using an onClick function (just for POC)
Its ready to be merged.
image

psiinon
psiinon requested changes May 15, 2024
View reviewed changes
Copy link
Member

@psiinon psiinon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hardcoded URLs must not be used for ZAP API calls, and the CSP must not be disabled.
You can do what you like locally, and run time settings are fine, but the default out-of-the-box experience must be secure.

@njmulsqb
Copy link
Contributor

CSP is not disabled anymore. As for hard-coded URLs so we can use a separate config file that defines the URLs (in-fact there are more code improvements that can be done but thats in my plan in upcoming PRs since this is not the final one)

The concern is, how can do development in the local environment with cross-origin i.e. from localhost:8000 (React's default server) if you just want to specify the endpoint and not host and port?

@psiinon
Copy link
Member

psiinon commented May 15, 2024

I dont mind how development is done, as long as when the code is checked in it uses the correct relative paths.

@njmulsqb
rename project
d45b048 
Signed-off-by: Najam Ul Saqib <najamulsaqib@tutamail.com>
@njmulsqb
fix build URL
d32b37a 
Signed-off-by: Najam Ul Saqib <najamulsaqib@tutamail.com>
@njmulsqb
utilize NODE_ENV to determine the environment
f276aa0 
Signed-off-by: Najam Ul Saqib <najamulsaqib@tutamail.com>
@njmulsqb
Copy link
Contributor

I dont mind how development is done, as long as when the code is checked in it uses the correct relative paths.

f276aa0 should handle this

@psiinon
Copy link
Member

psiinon commented May 16, 2024

Why are the React icons being added? They dont appear to be being used, and we wont need them in the future.
Not a big problem, just seem unnecessary..

@njmulsqb
Copy link
Contributor

@Moeez905 please remove unused logos

psiinon
psiinon reviewed May 16, 2024
View reviewed changes
addOns/webuipoc/src/main/pocs/reactWebUI/src/App.js
@@ -0,0 +1,42 @@

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All JS files (and CSS?) should include the std ZAP header.
e.g. as per https://github.com/zaproxy/zap-extensions/blob/main/addOns/webuipoc/src/main/java/org/zaproxy/addon/webuipoc/ExtensionWebUiPoc.java

psiinon
psiinon reviewed May 16, 2024
View reviewed changes
addOns/webuipoc/src/main/pocs/reactWebUI/src/App.js
</div>
<div className="flex flex-row justify-between text-center">
<div className="w-1/3 p-4" onClick={WebClickZAP}>
<p className='font-mono'>Click to fetch</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now this is ok, but for future PRs all text should be i18n'ed

psiinon
psiinon reviewed May 16, 2024
View reviewed changes
addOns/webuipoc/src/main/pocs/reactWebUI/src/App.test.js
@@ -0,0 +1,8 @@
import { render, screen } from '@testing-library/react';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whats this file for?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For writing unit tests, actually we've created this boilerplate using https://create-react-app.dev/ so everything comes packaged.

We can/should write unit tests in this file in future PRs

@Moeez905
icons remove
dd9648c 
Signed-off-by: Moeez905 <x311099@gmail.com>
@Moeez905
Copy link
Contributor Author

@Moeez905 please remove unused logos

Removed.

Moeez905 and others added 5 commits May 20, 2024 22:36
@Moeez905
feat: Update app metadata in manifest.json
d24951e 
Signed-off-by: Moeez905 <x311099@gmail.com>
@Moeez905
refactor: Remove icons metadata
f1d2f99 
Signed-off-by: Moeez905 <x311099@gmail.com>
@njmulsqb
update README
a2471be 
Signed-off-by: Najam Ul Saqib <najamulsaqib@tutamail.com>
@njmulsqb
remove unused metrics file
567f1ff
@njmulsqb
remove webvitals usage
2e4a9b6 
Signed-off-by: Najam Ul Saqib <najamulsaqib@tutamail.com>
@njmulsqb
Copy link
Contributor

PR is ready to be reviewed.

@njmulsqb
remove web-vitals
020b683 
Signed-off-by: Najam Ul Saqib <najamulsaqib@tutamail.com>
ricekot
ricekot approved these changes May 23, 2024
View reviewed changes
Copy link
Member

@ricekot ricekot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@thc202 thc202 merged commit fd4b62b into zaproxy:main May 23, 2024
10 checks passed
@thc202
Copy link
Member

thc202 commented May 23, 2024

Thank you both!

@github-actions github-actions bot locked and limited conversation to collaborators May 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@thc202 thc202 thc202 left review comments

@njmulsqb njmulsqb njmulsqb left review comments

@psiinon psiinon psiinon approved these changes

@ricekot ricekot ricekot approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
5 participants
@Moeez905 @thc202 @njmulsqb @psiinon @ricekot