Brave News won't include items with non-http-or-https destination urls #16519

petemill · 2023-01-04T07:29:17Z

Applies to both the combined feed (feed.json hosted on Brave's PCDN) and direct feeds (i.e. RSS feeds)

Resolves brave/brave-browser#27602

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

STR/Cases can be found via https://github.com/brave/internal/issues/987#issue-1526132512.

github-actions · 2023-01-04T07:29:29Z

⚠️ PR head is an unsigned commit
commit: 1bde50f47d5d2cf0e31db2fca25210650702f208
reason: unsigned
Please follow the handbook to configure commit signing
cc: @petemill

petemill · 2023-01-04T07:30:29Z

components/brave_today/browser/direct_feed_controller.cc

-          entry->data->score = entry->data->score * variety;
-          variety = variety * 2.0;
-        }
+        DirectFeedController::BuildArticles(articles, response->data,


Moved this logic to a more testable function

petemill · 2023-01-04T07:31:24Z

components/brave_today/browser/direct_feed_controller.cc

+    if (!item->data->url.SchemeIsHTTPOrHTTPS()) {
+      continue;
+    }


This bypasses non-http(s) items from direct-downloaded RSS feeds

petemill · 2023-01-04T07:32:02Z

components/brave_today/browser/feed_parsing.cc

+  if (!url.SchemeIsHTTPOrHTTPS()) {
+    VLOG(1) << "Item url was not HTTP or HTTPS: " << url.spec();
+    return false;
+  }


This bypasses non-http(s) items from the combined feed hosted by Brave

Is there a corresponding backend change? I think ideally we wouldn't be including any non-https items in the combined feed in the first place.

I would hope it wouldn't include it but doesn't hurt to double-check. And there's always the possibility someone points the browser to a third party combined feed source via the cli param...

Applies to both the combined feed (feed.json hosted on Brave's PCDN) and direct feeds (i.e. RSS feeds)

fallaciousreasoning

LGTM

fallaciousreasoning · 2023-01-05T21:14:08Z

components/brave_today/browser/feed_parsing.cc

+  if (!url.SchemeIsHTTPOrHTTPS()) {
+    VLOG(1) << "Item url was not HTTP or HTTPS: " << url.spec();
+    return false;
+  }


Is there a corresponding backend change? I think ideally we wouldn't be including any non-https items in the combined feed in the first place.

fallaciousreasoning · 2023-01-05T21:14:25Z

components/brave_today/browser/feed_parsing.h

+// Convert from the "combined feed" hosted remotely to Brave News mojom items.
+// TODO(petemill): Rename this file to combined_feed_parsing.h or similar,
+// in order to differentiate the "Combined Feed" from
+// a "Direct Feed" (a.k.a RSS).


sangwoo108

LGTM 👍

Brave News won't include items with non-http-or-https destination urls

kjozwiak · 2023-01-09T20:37:51Z

Verification PASSED on Win 11 x64 using the following build(s):

Brave | 1.49.11 Chromium: 109.0.5414.80 (Official Build) nightly (64-bit)
-- | --
Revision | 0f69b168d36a06cace4365e9f029fa987afa5633-refs/branch-heads/5414@{#1178}
OS | Windows 11 Version 22H2 (Build 22621.963)

Verification notes can be found via https://github.com/brave/internal/issues/987#issuecomment-1376133449.

petemill requested review from bridiver, diracdeltas, sangwoo108 and fallaciousreasoning January 4, 2023 07:29

petemill self-assigned this Jan 4, 2023

petemill commented Jan 4, 2023

View reviewed changes

petemill requested a review from fmarier January 4, 2023 18:27

diracdeltas approved these changes Jan 4, 2023

View reviewed changes

fmarier approved these changes Jan 4, 2023

View reviewed changes

Brave News won't include items with non-http-or-https destination urls

435c8e3

Applies to both the combined feed (feed.json hosted on Brave's PCDN) and direct feeds (i.e. RSS feeds)

petemill force-pushed the news-no-internal-protocol-links branch from 1bde50f to 435c8e3 Compare January 5, 2023 01:46

bridiver approved these changes Jan 5, 2023

View reviewed changes

fallaciousreasoning approved these changes Jan 5, 2023

View reviewed changes

sangwoo108 approved these changes Jan 6, 2023

View reviewed changes

petemill merged commit e46731b into master Jan 6, 2023

petemill deleted the news-no-internal-protocol-links branch January 6, 2023 04:32

github-actions bot added this to the 1.49.x - Nightly milestone Jan 6, 2023

petemill added a commit that referenced this pull request Jan 6, 2023

Merge pull request #16519 from brave/news-no-internal-protocol-links

4b0c10d

Brave News won't include items with non-http-or-https destination urls

This was referenced Jan 6, 2023

Uplift 1.48.x - Brave News won't include items with non-http-or-https destination urls #16563

Merged

Uplift 1.47.x - Brave News won't include items with non-http-or-https destination urls #16564

Merged

petemill added a commit that referenced this pull request Jan 7, 2023

Merge pull request #16519 from brave/news-no-internal-protocol-links

6b87700

Brave News won't include items with non-http-or-https destination urls

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Brave News won't include items with non-http-or-https destination urls #16519

Brave News won't include items with non-http-or-https destination urls #16519

petemill commented Jan 4, 2023 •

edited by kjozwiak

Loading

github-actions bot commented Jan 4, 2023

petemill Jan 4, 2023 •

edited

Loading

petemill Jan 4, 2023

petemill Jan 4, 2023

fallaciousreasoning Jan 5, 2023

petemill Jan 6, 2023

fallaciousreasoning left a comment

fallaciousreasoning Jan 5, 2023

fallaciousreasoning Jan 5, 2023

sangwoo108 left a comment

kjozwiak commented Jan 9, 2023

Brave News won't include items with non-http-or-https destination urls #16519

Brave News won't include items with non-http-or-https destination urls #16519

Conversation

petemill commented Jan 4, 2023 • edited by kjozwiak Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

github-actions bot commented Jan 4, 2023

petemill Jan 4, 2023 • edited Loading

Choose a reason for hiding this comment

petemill Jan 4, 2023

Choose a reason for hiding this comment

petemill Jan 4, 2023

Choose a reason for hiding this comment

fallaciousreasoning Jan 5, 2023

Choose a reason for hiding this comment

petemill Jan 6, 2023

Choose a reason for hiding this comment

fallaciousreasoning left a comment

Choose a reason for hiding this comment

fallaciousreasoning Jan 5, 2023

Choose a reason for hiding this comment

fallaciousreasoning Jan 5, 2023

Choose a reason for hiding this comment

sangwoo108 left a comment

Choose a reason for hiding this comment

kjozwiak commented Jan 9, 2023

petemill commented Jan 4, 2023 •

edited by kjozwiak

Loading

petemill Jan 4, 2023 •

edited

Loading