Skip to content
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

Wai Parse FilenameTooLong error incorrectly thrown for too long param name #1001

Open
moll opened this issue Aug 26, 2024 · 2 comments
Open

Comments

@moll
Copy link

moll commented Aug 26, 2024

Hey,

I think the FilenameTooLong error in Wai Parse is incorrectly thrown for a too long key name, not the file name.

Just (mct, name, Just filename) -> do
case prboKeyLength o of
Just maxKeyLength ->
when (S.length name > maxKeyLength) $
E.throwIO $
FilenameTooLong name maxKeyLength
Nothing -> return ()

The name length check mirrors the one just beneath the other Just (_ct, name, Nothing) case and that's for the parameter name. There's no code right now that checks the file name length, so the error itself seems unused.

@moll
Copy link
Author

moll commented Aug 26, 2024

Now that I'm looking at it, the UrlEncoded variant of conduitRequestBodyEx doesn't check the parameter name lengths at all. Either both should or neither should. The current method of just checking param name lengths of multipart forms is odd. Doesn't gain nor protect much. I think just checking the header line length is sufficient to protect against metadata (such as param name, file name) resource abuse. Individual param name and filename length checks should be left to business logic.

@Vlix
Copy link
Contributor

Vlix commented Oct 29, 2024

Good point. That logic is obviously conflating two separate things.

Let's see if anyone else has strong opinions about this. I personally also feel a "header length" check is sufficient.
I also don't really get why this exists. This module is only imported in RequestLogger and then only a handful of things. I wonder if anyone would directly use this module 🤔

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

No branches or pull requests

3 participants
@moll @Vlix and others