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

Improved http client #1098

Closed
Blquinn opened this issue Nov 23, 2020 · 10 comments
Closed

Improved http client #1098

Blquinn opened this issue Nov 23, 2020 · 10 comments

Comments

@Blquinn
Copy link

Blquinn commented Nov 23, 2020

I'm using v0.12.0 for reference.

Is your feature request related to a problem? Please describe.

The current http client only returns the body from the response. It needs to return all of the response meta data as well. Status, headers etc.

Describe the solution you'd like

I would just return a response object from the rust side and allow people to handle the response however they need in javascript. It would probably make sense to try to emulate the api of popular http clients in the js world, like axios. Perhaps tauri could even supply a backend for one of those clients.

Describe alternatives you've considered

browser's fetch api et al.

@nothingismagick
Copy link
Sponsor Member

Yeah, I agree that our http service in the tauri app (on the rust side) is a little weak. However, I am very much opposed to the general idea that some people have of using "fetch" or similar in the webview, especially considering the frequency with which axios is hit with vulns. We have discussed replacing tiny_http with something more robust. Are you interested in helping us out here @Blquinn ?

@lucasfernog
Copy link
Member

tiny_http is the internal http server library Tauri uses to serve the app's assets (JS, CSS, HTML, images...). The issue here is the HTTP client API, which uses attohttpc.

We'd love some contributions here, this is not so hard to write: main changes should be wrriten here along with type changes, docs etc.

@nklayman
Copy link
Member

@nothingismagick What is the disadvantage of using pure fetch? It seems like it is unlikely to have any vulns and it's modern enough that you don't need a third-party library like Axios to use it.

@nothingismagick
Copy link
Sponsor Member

The point is an architectural one. When you are using the webview to perform your "fetch-like" request, you are opening up the system to attacks that can directly influence the interface. I think it is generally better to do the fetching on the Rust side and then send the response to the webview after you have analysed it for correctness or even rewritten it for safety.

@Blquinn
Copy link
Author

Blquinn commented Jan 24, 2021

@nothingismagick @lucasfernog Sorry, missed all these replies for some reason!

I have a decent implementation in a toy project I'm working on specifically in src/tauri/http.ts and src-tauri/src/http.rs.

Pulls in dependencies on ureq (rust http client) and base64 (to serialize request bodies to/from webview). ureq seemed like a solid and tiny http lib, which I picked mainly because it had minimal impact on executable size.

Perhaps you can take a look at the exposed api and let me know what you think?

@Blquinn
Copy link
Author

Blquinn commented Jan 24, 2021

Actually... now that I look at that it's missing a few things. It's really just the low level request method which exposes all the dials.

Biggest issue is that there's no real way of handling binary responses, or large responses. It loads the whole response body in the webview as a string.

I think a couple things that would improve it are:

  1. Having a "download to file" api, which would be good for binary responses and large responses that you don't necessarily want to load into js memory. ("Upload from file" as well.)
  2. A few helper functions (one for each common http method) like python's requests package an others.

Also, I'm happy to make a contribution if you all like the cut of this http client's jib.

@nothingismagick
Copy link
Sponsor Member

nothingismagick commented Jan 24, 2021

I definitely like the look of it, but think we would have to add tests. I like download to file / upload from file, but I would like some kind of sandboxing there - where we give the downloaded file a randomized name that we can track. Otherwise it is an attack vector. Upload from file is similarly potentially an exfiltration tool.

I also think that we need to have some kind of optional header validation and that it would absolutely require a comprehensive acceptlist.

Again, I think having a test suite would be important. Also, some people might like to see benches...

But all in all would love to see a PR in this direction!

@nothingismagick
Copy link
Sponsor Member

I just skimmed the ts, and think that it would also be wise to have some kind of error handling there.

@Blquinn
Copy link
Author

Blquinn commented Jan 24, 2021

Seems like the file upload/download will need further consideration from a security perspective... Sounds like you're suggesting some kind of a whitelist from the rust side? I think that could make sense. I think it would be prudent to skip that feature for now, as it can easily be added later once proper consideration for security has been given.

I honestly have no idea what happens if an error is returned from the rust side, I assume that translates into some kind of an exception, but we probably want to catch it and translate it into something useful. Especially if the exception is a failure to resolve DNS or something like that.

It definitely needs tests, especially around error handling, both on the rust side and the typescript side.

@nothingismagick
Copy link
Sponsor Member

nothingismagick commented Jan 24, 2021

Seems like the file upload/download will need further consideration from a security perspective...

Yes, touching FS is pure danger, and allowing remote services to write to and read from is indeed something that needs some thought put into it.

acceptlist == whitelist

Yes, we could actually have a specific acceptlist for all granular features that this API endpoint would provide. Obviously "all" can be an option, but just because I want to allow outgoing does not mean I want to allow incoming.

errors

It depends on how you trap them, but actually i am more worried about the error handling on the TS side to be honest. But I am generally of the more paranoid type.

Long story short: glad you put some thought into it, and am excited to help review your work and get it merged. :)

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

No branches or pull requests

4 participants