Further improve Steam ruleset #15088
Conversation
xPaw
changed the title
src/chrome/content/rules/Steam.xml
Outdated
| to="https://store.steampowered.com/" /> | ||
|
|
||
| <rule from="^https?://((cdn|cdn\.store|www)\.)?steampowered\.com/" | ||
| <rule from="^http://((cdn|storefront|www)\.)?steampowered\.com/" |
There was a problem hiding this comment.
steampowered.com is not listed in your targets
There was a problem hiding this comment.
Good point. Tests didn't catch that.
There was a problem hiding this comment.
This rule does not work for deep links.
See http://storefront.steampowered.com/v/gfx/apps/210950/capsule_467x181.jpg
There was a problem hiding this comment.
I had a rule to cover /v/gfx before, but CI complained there was no test for it (even though there was). Do tests/rules work for covering paths and not just domains?
Also /v/gfx isn't directly used anywhere.
There was a problem hiding this comment.
Do tests/rules work for covering paths and not just domains?
Yes, they do. Since you are force pushing, I can't really tell what was wrong.
There was a problem hiding this comment.
I've pushed it again so you can take a look.
There was a problem hiding this comment.
It's because it's already rewritten by the previous rule. You should try switching L40 and L43. That should help.
If that does not work, you will need to use a stricter rule but let's try this first.
src/chrome/content/rules/Steam.xml
Outdated
| <target host="help.steampowered.com" /> | ||
| <test url="http://help.steampowered.com/" /> |
There was a problem hiding this comment.
This already exists as an implicit test url. Please remove all similar test urls.
You can read more here: https://github.com/EFForg/https-everywhere/blob/master/ruleset-testing.md
@cschanaj, didn't the recent change in travis tests prevent this?
src/chrome/content/rules/Steam.xml
Outdated
| @@ -7,47 +7,56 @@ | |||
| <test url="http://partner.steam-api.com/" /> | |||
|
|
|||
| <target host="steamcommunity.com" /> | |||
| <test url="http://steamcommunity.com/" /> | |||
| <test url="http://steamcommunity.com/broadcast/getbroadcastmpd/?steamid=76561197966726415" /> | |||
| <target host="*.steamcommunity.com" /> | |||
There was a problem hiding this comment.
There is a very small number of subdomains for this domain. Could you list them all? We are discouraging the use of wildcards.
|
All fixed up. |
| <target host="partner.steamgames.com" /> | ||
|
|
||
| <target host="steampowered.com" /> |
There was a problem hiding this comment.
Could you sort the subdomains according to our contributing guide?
| to="https://steamcommunity.com/" /> | ||
|
|
||
| <rule from="^http://(cdn\.steamcommunity|community\.akamai\.steamstatic|community\.edgecast\.steamstatic)\.com/" |
There was a problem hiding this comment.
There was a problem hiding this comment.
This rule doesn't cover the URL you posted?
There was a problem hiding this comment.
There was a problem hiding this comment.
What's the problem? If you change the domain to akamai cdn, it still works.
There was a problem hiding this comment.
We usually try to avoid this because it makes rulesets fragile. As soon as they change their CDN provider, it breaks things.
If @apaloma can get someone from Valve to officially comment that they reviewed the complex rules, I am okay with doing an exception.
There was a problem hiding this comment.
I've pinged him about this. But subdomains cdn.steampowered.com, cdn.store.steampowered.com and cdn.steamcommunity.com aren't really used. They use akamai.steamstatic.com and akamaihd.com variants on store and community.
There was a problem hiding this comment.
Yes, @Bisaloo I work for Valve (http://www.valvesoftware.com/email.php?recipient=Tony+Paloma) and provided @xPaw with rules here: #15087 (comment)
It is possible we change CDN at some point in the future, but unlikely that occurs before we're 100% HTTPS.
|
This does not seem safe. Please remove all complex rules (excepted redirected between If steam is still rolling out HTTPS support, we can add this a later time in a way that doesn't risk breaking the website for many users. |
|
Well, I checked 3 of them and 2 were already wrong. :/ How do you know @apaloma is a Valve employee? It is not listed on their profile. If that is indeed the case, please re-check the rewrites. |
|
I can add the I know he's an employee because of a contact outside of Github. |
|
For the record, I disagree with merging smaller rulesets into a big complex ruleset. I makes further updates much more of a pain. In my opinion, we should always try to stick to one domain per ruleset. However, since this is already a big improvement compared to the current master version, I am merging it as it is. |
#15087 (comment)