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 path #416

Merged
merged 3 commits into from
Oct 16, 2021
Merged

Fix path #416

merged 3 commits into from
Oct 16, 2021

Conversation

mattn
Copy link
Contributor

@mattn mattn commented Oct 5, 2021

No description provided.

@@ -286,7 +288,7 @@ func (s *Server) notFoundHandler(w http.ResponseWriter, r *http.Request) {
}

func sanitize(fileName string) string {
return path.Clean(path.Base(fileName))
return filepath.Base(fileName)
Copy link
Collaborator

@aspacca aspacca Oct 8, 2021

Choose a reason for hiding this comment

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

this seems to change the behaviour:
https://pkg.go.dev/path#Clean
https://pkg.go.dev/path/filepath#Base

It might be that's not what we need.
Can you clarify if it's an actual bug and provide steps to reproduce it?

Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand why you call path.Clean.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, sorry, I mislooked the fact that we apply path.Clean after path.Base

The path package should only be used for paths separated by forward slashes, such as the paths in URLs. This package does not deal with Windows paths with drive letters or backslashes; to manipulate operating system paths, use the path/filepath package.

I think path package is a better option here rather than filepath because the input is a URL path.
What do you think?

In case you agree you can remove path.Clean and keep only path.Base

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks your explaining. Updated.

@aspacca aspacca merged commit e5455d9 into dutchcoders:main Oct 16, 2021
GanZhiXiong added a commit to GanZhiXiong/transfer.sh that referenced this pull request Nov 10, 2021
…ch-1

* 'patch-1' of github.com:GanZhiXiong/transfer.sh:
  issue dutchcoders#420 added MaxDate.IsZero() check (dutchcoders#427)
  fix go1.16, add go1.17
  bump bluemonday
  Revert "issue dutchcoders#420 return 400 response when Max-Days is too big (dutchcoders#422)" (dutchcoders#426)
  issue dutchcoders#420 return 400 response when Max-Days is too big (dutchcoders#422)
  Implement Nix Flake (dutchcoders#424)
  fix missed errors (dutchcoders#417)
  Fix path (dutchcoders#416)
  Edited code of condunct for more information and corrected a grammatical error (dutchcoders#421)
  Update README.md (dutchcoders#415)
  Update README.md
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