rzo1 opened a new issue, #2096:
URL: https://github.com/apache/stormcrawler/issues/2096
## What happens
`IndexerBolt.buildQuery()` builds the `INSERT ... ON DUPLICATE KEY UPDATE`
statement by concatenating the labels returned by `filterMetadata()` into the
column list and into the update clause. Values are bound as parameters, but the
identifiers are neither validated nor quoted. With an explicit alias mapping
the label is operator-chosen and fine. With a glob mapping such as
`indexer.md.mapping: ["parse.*"]` the label is the raw metadata key, and
metadata keys can come from parsed page content, for example the names Tika
copies out of `<meta name="...">` elements.
## Where
`external/sql/src/main/java/org/apache/stormcrawler/sql/IndexerBolt.java:172-201`,
config key `indexer.md.mapping`.
```java
final String columns = String.join(", ", keys);
...
.map(k -> String.format(Locale.ROOT, "%s=VALUES(%s)", k, k))
```
The labels come from `AbstractIndexerBolt.filterMetadata()`
(core/src/main/java/org/apache/stormcrawler/indexing/AbstractIndexerBolt.java:238-281),
which returns `matchingKey` unchanged for a glob entry.
## Why it matters
The practical outcome today is a broken statement rather than a working
injection: every key a glob mapping produces is dotted, for example
`parse.title`, and unquoted MySQL reads that as `table.column`, so the INSERT
already fails for ordinary pages. That makes the glob mapping unusable with
this bolt, and it makes the bolt fragile: `execute()` fails the tuple without
emitting to the status stream, so a tuple whose metadata key breaks the SQL is
replayed for as long as the topology runs. The interpolation itself is the
wrong shape for a value that can originate in crawled content, and should not
be left to depend on the accident of the label being dotted.
## Reproduction
Save as
`external/sql/src/test/java/org/apache/stormcrawler/sql/IndexerBoltQueryBuildingTest.java`.
```java
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to you under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.apache.stormcrawler.sql;
import static org.junit.jupiter.api.Assertions.assertFalse;
import java.lang.reflect.Method;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import org.apache.storm.task.OutputCollector;
import org.apache.stormcrawler.TestOutputCollector;
import org.apache.stormcrawler.TestUtil;
import org.apache.stormcrawler.indexing.AbstractIndexerBolt;
import org.junit.jupiter.api.Test;
/**
* The labels returned by filterMetadata() end up in the column list and in
the ON DUPLICATE KEY
* UPDATE clause of the generated statement. With a glob mapping the label
is the raw metadata key,
* which can come from parsed content. buildQuery() neither validates nor
quotes it.
*/
class IndexerBoltQueryBuildingTest {
private String buildQuery(List<String> keys) throws Exception {
IndexerBolt bolt = new IndexerBolt();
Map<String, Object> conf = new HashMap<>();
conf.put(IndexerBolt.SQL_INDEX_TABLE_PARAM_NAME, "content");
conf.put(AbstractIndexerBolt.urlFieldParamName, "url");
List<String> mdMapping = new ArrayList<>();
mdMapping.add("parse.*");
conf.put(AbstractIndexerBolt.metadata2fieldParamName, mdMapping);
bolt.prepare(
conf, TestUtil.getMockedTopologyContext(), new
OutputCollector(new TestOutputCollector()));
Method m = IndexerBolt.class.getDeclaredMethod("buildQuery",
List.class);
m.setAccessible(true);
return (String) m.invoke(bolt, keys);
}
/** A dotted key, which is what a glob mapping such as parse.* produces
on every ordinary page. */
@Test
void dottedLabelIsQuoted() throws Exception {
String query = buildQuery(List.of("parse.title"));
System.out.println(query);
assertFalse(
query.contains("parse.title") &&
!query.contains("`parse.title`"),
"dotted label must be quoted or rejected, otherwise MySQL
reads it as table.column");
}
/** A key containing SQL punctuation, which a parser can copy out of a
crawled page. */
@Test
void punctuatedLabelIsNotInterpolated() throws Exception {
String label = "a), (b";
String query = buildQuery(List.of(label));
System.out.println(query);
assertFalse(
query.contains(label),
"label with punctuation must not be interpolated verbatim
into the statement");
}
}
```
Run it:
```
mvn -pl external/sql test -Dtest=IndexerBoltQueryBuildingTest
```
It calls `buildQuery` by reflection, so it needs no database. It fails on
main and becomes the regression test after the fix.
```
INSERT INTO content (url, parse.title)
VALUES (?, ?)
ON DUPLICATE KEY UPDATE parse.title=VALUES(parse.title)
INSERT INTO content (url, a), (b)
VALUES (?, ?)
ON DUPLICATE KEY UPDATE a), (b=VALUES(a), (b)
[ERROR] Tests run: 2, Failures: 2, Errors: 0, Skipped: 0
[ERROR] IndexerBoltQueryBuildingTest.dottedLabelIsQuoted:60 dotted label
must be quoted or rejected, otherwise MySQL reads it as table.column ==>
expected: <false> but was: <true>
[ERROR] IndexerBoltQueryBuildingTest.punctuatedLabelIsNotInterpolated:71
label with punctuation must not be interpolated verbatim into the statement ==>
expected: <false> but was: <true>
```
## Suggested fix
In `IndexerBolt.buildQuery`, check every label against a strict identifier
pattern such as `^[A-Za-z0-9_]+$` and quote it with backticks before it goes
into the column list and the update clause. Drop labels that do not match, and
log them once per key rather than per tuple, so an awkward metadata name cannot
fail the tuple forever. Note that this changes behaviour for anyone relying on
a glob mapping: those labels are dotted and do not currently work, so dropping
them is a change from a permanent failure to a skipped column. If dotted labels
should be supported, map them to a column name explicitly, for instance by
replacing the dots, and document the mapping.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
