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

requesting url that is not encoded correctly should return 400, not 404 #1264

Closed
iamdoron opened this issue Dec 27, 2013 · 4 comments
Closed
Assignees
Labels
breaking changes Change that can breaking existing code bug Bug or defect
Milestone

Comments

@iamdoron
Copy link
Contributor

Given a route /existingRoute/{path*}
When requesting /existingRoute/%/a
Then 400 should be returned

I'm not sure 400 is the right code; 404 means that is is not found but there is a chance that it will be found; is there a way for the path to be found? maybe if you define a route /existingRoute/%/{path*}, but is it a valid route?

my findings so far (from investigating #1253):

route is decoding url component using these functions:

internals.setParam = function (name, value, params, paramsArray, isEmptyOk) {

    var isValid = (isEmptyOk || value);

    if (isValid &&
        params) {

        var decoded = internals.decodeURIComponent(value || '');
        if (decoded === null) {
            isValid = false;
        }
        else {
            params[name] = decoded;
            paramsArray.push(decoded);
        }
    }

    return isValid;
};


internals.decodeURIComponent = function (value) {

    try {
        return decodeURIComponent(value);
    }
    catch (err) {
        return null;
    }
};  

which means that it treats it as an invalid route (which is ok), but all routes will run this code (or won't event reach it) and, eventually 404 will be returned.

@hueniverse
Copy link
Contributor

I am not sure it is worth the effort... if you ask for an invalid path that will never match an existing route if we ignored the invalid path, 400 is appropriate. But not for cases where the route is both invalid and missing where 404 is the right response (since 400 implies a match).

@ghost ghost assigned hueniverse Dec 29, 2013
@iamdoron
Copy link
Contributor Author

Or you could look it the other way around;
Let's say I'm trying to write a client for an api, and during some integration tests the client requests:

GET /thing/1/priceRaise%/

(The actual route is /things...)

First I get 400, saying something about encoding, so I encode it, fix the 400 and then get 404- I study the ap documentation and I see that the rout indeed does not exist.

I'm not sure 400 implies a match, it states that it is a bad request that shouldn't be requested- anyhow, I'm trying to think about the people integrating with the api. What will make them more comfortable and integrate faster (with the api). I guess that if you get 404 you might eventually find the problem, but it might slow you down a bit- because the route will seem fine, and if you are unaware of encoding you will find no answers in the api docs, because this is a 'language' problem, where you should know the language web APIs usually use.

With all being said, if it does prove to be an effort, I agree that it rarely happens because the api provider usually defines the route statically (with the exception of route parameter)

It just crossed my mind that we should check that a route is properly encoded (maybe it already happens, I didn't check) when declaring a route

hueniverse pushed a commit that referenced this issue Jan 4, 2014
@hueniverse
Copy link
Contributor

This was nasty to fix...

@iamdoron
Copy link
Contributor Author

iamdoron commented Jan 5, 2014

👍

jmonster pushed a commit to jmonster/hapi that referenced this issue Feb 10, 2014
jmonster pushed a commit to jmonster/hapi that referenced this issue Feb 10, 2014
@lock lock bot locked as resolved and limited conversation to collaborators Jan 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking changes Change that can breaking existing code bug Bug or defect
Projects
None yet
Development

No branches or pull requests

2 participants