feat: add goodbits #187

vasco-santos · 2022-08-31T12:55:13Z

Integrate goodbits into nftstorage.link.

Motivation

We introduced CSP #172 aiming to block phishing websites from operating. More than phishing websites, this measure brings to the table additional long term benefits:

prevent NFTs in the wild to have centralized dependencies instead of same origin content addressable content
ipfs:// browser handling will require all external sources to also rely on IPFS protocol (we can overcome these pains earlier on)

There are already occurrences on the wild of NFTs relying on primitives that are now blocked by default via CSP. We are shipping https://github.com/nftstorage/goodbits as a place where we can manually add these NFTs, in order to bypass CSP for them after manual validation.

Implementation details

Similarly to what we do in reads pipeline for denylist, a new KV is created for optimal access from CF Workers on the gateway operation.

This PR includes:

GOODBITS KV
Cron job that recurrently downloads goodbits list and updates KV with the new occurrences
gateway to bypass goodbits with appropriate tag to bypass CSP

Needs:

fix: require json objects web3-storage/goodbits#1

cloudflare-pages · 2022-08-31T13:01:47Z

Deploying with Cloudflare Pages

Latest commit:	`b8959b7`
Status:	✅ Deploy successful!
Preview URL:	https://bf4dd76b.nftstorage-link.pages.dev
Branch Preview URL:	https://feat-add-goodbits.nftstorage-link.pages.dev

View logs

olizilla

If it's blocking you, this could be merged as is, but I'd consider doing batching as you read the good bits list as an iterable, and fixing some of the minor issues raised.

olizilla · 2022-09-02T09:10:50Z

.github/workflows/cron-goodbits-sync.yml

+
+on:
+  schedule:
+    - cron: '13 0,5,10,15,20 * * *'


is this "every 5 hours at 13 mins past" to avoid the rush of doing things on the hour?

Kept same timings as we did for denylist nftstorage/nft.storage#1761

We can revisit later if we need more/less frequency. No strong opinions for now.

olizilla · 2022-09-02T09:13:53Z

.github/workflows/cron-goodbits-sync.yml

+    strategy:
+      matrix:
+        env: ['staging', 'production']
+    timeout-minutes: 20


does this job sometimes get stuck? why 20? seems like it should take a few seconds with a list of 1 thing, and may take hours with a list of 1 billion things.

no, this was kind of based on my actions template. But I will decrease it for 5 minutes for now

olizilla · 2022-09-02T09:20:47Z

packages/cron/src/jobs/goodbits.js

+  if (!res.ok) {
+    throw new Error(`unexpected status fetching goodbits list: ${res.status}`)
+  }
+  const list = ndjsonParser(await res.text())


y u no support your friendly neighbourhood software developers‽

https://www.npmjs.com/package/iterable-ndjson for modern iterable flavour goodness from alan

https://www.npmjs.com/package/ndjson the original streaming ndjson from contra & max

I didn't know about these packages :) Chose the one with better score in npms, but I will change to Alan's one

olizilla · 2022-09-02T09:23:00Z

packages/cron/src/jobs/goodbits.js

+
+  const kvNamespaces = wranglerEnvConfig.kv_namespaces || []
+  const goodbitsListKv = kvNamespaces.find(
+    (kv) => kv.binding === 'GOODBITSLIST'


nit: other uses of GOODBITS in this PR use an underscore separator e.g. GOODBITS_SOURCES

yeah, I went this naming to be consistent with KV for DENYLIST (no underscore there sadly)

packages/cron/src/jobs/goodbits.js

olizilla · 2022-09-02T09:49:04Z

packages/edge-gateway/src/gateway.js

@@ -24,7 +28,17 @@ export async function gatewayGet(request, env) {
    },
  })

-  return getTransformedResponseWithCustomHeaders(response)
+  // Validation layer - CSP bypass
+  const resourceCid = decodeURIComponent(response.headers.get('etag') || '')


could we store the "this is a good bit" on the cached response rather than having to check it in the KV every time? Like once a cid is added to the list it's very unlikely to be removed... It'd be a little more awkward, but might end up being more similar your proposal in #51 (comment)

We don't gain much with it, or am I missing something?

On the proposal from #51 , we want to purge from cache what we don't want to serve anymore to avoid checking Denylist before serving cached content. Here, we would add temporarily to cache, but given cache is LRU it would eventually leave cache. So, we always need to check KV anyway.

Given this is an optimization, if you have some angle that I am missing, can you please open an issue for us to discuss there?

olizilla · 2022-09-02T09:51:18Z

packages/edge-gateway/src/gateway.js

@@ -33,7 +47,7 @@ export async function gatewayGet(request, env) {
 *
 * @param {Response} response
 */
-function getTransformedResponseWithCustomHeaders(response) {
+function getTransformedResponseWithCspHeaders(response) {
  const clonedResponse = new Response(response.body, response)

  clonedResponse.headers.set(


maybe this is the plan for a future PR, but I feel like the list of allowed connect-src and maybe the CSP more generally should be driven by the tags in the goodbits list. Like "CID x needs to connect to url y etc.

We did not talk about this yet. I have that as a question mark on my head as well. We will figure out later as use cases appear :)

olizilla · 2022-09-02T09:53:10Z

packages/edge-gateway/src/gateway.js

+  }
+
+  // TODO: Remove once https://github.com/nftstorage/nftstorage.link/issues/51 is fixed
+  const goodbitsEntry = await pRetry(() => datastore.get(cid), { retries: 5 })


I think we are always going to need retries at this scale. Even if they fix some root issue, we will still see the occasional random failure that can be fixed by asking again. Pesky entropy.

olizilla · 2022-09-02T09:54:45Z

packages/edge-gateway/test/content security policy.spec.js

@@ -0,0 +1,18 @@
+import { test, getMiniflare } from './utils/setup.js'


I dunno man. I don't think us old developers are ready for filenames with spaces in them, even if the tooling is. 😊

loool, I took some time to realize the filename had spaces instead of underscores. Not sure what I did there

olizilla · 2022-09-02T09:56:51Z

packages/edge-gateway/test/index.spec.js

+  t.true(csp.includes("form-action 'self' ; navigate-to 'self';"))
+})
+
+test('Gets content with no csp header when goodbits csp bypass tag exists', async (t) => {


we should take a moment to see if there is a much more permissive CSP header we can provide rather than dropping it completely...

I guess we will think about that as other use cases rise in the goodbits and with tags. Until we spec a better solution, let's just keep the basics so that users with broken CIDs are not affected :)

codecov-commenter · 2022-09-02T11:49:31Z

Codecov Report

Merging #187 (b8959b7) into main (93d8688) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #187   +/-   ##
=======================================
  Coverage   98.98%   98.98%           
=======================================
  Files           4        4           
  Lines         491      491           
=======================================
  Hits          486      486           
  Misses          5        5

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

vasco-santos · 2022-09-02T12:29:59Z

Addressed most of the review. Merging this in so that we can actually starting to use the goodbits content and unblock comms around this. We can revisit and specify a spec for goodbits tags when we have better idea on what use cases can exist

vasco-santos force-pushed the feat/add-goodbits branch from 44d6a6c to 3153de2 Compare August 31, 2022 13:01

vasco-santos force-pushed the feat/add-goodbits branch 3 times, most recently from c7d2f5b to 0b91346 Compare August 31, 2022 13:56

feat: add goodbits

1509c4b

vasco-santos force-pushed the feat/add-goodbits branch from 0b91346 to 1509c4b Compare August 31, 2022 13:58

vasco-santos marked this pull request as ready for review August 31, 2022 14:01

vasco-santos requested a review from olizilla August 31, 2022 14:01

olizilla reviewed Sep 2, 2022

View reviewed changes

fix: address review

b8959b7

vasco-santos merged commit 9b8bae8 into main Sep 2, 2022

vasco-santos deleted the feat/add-goodbits branch September 2, 2022 12:30

This was referenced Sep 2, 2022

chore(main): release cron 1.1.0 #188

Merged

chore(main): release edge-gateway 1.14.0 #189

Merged

vasco-santos mentioned this pull request Nov 30, 2022

Move goodbits cron sync to reads pipeline web3-storage/reads#123

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add goodbits #187

feat: add goodbits #187

vasco-santos commented Aug 31, 2022 •

edited

cloudflare-pages bot commented Aug 31, 2022 •

edited

olizilla left a comment

olizilla Sep 2, 2022

vasco-santos Sep 2, 2022

olizilla Sep 2, 2022

vasco-santos Sep 2, 2022

olizilla Sep 2, 2022

vasco-santos Sep 2, 2022

olizilla Sep 2, 2022

vasco-santos Sep 2, 2022

olizilla Sep 2, 2022

vasco-santos Sep 2, 2022 •

edited

olizilla Sep 2, 2022

vasco-santos Sep 2, 2022

olizilla Sep 2, 2022

olizilla Sep 2, 2022

vasco-santos Sep 2, 2022

olizilla Sep 2, 2022

vasco-santos Sep 2, 2022

codecov-commenter commented Sep 2, 2022

vasco-santos commented Sep 2, 2022 •

edited

		@@ -0,0 +1,18 @@
		import { test, getMiniflare } from './utils/setup.js'

feat: add goodbits #187

feat: add goodbits #187

Conversation

vasco-santos commented Aug 31, 2022 • edited

Motivation

Implementation details

cloudflare-pages bot commented Aug 31, 2022 • edited

Deploying with Cloudflare Pages

olizilla 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

Choose a reason for hiding this comment

vasco-santos Sep 2, 2022 • edited

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

codecov-commenter commented Sep 2, 2022

Codecov Report

vasco-santos commented Sep 2, 2022 • edited

vasco-santos commented Aug 31, 2022 •

edited

cloudflare-pages bot commented Aug 31, 2022 •

edited

vasco-santos Sep 2, 2022 •

edited

vasco-santos commented Sep 2, 2022 •

edited