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

Fix OS-specific path separator #71

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix OS-specific path separator #71

wants to merge 1 commit into from

Conversation

anhnmt
Copy link

@anhnmt anhnmt commented Aug 18, 2024

Fix OS-specific path separator when i run on Windows

wfile = "downloads\TEST\Autofills"
x.OutputDir = "downloads/TEST"

@anhnmt anhnmt marked this pull request as ready for review August 18, 2024 02:47
@davidnewhall
Copy link
Contributor

Can you tell me about the problem this fixes? And how I would reproduce that problem to verify this fixes it.

@anhnmt
Copy link
Author

anhnmt commented Aug 19, 2024

@davidnewhall

This is the case when I use Windows to code

I'm using x.OutputDir = "downloads/TEST" but when i extract file, wfile is return wfile = x.clean() // "downloads\TEST\Autofills".

I looked at your x.clean() function and noticed that you used filepath.Clean()

xtractr/files.go

Lines 416 to 427 in 53cf1d6

// clean returns an absolute path for a file inside the OutputDir.
// If trim length is > 0, then the suffixes are trimmed, and filepath removed.
func (x *XFile) clean(filePath string, trim ...string) string {
if len(trim) != 0 {
filePath = filepath.Base(filePath)
for _, suffix := range trim {
filePath = strings.TrimSuffix(filePath, suffix)
}
}
return filepath.Clean(filepath.Join(x.OutputDir, filePath))
}

Therefore, I also reuse this function to bring the two paths to the same type, after checking again with condition !strings.HasPrefix(wfile, x.OutputDir) and it works for me

@davidnewhall
Copy link
Contributor

Why is your code not calling filepath.Clean before passing in a file system path for the wrong operating system? Hiding potentially broken code with a "let me fix your bad path for you" in this library doesn't feel appropriate.

@anhnmt
Copy link
Author

anhnmt commented Aug 19, 2024

You're right, maybe this pr isn't necessary.
But should warn users, because it took me a while to realize the difference when the error only returned as follows:

archived file contains invalid path: downloads\TEST\439596.txt (from: 439596.txt)

In addition, I also saw that the library uses filepath.Join to remove ./ prefix, so I thought why not use filepath.Clean to handle these problems?

xtractr/rar.go

Lines 95 to 96 in 53cf1d6

//nolint:gocritic // this 1-argument filepath.Join removes a ./ prefix should there be one.
if !strings.HasPrefix(wfile, filepath.Join(x.OutputDir)) {

@davidnewhall
Copy link
Contributor

It would be helpful to include the error message up front. It took me a while to understand the problem you're trying to fix. It turns out the problem is bad input. It took you a while to realize you were passing in two paths from different operating systems. Had this library hid that from you, you wouldn't even know it was happening. I'm still not sure this is the right thing to do.

@anhnmt
Copy link
Author

anhnmt commented Aug 19, 2024

Yeah, I understand what you're thinking. But in my opinion, this is not necessarily a bug, we should do something to help people use the library as simply as possible 😊

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.

2 participants