Skip to content

Commit 7a04813

Browse files
imorlandimorland
committed
[2.x] fix(search): harden pgsql search configuration handling
Bind the `pgsql_search_configuration` setting via `?::regconfig` instead of interpolating it into the SQL string. The value is now treated as data; Postgres rejects invalid configuration names at the cast.
1 parent 44786de commit 7a04813

3 files changed

Lines changed: 125 additions & 15 deletions

File tree

‎framework/core/src/Discussion/Search/FulltextFilter.php‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -121,27 +121,29 @@ protected function pgsql(DatabaseSearchState $state, string $value): void
121121

122122
$grammar = $query->getGrammar();
123123

124-
$matchCondition = "to_tsvector('$searchConfig', ".$grammar->wrap('posts.content').") @@ plainto_tsquery('$searchConfig', ?)";
125-
$matchScore = "ts_rank(to_tsvector('$searchConfig', ".$grammar->wrap('posts.content')."), plainto_tsquery('$searchConfig', ?))";
126-
$matchTitleCondition = "to_tsvector('$searchConfig', ".$grammar->wrap('discussions.title').") @@ plainto_tsquery('$searchConfig', ?)";
127-
$matchTitleScore = "ts_rank(to_tsvector('$searchConfig', ".$grammar->wrap('discussions.title')."), plainto_tsquery('$searchConfig', ?))";
124+
$matchCondition = 'to_tsvector(?::regconfig, '.$grammar->wrap('posts.content').') @@ plainto_tsquery(?::regconfig, ?)';
125+
$matchScore = 'ts_rank(to_tsvector(?::regconfig, '.$grammar->wrap('posts.content').'), plainto_tsquery(?::regconfig, ?))';
126+
$matchTitleCondition = 'to_tsvector(?::regconfig, '.$grammar->wrap('discussions.title').') @@ plainto_tsquery(?::regconfig, ?)';
127+
$matchTitleScore = 'ts_rank(to_tsvector(?::regconfig, '.$grammar->wrap('discussions.title').'), plainto_tsquery(?::regconfig, ?))';
128128
$mostRelevantPostId = 'CAST(SPLIT_PART(STRING_AGG(CAST('.$grammar->wrap('posts.id')." AS VARCHAR), ',' ORDER BY ".$matchScore.' DESC, '.$grammar->wrap('posts.number')."), ',', 1) AS INTEGER) as most_relevant_post_id";
129129

130+
$matchBindings = [$searchConfig, $searchConfig, $value];
131+
130132
$discussionSubquery = Discussion::select('id')
131133
->selectRaw('NULL as score')
132134
->selectRaw('first_post_id as most_relevant_post_id')
133-
->whereRaw($matchTitleCondition, [$value]);
135+
->whereRaw($matchTitleCondition, $matchBindings);
134136

135137
// Construct a subquery to fetch discussions which contain relevant
136138
// posts. Retrieve the collective relevance of each discussion's posts,
137139
// which we will use later in the order by clause, and also retrieve
138140
// the ID of the most relevant post.
139141
$subquery = Post::whereVisibleTo($state->getActor())
140142
->select('posts.discussion_id')
141-
->selectRaw("SUM($matchScore) as score", [$value])
142-
->selectRaw($mostRelevantPostId, [$value])
143+
->selectRaw("SUM($matchScore) as score", $matchBindings)
144+
->selectRaw($mostRelevantPostId, $matchBindings)
143145
->where('posts.type', 'comment')
144-
->whereRaw($matchCondition, [$value])
146+
->whereRaw($matchCondition, $matchBindings)
145147
->groupBy('posts.discussion_id')
146148
->union($discussionSubquery);
147149

@@ -168,8 +170,8 @@ protected function pgsql(DatabaseSearchState $state, string $value): void
168170
->fromSub($query, 'discussions')
169171
);
170172

