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

Suggestion: Joi.string().uri({ allowRelativeOnly }) #1015

Closed
davidjamesstone opened this issue Oct 26, 2016 · 7 comments
Closed

Suggestion: Joi.string().uri({ allowRelativeOnly }) #1015

davidjamesstone opened this issue Oct 26, 2016 · 7 comments
Assignees
Labels
feature New functionality or improvement
Milestone

Comments

@davidjamesstone
Copy link

Similar to the allowRelative option for Joi.string().uri() but to mandate that the uri is relative.

Why?

A common use case for passing urls around in query string is to perform some subsequent redirection e.g.

http:\\www.example.com\do-something?returnUrl=\summary

It's very likely that you would only ever want the returnUrl to be relative to the current domain. Doing so can also prevent Open Redirection Attacks.

We are currently using the is-relative-url package to ensure the returnUrl is local.

Alternatively, Microsoft's C# algorithm looks like this (here's a blog which includes a coffeescript implementation of the same):

public static bool IsUrlLocalToHost(this HttpRequestBase request, string url)
{
   return !url.IsEmpty() &&
          ((url[0] == '/' && (url.Length == 1 ||
           (url[1] != '/' && url[1] != '\\'))) ||   // "/" or "/foo" but not "//" or "/\"
           (url.Length > 1 &&
            url[0] == '~' && url[1] == '/'));   // "~/" or "~/foo"
}

This could be useful to base something off (removing the tilde ~ logic as it is only relevent to ASP).

It would be nice if we could have Joi do this for us. Thoughts?

@Marsup
Copy link
Collaborator

Marsup commented Oct 26, 2016

This shouldn't be a major change, happy to take a PR about it. Prefer shorter relativeOnly though.

@Marsup
Copy link
Collaborator

Marsup commented Nov 16, 2016

Fixed by #1034.

@Marsup Marsup closed this as completed Nov 16, 2016
@ilyaigpetrov
Copy link

ilyaigpetrov commented May 24, 2018

I warn you that //google.com is still accepted as relative by relativeOnly thus making it less useful, because you still have to manually check the redirect url.

My approach:

const schema = Joi.object({
    redirect_to_relative: Joi.string().uri({
        relativeOnly: true
    }).regex(/^\/\//, { invert: true })
});

@Marsup
Copy link
Collaborator

Marsup commented May 24, 2018

Technically it is a relative URI, relative to the root. It is following the RFC if I remember well.

@ilyaigpetrov
Copy link

ilyaigpetrov commented May 25, 2018

Yes, it conforms to the RFC, but it doesn't solve the redirection problem stated in the first post by @davidjamesstone, because it is still open to redirection attacks.
I propose to add Joi.string().uri({ isPathAndHash: true }). Path and hash definitions may be found here.

Because hash fragment is not passed to the server it must be percent-encoded into query string parameter (e.g. http://localhost/login?redirect_to_path=/protected#foo, but percent encoded) which is then validated by Joi.

@ilyaigpetrov
Copy link

ilyaigpetrov commented May 25, 2018

Or you may come up with a more flexible configuration like:

  • Joi.string().uri({ madeOfOnly: ['path', 'hash'] })
  • Joi.string().uri({ madeOfOnly: ['host', 'pathname'] }).
  • Joi.string().uri({ doesntInclude: ['protocol'] }).

@hueniverse hueniverse added feature New functionality or improvement and removed request labels Sep 19, 2019
@lock
Copy link

lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature New functionality or improvement
Projects
None yet
Development

No branches or pull requests

4 participants