Skip to content

Fix bandit warnings#344

Closed
oshoma wants to merge 10 commits intoAggregate-Intellect:mainfrom
oshoma:fix-bandit-warnings
Closed

Fix bandit warnings#344
oshoma wants to merge 10 commits intoAggregate-Intellect:mainfrom
oshoma:fix-bandit-warnings

Conversation

@oshoma
Copy link
Copy Markdown
Collaborator

@oshoma oshoma commented Apr 19, 2024

Your checklist for this pull request

Thank you for submitting a pull request! To speed up the review process, please follow this checklist:

  • My Pull Request is small and focused on one topic so it can be reviewed easily
  • My code follows the style guidelines of this project (make format)
  • Commit messages are detailed
  • I have performed a self-review of my code
  • I commented hard-to-understand parts of my code
  • I updated the documentation (docstrings, /docs)
  • My changes generate no new warnings (or explain any new warnings and why they're ok)
  • I have added tests that prove my fix is effective or that my feature works
  • All tests pass when I run pytest tests (offline mode)

Additional steps for code with networking dependencies:

Description

Add Bandit to provide security checks in make lint and address Bandit security warnings.

This change improves the security of our code.

Depends on #343 .

@oshoma
Copy link
Copy Markdown
Collaborator Author

oshoma commented Apr 23, 2024

let's hold off on this PR for now, I will rebase atop main after a few others are merged in

@oshoma oshoma force-pushed the fix-bandit-warnings branch from 15aa3a4 to 5265270 Compare April 30, 2024 02:54
@oshoma
Copy link
Copy Markdown
Collaborator Author

oshoma commented Apr 30, 2024

I rebased this branch atop main and ran into a test failure:

tests/integration_tests/test_orchestrator.py:75: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
sherpa_ai/orchestrator.py:54: in execute
    agent.run()
sherpa_ai/agents/base.py:92: in run
    action_output = self.act(action, inputs)
sherpa_ai/agents/base.py:141: in act
    return action.execute(**inputs)
sherpa_ai/actions/arxiv_search.py:38: in execute
    result = self.search_tool._run(query)
sherpa_ai/tools.py:58: in _run
    data = requests.get(url, timeout=HTTP_GET_TIMEOUT)

This is happening because I added a timeout in tools.py to make it safe to run in production, and this test is doing an actual HTTP GET which happened to run slowly just now and trigger the timeout.

The solution here is to mock the call to requests.get() in ArxivSearch._run at sherpa_ai/tools.py:58. We should not be using the network in our tests.

@oshoma oshoma requested a review from 20001LastOrder April 30, 2024 03:42
@oshoma
Copy link
Copy Markdown
Collaborator Author

oshoma commented Apr 30, 2024

I re-ran the test job and it succeeded. So this is an intermittent error, we should expect it to reoccur sometimes when github tests run slowly. This will keep happening until we fix the test to use a mock. I will create a github issue to address this.

@20001LastOrder please proceed on reviewing this PR. I have rebased it on main and it no longer depends on #343 .

@20001LastOrder
Copy link
Copy Markdown
Collaborator

This PR seems to have many duplicated changes as #343, @oshoma How should we proceed with merging them?

oshoma added 6 commits May 2, 2024 11:09
Run `poetry self update && poetry install` after picking up this commit.

Changes:
- Change all make steps to depend on poetry being installed
- Add mypy for static type checking, but leave disabled for now
- Use more sophisticated lint options
- Include Slack app in `make test`
- Include Slack tests in `make lint`
- Fail tests if coverage drops below 75%

mypy gives us static type checking. It triggers many warnings,
so I am disabling mypy for now and will create an issue to resolve
mypy warnings.
Change code to resolve these lint warnings:
- F821: Undefined names
- F841: Local variable name is assigned to but never used
- F811: Redefinition of unused name from line name
Change code to resolve these warnings:
- F811: Redefinition of unused name from line name
- W291: Trailing whitespace

`make lint` will now warn and fail if files contain trailing whitespace.
flake8 is currently configured to ignore these errors:
W503: Line break occurred before a binary operator
E501: Line too long (82 > 79 characters)
E402: Module level import not at top of file

This change makes the behavior consistent for both
`make lint` and `poetry run flake8`.

Also removing E402 as that warning was already resolved.
@oshoma oshoma force-pushed the fix-bandit-warnings branch from d9d9746 to a7719bf Compare May 2, 2024 15:18
@oshoma oshoma mentioned this pull request May 2, 2024
11 tasks
oshoma added 4 commits May 2, 2024 12:04
Create a Makefile just for Slackapp so we can run `make test` within
/sherpa/src/apps/slackapp using Slackapp's own Poetry environment.

Remove knowledge of Slackapp from Sherpa's makefile.

Change make to install Spacy model dependency.
Run `poetry self update && poetry install` to pick this up.
Fixes the following Bandit errors seen with `make lint`:

1. Issue: [B104:hardcoded_bind_all_interfaces] Possible binding to all interfaces.
   Severity: Medium   Confidence: Medium
   CWE: CWE-605 (https://cwe.mitre.org/data/definitions/605.html)
   More Info: https://bandit.readthedocs.io/en/1.7.8/plugins/b104_hardcoded_bind_all_interfaces.html
   Location: apps/slackapp/slackapp/bolt_app.py:346:13

Change: suppress the warning with #nosec B104, since our use of
host '0.0.0.0' is intentional. Also add a security note.

2. Issue: [B113:request_without_timeout] Requests call without timeout
   Location: apps/slackapp/slackapp/bolt_app.py:340:23
   Location: sherpa_ai/scrape/extract_github_readme.py:75:19
   Location: sherpa_ai/utils.py:83:15
   Location: sherpa_ai/scrape/extract_github_readme.py:52:19
   Location: sherpa_ai/scrape/file_scraper.py:69:19

Change:  add timeouts for each of these. Adjustment may be needed.

3. Issue: [B310:blacklist] Audit url open for permitted schemes.
Allowing use of file:/ or custom schemes is often unexpected.
   Severity: Medium   Confidence: High
   CWE: CWE-22 (https://cwe.mitre.org/data/definitions/22.html)
   More Info: https://bandit.readthedocs.io/en/1.7.8/blacklists/blacklist_calls.html#b310-urllib-urlopen
   Location: sherpa_ai/tools.py:59:15
   Location: sherpa_ai/utils.py:294:12

Change: add logic to ensure URLs are HTTP or HTTPS. Add tests.
@oshoma oshoma force-pushed the fix-bandit-warnings branch from a7719bf to e0a2c6e Compare May 2, 2024 16:11
@oshoma
Copy link
Copy Markdown
Collaborator Author

oshoma commented May 2, 2024

Closing this. I have created a new PR oshoma#1 for this work and will submit it to the Sherpa repo after #356 is merged.

@oshoma oshoma closed this May 2, 2024
@oshoma oshoma deleted the fix-bandit-warnings branch May 2, 2024 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants