Fix geometry type: perf, nil-limit crash, adapter pollution, and DDL hardening#6
Merged
Merged
Conversation
…hardening - Build the RGeo factory, WKB/WKT parsers, and WKB generator once in the Geometry type's initializer instead of rebuilding them on every cast/ serialize. A GEOS factory was previously allocated per value read; it is invariant per column. Eager construction (vs. lazy memoization) is also required because AR freezes Type instances after initialization. - Guard the nil-limit path: a bare `geometry` column (no type/srid modifier) has no limit, so `limit[:srid]` raised NoMethodError on read. Add a `srid` helper defaulting to 0 (PostGIS "unknown SRID"). - type_to_sql: coerce srid with `to_i` so it cannot carry arbitrary text into the geometry() type modifier. - Use `merge` instead of `merge!` for NATIVE_DATABASE_TYPES so the parent PostgreSQLAdapter's type map is not mutated with :geometry. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The test suite failed to load: ActiveSupport 8.1's active_support/testing/autorun now calls `Minitest.load :rails`, which requires railties' minitest/rails_plugin. This gem depends only on activerecord (not the full Rails), so railties is absent from the bundle and the require raised LoadError (surfaced by minitest 6 adding Minitest.load). Use plain minitest/autorun, the conventional choice for a non-Rails gem, instead of pulling in railties. Also rename the duplicate `test_polygon_with_hole` (the second definition silently shadowed the first) to `test_collection` so the geometry collection case runs as its own test. Add a CI workflow running the suite against a postgis/postgis service container on Ruby 3.2-3.4, installing libgeos-dev before bundle install so rgeo's CAPI extension compiles. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
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
Security & performance review of the PostGIS geometry type and adapter. Four fixes, ranked by impact.
1. Perf — RGeo factory rebuilt on every read
oid/geometry.rb—castallocated a fresh GEOS factory (andserialize/parsers) for every geometry value loaded from the DB, even though the SRID is fixed per column. The factory, WKB/WKT parsers, and WKB generator are now built once in the initializer. Eager construction (rather than lazy||=memoization) is also required because ActiveRecord freezesTypeinstances after initialization — lazy memoization raisedFrozenErrorin Rails 8.1 (verified).2. Correctness — bare
geometrycolumn crashed on readA column defined as plain
geometry(nogeometry(Type,srid)modifier) has nolimit, solimit[:srid]raisedNoMethodErroron read. Added asridhelper defaulting to0(PostGIS "unknown SRID").3. Bug —
merge!polluted the parent PostgreSQL adapterNATIVE_DATABASE_TYPES = PostgreSQLAdapter::NATIVE_DATABASE_TYPES.merge!(...)mutated the parent adapter's hash in place, leaking:geometryinto the vanillapostgresqladapter (and would raiseFrozenErrorif that constant were frozen). Changed to non-mutatingmerge.4. Security (low) — unvalidated interpolation into DDL
type_to_sqlinterpolatedsridstraight into thegeometry(...)type modifier. Now coerced withto_i, so it can't carry arbitrary SQL text. (typeis still interpolated; it's the developer-supplied geometry type name.)Verification
Exercised end-to-end against a real PostGIS database (all pass):
type_to_sql(:geometry, limit: { srid: "4326); DROP TABLE geos;--" })→geometry(Point,4326)Pointcreate/reload round-tripgeometrycolumn create/reload (nil-limit path, no crash)PostgreSQLAdapter::NATIVE_DATABASE_TYPESno longer contains:geometryNote: the
rake testtask currently fails to load in this environment due to a pre-existing incompatibility between minitest 6 and this ActiveSupport'sautorun(minitest/rails_pluginmissing) — unrelated to these changes; verification was done via a direct script exercising the same code paths.🤖 Generated with Claude Code