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

Fix typing in more places and configure mypy to follow imports #3932

Merged
merged 14 commits into from
Dec 10, 2024

Conversation

dekkers
Copy link
Contributor

@dekkers dekkers commented Dec 3, 2024

Changes

This contains a lot of fixes for missing or in some cases wrong typing. There are also a few small bugfixes in cases where there was a missing check for None.

Mypy is configured to follow imports now which means it will be able to catch a lot more issues. Only pydantic imports are not followed because that would generate too many errors.


Checklist for code reviewers:

Copy-paste the checklist from the docs/source/templates folder into your comment.


Checklist for QA:

Copy-paste the checklist from the docs/source/templates folder into your comment.

@dekkers dekkers requested a review from a team as a code owner December 3, 2024 22:21
@dekkers dekkers self-assigned this Dec 3, 2024
@underdarknl
Copy link
Contributor

This Pr is going to cause a lot of merge conflicts with Donny's new boefjes / container work I think.

Copy link
Contributor

@ammar92 ammar92 left a comment

Choose a reason for hiding this comment

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

Looks good to me

@dekkers
Copy link
Contributor Author

dekkers commented Dec 4, 2024

This Pr is going to cause a lot of merge conflicts with Donny's new boefjes / container work I think.

I assume you mean #3859 with that, but I just tried merging this branch into that branch and the only conflicts I got were the ones that already are there with current main.

@stephanie0x00
Copy link
Contributor

Checklist for QA:

  • I have checked out this branch, and successfully ran a fresh make reset.
  • I confirmed that there are no unintended functional regressions in this branch:
    • I have managed to pass the onboarding flow
    • Objects and Findings are created properly
    • Tasks are created and completed properly
  • I confirmed that the PR's advertised feature or hotfix works as intended.
  • I checked the logs for errors and/or warnings and made issues where necessary

What works:

  • CSV upload with multiple hosts
  • Raw file upload (burp file)
  • Onboarding + onboarding report
  • Enabled all boefjes that do not require an API key
  • Yielded objects under Tasks -> Normalizers are present and work.
  • Opening subreports for aggregate and normal reports
  • Opening main reports (normal and aggregate)
  • Scheduling of reports
  • Creating of new member

What doesn't work:

See below, two boefjes throw an error.

Bug or feature?:

CWE finding types

Traceback (most recent call last):
  File "/app/boefjes/boefjes/local.py", line 58, in run
    return boefje_resource.module.run(boefje_meta)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/boefjes/boefjes/plugins/kat_cwe_finding_types/main.py", line 24, in run
    name = weakness_elem.get("Name")
           ^^^^^^^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'get'

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/app/boefjes/boefjes/job_handler.py", line 144, in handle
    boefje_results = self.job_runner.run(boefje_meta, boefje_meta.environment)
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/boefjes/boefjes/local.py", line 60, in run
    raise JobRuntimeError("Boefje failed") from e
boefjes.runtime_interfaces.JobRuntimeError: Boefje failed

DNS Software version

Traceback (most recent call last):
  File "/app/boefjes/boefjes/local.py", line 58, in run
    return boefje_resource.module.run(boefje_meta)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/boefjes/boefjes/plugins/kat_dns_version/main.py", line 34, in run
    response = method(query, where=ip, timeout=timeout, port=port)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/dns/query.py", line 1028, in tcp
    (r, received_time) = receive_tcp(
                         ^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/dns/query.py", line 947, in receive_tcp
    ldata = _net_read(sock, 2, expiration)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/dns/query.py", line 858, in _net_read
    raise EOFError
EOFError

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/app/boefjes/boefjes/job_handler.py", line 144, in handle
    boefje_results = self.job_runner.run(boefje_meta, boefje_meta.environment)
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/boefjes/boefjes/local.py", line 60, in run
    raise JobRuntimeError("Boefje failed") from e
boefjes.runtime_interfaces.JobRuntimeError: Boefje failed

@stephanie0x00 stephanie0x00 added the 😸 Review/QA feedback Review/QA feedback provided label Dec 9, 2024
@ammar92
Copy link
Contributor

ammar92 commented Dec 10, 2024

See below, two boefjes throw an error.

Both errors seem not be related to this PR.

CWE finding types

Seems to be an error related to a CWE ID not being found in our local database. Perhaps the database is out of date and is now fixed by #3943?

DNS Software version

This error seems to be a runtime error that's not caught (either a network error while reading the socket, or abrupt closure of the connection by the remote service)

@underdarknl
Copy link
Contributor

See below, two boefjes throw an error.

Both errors seem not be related to this PR.

CWE finding types

Seems to be an error related to a CWE ID not being found in our local database. Perhaps the database is out of date and is now fixed by #3943?

DNS Software version

This error seems to be a runtime error that's not caught (either a network error while reading the socket, or abrupt closure of the connection by the remote service)

We should probably also catch the exception of the get() on the none or by doing a proper lookup of the expected key. Not being able to hydrate this CWE should be visible by telling the user we don't have information for the given CWE.

@stephanie0x00
Copy link
Contributor

See below, two boefjes throw an error.

Both errors seem not be related to this PR.

CWE finding types

Seems to be an error related to a CWE ID not being found in our local database. Perhaps the database is out of date and is now fixed by #3943?

Sadly not, I just ran into this error in #3943.

@ammar92
Copy link
Contributor

ammar92 commented Dec 10, 2024

Sadly not, I just ran into this error in #3943.

A raw file could help debugging this

@stephanie0x00
Copy link
Contributor

As discussed with @ammar92 the CWE bug is resolved in #3943. As for the DNS Software version I'll keep an eye out and see if I can trigger this again. This ticket can be merged.

@stephanie0x00 stephanie0x00 removed the 😸 Review/QA feedback Review/QA feedback provided label Dec 10, 2024
Copy link

sonarcloud bot commented Dec 10, 2024

@underdarknl underdarknl merged commit ec9d80e into main Dec 10, 2024
48 checks passed
@underdarknl underdarknl deleted the typing-fixes branch December 10, 2024 14:35
jpbruinsslot added a commit that referenced this pull request Dec 11, 2024
* main: (21 commits)
  Bump django from 5.0.9 to 5.0.10 in /rocky (#3940)
  Do not let enabling plugins affect the global plugin cache (#3944)
  Fix typing in more places and configure mypy to follow imports (#3932)
  Updates CWE archive to 4.16 (#3943)
  Report flaws (#3880)
  Translations update from Hosted Weblate (#3939)
  Fix report recipe API (#3942)
  Boefje runonce functionality in scheduler (#3906)
  fix: 🔨 do not store CDN findings (#3931)
  Dont check for Locations on local Ip's. (#3894)
  add unpkg.com to disallowed hostnames in CSP (#3927)
  Update website_discovery.py (#3921)
  Add export http boefje (#3901)
  Bump python-multipart from 0.0.9 to 0.0.18 in /bytes (#3925)
  Fix layout issues on scheduled reports page (#3930)
  Create scheduled report with zero objects selectable (#3907)
  Improve the KATalogus `/plugins` endpoint performance (#3892)
  Add bgp.jsonl and bgp-meta.json to .gitignore (#3928)
  Update pre-commit and all hooks (#3923)
  add support for detecting Lame dns delegations on ip ranges (#3899)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants