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.

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

First push of new independent Python API for ZAP #1

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

cr0hn
Copy link

@cr0hn cr0hn commented Apr 20, 2016

No description provided.

target = 'http://127.0.0.1'

# By default ZAP API client will connect to port 8080
zap = ZAPv2(proxies={'http': '127.0.0.1:9090', 'https': '127.0.0.1:9090'})
Copy link
Member

Choose a reason for hiding this comment

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

Should specify the proxy in this statement? If so, the comment and following example needs to be updated.

Copy link
Author

Choose a reason for hiding this comment

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

It's a mistake. I'll fix it ni next PR with new example

fix: removed redundant license file
required = f.read().splitlines()

setup(
name="python-owasp-zap-v2.4",
Copy link
Member

Choose a reason for hiding this comment

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

Hum, should this be renamed to python-owasp-zap-v2.5 ? As 2.5.0 is coming out soon?

Copy link
Author

Choose a reason for hiding this comment

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

Really, IMHO, the better choice si to use a standard name and unify the version. So there's a "version" param in setup(..). Currently in Pypi are available 3 different libraries:
shell

This is not a the correct way to send an update to Pypi. I'll change the name to:
setup(name="python-owasp-zap" ...
And only update the "version" param:
setup(... version=1.0.0 ...

For Python developers is more intuitive and easy if only one library is available, with different versions release.

When we update for a new version, not change the name name of library, only de version param, doing:
python setup.py sdist upload

Of, for the first release (if we change the name):
python setup.py sdist register upload

Copy link
Author

Choose a reason for hiding this comment

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

If you agree, I'll send a PR with the change

Copy link
Member

Choose a reason for hiding this comment

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

The Python library is using those names to prevent "incompatibility issues" between releases of ZAP. For example, newer versions include functionalities that will not work with older versions of ZAP, so the v2.4 (or v2.5) indicates the version of ZAP that the client implementation is targeting.

Copy link
Author

Choose a reason for hiding this comment

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

To do that, I think a better approach with having only one library, but with a different API. This is:

For version 2.4 of ZAP
from zap import ZAP_24
c = ZAP_24()

For version 2.5 of ZAP
from zap import ZAP_25
c = ZAP_25()

And so on. This way, all ZAP versions are unified in only one library.

Copy link
Member

Choose a reason for hiding this comment

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

The API is continually evolving, and will carry on doing so for the foreseeable future.
And we know users arent able to update to new versions of ZAP immediately, especially if any refactoring is involved.
I think we do need a plan for this, and I also like the idea of having just one library.
So a proposal would be great :)

Copy link
Author

Choose a reason for hiding this comment

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

The idea could be:

New organization of code, refactoring the Python entry point classes (ZAP_23, ZAP_25..) but, the entry points generated automatically with Java. Doing this, maybe we can unify the two approaches.

Copy link
Member

Choose a reason for hiding this comment

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

Works for me :)

Copy link
Author

Choose a reason for hiding this comment

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

Oka, I'll send a PR proposal

Copy link
Author

Choose a reason for hiding this comment

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

I just sent the proposal :)

@psiinon
Copy link
Member

psiinon commented Apr 22, 2016

Done with my comments - cant comment on the python code but looks better than the stuff I hacked together ;)

@cr0hn
Copy link
Author

cr0hn commented Jun 2, 2016

Hi @psiinon! What's about pull requests?

@psiinon
Copy link
Member

psiinon commented Jun 2, 2016

Ah, I was assuming we would need to sort out the proxying problem before the new code could be merged. Am I wrong?
I've submitted #3 for updating the code in the 'old' way, but v happy for that to be ditched once the new code is working fine...

@cr0hn
Copy link
Author

cr0hn commented Jun 7, 2016

What's the proxying problem? I don't know if I understand what you mean :)

@thc202 thc202 mentioned this pull request Jun 9, 2016
@Woolworths
Copy link
Contributor

Woolworths commented Jun 10, 2016

Just going to write my thoughts about this PR. First things first, I don't even know if this works with Python3. I see that you have imported the API accessor (from .acsrf import acsrf) but you fail to include them in the project. At the moment, the API accessors (spider, ascrf, ascan ...) do not support Python3 (as they use {}.iteritems() which is removed in Python3).
Next you do this (in _request_other):
get=None if get=None: get={}
Why?
You also do this: "%s?%s" % (url, urlencode(get)) which makes no sense as you are using requests (where you can just do params=get).
Furthermore you do this: return json.loads when again, requests has it's own builtin function (.json()) which you can't use because you prematurely append .text in your request.
In status_code() you open up a new requests.get instead of self.urlopen, meaning that there are now two requests you need to take care of. This is evident as you use proxies=self.__proxies twice.
You also have the same class twice (base.py and zap_24/__init__.py). This just over complicates the project, for literally no reason.
Also I think that we should be moving away from using project numbers in class names. Not only do they provide no advantage, but make maintaining harder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants