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

Remove dependency on deprecated system-filepath #70

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

hololeap
Copy link

@hololeap hololeap commented Dec 1, 2021

This patch removes system-filepath and tries to replace any existing functionality that that package provides.

system-filepath has been deprecated in favor of filepath:
https://hackage.haskell.org/package/system-filepath

@hololeap hololeap marked this pull request as draft December 1, 2021 10:50
@hololeap
Copy link
Author

hololeap commented Dec 1, 2021

The ability to resolve "/var/uploads/" </> "../uploads/home/../etc/passwd" to "/var/uploads/etc/passwd" has been removed from filepath.

How to implement combineSafe without the deprecated system-filepath package?

This patch is an attempt to address this question.

@hololeap
Copy link
Author

hololeap commented Dec 1, 2021

I added a test for the combineSafe function, which is passing with this patch.

@hololeap hololeap marked this pull request as ready for review December 1, 2021 13:45
@hololeap hololeap force-pushed the remove-deprecated branch 3 times, most recently from 47dea0b to f4d6a08 Compare December 1, 2021 20:23
This patch removes `system-filepath` and tries to replace any existing
functionality that that package provides.

`system-filepath` has been deprecated in favor of `filepath`:
https://hackage.haskell.org/package/system-filepath
`hie.yaml` is generated by [implicit-hie][1], part of the
[haskell-language-server][2] project.

[1]: https://github.com/Avi-D-coder/implicit-hie#readme
[2]: https://github.com/haskell/haskell-language-server
@hololeap hololeap marked this pull request as draft December 2, 2021 02:08
@ysangkok
Copy link
Contributor

ysangkok commented Dec 3, 2021

I tried the patch on Linux and it seems to work as expected. I used this:

main = simpleHTTP nullConf $ look "path" >>= serveFileFrom "/tmp" (asContentType "text/plain")

I noticed that combineSafe accepts trailing null bytes, that have no effect. But I guess that isn't really a problem.

I also tried testing it on Windows (wanted to see what happens if I read a pseudo-file like con), but that currently can't be done since the build of sendfile is broken:

[4 of 7] Compiling Network.Socket.SendFile.Win32 (
dist\build\Network\Socket\SendFile\Win32.hs,
dist\build\Network\Socket\SendFile\Win32.o )

src\Network\Socket\SendFile\Win32.hsc:103:1: error:
* Unacceptable result type in foreign declaration:
`IntPtr' cannot be marshalled in a foreign call
because its data constructor is not in scope
Possible fix: import the data constructor to bring it into scope
* When checking declaration:
foreign import ccall unsafe "io.h _get_osfhandle" c_get_osfhandle
:: Fd -> IO IntPtr
|
103 | foreign import ccall unsafe "io.h _get_osfhandle"
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^...
cabal.exe: Failed to build sendfile-0.7.11.1 (which is required by
happstack-server-7.7.1). See the build log above for details.

So I think this looks good. Why is it marked as a draft @hololeap ?

@hololeap
Copy link
Author

hololeap commented Dec 3, 2021

I am also looking at what is keeping turtle from migrating away from system-filepath. I want to hold off until I get a better understanding of what's happening, as there could be some development that will get wrapped into this PR as well.

@hololeap
Copy link
Author

hololeap commented Dec 3, 2021

I noticed that combineSafe accepts trailing null bytes, that have no effect. But I guess that isn't really a problem.

What exactly was the input you were giving it, and what was the behavior you expected?

@ysangkok
Copy link
Contributor

ysangkok commented Dec 3, 2021

@hololeap With this server simpleHTTP nullConf $ serveFileFrom "/tmp" (asContentType "text/plain") $ "donkey.txt" <> concat (replicate 100000 "\x00") and a file in /tmp/donkey.txt, you can do curl localhost:8000 and you get no error. Since serveFileFrom is supposed to accept untrusted input, the replicated bytes that are passed in could have been supplied by the user.

On the other hand, I guess it isn't causing any trouble in this concrete example. I'm more worried about pseudo-files on Windows. But I guess we could also just remove Windows support, I doubt anybody uses it.

@stepcut
Copy link
Member

stepcut commented Dec 22, 2021

Thanks for getting the ball rolling on this! Would definitely like to pull it into main once the issues are worked out.

@hololeap hololeap marked this pull request as ready for review February 5, 2022 07:46
@hololeap
Copy link
Author

hololeap commented Feb 8, 2022

Go ahead and pull this if it seems good. There doesn't seem to be much activity regarding Gabriella439/turtle#54.

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