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

engine/esm/url_helpers.js: fix HTTPS rewriting corner case #281

Merged
merged 1 commit into from
Nov 3, 2023

Conversation

pkgw
Copy link
Contributor

@pkgw pkgw commented Nov 3, 2023

If we were given a URL like http://mydomain.com:80/ to rewrite in force-HTTPS mode, we'd change it to https://mydomain.com:80/, which obviously isn't going to work.

In that one case, it makes sense to rewrite to https://mydomain.com/ and see what happens. In cases with non-standard port specifications, that's definitely not going to work, so in those cases, we just have to keep the URL as insecure HTTP and hope the browser doesn't ding us.

This change motivated by an issue in ShowImage.aspx, which seems to have started including the :80 in the URLs it emits in its redirects, breaking the Astrometry.Net workflow. I'm not sure why that changed, and we could try to change it back, but this modification here makes the rewriting code more correct, and should fully solve the problem. I hope.

@Carifio24 Would you mind giving this a quick read-over and seeing if this seems sensible to you? I want to get this fixed quickly, but don't want to rush a dumb mistake into a release either.

If we were given a URL like `http://mydomain.com:80/` to rewrite in
force-HTTPS mode, we'd change it to `https://mydomain.com:80/`, which
obviously isn't going to work.

In that one case, it makes sense to rewrite to `https://mydomain.com/`
and see what happens. In cases with non-standard port specifications,
that's definitely *not* going to work, so in those cases, we just have
to keep the URL as insecure HTTP and hope the browser doesn't ding us.

This change motivated by an issue in `ShowImage.aspx`, which seems to
have started including the `:80` in the URLs it emits in its redirects,
breaking the Astrometry.Net workflow. I'm not sure why that changed, and
we could try to change it back, but this modification here makes the
rewriting code more correct, and should fully solve the problem. I hope.
@pkgw pkgw merged commit 252f582 into WorldWideTelescope:master Nov 3, 2023
9 checks passed
@pkgw pkgw deleted the rewrite-ports branch November 3, 2023 18:22
@Carifio24
Copy link
Member

@pkgw Sorry, didn't see this before you merged it in. But the approach here makes sense to me.

@pkgw
Copy link
Contributor Author

pkgw commented Nov 3, 2023

Here's an Astrometry.Net/WWT URL to test the use case, pointing at the testing webclient which uses the latest engine SDK package:

https://worldwidetelescope.org/testing_webclient/?wtml=http%3A%2F%2Fwwtcoreapp-data-app.azurewebsites.net%3A80%2Fwwtweb%2FShowImage.aspx%3Freverseparity%3DFalse%26scale%3D2.815695%26name%3Dtmpdea8lnfx%26imageurl%3Dhttps%3A%2F%2Fnova.astrometry.net%2Fimage%2F20835133%26credits%3DAstrometry.net%2BUser%2B(All%2BRights%2BReserved)%26creditsUrl%3D%26ra%3D324.529527%26dec%3D66.875874%26x%3D2105.0%26y%3D1087.6%26rotation%3D-128.66%26thumb%3Dhttps%3A%2F%2Fnova.astrometry.net%2Fimage%2F20835138%26wtml%3Dtrue

As hoped, this now works. Since I haven't yet made a new release of the engine, the production webclient currently fails:

https://worldwidetelescope.org/webclient/?wtml=http%3A%2F%2Fwwtcoreapp-data-app.azurewebsites.net%3A80%2Fwwtweb%2FShowImage.aspx%3Freverseparity%3DFalse%26scale%3D2.815695%26name%3Dtmpdea8lnfx%26imageurl%3Dhttps%3A%2F%2Fnova.astrometry.net%2Fimage%2F20835133%26credits%3DAstrometry.net%2BUser%2B(All%2BRights%2BReserved)%26creditsUrl%3D%26ra%3D324.529527%26dec%3D66.875874%26x%3D2105.0%26y%3D1087.6%26rotation%3D-128.66%26thumb%3Dhttps%3A%2F%2Fnova.astrometry.net%2Fimage%2F20835138%26wtml%3Dtrue

The problem is that the ShowImage.aspx now thinks that its origin is http://wwtcoreapp-data-app.azurewebsites.net:80, when it used to get set to http://worldwidetelescope.org. This might be caused by the configuration change in the Azure App Gateway frontend to activate Constellations, but the way things are set up I didn't think that would affect this. It's possible that some other configuration change snuck in, or just Azure changed the behavior of something at some point.

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

Successfully merging this pull request may close these issues.

2 participants