Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: declarativeNetRequest: Add a way to feature detect whether a specific RuleConditon is supported on the current browser #638

Closed
danielhjacobs opened this issue Jun 12, 2024 · 7 comments
Labels
topic: dnr Related to declarativeNetRequest

Comments

@danielhjacobs
Copy link
Contributor

danielhjacobs commented Jun 12, 2024

I am trying to test Chrome Canary/Dev's support for matching a Rule based on responseHeaders, but I ran into an issue. On Chrome stable, my rules ignore the responseHeaders RuleCondition but are still applied with all the other conditions in effect. A RuleCondition I used (below) therefore triggers on every URL on Chrome stable, which is undesired behavior, but I don't want to up the minimum browser version for my extension to one that supports this condition just for a minor QOL improvement:

            {
                id: 1,
                action: {
                    type: chrome.declarativeNetRequest.RuleActionType.REDIRECT,
                    redirect: { regexSubstitution: chrome.runtime.getURL("/player.html") + "#\\0" },
                },
                condition: {
                    regexFilter: ".*",
                    responseHeaders: [
                        {
                            header: "content-type",
                            values: [
                                "application/x-shockwave-flash",
                                "application/futuresplash",
                                "application/x-shockwave-flash2-preview",
                                "application/vnd.adobe.flash.movie",
                            ],
                        },
                    ],
                    resourceTypes: [
                        chrome.declarativeNetRequest.ResourceType.MAIN_FRAME,
                    ],
                },
            },

I'd therefore like to use feature detection to check if the responseHeaders RuleCondition is supported before adding it, but there doesn't seem to be any check that can do this. An example of an API that has such a check is the scripting API, which has chrome.scripting.ExecutionWorld.MAIN defined only when chrome.scripting.RegisteredContentScript supports the MAIN world.

This issue based on #460 (comment)

@github-actions github-actions bot added needs-triage: chrome Chrome needs to assess this issue for the first time needs-triage: firefox Firefox needs to assess this issue for the first time needs-triage: safari Safari needs to assess this issue for the first time labels Jun 12, 2024
@danielhjacobs
Copy link
Contributor Author

danielhjacobs commented Jun 12, 2024

A solution just for responseHeaders, but which wouldn't work for any other RuleCondition, might be making support for HeaderInfo (https://source.chromium.org/chromium/chromium/src/+/main:extensions/common/api/declarative_net_request.idl;l=174) detectable.

@oliverdunk
Copy link
Member

This is related to #449. I think we might even be able to mark this as a duplicate, but leaving it open so others can see.

@Rob--W
Copy link
Member

Rob--W commented Jun 13, 2024

Unrecognized properties in static rules are ignored silently as agreed upon in #466. In Chrome and Firefox, invoking declarativeNetRequest.updateDynamicRules or declarativeNetRequest.updateSessionRules with unsupported properties or values throws. (in Safari an error is only raised if the property is recognized but the value invalid)

@danielhjacobs
Copy link
Contributor Author

danielhjacobs commented Jun 13, 2024

When I add a dynamic rule with an unrecognized condition in Chrome (such as a responseHeaders condition in Chrome stable) that's also silently ignored while the other conditions are used for the rule.

@hanguokai hanguokai added the topic: dnr Related to declarativeNetRequest label Jun 13, 2024
@danielhjacobs
Copy link
Contributor Author

danielhjacobs commented Jun 13, 2024

If this became a failed rule and that API were added, I'd be able to do my desired feature detection for a dynamic ruleset, but it does not become a failed rule in Chrome.

if (typeof chrome.declarativeNetRequest.getFailedRules !== "function") {
        await utils.declarativeNetRequest.updateDynamicRules({
            removeRuleIds: [1, 2, 3],
        });
}

I wouldn't even need to check if there were failed rules and remove them since I assume they'd fail to register, which is my desired behavior anyway.

@danielhjacobs
Copy link
Contributor Author

danielhjacobs commented Jun 13, 2024

In Chrome and Firefox, invoking declarativeNetRequest.updateDynamicRules or declarativeNetRequest.updateSessionRules with unsupported properties or values throws. (in Safari an error is only raised if the property is recognized but the value invalid)

In Chrome, using an unrecognized condition didn't throw though. Should it?

An invalid RuleCondition causing the Rule itself to throw would be the desired behavior for my use-case.

@Rob--W Rob--W removed needs-triage: chrome Chrome needs to assess this issue for the first time needs-triage: safari Safari needs to assess this issue for the first time needs-triage: firefox Firefox needs to assess this issue for the first time labels Jun 20, 2024
@Rob--W
Copy link
Member

Rob--W commented Jun 20, 2024

During today's meeting, we agreed that it would be useful to have a way to detect invalid rules, and that #449 is the issue to track that.

Note that the fact that Chrome does not throw is a bug (also confirmed by @oliverdunk during the meeting), because the ordinary behavior is to throw for unsupported values. It seems that even if the feature is disabled behind a flag (--enable-features=DeclarativeNetRequestResponseHeaderMatching), that the parameter validation still works. It is likely that an isRuleSupported method as suggested by #449 would suffer from the same issue.

To detect the feature, you could take advantage of the fact that Chrome performs some validation independently of the automatically generated validation derive from the IDL definition, which is skipped if the feature is disabled: https://source.chromium.org/chromium/chromium/src/+/main:extensions/browser/api/declarative_net_request/indexed_rule.cc;l=766-791;drc=90cac1911508d3d682a67c97aa62483eb712f69a

(edit: improved example to create a rule that never matches, with urlFilter, to make sure that the feature detection logic does not inadvertently block requests. Copied from https://github.com/mozilla/pdf.js/blob/96cdff4a73a43e2f9259aed015ba8dcf9c169766/extensions/chromium/pdfHandler.js#L222-L265)

// Usage:
console.log(await isHeaderConditionSupported()); // true or false

async function isHeaderConditionSupported() {
  const ruleId = 123456; // Some rule ID that is not already used elsewhere.
  try {
    // Throws synchronously if not supported.
    await chrome.declarativeNetRequest.updateSessionRules({
      addRules: [
        {
          id: ruleId,
          condition: {
            responseHeaders: [{ header: "whatever" }],
            urlFilter: "|does_not_match_anything",
          },
          action: { type: "block" },
        },
      ],
    });
  } catch {
    return false; // responseHeaders condition not supported.
  }
  // Chrome may recognize the properties but have the implementation behind a
  // flag. When the implementation is disabled, validation is skipped too.
  try {
    await chrome.declarativeNetRequest.updateSessionRules({
      removeRuleIds: [ruleId],
      addRules: [
        {
          id: ruleId,
          condition: {
            responseHeaders: [],
            urlFilter: "|does_not_match_anything",
          },
          action: { type: "block" },
        },
      ],
    });
    return false; // Validation skipped = feature disabled.
  } catch {
    return true; // Validation worked = feature enabled.
  } finally {
    await chrome.declarativeNetRequest.updateSessionRules({
      removeRuleIds: [ruleId],
    });
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: dnr Related to declarativeNetRequest
Projects
None yet
Development

No branches or pull requests

4 participants