[Wolfram.com] Update rulesets. #12386
Conversation
| <target host="www4d.wolframalpha.com" /> | ||
| <target host="www4f.wolframalpha.com" /> | ||
| <target host="www5a.wolframalpha.com" /> | ||
| <target host="www5b.wolframalpha.com" /> |
There was a problem hiding this comment.
These targets are redundant to the wildcard. Please transform them into test urls.
There was a problem hiding this comment.
The wildcard regex only covers ^http://(www[0-9][0-9]\.)?wolframalpha\.com/. Wolfram Alpha does not have HTTPS support on all subdomains, and hence replacing these with test urls would require the list to also be compacted into the regex, a practice that to my understanding is explicitly discouraged (?). It's not an ideal overlap, but this PR cuts the number of Wolfram rulesets in the whitelist from 3 to 1.
There was a problem hiding this comment.
This ruleset contains <target host="*.wolframalpha.com" /> and <rule from="^http:" to="https:" /> so it will rewrite anything to https. But you also listed some explicit targets like www5b.wolframalpha.com and special rewrites.
This is confusing and I think it does not behave like you planned.
So I would list all targets, keep the generic redirect and remove the wildcard target, or use a wildcard target with specific redirect rules and test urls.
There was a problem hiding this comment.
I think it does not behave like you planned.
You are very correct, such a rule would redirect all subdomains to HTTPS indiscriminately. Scarily, that seems to have also been the behavior of the previous ruleset, and probably explains why I copied it without thinking.
| - volunteer (sha1) | ||
| - www-cn (sha1) | ||
| - www-tw (sha1) | ||
| - www[0-9][0-9] (http only) |
There was a problem hiding this comment.
Please group by error type and be more precise.
|
@J0WI I feel like my comments frequently fail your expectations. Is there a checklist somewhere I can start running through, or is it mostly just experience? |
We have our CONTRIBUTING.md and some projects like #9906 |
|
LGTM now, Thanks! |
No description provided.