-
Notifications
You must be signed in to change notification settings - Fork 7
Check file path name #7086
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
Check file path name #7086
Conversation
| } | ||
| catch (Exception e) | ||
| { | ||
| return name; // don't try to be clever if the name is not legal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the name is not legal do we want to return it here or throw?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change is meant to skip the NetworkDrive.exists(f) check if the name is illegal. As for throwing, that would be a bigger change for when we enforce check more broadly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least one of the callers for tryName also does a check for NetworkDrive.exists with the name that is returned here. Is that not a problem if the name has dots and slashes in it? Or am I misunderstanding the usages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I've updated the caller (FileType line 352) to use a different method to resolve file. Looking at the callers it seems Path.of(name) would actually never be called since parentDir is never null. So it seems pretty safe to throw. I've updated to throw instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might also need to update FileType.getPath, which also uses parentdir.resolve(getName(...)), which calls tryName. There are also a couple of other usages of FileType.getName that might need validating or updating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Rationale
Related Pull Requests
Changes