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

Ensure inner URL = outer URL where possible #310

Closed
twifkak opened this issue May 21, 2019 · 5 comments
Closed

Ensure inner URL = outer URL where possible #310

twifkak opened this issue May 21, 2019 · 5 comments
Assignees

Comments

@twifkak
Copy link
Member

twifkak commented May 21, 2019

curl -s -H 'Accept: application/signed-exchange;v=b3' -H 'AMP-Cache-Transform: any' \
  https://amppackageexample.com/inde%78.html

returns an SXG whose inner URL has had the %78 unescaped. This doesn't meet Google's cache requirements.

amppkg should try to ensure inner URL = outer URL as much as possible (i.e. where doing so doesn't contradict the SXG requirements for fallbackUrl or the recommendations to mitigate content sniffing).

@twifkak twifkak added this to the v3: b3 + local transforms milestone May 21, 2019
@twifkak
Copy link
Member Author

twifkak commented May 21, 2019

(Alternatively/additionally, could have Google loosen its cache requirements, though it should be careful about doing so -- e.g. may not want to equate reserved chars with their encoded equivalents.)

@twifkak twifkak self-assigned this May 21, 2019
@twifkak
Copy link
Member Author

twifkak commented May 22, 2019

This is only a problem for the /priv/doc/https://blah form of request. If the frontend server encodes the request as /priv/doc?sign=https%3A%2F%2Fblah, the issue doesn't occur.

I'll have to dig in more, but at first glance it appears that the httprouter library is doing this undesired unescaping.

@twifkak
Copy link
Member Author

twifkak commented May 22, 2019

Yeah, it looks like changing Path to EscapedPath() in

fixes it.

But that might affect other users of httprouter, so I may have to add a config bool in my PR to the upstream lib.

@twifkak
Copy link
Member Author

twifkak commented May 22, 2019

Looks like there's a long-standing bug for this at julienschmidt/httprouter#208, with long-standing PRs to fix it at julienschmidt/httprouter#209 and julienschmidt/httprouter#55. Rather than forking (given it may need to exist for quite a while), consider using a different library that doesn't have this problem, such as https://github.com/dimfeld/httptreemux, which says it's inspired by httprouter, so probably has similar semantics/behavior in other ways.

@twifkak
Copy link
Member Author

twifkak commented May 23, 2019

It looks like https://github.com/dimfeld/httptreemux has the same bug buried deep, despite having a config option with a default meant to avoid this bug. The commit that added this appears unrelated, so it seems unintentional.

So, given that:

  • This is evidently a pretty niche need for an http mux.
  • Our other http mux feature needs are really minimal.

I'm going to try hand-rolling a really minimal one that addresses this need specifically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant