django-DefectDojo icon indicating copy to clipboard operation
django-DefectDojo copied to clipboard

#6620 Create API importer for Bugcrowd

Open Gby56 opened this issue 2 years ago • 6 comments

Implements https://github.com/DefectDojo/django-DefectDojo/issues/6620

Needs testing too

Gby56 avatar Jul 28 '22 12:07 Gby56

I have added a regex parsing for endpoints because I kept getting errors I have mentioned in https://github.com/DefectDojo/django-DefectDojo/issues/6580 ! Submissions can have random stuff in their bug_url, not just urls which is annoying...

Gby56 avatar Jul 28 '22 17:07 Gby56

@Gby56 This is a great start here! There a couple things left to do before we can approve

Maffooch avatar Jul 30 '22 18:07 Maffooch

@Maffooch all good :) I copied tests from Edgescan

Gby56 avatar Aug 02 '22 10:08 Gby56

@Gby56

  1. when I said use urlencode method instead, I was refering to something like this:
import urllib.request
import urllib.parse
params = urllib.parse.urlencode({'spam': 1, 'eggs': 2, 'bacon': 0})
url = "http://www.musi-cal.com/cgi-bin/query?%s" % params
with urllib.request.urlopen(url) as f:
    print(f.read().decode('utf-8'))

Which is easier to read and less bug prone (instead of pushing the string encoded directly in the code).

  1. We should convert this method get_findings(self, program, target) to be a generator function. If I understand well the data returned by GitLab API, the data are paginated. Which means that the function could return multiple time data. Perfect use case for a generator function.

damiencarol avatar Aug 02 '22 14:08 damiencarol

Done ! The bugcrowd api uses an annoying query format for JSONAPI, with brackets everywhere, this definitely looks better now

Gby56 avatar Aug 02 '22 15:08 Gby56

Implemented a generator pattern for the api client function, no gain in performance since it stays sequential, removes the "accumulator" logic from the function that's all A better pattern would be to build out the work pool initially, looking at a given total number of elements if it's returned by the API, and then split the work across workers. Otherwise, when the total number of pages is unknown, it would be possible to split between 2 workers (or more) by just taking odd/even page numbers for example ?

from concurrent import futures
maxWorker = min(10,len(total_amount_of_pages)) ## how many thread you want to deal in parallel. Here 10 maximum, or the amount of pages requested.
urls = ['url'*n for n in total_amount_of_pages] ## here I create an iterable that the function will consume.
with futures.ThreadPoolExecutor(workers) as executor:
                res = executor.map(requests.get,urls) ## it returns a generator
## it is consuming the function in the first argument and the iterable in the 2nd arguments, you can send more than 1 argument by adding new ones (as iterable). 
myresult = list(res)

Gby56 avatar Aug 03 '22 11:08 Gby56

Code is functional, could be improved a bit with URI parsing and handling more use cases (bug urls in Bugcrowd are not always URL/URIs, sometimes just hosts, sometimes just paths without the host which is almost unusable) I sent an email to Bugcrowd support about their poor input validation on that field, as it's not very useful if it has noise.

Gby56 avatar Aug 16 '22 16:08 Gby56

@damiencarol @Maffooch @kiblik if I can get a last review, it should be all good 🤔

Gby56 avatar Aug 18 '22 09:08 Gby56

Not sure why one test failed, is that normal ?

Gby56 avatar Aug 19 '22 09:08 Gby56

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions[bot] avatar Aug 19 '22 14:08 github-actions[bot]

Conflicts have been resolved. A maintainer will review the pull request shortly.

github-actions[bot] avatar Aug 19 '22 14:08 github-actions[bot]

@Gby56, sorry for the delay. I wrote my comments. I'm a little bit confused that some of them haven't been identified by Flake8

kiblik avatar Aug 23 '22 11:08 kiblik

@kiblik We'd like to see this one merged for the next release if possible - are there outstanding issues?

coheigea avatar Aug 29 '22 06:08 coheigea

Ouuuu, sorry @coheigea & @Gby56. 6 days ago, I wrote my comments but I forgot to click "Submit Review" so they haven't been visible to everybody and they stayed in "Pending" status.

kiblik avatar Aug 29 '22 16:08 kiblik

@kiblik Hello ! I have implemented all the suggestions, and improved the endpoint issue. I check before saving them that they are not broken, and completely skip if they are. The reason behind that is because of Bugcrowd's API/web ui, lots of researchers submit weird stuff in that field and I'd prefer to have an actual clean endpoint, than 20 broken ones.

Gby56 avatar Sep 13 '22 12:09 Gby56

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions[bot] avatar Sep 13 '22 15:09 github-actions[bot]

Conflicts have been resolved. A maintainer will review the pull request shortly.

github-actions[bot] avatar Sep 13 '22 15:09 github-actions[bot]

@kiblik yeah I did the upstream fetch on my fork and it pulled the master, but now that I merged dev into my branch it's not working either, anyways I copied over the exact files from dev and there are no conflicts here, it's weird how the diff is showing

Gby56 avatar Sep 13 '22 15:09 Gby56

I think it’s fine though ? It’s the same as the dev branch anyways

On Tue 13 Sep 2022 at 21:55, kiblik @.***> wrote:

@.**** commented on this pull request.

In components/package.json https://github.com/DefectDojo/django-DefectDojo/pull/6621#discussion_r970029742 :

@@ -1,6 +1,6 @@ { "name": "defectdojo",

  • "version": "2.14.0-dev",
  • "version": "2.15.0-dev",

Unfortunately, I'm not able to help you here. I have no idea. But I would try git rebase dev

— Reply to this email directly, view it on GitHub https://github.com/DefectDojo/django-DefectDojo/pull/6621#discussion_r970029742, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABTFKKCFPAAQSSBSEMMXMITV6DL4VANCNFSM545DRKNA . You are receiving this because you were mentioned.Message ID: @.***>

Gby56 avatar Sep 13 '22 19:09 Gby56

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions[bot] avatar Sep 14 '22 09:09 github-actions[bot]

Conflicts have been resolved. A maintainer will review the pull request shortly.

github-actions[bot] avatar Sep 14 '22 10:09 github-actions[bot]

the init file probably didn't get linted with black, the string delimiters were just different

Gby56 avatar Sep 14 '22 10:09 Gby56

@kiblik tell me if everything is ok for you, I have added the feedback from the http responses in case of a failure as requested

Gby56 avatar Sep 14 '22 14:09 Gby56

Yes @Gby56, it looks good to me. Thank you for the changes and your effort. Unfortunately, I do not have "approval" or "merge" rights here, so you will need to wait or ask somebody else.

kiblik avatar Sep 14 '22 14:09 kiblik