Skip to content

Conversation

ntrevino-virtru
Copy link
Contributor

@ntrevino-virtru ntrevino-virtru commented Sep 4, 2025

Proposed Changes

  • Sanitize db identifiers

I think I tried passing these as parameters to the query but that didn't work if I recall.

Checklist

  • I have added or updated unit tests
  • I have added or updated integration tests (if appropriate)
  • I have added or updated documentation

Testing Instructions

Copy link
Contributor

github-actions bot commented Sep 4, 2025

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 209.579707ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 103.500274ms

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 358.889117ms
Throughput 278.64 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 38.633647879s
Average Latency 384.626296ms
Throughput 129.42 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 25.897882886s
Average Latency 257.886098ms
Throughput 193.07 requests/second

@ntrevino-virtru ntrevino-virtru marked this pull request as ready for review September 4, 2025 15:27
@ntrevino-virtru ntrevino-virtru requested a review from a team as a code owner September 4, 2025 15:27
@jakedoublev jakedoublev requested a review from Copilot September 4, 2025 15:33
Copy link
Contributor

@Copilot 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 sanitizes database schema identifiers to prevent SQL injection vulnerabilities by using pgx's built-in Identifier.Sanitize() method instead of direct string concatenation.

  • Replaces unsafe string concatenation with pgx.Identifier sanitization for schema names
  • Adds import for pgx package to support the Identifier type

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
service/pkg/db/db_migration.go Sanitizes schema identifier in CREATE SCHEMA statement and adds pgx import
service/pkg/db/db.go Sanitizes schema identifier in SET search_path statement

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request correctly addresses a critical SQL injection vulnerability by sanitizing database schema identifiers. The use of pgx.Identifier{...}.Sanitize() is an effective fix. I've added a couple of suggestions for further refinement to use pgx's parameter binding for identifiers, which is a more idiomatic approach and improves code clarity and maintainability.

@strantalis strantalis changed the title chore(main): sanitize db schema identifiers fix: sanitize db schema identifiers Sep 4, 2025
@ntrevino-virtru ntrevino-virtru added this pull request to the merge queue Sep 4, 2025
Merged via the queue into main with commit 0d3dd94 Sep 4, 2025
37 checks passed
@ntrevino-virtru ntrevino-virtru deleted the chore/dspx-1543/db-identifiers branch September 4, 2025 16:27
github-merge-queue bot pushed a commit that referenced this pull request Sep 17, 2025
🤖 I have created a release *beep* *boop*
---


##
[0.10.0](service/v0.9.0...service/v0.10.0)
(2025-09-17)


### ⚠ BREAKING CHANGES

* **policy:** Add manager column to provider configuration for
multi-instance support
([#2601](#2601))

### Features

* **authz:** add obligation policy decision point
([#2706](#2706))
([bb2a4f8](bb2a4f8))
* **core:** add service negation for op mode
([#2680](#2680))
([029db8c](029db8c))
* **core:** Bump default write timeout.
([#2671](#2671))
([6a233c1](6a233c1))
* **core:** Encapsulate>Encrypt
([#2676](#2676))
([3c5a614](3c5a614))
* **core:** Lets key manager factory take context
([#2715](#2715))
([8d70993](8d70993))
* **policy:** add FQN of obligation definitions/values to protos
([#2703](#2703))
([45ded0e](45ded0e))
* **policy:** Add manager column to provider configuration for
multi-instance support
([#2601](#2601))
([a5fc994](a5fc994))
* **policy:** Add obligation triggers
([#2675](#2675))
([22d0837](22d0837))
* **policy:** add protovalidate for obligation defs + vals
([#2699](#2699))
([af5c049](af5c049))
* **policy:** Allow creation and update of triggers on Obligation Values
([#2691](#2691))
([b1e7ba1](b1e7ba1))
* **policy:** Allow for additional context to be added to obligation
triggers ([#2705](#2705))
([7025599](7025599))
* **policy:** Include Triggers in GET/LISTable reqs
([#2704](#2704))
([b4381d1](b4381d1))
* **policy:** obligations + values CRUD
([#2545](#2545))
([c194e35](c194e35))
* use public AES protected key from lib/ocrypto
([#2600](#2600))
([75d7590](75d7590))


### Bug Fixes

* **core:** remove extraneous comment
([#2741](#2741))
([ada8da6](ada8da6))
* **core:** return services in the order they were registered
([#2733](#2733))
([1d661db](1d661db))
* **deps:** bump github.com/opentdf/platform/lib/ocrypto from 0.3.0 to
0.6.0 in /service
([#2714](#2714))
([00354b3](00354b3))
* **deps:** bump github.com/opentdf/platform/protocol/go from 0.7.0 to
0.9.0 in /service
([#2726](#2726))
([9004368](9004368))
* **deps:** bump protocol/go to 0.10.0 in service
([#2734](#2734))
([11e6201](11e6201))
* **deps:** update protovalidate to v0.14.2 to use new buf validate
MessageOneofRule
([#2698](#2698))
([1cae18e](1cae18e))
* **policy:** Registered Resources should consider actions correctly
within Decision Requests
([#2681](#2681))
([cf264a2](cf264a2))
* sanitize db schema identifiers
([#2682](#2682))
([0d3dd94](0d3dd94))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants