You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2020/06/11 23:35:20 UTC
[GitHub] [lucene-solr] janhoy opened a new pull request #1572: SOLR-14561 CoreAdminAPI's parameters instanceDir and dataDir are now validated
janhoy opened a new pull request #1572:
URL: https://github.com/apache/lucene-solr/pull/1572
See https://issues.apache.org/jira/browse/SOLR-14561
The `instanceDir` and `dataDir` params must now be relative to either `SOLR_HOME`, `SOLR_DATA_HOME` or `coreRootDir`.
Added new solr.xml config 'allowPaths', controlled by system property 'solr.allowPaths' that allows you to add other allowed paths when needed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] janhoy commented on a change in pull request #1572: SOLR-14561 CoreAdminAPI's parameters instanceDir and dataDir are now validated
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #1572:
URL: https://github.com/apache/lucene-solr/pull/1572#discussion_r441856227
##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -1259,6 +1277,28 @@ public SolrCore create(String coreName, Path instancePath, Map<String, String> p
}
}
+ /**
+ * Checks that the given path is relative to SOLR_HOME, SOLR_DATA_HOME, coreRootDirectory or one of the paths
+ * specified in solr.xml's allowPaths element. Delegates to {@link SolrPaths#assertPathAllowed(Path, Set)}
+ * @param pathToAssert path to check
+ * @throws SolrException if path is outside allowed paths
+ */
+ public void assertPathAllowed(Path pathToAssert) throws SolrException {
+ SolrPaths.assertPathAllowed(pathToAssert, allowPaths);
+ }
+
+ /**
+ * <p>Return the file system paths that should be allowed for various API requests.
Review comment:
@dsmiley see JavaDoc. I was hoping to keep this method private. Don't we have a special annotation that will allow access from test scope even if the method is not public?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] janhoy commented on a change in pull request #1572: SOLR-14561 CoreAdminAPI's parameters instanceDir and dataDir are now validated
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #1572:
URL: https://github.com/apache/lucene-solr/pull/1572#discussion_r441858025
##########
File path: solr/core/src/java/org/apache/solr/core/SolrPaths.java
##########
@@ -128,4 +130,35 @@ private static void logOnceInfo(String key, String msg) {
log.info(msg);
}
}
+
+ /**
+ * Checks that the given path is relative to SOLR_HOME, SOLR_DATA_HOME, coreRootDirectory or one of the paths
+ * specified in solr.xml's allowPaths element. The following paths will fail validation
+ * <ul>
+ * <li>Relative paths starting with <code>..</code></li>
+ * <li>Windows UNC paths (<code>\\host\share\path</code>)</li>
+ * <li>Absolute paths which are not below the list of allowed paths</li>
+ * </ul>
+ * @param pathToAssert path to check
+ * @param allowPaths list of paths that should be allowed prefixes
+ * @throws SolrException if path is outside allowed paths
+ */
+ public static void assertPathAllowed(Path pathToAssert, Set<Path> allowPaths) throws SolrException {
+ if (OS.isFamilyWindows() && pathToAssert.toString().startsWith("\\\\")) {
+ throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
+ "Path " + pathToAssert + " disallowed. UNC paths not supported. Please use drive letter instead.");
+ }
+ // Conversion Path -> String -> Path is to be able to compare against org.apache.lucene.mockfile.FilterPath instances
+ final Path path = Path.of(pathToAssert.toString()).normalize();
+ if (path.startsWith("..")) {
+ throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
+ "Path " + pathToAssert + " disallowed due to path traversal..");
+ }
+ if (!path.isAbsolute()) return; // All relative paths are accepted
+ if (allowPaths.contains(Paths.get("_ALL_"))) return; // Catch-all path "*"/"_ALL_" will allow all other paths
Review comment:
This is the workaround I did after realizing that Windows `Path` class is not happy with `*` as a path. When parsing the value from solr.xml/sysprop, we detect `*` and store it as a Path `_ALL_`. Then in the assert method we check for that special path and skip further testing.
Exception is UNC paths and `..` paths which are still rejected (should they?)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] janhoy edited a comment on pull request #1572: SOLR-14561 CoreAdminAPI's parameters instanceDir and dataDir are now validated
Posted by GitBox <gi...@apache.org>.
janhoy edited a comment on pull request #1572:
URL: https://github.com/apache/lucene-solr/pull/1572#issuecomment-644006967
Ran the whole test suite and uncovered various tests that use "illegal" temp test folders, that now fail. That was expected. So the last commit ba0b544 addresses these tests:
Give a way to whitelist all paths by setting `-Dsolr.allowPaths=*`
Add a `CoreContainer.getAllowPaths()` method that tests use to allow individual folders (I like that better than letting tests set global sysprops)
This also led to a small change in the path comparison - we now convert Path -> String -> Path to make sure paths are comparable, even Lucene's `FilterPath` class used in tests
To review, the easiest is probably to just load the last commit ba0b544
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] janhoy commented on a change in pull request #1572: SOLR-14561 CoreAdminAPI's parameters instanceDir and dataDir are now validated
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #1572:
URL: https://github.com/apache/lucene-solr/pull/1572#discussion_r439884150
##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -338,6 +339,19 @@ public CoreContainer(NodeConfig config, CoresLocator locator, boolean asyncSolrC
ExecutorUtil.newMDCAwareCachedThreadPool(
cfg.getReplayUpdatesThreads(),
new SolrNamedThreadFactory("replayUpdatesExecutor")));
+
+ this.allowPaths = new java.util.HashSet<>();
+ this.allowPaths.add(cfg.getSolrHome().toAbsolutePath());
Review comment:
Ok, simplified it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] janhoy commented on a change in pull request #1572: SOLR-14561 CoreAdminAPI's parameters instanceDir and dataDir are now validated
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #1572:
URL: https://github.com/apache/lucene-solr/pull/1572#discussion_r441854963
##########
File path: solr/core/src/java/org/apache/solr/core/SolrPaths.java
##########
@@ -128,4 +130,33 @@ private static void logOnceInfo(String key, String msg) {
log.info(msg);
}
}
+
+ /**
+ * Checks that the given path is relative to SOLR_HOME, SOLR_DATA_HOME, coreRootDirectory or one of the paths
+ * specified in solr.xml's allowPaths element. The following paths will fail validation
+ * <ul>
+ * <li>Relative paths starting with <code>..</code></li>
+ * <li>Windows UNC paths (<code>\\host\share\path</code>)</li>
+ * <li>Absolute paths which are not below the list of allowed paths</li>
+ * </ul>
+ * @param pathToAssert path to check
+ * @param allowPaths list of paths that should be allowed prefixes
+ * @throws SolrException if path is outside allowed paths
+ */
+ public static void assertPathAllowed(Path pathToAssert, Set<Path> allowPaths) throws SolrException {
+ if (OS.isFamilyWindows() && pathToAssert.toString().startsWith("\\\\")) {
Review comment:
I have tested on Windows, validated that UNC is blocked, and modified the tests with separate ones running in Windows and non-Windows environments.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1572: SOLR-14561 CoreAdminAPI's parameters instanceDir and dataDir are now validated
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1572:
URL: https://github.com/apache/lucene-solr/pull/1572#discussion_r441858120
##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -1259,6 +1277,28 @@ public SolrCore create(String coreName, Path instancePath, Map<String, String> p
}
}
+ /**
+ * Checks that the given path is relative to SOLR_HOME, SOLR_DATA_HOME, coreRootDirectory or one of the paths
+ * specified in solr.xml's allowPaths element. Delegates to {@link SolrPaths#assertPathAllowed(Path, Set)}
+ * @param pathToAssert path to check
+ * @throws SolrException if path is outside allowed paths
+ */
+ public void assertPathAllowed(Path pathToAssert) throws SolrException {
+ SolrPaths.assertPathAllowed(pathToAssert, allowPaths);
+ }
+
+ /**
+ * <p>Return the file system paths that should be allowed for various API requests.
Review comment:
Javadoc is good; thanks.
`com.google.common.annotations.VisibleForTesting`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] janhoy merged pull request #1572: SOLR-14561 CoreAdminAPI's parameters instanceDir and dataDir are now validated
Posted by GitBox <gi...@apache.org>.
janhoy merged pull request #1572:
URL: https://github.com/apache/lucene-solr/pull/1572
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] janhoy commented on pull request #1572: SOLR-14561 CoreAdminAPI's parameters instanceDir and dataDir are now validated
Posted by GitBox <gi...@apache.org>.
janhoy commented on pull request #1572:
URL: https://github.com/apache/lucene-solr/pull/1572#issuecomment-645645529
All tests now passing on macOS and hopefully Windows (running tests now in a slow VirtualBox).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] janhoy commented on a change in pull request #1572: SOLR-14561 CoreAdminAPI's parameters instanceDir and dataDir are now validated
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #1572:
URL: https://github.com/apache/lucene-solr/pull/1572#discussion_r439772292
##########
File path: solr/core/src/java/org/apache/solr/core/SolrPaths.java
##########
@@ -128,4 +130,33 @@ private static void logOnceInfo(String key, String msg) {
log.info(msg);
}
}
+
+ /**
+ * Checks that the given path is relative to SOLR_HOME, SOLR_DATA_HOME, coreRootDirectory or one of the paths
+ * specified in solr.xml's allowPaths element. The following paths will fail validation
+ * <ul>
+ * <li>Relative paths starting with <code>..</code></li>
+ * <li>Windows UNC paths (<code>\\host\share\path</code>)</li>
+ * <li>Absolute paths which are not below the list of allowed paths</li>
+ * </ul>
+ * @param pathToAssert path to check
+ * @param allowPaths list of paths that should be allowed prefixes
+ * @throws SolrException if path is outside allowed paths
+ */
+ public static void assertPathAllowed(Path pathToAssert, Set<Path> allowPaths) throws SolrException {
+ if (OS.isFamilyWindows() && pathToAssert.toString().startsWith("\\\\")) {
Review comment:
I have not tested this on Windows. On my mac, the `Path` class uses an OSX implementation so I think it will not detect the UNC style path, it does not manage to normalize or make it absolute, so I scoped the check for Windows only. I test on the string version before normalizing since normalize may mess up UNC paths.
I decided to block UNC totally instead of trying to be smart about it. Users can always map a drive letter to the desired share to work around it?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] janhoy commented on a change in pull request #1572: SOLR-14561 CoreAdminAPI's parameters instanceDir and dataDir are now validated
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #1572:
URL: https://github.com/apache/lucene-solr/pull/1572#discussion_r439730789
##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -1259,6 +1277,20 @@ public SolrCore create(String coreName, Path instancePath, Map<String, String> p
}
}
+ /**
+ * Checks that the given path is relative to SOLR_HOME, SOLR_DATA_HOME, coreRootDirectory or one of the paths
+ * specified in solr.xml's allowPaths element.
+ * @param path path to check
+ * @throws SolrException if path is outside allowed paths
+ */
+ public void assertPathAllowed(Path path) throws SolrException {
+ if (path.normalize().equals(path) && !path.isAbsolute()) return;
Review comment:
You are right. We need a more thorough check
* Disallow relative paths starting with "."
* Always `normalize()` the path before `toAbsolutePath()` to catch the `/var/solr/../../etc` case, else that example would return true for `startsWith("/var/solr")`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] janhoy commented on a change in pull request #1572: SOLR-14561 CoreAdminAPI's parameters instanceDir and dataDir are now validated
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #1572:
URL: https://github.com/apache/lucene-solr/pull/1572#discussion_r439772139
##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -1259,6 +1277,20 @@ public SolrCore create(String coreName, Path instancePath, Map<String, String> p
}
}
+ /**
+ * Checks that the given path is relative to SOLR_HOME, SOLR_DATA_HOME, coreRootDirectory or one of the paths
+ * specified in solr.xml's allowPaths element.
+ * @param path path to check
+ * @throws SolrException if path is outside allowed paths
+ */
+ public void assertPathAllowed(Path path) throws SolrException {
+ if (path.normalize().equals(path) && !path.isAbsolute()) return;
Review comment:
I pushed a commit addressing this. Quite a few changes, but we now detect `..` specifically.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] tflobbe commented on a change in pull request #1572: SOLR-14561 CoreAdminAPI's parameters instanceDir and dataDir are now validated
Posted by GitBox <gi...@apache.org>.
tflobbe commented on a change in pull request #1572:
URL: https://github.com/apache/lucene-solr/pull/1572#discussion_r439686403
##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -1259,6 +1277,20 @@ public SolrCore create(String coreName, Path instancePath, Map<String, String> p
}
}
+ /**
+ * Checks that the given path is relative to SOLR_HOME, SOLR_DATA_HOME, coreRootDirectory or one of the paths
+ * specified in solr.xml's allowPaths element.
+ * @param path path to check
+ * @throws SolrException if path is outside allowed paths
+ */
+ public void assertPathAllowed(Path path) throws SolrException {
+ if (path.normalize().equals(path) && !path.isAbsolute()) return;
Review comment:
This doesn't cover the case of a `../foo` path, right? is that covered somewhere else?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1572: SOLR-14561 CoreAdminAPI's parameters instanceDir and dataDir are now validated
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1572:
URL: https://github.com/apache/lucene-solr/pull/1572#discussion_r439789761
##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -338,6 +339,19 @@ public CoreContainer(NodeConfig config, CoresLocator locator, boolean asyncSolrC
ExecutorUtil.newMDCAwareCachedThreadPool(
cfg.getReplayUpdatesThreads(),
new SolrNamedThreadFactory("replayUpdatesExecutor")));
+
+ this.allowPaths = new java.util.HashSet<>();
+ this.allowPaths.add(cfg.getSolrHome().toAbsolutePath());
Review comment:
paths in cfg are already absolute -- thanks to me recently :-)
##########
File path: solr/core/src/test-files/solr/solr-50-all.xml
##########
@@ -24,6 +24,7 @@
<str name="configSetsHandler">testConfigSetsHandler</str>
<str name="managementPath">testManagementPath</str>
<str name="sharedLib">testSharedLib</str>
+ <str name="allowPaths">/tmp,/home/john</str>
Review comment:
Since this file is a NamedList, shouldn't we be using `<arr` for an array of strings?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] janhoy commented on pull request #1572: SOLR-14561 CoreAdminAPI's parameters instanceDir and dataDir are now validated
Posted by GitBox <gi...@apache.org>.
janhoy commented on pull request #1572:
URL: https://github.com/apache/lucene-solr/pull/1572#issuecomment-645245313
The tests still don't pass on Windows, and I have found the reason. Will push a few more changes on friday.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] janhoy commented on a change in pull request #1572: SOLR-14561 CoreAdminAPI's parameters instanceDir and dataDir are now validated
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #1572:
URL: https://github.com/apache/lucene-solr/pull/1572#discussion_r439883240
##########
File path: solr/core/src/test-files/solr/solr-50-all.xml
##########
@@ -24,6 +24,7 @@
<str name="configSetsHandler">testConfigSetsHandler</str>
<str name="managementPath">testManagementPath</str>
<str name="sharedLib">testSharedLib</str>
+ <str name="allowPaths">/tmp,/home/john</str>
Review comment:
It could, but to make it convenient to set it as a Java Sysprop, I kept it as a simple string. De default `solr.xml` defines it as `${solr.allowPaths:}` so that `-Dsolr.allowPaths=foo,bar,baz` will work. I'm not aware of any way to specify a native array as Java Sysprop?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] janhoy commented on pull request #1572: SOLR-14561 CoreAdminAPI's parameters instanceDir and dataDir are now validated
Posted by GitBox <gi...@apache.org>.
janhoy commented on pull request #1572:
URL: https://github.com/apache/lucene-solr/pull/1572#issuecomment-645659284
I think this is approaching committable state. Appreciate if someone with a good Windows box would run the full test suite on Windows. But I think I'll anyway merge to master and let Jenkins work on it for a few rounds. Then I'll backport to 8.x branch in good time before 8.6 branch cut.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on pull request #1572: SOLR-14561 CoreAdminAPI's parameters instanceDir and dataDir are now validated
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #1572:
URL: https://github.com/apache/lucene-solr/pull/1572#issuecomment-645742371
@mkhludnev ? I recall you use Windows.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] janhoy commented on pull request #1572: SOLR-14561 CoreAdminAPI's parameters instanceDir and dataDir are now validated
Posted by GitBox <gi...@apache.org>.
janhoy commented on pull request #1572:
URL: https://github.com/apache/lucene-solr/pull/1572#issuecomment-642990676
I think the only thing I'm lacking is a real integration test. I validated manually that core creation fails in `/tmp`, and that setting `-Dsolr.allowPaths=/tmp` allows it:
<img width="1173" alt="allowPath" src="https://user-images.githubusercontent.com/409128/84450542-b9a23d00-ac50-11ea-9ff1-253f9139685e.png">
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] janhoy commented on pull request #1572: SOLR-14561 CoreAdminAPI's parameters instanceDir and dataDir are now validated
Posted by GitBox <gi...@apache.org>.
janhoy commented on pull request #1572:
URL: https://github.com/apache/lucene-solr/pull/1572#issuecomment-644006967
Ran the whole test suite and uncovered various tests that use "illegal" temp test folders, that now fail. That was expected. So the last commit ba0b544 addresses these tests:
* Give a way to whitelist all paths by setting `-Dsolr.allowPaths=*`
* Add a `CoreContainer.getAllowPaths()` method that tests use to allow individual folders (I like that better than letting tests set global sysprops)
* This also led to a small change in the path comparison - we now convert Path -> String -> Path to make sure paths are comparable, even Lucene's `FilterPath` class used in tests
To review, the easiest is probably to just load the last commit ba0b544
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1572: SOLR-14561 CoreAdminAPI's parameters instanceDir and dataDir are now validated
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1572:
URL: https://github.com/apache/lucene-solr/pull/1572#discussion_r439936737
##########
File path: solr/core/src/test-files/solr/solr-50-all.xml
##########
@@ -24,6 +24,7 @@
<str name="configSetsHandler">testConfigSetsHandler</str>
<str name="managementPath">testManagementPath</str>
<str name="sharedLib">testSharedLib</str>
+ <str name="allowPaths">/tmp,/home/john</str>
Review comment:
ok... I suppose both could be supported provided that the consumer checked with the aid of some new utility method that doesn't exist like `Utils.toListOfString(Object, char separator) : String` but that might be a bit much. I suspect we have other places where we trivially use a delimited string and fail to leverage NamedList. Your call; no strong opinion here.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] janhoy commented on a change in pull request #1572: SOLR-14561 CoreAdminAPI's parameters instanceDir and dataDir are now validated
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #1572:
URL: https://github.com/apache/lucene-solr/pull/1572#discussion_r440028825
##########
File path: solr/core/src/java/org/apache/solr/core/SolrPaths.java
##########
@@ -128,4 +130,33 @@ private static void logOnceInfo(String key, String msg) {
log.info(msg);
}
}
+
+ /**
+ * Checks that the given path is relative to SOLR_HOME, SOLR_DATA_HOME, coreRootDirectory or one of the paths
+ * specified in solr.xml's allowPaths element. The following paths will fail validation
+ * <ul>
+ * <li>Relative paths starting with <code>..</code></li>
+ * <li>Windows UNC paths (<code>\\host\share\path</code>)</li>
+ * <li>Absolute paths which are not below the list of allowed paths</li>
+ * </ul>
+ * @param pathToAssert path to check
+ * @param allowPaths list of paths that should be allowed prefixes
+ * @throws SolrException if path is outside allowed paths
+ */
+ public static void assertPathAllowed(Path pathToAssert, Set<Path> allowPaths) throws SolrException {
+ if (OS.isFamilyWindows() && pathToAssert.toString().startsWith("\\\\")) {
Review comment:
Anyone who have a Windows box to test this on?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org