Improved handling of non-standard http methods

[yedhink]

Support plan

• which support plan is this issue covered by? (e.g. Community, Core, Plus, or Enterprise): Community
• is this issue currently blocking your project? (yes/no):no
• is this issue affecting a production system? (yes/no):yes

Context

• node version: v14.2.0
• module version: @hapi/hapi ^19.1.1
• environment (e.g. node, browser, native): node
• used with (e.g. hapi application, another framework, standalone, ...): hapi application(a server in localhost)
• any other relevant information: Linux 5.6.13-arch1-1

How can we help?

TL;DR

What is the usecase of validating any alpha-numeric combination as a valid HTTP method in Server Route in hapi? I think this is the relevant code in source. Does hapi support non-standard http methods out of the box? If yes can you point me to docs mentioning/implementing the same?

My Research

I checked the nodejs http-parser(relevant code) and it supports several well defined standard http methods and all non-standard http methods are parsed as HTPP_unknown too. If Non-Standard http methods(custom defined methods) are not supported by hapi, then wouldn't validating the exact methods like "GET", "POST" etc be the right way to do it rather than validating all alpha-numeric combinations?

Why I feel this is problematic?

Consider this simple example where i define a route:-

const server: Server = new Server({
    "host": "localhost",
    "port": 3000,
    "debug": {
        "request": ["*"],
        "log": ["*"],
    },
});

server.route([
        {
            "method": "GETs", // here "GETs" is not a valid standard HTTP_METHOD
            "path": "/",
            "handler": myHandler,
        }
    ]);

In this scenario, while typing the method value was incorrectly set to GETs where it should have been GET. Since any string value is valid currently for the method property, no error is thrown after compilation/running until a request is made to the corresponding path. Even when the error is thrown once the request is made, it is kinda not pointing out that giving a invalid method name like GETs was the problem in the first place - which makes debugging pretty hard.

A much more important point to note is that, when using hapi with typescript, if the method is not a standard method, then it can actually help in type checking and raise an error while coding itself. But currently it is not able to provide such a type checking since any string is valid for the method property in server route. I have raised an issue about the same in the types for hapi at DefinitelyTyped. https://github.com/DefinitelyTyped/DefinitelyTyped/issues/44806

Final Thoughts

So if hapi doesn't support non-standard http methods without modifying the core nodejs http parser, wouldn't it be much more precise to validate only standard http methods like "GET,"POST" etc so that errors could be easily found during compilation itself rather than waiting for a request to be made? If hapi supports non-standard http methods out of the box, then how can i guard the method property from not being assigned a invalid http method like GETs? or even how can the error handling be improved to say point out that GETs is not a defined standard/non-standard http method?

[sholladay]

I don't know if this applies to the HTTP parser that Node recently switched to, but historically, Node rejected requests with non-standard HTTP methods. It did so early enough in the request lifecycle that I believe hapi would never be able to notice or intercept it, let alone change the behavior.

[yedhink]

@sholladay Yea exactly. That's what i also thought. So if non-standard http methods are rejected, then why is there a need for validating all possible strings as valid for server route(relevant source code)? It should be much more precise right? Like only validate known standard HTTP methods like "GET", "POST" etc, such that when the user tries to set a non-standard http method in Server Route, it will result in a error. What do you think?

[kanongil]

I think it would make sense to limit the accepted methods in the route definition to any that node can handle. Unfortunately there is no way to fetch this list at runtime, and it would have to be kept in sync manually.

[sholladay]

I'm in favor of hardcoding the valid methods, given the improved error messages that could be thrown when someone makes a typo. My guess is that the current implementation is designed to be forwards compatible with any new HTTP methods that may be added in the future. But I think that's both unlikely/rare to happen and also not super important, anyway. Just my opinion.

[yedhink]

Yes exactly. The typo errors are a real thing. I dug deep into this since i was burning my head to debug this particular error.

I'm willing to make a PR in which I could hard code the valid HTTP methods possible in server route method property, such that:-

1. Unintentional typos are caught during compilation itself
2. Better auto completions can be triggered for methods property since values are hardcoded.

@kanongil @sholladay What do you suggest? Should i go ahead with the PR? How does it work? Are you the folks who review and merge the PR?

[devinivy]

It's worth mentioning that there are plugins that do make use of non-standard methods, mostly for "internal-use" routes. For example, you can see here in nes that a route with the method 'auth' is created specifically for simulating HTTP authentication: https://github.com/hapijs/nes/blob/25819e955cfef3fdca69d8f545c3ef179ad86a19/lib/index.js#L179-L185

[yedhink]

@devinivy That makes sense. But i believe the custom auth method works since the authentication was configured on the route, by the options.auth parameter right? I mean like there exists an option for authentication within the route configuration of hapi right. That is why naming a custom method with name auth and adding the functionality for authentication makes sense right? So why not enforce auth as a valid http method specific for authentication purposes within hapi such that if a person makes a type error like auths while naming the method, then a error will be thrown?

Maybe i'm missing the point here. So correct me if I have misunderstood the point.

[devinivy]

To my knowledge there is nowhere that prescribes the special use of the 'auth' method, whether or not you're using hapi's route options.auth option. I believe it was just a way to categorize the route in such a way that it could only be used internally and not over HTTP, but I am not sure. The reason I mention it is because I think this 'auth' method would no longer be allowed based upon the rules mentioned above, so I wanted to flag that this change would cause breaking changes to at least one plugin in the hapijs org. It's not a reason not to make this change, just a consideration.

Overall I agree with the idea of hard-coding the list of methods and I think it consistent with hapi's tendency to fail early and often. But my impression is that it was an intentional decision to allow hapi's router to accept non-standard methods, and perhaps we'll still want to allow that in some way. Probably a question for Eran, really. If this were treated similarly to other parts of the hapi interface (e.g. events) then I would expect to see custom methods defined at runtime directly on the server (e.g. server.method('auth').

[yedhink]

@devinivy I agree with you. So this is what I am able to conclude:-

1. It would be a major breaking change if hard-coding part is done given the caveats that you described.
2. The advantages of hard-coding the HTTP methods are also noteworthy since it really helps in catching errors.
3. For the time being maybe these are the options we have:-
   • Write tests which would explicitly check whether method property belongs to desired HTTP methods
   • Mention about the server route method situation described above somewhere in the docs

I'm thinking of leaving this issue open until in official docs something about will be mentioned.

[hueniverse]

I'm fine documenting this but I am not going to change the code. This is not a real problem. I haven't seen a single issue opened about this causing problems.

[beautifulcoder]

I wonder if TypeScript type definitions might help here. The method property could be set to this declaration:

method: 'GET' | 'POST' | ...;

I won't need code changes since it'd get flagged by the compiler. This does need to go in the type definition file.