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(login): add login disclaimer #392

Merged
merged 1 commit into from
Dec 15, 2020
Merged

feat(login): add login disclaimer #392

merged 1 commit into from
Dec 15, 2020

Conversation

ferferga
Copy link
Member

image

@ferferga ferferga added this to In progress in Feature parity with Jellyfin Web via automation Dec 15, 2020
@ferferga ferferga moved this from In progress to Review in progress in Feature parity with Jellyfin Web Dec 15, 2020
@sonarcloud
Copy link

sonarcloud bot commented Dec 15, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Feature parity with Jellyfin Web automation moved this from Review in progress to Reviewer approved Dec 15, 2020
@heyhippari heyhippari merged commit 4b8bdb3 into master Dec 15, 2020
Feature parity with Jellyfin Web automation moved this from Reviewer approved to Done Dec 15, 2020
@heyhippari heyhippari deleted the disclaimer branch December 15, 2020 19:22
@Nickbert7
Copy link

Emby somewhen in the past removed the possibility to add HTML code via the branding function. (Somebody used it to add a donate button and if I remember correct the official excuse was that the stores do not accept custom content)
Was this "removal" server-side or part of the web client?
Basically my question is: Can I add a html link via the branding function or will it be again just text?

@ferferga
Copy link
Member Author

Emby somewhen in the past removed the possibility to add HTML code via the branding function. (Somebody used it to add a donate button and if I remember correct the official excuse was that the stores do not accept custom content)
Was this "removal" server-side or part of the web client?
Basically my question is: Can I add a html link via the branding function or will it be again just text?

With this PR it will only appear as text.

We shouldn't allow that by any means as it can lead to XSS, so good thing you brought this up

I think that we should do all the handling client side.

@Nickbert7
Copy link

Emby somewhen in the past removed the possibility to add HTML code via the branding function. (Somebody used it to add a donate button and if I remember correct the official excuse was that the stores do not accept custom content)
Was this "removal" server-side or part of the web client?
Basically my question is: Can I add a html link via the branding function or will it be again just text?

With this PR it will only appear as text.

We shouldn't allow that by any means as it can lead to XSS, so good thing you brought this up

I think that we should do all the handling client side.

I understand that it can lead to security risks, but how about allowing just basic thinks a likes for example to a legal notice, the data protection policy or maybe to the project itself?

@ferferga
Copy link
Member Author

@Nickbert7 I see your point but imo that's over complication for a thing that's intended to be really basic. We can spend the same time it could take us to do this on things that are more worth it.

People that want that level of customization for the disclaimer are probably already building their own versions of the client.

Anyway, this is my opinion, you should open a discussion so this can be brought to the attention of more people, as in this PR will probably go unnoticed.

@Nickbert7
Copy link

It is not such important for me. (As I use it for private purposes and most of my friends will stay signed in and never see those links regularly)
It just have remembered what has changed in the past and I thought about what could be possible via the branding and customs CSS feature.
This could also be important if you add custom themes in the future.
Should they:

  • change colors
  • change colors and change sizes
  • change colors, change sizes and chage content
  • change colors, change sizes, change content and add new content

@camc314
Copy link
Contributor

camc314 commented Dec 15, 2020

I agree with ferferga. We shouldn't be using HTML in these places.

In the future, we might be able to use a custom Vue component. But that is some way down the line.

As Nick mentioned, custom themes should definitely be implemented, although with scoped styles, this could be tricky to change specific areas of the client.

@heyhippari
Copy link
Contributor

We shouldn't allow that by any means as it can lead to XSS, so good thing you brought this up

Do note that we hoist dompurify specifically for these use cases.

While I'm not for complete HTML in the branding, I think allowing a few basic things like bold, italics, etc would be nice. Though I also believe that Markdown is a better fit for it than straight HTML.

heyhippari added a commit that referenced this pull request Dec 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants