Implement finch tracker #732

atuchin-m · 2023-08-30T14:59:11Z

For #741
Specs: https://docs.google.com/document/d/1hRWkAnJtPjyvFglUQE0GegtyNtcVkjFpHPssdJJAqN0/

Ready to review.

TODO:

change private repo URL
change slack group ID for notifications
deploy new griffin.brave.com after the PR is merged

socket-security · 2023-08-30T15:02:00Z

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Packages	Version	New capabilities	Transitives	Size	Publisher
protobufjs	7.2.5	None	`+1`	2.89 MB	google-wombot
eslint-plugin-licenses	1.0.2	None	`+41`	1.85 MB	fallaciousreasoning
eslint-config-prettier	9.0.0	environment	`+41`	1.81 MB	lydell
@types/react-dom	18.2.7	None	`+0`	30.2 kB	types
ts-jest	29.1.1	eval, filesystem, environment	`+76`	43.8 MB	kul
@typescript-eslint/parser	6.7.0	None	`+53`	42.9 MB	jameshenry
@commander-js/extra-typings	11.0.0	None	`+1`	224 kB	shadowspawn
protobufjs-cli	1.1.2	eval, filesystem, environment	`+64`	12.7 MB	google-wombot
webpack-dev-server	4.15.1	network, shell, environment	`+200`	15.3 MB	evilebottnawi
react	18.2.0	environment	`+2`	337 kB	gnoff
webpack-cli	4.10.0	environment	`+47`	7.46 MB	evilebottnawi
webpack	5.88.2	None	`+31`	6.65 MB	thelarkinn
react-dom	18.2.0	environment	`+4`	4.93 MB	gnoff
ts-loader	9.4.4	filesystem	`+45`	47.9 MB	johnnyreilly
style-loader	3.3.3	None	`+32`	6.71 MB	evilebottnawi
css-loader	5.2.7	None	`+37`	7.1 MB	evilebottnawi
commander	11.0.0	environment	`+0`	176 kB	abetomo
eslint-plugin-react	7.33.2	None	`+62`	4.76 MB	ljharb
eslint-plugin-prettier	5.0.0	None	`+84`	3.16 MB	jounqin
eslint-config-standard-with-typescript	39.0.0	None	`+85`	47.2 MB	mightyiam
typescript	5.2.2	filesystem	`+0`	40.6 MB	typescript-bot

🚮 Removed packages: css-loader@5.1.3, protobufjs@6.11.3, style-loader@2.0.0, vue@2.6.12, webpack@5.76.0, webpack-cli@4.5.0

src/core/blocklists.ts

src/finch_tracker/node_utils.ts

src/core/study_processor.ts

src/core/version.ts

src/web/src/app.tsx

src/core/study_processor.ts

petemill

This is great and I only have minimal feedback. I haven't run it with data yet because I don't know the params to give to npm run tracker. Could you provide them please and I'll look more at the UI?

In the meantime, if you want to use less CSS, less bootstrap and more Brave UI style, you might benefit from using the design system which exports react UI Components as well as css variables for colors / fonts / effects. The repo is at https://github.com/brave/leo and preview of all the components is at https://leo.bravesoftware.com/. You could simply use the Button, Toggle and Input components, if helpful.

petemill · 2023-09-14T04:44:24Z

src/core/core_utils.ts

@@ -0,0 +1,88 @@
+// Copyright (c) 2023 The Brave Authors. All rights reserved.


nit: instead of using a utils file, consider either splitting to more specific-named files or perhaps even recognising that these are all similarly related and perhaps calling this config_params.ts or maybe just params.ts?

Done, split the file.

src/core/study_processor.ts

src/finch_tracker/finch_tracker_main.ts

petemill · 2023-09-14T05:02:38Z

src/finch_tracker/node_utils.ts

+
+export function getStudyPath(storageDir: string): string {
+  return path.join(storageDir, 'study');
+}


does getSeedPath and getStudyPath belong in your (currently named) core_utils file?

They are only for tracker, not relevant for web/core code.

src/web/src/app.tsx

src/web/src/models.ts

petemill · 2023-09-14T05:42:24Z

src/web/src/app.tsx

+    .get(currentSeed)
+    ?.studies(filter)
+    .map((study, i) => (
+      <StudyItem key={study.name() + i} study={study} filter={filter} />


If study.name is guaranteed to be unique, then it's not best practice to use i in the key as the re-use of elements won't be as optimized. But if items never get added or removed then it's irrelevant anyway.

Sadly, study.name is not unique. There is not good key for the list.
Also, we load the lists once, so we could use index & seedType as key.

src/web/src/app.tsx

petemill · 2023-09-14T05:47:09Z

src/web/src/app.tsx

+  const studyList = props.studies
+    .get(currentSeed)
+    ?.studies(filter)


This should also be memoized if we're going to memoize in our children, in order to make sure each study object is ===.

Done. Could you take a look?
I have a doubt that useMemo() work well for filter object that recreated each time we render.
Check in devtools that it's not a hot path.

petemill · 2023-09-14T06:31:45Z

I also created this PR based on this branch which starts webpack's dev web server and enables hot reloading of the UI. Also moves typescript compilation for the web project to webpack. #744

atuchin-m · 2023-09-14T06:35:16Z

This is great and I only have minimal feedback. I haven't run it with data yet because I don't know the params to give to npm run tracker. Could you provide them please and I'll look more at the UI?

Thanks for the feedback, @petemill.
To run the frontend: npm run build in 'srcthenpython3 -m http.serverinsrc/webAlso there is some issue with API access from localhost. Useuse-local-apibranch in this repo to bypass. To run the CI job usenpm run tracker -- /tmp/somedir`. Also https://github.com/brave/finch-data-private/blob/main/.github/workflows/finch-tracker.yml

In the meantime, if you want to use less CSS, less bootstrap and more Brave UI style, you might benefit from using the design system which exports react UI Components as well as css variables for colors / fonts / effects. The repo is at https://github.com/brave/leo and preview of all the components is at https://leo.bravesoftware.com/. You could simply use the Button, Toggle and Input components, if helpful.

Thanks, I will take a look. The idea to reuse leo repo sounds good to me. Although, the main purpose of this PR is to start collecting Finch data to the private repo and send notifications. So maybe we need another iteration for that.

github-actions · 2023-09-14T08:52:49Z

src/web/app/app.tsx

+      className={props.className}
+      target="blank"
+      rel="noreferrer"
+      href={props.feature.link}


_{reported by reviewdog 🐶}
[semgrep] Detected a variable used in an anchor tag with the 'href' attribute. A malicious actor may be able to input the 'javascript:' URI, which could cause cross-site scripting (XSS). It is recommended to disallow 'javascript:' URIs within your application.

Source: https://semgrep.dev/r/typescript.react.security.audit.react-href-var.react-href-var

Cc @thypon @bcaller

In fact, this could be only https:// URL. How to fix the warning?

function sanitizeUrl(url) { try { const ALLOWED_PROTOCOLS = ['http:', 'https:']; const trimmedUrl = url.trim(); return ALLOWED_PROTOCOLS.includes(new URL(trimmedUrl, document.baseURI).protocol) ? trimmedUrl : '#'; } catch(err) { return '#'; } }

Wrapping the link in a sanitizeUrl should do the trick, here

src/finch_tracker/tracker_lib.ts

atuchin-m · 2023-09-20T20:35:00Z

@thypon Anything from your side? The files were moved a bit.

src/web/app/app.tsx

src/web/app/seed_loader.ts

petemill

Nice. Just a couple minor suggestions that are up to you!

thypon · 2023-09-22T11:53:04Z

src/web/app/app.tsx

+      className={props.className}
+      target="blank"
+      rel="noreferrer"
+      href={props.feature.link}


function sanitizeUrl(url) { try { const ALLOWED_PROTOCOLS = ['http:', 'https:']; const trimmedUrl = url.trim(); return ALLOWED_PROTOCOLS.includes(new URL(trimmedUrl, document.baseURI).protocol) ? trimmedUrl : '#'; } catch(err) { return '#'; } }

Wrapping the link in a sanitizeUrl should do the trick, here

src/web/app/app.tsx

atuchin-m self-assigned this Aug 30, 2023

atuchin-m force-pushed the implement-finch-tracker branch from 48a0ccd to 1466ccd Compare August 30, 2023 15:00

atuchin-m marked this pull request as ready for review September 1, 2023 13:02

github-actions bot reviewed Sep 4, 2023

View reviewed changes

src/core/blocklists.ts Show resolved Hide resolved

src/core/blocklists.ts Show resolved Hide resolved

src/finch_tracker/node_utils.ts Show resolved Hide resolved

src/finch_tracker/node_utils.ts Show resolved Hide resolved

github-actions bot added the needs-security-review label Sep 4, 2023

github-actions bot assigned bcaller and thypon Sep 4, 2023

atuchin-m requested review from iefremov, boocmp and petemill September 8, 2023 10:01

thypon mentioned this pull request Sep 12, 2023

Allowlist slackapi in https://github.com/brave/security-action/blob/main/assets/semgrep_rules/services/brave-third-party-action-not-pinned-to-commit-sha.yaml brave/security-action#332

Closed

2 tasks

boocmp reviewed Sep 12, 2023

View reviewed changes

src/core/study_processor.ts Outdated Show resolved Hide resolved

src/core/version.ts Outdated Show resolved Hide resolved

src/web/src/app.tsx Outdated Show resolved Hide resolved

boocmp reviewed Sep 12, 2023

View reviewed changes

src/core/study_processor.ts Show resolved Hide resolved

petemill reviewed Sep 14, 2023

View reviewed changes

github-actions bot reviewed Sep 14, 2023

View reviewed changes

atuchin-m force-pushed the implement-finch-tracker branch from 7f6a1e2 to 03d43da Compare September 14, 2023 12:56

boocmp approved these changes Sep 15, 2023

View reviewed changes

atuchin-m force-pushed the implement-finch-tracker branch from 09070d3 to 09bec21 Compare September 16, 2023 09:42

github-actions bot reviewed Sep 20, 2023

View reviewed changes

src/finch_tracker/tracker_lib.ts Show resolved Hide resolved

petemill reviewed Sep 21, 2023

View reviewed changes

src/web/app/app.tsx Outdated Show resolved Hide resolved

petemill approved these changes Sep 21, 2023

View reviewed changes

src/web/app/app.tsx Outdated Show resolved Hide resolved

src/web/app/app.tsx Outdated Show resolved Hide resolved

src/web/app/seed_loader.ts Outdated Show resolved Hide resolved

src/web/app/seed_loader.ts Outdated Show resolved Hide resolved

petemill approved these changes Sep 21, 2023

View reviewed changes

This comment was marked as duplicate.

Sign in to view

thypon requested changes Sep 22, 2023

View reviewed changes

atuchin-m added 2 commits September 26, 2023 15:02

Add finch_parser

3074129

Fix classifying

7289520

atuchin-m added 20 commits September 26, 2023 15:02

Add jest to package.json

bebf666

Add ts-jest to package.json

daf5111

Change jest.config.js

62b7b98

Add summary tests

b72ea18

Notify about parsing error, cut archived

76ef00f

Fix directory structure

97b952e

Split models

7d00f61

Add study_models.tests.ts

0ae1407

Fix seed loading

95f135b

Reject for non HTTP 200

d335757

Better sourcegraph regexs

5718084

Control_ => Control

b44fef2

Update README.md

909f5c2

Add comments, use -1 version for Brave

63b8194

Add src/README.md

56976cf

Show min/max version

5b01a85

Sanitize study.name

e25e06d

Improve killswitch detection

6a784fe

Add sanitizeUrl

1b77433

Fix review issues

efc6892

atuchin-m force-pushed the implement-finch-tracker branch from 22ac8d5 to efc6892 Compare September 26, 2023 08:02

atuchin-m requested a review from thypon September 26, 2023 08:02

bcaller reviewed Sep 26, 2023

View reviewed changes

src/web/app/app.tsx Outdated Show resolved Hide resolved

Fix sanitizeUrl()

a8aa3b8

thypon approved these changes Sep 26, 2023

View reviewed changes

thypon removed the needs-security-review label Sep 26, 2023

thypon unassigned thypon and bcaller Sep 26, 2023

atuchin-m merged commit 30f5653 into main Sep 26, 2023
5 checks passed

atuchin-m deleted the implement-finch-tracker branch September 26, 2023 11:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement finch tracker #732

Implement finch tracker #732

atuchin-m commented Aug 30, 2023 •

edited

Loading

socket-security bot commented Aug 30, 2023 •

edited

Loading

petemill left a comment

petemill Sep 14, 2023

atuchin-m Sep 14, 2023

petemill Sep 14, 2023

atuchin-m Sep 14, 2023

petemill Sep 14, 2023

atuchin-m Sep 14, 2023

petemill Sep 14, 2023

atuchin-m Sep 14, 2023

petemill commented Sep 14, 2023

atuchin-m commented Sep 14, 2023 •

edited

Loading

github-actions bot Sep 14, 2023

atuchin-m Sep 20, 2023

thypon Sep 22, 2023

atuchin-m commented Sep 20, 2023

petemill left a comment

This comment was marked as duplicate.

thypon Sep 22, 2023

		@@ -0,0 +1,88 @@
		// Copyright (c) 2023 The Brave Authors. All rights reserved.

Implement finch tracker #732

Implement finch tracker #732

Conversation

atuchin-m commented Aug 30, 2023 • edited Loading

socket-security bot commented Aug 30, 2023 • edited Loading

petemill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petemill commented Sep 14, 2023

atuchin-m commented Sep 14, 2023 • edited Loading

github-actions bot Sep 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atuchin-m commented Sep 20, 2023

petemill left a comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

Choose a reason for hiding this comment

atuchin-m commented Aug 30, 2023 •

edited

Loading

socket-security bot commented Aug 30, 2023 •

edited

Loading

atuchin-m commented Sep 14, 2023 •

edited

Loading