ninjaframework / ninja

Ninja is a full stack web framework for Java. Rock solid, fast and super productive.
http://www.ninjaframework.org
Apache License 2.0
1.91k stars 522 forks source link

HTTP head philosophy #592

Open danielsawan opened 6 years ago

danielsawan commented 6 years ago

I recently had some problem integrating a forum with my ninja app and it appears that the bug was coming from Ninja returning "404 Not Found" instead of "405 Method Not Allowed" when the HEAD HTTP method wasn't implemented / defined.

And more generally i was asking myself should every controller method answer to HEAD request ? I understand that HTTP head method is something different then CRUD (GET,POST,PUT,DELETE).

Should NINJA automatically answer to head request when not implemented ?

All start here : https://meta.discourse.org/t/sso-avatar-sync/74979/7?u=dany

Thanks for your answers.

danielsawan commented 6 years ago

I tested on https://projects.spring.io/spring-boot/ and https://www.playframework.com/ and they both automaticly answer to HEAD request.

I also tested some other java web framework that doesn't https://github.com/decebals/pippo and https://mangoo.io/

jjlauer commented 6 years ago

I haven't thought much about the default, but it'd be easy to add in 1 line to your conf.Routes file:

router.HEAD().route("*").with(() -> Results.status(405));

You could place that near the bottom so that ones before it come in priority. That would match any head request and return a 405 error.

On Fri, Dec 1, 2017 at 7:28 AM, Daniel notifications@github.com wrote:

I tested on https://projects.spring.io/spring-boot/ and https://www.playframework.com/ and they both automaticly answer to HEAD request.

I also tested some other java web framework that doesn't https://github.com/decebals/pippo and https://mangoo.io/

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/ninjaframework/ninja/issues/592#issuecomment-348482661, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjwAr5FDVIOgJg3MeVEAVKuuDqQYoNPks5s7_DrgaJpZM4QxUSh .

danielsawan commented 6 years ago

Thanks for the tips but what about making this being the default behaviour ?

And more generally what about the idea that all GET method should automatically answer to HEAD requests ? They seems like to be pretty sure about that https://meta.discourse.org/t/sso-avatar-sync-not-working-due-to-incorrect-http-head-response/74979/12?u=dany

jjlauer commented 6 years ago

Hey Daniel,

I read thru the link and you are correct about most frameworks disabling/not caring about HEAD requests. In my decades of web work, I'm not sure I've use a HEAD request more than a couple times.

The main reason why its good practice to disable / not support HEAD requests is around security. It's always good security practice to disable anything you don't use. For example, some logging frameworks skip reporting HEAD requests -- so you could have someone leverage that to DoS your site w/o possibly getting logged. That's just one example and I'm sure there are more.

As I mentioned in my previous email, its not that I'm against changing the default, I just personally haven't put any thought into it. Unless Raphael has other thoughts on it, I'm sure we'd consider a PR that changes the defaults or makes it configurable.

As for supporting HEAD -> GET requests under-the-hood: I believe that could be accomplished in the short term with some Ninja-foo. The pseudo code would be a controller method that takes the HEAD request, changes the method on the Context, then calls ninja.onRouteRequest again itself, gets the Result, changes the Renderable to be a noop, and then returns that. If successful, that bit of logic could be put into a Controller we distribute with Ninja by default. Then folks could use it to support HEAD requests if they wanted.

-Joe

On Fri, Dec 1, 2017 at 7:55 AM, Daniel notifications@github.com wrote:

Thanks for the tips but what about making this being the default behaviour ?

And most generally what about the idea that all GET method should automatically answer to HEAD requests ? They seems like to be sure about that https://meta.discourse.org/t/sso-avatar-sync-not-working- due-to-incorrect-http-head-response/74979/12?u=dany

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/ninjaframework/ninja/issues/592#issuecomment-348487944, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjwAggHsuCr6_NXfFG96SKp6rz1hEeiks5s7_dPgaJpZM4QxUSh .

danielsawan commented 6 years ago

In my decades of web work, I'm not sure I've use a HEAD request more than a couple times.

Me neither...

It's always good security practice to disable anything you don't use.

I agree 100% but it seems like to be a Standard. And like JavaNamingConvention.class we are not forcedToUseIt() and it work without but it is better to follow the movement. And most of all i am a little bit afraid about SEO here. Maybe Google bots also rely on some Head request sometimes (no idea at all just speculation)

I think the best thing to do is to make it the default behaviour and make it configurable for desactivation if needed.

communiteq commented 6 years ago

The main reason why its good practice to disable / not support HEAD requests is around security. It's always good security practice to disable anything you don't use. For example, some logging frameworks skip reporting HEAD requests -- so you could have someone leverage that to DoS your site w/o possibly getting logged.

I think that is a different discussion. Of course these things should be logged, but the fact that some logging frameworks don't do that, is not a reason to disable something.

The HTTP specification defines the HEAD request, it has been around for 25 years, you're not more secure when you disable / fail to implement it, and you expose your users to the most obscure category of problems if your code fails to respond with the required 405 Method Not Allowed response.

Oh, and this. Eight years old and apparently still relevant.

SamSaffron commented 6 years ago

note ... there are only really 2 options you have here, reply with 405 for every HEAD req that basically says you do not support it or forward to the app and eat the body if the app returns it.

Currently it appears you reply with 404 and that throws stuff off big time.

riking commented 6 years ago

MDN seems to think that support for HEAD is mandatory: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/405

Note: The two mandatory methods, GET and HEAD, must never be disabled and should not return this error code.

Relevant text from the RFC:

The HEAD method is identical to GET except that the server MUST NOT return a message-body in the response.

The correct thing for a framework to do when the user has not specified special handling for HEAD requests is to process it as a GET, and throw out the body.

And yes, I'm fairly sure that Google generates HEAD requests on occasion. wget will definitely generate them when checking whether to redownload a file.

raphaelbauer commented 6 years ago

Thanks @danielsawan for bringing up the topic :) I think it is totally correct that http compliant support for HEAD requests is missing in Ninja.

I don't have a solution in my head right now. Just some thoughts:

PRs are of course welcome, we can provide guidance (and @jjlauer already sketched a nice idea).