Use query parameters wherever possible in Neo4jStore #330

LilDojd · 2024-11-16T22:53:29Z

This PR closes #259 by replacing f-string substitutions with parameterized queries in the Neo4jStore class. The changes should slightly improve query performance but the main goal of the PR is to make Cypher injections impossible

Changes Made

Added parametrized queries where they were missing
Added a test to showcase how previous queries could be abused for unintended behavior 10456b9
When building a ScopedKey, GufeKey is validated for common exploits, including whitespace checks, unicode encoding stuff and general adherence to GufeKey format 77d3cde dd65007 99034dc
_generate_claim_query is replaced with a const str CLAIM_QUERY. Parameters are now supplied on-site when performing a transaction
In cases where queries contained statuses from the TaskStatusEnum, f-strings are now consistently used for ease of work with LSP

Please let me know if I have missed anything and thanks!

This commit changes `_query()` function in Neo4jStore and adds test that shows how the exploit could be abused in previous versions

ianmkenney

@LilDojd Thanks so much for this PR. Please see my review comments---I have a couple of observations/suggestions.

alchemiscale/models.py

alchemiscale/storage/statestore.py

ianmkenney · 2024-11-18T18:23:07Z

alchemiscale/storage/statestore.py

@@ -2229,7 +2238,9 @@ def set_task_priority(
            RETURN scoped_key, t
            """
            res = tx.run(
-                q, scoped_keys=[str(t) for t in tasks], priority=priority
+                q,
+                scoped_keys=[str(t) for t in tasks if t is not None],


tasks will not have (🤞) None elements based on the type annotation. I'd say it's safe to keep this as is, but I also don't think it will hurt. Maybe @dotsdl has a differing opinion here.

@LilDojd did you notice a case where inputs to this method may include None?

Not really.

But since you have used cypher_list_from_scoped_keys(tasks), which filters out Nones internally, in action_tasks, and it, too, has List[ScopedKey] in the signature for tasks, I thought it was better to take a precaution

alchemiscale/alchemiscale/storage/statestore.py

Lines 1393 to 1399 in 4926573

def action_tasks(

self,

tasks: List[ScopedKey],

taskhub: ScopedKey,

) -> List[Union[ScopedKey, None]]:

"""Add Tasks to the TaskHub for a given AlchemicalNetwork.

Anyway, there would be no Nones on the server, since

asc.set_tasks_priority(tasks + [None], 1)

Would just throw
AttributeError: 'NoneType' object has no attribute 'dict' client-side

alchemiscale/alchemiscale/interface/client.py

Lines 1274 to 1277 in 4926573

async def _set_task_priority(

self, tasks: List[ScopedKey], priority: int

) -> List[Optional[ScopedKey]]:

data = dict(tasks=[t.dict() for t in tasks], priority=priority)

Ah cool, thank you for giving your rationale @LilDojd! I see no harm in leaving this as you have done it here, as we have in action_tasks. Up to you if you'd like to leave it in.

I have decided to remove None checks for cleanliness. Will save a few clock cycles :)

ianmkenney · 2024-11-18T18:23:58Z

alchemiscale/storage/statestore.py

@@ -2266,7 +2277,7 @@ def get_task_priority(self, tasks: List[ScopedKey]) -> List[Optional[int]]:
            WHERE t._scoped_key = scoped_key
            RETURN t.priority as priority
            """
-            res = tx.run(q, scoped_keys=[str(t) for t in tasks])
+            res = tx.run(q, scoped_keys=[str(t) for t in tasks if t is not None])


Same as above: annotation + potential @dotsdl opinion

As above, I don't mind keeping the if t is not None, but agree it isn't necessary here. Up to @LilDojd what he would like to do.

ianmkenney · 2024-11-18T18:46:54Z

alchemiscale/tests/integration/storage/test_statestore.py

+            assert len(n4js.query_transformations(scope=multiple_scopes[0])) == 0
+
+        mark = n4js._query(qualname="InjectionMark")
+        assert len(mark) == 0


In addition to the *query* methods from here down, we should also check directly with cypher via the n4js.execute_query method, even though it appears to be duplicated work. I think this issue is important enough to justify explicitly checking that the convenience query methods are consistent.

MATCH (n:InjectionMark) RETURN n

codecov · 2024-11-18T19:03:46Z

Codecov Report

Attention: Patch coverage is 95.69892% with 4 lines in your changes missing coverage. Please review.

Project coverage is 80.16%. Comparing base (4926573) to head (4848236).
Report is 176 commits behind head on main.

Files with missing lines	Patch %	Lines
alchemiscale/storage/statestore.py	95.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #330      +/-   ##
==========================================
+ Coverage   80.10%   80.16%   +0.06%     
==========================================
  Files          27       27              
  Lines        3478     3484       +6     
==========================================
+ Hits         2786     2793       +7     
+ Misses        692      691       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

LilDojd · 2024-11-18T19:40:31Z

Thank you for your comments, @ianmkenney
It's a bit late, so I will address them tomorrow when I get the time

Co-authored-by: Ian Kenney <ianmichaelkenney@gmail.com>

dotsdl

Fantastic work @LilDojd! Seriously thank you so much for this!

Just a few comments from me, but nothing blocking in my view once @ianmkenney's comments are addressed!

alchemiscale/storage/statestore.py

dotsdl · 2024-11-19T00:49:30Z

alchemiscale/storage/statestore.py

-                    RETURN task, ar
-                    """
-                    results.append(tx.run(q).to_eager_result())
+                tasks_list = [{"task": str(t), "weight": w} for t, w in tasks.items()]


