Skip to content

Enable ruff security rules and related fixes#2775

Open
hugo093 wants to merge 3 commits intodevelopfrom
IA-4809-Enable-bandit-rules-in-ruff
Open

Enable ruff security rules and related fixes#2775
hugo093 wants to merge 3 commits intodevelopfrom
IA-4809-Enable-bandit-rules-in-ruff

Conversation

@hugo093
Copy link
Copy Markdown
Contributor

@hugo093 hugo093 commented Mar 2, 2026

What problem is this PR solving?

Security rules were not enabled in the ruff config

Related JIRA tickets

IA-4809

Changes

  • Enabled "S" rule for ruff in pyproject.toml
  • Did the right fixes when I could, otherwise left a todo

How to test

  • run the command ruff check .

Notes

  • We need to decide what to do in case of empty try - except : log warn ?
  • Perhaps the most urgent fix to decide is the one located in iaso/isao/models/instances.py with the pseudo check on the url

@hugo093 hugo093 changed the title Enable ruff bandit and first fixes Enable ruff security rules and related fixes Mar 2, 2026
@hugo093 hugo093 marked this pull request as ready for review March 2, 2026 11:10
@mestachs
Copy link
Copy Markdown
Contributor

mestachs commented Mar 3, 2026

global feeling

  • adding short timeout looks risky in some calls
    • ex writing data to dhis2 might be slow, reading the pyramid too
    • ex iaso task being a self client in the zip creation (especially for entities I would ask Philippe to really test data with A LOT of data, there are a lot of entities there)
    • I would specify at least per product
      • DHIS2, superset, iaso, s3, enketo, powerbi, ona/odk => constants
      • for dhis2 I would specify higher (different contant) timeout for "big write" and the pyramid synchronisation : apparently we use dhis2 python package (don't know if there's timeout specified)
      • to mitigate this I would allow to override it via env variables so in case of trouble we don't need a hot fix
      • iaso/dhis2/authentication.py can "low" timeout
      • this ona/odk might be slow too :
        response = requests.get(paginated_url, auth=(login, password), timeout=(3.05, 30))
    • note I think it's good idea (already had apps deadlocked/all threads starved due to unresponsive external server), I would just be a bit more cautious (moving from a really permissive environment to a 30 sec timeout is perhaps a bit too optimistic)
  • not sure everything is covered by a test (polio admin html thing)
  • I hope the xml parser is not too risky, definitively need to be tested by a real submission via the mobile and enketo

geometry_field,
function="ST_AsGeoJSON",
output_field=JSONField(),
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

are you sure it's equivalent ?

def read_parquet(f):
with duckdb.connect() as con:
result = con.execute(f"SELECT * FROM read_parquet('{f.name}')")
result = con.execute(f"SELECT * FROM read_parquet('{f.name}')") # noqa: S608
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would have fixed it instead of adding an exception to the rule

@hugo093 hugo093 requested a review from mestachs March 3, 2026 09:33
dsn = connection.get_connection_params()

tmpdir = "/tmp/duckdb_tmp"
# todo : could we use a TempFile here instead ?
Copy link
Copy Markdown
Contributor

@mestachs mestachs Mar 3, 2026

Choose a reason for hiding this comment

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

I don't know it's used to let duckdb overflow on disk instead of doing everything in memory, probably a random directory is better (it's normally covered by the test, so you can try to do the switch)

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