Fix security vulnerabilities in Node.js driver #2329
Conversation
github-actions
bot
added
master
override-stale
Note: This PR was created with AI tools and a human. - Add input validation for graph names, label names, and column names to prevent SQL injection via string interpolation. Graph name rules align with Apache AGE's internal validation and Neo4j/openCypher conventions (hyphens and dots permitted, min 3 chars, max 63 chars). Label names follow AGE's stricter rules (no hyphens or dots). - Add safe query helpers: queryCypher(), createGraph(), dropGraph() with graph name validation and dollar-quoting for Cypher strings - Use BigInt for integer values exceeding Number.MAX_SAFE_INTEGER to prevent silent precision loss with 64-bit AGE graph IDs - Make CREATE EXTENSION opt-in via SetAGETypesOptions.createExtension instead of running DDL automatically without user consent - Add descriptive error message when AGE extension is not found - Tighten pg dependency from >=6.0.0 to >=8.0.0 - Add comprehensive test suites covering validation, SQL injection and prevention modified: drivers/nodejs/package.json modified: drivers/nodejs/src/antlr4/CustomAgTypeListener.ts modified: drivers/nodejs/src/index.ts modified: drivers/nodejs/test/Agtype.test.ts modified: drivers/nodejs/test/index.test.ts
jrgemignani
force-pushed
the
fix_security_nodejs_driver
branch
from
6bc32a4 to
b1c8379
Compare
There was a problem hiding this comment.
Pull request overview
This PR hardens the Node.js AGE driver against SQL injection and correctness issues by adding identifier validation, safer Cypher execution helpers, and improved agtype parsing behavior for large integers.
Changes:
- Added validation helpers for graph/label/column identifiers and introduced safer query wrappers (
queryCypher,createGraph,dropGraph). - Updated agtype integer parsing to use
BigIntwhen values exceedNumber.MAX_SAFE_INTEGER. - Adjusted driver behavior/tests:
setAGETypesno longer auto-runsCREATE EXTENSIONby default, updated tests andpgdependency floor.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| drivers/nodejs/package.json | Tightens pg dependency to >=8.0.0. |
| drivers/nodejs/src/antlr4/CustomAgTypeListener.ts | Parses large integers as BigInt to avoid precision loss. |
| drivers/nodejs/src/index.ts | Adds validation + safe query helpers; makes CREATE EXTENSION opt-in; improves missing agtype error message. |
| drivers/nodejs/test/Agtype.test.ts | Adds coverage for BigInt vs number parsing behavior. |
| drivers/nodejs/test/index.test.ts | Adds validation/injection-prevention tests and exercises new helper APIs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (createExtension) { | ||
| await client.query('CREATE EXTENSION IF NOT EXISTS age;') | ||
| } | ||
|
|
||
| await client.query("LOAD 'age';") | ||
| await client.query('SET search_path = ag_catalog, "$user", public;') |
There was a problem hiding this comment.
setAGETypes calls LOAD 'age' directly; when the AGE library/extension isn’t available, the thrown pg error is typically low-level (e.g., missing shared library) and bypasses the more descriptive “agtype type not found” message. Consider wrapping the LOAD/search_path/type-OID block in a try/catch and rethrowing a clearer error that mentions installing/creating the extension and/or using createExtension: true.
| /** | ||
| * Valid graph name pattern aligned with Apache AGE's internal validation | ||
| * and Neo4j/openCypher naming conventions. Allows letters, digits, | ||
| * underscores, plus dots and hyphens in middle positions. | ||
| * | ||
| * Start: letter or underscore | ||
| * Middle: letter, digit, underscore, dot, or hyphen | ||
| * End: letter, digit, or underscore | ||
| */ | ||
| const VALID_GRAPH_NAME = /^[A-Za-z_][A-Za-z0-9_.\-]*[A-Za-z0-9_]$/ | ||
|
|
||
| /** | ||
| * Valid label name pattern aligned with Apache AGE's internal validation. | ||
| * Labels follow stricter identifier rules than graph names — dots and | ||
| * hyphens are NOT permitted in label names. | ||
| */ | ||
| const VALID_LABEL_NAME = /^[A-Za-z_][A-Za-z0-9_]*$/ | ||
|
|
There was a problem hiding this comment.
The identifier validation patterns are documented as “aligned with Apache AGE’s internal validation”, but they currently only allow ASCII letters/digits/underscore (plus dot/hyphen for graphs). AGE’s own name validation accepts non-ASCII alphabetic characters for graph/label names (see regress/sql/name_validation.sql, e.g. mydätabase, myläbelx). Either broaden the regexes to support Unicode letters/numbers (e.g. Unicode property escapes with the u flag) or adjust the docs/error messages to clearly state the driver intentionally restricts names beyond AGE’s rules.
| async function dropGraph (client: Client, graphName: string, cascade: boolean = false): Promise<void> { | ||
| validateGraphName(graphName) | ||
| await client.query(`SELECT * FROM ag_catalog.drop_graph('${graphName}', ${cascade});`) | ||
| } |
There was a problem hiding this comment.
dropGraph interpolates cascade directly into the SQL string. Even though it’s typed as boolean in TS, this library is consumable from plain JS and a crafted non-boolean value could be injected into the query. Prefer using a parameterized query for graphName/cascade (or at least validate typeof cascade === 'boolean' before interpolation).
| await client.connect() | ||
| await setAGETypes(client, types) | ||
| await client.query(`SELECT create_graph('${testGraphName}');`) | ||
| await setAGETypes(client, types, { createExtension: false }) |
There was a problem hiding this comment.
The integration tests now call setAGETypes(..., { createExtension: false }), which makes the test suite depend on the target database already having CREATE EXTENSION age; applied. Previously the tests were self-contained via the implicit CREATE EXTENSION IF NOT EXISTS in setAGETypes. Consider using { createExtension: true } in tests (or explicitly creating the extension in test setup) to avoid environment-dependent failures.
| await setAGETypes(client, types, { createExtension: false }) | |
| await setAGETypes(client, types, { createExtension: true }) |
1 participant
Note: This PR was created with AI tools and a human.
All existing regression tests passed.
modified: drivers/nodejs/package.json
modified: drivers/nodejs/src/antlr4/CustomAgTypeListener.ts
modified: drivers/nodejs/src/index.ts
modified: drivers/nodejs/test/Agtype.test.ts
modified: drivers/nodejs/test/index.test.ts