Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SOLR-14561 CoreAdminAPI's parameters instanceDir and dataDir are now validated #1572

Merged
merged 28 commits into from
Jun 18, 2020

Conversation

janhoy
Copy link
Contributor

@janhoy janhoy commented Jun 11, 2020

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.

@janhoy janhoy requested a review from dsmiley June 11, 2020 23:35
@janhoy
Copy link
Contributor Author

janhoy commented Jun 12, 2020

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:
allowPath

Add several extra tests, restructured tests for better readability
@janhoy
Copy link
Contributor Author

janhoy commented Jun 15, 2020

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

@janhoy
Copy link
Contributor Author

janhoy commented Jun 17, 2020

The tests still don't pass on Windows, and I have found the reason. Will push a few more changes on friday.

@janhoy
Copy link
Contributor Author

janhoy commented Jun 17, 2020

All tests now passing on macOS and hopefully Windows (running tests now in a slow VirtualBox).

"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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

@janhoy
Copy link
Contributor Author

janhoy commented Jun 17, 2020

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.

@dsmiley
Copy link
Contributor

dsmiley commented Jun 18, 2020

@mkhludnev ? I recall you use Windows.

@janhoy janhoy merged commit 936b9d7 into apache:master Jun 18, 2020
@janhoy janhoy deleted the solr14561 branch June 18, 2020 14:13
janhoy added a commit that referenced this pull request Jun 19, 2020
…validated (#1572)

(cherry picked from commit 936b9d7)
Changed some Java11 syntax to Java8 syntax
MarcusSorealheis added a commit to MarcusSorealheis/lucene-solr that referenced this pull request Jun 20, 2020
* upstream/master: (218 commits)
  LUCENE-9412 Do not validate jenkins HTTPS cert
  LUCENE-8962: add ability to selectively merge on commit (apache#1552)
  Replace DWPT.DocState with simple method parameters (apache#1594)
  LUCENE-9402: Let MultiCollector handle minCompetitiveScore (apache#1567)
  SOLR-14574: Fix or suppress warnings in solr/core/src/test (part 2)
  SOLR-14561 CoreAdminAPI's parameters instanceDir and dataDir are now validated (apache#1572)
  SOLR-14532: Add *.iml files to gitignore
  SOLR-14577: Return BAD REQUEST when field is missing in terms QP (apache#1588)
  SOLR-14574: Fix or suppress warnings in solr/core/src/test (part 1)
  remove debug code
  LUCENE-9408: roll back only called once enforcement
  LUCENE-8962: Allow waiting for all merges in a merge spec (apache#1585)
  SOLR-14572 document missing SearchComponents (apache#1581)
  LUCENE-9359: Avoid test failures when the extra file is a dir.
  SOLR-14573: Fix or suppress warnings in solrj/src/test
  LUCENE-9353: Move terms metadata to its own file. (apache#1473)
  Cleanup TermsHashPerField (apache#1573)
  SOLR-14558: Record all log lines in SolrLogPostTool (apache#1570)
  LUCENE-9404: simplify checksum calculation of ByteBuffersIndexOutput
  LUCENE-9403: tune BufferedChecksum.DEFAULT_BUFFERSIZE
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants