Fix localhost issue for server actions #589
Conversation
🦋 Changeset detectedLatest commit: e78007b The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
🧪 Prereleases are available for testing 🧪 @cloudflare/next-on-pagesYou can install this latest build in your project with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/next-on-pages/runs/7195370569/npm-package-next-on-pages-589@cloudflare/eslint-plugin-next-on-pagesYou can install this latest build in your project with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/next-on-pages/runs/7195370569/npm-package-eslint-plugin-next-on-pages-589 |
|
|
||
| make sure that server actions can work in `wrangler pages dev` | ||
|
|
||
| fix `wrangler pages dev` breaking server actions because of a misalignment between headers |
There was a problem hiding this comment.
Question: If this is only intended to be for wrangler pages dev, would it be problematic or introduce a security concern if someone deploys the build?
There was a problem hiding this comment.
It shouldn't introduce any problem/security concern, if it would then I don't think the change would be worth it and we could instead ask people to set an appropriate allowedOrigins for during local development (it could rely on an (no, next-on-pages works on production builds so it couldn't be applied conditionally 😓))..env.local file, or on process.env.NODE_ENV, making it, although still inconvenient, not too too terrible
It would be a bit quite cumbersome so I would prefer to try to avoid that if we could 😓
Regarding why I don't think it's introducing a risk, the fact is that by the code below you can see that I am updating a pretty specific line and I add a regex check that strictly changes logic only on hosts like: localhost:xxxx so I feel like it's not introducing any risks.
(then we also update a console.warn but even if the changes break that I don't think that's not the end of the world)
More than introducing security concerns I am a bit afraid that this is a bit brittle and can easily break... hopefully not but I guess we can only be sure by actually seeing how it performs in the wild 😅
There was a problem hiding this comment.
But if you see a security concern or something that looks problematic to you please let me know 🙏
(I think this could be quite important for local development but for sure, better to break local dev for users instead of breaking their production applications 😅)
There was a problem hiding this comment.
Hmm, I'm really not sure I like the idea of doing a modification to something that has a high chance of being deployed to production, when the modification is only supposed to be present in one single specific dev environment...
I really think that we need a way to determine if the user is using wrangler pages dev if we want to do this kind of hack. These kind of changes/hacks we've done in the past have always been intended to be shipped to production, whereas this change is only intended to be used in wrangler pages dev.
And since the Next.js PR for wildcards got merged, then surely it would be better to leave this to the user if they want to do this in their next.config.js instead. It might take a week for a new Next.js release, but to me that is a better choice than making modifications to their CSRF protections that would be shipped to prod when it shouldn't be @dario-piotrowicz
There was a problem hiding this comment.
As I mentioned allowedOrigins even when the wildcard change is released, is going to be pretty tedious and error prone in my opinion (maybe having a local address in the as an allowed origin in production is not going to be an issue, so people could just add it there and leave it... I am not really sure). Ideally I would really not want developers to have to deal with it themselves.
Regarding detecting if the build output is going to be used for wrangler pages dev I think that's not really a great idea based on how the adapter works. Since it just builds the worker for you, I don't think it should have any knowledge on how you are going to use that worker. Actually, I can imagine people building their worker with next-on-pages, previewing it with wrangler pages dev and then deploying it with wrangler pages deploy, so the same exact worker should work in both dev and production (and it should be the same worker since people are trying to validate it before the deployment, if we produced different workers then people could not actually accurately "preview" the actual production worker before deploying it).
I think it is very clear that modifying production code for the sake of local development is not ideal in the slightest, I just think that the changes here are so negligible (both in terms of performance and security) that it's worth it in this case given the DX gain.
There was a problem hiding this comment.
I just think that the changes here are so negligible (both in terms of performance and security)
But this is a limitation that Next.js specifically added for production builds to help prevent CSRF attacks, so I would really have to disagree that this is negligible. They wouldn't go through the effort to add this additional protection if there wasn't any value in it.
If we are modifying it to allow localhost:{num} to be bypass this protection in any deployment, then IMO that is a very slippery slope and could end up being consequential for some of our users.
Regarding detecting if the build output is going to be used for wrangler pages dev I think that's not really a great idea based on how the adapter works
I meant through something like an env variable existing from wrangler to dictate that this was in pages dev as a gate for when this hack works in their code. something like process.env.WRANGLER_DEV && /.../.test(...) ? null : origin, but i dont know if that is currently possible with wrangler. this would then give the desired behaviour in local dev, and the correct behaviour/protections in deployments.
There was a problem hiding this comment.
I meant through something like an env variable existing from wrangler to dictate that this was in pages dev as a gate for when this hack works in their code. something like
process.env.WRANGLER_DEV && /.../.test(...) ? null : origin, but i dont know if that is currently possible with wrangler. this would then give the desired behaviour in local dev, and the correct behaviour/protections in deployments.
ah ok, yeah that sounds like an interesting idea, I misunderstood what you were suggesting 🙂
There was a problem hiding this comment.
anyways thanks a lot for the insights 🙂
but as we were discussing this with @petebacondarwin, it sounds like maybe things can be updated in wrangler so to have it set a proper x-forwarded-host when passing requests from the proxy worker to the actual worker without any hacky changes needed in next-on-pages
so I'm going to close the PR for now and see if things can be changes in wrangler instead, if not we can revisit things here later 🙂
There was a problem hiding this comment.
ah nice, that actually sounds like a pretty good middleground!
dario-piotrowicz
force-pushed
the
server-actions-localhost
branch
from
52b526c to
dddb508
Compare
| /"string"==typeof (\w+)\.headers\.origin\?(new URL\(\1\.headers\.origin\)\.host):void 0,/; | ||
| if (contents.match(headerOriginRegex)) { | ||
| contents = contents.replace(headerOriginRegex, (fullMatch, _i, origin) => { | ||
| const originWithCheck = `(/^localhost:\\d{4}$/.test(${origin})?null:${origin})`; |
There was a problem hiding this comment.
Out of interest, why use null here when the original code is using void 0?
Better to be consistent?
There was a problem hiding this comment.
no reason actually, it's just what felt more natural to me 😅, since this is just changing the already built and minified code I didn't think it'd make much difference if it were consistent or not
| * the same as that of other Next.js-specific headers (this is a measure against CSRF attacks) | ||
| * | ||
| * This resulted problematic when trying to run the application with `wrangler pages dev` as the | ||
| * two hosts can get misaligned (one being `localhost:<port>` while the other `127.0.0.1:<port>`) |
There was a problem hiding this comment.
I kind of feel like this is a problem with wrangler pages dev no?
Why is there this misalignment? Could you expand on that?
There was a problem hiding this comment.
That's a totally valid point, I am not sure if anything can be changes in wrangler but I can check with the team
The problem is that this issue is caused by a fundamental design aspect of the startDevWorker changes
Basically wrangler pages dev gives you the proxy worker's address (e.g. http://localhost:8788) but the actual worker is running on a different port (and not on localhost) (e.g. http://127.0.0.1:51405/) so, when, with the browser you to go the address wrangler pages dev provides you and try to make a request, the host from the browser (which is the proxy worker's address) is different from the actual worker's address (which is not really visible by the user) and that's what causing the misalignment.
So if in the browser I were to go to the actual worker's address (e.g. http://127.0.0.1:51405/server-actions/simple-kv-forms) things would just work as expected, the issues there are that:
- the address is not provided to you by wrangler (the way I find it is by going to the standard proxy address, trying to use actions, checking the terminal error and picking up the address from that)
- the address is not stable and (as far as I know) changes whenever the worker code changes (so people rebuilding the worker on the background wouldn't have a stable address to use but would have to use a different address each time)
dario-piotrowicz
force-pushed
the
server-actions-localhost
branch
from
efd409e to
e78007b
Compare
|
Closing this PR since, as discussed with @petebacondarwin, it sounds like maybe the proper fix for this issue needs to be applied in wrangler itself (having it set a proper |
resolves #588