You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by mattyb149 <gi...@git.apache.org> on 2018/06/04 21:05:51 UTC
[GitHub] nifi pull request #2760: NIFI-5266: Sanitize ES parameters in PutElasticsear...
GitHub user mattyb149 opened a pull request:
https://github.com/apache/nifi/pull/2760
NIFI-5266: Sanitize ES parameters in PutElasticsearchHttp processors
I ran the PutESHttp integration tests as well (after adding new ones to test this behavior). Also fixed a couple of minor issues while I was in here, such as adding unit tests around Expression Language for the ES URL, fixing the result/reason error handling to return the right string, etc.
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [x] Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [x] Has your PR been rebased against the latest commit within the target branch (typically master)?
- [x] Is your initial contribution a single, squashed commit?
### For code changes:
- [x] Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
- [x] Have you written or updated unit tests to verify your changes?
- [x] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
- [ ] If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
- [ ] If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mattyb149/nifi NIFI-5266
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/2760.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2760
----
commit 5e5cc7e00092893c762e859a4b2ba305446347e7
Author: Matthew Burgess <ma...@...>
Date: 2018-06-04T21:04:01Z
NIFI-5266: Sanitize ES parameters in PutElasticsearchHttp processors
----
---
[GitHub] nifi pull request #2760: NIFI-5266: Sanitize ES parameters in PutElasticsear...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2760#discussion_r193132823
--- Diff: nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-processors/src/main/java/org/apache/nifi/processors/elasticsearch/PutElasticsearchHttp.java ---
@@ -288,42 +290,23 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
session.read(file, in -> {
json.append(IOUtils.toString(in, charset).replace("\r\n", " ").replace('\n', ' ').replace('\r', ' '));
});
- if (indexOp.equalsIgnoreCase("index")) {
- sb.append("{\"index\": { \"_index\": \"");
- sb.append(index);
- sb.append("\", \"_type\": \"");
- sb.append(docType);
- sb.append("\"");
- if (!StringUtils.isEmpty(id)) {
- sb.append(", \"_id\": \"");
- sb.append(id);
- sb.append("\"");
- }
- sb.append("}}\n");
- sb.append(json);
- sb.append("\n");
- } else if (indexOp.equalsIgnoreCase("upsert") || indexOp.equalsIgnoreCase("update")) {
- sb.append("{\"update\": { \"_index\": \"");
- sb.append(index);
- sb.append("\", \"_type\": \"");
- sb.append(docType);
- sb.append("\", \"_id\": \"");
- sb.append(id);
- sb.append("\" }\n");
- sb.append("{\"doc\": ");
- sb.append(json);
- sb.append(", \"doc_as_upsert\": ");
- sb.append(indexOp.equalsIgnoreCase("upsert"));
- sb.append(" }\n");
- } else if (indexOp.equalsIgnoreCase("delete")) {
- sb.append("{\"delete\": { \"_index\": \"");
- sb.append(index);
- sb.append("\", \"_type\": \"");
- sb.append(docType);
- sb.append("\", \"_id\": \"");
- sb.append(id);
- sb.append("\" }\n");
+
+ String jsonString = json.toString();
+
+ // Ensure the JSON body is well-formed
+ try {
--- End diff --
mapper should be static
---
[GitHub] nifi pull request #2760: NIFI-5266: Sanitize ES parameters in PutElasticsear...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2760#discussion_r193133140
--- Diff: nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-processors/src/main/java/org/apache/nifi/processors/elasticsearch/PutElasticsearchHttp.java ---
@@ -288,42 +290,23 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
session.read(file, in -> {
json.append(IOUtils.toString(in, charset).replace("\r\n", " ").replace('\n', ' ').replace('\r', ' '));
});
- if (indexOp.equalsIgnoreCase("index")) {
- sb.append("{\"index\": { \"_index\": \"");
- sb.append(index);
- sb.append("\", \"_type\": \"");
- sb.append(docType);
- sb.append("\"");
- if (!StringUtils.isEmpty(id)) {
- sb.append(", \"_id\": \"");
- sb.append(id);
- sb.append("\"");
- }
- sb.append("}}\n");
- sb.append(json);
- sb.append("\n");
- } else if (indexOp.equalsIgnoreCase("upsert") || indexOp.equalsIgnoreCase("update")) {
- sb.append("{\"update\": { \"_index\": \"");
- sb.append(index);
- sb.append("\", \"_type\": \"");
- sb.append(docType);
- sb.append("\", \"_id\": \"");
- sb.append(id);
- sb.append("\" }\n");
- sb.append("{\"doc\": ");
- sb.append(json);
- sb.append(", \"doc_as_upsert\": ");
- sb.append(indexOp.equalsIgnoreCase("upsert"));
- sb.append(" }\n");
- } else if (indexOp.equalsIgnoreCase("delete")) {
- sb.append("{\"delete\": { \"_index\": \"");
- sb.append(index);
- sb.append("\", \"_type\": \"");
- sb.append(docType);
- sb.append("\", \"_id\": \"");
- sb.append(id);
- sb.append("\" }\n");
+
+ String jsonString = json.toString();
+
+ // Ensure the JSON body is well-formed
+ try {
--- End diff --
I wonder if a generally available validate json wouldn't be better, and something the next person couldn't use.
---
[GitHub] nifi issue #2760: NIFI-5266: Sanitize ES parameters in PutElasticsearchHttp ...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/nifi/pull/2760
+1 (non-binding), contrib-check build looks good, tests updated.
---
[GitHub] nifi pull request #2760: NIFI-5266: Sanitize ES parameters in PutElasticsear...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2760#discussion_r193133584
--- Diff: nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-processors/src/main/java/org/apache/nifi/processors/elasticsearch/PutElasticsearchHttpRecord.java ---
@@ -261,7 +259,10 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
OkHttpClient okHttpClient = getClient();
final ComponentLog logger = getLogger();
- final String baseUrl = trimToEmpty(context.getProperty(ES_URL).evaluateAttributeExpressions().getValue());
+ final String baseUrl = context.getProperty(ES_URL).evaluateAttributeExpressions().getValue().trim();
+ if (StringUtils.isEmpty(baseUrl)) {
--- End diff --
Same as previous wrt baseUrl always being empty in the message
---
[GitHub] nifi issue #2760: NIFI-5266: Sanitize ES parameters in PutElasticsearchHttp ...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the issue:
https://github.com/apache/nifi/pull/2760
Reviewing...
---
[GitHub] nifi pull request #2760: NIFI-5266: Sanitize ES parameters in PutElasticsear...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2760#discussion_r193145047
--- Diff: nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-processors/src/main/java/org/apache/nifi/processors/elasticsearch/PutElasticsearchHttp.java ---
@@ -288,42 +290,23 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
session.read(file, in -> {
json.append(IOUtils.toString(in, charset).replace("\r\n", " ").replace('\n', ' ').replace('\r', ' '));
});
- if (indexOp.equalsIgnoreCase("index")) {
- sb.append("{\"index\": { \"_index\": \"");
- sb.append(index);
- sb.append("\", \"_type\": \"");
- sb.append(docType);
- sb.append("\"");
- if (!StringUtils.isEmpty(id)) {
- sb.append(", \"_id\": \"");
- sb.append(id);
- sb.append("\"");
- }
- sb.append("}}\n");
- sb.append(json);
- sb.append("\n");
- } else if (indexOp.equalsIgnoreCase("upsert") || indexOp.equalsIgnoreCase("update")) {
- sb.append("{\"update\": { \"_index\": \"");
- sb.append(index);
- sb.append("\", \"_type\": \"");
- sb.append(docType);
- sb.append("\", \"_id\": \"");
- sb.append(id);
- sb.append("\" }\n");
- sb.append("{\"doc\": ");
- sb.append(json);
- sb.append(", \"doc_as_upsert\": ");
- sb.append(indexOp.equalsIgnoreCase("upsert"));
- sb.append(" }\n");
- } else if (indexOp.equalsIgnoreCase("delete")) {
- sb.append("{\"delete\": { \"_index\": \"");
- sb.append(index);
- sb.append("\", \"_type\": \"");
- sb.append(docType);
- sb.append("\", \"_id\": \"");
- sb.append(id);
- sb.append("\" }\n");
+
+ String jsonString = json.toString();
+
+ // Ensure the JSON body is well-formed
+ try {
--- End diff --
will change to static. A generally available one would be good, just need to find the right place (like a utility JAR) and make sure it doesn't interfere with other bundles using it (like if they need their own Jackson or don't want all the Jackson dependencies when they resolve the utility JAR)
---
[GitHub] nifi pull request #2760: NIFI-5266: Sanitize ES parameters in PutElasticsear...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2760#discussion_r193144611
--- Diff: nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-processors/src/main/java/org/apache/nifi/processors/elasticsearch/PutElasticsearchHttp.java ---
@@ -227,7 +226,10 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
List<FlowFile> flowFilesToTransfer = new LinkedList<>(flowFiles);
final StringBuilder sb = new StringBuilder();
- final String baseUrl = trimToEmpty(context.getProperty(ES_URL).evaluateAttributeExpressions().getValue());
+ final String baseUrl = context.getProperty(ES_URL).evaluateAttributeExpressions().getValue().trim();
+ if (StringUtils.isEmpty(baseUrl)) {
--- End diff --
D'oh, good catch, I was doing it differently before and left in the useless error message :P Will change in both places.
---
[GitHub] nifi pull request #2760: NIFI-5266: Sanitize ES parameters in PutElasticsear...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2760#discussion_r193183815
--- Diff: nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-processors/src/test/java/org/apache/nifi/processors/elasticsearch/PutElasticsearchHttpRecordIT.java ---
@@ -227,6 +227,29 @@ public void testBadIndexName() throws Exception {
runner.assertTransferCount(PutElasticsearchHttpRecord.REL_SUCCESS, 0);
}
+ @Test
+ public void testIndexNameWithJsonChar() throws Exception {
+ // Undo some stuff from setup()
+ runner.setProperty(PutElasticsearchHttpRecord.INDEX, "people}test");
+ runner.setProperty(PutElasticsearchHttpRecord.TYPE, "person");
+ recordReader.addRecord(1, new MapRecord(personSchema, new HashMap<String,Object>() {{
+ put("name", "John Doe");
+ put("age", 48);
+ put("sport", null);
+ }}));
+
+ List<Map<String, String>> attrs = new ArrayList<>();
+ Map<String, String> attr = new HashMap<>();
+ attr.put("doc_id", "1");
+ attrs.add(attr);
+
+ runner.enqueue("");
+ runner.run(1, true, true);
+ runner.assertTransferCount(PutElasticsearchHttpRecord.REL_FAILURE, 0);
+ runner.assertTransferCount(PutElasticsearchHttpRecord.REL_RETRY, 0);
--- End diff --
Should this succeed or fail?
---
[GitHub] nifi pull request #2760: NIFI-5266: Sanitize ES parameters in PutElasticsear...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2760#discussion_r193189510
--- Diff: nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-processors/src/test/java/org/apache/nifi/processors/elasticsearch/PutElasticsearchHttpRecordIT.java ---
@@ -227,6 +227,29 @@ public void testBadIndexName() throws Exception {
runner.assertTransferCount(PutElasticsearchHttpRecord.REL_SUCCESS, 0);
}
+ @Test
+ public void testIndexNameWithJsonChar() throws Exception {
+ // Undo some stuff from setup()
+ runner.setProperty(PutElasticsearchHttpRecord.INDEX, "people}test");
+ runner.setProperty(PutElasticsearchHttpRecord.TYPE, "person");
+ recordReader.addRecord(1, new MapRecord(personSchema, new HashMap<String,Object>() {{
+ put("name", "John Doe");
+ put("age", 48);
+ put("sport", null);
+ }}));
+
+ List<Map<String, String>> attrs = new ArrayList<>();
+ Map<String, String> attr = new HashMap<>();
+ attr.put("doc_id", "1");
+ attrs.add(attr);
+
+ runner.enqueue("");
+ runner.run(1, true, true);
+ runner.assertTransferCount(PutElasticsearchHttpRecord.REL_FAILURE, 0);
+ runner.assertTransferCount(PutElasticsearchHttpRecord.REL_RETRY, 0);
--- End diff --
ah, ok sorry. Sometimes the intent isn't clear just looking at the test without a descriptive name or comment. Sorry
---
[GitHub] nifi pull request #2760: NIFI-5266: Sanitize ES parameters in PutElasticsear...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2760#discussion_r193186333
--- Diff: nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-processors/src/test/java/org/apache/nifi/processors/elasticsearch/PutElasticsearchHttpRecordIT.java ---
@@ -227,6 +227,29 @@ public void testBadIndexName() throws Exception {
runner.assertTransferCount(PutElasticsearchHttpRecord.REL_SUCCESS, 0);
}
+ @Test
+ public void testIndexNameWithJsonChar() throws Exception {
+ // Undo some stuff from setup()
+ runner.setProperty(PutElasticsearchHttpRecord.INDEX, "people}test");
+ runner.setProperty(PutElasticsearchHttpRecord.TYPE, "person");
+ recordReader.addRecord(1, new MapRecord(personSchema, new HashMap<String,Object>() {{
+ put("name", "John Doe");
+ put("age", 48);
+ put("sport", null);
+ }}));
+
+ List<Map<String, String>> attrs = new ArrayList<>();
+ Map<String, String> attr = new HashMap<>();
+ attr.put("doc_id", "1");
+ attrs.add(attr);
+
+ runner.enqueue("");
+ runner.run(1, true, true);
+ runner.assertTransferCount(PutElasticsearchHttpRecord.REL_FAILURE, 0);
+ runner.assertTransferCount(PutElasticsearchHttpRecord.REL_RETRY, 0);
--- End diff --
The error from Elasticsearch (as logged in testIllegalIndexName()) is:
`must not contain the following characters [ , ", *, \, <, |, ,, >, /, ?]`
According to that, `}` is a legitimate character in an index name, it is included here to show that a brace doesn't mess with the generated JSON body issued to the REST endpoint
---
[GitHub] nifi pull request #2760: NIFI-5266: Sanitize ES parameters in PutElasticsear...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/2760
---
[GitHub] nifi pull request #2760: NIFI-5266: Sanitize ES parameters in PutElasticsear...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2760#discussion_r193132253
--- Diff: nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-processors/src/main/java/org/apache/nifi/processors/elasticsearch/PutElasticsearchHttp.java ---
@@ -227,7 +226,10 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
List<FlowFile> flowFilesToTransfer = new LinkedList<>(flowFiles);
final StringBuilder sb = new StringBuilder();
- final String baseUrl = trimToEmpty(context.getProperty(ES_URL).evaluateAttributeExpressions().getValue());
+ final String baseUrl = context.getProperty(ES_URL).evaluateAttributeExpressions().getValue().trim();
+ if (StringUtils.isEmpty(baseUrl)) {
--- End diff --
if baseUrl is empty should the exception message be that "Elasticsearch URL evaluates to empty" or something? Your message is always going to be "... not valid: " since baseUrl will be empty.
---
[GitHub] nifi pull request #2760: NIFI-5266: Sanitize ES parameters in PutElasticsear...
Posted by zenfenan <gi...@git.apache.org>.
Github user zenfenan commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2760#discussion_r196073711
--- Diff: nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-processors/src/main/java/org/apache/nifi/processors/elasticsearch/PutElasticsearchHttp.java ---
@@ -288,42 +290,23 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
session.read(file, in -> {
json.append(IOUtils.toString(in, charset).replace("\r\n", " ").replace('\n', ' ').replace('\r', ' '));
});
- if (indexOp.equalsIgnoreCase("index")) {
- sb.append("{\"index\": { \"_index\": \"");
- sb.append(index);
- sb.append("\", \"_type\": \"");
- sb.append(docType);
- sb.append("\"");
- if (!StringUtils.isEmpty(id)) {
- sb.append(", \"_id\": \"");
- sb.append(id);
- sb.append("\"");
- }
- sb.append("}}\n");
- sb.append(json);
- sb.append("\n");
- } else if (indexOp.equalsIgnoreCase("upsert") || indexOp.equalsIgnoreCase("update")) {
- sb.append("{\"update\": { \"_index\": \"");
- sb.append(index);
- sb.append("\", \"_type\": \"");
- sb.append(docType);
- sb.append("\", \"_id\": \"");
- sb.append(id);
- sb.append("\" }\n");
- sb.append("{\"doc\": ");
- sb.append(json);
- sb.append(", \"doc_as_upsert\": ");
- sb.append(indexOp.equalsIgnoreCase("upsert"));
- sb.append(" }\n");
- } else if (indexOp.equalsIgnoreCase("delete")) {
- sb.append("{\"delete\": { \"_index\": \"");
- sb.append(index);
- sb.append("\", \"_type\": \"");
- sb.append(docType);
- sb.append("\", \"_id\": \"");
- sb.append(id);
- sb.append("\" }\n");
+
+ String jsonString = json.toString();
+
+ // Ensure the JSON body is well-formed
+ try {
--- End diff --
@ottobackwards @mattyb149 As part of NIFI-5261 and NIFI-5271 introduced a JSON validator. I'm taking this as an opportunity to share it with you guys and whoever takes a look at this comment. It could be of use here and in your future contributions.
---
[GitHub] nifi issue #2760: NIFI-5266: Sanitize ES parameters in PutElasticsearchHttp ...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the issue:
https://github.com/apache/nifi/pull/2760
Thanks @mattyb149 . I ran through this using the template you provided. Ran `contrib-check` and all tests pass. I squashed the commits. +1, merging.
---