fix(byokg-rag): escape property names in Neptune get_nodes/get_one_hop_edges - #13
Open
noel-improv wants to merge 17 commits into
Open
noel-improv wants to merge 17 commits into
noel-improv wants to merge 17 commits into
Conversation
|
BYOKG-RAG Coverage Report: The coverage is at 94.88% (target: 80%). Download the HTML report here. |
* [FEATURE] Add progress construct for extract/build Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com> * Add more tests Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com> * Update tests to use logger; Add logger to workshop examples Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com> * Remove unnecessary imports Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com> --------- Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>
|
BYOKG-RAG Coverage Report: The coverage is at 94.88% (target: 80%). Download the HTML report here. |
|
BYOKG-RAG Coverage Report: The coverage is at 94.88% (target: 80%). Download the HTML report here. |
…abs#352) Run gitleaks on pull requests and pushes (scanning only the new commits) and on a weekly schedule (full history), uploading redacted SARIF to the Security tab. Uses the MIT-licensed gitleaks binary pinned by version and SHA-256, modeled on the awslabs org pattern in agent-plugins and aidlc-workflows. Defense-in-depth over org-level push protection.
|
BYOKG-RAG Coverage Report: The coverage is at 94.88% (target: 80%). Download the HTML report here. |
* Update llama-index dependencies Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com> * Loosen requirement constraints Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com> --------- Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>
Bumps [astro](https://github.com/withastro/astro/tree/HEAD/packages/astro) from 6.4.8 to 7.0.6. - [Release notes](https://github.com/withastro/astro/releases) - [Changelog](https://github.com/withastro/astro/blob/main/packages/astro/CHANGELOG.md) - [Commits](https://github.com/withastro/astro/commits/astro@7.0.6/packages/astro) --- updated-dependencies: - dependency-name: astro dependency-version: 7.0.6 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
) Bumps [sharp](https://github.com/lovell/sharp) from 0.35.2 to 0.35.3. - [Release notes](https://github.com/lovell/sharp/releases) - [Commits](lovell/sharp@v0.35.2...v0.35.3) --- updated-dependencies: - dependency-name: sharp dependency-version: 0.35.3 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…tform and Rules constraints (awslabs#375) Bump the notebook PlatformIdentifier from the retired notebook-al2-v3 to notebook-al2023-v1 across all eight lexical-graph Neptune templates. AWS retired the Amazon Linux 2 notebook platforms, so stacks using al2-v3 now fail at create time with "notebook-al2-v3 is not supported". In graphrag-toolkit-neptune-db-opensearch-serverless.json, replace the two Fn::Select-based subnet checks in the Rules block with Fn::EachMemberEquals. Fn::Select is not among the intrinsic functions permitted inside Rules, so the template rolled back at create-stack time on the VPC-reuse path.
…ite (awslabs#378) Bumps [@astrojs/starlight](https://github.com/withastro/starlight/tree/HEAD/packages/starlight) from 0.40.0 to 0.41.3. - [Release notes](https://github.com/withastro/starlight/releases) - [Changelog](https://github.com/withastro/starlight/blob/main/packages/starlight/CHANGELOG.md) - [Commits](https://github.com/withastro/starlight/commits/@astrojs/starlight@0.41.3/packages/starlight) --- updated-dependencies: - dependency-name: "@astrojs/starlight" dependency-version: 0.41.3 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…awslabs#369) * fix(lexical-graph): escape OpenCypher metadata filter values and keys Metadata filters were converted to an OpenCypher filter substring by raw string interpolation: text values were wrapped in single quotes with no escaping and property keys were interpolated as source.{key}. A filter value or key containing a single quote (or backtick) could break out and inject OpenCypher. This substring is spliced into queries by several callers, so it bypasses the parameter binding the graph store uses elsewhere. Escape at the point of interpolation, reusing the module's existing backtick-escaping helper: - new escape_cypher_string doubles backslashes then backslash-escapes single quotes, so a value cannot close its literal. - formatter_for_type routes text and timestamp values through it. - to_key backtick-quotes the property key via escape_cypher_label (source.`{key}`), so a key cannot break out of the identifier. Adds tests asserting values are escaped inside their literal and keys are backtick-quoted (embedded backticks doubled), plus the benign positive paths. Existing graph_utils assertions are updated for the backtick-quoted keys. * docs(lexical-graph): note backslash-doubling semantics and the unquoted numeric path Records that doubling a backslash in escape_cypher_string changes match semantics for a value containing a literal backslash (acceptable, since a correct non-injectable filter takes priority), and that the numeric formatter is intentionally unquoted because the type is derived from the value's Python type. Comments only, no behaviour change. * test(security): add OpenCypher filter-injection integration test Couples the integration test with this fix. It runs the clause emitted by the real filter builder against a live openCypher engine, so a regression that drops the escaping is caught. A dedicated workflow runs it against a Neo4j service container on release and on PRs touching the sink or the test. Skips when NEO4J_TEST_URI is unset. * test(lexical-graph): consolidate filter-injection test into cypher-safety suite Move the standalone OpenCypher filter-injection test into the lexical cypher-safety suite as LexicalGraphFilterInjectionSafety, and register it in lexical.short. Removes the standalone workflow and test file it replaces. Default the Neptune DB data-plane endpoint to port 8182 in the suite's _make_graph_store helpers. Without it the neptunedata client uses the https default (443), which the cluster does not serve, so the neptune-db tests time out instead of connecting. Validated live on a Neptune DB stack: LexicalGraphFilterInjectionSafety and the LexicalGraphLabelInjectionSafety baseline both pass; the filter key and value escaping holds (canary survives key and value injection).
…wslabs#377) Bumps [@astrojs/react](https://github.com/withastro/astro/tree/HEAD/packages/integrations/react) from 4.4.2 to 6.0.1. - [Release notes](https://github.com/withastro/astro/releases) - [Changelog](https://github.com/withastro/astro/blob/main/packages/integrations/react/CHANGELOG.md) - [Commits](https://github.com/withastro/astro/commits/@astrojs/react@6.0.1/packages/integrations/react) --- updated-dependencies: - dependency-name: "@astrojs/react" dependency-version: 6.0.1 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Updates the requirements on [hypothesis](https://github.com/HypothesisWorks/hypothesis) to permit the latest version. - [Release notes](https://github.com/HypothesisWorks/hypothesis/releases) - [Commits](HypothesisWorks/hypothesis@v6.155.7...v6.156.1) --- updated-dependencies: - dependency-name: hypothesis dependency-version: 6.156.1 dependency-type: direct:development ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
|
BYOKG-RAG Coverage Report: The coverage is at 94.88% (target: 80%). Download the HTML report here. |
|
BYOKG-RAG Coverage Report: The coverage is at 94.88% (target: 80%). Download the HTML report here. |
…slabs#374) * Update boto3 api: remove readOnly param from neptune-graph client Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com> * Fix integration tests; fix unit test Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com> --------- Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>
…p_edges
get_nodes and get_one_hop_edges build their WHERE clause by interpolating
property names from node_type_to_property_mapping straight into the OpenCypher
query (n.{prop}). Node ids are bound as $node_ids, but the property name is
raw, so a name containing a backtick (sourced from schema or caller config)
could break out of the identifier and inject Cypher.
Backtick-quote the property and escape it with the module's existing
_escape_cypher_label helper (n.`{escaped}`), matching how node labels are
already handled. This also brings both sinks under the TestNoUnescapedLabelSinks
regression guard, which only inspects backtick-quoted spans.
Adds tests asserting the property is backtick-quoted with embedded backticks
doubled on both sinks, plus the benign positive path.
Couples the integration test with this fix. It drives the real get_nodes /
get_one_hop_edges sinks and runs the emitted OpenCypher against a live
openCypher engine, so a regression that reverts a sink to raw n.{prop} is
caught. A dedicated workflow runs it against a Neo4j service container on
release and on PRs touching the sink or the test. Skips when NEO4J_TEST_URI is
unset.
…tate
The property-name injection tests asserted the escaped form is present but not
that the raw n.{prop} form is absent, and had no escape-disabled red-state. Add
a "raw form absent" assertion to both sink tests and a test that disables
_escape_cypher_label to confirm the doubling is what neutralizes the injection.
…n suite Consolidate the standalone property-name injection test into the byokg integration suite as BYOKGPropertyNameInjectionSafety, and register it in byokg.short/byokg.all. Removes the standalone workflow and test file it replaces. Default the Neptune DB data-plane endpoint to port 8182 in the suite's _make_graph_store helpers. Without it the neptunedata client uses the https default (443), which the cluster does not serve, so the neptune-db tests time out instead of connecting. Validated live on a Neptune DB stack: BYOKGPropertyNameInjectionSafety and BYOKGS3PathInjectionSafety pass; the property-name escaping holds (canary survives the injection payload).
Fold the get_nodes and get_one_hop_edges backtick-escaping tests into a single parametrized test over the sink, and drop a trailing-whitespace line. Coverage is unchanged. Addresses review feedback on awslabs#370.
Extract make_graph_store() and canary_count() to module level in byokg_cypher_safety.py and point all three BYOKG security test classes at them. The store builder was copied three times, each with its own port-8182 fix, and the canary counter three times, so a change to one could miss the others. Behavior is unchanged. Addresses review feedback on awslabs#370.
noel-improv
force-pushed
the
fix/byokg-neptune-property-name-injection
branch
from
July 7, 2026 22:41
f1cdce5 to
4170cca
Compare
|
Lexical Graph Coverage Report: The coverage is at 60.33% (target: 80%). Download the HTML report here. |
|
BYOKG-RAG Coverage Report: The coverage is at 94.88% (target: 80%). Download the HTML report here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Escape property names in the byokg-rag Neptune graph store.
get_nodesandget_one_hop_edgesinterpolate property names fromnode_type_to_property_mappingstraight into the OpenCypher WHERE clause (n.{prop}). Node ids are bound as$node_ids, but the property name is raw, so a name containing a backtick — sourced from schema or caller config — could break out of the identifier and inject Cypher.Changes
Backtick-quote the property and escape it with the module's existing
_escape_cypher_labelhelper (n.\{escaped}`), matching how node labels are already handled in this file. This also brings both sinks under the existingTestNoUnescapedLabelSinks` regression guard, which only inspects backtick-quoted spans.Tests
Adds tests asserting the property is backtick-quoted with embedded backticks doubled on both sinks, plus the benign positive path. The emitted query was also validated against a live openCypher engine (Neo4j): a backtick-quoted property is treated as a literal key, an injection-laden name is inert, and the old raw interpolation raises a syntax error. Bandit reports no findings.