[gr8man]

Description

PostgreHandler::lockSession() and releaseLock() interpolated the session ID and the IP address directly into the advisory lock SQL. They now escape both values with $this->db->escape() before the statement is built, and still send it through simpleQuery().

This is defensive escaping; it is not a fix for a demonstrated exploitable path. In the stock framework flow, Session validates the session ID against $sidRegexp and RequestTrait validates the IP address with filter_var(..., FILTER_VALIDATE_IP), so neither value can break out of the string literal today.

What the change does address:

• PostgreHandler is the only database session handler that interpolates its lock arguments. MySQLiHandler hashes the argument with md5() first, which yields a clean hex string, while pg_advisory_lock() needs an integer, so PostgreSQL's hashtext() is applied to the raw value.
• A handler used directly, extended, or given a custom session ID format no longer produces a syntax error when the value contains a quote, and SAST tools no longer raise a false-positive CWE-89 finding.
• simpleQuery() is kept on purpose: query() would emit DBQuery events into the Debug Toolbar while the session lock is acquired and released, and the tests assert that no DBQuery event fires. The value stored in $this->lock, the lock lifecycle, and the return types are unchanged.

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value (without duplication)
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[michalsn]

I'm strongly against merging this in its current form. In the normal framework flow, the session cookie and IP address are validated before reaching these queries. What actual bug or exploitable path does this fix?

Switching from simpleQuery() to query() and changing the lock state representation introduce behavioral changes beyond escaping. Without a demonstrated problem, I don’t think that additional scope is justified.

[paulbalandan]

Please clarify the description and changelog as defensive escaping unless there is a demonstrated path for these values to reach the handler through normal session handling. The tests inject an invalid session ID and IP address directly; they do not establish an exploitable framework path.

[gr8man]

Thanks @paulbalandan and @michalsn for the feedback!

I agree with your assessment: in the standard framework HTTP lifecycle with stock Session.php, there is no exploitable vulnerability path, because $sessionID is strictly validated by $sidRegexp ([0-9a-v]{32}) in Session.php:113 and the IP address is validated via filter_var(..., FILTER_VALIDATE_IP) in RequestTrait.php:100.

To directly address the question of what actual issue this fixes, I have updated the PR description and changelog to frame this change purely as defensive escaping / hardening:

• Contrast with MySQLiHandler and the role of hashtext(): In MySQLiHandler::lockSession(), the lock argument is pre-hashed in PHP using md5(...), producing a clean hex string [0-9a-f]{32} where raw interpolation cannot break SQL syntax. In PostgreHandler, PostgreSQL's pg_advisory_lock() requires an integer/bigint rather than a string name, delegating hashing to the database via hashtext('{$sessionID}'). As a result, PostgreHandler was the only session handler in the framework interpolating unescaped parameters directly into SQL.
• Database-layer defense & syntax error prevention: As a standalone database handler, PostgreHandler should not assume that input is always pre-sanitized by Session.php. If a project extends PostgreHandler, uses custom session ID formats, or invokes the handler directly in testing, unescaped quotes break the query syntax (ERROR: syntax error at or near ...). Escaping via $this->db->escape() ensures valid SQL and eliminates false-positive CWE-89 findings in SAST tools (SonarQube, Semgrep).
• Consistency across DatabaseHandler: In DatabaseHandler, all other operations (read(), write(), destroy()) consistently use Query Builder methods ($builder->where('id', ...)), which bind and escape parameters. Only PostgreHandler::lockSession() bypassed parameter escaping.
• Performance & behavior preservation: Retained simpleQuery() with direct $this->db->escape() instead of query(). This avoids firing DBQuery events into the Debug Toolbar, avoids object allocations, and preserves the exact lock lifecycle and return types without behavioral changes.
• Changelog updated: Rephrased the entry in docs/changelogs/v4.7.7.md under Bugs Fixed to explicitly describe defensive escaping:

  PostgreHandler::lockSession() and releaseLock() now defensively escape session IDs and IP addresses using BaseConnection::escape().

[paulbalandan]

Please update the PR description too. It still says the queries use bindings, but the implementation uses escape() with simpleQuery(), which also preserves the existing DBQuery event behavior.

[michalsn]

Please move it into the "Changes" section.

Suggested change

	- **Session:** `PostgreHandler::lockSession()` and `releaseLock()` now defensively escape session IDs and IP addresses using `BaseConnection::escape()`.
	- **Session:** Added defensive escaping of session lock arguments in `PostgreHandler`, preserving the existing locking behavior.