-
Notifications
You must be signed in to change notification settings - Fork 199
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
Sec-Browsing-Topics header format seems a bit awkward #183
Comments
The trade-off here is bytes on the wire (~15 bytes per version) vs ease-of-use. Combined with the desire to pad the header to ensure that we're not leaking the number of topics sent, with your proposal we'd always need to send 3 versions (45 bytes for versions per request). The folks that we expect to use the API is pretty small in number (ad-tech) and generally pretty experienced. I agree that parsing it requires slightly more logic than simply reading a list, but it's pretty rudimentary.
If a proxy were to reorder the list that seems bad on many levels? Sometimes ordering matters. In the end I don't feel strongly here and could be persuaded either way, but we'll wind up having large padding buffers on many requests as a result. |
When you say "many requests", you just mean the requests which have opted in to getting this data, right? As you say, this is a pretty small number of users. Given that those are generally for multi-kilobyte documents, it seems to me like penny-pinching to worry about 45 bytes. |
The number of users is small, but their reach is enormous. Let's assume at least a few requests per page on a significant fraction of page loads, ~50%? |
It still seems not worth worrying about, compared to the transfer sizes of ads themselves. |
In an offline chat, @jyasskin suggested using inner lists to group:
This is nice! However we then have to extend it with the padding idea from #186, which makes it more awkward. The nice thing about #186 is that consumers know they can just throw away the entire map entry with key I think the following is a reasonable way of getting both benefits:
This seems like it would be pretty easy to parse because it's effectively saying "there are no topics with version = Detailed comparison of parsing easeAssume our software's goal is to create a list of Simplest format with padding
const parsed = genericStructuredHeaderParser(headerValue);
let result = [];
for (const item of parsed["t"]) {
result.push(item.value, item.params["v"]);
} Abbreviated format with padding (removes repeated versions)
const parsed = genericStructuredHeaderParser(headerValue);
let result = [];
let lastVersion;
for (const item of parsed["t"]) {
if (item.params["v"]) {
result.push(item.value, item.params["v"]);
lastVersion = item.params["v"];
} else {
result.push(item.value, lastVersion);
}
} Inner list with groups with padding
const parsed = genericStructuredHeaderParser(headerValue);
let result = [];
for (const innerList of parsed) {
const version = innerList.params["v"];
for (const topic of innerList) {
result.push(topic.value, version);
}
} (The inner loop is a no-op for the zero-length padding inner list.) |
I also like Jeffrey's suggestion. How about a compromise that keeps the dictionary which makes it a bit more expicit that it's for padding and uses the same number of characters when whitespace is removed? Having a dictionary also allows us to add extra keys in the future if necessary which is a nice-to-have.
const parsed = genericStructuredHeaderParser(headerValue);
let result = [];
for (const innerList of parsed["t"]) {
for (const topic of innerList) {
result.push(topic.value, innerList.params["v"]);
}
} Correction: Not exactly the same number of chars, but quite close. |
Inner lists can't have inner list members :( |
Ah, right. That's annoying. Okay, I'd prefer to rename the pad param to 'p' in your example, like so:
And then the code would be: const parsed = genericStructuredHeaderParser(headerValue);
let result = [];
for (const innerList of parsed) {
for (const topic of innerList) {
result.push(topic.value, innerList.params["v"]);
}
} There is no longer a guaranteed |
Yao points out that structured headers do support '.' and ':' in tokens. |
Changed the format of the topics request header from: "t=(1;v=chrome.1:1:2, 2), p=P00000" to "(1 2);v=chrome.1:1:2, ();p=P00000" As discussed in patcg-individual-drafts/topics#183 Bug: 1443540 Change-Id: I3ea40df9f8732f5eff953110f22f4da2163480fb
Changed the format of the topics request header from: "t=(1;v=chrome.1:1:2, 2), p=P00000" to "(1 2);v=chrome.1:1:2, ();p=P00000" As discussed in patcg-individual-drafts/topics#183 Bug: 1443540 Change-Id: I3ea40df9f8732f5eff953110f22f4da2163480fb
Changed the format of the topics request header from: "t=(1;v=chrome.1:1:2, 2), p=P00000" to "(1 2);v=chrome.1:1:2, ();p=P00000" As discussed in patcg-individual-drafts/topics#183 Bug: 1443540 Change-Id: I3ea40df9f8732f5eff953110f22f4da2163480fb
Changed the format of the topics request header from: "t=(1;v=chrome.1:1:2, 2), p=P00000" to "(1 2);v=chrome.1:1:2, ();p=P00000" As discussed in patcg-individual-drafts/topics#183 Bug: 1443540 Change-Id: I3ea40df9f8732f5eff953110f22f4da2163480fb
Changed the format of the topics request header from: "t=(1;v=chrome.1:1:2, 2), p=P00000" to "(1 2);v=chrome.1:1:2, ();p=P00000" As discussed in patcg-individual-drafts/topics#183 Bug: 1443540 Change-Id: I3ea40df9f8732f5eff953110f22f4da2163480fb Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4576009 Reviewed-by: Yao Xiao <yaoxia@chromium.org> Commit-Queue: Josh Karlin <jkarlin@chromium.org> Reviewed-by: Andrew Paseltiner <apaseltiner@chromium.org> Cr-Commit-Position: refs/heads/main@{#1151938}
Changed the format of the topics request header from: "t=(1;v=chrome.1:1:2, 2), p=P00000" to "(1 2);v=chrome.1:1:2, ();p=P00000" As discussed in patcg-individual-drafts/topics#183 Bug: 1443540 Change-Id: I3ea40df9f8732f5eff953110f22f4da2163480fb Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4576009 Reviewed-by: Yao Xiao <yaoxia@chromium.org> Commit-Queue: Josh Karlin <jkarlin@chromium.org> Reviewed-by: Andrew Paseltiner <apaseltiner@chromium.org> Cr-Commit-Position: refs/heads/main@{#1151938}
Changed the format of the topics request header from: "t=(1;v=chrome.1:1:2, 2), p=P00000" to "(1 2);v=chrome.1:1:2, ();p=P00000" As discussed in patcg-individual-drafts/topics#183 Bug: 1443540 Change-Id: I3ea40df9f8732f5eff953110f22f4da2163480fb Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4576009 Reviewed-by: Yao Xiao <yaoxia@chromium.org> Commit-Queue: Josh Karlin <jkarlin@chromium.org> Reviewed-by: Andrew Paseltiner <apaseltiner@chromium.org> Cr-Commit-Position: refs/heads/main@{#1151938}
Addresses the following issues: - patcg-individual-drafts#176 - patcg-individual-drafts#177 - patcg-individual-drafts#178 - patcg-individual-drafts#179 - patcg-individual-drafts#180 - patcg-individual-drafts#181 - patcg-individual-drafts#182 - patcg-individual-drafts#183 (already addressed the main issue, but there was one unresolved PR comment in patcg-individual-drafts#186 (review))
Why does this header have the |
… read and parse, a=testonly Automatic update from web-platform-tests [Topics] Refactor header to be easier to read and parse Changed the format of the topics request header from: "t=(1;v=chrome.1:1:2, 2), p=P00000" to "(1 2);v=chrome.1:1:2, ();p=P00000" As discussed in patcg-individual-drafts/topics#183 Bug: 1443540 Change-Id: I3ea40df9f8732f5eff953110f22f4da2163480fb Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4576009 Reviewed-by: Yao Xiao <yaoxia@chromium.org> Commit-Queue: Josh Karlin <jkarlin@chromium.org> Reviewed-by: Andrew Paseltiner <apaseltiner@chromium.org> Cr-Commit-Position: refs/heads/main@{#1151938} -- wpt-commits: ae92b6b433768fd3b35de1dbff6cc8f03197fdc5 wpt-pr: 40332
… read and parse, a=testonly Automatic update from web-platform-tests [Topics] Refactor header to be easier to read and parse Changed the format of the topics request header from: "t=(1;v=chrome.1:1:2, 2), p=P00000" to "(1 2);v=chrome.1:1:2, ();p=P00000" As discussed in patcg-individual-drafts/topics#183 Bug: 1443540 Change-Id: I3ea40df9f8732f5eff953110f22f4da2163480fb Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4576009 Reviewed-by: Yao Xiao <yaoxia@chromium.org> Commit-Queue: Josh Karlin <jkarlin@chromium.org> Reviewed-by: Andrew Paseltiner <apaseltiner@chromium.org> Cr-Commit-Position: refs/heads/main@{#1151938} -- wpt-commits: ae92b6b433768fd3b35de1dbff6cc8f03197fdc5 wpt-pr: 40332
… read and parse, a=testonly Automatic update from web-platform-tests [Topics] Refactor header to be easier to read and parse Changed the format of the topics request header from: "t=(1;v=chrome.1:1:2, 2), p=P00000" to "(1 2);v=chrome.1:1:2, ();p=P00000" As discussed in patcg-individual-drafts/topics#183 Bug: 1443540 Change-Id: I3ea40df9f8732f5eff953110f22f4da2163480fb Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4576009 Reviewed-by: Yao Xiao <yaoxiachromium.org> Commit-Queue: Josh Karlin <jkarlinchromium.org> Reviewed-by: Andrew Paseltiner <apaseltinerchromium.org> Cr-Commit-Position: refs/heads/main{#1151938} -- wpt-commits: ae92b6b433768fd3b35de1dbff6cc8f03197fdc5 wpt-pr: 40332 UltraBlame original commit: 540daebf60a1596746b438bce17e3d57ace5979a
… read and parse, a=testonly Automatic update from web-platform-tests [Topics] Refactor header to be easier to read and parse Changed the format of the topics request header from: "t=(1;v=chrome.1:1:2, 2), p=P00000" to "(1 2);v=chrome.1:1:2, ();p=P00000" As discussed in patcg-individual-drafts/topics#183 Bug: 1443540 Change-Id: I3ea40df9f8732f5eff953110f22f4da2163480fb Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4576009 Reviewed-by: Yao Xiao <yaoxiachromium.org> Commit-Queue: Josh Karlin <jkarlinchromium.org> Reviewed-by: Andrew Paseltiner <apaseltinerchromium.org> Cr-Commit-Position: refs/heads/main{#1151938} -- wpt-commits: ae92b6b433768fd3b35de1dbff6cc8f03197fdc5 wpt-pr: 40332 UltraBlame original commit: 540daebf60a1596746b438bce17e3d57ace5979a
… read and parse, a=testonly Automatic update from web-platform-tests [Topics] Refactor header to be easier to read and parse Changed the format of the topics request header from: "t=(1;v=chrome.1:1:2, 2), p=P00000" to "(1 2);v=chrome.1:1:2, ();p=P00000" As discussed in patcg-individual-drafts/topics#183 Bug: 1443540 Change-Id: I3ea40df9f8732f5eff953110f22f4da2163480fb Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4576009 Reviewed-by: Yao Xiao <yaoxiachromium.org> Commit-Queue: Josh Karlin <jkarlinchromium.org> Reviewed-by: Andrew Paseltiner <apaseltinerchromium.org> Cr-Commit-Position: refs/heads/main{#1151938} -- wpt-commits: ae92b6b433768fd3b35de1dbff6cc8f03197fdc5 wpt-pr: 40332 UltraBlame original commit: 540daebf60a1596746b438bce17e3d57ace5979a
The sec- prefix protects the header from being modified by script. |
@jkarlin See #25 and discussion at https://github.com/patcg-individual-drafts/topics/blob/55153e6166c4aa574ba20226a83bf7e693c07ed6/meetings/2023-06-05-minutes.md -- trying to figure out how a user is going to be able to use a browser extension to filter Topics API data (as they can already do now with cookie management extensions) |
If a user doesn't want/like topics, then they should simply disable it, which extensions can do. I don't think it makes sense to provide extension support simply to modify topics. |
Yes, but that is a factual statement. You didn't address the underlying question, which was whether there was a need for the value to be protected from modification. Is there some case where a page gains access to a topic value and makes requests - in a context where adversarial script might also be present - and wants to ensure that the adversarial script can't change the topics? Or is this just the case that all new header fields gain a sec- prefix because we don't want to think about the consequences if we mistakenly omit sec- and someone discovers an attack? |
Just as Martin says, we've heard several times during the development of the various Privacy Sandbox APIs that potential ad tech users are concerned about the risk that a script on the page might try to maliciously modify headers for some fraud purpose. A |
Maybe, But I highly doubt that If it has to be a forbidden header, I propose a new, industry-specific prefix |
As the person who originally opened this issue, I consider it closed by 9d8fec1. It seems like a new discussion has started about the Sec- prefix, which does not seem relevant to the OP. Probably that discussion is best moved to another issue? |
In particular, emitting the version, but only sometimes, seems error-prone for consumers. It means consumers need to do extra work to reconstruct the correct version + topic pairs, on the server side, using a sort of stateful parser loop. They can't just use out-of-the-box HTTP structured field parsers.
Another example failure mode would be if something in their sever or proxy stack sorts HTTP header lists, this could be disastrous. E.g. it could turn
Sec-Browsing-Topics: 5;v="x", 3, 1;v="y"
(which is equivalent to{(1, "y"), (3, "x"), (5, "x")}
) intoSec-Browsing-Topics: 1;v="y", 3, 5;v="x"
(which is equivalent to{(1, "y"), (3, "y"), (5, "x")}
).The text was updated successfully, but these errors were encountered: