Skip to content

update external rewrite destination in pages-router #723

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

Closed

Conversation

dario-piotrowicz
Copy link
Contributor

the current url used for the pages-router external rewrite (https://opennext.js.org/share.png) errors when being fetched from a Cloudflare worker, the error being:
Screenshot 2025-02-03 at 16 21 51

To solve that the url has been updated to a github link (https://github.com/raw/opennextjs/docs/refs/heads/main/public/share.png) which points to the same exact image


I had to make this change in the Cloudflare repo and I am replicating the same change here to keep our tests in sync, see: original conversation

Copy link

changeset-bot bot commented Feb 3, 2025

⚠️ No Changeset found

Latest commit: cf35bbf

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

the current url used for the pages-router external rewrite (https://opennext.js.org/share.png)
errors when being fetched from a Cloudflare worker, to solve that the url has been updated
to a github link (https://github.com/raw/opennextjs/docs/refs/heads/main/public/share.png)
which points to the same exact image
@dario-piotrowicz dario-piotrowicz force-pushed the dario/pages-router-cloudflare-img-rewrite branch from db0c436 to cf35bbf Compare February 3, 2025 18:35
Copy link
Contributor

github-actions bot commented Feb 3, 2025

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 23.96% 1922 / 8020
🔵 Statements 23.96% 1922 / 8020
🔵 Functions 51.15% 111 / 217
🔵 Branches 71.23% 483 / 678
File CoverageNo changed files found.
Generated in workflow #925 for commit cf35bbf by the Vitest Coverage Report Action

Copy link
Contributor

@conico974 conico974 left a comment

Choose a reason for hiding this comment

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

LGTM Thanks!
Just one thing about that issue, does it mean that external rewrites to website hosted on cloudflare would fail with this error on workers ? If that's the case it should probably be added to the docs

@vicb
Copy link
Contributor

vicb commented Feb 3, 2025

My understanding is that it fails only if the workers are in the same data center - not saying it's much better, only giving some context.
Maybe something to investigate on our side, and agreed that this should be documented

@dario-piotrowicz
Copy link
Contributor Author

@conico974 that's a good point, I also tried reproducing this with a simple worker but there this issue doesn't seem to appear, which makes me think that this might be related on how Next.js rewrites work 🤔

shall I create a new github issue to track this?

@conico974
Copy link
Contributor

If the issue is with OpenNext yeah you should create an issue here.
The rewrite itself is happening here https://github.com/opennextjs/opennextjs-aws/blob/main/packages/open-next/src/overrides/proxyExternalRequest/fetch.ts
We pass down every header from the request, maybe it needs to be filtered (that's something that needs to be done on aws)

@vicb
Copy link
Contributor

vicb commented Feb 3, 2025

This is definitely a cloudflare infra limitation.
You can google for "DNS points to prohibited IP" or "you've requested a page on a website that is on the cloudflare network".
@IgorMinar knows more - it would be interesting to know if we plan on fixing that.

@dario-piotrowicz
Copy link
Contributor Author

@vicb I wasn't saying that it's not a Cloudflare infra limitation, I was just saying that it doesn't always happen (e.g. I can fetch the image from a worker) and something in Next.js/OpenNext is triggering it, so it would be good to investigate that and understand specifically what is triggering it and if anything can be changed to address it (maybe not, but it'd still might be worth an investigation)

@conico974
Copy link
Contributor

conico974 commented Feb 3, 2025

@dario-piotrowicz Are you adding header while fetching the image ? This kind of rewrite does not touch Next code
https://developers.cloudflare.com/support/troubleshooting/cloudflare-errors/troubleshooting-cloudflare-1xxx-errors/#error-1000-dns-points-to-prohibited-ip

  • The request X-Forwarded-For header is longer than 100 characters.
  • The request includes two X-Forwarded-For headers.
  • The request includes a CF-Connecting-IP header.

Might be one of those, other than that i don't see anything that could cause this kind of issue in OpenNext code

@vicb
Copy link
Contributor

vicb commented Feb 4, 2025

@vicb I wasn't saying that it's not a Cloudflare infra limitation, I was just saying that it doesn't always happen (e.g. I can fetch the image from a worker) and something in Next.js/OpenNext is triggering it, so it would be good to investigate that and understand specifically what is triggering it and if anything can be changed to address it (maybe not, but it'd still might be worth an investigation)

Mostly what I said in my first message :) inter colo works fine.

@dario-piotrowicz
Copy link
Contributor Author

@dario-piotrowicz Are you adding header while fetching the image ? This kind of rewrite does not touch Next code developers.cloudflare.com/support/troubleshooting/cloudflare-errors/troubleshooting-cloudflare-1xxx-errors#error-1000-dns-points-to-prohibited-ip

  • The request X-Forwarded-For header is longer than 100 characters.
  • The request includes two X-Forwarded-For headers.
  • The request includes a CF-Connecting-IP header.

Might be one of those, other than that i don't see anything that could cause this kind of issue in OpenNext code

No I'm not doing any of that 😕, this comes straight from copy-pasting the pages-router app from your repo: https://github.com/opennextjs/opennextjs-cloudflare/blob/e2918eb0bf7250884f41eac9eb0b6e751a7a9079/examples/e2e/pages-router/next.config.ts#L45-L46

@conico974
Copy link
Contributor

@dario-piotrowicz i took a quick look, and as i suspected cloudflare sets CF-Connecting-IP on the request, and since we are forwarding all headers on the rewrite this is causing the issue.
Filtering this headers should do the trick :


Maybe it's safer to just remove any cf-* header ?

@dario-piotrowicz
Copy link
Contributor Author

@conico974 you're a legend! 🚀 🚀 🚀 ❤️ 🚀🚀 🚀

This does fix the issue:
Screenshot 2025-02-04 at 10 39 14


Maybe it's safer to just remove any cf-* header ?

I think we can, but since CF-Connecting-IP is the only one triggering this issue it might be ok just to remove that one (and we can remove more or all cf-* if/when get issues about things not working)

@dario-piotrowicz
Copy link
Contributor Author

@conico974 what shall we do here? shall I close the PR? or update it to filer out the header? (PS: I am happy to do that but I then don't know how to test this, maybe testing it in the Cloudflare repo will be enough? (since there I would revert back the url change))

@conico974
Copy link
Contributor

It's probably cleaner to make a new PR for the header, as for the test we can't really do e2e test here for that because the fetch proxy don't really work on node.
You could test it with unit test it in here https://github.com/opennextjs/opennextjs-aws/tree/main/packages/tests-unit

@dario-piotrowicz
Copy link
Contributor Author

It's probably cleaner to make a new PR for the header, as for the test we can't really do e2e test here for that because the fetch proxy don't really work on node. You could test it with unit test it in here main/packages/tests-unit

Cool, I'm on it 🫡

@dario-piotrowicz
Copy link
Contributor Author

Closing as per the above comments

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.

3 participants