Repository navigation
Enforce Grammar for default_off Messages #9906
Description
Reason
Unlike the platform attribute, there is not any grammatical requirement on the default_offattribute, This make the process of performing automated check to re-activate disable ruleset difficult.
To illustrate the situation, we have mismatch, mismatches, mismatched and their capitalization variants for the mismatched error. These variants make it hard for contributors to perform audit. See #9582, #9842
A particularly bad example could be BufferedIO.xml. It should be a platform=cacert ruleset, but the message is given in default_off. (It is a mismatch now)
Goal
The goal is to standardize the default_off attributes and to enforce the grammar with relaxng.xml such that automated audit can be done easier.
Steps
-
Modify
default_offattributes in the existingrulesetto an agreed set ofkeyword, add therulesettoruleset-coverage-whitelist.txtif necessary. See Update default_off="mismatched" rulesets #9884. -
Update
CONTRIBUTING.mdto explain thekeywordwe use. -
Enforce the grammar with
relaxng.xml.
Resource
Patterns
Each keyword should be separated by a comma and a space
(keyword1|keyword2)(, (keyword1|keyword2))*
Note
Some rule use ' (single quote) instead of " (double quote), it will be great if you can do a replacement as wel; :)
Keywords
List of keywords (feel free to suggest new ones)
regional
refused
timeout
cert-algo
cert-chain // curl: (60) SSL certificate problem: unable to get local issuer certificate
expired
self-signed
mismatched
ssl-error // reset, or any other ssl error not stated above
status-unexpected // 200 Error pages
status-others // other than 200, i.e. 4xx/5xx
loops // secure connection redirects to plaintext
content-different // visual/ contextual difference, also for different status code
breaks-site // in a board sense.
breaks-third-parties
cors // CORS issues
ruleset-test-failed // failed ruleset test (avoid space within keywords)
request-owner
request-user
others // details should be stated in the ruleset comment
Existing Variants (200+ Variants)
Updated 2017.06.28 Please refer to default-off-attributes.txt
Activity
Bisaloo commentedon May 13, 2017
Aren't
cert-chainandcert-local-issuerthe same thing?Bisaloo commentedon May 13, 2017
[York.edu] Add subdomains and reactivate ruleset #9911 fixesCertificates are invalid[Pastebin.ca] Add subdomains and reactivate ruleset #9913 fixesNo HTTPS support anymore[RecycleNow.com] Update, rename and reactivate ruleset #9918 fixesSite does not support HTTPS currentlycschanaj commentedon May 13, 2017
@Bisaloo You're right, I will modify the list according.
Bisaloo commentedon May 14, 2017
mismatchedandstatus-mismatched?SSL_ERROR_NO_CYPHER_OVERLAPorSSL_ERROR_RX_RECORD_TOO_LONGlike forurlquery.net)?cschanaj commentedon May 14, 2017
@Bisaloo
mismatchedrefers to acertificate mismatchederror.status-mismatchedrefers to the situation whereHTTPandHTTPSresponse clients with differentHTTP status code, likeHTTP (200)andHTTPS (404 with valid certificates). The problem is that it breaks Travis build and requires sometest url. Now I think it should be merged withruleset-test-failed, do you have any preference or suggestion?I would like a
ssl-errorkeyword to be used in the situation you mentioned (and any other complex SSL errors not stated above) because these errors make little difference to the ruleset auditing process (please correct me if I am wrong).Remark
audit-requiredfrom the list ofkeywordbecause it is too ambiguous.Bisaloo commentedon May 14, 2017
I see. I would put that as
content-different.No, I agree. I think we can also merge
resetwithssl-error.cschanaj commentedon May 14, 2017
Alright. Let's put
Different HTTP status codeascontent-differentandresetasssl-error.Updated I think that we will need add a section for the
default_offattributes inCONTRIBUTING.mdwhen we are closed to resolving this issues.J0WI commentedon May 14, 2017
I'm not really happy with
default_off="regional". Maybe a user wants to enable/disable only the rules for his own region, but not for other regions.EDIT: Sorry, accidentally closed.
jeremyn commentedon May 14, 2017
What exactly is the "ruleset auditing process" mentioned in #9906 (comment)? The specific process should make a difference in what keywords we want. (And if it doesn't make a difference, then what's the point of this issue anyway?)
I'm not sure what
regionalis for. I guess it's for the "only available in China" problems? Chinese sites are a special case due to the Great Firewall. We should get @gloomy-ghost 's and @ivysrono 's input on what they would like to see for that, though I know they are both busy with other things right now.We should have only a small number of categories. Each extra category is one more thing to get confused about and one more potential argument to have with contributors.
For a while now in reviewing rulesets I've preferred
invalid certificateto a more specific categorization in ruleset top comments. Here I would combineexpired,self-signed,mismatched,cert-chain, andcert-algointoinvalid-certificate.I think
otheris slightly better wording thanothers.Small, infrequent problems l should be moved into
other. For example if there are only a few CORS rulesets, thencorsshould be included underother.I don't know what
request-ownerandrequest-usermean.We might want to allow extra attributes, such as
date-expired="2017-05-14"forinvalid-certificate.Without knowing more about what an audit looks like, I would prefer just these keywords:
regional(to cover "only available in/always breaks in X region"),refused,timeout,invalid-certificate,other, and possiblycorsif there are a lot ofdefault_offfor those.jeremyn commentedon May 14, 2017
I just want to add that I think it's great people are working on automating the review process 😃
cschanaj commentedon May 14, 2017
@jeremyn thank you for joining the discussion!
To me, the audit process means to re-activate (or delete) the
default_offrulesets if possible (either automatically/ manually). We have around 4kdefault_offruleset and quite a number of them have not been re-activate for years. When a cleanup is necessary, the huge number of variants of thedefault_offmake the process troublesome.In particular, a contributor might want to focus on a specific type of
default_off, e.g. anexpiredis much more likely to be fixed by web-master than arefused. And acert-chainmight require contributor to use a new Firefox profile. Without a consistentdefault_offmessage, a large portion of time will be spent on searching for ruleset.Yes. There is only 5 files will be applying the
regionalkeyword AFAIK. See #9929. @gloomy-ghost, @ivysrono please let me know what do you prefer.I agree that we should have fewer categories. Suppose that we have an automated process since they are basically the same to a robot (or a script), but I am not sure if this will affect manual audit or not. (I use a bot to re-generate a ruleset for a
default_offhost almost every time I would like to perform an audit and I do createdefault_offruleset myself). :)I am OK with
expired,self-signed,mismatched,cert-chainandcert-algomerged into the categorycert-invalidif there is no problem with that.request-ownermean web-master/ site owner want to have the ruleset disabled, like #9528request-userhttps-everywhere user reported that the ruleset is causing problems, like #9371If adding extra attributes is an option, I would really love to see an
date-expiredattribute. Beside, we might want to have something likephase-out="2022"(YYYY+5) to schedule the removal ofdefault_offruleset.Which category should
failed ruleset testgo into in this case? We have 1.7k such ruleset (nearly half of the thedefault_offjeremyn commentedon May 15, 2017
@cschanaj I get the gist of what the auditing process is -- someone does a manual or semi-automated search over the rulesets looking for categories of
default_off-- but the specific process/program/script used and its goals matter. For example if we want a detailed analysis then we should use fine-grained categories, but if we are trying to quickly identify "junk" rulesets then maybe broad categories are better.Perhaps more compelling is that I don't like the idea of the hassle of constraining
default_offvalues to support an auditing program that we can't see and have no input into. We don't want to have to justify these categories to contributors on the sole basis of "@cschanaj says this helps them with their auditing script" (no offense intended to you). Even worse is imagine in five years, suppose none of us here are involved with the project any more, and the future maintainers see the categories in the XML grammar but have no idea why they are there and what may break if they change them or if they add new categories. It introduces a dependency that we can't publicly see.For
regional, maybe we should mention China (gfw-of-china-issuesor something) specifically since it is so far the only region that has special issues.request-ownercould instead beat-website-admin-request, so thatdefault_off="at-website-admin-request"reads in a nice way.request-useris not a valid category, we should only disable a ruleset if there is a verifiable reason or the site owner requests it. We wouldn't saydefault_off="someone-reported-a-problem".I disagree with a
phase-outattribute, we don't need to include a specific policy like a five-year grace period in the ruleset. Also doing even that simple math by hand would be tedious and error-prone. People doing the later analysis can decide if a ruleset should be disabled based on policy or common sense.failed ruleset testis also not a valid category, it basically meansbroken. Since you say there are 1.7k of these rulesets then unfortunately unless a robust automated review program appears, no one is going to systematically review them all, so I suggest we mark them asunknown-legacyor something as a one-off. Future maintainers can delete them as a group if they become too burdensome to keep for some reason.14 remaining items
Hainish commentedon May 17, 2017
I should note that the discussion around browser prefs will be obsoleted in the transition to WebExtensions. Extensions built in this newer API will not be able to do pref introspection, see https://wiki.mozilla.org/WebExtensions/FAQ#Will_I_have_access_to_about:config_or_the_preferences.3F. This transition will happen in the next few months.
As far as the
default_offstring goes, since this is exposed in the WebExtensions UI, I'd be in favor of keeping it descriptive but concise. Without user testing, I'm unsure how many users ever see thedefault_offstrings, but I would guess it isn't very many. Still, it is exposed and should be treated as user-facing - thanks @gloomy-ghost for pointing this out.The comments in the rulesets are for maintainability only, and never make it into the actual extension, so we can't rely on any mechanism of scanning comments for in-extension functionality.
We already have the
platformattribute which is underutilized, mainly to indicate which rules should be enabled when mixed content blocking is disabled. This attribute seems appropriate when indicating certain types of rules, such as those that should be enabled on platforms within the GFW. We could extend the grammar ofplatformso that it indicates a comma-separated set of values, which we can document withinCONTRIBUTING.md. Using this attribute we can then parse those values and take appropriate in-extension action.I'm generally in favor of having a high level of granularity when documenting the certificate errors, for the reason described here: #9906 (comment). Expired certs are more likely to be renewed quickly than, say, self-signed certs. But I will defer to the ruleset maintainers on what level of granularity is useful or appropriate here.
jeremyn commentedon May 17, 2017
@Hainish Is it possible to override what the user sees from the tooltip hover? For example, we write
cert-invalidbut the user seesInvalid certificate. Keep in mind there are thousands of these rulesets so this basic level of abstraction is important. We don't want pull requests changing thousands of files to modify a line of descriptive text.Out of all these categories we've come up with, only
regionalmakes sense to me as a platform, and only then if we actually specify the region e.g.china. I disagree with @J0WI thatcert-chainis a good platform. As I understand it, whether a specific user gets a cert chain error for a particular site depends on whether they have randomly visited another site that is correctly configured and serves the same intermediate certificate that the browser then caches. It doesn't make sense to enable all of these rulesets at once.Sort of tangential but "comment maintenance" is a disproportionately large and tedious part of ruleset work. The comments are not standardized in either form or content and are not currently reviewable by automated testing. Recently a pull request was unmerged for almost two months because of an extremely minor comment problem that I waited for the contributor to make, until I finally merged it as-is and opened another PR for the minor change. At the same time I can't remember any case where a big list of broken subdomains made a real difference in maintaining a ruleset. It's a lot of work for little gain, not a pattern I want to see replicated here (or anywhere else).
cschanaj commentedon May 17, 2017
@Hainish I agree that we should have
regionalaplatformif it is possible to activate them by default for china users behind the GFW. However, we lack a measure to tell the users to disable-this-rule-when-you-are-on-a-trip sinceplatformis not exposed to the users. Any advise/ solution to this?@jeremyn I would like to see this enhancement happening as well, this improve readability of the
default_offmessages. Though I would like to noted that thedefault_offmessage is not visible toFirefoxusers AFAIK, so it will be achromium-onlyfeature. Any chance the tooltip hover (or something similar) will be available onFirefoxalso?Apart from this issue, we might get exceptionally lengthy
default_offif a domain is poorly configured, for exampleinstead of
cschanaj commentedon May 17, 2017
I do not see which category
Ghostery_metrics_optout.xmlshould fall into. The only rule redirectd.ghostery.com(with a working HTTPS connection) tolocalhost.P.S. Personally, I do not think this ruleset match the goal of https-everywhere because it blocks connections rather than redirecting users to secure connections. Any suggestion?
ivysrono commentedon Jun 11, 2017
BTW, #10282 #10288 #10292
There are just 2 ruleset that only available in China now.
Bisaloo commentedon Jun 15, 2017
Can we settle on a set of keywords to use in the
default_offfield? I am okay with whatever solution is chosen but I'd like to see this resolved.We can't really start to work on the rulesets as long as we haven't decided which keywords to use and each day new rulesets with potentially new
default_offreasons are pushed.jeremyn commentedon Jun 15, 2017
I don't see the urgency. No one is creating new
default_offrulesets as far as I know.For:
What does this mean?
cschanaj commentedon Jun 16, 2017
@Bisaloo currently, I work on this issue by either re-activating the ruleset/ deleting the obsolete ruleset. IMHO, it is fine if someone create
default_offruleset as usual at the moment, as long as it is not a massive update like #9884 . If you prefer some suggested keywords, #9906 (comment) should work well.In fact, I did some simple statistics on this issue in #10378. It looks really good to me, so I do not think it is urgent to resolve this.
Bisaloo commentedon Jun 21, 2017
What about redirect loops? For example
Strawpoll.me.xml.cschanaj commentedon Jun 22, 2017
@Bisaloo I think that
otherfrom #9906 (comment) is good enough for this.ivysrono commentedon Jul 5, 2017
#10522
Reduce CN only rule as much as possible
Bulk change default_off to cert-chain, related #9906 (#17407)
pipboy96 commentedon Oct 25, 2019
@zoracon Do you think this should be enforced by ruleset checker at some point?
zoracon commentedon Aug 10, 2020
To be honest, not sure. I want to go over the ruleset checker at some point after I finally get the fetch test issues resolved. But closing this for now since it seems like this came to a decent conclusion.