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

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

Closed
ocdtrekkie opened this issue Aug 31, 2020 · 17 comments
Closed

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

ocdtrekkie opened this issue Aug 31, 2020 · 17 comments

Comments

@ocdtrekkie
Copy link

Reported here: https://groups.google.com/forum/#!topic/sandstorm-dev/EBmrwrGQhDk
Related PR: sandstorm-io/sandstorm#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
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

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
Copy link

zenhack commented Sep 1, 2020

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
Copy link
Owner

dwrensha commented Sep 1, 2020

@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
Copy link
Owner

dwrensha commented Sep 1, 2020

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

@dwrensha
Copy link
Owner

dwrensha commented Sep 1, 2020

I needed to change something because otherwise I got the error

Destination (in-package) path must not start with '/': /

@ocdtrekkie
Copy link
Author

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

Seems to work for me too.

@dwrensha
Copy link
Owner

dwrensha commented Sep 1, 2020

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
Copy link

zenhack commented Sep 1, 2020

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

@dwrensha
Copy link
Owner

dwrensha commented Sep 1, 2020

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

@dwrensha
Copy link
Owner

dwrensha commented Sep 1, 2020

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

@ocdtrekkie
Copy link
Author

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
Copy link

zenhack commented Sep 1, 2020

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

@ocdtrekkie
Copy link
Author

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
Copy link
Owner

dwrensha commented Sep 1, 2020

zenhack/patch-sharelatex#1

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

@ocdtrekkie
Copy link
Author

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

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

3 participants