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

Route constraints which contain a period do not work without trailing '/' #969

Closed
mathewc opened this Issue Nov 22, 2016 · 22 comments

Comments

Projects
None yet
9 participants
@mathewc
Contributor

mathewc commented Nov 22, 2016

If you try to define a custom route products/{category:alpha}/{id:regex(\d+.\d+.\d+)} it will match requests like https://function-fun.azurewebsites.net/api/products/electronics/1.2.3/ but not https://function-fun.azurewebsites.net/api/products/electronics/1.2.3 (no trailing slash).

@mamaso mamaso changed the title from Regex route constraint doesn't work without trailing '/' to Route constraints which contain a period do not work without trailing '/' Nov 22, 2016

@davidebbo

This comment has been minimized.

Contributor

davidebbo commented Nov 22, 2016

We've tried two things that seem to fix it:

<system.webServer>
  <modules>
    <remove name="UrlRoutingModule-4.0" />
    <add name="UrlRoutingModule-4.0" type="System.Web.Routing.UrlRoutingModule" />
  </modules>
</system.webServer>

or

<system.webServer>
  <modules runAllManagedModulesForAllRequests="true" />
</system.webServer>

But we'll need to make sure this doesn't regress cold start perf and functionality.

edit also found a http handler solution: http://stackoverflow.com/a/12151501/5915331

@mamaso

This comment has been minimized.

Contributor

mamaso commented Nov 23, 2016

A little more update, checked freb logs on the requests, I'm pretty sure this handler change is causing managed modules (urlrouting in particular) to be skipped. Explains why rammfar and the url routing module change (removing manage precondition) fix this.

image

@lindydonna

This comment has been minimized.

Contributor

lindydonna commented Nov 28, 2016

Notes from triage: we suspect no perf impact on routes that we already handle, but we should do perf tests to confirm.

@lindydonna lindydonna added this to the Next - Triaged milestone Nov 28, 2016

@lindydonna lindydonna added the bug label Nov 28, 2016

@ricklove

This comment has been minimized.

ricklove commented Jan 20, 2017

This is a problem with wildcards as well (I'm using node.js):

      "route": "testpath/{*pathName}"

This works fine for any route unless the route has a filename with extension at the end (with or without query string).

Adding a trailing / does fix it.

Since I am using node.js, I'm not sure how to control the IIS settings mentioned above that are normally controlled by web.config.

Note: I also found another web.config setting mentioned that could be relevant:

<httpRuntime relaxedUrlToFileSystemMapping="true" />

From: http://stackoverflow.com/questions/294495/semantic-urls-with-dots-in-net#328873

Anyway, any ideas how to work around this for node.js?

@mikezks

This comment has been minimized.

mikezks commented Mar 1, 2017

@safihamid mentioned on stackoverflow that the dot issue - at least for functions proxies - is fixed now.

unfortunately it still does not work for me (i restarted the function app).

proxy.json

{
    "proxies": {
        "Proxy1": {
            "matchCondition": {
                "route": "/test.html"
            },
            "backendUri": "https://<funcapp>.azurewebsites.net/func1"
        }
    }
}
@safihamid

This comment has been minimized.

Contributor

safihamid commented Mar 1, 2017

@mikezks please make sure you have updated the proxy version to ~0.1. I verified the above works with the latest versin

image

@ricklove

This comment has been minimized.

ricklove commented Mar 1, 2017

Yes! The final piece to the puzzle!

Thanks Azure Functions team!

I can confirm this is working:

https://test-azure-functions-proxies.azurewebsites.net/file/file.json

Does this work only with proxies, or can this work with the normal function route as well?

@mikezks

This comment has been minimized.

mikezks commented Mar 1, 2017

@safihamid:
yes, i updated the proxy version before, but it did not work yesterday.
now everything is fine and i can confirm as well that this issue is resolved. thanks a lot! 😃

@mikezks

This comment has been minimized.

mikezks commented Mar 7, 2017

I just noticed that the dot issue still occurs for Azure Functions routing.
...but it is correct that it works for Functions Proxies.

Though this is not a big issue for me because we now have the workaround with Functions Proxies, this needs to be fixed one day. Thanks!

@kzu

This comment has been minimized.

kzu commented Jul 18, 2017

Indeed, I'm facing this in functions routing.

I've got:

      "route": "test/{owner:alpha}/{repo:alpha}/{sha:alpha}"

As soon as the client url contains a dot in any of the url patterns (i.e. test/kzu/foo.bar/baz), it stops matching. Adding a trailing / doesn't fix it for functions, neither any of the web.config workarounds mentioned here. :(

@lindydonna

This comment has been minimized.

Contributor

lindydonna commented Jul 18, 2017

@kzu Note that you can't customize web.config when running on Azure Functions.

CC @mathewc and @fabiocav

@kzu

This comment has been minimized.

kzu commented Jul 18, 2017

@lindydonna fair. Still, the dot is broken if it shows up in any of the url patterns :(. Any suggested workarounds?

@kzu

This comment has been minimized.

kzu commented Jul 18, 2017

btw, I was hoping uploading a web.config via https://*.scm.azurewebsites.net/dev/wwwroot/ would work :(

@kzu

This comment has been minimized.

kzu commented Jul 18, 2017

Given the following proxy definition:

{
    "$schema": "http://json.schemastore.org/proxies",
    "proxies": {
        "root": {
            "matchCondition": {
                "route": "{owner}",
                "methods": [
                    "GET"
                ]
            },
            "backendUri": "https://APP.azurewebsites.net/test/{owner}"
        }
    }
}

I see the following:

OK: http://APP.azurewebsites.net/test/foo

FAIL: http://APP.azurewebsites.net/test/fo_o
FAIL: http://APP.azurewebsites.net/test/fo.o
FAIL: http://APP.azurewebsites.net/test/fo%60o
FAIL: http://APP.azurewebsites.net/test/fo%20o
FAIL: http://APP.azurewebsites.net/test/fo|o

(with appropriate URL escaping performed by the browser itself, they all fail with or without a trailing /).

Makes the routing very very brittle.

Note: I was looking for a way to turn the problematic dots into some fairly uncommon char just for routing and inside the function convert it back, and that's how I found that it's broken for at least a whole bunch of other chars.

@mikezks

This comment has been minimized.

mikezks commented Jul 18, 2017

Please try adding a slash in your proxy's config:
"backendUri": "https://APP.azurewebsites.net/test/{owner}/"
...I hope I remember this workaround correctly.

@ricklove

This comment has been minimized.

ricklove commented Jul 19, 2017

Below is how I do it with proxies, and it seems to handle dots fine. (I think the trick is to route the params to a query string which doesn't blow up the Function routing.)

(I use this for static file serving to a CDN and also something like this for my SPA apps where the app does internal routing.)

I use a wildcard and it seems to handle all sub routes which can be passed to a function.

Also, if you leave the route "" (blank), you can override the root index page. (But you have to edit the proxies.json file manually because the Functions editor doesn't accept a blank route.)

{
    "$schema": "http://json.schemastore.org/proxies",
    "proxies": {
        "Static": {
            "matchCondition": {
                "route": "static/{*file}"
            },
            "backendUri": "https://told-stack-demo.azurewebsites.net/api/static?file={file}"
        },
        "Root": {
            "matchCondition": {
                "route": "",
                "methods": [
                    "GET"
                ]
            },
            "backendUri": "https://told-stack-demo.azurewebsites.net/api/static?file=demo/"
        }
    }
}

Example:

Also, I tested the error cases, and the proxy sent them to the function correctly (where the file was not found so the function reported a File Path Error):

@kzu

This comment has been minimized.

kzu commented Jul 19, 2017

That's cheating ;). I also got it to "work" by having one route that consumes everything with {*everything} and making that a query string arg for the function. But the fact is that if the function has a route, any dots (or any other chars as I showed) don't work. Adding trailing slashes doesn't work either.

So, a "catch-all" route on the proxy, and no route at all possible in functions, is not a great experience, quite the contrary.

@ricklove

This comment has been minimized.

ricklove commented Jul 19, 2017

True, it would be nice if the function routing could accept any valid URL value.

(For example when doing Function Binding with BlobNames, it would likely have a dot in the parameter.)

At least the proxy can function as a work around, which we didn't have a few months ago.

@codeitcody

This comment has been minimized.

codeitcody commented Jul 31, 2017

Ran into this while building an HTTP API with dots in the route.

https://api.site.com/users.info
https://api.site.com/users.create
https://api.site.com/users.update
https://api.site.com/users.delete

Its not as common but is definitely legal routing. I agree the route should allow any valid URL.

@kzu

This comment has been minimized.

kzu commented Aug 1, 2017

Indeed @codeitcody

omkarmore83 added a commit that referenced this issue Aug 9, 2017

Proxies allows users to have routes with "." period in it. Currently
Functions does not handle such requests. This is covered in  issue
#969 (comment)
@kzu

This comment has been minimized.

kzu commented Sep 17, 2017

@mathewc any ideas when this is hitting production?

@davidebbo

This comment has been minimized.

Contributor

davidebbo commented Sep 17, 2017

@kzu it's partially deployed. If you have the cycles, you can verify it in North Central US. It should reach everywhere on Tuesday.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment