[I] SQL IndexerBolt interpolates metadata labels into the statement as unquoted identifiers [stormcrawler]

["rzo1 (via GitHub)" <gi...@apache.org>]

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: dev-unsubscribe@stormcrawler.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] SQL IndexerBolt interpolates metadata labels into the statement as unquoted identifiers [stormcrawler]

["rzo1 (via GitHub)" <gi...@apache.org>]

rzo1 closed issue #2096: SQL IndexerBolt interpolates metadata labels into the statement as unquoted identifiers
URL: https://github.com/apache/stormcrawler/issues/2096


-- 
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: dev-unsubscribe@stormcrawler.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org