fix: prevent query param override of URL-defined connection options#4162
fix: prevent query param override of URL-defined connection options#4162wellwelwel merged 1 commit intosidorares:masterfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4162 +/- ##
==========================================
+ Coverage 90.41% 90.47% +0.05%
==========================================
Files 86 86
Lines 13977 13980 +3
Branches 1728 1732 +4
==========================================
+ Hits 12638 12648 +10
+ Misses 1339 1332 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This PR also indirectly fixes the possibility of prototype pollution (manipulating the |
|
@wellwelwel perfect, will add it to the advisory and writeup when published, thanks for the heads up! In the meantime, there'a slight edge case that might also be worth looking at while we're already at it: The bounds checks before each read prevent the RangeError crash, but the loop count itself (num/numPoints/numRings) is still taken at face value from the buffer. For the multi-geometry types (cases 4–7), a payload with numGeometries = 0xFFFFFFFF and no actual geometry data causes a bunch of no-op iterations of parseGeometry() > each call hits the bounds check and returns null, but the loop still spins. I tested this against the canary build: Count Time (ms) A single malformed geometry response could freeze the event loop for more than half a minute. Same approach would work for numPoints in WKBLineString and numRings in WKBPolygon. Something like: // after reading num/numPoints/numRings: 9 bytes = minimum sub-geometry size (1 byte order + 4 wkbType + 4 count). For points it'd be /16 (two doubles), and for rings /4 (minimum ring header). Obviously minor compared to the other fixes, but same preconditions as the original finding (attacker-controlled server data). Just figured I'd flag it while the code is fresh :D |
Query parameters in
parseUrlcould override connection options already extracted from the URL structure (e.g.,host,port,user,password, anddatabase):Now, keys already present in the parsed options object are skipped during query parameter processing.
Dynamic options like
ssl,multipleStatements, etc. remain configurable via query parameters since they are not part of the URL structure and can/are commonly used in DSN strings, it also prevent a potential breaking change to this fix, since whoever controls the DSN, usually already controls the host and credentials.