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

feat: support for setting UA for URL previews #17968

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

MomentQYC
Copy link
Contributor

@MomentQYC MomentQYC commented Nov 27, 2024

Fix #17873

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Code style is correct
    (run the linters)

close #17873

@github-actions github-actions bot deployed to PR Documentation Preview November 27, 2024 02:11 Active
@github-actions github-actions bot deployed to PR Documentation Preview November 27, 2024 02:16 Active
@MomentQYC MomentQYC marked this pull request as ready for review November 27, 2024 02:40
@MomentQYC MomentQYC requested a review from a team as a code owner November 27, 2024 02:40
Example configuration:
```yaml
url_preview_user_agent: "Hello Matrix"
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should put this next to the other url_preview_xxx options

or "Synapse (bot; +https://github.com/matrix-org/synapse)"
)
if len(self.url_preview_user_agent.strip()) == 0:
raise ConfigError("The 'url_preview_user_agent' must be a valid User-Agent")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
raise ConfigError("The 'url_preview_user_agent' must be a valid User-Agent")
raise ConfigError(
"Must be a valid User-Agent string",
("url_preview_user_agent",),
)

Comment on lines +791 to +797
self.url_preview_user_agent: str = (
config.get("url_preview_user_agent")
or "Synapse (bot; +https://github.com/matrix-org/synapse)"
)
if len(self.url_preview_user_agent.strip()) == 0:
raise ConfigError("The 'url_preview_user_agent' must be a valid User-Agent")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should do this processing alongside the other url_preview_config in synapse/config/repository.py

Comment on lines +792 to +793
config.get("url_preview_user_agent")
or "Synapse (bot; +https://github.com/matrix-org/synapse)"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
config.get("url_preview_user_agent")
or "Synapse (bot; +https://github.com/matrix-org/synapse)"
config.get("url_preview_user_agent", "Synapse (bot; +https://github.com/matrix-org/synapse)")

b"User-Agent": [
"Synapse (bot; +https://github.com/matrix-org/synapse)"
],
b"User-Agent": [self.hs.config.server.url_preview_user_agent],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to what we do for max_spider_size in __init__, we can do the same for url_preview_user_agent

Suggested change
b"User-Agent": [self.hs.config.server.url_preview_user_agent],
b"User-Agent": [self.url_preview_user_agent],

@@ -0,0 +1 @@
Add support for setting User-Agent for URL previewing.
Copy link
Contributor

@MadLittleMods MadLittleMods Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The option itself seems relatively straightforward. Our sane default feels like it should be good enough. I'm going to add this to the To-Discuss board to talk with the team on Monday whether we want to introduce this.

The workaround use case seems kinda meh but I appreciate the context. (adding yet another option for a problem that will go away/change in the future)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed with the team and we're not keen on pretending to be someone else. If someone doesn't want to serve content to Synapse, that's their prerogative.

This might be a XY problem. The goal is make YouTube URL previews work. Do they have some API/flow that they prefer people to use? Should we be using OpenGraph/oEmbed instead of scraping, etc? We'd rather have PR's for that instead of this workaround.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

he goal is make YouTube URL previews work. Do they have some API/flow that they prefer people to use? Should we be using OpenGraph/oEmbed instead of scraping, etc?

See #17462.

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

Successfully merging this pull request may close these issues.

Ability to configure User-Agent for URL previews
3 participants