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

Add REQUEST_PATH on parse error message #1831

Merged
merged 1 commit into from Jul 27, 2019

Conversation

spk
Copy link
Contributor

@spk spk commented Jul 1, 2019

Request get /plop and query string bigger than 1024*10

2019-07-01 21:31:08 +0200: HTTP parse error, malformed request (127.0.0.1/plop): #<Puma::HttpParserError: HTTP element QUERY_STRING is longer than the (1024 * 10) allowed length (was 20481)>

ref #1768

@spk spk force-pushed the show-path-on-parse-error branch 2 times, most recently from f15331c to 9d44dac Compare July 1, 2019 19:57
@spk spk changed the title Add REQUEST_PATH as fallback on parse error message Add REQUEST_PATH on parse error message Jul 1, 2019
@nateberkopec
Copy link
Member

Works for me. Test?

@spk
Copy link
Contributor Author

spk commented Jul 4, 2019

Works for me. Test?

Added in ea555ef

@ylecuyer
Copy link
Contributor

ylecuyer commented Jul 4, 2019

Would it be possible to log the full url? Instead of just the path

@nateberkopec
Copy link
Member

Awesome, thanks for adding a test ❤️

@spk
Copy link
Contributor Author

spk commented Jul 5, 2019

Would it be possible to log the full url? Instead of just the path

If you mean with the full url adding the query string in this case it wouldn't be possible since we cannot parse it, the QUERY_STRING is going to be empty.

For the IP its going to be present when puma is behind an http roxy forwarding the env x-forwarded-for or remote-addr cf https://github.com/spk/puma/blob/ea555ef9d617782c03ca0554ca8069d92f109838/lib/puma/events.rb#L97

I hope I answer your question and of course I let Nate correct me if I'm wrong !

@nateberkopec
Copy link
Member

I appreciate the effort you put in to the test here, but I'm trying to avoid adding more integration tests like this unless absolutely necessary.

I'd prefer a simpler test like the one here., or an even simpler one that simply calls parse_error and checks stdout.

@nateberkopec nateberkopec added the waiting-for-changes Waiting on changes from the requestor label Jul 16, 2019
@spk
Copy link
Contributor Author

spk commented Jul 17, 2019

I appreciate the effort you put in to the test here, but I'm trying to avoid adding more integration tests like this unless absolutely necessary.

I'd prefer a simpler test like the one here., or an even simpler one that simply calls parse_error and checks stdout.

I've looked up I don't think its possible to check the stderr log here, we already have a test for max params and this raise directly... Maybe I'm wrong thanks for your help!

@nateberkopec nateberkopec removed the waiting-for-changes Waiting on changes from the requestor label Jul 17, 2019
@nateberkopec
Copy link
Member

@spk Since the method you're changing is actually in Puma::Events, you'll need to start a new Puma::Server with a custom IO object (don't use global stderr/stdout), and then you can cause a parser error and check the IO object. Maybe the tests for Server or Events would be a better inspiration. I would put this test in with the event tests because that's where this method actually lives.

@nateberkopec nateberkopec added the waiting-for-changes Waiting on changes from the requestor label Jul 21, 2019
```
2019-07-01 21:31:08 +0200: HTTP parse error, malformed request (127.0.0.1/plop): #<Puma::HttpParserError: HTTP element QUERY_STRING is longer than the (1024 * 10) allowed length (was 20481)>
```

ref puma#1768
@spk spk force-pushed the show-path-on-parse-error branch from ea555ef to 55334b2 Compare July 21, 2019 19:05
@spk
Copy link
Contributor Author

spk commented Jul 21, 2019

Thanks Nate for your help hope its ok with this spk@55334b2

@nateberkopec nateberkopec removed the waiting-for-changes Waiting on changes from the requestor label Jul 22, 2019
@nateberkopec nateberkopec merged commit 0cb1088 into puma:master Jul 27, 2019
@spk spk deleted the show-path-on-parse-error branch September 8, 2023 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants