framesurge / perseus

A state-driven web development framework for Rust with full support for server-side rendering and static generation.
https://framesurge.sh/perseus/en-US
MIT License
2.17k stars 90 forks source link

GZIP Compression #199

Open danielnehrig opened 2 years ago

danielnehrig commented 2 years ago

This issue is requesting an enhancement to Perseus. Details of the scope will be available in issue labels. The user described the problem related to this request as follows:

server responses to the client can be quite big and adding another proxy infront of the app to gzip compress the response introduces additional cost on the infrastructure if might not needed.

The user described the issue as follows:

I recommend adding gzip compression to perseus responses

  • The author is willing to attempt an implementation: true
Tribble internal data dHJpYmJsZS1yZXBvcnRlZCxDLWVuaGFuY2VtZW50LGF1dGhvci13aWxsaW5nLXRvLWltcGw=
arctic-hen7 commented 2 years ago

This is perfectly reasonable, but should be done at the level of server integrations. Serving exported apps with gzip should be the responsibility of the file host.

This should just be a matter of using the right middleware for each integration.

(PRs are welcome!)

danielnehrig commented 2 years ago

warp already supports it what do you think about adding the .with(warp::filters::compression:gzip()) to the perseus::dflt_server ? @arctic-hen7

arctic-hen7 commented 2 years ago

Yeah certainly! Do you want to submit a PR for that?

danielnehrig commented 2 years ago

yes

ITwrx commented 2 years ago

i think this should be opt-in for security reasons.

What i would like is if pre-rendered pages (and any other relevant assets) were automatically, statically gzipped by Perseus, so nginx could serve that without anything using on-the-fly gzip compression.

danielnehrig commented 2 years ago

could you elaborate how the dynamic warp compression filter would be exploitable in that way?

also i'll build it into the perseus builder pattern

arctic-hen7 commented 2 years ago

The idea of static gzipping is attractive, but only in certain cases. Overall, I think it should be done at request time, simply because Perseus needs to read and modify the stuff on disk extensively when being run aa a server. When exported, I think it should be solely the responsibility of the server the user chooses, just to make things simple.

ITwrx commented 2 years ago

@danielnehrig I don't know anything about Warp's implementation and was only referring to on-the-fly compression side channel attacks, in general; like BREACH I'm also not trying to discourage the development of the feature, as on-the-fly compression makes a noticeable difference and some projects may determine that compression side channel attacks are beyond their security concerns. I only think that it should not be enabled/activated by default, especially since this is something that is usually decided by the server admin and sometimes the developer and the server admin(s) are not the same people. I'm not familiar with the Perseus builder pattern. Maybe that means the user would have to opt-in?

@arctic-hen7 I only mention static gzipping as a safe alternative way to get some compression benefits when feasible, not suggesting as a complete replacement for every project. I can also believe that on-the-fly compression is more convenient to implement and is certainly more comprehensive in it's coverage. FWICT, (and IIRC) it is also fundamentally insecure.

When exported, I think it should be solely the responsibility of the server the user chooses, just to make things simple.

would that also include the manual enabling of the feature? because that's what i'm mainly concerned with. I think any on-the-fly compression option should be documented as a possible security risk and made opt-in to protect users who may not know about the risks.

danielnehrig commented 2 years ago

I'm not familiar with the Perseus builder pattern. Maybe that means the user would have to opt-in?

yes it would be opt-in

arctic-hen7 commented 2 years ago

@ITwrx understood. With static exporting, the user uses some third-party program to serve their files, which should handle any compression independently of Perseus.

I think what @danielnehrig means by the builder pattern is the PerseusApp struct that allows setting options for the app in a builder style. (Do correct me if that's not what you mean though!) Adding an option to signal to server integrations that compression should be used could work, or it might be simpler to use feature flags on the integrations themselves. That is, the perseus-warp package's default server would have compression added only if the dflt-server-gzip flag were set. That would make it opt-in, and would allow the user full control when working with a custom server. This would also allow individual server integrations to support other types of compression in future if needed, without modifying the core. I would prefer this over integration with PerseusApp to be honest.

danielnehrig commented 2 years ago

@ITwrx understood. With static exporting, the user uses some third-party program to serve their files, which should handle any compression independently of Perseus.

I think what @danielnehrig means by the builder pattern is the PerseusApp struct that allows setting options for the app in a builder style. (Do correct me if that's not what you mean though!) Adding an option to signal to server integrations that compression should be used could work, or it might be simpler to use feature flags on the integrations themselves. That is, the perseus-warp package's default server would have compression added only if the dflt-server-gzip flag were set. That would make it opt-in, and would allow the user full control when working with a custom server. This would also allow individual server integrations to support other types of compression in future if needed, without modifying the core. I would prefer this over integration with PerseusApp to be honest.

Correct that's what I meant what would you prefer though over feature flag or the builder pattern ?

arctic-hen7 commented 2 years ago

@danielnehrig I think the feature flag approach would work best.

danielnehrig commented 2 years ago

alright then

arctic-hen7 commented 1 year ago

Having looked into all this further, I think compression should be the default for the Wasm bundle in particular (since I've had reports on Discord of up to seven point Lighthouse improvements, to 100, with vs. without this), and shoudl be done statically. To my understanding, BREACH is not a concern here because it's a static file with no secrets in it. As for page states, that should be completely opt-in if we even decide to support it at all. But brotli compression of the Wasm bundle should be done automatically by the CLI (with an argument to disable this behavior). There should also be gzip compression, and server integrations should be responsible for figuring out what to send. I recall this article had some nice explanations of this.

@danielnehrig are you still thinking of working on this?

danielnehrig commented 1 year ago

i would tackle this next after the cookies #158 implementation but probably only for warp for now since i've already implemented that for my project