Skip to content

fix(dynamodb-mcp-server): validate and escape schema names in Python generator - #4579

Open
LeeroyHannigan wants to merge 5 commits into
mainfrom
fix/python-repo-generator-name-injection
Open

fix(dynamodb-mcp-server): validate and escape schema names in Python generator#4579
LeeroyHannigan wants to merge 5 commits into
mainfrom
fix/python-repo-generator-name-injection

Conversation

@LeeroyHannigan

@LeeroyHannigan LeeroyHannigan commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Summary

The Python data-access-layer generator (repo_generation_tool) renders source code through
Jinja2 with autoescaping disabled, and nothing downstream escaped schema values, so values
from schema.json were written into the generated code verbatim.

Schema validation did not prevent this. It checked required fields, JSON types, enum values
and uniqueness, but applied no character or identifier constraint to any name — while a
comment in jinja2_generator.py asserted the opposite ("Schema validation ensures all inputs
are safe before template rendering"). That comment is corrected here.

This applies to the Python generator the same discipline #4384 applied to the CDK generator.
Values are constrained during schema validation, which already gates rendering (generate()
returns before create_generator() when validation fails), so an invalid schema is rejected
before any template runs.

Approach

A new core/name_validator.py holds the rules, applied according to where a value lands in
the generated code — the position determines what is possible, so one rule does not fit all:

Position Rule Applies to
Identifier (class/def/attribute/import name) must be a valid non-keyword Python identifier entity names, field names, access pattern names, parameter names, key template placeholders
Identifier fragment (method-name suffix) identifier characters only; may start with a digit GSI names
String literal / docstring / comment no quotes, backslashes, newlines or braces table names, key attribute names, entity types, index names
Prose as above but apostrophes allowed descriptions

Escaping is impossible in an identifier position, which is why those values must be rejected
rather than escaped. Conversely, DynamoDB permits almost any character in an attribute name,
so string-literal positions are not restricted to an identifier — spaces and non-ASCII text
remain valid.

Braces are rejected in string-literal positions because several of those positions are
f-strings, where a brace is a live expression rather than literal text.

Key templates are validated separately: they are rendered into f-strings, so a brace region
that is not a supported {placeholder} is rejected. The existing placeholder regex matches
only word-character placeholders and silently ignores any other brace region, so such a region
previously reached an f-string unchecked.

Sinks covered

Beyond the names above, these reached generated code and no validator visited them:

  • parameter default — rendered into a generated def signature. Validated here, and the
    generator now also escapes it with json.dumps so correctness does not rest on validation
    alone.
  • parameter description — reaches the generated method docstring.
  • filter_expression param / param2 / params[] — reach a docstring and comments.
  • cross-table pattern name and description — reach a def name and its docstring in the
    transaction service.
  • entities_involved[].condition — reaches comments in the transaction service.
  • table-level gsi_list entries — GSIValidator validated their structure but never their
    characters, while their key attribute names reach generated query code.

Separately, the usage_data string escaper handled backslashes and double quotes but not
newlines, so a newline in a sample value produced an unterminated string literal in the
generated example. It now escapes newlines and carriage returns too.

Confirmed already constrained and deliberately left alone: range_condition and
projection_type are restricted to enum allowlists; item_type is never interpolated and
unknown values map to Any; parameter entity_type and entities_involved[].entity are
membership-checked against entity names, which this change makes safe.

Behaviour change

A schema whose entity or field names are not valid Python identifiers is now rejected with a
clear validation error instead of producing code that cannot be imported. Worth a release note.

Testing

  • New tests/repo_generation_tool/unit/test_name_validator.py: unit coverage per rule plus
    end-to-end assertions that a schema targeting each sink fails validation and writes no file.
  • Verified per sink that a payload which previously reached generated code is now rejected. For
    the two that produced executable output, executability was confirmed beforehand by parsing
    the generated file and finding the payload as a live AST node rather than as text in a
    comment — and after the change both are rejected during validation.
  • Legitimate schemas still generate unchanged, including an attribute name containing a space
    and a description containing an apostrophe (36 of the 447 descriptions in the repo fixtures
    contain apostrophes, which is why prose allows them).
  • Full package suite: 1582 passed. ruff check, ruff format --check and pyright clean.
  • pre-commit could not run end to end in my environment: its gitleaks hook cannot fetch Go
    modules through my network. The Python hooks it wraps (ruff, pyright) were run directly.

One pre-existing property test asserted that arbitrary text is a valid GSI name, which encoded
the unsafe behaviour; its strategy is narrowed to names usable in generated code, with
rejection covered by the new tests.


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.

…generator

The Python data-access-layer generator renders source code through Jinja2 with autoescaping
disabled, and nothing downstream escaped schema values, so values from schema.json were
written into the generated code verbatim. Schema validation did not prevent this: it checked
required fields, JSON types, enum values and uniqueness, but applied no character or
identifier constraint to any name, while a comment in the generator asserted the opposite.

This applies to the Python generator the discipline #4384 applied to the CDK generator.
Values are constrained during schema validation, which already gates rendering, so an invalid
schema is rejected before any template runs.

A new core/name_validator.py holds the rules, applied according to where a value lands in the
generated code:

- Identifier positions -- entity names, field names, access pattern names, parameter names and
  key template placeholders become class names, def names and attribute names, where escaping
  is not possible. These must be valid non-keyword Python identifiers. In practice this is not
  a new restriction: a value that is not an identifier produced a SyntaxError, not usable code.
- Identifier fragments -- a GSI name becomes a suffix of generated method names, so it needs
  only identifier characters and may begin with a digit.
- String-literal positions -- table names, key attribute names, entity types and index names
  are checked only for characters that would end the enclosing literal, docstring or comment,
  and for braces, because some of these positions are f-strings where a brace is an
  expression. DynamoDB permits almost any character in an attribute name, so spaces and
  non-ASCII text remain valid.
- Prose -- descriptions additionally allow apostrophes, which are ordinary in English.

Key templates are validated separately: they are rendered into f-strings, so a brace region
that is not a supported {placeholder} is rejected. The existing placeholder regex matches only
word-character placeholders and ignores any other brace region, so such a region previously
reached an f-string unchecked.

Covered sinks that no validator previously visited: parameter defaults (rendered into a def
signature, and now also escaped with json.dumps in the generator so correctness does not rest
on validation alone), parameter descriptions, filter expression parameter references,
cross-table pattern names and descriptions, and entity involvement conditions. Table-level
gsi_list entries are covered too -- GSIValidator validated their structure but never their
characters, while their key attribute names reach generated query code.

Separately, the usage_data string escaper handled backslashes and double quotes but not
newlines, so a newline in a sample value produced an unterminated string literal in the
generated example. It now escapes newlines and carriage returns as well.

Left as-is, having been confirmed already constrained: range_condition and projection_type are
restricted to enum allowlists, item_type is never interpolated and unknown values map to Any,
and parameter entity_type and entity involvement entity are membership-checked against entity
names.

Behaviour change: a schema whose entity or field names are not valid Python identifiers is now
rejected with a clear validation error instead of producing code that cannot be imported.

One property test asserted that arbitrary text is a valid GSI name; its strategy is narrowed
to names usable in generated code, and the new tests cover rejection.
@codecov

codecov Bot commented Sep 3, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.19728% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.27%. Comparing base (e9f2439) to head (eb5cc9d).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...rver/repo_generation_tool/core/schema_validator.py 88.13% 1 Missing and 6 partials ⚠️
...repo_generation_tool/core/cross_table_validator.py 71.42% 0 Missing and 2 partials ⚠️
...eneration_tool/core/filter_expression_validator.py 87.50% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main    #4579    +/-   ##
========================================
  Coverage   93.27%   93.27%            
========================================
  Files        1053     1054     +1     
  Lines       89208    89344   +136     
  Branches    14385    14441    +56     
========================================
+ Hits        83205    83337   +132     
+ Misses       3626     3624     -2     
- Partials     2377     2383     +6     

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

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Adds coverage for the branch in each validator that reports a non-string value instead of
raising, and marks the defensive isinstance re-check in validate_key_template as unreachable
through that function, since validate_literal_safe rejects a non-string first.
…ibutes

Covers the remaining validation branches for table-level gsi_list entries: composite
partition and sort keys given as attribute lists, included_attributes, and a malformed
entry that the name checks must walk past without raising.
Moves the string type check in validate_key_template ahead of the delegation to
validate_literal_safe. The check was previously placed after it, which made it unreachable
and required a coverage exclusion; doing it first also makes the function's own type
narrowing explicit rather than inherited.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: To triage

Development

Successfully merging this pull request may close these issues.

2 participants