171-
$state->setDefaultSort(function (Builder $query) use ($value, $matchTitleScore) {
172-
$query->orderByRaw("$matchTitleScore desc", [$value]);
173+
$state->setDefaultSort(function (Builder $query) use ($matchBindings, $matchTitleScore) {
174+
$query->orderByRaw("$matchTitleScore desc", $matchBindings);
173175
$query->orderBy('discussions.score', 'desc');
174176
});
175177
}

‎framework/core/src/Post/Filter/FulltextFilter.php‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -70,13 +70,15 @@ protected function pgsql(DatabaseSearchState $state, string $value): void
7070

7171
$grammar = $query->getGrammar();
7272

73-
$matchCondition = "to_tsvector('$searchConfig', ".$grammar->wrap('posts.content').") @@ plainto_tsquery('$searchConfig', ?)";
74-
$matchScore = "ts_rank(to_tsvector('$searchConfig', ".$grammar->wrap('posts.content')."), plainto_tsquery('$searchConfig', ?))";
73+
$matchCondition = 'to_tsvector(?::regconfig, '.$grammar->wrap('posts.content').') @@ plainto_tsquery(?::regconfig, ?)';
74+
$matchScore = 'ts_rank(to_tsvector(?::regconfig, '.$grammar->wrap('posts.content').'), plainto_tsquery(?::regconfig, ?))';
7575

76-
$query->whereRaw($matchCondition, [$value]);
76+
$matchBindings = [$searchConfig, $searchConfig, $value];
7777

78-
$state->setDefaultSort(function (Builder $query) use ($value, $matchScore) {
79-
$query->orderByRaw($matchScore.' desc', [$value]);
78+
$query->whereRaw($matchCondition, $matchBindings);
79+
80+
$state->setDefaultSort(function (Builder $query) use ($matchBindings, $matchScore) {
81+
$query->orderByRaw($matchScore.' desc', $matchBindings);
8082
});
8183
}
8284
}

‎framework/core/tests/integration/api/discussions/PgsqlSearchConfigurationInjectionTest.php‎

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
<?php
2+
3+
/*
4+
* This file is part of Flarum.
5+
*
6+
* For detailed copyright and license information, please view the
7+
* LICENSE file that was distributed with this source code.
8+
*/
9+
10+
namespace Flarum\Tests\integration\api\discussions;
11+
12+
use Flarum\Discussion\Discussion;
13+
use Flarum\Post\Post;
14+
use Flarum\Testing\integration\RetrievesAuthorizedUsers;
15+
use Flarum\Testing\integration\TestCase;
16+
use PHPUnit\Framework\Attributes\Test;
17+
18+
class PgsqlSearchConfigurationInjectionTest extends TestCase
19+
{
20+
use RetrievesAuthorizedUsers;
21+
22+
protected function setUp(): void
23+
{
24+
parent::setUp();
25+
26+
if ($this->database()->getDriverName() !== 'pgsql') {
27+
$this->markTestSkipped('Postgres-specific filter path.');
28+
}
29+
30+
$this->database()->rollBack();
31+
32+
$this->database()->table('discussions')->insert($this->rowsThroughFactory(Discussion::class, [
33+
['id' => 1, 'title' => 'hello world', 'user_id' => 1],
34+
]));
35+
$this->database()->table('posts')->insert($this->rowsThroughFactory(Post::class, [
36+
['id' => 1, 'discussion_id' => 1, 'user_id' => 1, 'content' => '<t><p>hello world body</p></t>'],
37+
]));
38+
39+
$this->database()->beginTransaction();
40+
41+
$this->populateDatabase();
42+
}
43+
44+
protected function tearDown(): void
45+
{
46+
parent::tearDown();
47+
48+
$this->database()->table('discussions')->delete();
49+
$this->database()->table('posts')->delete();
50+
51+
// Reset the setting in case the post-condition assertion was hit before reset.
52+
$this->database()->table('settings')->where('key', 'pgsql_search_configuration')->update(['value' => 'english']);
53+
}
54+
55+
#[Test]
56+
public function baseline_default_search_config_returns_200(): void
57+
{
58+
// Sanity check: with the default config, a search succeeds.
59+
$response = $this->send(
60+
$this->request('GET', '/api/discussions')
61+
->withQueryParams(['filter' => ['q' => 'hello']])
62+
);
63+
64+
$this->assertEquals(200, $response->getStatusCode(), (string) $response->getBody());
65+
}
66+
67+
#[Test]
68+
public function malicious_search_config_is_not_interpolated_into_sql(): void
69+
{
70+
// Admin (or anyone who can write to the settings table) attempts to inject
71+
// SQL via the pgsql_search_configuration setting.
72+
$payload = "english') || pg_sleep(0) || ('";
73+
74+
/** @var \Flarum\Settings\SettingsRepositoryInterface $settings */
75+
$settings = $this->app()->getContainer()->make(\Flarum\Settings\SettingsRepositoryInterface::class);
76+
$settings->set('pgsql_search_configuration', $payload);
77+
78+
$response = $this->send(
79+
$this->request('GET', '/api/discussions')
80+
->withQueryParams(['filter' => ['q' => 'hello']])
81+
);
82+
83+
$body = (string) $response->getBody();
84+
85+
// Under the original vulnerability, the payload was concatenated into
86+
// SQL as code: Postgres parsed `tsvector || pg_sleep(...)` and raised
87+
// SQLSTATE 42883 "operator does not exist: tsvector || void".
88+
//
89+
// Under the fix, the payload is bound as a parameter and cast to
90+
// `regconfig`. Postgres rejects it with SQLSTATE 42602 "invalid name
91+
// syntax" — the value reached the DB as data, not code.
92+
//
93+
// We assert on the SQLSTATE, which is unambiguous: 42883 ↔ injection
94+
// open, 42602 ↔ injection closed.
95+
$this->assertStringNotContainsString(
96+
'42883',
97+
$body,
98+
"Postgres tried to evaluate the payload as SQL (operator-does-not-exist error). Injection still possible.\nResponse body: $body"
99+
);
100+
$this->assertStringContainsString(
101+
'42602',
102+
$body,
103+
"Expected Postgres to reject the payload at the regconfig cast (SQLSTATE 42602).\nResponse body: $body"
104+
);
105+
}
106+
}

0 commit comments

Comments
 (0)