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: add dictionary operations to DNR modifyHeaders #440

Closed
carlosjeurissen opened this issue Aug 16, 2023 · 2 comments
Closed

Proposal: add dictionary operations to DNR modifyHeaders #440

carlosjeurissen opened this issue Aug 16, 2023 · 2 comments
Labels
proposal Proposal for a change or new feature topic: dnr Related to declarativeNetRequest

Comments

@carlosjeurissen
Copy link
Contributor

carlosjeurissen commented Aug 16, 2023

Problem

Currently when attempting to change headers with a dictionary structure one can only set or remove the header. For many use cases like #169 and https://crbug.com/1254637, this is problematic. This proposal is there to make sure dnr can replace as many webRequest usecases.

In addition, if multiple extensions attempt to modify headers conflicts are likely to occur.

Proposal

In addition to the 'append', 'set' and 'remove' operations, we can introduce a dictionary operation with a set of key-value transformations. The value property will be an Array with headerDictionaryAction as such:

"responseHeaders": [{
  "header": "Set-Cookie",
  "operation": "dictionary",
  "value": [{
    "key": "SameSite",
    "operation": "set",
    "value": "Strict"
    }, {
    "key": "Secure",
    "operation": "set",
    "value": true
  }]
}]

The operations available for headerDictionaryAction are 'set', 'append', 'remove' and 'removeValue'.

"responseHeaders": [{
  "header": "Permissions-Policy",
  "operation": "dictionary",
  "value": [{
    "key": "geolocation",
    "operation": "removeValue",
    "value": "self"
  }, {
    "key": "geolocation",
    "operation": "append",
    "value": "self"
  }, {
    "key": "geolocation",
    "operation": "remove"
  }]
}]

Since headers using dictionaries have different syntaxes (for example, Content-Security-Policy, Permissions-Policy, Set-Cookie), this is something we have to consider.

The Content-Security-Policy header can have multiple dictionaries. Which makes this especially interesting. My proposal would be to have the operations be applied to each dictionary individually. Keep in mind CSP values with multiple dictionaries are very common, but common enough we need to handle them.

Say we have a Content-Security-Policy header with the following value:
default-src 'none'; img-src 'self'; script-src 'self'; frame-src 'self', default-src 'self'; media-src 'none', default-src 'none'; connect-src 'self'

The dictionaries are separated by commas. So we deal with three CSPs here, being:
default-src 'none'; img-src 'self'; script-src 'self'; frame-src 'none'
default-src 'self'
default-src 'none'; connect-src 'self'

With below operations:

"responseHeaders": [{
  "header": "Content-Security-Policy",
  "operation": "dictionary",
  "value": [{
    "key": "img-src",
    "operation": "removeValue",
    "value": "'self'"
  }, {
    "key": "frame-src",
    "operation": "removeValue",
    "value": "'self'"
  }, {
    "key": "media-src",
    "operation": "remove"
  }, {
    "key": "img-src",
    "operation": "append",
    "value": "https://example.com/image.png"
  }, {
    "key": "connect-src",
    "operation": "set",
    "value": "'none'"
  }]
}]

This would result in:
default-src 'none'; img-src https://example.com/image.png; script-src 'self'; frame-src 'none'; connect-src 'none'
default-src 'self'; img-src https://example.com/image.png; connect-src 'none'
default-src 'none'; connect-src 'none'; img-src https://example.com/image.png

Note the difference between the handling of frame-src and media-src. Since 'self' is removed from frame-src, we have a special situation in which no value is present. This would mean no value is present for frame-src. To keep the CSP valid, a value of 'none' would thus be added. If the preferred result is to completely remove it one can use the remove operator as demonstrated with media-src.

Which would then be merged as like such:
default-src 'none'; img-src https://example.com/image.png; script-src 'self'; frame-src 'none'; connect-src 'none', default-src 'self'; img-src https://example.com/image.png; connect-src 'none',default-src 'none'; connect-src 'none'; img-src https://example.com/image.png

@rdcronin
Copy link
Contributor

We discussed this at the WECG Meet-Up.

Browser vendors were agreed that, in a perfect world, we would like to support something like this. However, there's two major concerns we have with this approach:

  1. The complexity this introduces.
  2. In order to apply structured modifications to certain headers (such as CSP), we need to parse those headers. declarativeNetRequest evaluates rules directly in the privileged browser process (which is a major performance aspect of the API). However, in Chrome (and maybe other browser engines), we would be unable to parse untrusted string input into CSP in the browser process in a secure way (as of today). We would then need to punt this parsing to a sandboxed process, introducing significant latency.

Because of these, we're unlikely to pursue this at this time.

However, we agreed that CSP modification in particular may be worth exploring with other functionality (even independent of declarativeNetRequest, as CSP can also be provided through e.g. <meta> tags). We would definitely be open to other ideas of how we might pursue that.

We are also all supportive of allowing something similar (though less structured) as proposed in #439.

Thanks again for the proposal, @carlosjeurissen ; this was definitely a cool idea to explore.

@nir-walkme
Copy link

We would definitely be open to other ideas of how we might pursue that.

Hi @rdcronin . I have submitted a proposal. Happy to hear your thoughts on it: #574

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

No branches or pull requests

4 participants