Skip to content

validating meta fields to prevent SQL injection#3511

Open
davidsbatista wants to merge 1 commit into
mainfrom
fix/oracle-doc-store-SQL-injection
Open

validating meta fields to prevent SQL injection#3511
davidsbatista wants to merge 1 commit into
mainfrom
fix/oracle-doc-store-SQL-injection

Conversation

@davidsbatista

@davidsbatista davidsbatista commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Related Issues

Proposed Changes:

  • Moves _SAFE_FIELD_PATH and _validate_field_path from document_store.py into filters.py
  • To avoid circular imports, document_store.py already imports FilterTranslator from filters.py
  • Calls _validate_field_path inside _field_to_sql before any interpolation

How did you test it?

  • new unit tests in test_filter_translator.py covering the use cases raised in the issue

Checklist

@github-actions

Copy link
Copy Markdown
Contributor

Coverage report (oracle)

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  integrations/oracle/src/haystack_integrations/document_stores/oracle
  document_store.py
  filters.py
Project Total  

This report was generated by python-coverage-comment-action

@davidsbatista davidsbatista marked this pull request as ready for review June 30, 2026 14:51
@davidsbatista davidsbatista requested a review from a team as a code owner June 30, 2026 14:51
@davidsbatista davidsbatista requested review from bogdankostic and removed request for a team June 30, 2026 14:51

@bogdankostic bogdankostic left a comment

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.

Looks good, just one minor suggestion, I'm also okay with merging it as is.

def _validate_field_path(field_path: str) -> None:
if not _SAFE_FIELD_PATH.match(field_path):
msg = f"Invalid metadata field name: {field_path!r}"
raise ValueError(msg)

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.

Suggestion: I know we used ValueError also before, but while we're at it maybe we chould change this to FilterError?

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