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

Allow configuring query string max length #2485

Conversation

elct9620
Copy link
Contributor

@elct9620 elct9620 commented Nov 13, 2020

Description

Please describe your pull request. Thank you for contributing! You're the best.

According to #2273 discussion, I add an optional cflags to configure query string max length when we compile puma.

This change is reference @maleksiuk version (maleksiuk@1c56e5f) to keep source code clean, but I didn't put the environment variable detect part which I think isn't necessary when we have --cflags options.

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] or [ci skip] to the pull request title.
  • I have added appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

@maleksiuk
Copy link
Contributor

One problem with this, which my commit had too, is that the error message in the exception that gets raised has changed from

HTTP element QUERY_STRING is longer than the 1024*10 allowed length (was 900000)

to

HTTP element QUERY_STRING is longer than the PUMA_QUERY_STRING_MAX_LENGTH allowed length (was 900000)

That might be reasonable when the user has specified PUMA_QUERY_STRING_MAX_LENGTH but is confusing and unhelpful when they haven't.

@elct9620
Copy link
Contributor Author

How about this

/** Defines common length and error messages for input length validation. */
#define QUOTE(s) #s
#define EXPLAND_MAX_LENGHT_VALUE(s) QUOTE(s)
#define DEF_MAX_LENGTH(N,length) const size_t MAX_##N##_LENGTH = length; const char *MAX_##N##_LENGTH_ERR = "HTTP element " # N  " is longer than the " EXPLAND_MAX_LENGHT_VALUE(length) " allowed length (was %d)"

Validated with gcc -E ext/puma_http11/puma_http11.c will generated

# 59 "ext/puma_http11/puma_http11.c"
const size_t MAX_FIELD_NAME_LENGTH = 256; const char *MAX_FIELD_NAME_LENGTH_ERR = "HTTP element " "FIELD_NAME" " is longer than the " "256" " allowed length (was %d)";
const size_t MAX_FIELD_VALUE_LENGTH = 80 * 1024; const char *MAX_FIELD_VALUE_LENGTH_ERR = "HTTP element " "FIELD_VALUE" " is longer than the " "80 * 1024" " allowed length (was %d)";
const size_t MAX_REQUEST_URI_LENGTH = 1024 * 12; const char *MAX_REQUEST_URI_LENGTH_ERR = "HTTP element " "REQUEST_URI" " is longer than the " "1024 * 12" " allowed length (was %d)";
const size_t MAX_FRAGMENT_LENGTH = 1024; const char *MAX_FRAGMENT_LENGTH_ERR = "HTTP element " "FRAGMENT" " is longer than the " "1024" " allowed length (was %d)";
const size_t MAX_REQUEST_PATH_LENGTH = 8192; const char *MAX_REQUEST_PATH_LENGTH_ERR = "HTTP element " "REQUEST_PATH" " is longer than the " "8192" " allowed length (was %d)";
const size_t MAX_QUERY_STRING_LENGTH = (1024 * 10); const char *MAX_QUERY_STRING_LENGTH_ERR = "HTTP element " "QUERY_STRING" " is longer than the " "(1024 * 10)" " allowed length (was %d)";
const size_t MAX_HEADER_LENGTH = (1024 * (80 + 32)); const char *MAX_HEADER_LENGTH_ERR = "HTTP element " "HEADER" " is longer than the " "(1024 * (80 + 32))" " allowed length (was %d)";

But I think the QUOTE and EXPLAN_MAX_LENGTH_VALUE isn't a good naming in this case.

@nateberkopec
Copy link
Member

@elct9620 That's OK with me

@nateberkopec nateberkopec added the waiting-for-changes Waiting on changes from the requestor label Nov 17, 2020
@elct9620
Copy link
Contributor Author

@nateberkopec I have updated the code to expand the common length variable in the error message. If there is no other issue to fix, I will update the document for it.

@nateberkopec
Copy link
Member

@elct9620 Looks good to me. Please update docs and changelog and we'll merge 👍

@nateberkopec
Copy link
Member

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature waiting-for-changes Waiting on changes from the requestor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants