Skip to content
This repository was archived by the owner on Nov 6, 2023. It is now read-only.

Conversation

@injust
Copy link
Contributor

@injust injust commented Sep 1, 2018

No description provided.

@J0WI
Copy link
Contributor

J0WI commented Oct 14, 2018

Does this actually change/fix something?

to="https://$1.$2.omtrdc.net/" />

<rule from="^http://([\w-]+)\.d([1-3])\.sc\.omtrdc\.net/"
to="https://$1.d$2.sc.omtrdc.net/" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found that regex groups become potentially problematic over time. Could you take the possible targets and list them above? Or perhaps they should rely on the wildcard above? Otherwise that would be preferred with the trivial rule <rule from="^http:" to="https:" /> still in place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wildcard <target> is necessary because all subdomains exist.

I don't see a way to do the <rule>s without regex groups. We want to redirect http://a.sc.omtrdc.net but not http://a.a.sc.omtrdc.net.

@J0WI
Copy link
Contributor

J0WI commented Mar 9, 2019

@injust would be great if you could have a look here.

@injust
Copy link
Contributor Author

injust commented Mar 9, 2019

The existing ruleset would redirect http://a.b.d1.sc.omtrdc.net/optout.html to https://a-b.d1.sc.omtrdc.net/optout.html (note the hyphen), which is a different page.

It would also force HTTPS on a URL like http://a.b.c.d1.sc.omtrdc.net/, which is not covered by the certificate.

@ivysrono ivysrono removed the conflict label Sep 26, 2019
@ivysrono ivysrono closed this Sep 26, 2019
@ivysrono ivysrono reopened this Sep 26, 2019
@ivysrono ivysrono requested a review from zoracon September 28, 2019 05:59
@ivysrono
Copy link
Contributor

#17578

@ivysrono ivysrono closed this Sep 28, 2019
@ivysrono ivysrono removed the request for review from zoracon September 28, 2019 06:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants