Skip to content

in directory, fix the listing of subdirs that reside in a subdirs that have spaces - #1253

Merged
hueniverse merged 2 commits into
hapijs:masterfrom
iamdoron:master
Dec 29, 2013
Merged

hueniverse merged 2 commits into
hapijs:masterfrom
iamdoron:master

Conversation

@iamdoron

Copy link
Copy Markdown
Contributor

there was a problem with listing a subdir that is within a subdir with spaces (for example GET /directory/some%20dir/subdir) because it was actually encoded twice.
at the beginning I thought I might change pathEncode implementation to something like:

return encodeURI(decodeURI(path));

but I guess the resource was the one causing the issue.

I also replaced encodeURIComponent with encodeURI - if it is necessary we should have a test that captures it.

encoding twice:

> encodeURIComponent(encodeURI("a b c"))
'a%2520b%2520c'
> encodeURIComponent(("a b c"))
'a%20b%20c'

update: I reverted to encodeURIComponent, and I found out that route already decodes the url parameters, so decoding the path parameter may not throw

Comment thread lib/directory.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

decodeURI can throw... decodeURI('%f')

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hueniverse, so here I'm a bit confused.
Is it possible to request a not encoded url? yes (at least with the basic http module as server & curl it is possible)
is it allowed? isn't it a bad request? for example, is GET /dir/%/ allowed as a good request? shouldn't it be GET /dir/%25/ ?
It's also harder to test this scenario, since the hapi router.match doesn't match it (at least from what I see, I will dig a bit deeper).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hueniverse, so I found out that route is actually 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 ultimately means- a path that is not decoded correctly is invalid. However, note that it will return 404, and it will try to decode it foreach route.

@ghost ghost assigned hueniverse Dec 26, 2013
hueniverse pushed a commit that referenced this pull request Dec 29, 2013
in directory, fix the listing of subdirs that reside in a subdirs that have spaces
@hueniverse
hueniverse merged commit 96b73b0 into hapijs:master Dec 29, 2013
jmonster pushed a commit to jmonster/hapi that referenced this pull request Feb 10, 2014
in directory, fix the listing of subdirs that reside in a subdirs that have spaces
@lock lock Bot locked as resolved and limited conversation to collaborators Jan 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug Bug or defect

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants