-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
DoS vulnerability: adapter-node crashes with malformed URI #5090
Comments
Any feedback on this? This defect exposes a trivial DoS vulnerability, so I think it warrants some attention. As I mentioned above, I'd be happy to work on a PR to address this if someone can advise me on how to get tests working. For others experiencing this issue in production, I wanted to share how we are mitigating this until it is fixed in the adapter: |
I can't reproduce this with the actual production output with the Node adapter (what you would get when running If this is only affecting |
Ah, I see there were two different requests mentioned in the OP. Yes, |
@Conduitry good point – I had not noticed the difference between |
The diff --git a/packages/kit/src/core/preview/index.js b/packages/kit/src/core/preview/index.js
index 90adb401..2ff58e4d 100644
--- a/packages/kit/src/core/preview/index.js
+++ b/packages/kit/src/core/preview/index.js
@@ -78,7 +78,7 @@ export async function preview({ port, host, https: use_https = false }) {
(req, res, next) => {
const original_url = /** @type {string} */ (req.url);
- const { pathname } = new URL(original_url, 'http://dummy');
+ const { pathname } = new URL('http://dummy' + original_url);
if (pathname.startsWith(base)) {
next();
@@ -106,7 +106,8 @@ export async function preview({ port, host, https: use_https = false }) {
return;
}
- const { pathname } = new URL(/** @type {string} */ (req.url), 'http://dummy');
+ const original_url = /** @type {string} */ (req.url);
+ const { pathname } = new URL('http://dummy' + original_url);
// only treat this as a page if it doesn't include an extension
if (pathname === '/' || /\/[^./]+\/?$/.test(pathname)) { The more serious issue - the one with the built production app - is coming from an exception thrown at
Presumably we should be trying to catch an error here and just returning a 400 instead. |
Makes sense! I'd be happy to submit a PR for this, but I'd prefer to first see a failing test first & then make it pass. Any insight on how to get tests working for |
preview was recently rewritten and doesn't seem to be crashing with the latest when I do |
Hi @benmccann – just tested No change when running with |
Nothing is crashing for me with the latest versions of everything, either in preview or production, for |
Verified based on latest versions – LGTM 👍. |
Describe the bug
Using the latest version of SvelteKit and adatper-node, a malformed URI causes the
node
process to crash.We recently encountered this in production on the tradingstrategy.ai.
We implemented a work-around by creating a
server.js
wrapper and calling thehandler
function exported by adapter-node. We first tried wrappinghandler
in atry/catch
… this didn't work / the server still crashed. We ended up manually callingdecodeURI()
in atry/catch
to detect the error before callinghandler
.I am open to working on a PR for this. There appears to be a test that would catch this issue, but the test setup appears to be broken. If you can advise me on how to get tests working, I can confirm that this test fails and work on a solution.
Reproduction
This is trivial to reproduce with a vanilla SvelteKit app using adapter-node, so in lieu of a repro repo, here are simple steps to recreate…
1. Create vanilla SvelteKit app with adapter-node
Select the most vanilla options:
Skeleton
,None
,No
,No
,No
2. Build and preview
3. Hit it with some malformed URIs
3.a. Simple invalid URI
See server logs below.
3.b. Invalid encoded value
See server logs below.
Logs
System Info
Severity
serious, but I can work around it
Additional Information
This seems to be a regression of something that worked in the past. See:
The text was updated successfully, but these errors were encountered: