dwrensha / sharelatex

A web-based collaborative LaTeX editor
GNU Affero General Public License v3.0
19 stars 8 forks source link

ShareLatex broken by client-side loading changes in 0.270 #15

Closed ocdtrekkie closed 4 years ago

ocdtrekkie commented 4 years ago

Reported here: https://groups.google.com/forum/#!topic/sandstorm-dev/EBmrwrGQhDk Related PR: https://github.com/sandstorm-io/sandstorm/pull/3409

ShareLatex apparently primarily loads fonts from a CDN, and this has caused breakage as of 0.270's implementation of the client security policy. We should look at patching this app up since people actively use it and find the breakage problematic.

ocdtrekkie commented 4 years ago

So we should look to embed Open Sans, PT Serif, and FontAwesome. The latter being the primary issue here.

Content Security Policy: The page’s settings blocked the loading of a resource at https://netdna.bootstrapcdn.com/font-awesome/4.2.0/css/font-awesome.min.css (“style-src”).

Content Security Policy: The page’s settings blocked the loading of a resource at https://fonts.googleapis.com/css?family=Open+Sans:300,400,600,700 (“style-src”).

Content Security Policy: The page’s settings blocked the loading of a resource at https://fonts.googleapis.com/css?family=PT+Serif:400,600,700 (“style-src”).
dwrensha commented 4 years ago

Can Sandstorm implement an allow-list of package IDs for which the content security policy should not be enforced? Then we wouldn't need to dig into this kind of update.

ocdtrekkie commented 4 years ago

I think we expect the amount of breakage to be small enough we can mostly cope with not leaving large legacy security holes. We do have a workaround to relax/temporarily revert the policy server-wide for users who need a broken app, so we are not in an emergency situation here. (If this breaks a lot more than we expected, we can also push that workaround to all servers while we address them.)

I also think if we have an app from 2015 that's still in active use in which this is a problem, it's valuable to the community for us to get it up to a 2020 level anyhow. URLs are rarely reliable in the span of decades, I don't think we should assume a CDN URL set in 2015 will work indefinitely.

zenhack commented 4 years ago

I put together some scripts to monkey-patch the existing spk here:

https://github.com/zenhack/patch-sharelatex

The README reflects conversation on IRC re: what to do here. @dwrensha, running those scripts and then spk pack should give you a working spk, if you're up for it.

Note that whitelisting kinda misses the point -- while reaching out to google for fonts isn't exactly the most nefarious thing I've ever seen, it is actively violating expectations we set about how apps use the platform, and it should be blocked -- the fact that not doing so will soon break the app makes this more urgent, but we should be doing it anyway.

dwrensha commented 4 years ago

@zenhack thanks for setting all of that up. I ran your scripts and updated the version numbers. I also needed to change alwaysInclude = ["/"] into alwaysInclude = "." (I think that might include more than is actually necessary, but I don't think it's a problem.) I then did spk publish, so I think this is up to the App Market review team now.

dwrensha commented 4 years ago

hm... now I'm worried that alwaysInclude = "." was the wrong thing.

dwrensha commented 4 years ago

I needed to change something because otherwise I got the error

Destination (in-package) path must not start with '/': /
ocdtrekkie commented 4 years ago

My stupid check: Old package is 173 MB New package is 174 MB (sounds about right?)

Seems to work for me too.

dwrensha commented 4 years ago

I'm reading the documentation at https://github.com/sandstorm-io/sandstorm/blob/master/src/sandstorm/package.capnp

It looks like alwaysInclude refers to package paths (rather than source paths) and resolves relative to root, so I think this is correct, but I'd like @zenhack to double check this.

zenhack commented 4 years ago

That sounds right, and doing an spk unpack gives me something that looks reasonable.

dwrensha commented 4 years ago

OK, let me make a version that updates the changelog too.

dwrensha commented 4 years ago

Done -- I've run spk publish on the new spk with an updated changelog.

ocdtrekkie commented 4 years ago

How are we documenting the source on this? Do we care? Do we need a blurb on this repo that says "take the SPK this repo made, use this patch script on it", etc.?

zenhack commented 4 years ago

Maybe we should merge @dwrensha's changes into the repo I put together, and then add a link from the main repo?

ocdtrekkie commented 4 years ago

Maybe we should merge @dwrensha's changes into the repo I put together, and then add a link from the main repo?

This seems like the best approach we're gonna get.

dwrensha commented 4 years ago

https://github.com/zenhack/patch-sharelatex/pull/1

I would accept a pull request adding a note to this repo.

ocdtrekkie commented 4 years ago

I am going to go ahead and close this, as pending release, this issue is resolved.