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

Preload common JSON-LD contexts #9412

Merged
merged 1 commit into from Dec 2, 2018

Conversation

Projects
None yet
4 participants
@ThibG
Copy link
Collaborator

ThibG commented Dec 2, 2018

Fixes #9411

Gemfile Outdated
@@ -90,6 +90,7 @@ gem 'webpacker', '~> 3.5'
gem 'webpush'

gem 'json-ld', '~> 2.2'
gem "json-ld-preloaded", "~> 2.2"

This comment has been minimized.

@nightpool

nightpool Dec 2, 2018

Collaborator

single quotes per rubocop

@ThibG ThibG force-pushed the ThibG:fixes/include-common-remote-contexts branch from fb74bf0 to 1653839 Dec 2, 2018

@nightpool

This comment has been minimized.

Copy link
Collaborator

nightpool commented Dec 2, 2018

@ThibG have you tested to make sure this solves the issue at hand?

@ThibG

This comment has been minimized.

Copy link
Collaborator Author

ThibG commented Dec 2, 2018

No, I don't have an affected instance, but I'm going to try a few things

@ThibG

This comment has been minimized.

Copy link
Collaborator Author

ThibG commented Dec 2, 2018

@nightpool It doesn't go through the load_jsonld_context codepath for those for the affected JSON-LD contexts, so it should be ok.

@nightpool
Copy link
Collaborator

nightpool left a comment

i want to make sure this gets tested on an affected instance (can just be locally after clearing redis) before merging.

@ThibG

This comment has been minimized.

Copy link
Collaborator Author

ThibG commented Dec 2, 2018

It doesn't try to get the cache anymore when I send a toot, so I'm pretty confident it fixes it.

@kit-ty-kate

This comment has been minimized.

Copy link

kit-ty-kate commented Dec 2, 2018

I'm testing it right now. Give my 30 minutes to rebuild the images and restart the services.

@Gargron

This comment has been minimized.

Copy link
Member

Gargron commented Dec 2, 2018

This is counter to #7301

@nightpool

This comment has been minimized.

Copy link
Collaborator

nightpool commented Dec 2, 2018

@Gargron yes, because of urgent federation problems that some instances are experiencing.

@ThibG

This comment has been minimized.

Copy link
Collaborator Author

ThibG commented Dec 2, 2018

@Gargron I would argue that not depending on HTTP requests to a potentially unreachable remote JSON-LD document that isn't supposed to change is probably worth a slight increase in initial RAM usage

@Gargron

This comment has been minimized.

Copy link
Member

Gargron commented Dec 2, 2018

OK, I've read the problem description. Thoughts:

Context is not neccessary for HTTP signatures, so why is federation broken when the context is unavailable, even though only linked data signatures should be broken? We should contain that error within that functionality, and make sure it doesn't break everything.

I have originally replaced the preloads with a cache mechanism because there is no guarantee that an incoming document will use any of the preloaded contexts, which would mean an arbitrary HTTP request. The preloads are a false sense of security in a way. Perhaps even more importantly, using cache instead of preloads allows even older instances to understand signatures from newer instances if the context has changed. However, I can't deny that currently things are bad because we don't have preloads as a fallback anymore.

@ThibG

This comment has been minimized.

Copy link
Collaborator Author

ThibG commented Dec 2, 2018

@Gargron yes, we could decouple those two things (signing the payload, and sending a potentially unsigned payload), but I don't think there is much sense in that if we can make sure signing the payload is always ok.

My commit doesn't remove the loading and caching of unknown remote contexts. I don't think such “core” contexts are supposed to change, are they? Also, I think preloading them has performances advantages over re-parsing and re-loading them from cache.

@nightpool

This comment has been minimized.

Copy link
Collaborator

nightpool commented Dec 2, 2018

@Gargron i agree, but we should get this fix out urgently and then work on making LDsigs fail-fast later.

@ThibG

I don't think such “core” contexts are supposed to change, are they?

contexts are assumed to be mutable by default, although i believe w3c does provide versioned activitystreams contexts now.

@Gargron

Gargron approved these changes Dec 2, 2018

@nightpool

This comment has been minimized.

Copy link
Collaborator

nightpool commented Dec 2, 2018

I've confirmed that the fix works locally

@Gargron Gargron merged commit 84e5ed4 into tootsuite:master Dec 2, 2018

11 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: check-i18n Your tests passed on CircleCI!
Details
ci/circleci: install Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.3 Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.4 Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.5 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.3 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.4 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.5 Your tests passed on CircleCI!
Details
ci/circleci: test-webui Your tests passed on CircleCI!
Details
codeclimate All good!
Details

Gargron added a commit that referenced this pull request Dec 2, 2018

@kit-ty-kate

This comment has been minimized.

Copy link

kit-ty-kate commented Dec 2, 2018

I confirm that the fixes works on my instance (sorry for the delay, the build failed halfway through)

Gargron added a commit that referenced this pull request Dec 2, 2018

Gomasy added a commit to Gomasy/mastodon that referenced this pull request Dec 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment