Skip to content

feat: adding google malware detection#36

Merged
vasco-santos merged 4 commits intomainfrom
adding-google-malware-detection
Sep 12, 2022
Merged

feat: adding google malware detection#36
vasco-santos merged 4 commits intomainfrom
adding-google-malware-detection

Conversation

@jsdevel
Copy link
Copy Markdown
Contributor

@jsdevel jsdevel commented Aug 28, 2022

  • Test ipfs malware sample from google with lookup and evaluate.

@jsdevel jsdevel force-pushed the adding-google-malware-detection branch from 0fe2ded to 1b9c179 Compare August 28, 2022 08:13
@jsdevel jsdevel changed the title Adding google malware detection. feat: Adding google malware detection. Aug 28, 2022
@jsdevel jsdevel changed the title feat: Adding google malware detection. feat: adding google malware detection Aug 28, 2022
@jsdevel jsdevel force-pushed the adding-google-malware-detection branch from 1b9c179 to e6f4227 Compare August 28, 2022 08:14
Copy link
Copy Markdown
Contributor

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

Thanks for the first iteration @jsdevel

Let's please create a package from the start as previously talked and specified in https://hackmd.io/Y1vEmm9qTBmQU9Wzc3PY2w. Once we hook up tests, KV to store results / KV Locker it will be a bunch of logic that should be isolated. Also, we will want the logs for metrics + trigger badbits mail

For the package, we want a worker quite similar to this one with:

  • wrangler.toml
  • index.js
  • cors.js
  • error-handler.js
  • env.js

Comment thread packages/edge-gateway/src/gateway.js Outdated
Comment thread packages/edge-gateway/src/gateway.js Outdated
Comment thread packages/edge-gateway/src/gateway.js Outdated
Copy link
Copy Markdown
Contributor

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

Thanks for the new iteration Joe. Added some initial feedback even though I know this is still WIP 😉

Comment thread packages/cid-verifier/scripts/cli.js Outdated
Comment thread packages/cid-verifier/scripts/cli.js Outdated
Comment thread packages/cid-verifier/scripts/heartbeat.js Outdated
Comment thread packages/cid-verifier/package.json Outdated
Comment thread packages/cid-verifier/src/bindings.d.ts Outdated
Comment thread packages/cid-verifier/src/bindings.d.ts Outdated
Comment thread packages/cid-verifier/src/env.js Outdated
Comment thread packages/cid-verifier/src/env.js Outdated
@jsdevel jsdevel force-pushed the adding-google-malware-detection branch 2 times, most recently from 9f4a736 to c3dd17c Compare September 1, 2022 09:13
Copy link
Copy Markdown
Contributor

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

Thanks for the new iteration. Glad things are shaping up. A couple of general comments I could not have a place to add in review:

  • github actions workflow for cid verifier package (can be based on edge-gateway
  • CI is having a bad time, looks like pnpm lockfile should be commited? perhaps run on root dir pnpm clean && pnpm i

Also, this is first set of comments and I did not get into the actual code for CID verification. Just all the boilerplate around

Comment thread packages/cid-verifier/package.json Outdated
Comment thread packages/cid-verifier/src/bindings.d.ts Outdated
Comment thread packages/cid-verifier/src/bindings.d.ts Outdated
Comment thread packages/cid-verifier/src/bindings.d.ts Outdated
Comment thread packages/cid-verifier/src/bindings.d.ts Outdated
Comment thread packages/cid-verifier/package.json Outdated
Comment thread packages/edge-gateway/wrangler.toml Outdated
Comment thread packages/edge-gateway/wrangler.toml Outdated
Comment thread packages/edge-gateway/src/utils/deny-list.js
Comment thread packages/edge-gateway/package.json Outdated
Copy link
Copy Markdown
Contributor

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

Extra thoughts, we should be good after this. Good job

Comment thread packages/cid-verifier/wrangler.toml Outdated
Comment thread packages/cid-verifier/wrangler.toml Outdated
Comment thread packages/cid-verifier/wrangler.toml Outdated
Comment thread packages/cid-verifier/wrangler.toml Outdated
Comment thread packages/cid-verifier/test/utils/setup.js Outdated
Comment thread packages/cid-verifier/src/verification.js Outdated
Comment thread packages/cid-verifier/src/verification.js Outdated
Comment thread packages/cid-verifier/src/verification.js Outdated
Comment thread packages/cid-verifier/src/verification.js Outdated
Comment thread packages/cid-verifier/src/verification.js Outdated
@jsdevel
Copy link
Copy Markdown
Contributor Author

jsdevel commented Sep 2, 2022

Thanks for the new iteration. Glad things are shaping up. A couple of general comments I could not have a place to add in review:

  • github actions workflow for cid verifier package (can be based on edge-gateway
  • CI is having a bad time, looks like pnpm lockfile should be commited? perhaps run on root dir pnpm clean && pnpm i

Also, this is first set of comments and I did not get into the actual code for CID verification. Just all the boilerplate around

i saw that error. i checked in the pnpm lock file. i think locally i'm using a later version of pnpm and that's causing it.

@jsdevel jsdevel force-pushed the adding-google-malware-detection branch 16 times, most recently from de4735a to 57a0953 Compare September 7, 2022 07:01
Comment thread packages/cid-verifier/package.json Outdated
Comment thread packages/cid-verifier/src/verification.js Outdated
@jsdevel jsdevel force-pushed the adding-google-malware-detection branch from 57a0953 to cc9b4f3 Compare September 7, 2022 22:20
@storacha storacha deleted a comment from vasco-santos Sep 7, 2022
@jsdevel
Copy link
Copy Markdown
Contributor Author

jsdevel commented Sep 8, 2022

New set of changes. Please also add README file, gather a copy from edge-gateway and basically need specific secrets for here specified there

i'll add a README once the approach is finalized

Comment thread packages/cid-verifier/src/verification.js
export interface EnvInput {
ENV: string;
DEBUG: string;
GOOGLE_EVALUATE_SAFE_CONFIDENCE_LEVELS: Array<string>;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i had this in case there were other levels we'd want to configure other than SAFE

Comment thread packages/cid-verifier/src/index.js Outdated
Comment on lines +20 to +21
.get('/verification', withCorsHeaders(verificationGet))
.post('/verification', withCorsHeaders(verificationPost))
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

we can go over this in our call

Comment thread packages/cid-verifier/src/verification.js
Comment thread packages/cid-verifier/src/verification.js Outdated
@jsdevel jsdevel force-pushed the adding-google-malware-detection branch 2 times, most recently from 8833a3f to 3960f33 Compare September 9, 2022 04:05
Comment thread packages/cid-verifier/src/verification.js
export interface EnvInput {
ENV: string;
DEBUG: string;
GOOGLE_EVALUATE_SAFE_CONFIDENCE_LEVELS: Array<string>;
Copy link
Copy Markdown
Contributor

@vasco-santos vasco-santos Sep 9, 2022

Choose a reason for hiding this comment

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

Ok, I got it thanks, this also needs to go to docs. And added in wrangler toml for other environments

Comment thread packages/cid-verifier/src/verification.js
Comment thread packages/cid-verifier/src/verification.js
Comment thread packages/cid-verifier/src/verification.js Outdated
Comment thread packages/cid-verifier/src/verification.js Outdated
Comment thread packages/cid-verifier/src/verification.js Outdated
Comment thread packages/edge-gateway/wrangler.toml Outdated
Comment thread packages/edge-gateway/wrangler.toml Outdated
Comment thread packages/cid-verifier/wrangler.toml Outdated
Comment thread packages/cid-verifier/src/verification.js Outdated
Comment thread packages/cid-verifier/src/verification.js Outdated
@jsdevel jsdevel force-pushed the adding-google-malware-detection branch 2 times, most recently from bd6d31c to 7d51262 Compare September 9, 2022 19:15
* Moving DENYLIST logic to cid-verifier from edge-gateway
@jsdevel jsdevel force-pushed the adding-google-malware-detection branch from 7d51262 to 65a107e Compare September 10, 2022 02:48
Copy link
Copy Markdown
Contributor Author

@jsdevel jsdevel left a comment

Choose a reason for hiding this comment

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

Hoping you can get this sample.html test to work.

t.is(await response.text(), 'Hello dot.storage! 😎')
})

test('Sends HTML files to cid-verifier', async (t) => {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@vasco-santos i'm having trouble getting this to work. can you take a look?

@vasco-santos vasco-santos force-pushed the adding-google-malware-detection branch from 6114aa7 to f3c6b84 Compare September 12, 2022 10:02
Copy link
Copy Markdown
Contributor

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

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.

2 participants