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

Add option for RSS sanitization level #2429

Closed
wants to merge 2 commits into from

Conversation

TrimmingFool
Copy link
Contributor

@TrimmingFool TrimmingFool commented Jan 3, 2023

@stickz Here you go. #2428 (comment)

Additionally, I included Novik's noreferrer suggestion #2426 (comment). This problem is also addressed here: https://developer.mozilla.org/en-US/docs/Web/Security/Referer_header:_privacy_and_security_concerns#how_can_we_fix_this

Idea: some users want to have a less restricted view as in #2426.
This PR makes the sanitization levels (RESTRICTED, BASIC, RELAXED) available to the user.

WIP: security requirements are unclear (possibly completely different sanitization profiles required)

@stickz
Copy link
Collaborator

stickz commented Jan 5, 2023

We need more security features to protect against things like malware and child exploitation, before we can allow relaxing the RSS sanitation level changes. We can't allow images to be blindly displayed and force users to click on links without preview.

Could you limit the scope of this pull request to links and open a new one for images? We can't easily review mixed features and hundreds of lines of code. It's almost guaranteed to introduce a regression and make it harder to find.

  1. We need a hint box displaying the URL when the user hovers over the link in the RSS feed.

  2. We need a warning hint box in the settings menu that phishing links cause malware.

  3. We need to whitelist image hosts that are not part of the domain of the RSS feed .

transformers: [bbclassTransform],
});
const rawHTML = String(data);
const dirtyHTML = theWebUI.mapBBCodeToHTML(rawHTML);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is from the other pull request. We need changes tested and we can't be stringing pull requests like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I only fully test all changes at once. This is what the jest tests should have caught.

@@ -74,6 +74,8 @@
theUILang.rssAllFiters = "All filters";
theUILang.rssUpdateInterval = "Update interval";
theUILang.rssShowErrorsDelayed = "Delay RSS error notification until the log tab or RSS feed is visible";
theUILang.rssSanitizationLevel = "Show external RSS content";
theUILang.rssSanitizationLevels = {RESTRICTED: "Restricted (no links or images)", BASIC: "Basic (no images)", RELAXED: "Relaxed"};
Copy link
Collaborator

Choose a reason for hiding this comment

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

I disagree that we require three sanitation levels here. I think Secure and Insecure should be fine. Stronger terminology also allows the user to make a more education decision about this functionality.

@stickz stickz added the WIP label Jan 5, 2023
@TrimmingFool TrimmingFool marked this pull request as draft January 5, 2023 14:29
@TrimmingFool
Copy link
Contributor Author

@stickz I think, it should be made clear what we assume to be the threat model, here.
Users who automate downloads of torrents trust in the content moderation and integrity of the RSS feed, already.
Thus, besides obviously protecting against XSS, users only need options to relax the default strict protection against privacy threats.

While a allowlist of image hosts and a hint box for sanitized links <a> make sense to me, I doubt users need to be informed by ruTorrent of the dangers of viewing or downloading content of untrusted sources.

Further suggestions:

  1. A trust level profile (such as public, private) for individual RSS feeds may make sense, instead of Secure and Insecure
  2. Images could also be proxied (with action.php?fetchurl=...)
  3. The domain of the RSS links guid (opened with dblclick) or the torrent url should also match the RSS feed domain

However, I am not motivated to work on this PR for now. Maybe someone else can take this on.

@stickz
Copy link
Collaborator

stickz commented Jan 5, 2023

@TrimmingFool I agree with the concept of implementing a trust level profile for individual RSS feeds, but I still think we require anther feature to whitelist image hosts. Many torrent websites require the use of a third party image host for images.

The reason why I think we need these security improvements is based on extensive web security training. An RSS feed with relaxed sanitation restrictions could infect someone with malware almost instantly by phishing. It is a risk to download files from torrent websites; however they pose no risk to the user, until the files are opened and scanned by a virus scanner. There is also additional improvements to operating systems (such as signatures on executable programs) which mitigate the risk of malware.

If we don't protect against phishing links, thousands of users could be infected easily with ransomware attacks. This is the gravity of the situation I'm basing these suggestions off. Whitelisting individual RSS feeds would greatly improve user security. I would much rather a knowledge user add a feed to a whitelist, than have users select an alternative insecure torrent web client.

@TrimmingFool TrimmingFool changed the base branch from master to develop January 8, 2023 15:01
restrict sending referer header for `img` and `a`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants