fix(UNC): backup device type#122
Conversation
|
Fixed Issues (2)Great job! The following issues were fixed in this Pull Request
Communicate with Checkmarx by submitting a PR comment with Checkmarx (@Checkmarx) followed by one of the supported commands. Learn about the supported commands here. |
Miguel Arana (mja9)
changed the title
Aditya Niraula (AdityaNPL)
left a comment
Aditya Niraula (AdityaNPL)
left a comment
There was a problem hiding this comment.
Overall glance, looks good to me
But it's been some time 😅
Just a couple things I thought worth highlighting
| public static bool IsValidFileUrl(string path) | ||
| { | ||
| return Uri.TryCreate(path, UriKind.Absolute, out var uriResult) | ||
| && (uriResult.Scheme == Uri.UriSchemeHttp || uriResult.Scheme == Uri.UriSchemeHttps || uriResult.IsUnc) |
There was a problem hiding this comment.
Any upstream dependency breaking cause it was relying on UNC urls to succeed here which it won't now do?
There was a problem hiding this comment.
To my knowledge no, but I'll see if I can't confirm that through unit tests.
There was a problem hiding this comment.
Ya, that should be good enough, the lib version upgrade can always be backed out and fixed during server integration
There was a problem hiding this comment.
I actually can't find an example of this. From what I gather, a network file represented through a UNC path should always be a FILE device type.
As far as I can tell, URL device types should only be used when backing up to Azure Blob Storage or S3-compatible object storage. I think the current test cases for URLs effectively cover these cases.
In other words, I don't think we should ever have a case where a UNC path should be identified as a URL device type.
3 participants
Experienced an issue where backup device type was being misidentified as URL because of UNC format.
Added test cases to confirm correct device type identification for UNC paths.