Strip any number of trailing slashes from realm URLs

[chrisbobbe]

This does the following, to fix #3567:

Things I expect we'll want to do to clean this up:

• Tweak fixRealmUrl so it strips any number of trailing slashes.
• Add a migration that strips trailing slashes on realm in all existing accounts.

It also removes some "DEPRECATED" comments that don't seem to apply.

[chrisbobbe]

I wonder if importing and using fixRealmUrl for the migration is correct, or if we should instead clone fixRealmUrl and use that, so future changes to it don’t unintentionally change the migration’s behavior.

[rk-for-zulip]

I still think it's possible to configure a proxy server in such a way that the extra slash is needed; but if that's explicitly not a case we want to support, I'm okay with that, as long as we make a note of it somewhere. (Possibly this comment.)

---

hasProtocol was marked DEPRECATED in e0be99a, and fixRealmUrl was marked as DEPRECATED in 4b687d4, but neither commit explained the reason for the marking, and they're both still being used.

Mea culpa. I marked those as /** DEPRECATED */ mostly because I think we should move toward using some sort of URL-object, rather than passing around a bare string to do ad-hoc regex hacking on. I may also have done so because:

• They both have subtle edge cases and fail to conform to the relevant specifications.
• fixRealmUrl is a DWIM function, and is therefore closely tied to its context. It shouldn't be used anywhere but SmartUrlInput.js (and so shouldn't be in url.js at all, much less imported by anyone else).
• Most of fixRealmUrl's functionality is duplicated in or has been moved to autocompleteRealm[Pieces] anyway. It probably isn't even needed in SmartUrlInput.js anymore.

At any rate, I certainly believe the latter two of those things now, and wouldn't be surprised by the first. 🙂

I wonder if importing and using fixRealmUrl for the migration is correct, or if we should instead clone fixRealmUrl and use that, so future changes to it don’t unintentionally change the migration’s behavior.

Neither, I think. You're on the right track with the second of those two, but fixRealmUrl does more than just strip trailing slashes. (It's supposed to be idempotent, but that's probably not safe to rely on.)

I'm pretty sure we want the migration to just strip slashes, nothing more.

[chrisbobbe]

I still think it's possible to configure a proxy server in such a way that the extra slash is needed; but if that's explicitly not a case we want to support, I'm okay with that, as long as we make a note of it somewhere. (Possibly this comment.)

Hmm, I don't have a competent answer to this. @gnprice?

I think we should move toward using some sort of URL-object

If that's the case, maybe larger changes are needed, and this PR should be closed?

I'm pretty sure we want the migration to just strip slashes, nothing more.

Yes, that makes a lot of sense. I just updated the PR with this (and as always, feedback is welcome).

[rk-for-zulip]

If that's the case, maybe larger changes are needed, and this PR should be closed?

Setting aside my other concerns for a moment: I think this is a perfectly fine temporary mitigation – it won't make moving to a URL-object any harder, if and when we do that.

[chrisbobbe]

OK, sounds good. I've added that inline comment and made some wording/conventions improvements to the commit messages. And, here, just a gentle bump to @gnprice to take a look at Ray's question about proxy server configuration and the scope of what we'll support, above.

[gnprice]

Marking as priority, following the issue.

[gnprice]

(Bumping this to remind myself to come back to it next week.)

[gnprice]

Thanks @chrisbobbe ! Sorry this has sat for so long.

These changes LGTM. About to merge.

I still think it's possible to configure a proxy server in such a way that the extra slash is needed; but if that's explicitly not a case we want to support,

Yeah, Zulip in general doesn't support going through a proxy that involves altering the URLs. Like many complex webapps, it has a notion of "what is my base URL", and using that it expects to be able to know exactly what URL from the user perspective corresponds to a given route.

I'm not sure we've ever had someone run into trouble from this kind of attempted proxy use case; if that does come up, we'll address it in our production docs.

Note we absolutely do support reverse proxies, which are a common tool in deploying webapps. A working reverse proxy has no problem setting up appropriate URLs.

[chrisbobbe]

Thanks @chrisbobbe ! Sorry this has sat for so long.

No problem! 🙂