Prevent Server Errors from Malformed URL Requests

[yoni-noma]

Description
This PR ensures that the server properly handles malformed URL requests that cannot be decoded. By validating req.url with decodeURIComponent, we can catch decoding errors early and respond with a 400 Bad Request instead of allowing them to cause unexpected issues.

Test Plan
Start the server.

• Send a request with a properly encoded URL → Request should proceed as expected.
• Send a request with a malformed URL:

curl -X POST 'http://localhost:3000/hello.world?%ADd+allow_url_include%3d1+%ADd+auto_prepend_file%3dphp://input' \
-H 'Content-Type: application/x-www-form-urlencoded' \
-H 'User-Agent: Custom-AsyncHttpClient' \
--data '{}'

Server should log an error and respond with 400 Bad Request: Malformed URL.

• Ensure normal requests are unaffected.

[kentcdodds]

Just had an idea for an improvement. Thanks!

[kentcdodds]

Why don’t we use URL.canParse for this? https://developer.mozilla.org/en-US/docs/Web/API/URL/canParse_static

[yoni-noma]

ive tested both, URL.canParse returns true because it only checks if the URL is syntactically valid. It doesn’t validate percent-encoded sequences, so malformed encodings (e.g., %AD) pass even though decodeURIComponent would fail.

[kentcdodds]

After looking into this a little bit more, it looks like this is a problem with the logging library we use called Morgan. It looks like it is not able to handle this URL even though that is technically a valid URL. So I don't think we actually have a problem ourselves, and this doesn't seem like this is the right place for us to solve this problem. I think we could actually cause problems if we were to add this middleware that you've suggested.

Sorry for telling you to open up this pull request and not accepting it, but I do think that the right place to make this fix would be inside of Morgan (our logging tool) rather than inside of the Epic stack. While you're waiting for Morgan to add a fix for this, you would be perfectly fine adding this middleware yourself as long as it's not causing you trouble in other ways.

For more details, check out my live stream where I worked on reviewing this (at around 11 minutes): https://www.youtube.com/watch?v=x72lBF5C2hw

[kentcdodds]

To be clear, I think that we should probably open an issue on Morgan with a reproducible example and then potentially somebody can make a pull request for that.

[yoni-noma]

@kentcdodds Sure, no worries. Just to clarify, the line that tries to decodeURIComponent is inside the epic-stack server/index.ts at line 91:
morgan.token('url', req => decodeURIComponent(req.url ?? '')).

The failure seems to occur during the decoding phase. If Morgan only supports decoded URLs, I'm not sure where the fix should happen here or an improvement inside morgan.

Anyway, this crashes the app server. So, for anyone encountering this issue and not wanting to add middleware until they fix the issue, you can probably just wrap this callback in a try-catch.