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 Artifact Tracking SQL Injection Bug #1034

Merged
merged 5 commits into from Oct 11, 2022
Merged

Fix Artifact Tracking SQL Injection Bug #1034

merged 5 commits into from Oct 11, 2022

Conversation

ghost
Copy link

@ghost ghost commented Oct 8, 2022

Hey everyone,

Artifact tracking appears to be broken when a parent directory contains a SQL special character. The code that I modified (local_file_hash_storage.py) assumed that Unix paths could be injected without being escaped into SQL statements -- so I just did the standard ? SQL escaping. If you are tracking artifacts in a directory that has a parent that contains, for example, ' (e.g. Frederick's Workspace/) would break the SQL queries necessary for tracking artifacts.

@Blaizzy
Copy link
Contributor

Blaizzy commented Oct 8, 2022

Hey @fmorlock-tt

Thank you very much for the detailed explanation of the issue and the PR!

I will talk to the engineering team and come back to you as soon as I hear from them.

@Raalsky
Copy link
Contributor

Raalsky commented Oct 10, 2022

Hey @fmorlock-tt, I really appreciate your work!

LGTM, but two minor things:

  • Could you rebase/sync your fork as we've rewritten some Github Workflows settings this should be fixed now
  • Update the CHANGELOG

@ghost
Copy link
Author

ghost commented Oct 10, 2022

Hi @Raalsky and @Blaizzy

I made synced my branch and added a line to the Changelog. Please feel free to make edits to my branch if you'd like since maintainers should have access (I think).

Best,
Frederick

@Raalsky Raalsky merged commit 93e3c70 into neptune-ai:master Oct 11, 2022
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