You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/10/30 14:37:10 UTC
[GitHub] [solr] janhoy opened a new pull request, #1151: SOLR-16155 DocumentBuilder should not include field values in error msgs
janhoy opened a new pull request, #1151:
URL: https://github.com/apache/solr/pull/1151
https://issues.apache.org/jira/browse/SOLR-16155
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-16155 DocumentBuilder should not include field values in error msgs [solr]
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy commented on PR #1151:
URL: https://github.com/apache/solr/pull/1151#issuecomment-5130673785
So I had claude take a shot at bringing this PR up to date and address some of the feedback both here and in the JIRA. Would be nice to complete a 6 years old PR :)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-16155 DocumentBuilder should not include field values in error msgs [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1151:
URL: https://github.com/apache/solr/pull/1151#discussion_r3682802466
##########
changelog/unreleased/solr16155.yml:
##########
@@ -0,0 +1,12 @@
+title: >
+ Document field values are no longer included in indexing error messages, since they may leak
+ sensitive data into logs and API responses. To aid debugging, enable TRACE level logging for
+ org.apache.solr.update.DocumentBuilder or org.apache.solr.handler.loader.JavabinLoader
Review Comment:
Seems like this deserves some docs in the Ref Guide? So that folks actually know how to use it!
##########
solr/core/src/java/org/apache/solr/update/DocumentBuilder.java:
##########
@@ -163,21 +168,20 @@ public static Document toDocument(
&& field.getValueCount() > 1
&& !(sfield.getType() instanceof DenseVectorField)) {
- // Ensure we do not flood the logs with extremely long values
- String fieldValue = field.getValue().toString();
- if (fieldValue.length() > MAX_VALUES_AS_STRING_LENGTH) {
- assert fieldValue.endsWith("]");
- fieldValue = fieldValue.substring(0, MAX_VALUES_AS_STRING_LENGTH - 4) + "...]";
+ if (log.isTraceEnabled()) {
+ log.trace(
+ "{}multiple values encountered for non multiValued field {}: {}",
Review Comment:
is this `{}` quite right? I think we are getting an id, so the opening sentence on line 173 seems like it needs to tweak?
##########
solr/core/src/java/org/apache/solr/update/DocumentBuilder.java:
##########
@@ -363,6 +369,27 @@ private static boolean addCopyFields(
return used;
}
+ private static void traceFieldError(
+ SolrInputDocument doc, IndexSchema schema, SolrInputField field, Exception ex) {
+ if (log.isTraceEnabled()) {
+ log.trace(
+ "{}Error adding field '{}'='{}'",
+ getID(doc, schema),
+ field.getName(),
+ truncatedValue(field.getValue()),
+ ex);
+ }
+ }
+
+ /** Renders a field value as a string, truncated to avoid flooding the logs */
+ private static String truncatedValue(Object value) {
Review Comment:
likewise surprised this isn't in many places a need?
##########
solr/core/src/test/org/apache/solr/schema/DenseVectorFieldTest.java:
##########
@@ -718,7 +718,7 @@ public void denseVectorByteEncoding_shouldRaiseExceptionWithValuesOutsideBoundar
assertThat(
thrown.getCause().getMessage(),
is(
- "Error while creating field 'vector_byte_encoding{type=knn_vector_byte_encoding,properties=indexed,stored,omitNorms,omitTermFreqAndPositions,useDocValuesAsStored}' from value '[128, 6, 7, 8]'"));
+ "Error while creating field 'vector_byte_encoding{type=knn_vector_byte_encoding,properties=indexed,stored,omitNorms,omitTermFreqAndPositions,useDocValuesAsStored}'"));
Review Comment:
I will put a more general comment on the thrust of this whole feature, but while this is good, it also makes things harder to read too when you are going through the code...
##########
solr/core/src/java/org/apache/solr/update/DocumentBuilder.java:
##########
@@ -363,6 +369,27 @@ private static boolean addCopyFields(
return used;
}
+ private static void traceFieldError(
Review Comment:
I wonder if this method needs to be promoted to being a widely used method....? Centralize this tracing capability?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-16155 DocumentBuilder should not include field values in error msgs [solr]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #1151:
URL: https://github.com/apache/solr/pull/1151#issuecomment-5901555128
This PR has had no activity for 60 days and is now labeled as stale. Any new activity will remove the stale label. To attract more reviewers, please tag people who might be familiar with the code area and/or notify the dev@solr.apache.org mailing list. To exempt this PR from being marked as stale, make it a draft PR or add the label "exempt-stale". If left unattended, this PR will be closed after another 60 days of inactivity. Thank you for your contribution!
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1151: SOLR-16155 DocumentBuilder should not include field values in error msgs
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1151:
URL: https://github.com/apache/solr/pull/1151#discussion_r1024939636
##########
solr/core/src/test/org/apache/solr/update/processor/TolerantUpdateProcessorTest.java:
##########
@@ -260,8 +259,7 @@ public void testMaxErrors0() {
"tolerant-chain-max-errors-10", smallBatch, requestParams, "1"));
assertTrue(
e.getMessage()
- .contains(
- "ERROR: [doc=1] Error adding field 'weight'='b' msg=For input string: \"b\""));
Review Comment:
Yea, you're right, still leaks the value from the exception message. The "For input string" text is from `NumberFormatException`, probably in `Integer.parseInt()` when some class somewhere parses a weight field. Question is whether we could instead print something like
> Error adding fidle 'weight'. Exception: NumberFormatException
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1151: SOLR-16155 DocumentBuilder should not include field values in error msgs
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1151:
URL: https://github.com/apache/solr/pull/1151#discussion_r1024939636
##########
solr/core/src/test/org/apache/solr/update/processor/TolerantUpdateProcessorTest.java:
##########
@@ -260,8 +259,7 @@ public void testMaxErrors0() {
"tolerant-chain-max-errors-10", smallBatch, requestParams, "1"));
assertTrue(
e.getMessage()
- .contains(
- "ERROR: [doc=1] Error adding field 'weight'='b' msg=For input string: \"b\""));
Review Comment:
Yea, you're right, still leaks the value from the exception message. The "For input string" text is from `NumberFormatException`, probably in `Integer.parseInt()` when some class somewhere parses a weight field. Question is whether we could instead print something like
> Error adding field 'weight'. Exception: NumberFormatException
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1151: SOLR-16155 DocumentBuilder should not include field values in error msgs
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1151:
URL: https://github.com/apache/solr/pull/1151#discussion_r1008915536
##########
solr/CHANGES.txt:
##########
@@ -37,6 +37,11 @@ New Features
* SOLR-16500: Added --enable-preview for JDK 19 to enable MemorySegment support in MMapDirectory (Kevin Risden)
+Bug Fixes
Review Comment:
Thanks :)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-16155 DocumentBuilder should not include field values in error msgs [solr]
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy commented on PR #1151:
URL: https://github.com/apache/solr/pull/1151#issuecomment-5141738466
Good feedback.
* Perhaps `DEBUG` level is sufficient here. But I thought it is not uncommon to put an entire solr node in DEBUG mode for some extended time during any debugging, and that would then lead to potential unintended leaks to logs. So having to enable TRACE on the targeted classes (if documented) is safer
* Could be that there should be some system-wide utility except for log4j config to control or unify disclosure of index content outside of a search. Perhaps a dedicated INFOSTREAM like log that gives insight into the ingest pipeline? But that's out of scope for this JIRA
Next action on this could be
A) Abandon, no need leave as is
B) Use DEBUG level, document it, ship it. Simplest thing without new architectural constructs.
C) Introduce mechanism X cross the board
I agree that fixing this in the simplest way now gives no guarantee that a developer adds new error logging that leaks data tomorrow. But that is kind of hard to guard against even if there exists some common class or utility, if it is not used. Kind of like our allowLists...
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1151: SOLR-16155 DocumentBuilder should not include field values in error msgs
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1151:
URL: https://github.com/apache/solr/pull/1151#discussion_r1008878276
##########
solr/CHANGES.txt:
##########
@@ -37,6 +37,11 @@ New Features
* SOLR-16500: Added --enable-preview for JDK 19 to enable MemorySegment support in MMapDirectory (Kevin Risden)
+Bug Fixes
Review Comment:
There is already a section for bug fixes
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-16155 DocumentBuilder should not include field values in error msgs [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1151:
URL: https://github.com/apache/solr/pull/1151#issuecomment-5143294095
I'd vote for B... It's a step forward, and progress, not perfection. I think the point about dev/test/prod is an interesting one as well, the filtering logic get more interesting if you get them by default in prod and disable in dev for example ;-)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-16155 DocumentBuilder should not include field values in error msgs [solr]
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy commented on PR #1151:
URL: https://github.com/apache/solr/pull/1151#issuecomment-1888921332
Test of auto JIRA links feature: SOLR-16155
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1151: SOLR-16155 DocumentBuilder should not include field values in error msgs
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1151:
URL: https://github.com/apache/solr/pull/1151#discussion_r1020283731
##########
solr/core/src/test/org/apache/solr/update/processor/TolerantUpdateProcessorTest.java:
##########
@@ -260,8 +259,7 @@ public void testMaxErrors0() {
"tolerant-chain-max-errors-10", smallBatch, requestParams, "1"));
assertTrue(
e.getMessage()
- .contains(
- "ERROR: [doc=1] Error adding field 'weight'='b' msg=For input string: \"b\""));
Review Comment:
In `For input string: \"b\""` - "b" is still the value? At least it looks like it was in `'weight'='b'`
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1151: SOLR-16155 DocumentBuilder should not include field values in error msgs
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1151:
URL: https://github.com/apache/solr/pull/1151#discussion_r1020283731
##########
solr/core/src/test/org/apache/solr/update/processor/TolerantUpdateProcessorTest.java:
##########
@@ -260,8 +259,7 @@ public void testMaxErrors0() {
"tolerant-chain-max-errors-10", smallBatch, requestParams, "1"));
assertTrue(
e.getMessage()
- .contains(
- "ERROR: [doc=1] Error adding field 'weight'='b' msg=For input string: \"b\""));
Review Comment:
In `Isn't `For input string: \"b\""` - "b" is still the value? At least it looks like it was in `'weight'='b'`
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-16155 DocumentBuilder should not include field values in error msgs [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1151:
URL: https://github.com/apache/solr/pull/1151#issuecomment-5131084974
IMO "debug" level is more appropriate than "trace".
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-16155 DocumentBuilder should not include field values in error msgs [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1151:
URL: https://github.com/apache/solr/pull/1151#issuecomment-5143036556
> But I thought it is not uncommon to put an entire solr node in DEBUG mode for some extended time during any debugging,
I think it's quite reasonable to "leak", as you say, data values Solr is rejecting at debug level. It's *debug* after all -- I can't imagine a user complaining of such.
If one day we have a production|develop|test|... mode switch for Solr then I would love to see us find a way to make the logging noise conditional on that.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org