SQL IndexerBolt interpolates metadata labels into the statement as unquoted identifiers

[rzo1]

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.

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.

/*
 * 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.