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

feat: add gateway forbidden content types #46

Closed
wants to merge 2 commits into from

Conversation

vasco-santos
Copy link
Contributor

This PR adds forbidden content types to avoid abuse as we were seeing ~28% of traffic with bin files from last 12 hours https://dash.cloudflare.com/fffa4b4363a7e5250af8357087263b3a/nftstorage.link/analytics/traffic?content-type=bin

This can also be problematic for gateway being blocked by other parties and we should investigate other types to block.

CF may still just put bin on other kind of types if does not know about. Anyway, we should just block octet-stream to start. This should likely evolve into a wrangler secret / Admin feature.

@vasco-santos vasco-santos force-pushed the feat/add-gateway-forbidden-content-types branch from e1a080b to 00f0d80 Compare April 21, 2022 16:37
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Apr 21, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 21b9408
Status: ✅  Deploy successful!
Preview URL: https://fb665b9d.nftstorage-link.pages.dev

View logs

@vasco-santos vasco-santos requested a review from alanshaw April 21, 2022 16:40
@alanshaw
Copy link

I'm not sure we should block content simply because we see high traffic usage...

@vasco-santos
Copy link
Contributor Author

I'm not sure we should block content simply because we see high traffic usage...

I don't have a strong opinion. But, per recent meetings regarding gateway blockages one of the things we have been talking about is this. We will also consider blocking websites with JS and add other headers in the future.

Let's gather feedback from @dchoi27

@dchoi27
Copy link
Contributor

dchoi27 commented Apr 25, 2022

We're running our gateway as a public service for NFTs. For file extensions that are clearly not being used for NFT content (the referrers of this content are giant movie pirating pages), and that has small risk of false positives, we should block. If folks later start making NFTs with this sort of file extension, we can figure out how to be more precise. But we want to prevent early on being entrenched as a solution for pirates.

@dchoi27
Copy link
Contributor

dchoi27 commented Apr 25, 2022

Looking at the data, there was also a drop in cache hit rate around when the traffic spiked for this type of content, meaning that the usage of the gateway for these purposes is adversely affecting our core users.

@alanshaw
Copy link

We're running our gateway as a public service for NFTs. For file extensions that are clearly not being used for NFT content (the referrers of this content are giant movie pirating pages), and that has small risk of false positives, we should block. If folks later start making NFTs with this sort of file extension, we can figure out how to be more precise. But we want to prevent early on being entrenched as a solution for pirates.

ACK, this makes sense - thanks for the clarification.

packages/edge-gateway/src/gateway.js Outdated Show resolved Hide resolved
packages/edge-gateway/src/errors.js Outdated Show resolved Hide resolved
`Forbidden content type: ${winnerGwResponse.response.headers.get(
'content-type'
)}`
)

Choose a reason for hiding this comment

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

We need to do this check before the waitUntil above don't we? Otherwise we cache the response and on subsequent requests it'll be served from the cache.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah good catch

@vasco-santos vasco-santos force-pushed the feat/add-gateway-forbidden-content-types branch from e2ff7f5 to 21b9408 Compare April 27, 2022 08:20
@vasco-santos vasco-santos requested a review from alanshaw April 27, 2022 10:03
@mikeal
Copy link

mikeal commented Apr 27, 2022

It’s very aggressive to block all binary content type returns. I also don’t think it’ll work all that well since they could change this to any other content type pretty easily, they obviously aren’t depending on the content type for any rendering since they are getting back as a binary stream.

Digging into this a bit with @vasco-santos, I think that we should exhaust our other content blocking options (referrers, user-agents, etc) before resorting to this. We’re losing referrers right now on redirect to subdomains so we need to push a fix for that before we’ll have good data to work with.

@vasco-santos
Copy link
Contributor Author

Closing this for now. Follow up fixing referers #68 and we will rely on CF Firewall for blocking

@vasco-santos vasco-santos deleted the feat/add-gateway-forbidden-content-types branch April 27, 2022 17:19
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.

4 participants