Skip to content

Conversation

@XingY
Copy link
Contributor

@XingY XingY commented Sep 30, 2025

Rationale

Related Pull Requests

Changes

@XingY XingY requested a review from labkey-susanh September 30, 2025 17:30
}
catch (Exception e)
{
return name; // don't try to be clever if the name is not legal
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

@XingY XingY Sep 30, 2025

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.

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@XingY XingY requested a review from labkey-susanh September 30, 2025 18:42
@XingY XingY merged commit 02ce413 into develop Sep 30, 2025
10 checks passed
@XingY XingY deleted the fb_filePathCheck branch September 30, 2025 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants