-
Notifications
You must be signed in to change notification settings - Fork 55
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
Comments
(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.) |
This is only a problem for the I'll have to dig in more, but at first glance it appears that the httprouter library is doing this undesired unescaping. |
Yeah, it looks like changing
But that might affect other users of httprouter, so I may have to add a config bool in my PR to the upstream lib. |
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. |
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:
I'm going to try hand-rolling a really minimal one that addresses this need specifically. |
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).The text was updated successfully, but these errors were encountered: