Skip to content

Fix security and typo bugs in physchem_filters.py#107

Merged
hmacdope merged 2 commits intoOpenADMET:mainfrom
SergeiNikolenko:pr/fix-filtering-security-and-typo
Mar 2, 2026
Merged

Fix security and typo bugs in physchem_filters.py#107
hmacdope merged 2 commits intoOpenADMET:mainfrom
SergeiNikolenko:pr/fix-filtering-security-and-typo

Conversation

@SergeiNikolenko
Copy link
Copy Markdown
Contributor

Description

Fix two bugs in openadmet/toolkit/filtering/physchem_filters.py:

  1. Unsafe dynamic code execution in DatamolFilter (line ~504): eval(f"dm.descriptors.{self.name}") allows arbitrary code execution through crafted descriptor names. Replaced with safe getattr(dm.descriptors, self.name) which is functionally identical but eliminates the injection risk.

  2. AttributeError in ProximityFilter.calculate (line ~215): self.set_mols(smiles=smiles) calls a non-existent method. The correct method is self.get_mols() (defined in BaseFilter). This causes AttributeError when calculate() is called with mols=None.

Status

  • Ready to go

Developers certificate of origin

eval(f"dm.descriptors.{self.name}") is a code injection risk since
self.name comes from user configuration. Using getattr(dm.descriptors,
self.name) achieves the same dynamic attribute lookup safely.

Signed-off-by: Nikolenko.Sergei <Nikolenko.Sergei@icloud.com>
The DistanceFilter.calculate() method called self.set_mols() which does
not exist on the class. The correct method is self.get_mols() inherited
from the base class, causing an AttributeError at runtime.

Signed-off-by: Nikolenko.Sergei <Nikolenko.Sergei@icloud.com>
@hmacdope
Copy link
Copy Markdown
Contributor

hmacdope commented Mar 2, 2026

Hi @SergeiNikolenko thanks for the contribution. Can I ask what brought you here and what you are hoping to achieve with the toolkit?

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 2, 2026

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@SergeiNikolenko
Copy link
Copy Markdown
Contributor Author

Hi @hmacdope! Thank you for getting back to me. I really appreciate the work you’re doing — it’s genuinely exciting and very relevant to what we’re building.
I lead a team of cheminformaticians at my company, and we’re currently exploring plans to integrate OpenADMET as the ADMET prediction engine within our molecular generator pipeline. The idea is to use it as part of early-stage drug discovery workflows in applied medicinal chemistry projects.
I’d be very glad to contribute where possible and help improve the toolkit and models reps along the way.

@SergeiNikolenko
Copy link
Copy Markdown
Contributor Author

At the moment, I’m going through the codebase to better understand the architecture and design decisions. I may submit some PRs if I find improvements or additions that seem useful.
If anything doesn’t align with your roadmap or expectations, please let me know — I’m happy to adjust.

@hmacdope
Copy link
Copy Markdown
Contributor

hmacdope commented Mar 2, 2026

No problem! Appreciate any and all bug fixes.

May I suggest that the core of our work on the modelling side is done over at openadmet-models here, this functionality works but has more rough edges: https://github.com/OpenADMET/openadmet-models. We also have a discord and I just made a channel for us to talk infra etc: https://discord.gg/eUjpFaUv6m

@hmacdope hmacdope merged commit aff584b into OpenADMET:main Mar 2, 2026
7 checks passed
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