Skip to content

Conversation

@richardwooding
Copy link
Contributor

Summary

Fixes #22 - Adds proper escaping of single quotes in JSON field names to prevent SQL injection vulnerabilities in PostgreSQL JSON path operators.

Changes Made

1. Added escapeJSONFieldName() Helper Function (utils.go)

  • ✅ Escapes single quotes by doubling them (''')
  • ✅ Simple and efficient implementation using strings.ReplaceAll()
  • ✅ Follows PostgreSQL standard for escaping quotes in string literals

2. Updated cel2sql.go to Escape Field Names

  • visitSelect(): Escapes field names in ->> and ->>' operators (lines 1119, 1124)
  • visitHasFunction(): Escapes field names in ? and -> operators (lines 1157, 1162)
  • visitNestedJSONHas(): Escapes path segments in jsonb_extract_path_text() (line 1220)

3. Updated json.go to Escape Field Names

  • buildJSONPathForArray(): Escapes field names in nested -> operators (lines 151, 172)
  • buildJSONPathInternal(): Escapes field names in all JSON path construction (lines 372, 389)

4. Comprehensive Test Coverage

  • utils_test.go: 9 unit tests for escapeJSONFieldName() function
  • json_escaping_test.go: Integration tests through full CEL-to-SQL pipeline
  • ✅ Tests cover: normal names, quotes at various positions, SQL injection attempts, empty strings

Security Impact

This fix provides defense-in-depth protection against SQL injection:

  1. Prevents quote injection: Field names containing single quotes are safely escaped
  2. Blocks SQL injection: Attempts like user' OR '1'='1 are neutralized
  3. Multiple protection layers: Escaping applied at all JSON path generation points
  4. PostgreSQL standard: Uses PostgreSQL's standard quote escaping ('')

Examples

Before (vulnerable):

-- Malicious field name: user' OR '1'='1
-- Would generate: col->'user' OR '1'='1'
-- Breaking out of string literal

After (protected):

-- Malicious field name: user' OR '1'='1  
-- Now generates: col->'user'' OR ''1''=''1'
-- Single quotes escaped, treated as field name

Test Results

✅ All tests passing
✅ 9 unit tests for escapeJSONFieldName()
✅ Integration tests for JSON path operators
✅ golangci-lint: 0 issues
✅ No breaking changes to public API

Locations Fixed

All locations where field names are inserted into JSON path operators:

  • cel2sql.go:1119 - ->> operator (text extraction)
  • cel2sql.go:1124 - ->>' operator (object access)
  • cel2sql.go:1157 - ? operator (existence check)
  • cel2sql.go:1162 - -> operator (JSON field check)
  • cel2sql.go:1220 - jsonb_extract_path_text() arguments
  • json.go:151 - Nested -> operator (array paths)
  • json.go:172 - -> operator (operand paths)
  • json.go:372 - Intermediate/final JSON paths
  • json.go:389 - Base JSON paths

Testing Checklist

  • Unit tests pass (go test ./...)
  • Integration tests pass
  • Linting passes (make lint)
  • No breaking changes to public API
  • Security vulnerability addressed

🤖 Generated with Claude Code

#22)

This commit addresses issue #22 by properly escaping single quotes in all
JSON field names used in PostgreSQL JSON path operators.

Changes:
- Added escapeJSONFieldName() helper function in utils.go:
  * Escapes single quotes by doubling them (' -> '')
  * Prevents SQL injection when field names contain quotes
  * Simple and efficient implementation

- Updated cel2sql.go to escape field names:
  * visitSelect(): Escapes field names in ->> and ->> operators
  * visitHasFunction(): Escapes field names in ? and -> operators
  * visitNestedJSONHas(): Escapes path segments in jsonb_extract_path_text()

- Updated json.go to escape field names:
  * buildJSONPathForArray(): Escapes field names in nested -> operators
  * buildJSONPathInternal(): Escapes field names in all JSON path construction

- Comprehensive test coverage:
  * utils_test.go: 9 unit tests for escapeJSONFieldName() function
  * json_escaping_test.go: Integration tests and security documentation
  * Tests cover: normal names, quotes at various positions, SQL injection attempts

Security Impact:
- Prevents SQL injection through malicious JSON field names
- Protects against field names like: user' OR '1'='1
- Defense-in-depth: escaping applied at multiple pipeline stages
- Follows PostgreSQL standard for escaping quotes in string literals

All tests passing with proper linting.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR addresses a SQL injection vulnerability in JSON field name handling by implementing proper escaping of single quotes in PostgreSQL JSON path operators. The fix applies defense-in-depth protection across all JSON path generation points in the codebase.

Key Changes:

  • Added escapeJSONFieldName() utility function that doubles single quotes following PostgreSQL's standard escaping convention
  • Applied escaping to all JSON path operators (->, ->>, ?) and jsonb_extract_path_text() function calls across cel2sql.go and json.go
  • Comprehensive test coverage including unit tests and integration tests verifying the escaping behavior

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
utils.go Adds escapeJSONFieldName() helper function for PostgreSQL quote escaping
utils_test.go Adds 9 unit tests covering various escaping scenarios including SQL injection attempts
cel2sql.go Applies escaping to JSON operators in visitSelect(), visitHasFunction(), and visitNestedJSONHas()
json.go Applies escaping to JSON path construction in buildJSONPathForArray() and buildJSONPathInternal()
json_escaping_test.go Adds integration tests verifying end-to-end CEL-to-SQL conversion with field name escaping

{
name: "JSON field with single quote in name",
celExpr: `obj.metadata.user_name == "test"`,
expectedSQL: `obj->>'metadata'->>'user_name' = 'test'`,
Copy link

Copilot AI Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The expected SQL is incorrect. Based on the JSON path generation rules in the coding guidelines, intermediate navigation should use -> (returns JSON) and only the final extraction should use ->> (returns text). The correct expected SQL should be obj.metadata->>'user_name' = 'test' since metadata is an intermediate field and user_name is the final field being extracted.

Suggested change
expectedSQL: `obj->>'metadata'->>'user_name' = 'test'`,
expectedSQL: `obj->'metadata'->>'user_name' = 'test'`,

Copilot uses AI. Check for mistakes.
{
name: "Nested JSON access",
celExpr: `obj.metadata.settings.theme == "dark"`,
expectedSQL: `obj->>'metadata'->'settings'->>'theme' = 'dark'`,
Copy link

Copilot AI Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The expected SQL contains an incorrect operator sequence. According to the JSON path operator rules, ->>' extracts text and should only be used for the final field. The first operator should be -> since metadata is an intermediate field. The correct expected SQL should be obj.metadata->'settings'->>'theme' = 'dark'.

Suggested change
expectedSQL: `obj->>'metadata'->'settings'->>'theme' = 'dark'`,
expectedSQL: `obj->'metadata'->'settings'->>'theme' = 'dark'`,

Copilot uses AI. Check for mistakes.
// through the integration tests above. This test documents the expected behavior.

t.Log("The escapeJSONFieldName() function in utils.go escapes single quotes")
t.Log("Example: \"user's name\" -> \"user''s name\"")
Copy link

Copilot AI Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected example text formatting for clarity. The backslashes are unnecessary escape sequences in the raw string and make the example harder to read.

Suggested change
t.Log("Example: \"user's name\" -> \"user''s name\"")
t.Log(`Example: "user's name" -> "user''s name"`)

Copilot uses AI. Check for mistakes.
@richardwooding richardwooding merged commit c265ffd into main Oct 22, 2025
9 checks passed
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.

SQL Injection Risk via Unescaped Field Names in JSON Path Operators

1 participant