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

Trusted Domains: Allow trusting a domain regardless of HTTP basic auth credentials #437

Merged
merged 3 commits into from Oct 17, 2021

Conversation

Tremolo4
Copy link
Contributor

TL;DR: If you want to trust URIs with credentials in them, you have to add the full thing to the trusted domains: user:pass@example.com. This PR makes it so you can just add example.com, even when using credentials.

Long version
I have the following use-case: I have a webserver that serves videos, but they are protected with HTTP basic authentication. It is possible to embed the credentials in a URL like so:

https://username:password@example.com/video.mkv

This URL works, for example, in mpv. I can add this URL in the syncplay playlist and it works fine. But I get the untrusted domain warning and have to click the playlist entry manually. If I add example.com to the "trusted domains" in the Advanced menu, I still get the warning. Only if I add username:password@example.com to the trusted domains, the warning disappears.

This workaround is not very good because the credentials are saved on disk in the syncplay settings and also the trusted domain entry has to be adjusted if the credentials change.

Therefore, I've rewritten the trusted domain checking code to ignore the credentials. I use urllib.parse to make it quite simple and safe.

I have taken care to match the old behavior exactly, except: If someone already uses the workaround above, i.e. already has a domain with basic auth credentials in the trusted domains list, it will no longer match anything. If this exception is undesired, I can easily make it so that trusted domains with embedded credentials are also still matched.

Note that trusted domains can not only be domains, they can also have a path attached, e.g. example.com/videos. For these cases, there is another -- IMO unimportant -- difference: The old code only trusts URLs if a trusted domain is a prefix plus a slash. For example:
Trusted domain example.com/videos allows example.com/videos/wat.mkv but not example.com/videoswat.mkv.
Trusted domain example.com/videos/ allows example.com/videos//wat.mkv but not example.com/videos/wat.mkv.
My new code does not add the extra slash -- each of the two trusted domains above match both given example URLs. I prefer this compared to the old behavior, but I can change it if desired.

@Et0h
Copy link
Contributor

Et0h commented Jul 23, 2021

I've not tested it yet, but this looks like a great improvement to me. Thanks for your contribution.

@Tremolo4
Copy link
Contributor Author

Tremolo4 commented Jul 24, 2021

Thanks!

I just noticed that there is an "add [domain] as trusted" option when right clicking a domain in the playlist. I made it so that the credentials are also stripped there.

@Et0h Et0h merged commit 2bf3931 into Syncplay:master Oct 17, 2021
@Et0h
Copy link
Contributor

Et0h commented Oct 17, 2021

Great, thanks for your work on this!

@Tremolo4 Tremolo4 deleted the pr/trusted-domain-credentials branch October 24, 2021 23:48
Et0h added a commit that referenced this pull request Oct 27, 2021
* Create pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* add libxcb manually

* Add missing libxcb-util to build environment

* Remove references to IRC (#430)

* Add reference to GitHub discussions

* Update issue templates

* Revert "Merge branch 'master' into master"

This reverts commit 173007e, reversing
changes made to 6105da8.

* Make Windows build 32-bit

* Support for LAV Filters Megamix (#457)

* Upver to 1.7.0 (r100)

* Refactor MPC-HC player path code (#453)

* Add LANG Parameter (#460)

If LANG parameter set, don't show language dialog.
Example usage: .\Syncplay-X.X.X-Setup.exe /S /LANG=1033

* Trusted Domains: Allow trusting a domain regardless of HTTP basic auth credentials (#437)

* Trusted Domains: don't consider HTTP basic auth credentials part of the domain name

* Trusted Domains: hide "add as trusted domain" menu item if entry does not contain domain

* Trusted Domains: strip HTTP basic auth credentials also when adding as trusted domain via context menu

* Allow .m3u/.m3u8 files to be played from network (#419)

* Fix room name case sensitivity UI issue (#403)

* Remove redundant help button from dialogs (#403)

* darkdetect: update vendor copy to 0.5.0

* macOS build: upgrade to Python 3.9 and PySide2 5.15.2

* Remove Encoding from .desktop files as it's depreciated now.

* Add Keywords entry to .desktop files.

* Console: Document setting offset in help (#435)

* Add deprecation notice for offset help (#435)

* Begin move from m3u/m3u8 to txt for playlist (#408)

Co-authored-by: Daniel Wróbel <wrobel.dan@gmail.com>
Co-authored-by: daniel-123 <1662391+daniel-123@users.noreply.github.com>
Co-authored-by: Assistant <assistant.moetron@gmail.com>
Co-authored-by: ImportTaste <53661808+ImportTaste@users.noreply.github.com>
Co-authored-by: Ata Gülalan <xava@hotmail.com.tr>
Co-authored-by: Tremolo4 <Tremolo4@users.noreply.github.com>
Co-authored-by: Ridan Vandenbergh <RidanVandenbergh@gmail.com>
Co-authored-by: Alberto Sottile <alby128@gmail.com>
Co-authored-by: Luke Dashjr <luke_github1@dashjr.org>
@name-snrl
Copy link

@Tremolo4 hello, I think this commit broke wildcard matching mechanism. can you fix that? or do I need to create a new issue?

@Tremolo4
Copy link
Contributor Author

Tremolo4 commented Feb 9, 2023

When you say wildcard-matching, do you mean using a *? Just try removing the *. I think it always does a prefix match.

@name-snrl
Copy link

name-snrl commented Feb 9, 2023

Just try removing the *

Unfortunately, this did not solve the problem. I tried trusteddomains = ['youtube.com', 'youtu.be', 'eu.ngrok.io'] and trusteddomains = ['youtube.com', 'youtu.be', '.eu.ngrok.io'], still doesn't work(

ERROR: Could not automatically load [example] because it is not on a trusted domain. You can switch to the URL manually by double clicking it in the playlist, and add trusted domains via File->Advanced->Set Trusted Domains. If you right click on a URL then you can add its domain as a trusted domain via the context menu.

@Tremolo4
Copy link
Contributor Author

Tremolo4 commented Feb 9, 2023

I see. I thought you were using the * in the path component (after /), not subdomains. I'll see what I can do.

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.

None yet

3 participants