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

Colon in uri generate response 404 #2553

Closed
4n1qz5skwv opened this issue Sep 26, 2022 · 9 comments
Closed

Colon in uri generate response 404 #2553

4n1qz5skwv opened this issue Sep 26, 2022 · 9 comments

Comments

@4n1qz5skwv
Copy link

Sanic can't find the route if there's a colon in the uri.

Code snippet

from sanic import Sanic, Request, HTTPResponse

app = Sanic('zzz')


@app.get(f'/abc/x:y')
async def main(_: Request):
    return HTTPResponse()


if __name__ == '__main__':
    app.run()

Expected behavior
GET http://127.0.0.1:8000/abc/x:y 200 0

Actual behavior
GET http://127.0.0.1:8000/abc/x:y 404 733

Environment

[2022-09-26 20:50:40 +0600] [13060] [INFO] Sanic v22.6.2
[2022-09-26 20:50:40 +0600] [13060] [INFO] Goin' Fast @ http://127.0.0.1:8000
[2022-09-26 20:50:40 +0600] [13060] [INFO] mode: production, single worker
[2022-09-26 20:50:40 +0600] [13060] [INFO] server: sanic, HTTP/1.1
[2022-09-26 20:50:40 +0600] [13060] [INFO] python: 3.10.5
[2022-09-26 20:50:40 +0600] [13060] [INFO] platform: Windows-10-10.0.19044-SP0
[2022-09-26 20:50:40 +0600] [13060] [INFO] packages: sanic-routing==22.3.0
[2022-09-26 20:50:40 +0600] [13060] [INFO] Starting worker [13060]

Additional context
tiangolo/fastapi#4892
encode/starlette#1657

@4n1qz5skwv 4n1qz5skwv added the bug label Sep 26, 2022
@sjsadowski
Copy link
Contributor

Please open this in https://github.com/sanic-org/sanic-routing as well

@ahopkins
Copy link
Member

@sjsadowski no need to do that, we can just move it between projects

@ahopkins
Copy link
Member

For reference, this is not a path defined character, and is explicitly reserved in the spec. I'm not sure rhat it is something in need of changing, and I certainly would not call this a bug.

@ahopkins
Copy link
Member

This works as expected if you pass the value as URL encoded.

image

@4n1qz5skwv
Copy link
Author

4n1qz5skwv commented Sep 30, 2022

if you pass the value as

That if is a problem. Have you tried it from a browser (not encoded)? Browsers or webhook will not do that.

If a route is, hello.com/item:123, and a user input this URL in the browser, it should just work.
App should handle this internally. Other libs seems to just work by default.

@prryplatypus
Copy link
Member

fwiw same happens with other characters such as @

@ahopkins
Copy link
Member

I guess the question then is what non-spec-compliant characters (if any) should we support?

@ahopkins
Copy link
Member

I am going to put this aside for now unless someone can make a compelling argument for why we need to handle this. It is the first time I have heard this come up and I am hesitant to add in support for this edge case when a viable alternative exists.

For anyone that needs:

from urllib.parse import quote

from sanic import Request, Sanic, json
from sanic.router import Router


class MyRouter(Router):
    def get(self, path, *args, **kwargs):
        return super().get(quote(path), *args, **kwargs)


app = Sanic("TestApp", router=MyRouter())


@app.route("/x:y")
async def handler(request: Request):
    return json({"foo": "bar"})

Or, perhaps a try/except is warranted:

    def get(self, path, *args, **kwargs):
        try:
            return super().get(path, *args, **kwargs)
        except NotFound:
            return super().get(quote(path), *args, **kwargs)

@Tronic
Copy link
Member

Tronic commented Dec 18, 2022

Closing as WONTFIX, given that it against specs and as @ahopkins noted a rare corner case with viable workarounds for those who need it anyway.

@Tronic Tronic closed this as completed Dec 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants