You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "epugh (via GitHub)" <gi...@apache.org> on 2024/08/16 13:32:26 UTC
[PR] SOLR-12429: Prevent symbolic links from being uploaded as part of a configset [solr]
epugh opened a new pull request, #2651:
URL: https://github.com/apache/solr/pull/2651
https://issues.apache.org/jira/browse/SOLR-12429
# Description
Preventing the upload of a configset via bin/solr zk upload that contains symbolic links to files and dirs. This prevents a obscure error message, and tells the user what went wrong.
# Solution
I added a check in `FileTypeMagicUtil` which maybe not quite perfect, but let me tap into a custom error message. Maybe the class should be renamed to "ConfigSetFileValidator" ???
Because of java security manager limitations, my attempt to create a symbolic file and directory in a unit test failed, so I backed out and added a BATS style test instead.
I did notice some other places where MAGIC file checking is done, BackupManager, ZkConfigSetService.. But not sure in those situations you can actually get a symbolic link in those places, since you are "inside solr" not on a file system...
# Tests
Bats and manual.
# Checklist
Please review the following and check all that apply:
- [ ] I have reviewed the guidelines for [How to Contribute](https://github.com/apache/solr/blob/main/CONTRIBUTING.md) and my code conforms to the standards described there to the best of my ability.
- [ ] I have created a Jira issue and added the issue ID to my pull request title.
- [ ] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [ ] I have developed this patch against the `main` branch.
- [ ] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
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-12429: Prevent symbolic links from being uploaded as part of a configset [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #2651:
URL: https://github.com/apache/solr/pull/2651#discussion_r1729415225
##########
solr/core/src/java/org/apache/solr/util/FileTypeMagicUtil.java:
##########
@@ -60,6 +60,14 @@ public static void assertConfigSetFolderLegal(Path confPath) throws IOException
new SimpleFileVisitor<>() {
@Override
public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) {
+ if (Files.isSymbolicLink(file)) {
Review Comment:
So, right now symbolic links don't work, they blow up with a cryptic error. This PR 's goal is to replace the cryptic error with a nicer error! I think your suggestion is interesting, and if we wanted to support symbolic link lookup, that sounds like a nice addition to do in a separate PR that removes the limitation. It isn't an immediate itch for me to scratch however. Does that sound reasonable?
--
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-12429: Prevent symbolic links from being uploaded as part of a configset [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh merged PR #2651:
URL: https://github.com/apache/solr/pull/2651
--
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-12429: Prevent symbolic links from being uploaded as part of a configset [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #2651:
URL: https://github.com/apache/solr/pull/2651#discussion_r1729415225
##########
solr/core/src/java/org/apache/solr/util/FileTypeMagicUtil.java:
##########
@@ -60,6 +60,14 @@ public static void assertConfigSetFolderLegal(Path confPath) throws IOException
new SimpleFileVisitor<>() {
@Override
public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) {
+ if (Files.isSymbolicLink(file)) {
Review Comment:
So, right now symbolic links don't work, they blow up with a cryptic error. This PR replaces the cryptic error with a nicer error! I think your suggestion is interesting, and if we wanted to support symbolic link lookup, that sounds like a nice addition to do in a seperate PR that removes the limitation. Does that sound reasonable?
--
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-12429: Prevent symbolic links from being uploaded as part of a configset [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #2651:
URL: https://github.com/apache/solr/pull/2651#issuecomment-2312082732
I am going to merge this PR, however if we want to pair on supporting symbolic links in another PR, all for supporting 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
Re: [PR] SOLR-12429: Prevent symbolic links from being uploaded as part of a configset [solr]
Posted by "fsparv (via GitHub)" <gi...@apache.org>.
fsparv commented on code in PR #2651:
URL: https://github.com/apache/solr/pull/2651#discussion_r1719890154
##########
solr/core/src/java/org/apache/solr/util/FileTypeMagicUtil.java:
##########
@@ -60,6 +60,14 @@ public static void assertConfigSetFolderLegal(Path confPath) throws IOException
new SimpleFileVisitor<>() {
@Override
public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) {
+ if (Files.isSymbolicLink(file)) {
Review Comment:
Ah so much easier then when I had to detect symlinks for Apache Ant back in the day :) (https://bz.apache.org/bugzilla/show_bug.cgi?id=1550#c14)
However, I'm not sure it's valid to assume the user never wants to follow symbolic link. I can certainly imagine cases where folks would use a symlink for things they wanted to keep synced across config sets. It seems to me the correct behavior is to A) check for loops and fail if the symlinks are circular, and B) unroll the symlink (possibly emitting a warning) so that the contents are represented as the same tree that a person navigating with a shell would perceive.
--
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-12429: Prevent symbolic links from being uploaded as part of a configset [solr]
Posted by "gus-asf (via GitHub)" <gi...@apache.org>.
gus-asf commented on code in PR #2651:
URL: https://github.com/apache/solr/pull/2651#discussion_r1719891393
##########
solr/core/src/java/org/apache/solr/util/FileTypeMagicUtil.java:
##########
@@ -60,6 +60,14 @@ public static void assertConfigSetFolderLegal(Path confPath) throws IOException
new SimpleFileVisitor<>() {
@Override
public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) {
+ if (Files.isSymbolicLink(file)) {
Review Comment:
^ me on wrong browser again
--
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