fix: prevent use of local ips on webhooks #20487
Conversation
Marc-Roig
requested review from
innerdvations,
alexandrebodin and
derrickmehaffy
|
Let me know your opinion on this approach before I commit on some UI changes and api tests |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@derrickmehaffy I believe the fetch api returns a 500 by default if the port is not found, or at least guarantees the status code is present on the response. |
|
After a discussion with Alex , we decided to just prevent local ips to be used on webhook settings. |
|
I am now working on some api tests, but logic should be ready (thank you Remi! 🚀 ) |
packages/core/admin/admin/src/pages/Settings/pages/Webhooks/EditPage.tsx
Outdated
Show resolved
Hide resolved
Marc-Roig
changed the title
| .test( | ||
| 'is-public-url', | ||
| "Url is not supported because it isn't reachable over the public internet", | ||
| async (url) => { |
There was a problem hiding this comment.
Aren't we supposed to prevent this only in production env ?
There was a problem hiding this comment.
oops, yes! I added the if case
There was a problem hiding this comment.
🤔 I think we might want !== production in case we are doing tests for ex
Marc-Roig
added
source: core:admin
|
|
||
| try { | ||
| const parsedUrl = new URL(url!); | ||
| const isLocalUrl = await isLocalhostIp(parsedUrl.hostname); |
There was a problem hiding this comment.
To be completely safe you need to use punycode on the url first since is-localhost-ip doesn't support international character sets by default. Otherwise someone could, for example, use an internationalized domain name pointed at 127.0.0.1 to achieve the same effect.
|
This could be considered a breaking change, because there are probably users making use of this as a hack to call local scripts, so I think we should include a release note about the change with a link to the right way of doing it. |
|
Size Change: +18 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
Fix for
GHSA-v8wj-f5c7-pvxfPrevents local ips to be used on webhooks.
User will see the following error if trying to use one:
Notes
V5 migration should be straightforward on the BE side, but we will need to check on the FE side if errors are being displayed on the Webhooks settings page