dotsdl · 2024-11-19T00:52:28Z

alchemiscale/storage/statestore.py

+            OPTIONAL MATCH (th:TaskHub {_scoped_key: $taskhub})-[ar:ACTIONS]->(task:Task {_scoped_key: task_scoped_key})
+            RETURN task_scoped_key, ar.weight AS weight


Excellent, this should preserve the behavior that if there is no matching Task actioned on the TaskHub then weight should come back as None.

dotsdl · 2024-11-19T01:14:11Z

alchemiscale/storage/statestore.py

@@ -2229,7 +2238,9 @@ def set_task_priority(
            RETURN scoped_key, t
            """
            res = tx.run(
-                q, scoped_keys=[str(t) for t in tasks], priority=priority
+                q,
+                scoped_keys=[str(t) for t in tasks if t is not None],


Ah cool, thank you for giving your rationale @LilDojd! I see no harm in leaving this as you have done it here, as we have in action_tasks. Up to you if you'd like to leave it in.

dotsdl · 2024-11-19T01:16:29Z

alchemiscale/storage/statestore.py

@@ -2266,7 +2277,7 @@ def get_task_priority(self, tasks: List[ScopedKey]) -> List[Optional[int]]:
            WHERE t._scoped_key = scoped_key
            RETURN t.priority as priority
            """
-            res = tx.run(q, scoped_keys=[str(t) for t in tasks])
+            res = tx.run(q, scoped_keys=[str(t) for t in tasks if t is not None])


As above, I don't mind keeping the if t is not None, but agree it isn't necessary here. Up to @LilDojd what he would like to do.

alchemiscale/storage/statestore.py

LilDojd · 2024-11-19T10:38:20Z

Should be taken care of now, please do check. Thanks!

ianmkenney · 2024-11-19T18:44:20Z

@LilDojd Thanks for bringing this to our attention and providing this great solution to the issue!

LilDojd added 6 commits November 16, 2024 18:20

Validate GufeKey for most common exploits before creating ScopedKey

77d3cde

Add tests for GufeKey validation

dd65007

Add positive tests for GufeKey validation

99034dc

Use cypher params in _query

10456b9

This commit changes `_query()` function in Neo4jStore and adds test that shows how the exploit could be abused in previous versions

WIP on transitioning some queries to parametrised in Neo4jStore

71f1146

Fix tests

80332c0

ianmkenney requested changes Nov 18, 2024

View reviewed changes

ianmkenney requested a review from dotsdl November 18, 2024 18:53

dotsdl mentioned this pull request Nov 18, 2024

Use query parameters wherever possible in Neo4jStore instead of f-string substitutions #259

Closed

Update alchemiscale/storage/statestore.py

1da0d87

Co-authored-by: Ian Kenney <ianmichaelkenney@gmail.com>

dotsdl reviewed Nov 19, 2024

View reviewed changes

LilDojd added 4 commits November 19, 2024 14:10

Address s in review OpenFreeEnergy#330

fb9c9b8

Stay true to signature review OpenFreeEnergy#330

cd59ed9

Add additional check in query test review OpenFreeEnergy#330

4b97635

Black black black

4848236

ianmkenney merged commit 49182dc into OpenFreeEnergy:main Nov 19, 2024
7 checks passed

LilDojd deleted the feat/query-params branch November 20, 2024 06:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use query parameters wherever possible in Neo4jStore #330

Use query parameters wherever possible in Neo4jStore #330

LilDojd commented Nov 16, 2024 •

edited

Loading

ianmkenney left a comment

ianmkenney Nov 18, 2024

dotsdl Nov 18, 2024

LilDojd Nov 18, 2024

LilDojd Nov 18, 2024

dotsdl Nov 19, 2024

LilDojd Nov 19, 2024

ianmkenney Nov 18, 2024

dotsdl Nov 19, 2024

ianmkenney Nov 18, 2024

codecov bot commented Nov 18, 2024 •

edited

Loading

LilDojd commented Nov 18, 2024

dotsdl left a comment

dotsdl Nov 19, 2024

dotsdl Nov 19, 2024

dotsdl Nov 19, 2024

dotsdl Nov 19, 2024

LilDojd commented Nov 19, 2024

ianmkenney commented Nov 19, 2024

	def action_tasks(
	self,
	tasks: List[ScopedKey],
	taskhub: ScopedKey,
	) -> List[Union[ScopedKey, None]]:
	"""Add Tasks to the TaskHub for a given AlchemicalNetwork.

	async def _set_task_priority(
	self, tasks: List[ScopedKey], priority: int
	) -> List[Optional[ScopedKey]]:
	data = dict(tasks=[t.dict() for t in tasks], priority=priority)

		OPTIONAL MATCH (th:TaskHub {_scoped_key: $taskhub})-[ar:ACTIONS]->(task:Task {_scoped_key: task_scoped_key})
		RETURN task_scoped_key, ar.weight AS weight

Use query parameters wherever possible in Neo4jStore #330

Use query parameters wherever possible in Neo4jStore #330

Conversation

LilDojd commented Nov 16, 2024 • edited Loading

Changes Made

ianmkenney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 18, 2024 • edited Loading

Codecov Report

LilDojd commented Nov 18, 2024

dotsdl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LilDojd commented Nov 19, 2024

ianmkenney commented Nov 19, 2024

LilDojd commented Nov 16, 2024 •

edited

Loading

codecov bot commented Nov 18, 2024 •

edited

Loading