Skip to content

matrix-synapse service: Make url_preview_enabled optional #20609

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 28, 2016
Merged

matrix-synapse service: Make url_preview_enabled optional #20609

merged 1 commit into from
Nov 28, 2016
+48 −2

Conversation

eqyiel
Copy link
Contributor

@eqyiel eqyiel commented Nov 21, 2016

Motivation for this change

I wanted to enable URL previews for matrix-synapse.

Previously, it was possible to set url_preview_enabled: true using extraConfig, but the configuration would not be applied because synapse complains that lxml is missing.

So lxml has been added to propagatedBuildInputs, and there are config options for url_preview_enabled, url_preview_ip_range_blacklist, url_preview_ip_range_whitelist and url_preview_url_blacklist.

This change should be completely transparent to existing users since url_preview_enabled defaults to false (like it was before).

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

Sorry, something went wrong.

@mention-bot
Copy link

@eqyiel, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Ralith, @ehmry and @roblabla to be potential reviewers.

@Ralith
Copy link
Contributor

Ralith commented Nov 21, 2016

Aren't there default/initial values for the blacklist ranges? Considering the security implications of this feature, I think it should be as easy as possible to configure it properly.

@eqyiel
Copy link
Contributor Author

eqyiel commented Nov 21, 2016

@Ralith there are no defaults for url_preview_ip_range_blacklist (nor the others as far as I can tell).

I agree with you though. I noticed that synapse will refuse to start if you have url_preview_enabled but no url_preview_ip_range_blacklist - do you think it's better to let synapse throw an error, or to set our own default value of url_preview_ip_range_blacklist?

According to the README:

At the very least we recommend that your loopback and RFC1918 IP addresses are blacklisted.

I guess I'll ask in Matrix HQ / matrix-dev what a reasonable default would be. Both of these options require changes to this PR though, so probably don't merge this yet.

@Ralith
Copy link
Contributor

Ralith commented Nov 25, 2016 •
edited
Loading

@eqyiel The defaults are defined in the source.

edit: Er, nevermind, I see that's what you linked to already. I'd think defaulting to the suggested options would be safest, since we can update that default in the face of errors while an individual admin might not, but I don't feel strongly about it.

@eqyiel
Copy link
Contributor Author

eqyiel commented Nov 27, 2016

@Ralith I have amended the PR to use the suggested options!

@fpletz fpletz changed the title matrix-synapse: Make url_preview_enabled optional matrix-synapse service: Make url_preview_enabled optional Nov 28, 2016
@fpletz fpletz merged commit 9c9a21d into NixOS:master Nov 28, 2016
@fadenb fadenb mentioned this pull request May 5, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fpletz fpletz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@eqyiel @mention-bot @Ralith @fpletz