Merge pull request #6 from waratuman/waratuman/security-performance-review

Fix geometry type: perf, nil-limit crash, adapter pollution, and DDL hardening

Author: waratuman

.github/workflows/ci.yml

@@ -0,0 +1,52 @@
+name: CI
+
+on:
+  push:
+    branches: [master]
+  pull_request:
+
+jobs:
+  test:
+    runs-on: ubuntu-latest
+
+    services:
+      postgres:
+        image: postgis/postgis:16-3.4
+        env:
+          POSTGRES_USER: postgres
+          POSTGRES_PASSWORD: postgres
+          POSTGRES_DB: postgis_adapter_test
+        ports:
+          - 5432:5432
+        options: >-
+          --health-cmd pg_isready
+          --health-interval 10s
+          --health-timeout 5s
+          --health-retries 5
+
+    env:
+      PGHOST: localhost
+      PGPORT: 5432
+      PGUSER: postgres
+      PGPASSWORD: postgres
+
+    strategy:
+      fail-fast: false
+      matrix:
+        ruby: ['3.2', '3.3', '3.4']
+
+    steps:
+      - uses: actions/checkout@v5
+
+      # rgeo's CAPI extension links against GEOS at build time, so libgeos-dev
+      # must be present before `bundle install` compiles the native extension.
+      - name: Install GEOS
+        run: sudo apt-get update && sudo apt-get install -y libgeos-dev
+
+      - uses: ruby/setup-ruby@v1
+        with:
+          ruby-version: ${{ matrix.ruby }}
+          bundler-cache: true
+
+      - name: Run tests
+        run: bundle exec rake test

lib/active_record/connection_adapters/postgis/oid/geometry.rb

@@ -8,6 +8,19 @@ module OID
         class Geometry < Type::Value
           include ActiveModel::Type::Helpers::Mutable
 
+          # Build the RGeo factory, parsers and generator once, up front. The
+          # SRID is fixed for the column, so these are invariant for the life of
+          # the type instance. Doing it here (rather than lazily on each cast)
+          # avoids rebuilding a GEOS factory on every value read, and is
+          # required because Type instances are frozen after initialization.
+          def initialize(...)
+            super
+            @rgeo_factory = RGeo::Geos.factory(srid: srid)
+            @wkb_parser = RGeo::WKRep::WKBParser.new(@rgeo_factory, support_ewkb: true, default_srid: srid)
+            @wkt_parser = RGeo::WKRep::WKTParser.new(@rgeo_factory, support_ewkt: true, default_srid: srid)
+            @wkb_generator = RGeo::WKRep::WKBGenerator.new(hex_format: true, type_format: :ewkb, emit_ewkb_srid: true)
+          end
+
           def type
             :geometry
           end
@@ -20,9 +33,9 @@ def cast(value)
               value
             when ::String # HEXEWKB
               if value[0,1] == "\x00" || value[0,1] == "\x01" || value[0,4] =~ /[0-9a-fA-F]{4}/
-                RGeo::WKRep::WKBParser.new(rgeo_factory_generator, support_ewkb: true, default_srid: limit[:srid]).parse(value)
+                @wkb_parser.parse(value)
               else
-                RGeo::WKRep::WKTParser.new(rgeo_factory_generator, support_ewkt: true, default_srid: limit[:srid]).parse(value)
+                @wkt_parser.parse(value)
               end
             else
               raise ArgumentError, "Unsupported argument type (#{value.class})"
@@ -32,7 +45,7 @@ def cast(value)
           def serialize(value)
             case value
             when  RGeo::Feature::Instance
-              ::RGeo::WKRep::WKBGenerator.new(hex_format: true, type_format: :ewkb, emit_ewkb_srid: true).generate(value)
+              @wkb_generator.generate(value)
             else
               value
             end
@@ -44,8 +57,8 @@ def changed_in_place?(raw_old_value, new_value)
 
           private
 
-          def rgeo_factory_generator
-            RGeo::Geos.factory(srid: limit[:srid].to_i)
+          def srid
+            limit ? limit[:srid].to_i : 0
           end
 
         end

lib/active_record/connection_adapters/postgis/schema_statements.rb

@@ -14,7 +14,7 @@ def type_to_sql(sql_type, limit: nil, precision: nil, scale: nil, array: nil, ty
               type ||= 'Geometry'
               srid ||= 4326
             end
-            "geometry(#{type},#{srid})"
+            "geometry(#{type},#{srid.to_i})"
           else
             super
           end

lib/active_record/connection_adapters/postgis_adapter.rb

@@ -12,7 +12,7 @@ module ConnectionAdapters
     class PostGISAdapter < PostgreSQLAdapter
       ADAPTER_NAME = 'PostGIS'.freeze
 
-      NATIVE_DATABASE_TYPES = PostgreSQLAdapter::NATIVE_DATABASE_TYPES.merge!({
+      NATIVE_DATABASE_TYPES = PostgreSQLAdapter::NATIVE_DATABASE_TYPES.merge({
         geometry: { name: "geometry" },
       })
 

test/cases/adapters/postgis/geometry_test.rb

@@ -27,7 +27,7 @@ def test_polygon_with_hole
     assert_equal "POLYGON ((0 0, 10 0, 10 10, 0 10, 0 0), (1 1, 1 2, 2 2, 2 1, 1 1))", geo.polygon_with_hole.as_text
   end
 
-  def test_polygon_with_hole
+  def test_collection
     geo = Geo.create(collection: 'GEOMETRYCOLLECTION(POINT(2 0),POLYGON((0 0, 1 0, 1 1, 0 1, 0 0)))')
     assert geo.collection.is_a?(RGeo::Geos::CAPIGeometryCollectionImpl)
     assert_equal "GEOMETRYCOLLECTION (POINT (2 0), POLYGON ((0 0, 1 0, 1 1, 0 1, 0 0)))", geo.collection.as_text

test/cases/helper.rb

@@ -3,7 +3,8 @@
 require "postgis_adapter"
 
 require "active_support"
-require "active_support/testing/autorun"
+require "active_support/test_case"
+require "minitest/autorun"
 
 ActiveRecord::Base.logger = ActiveSupport::Logger.new("debug.log", 0, 100 * 1024 * 1024)
 ActiveRecord::Base.configurations = ActiveRecord::DatabaseConfigurations.new({