Reporting: Assumption that origins only include each endpoint in one group |
|||
Issue description
Currently we assume that each origin only registers a given endpoint in one group, as the endpoint configs (ReportingClients) are stored as a std::map<url::Origin, std::map<GURL, std::unique_ptr<ReportingClient>>> keyed on the origin, then the endpoint URL. This could be inaccurate if the same endpoint is in multiple groups for an origin (perhaps with different priorities or weights), as the spec does not preclude this.
This could result in missing (overwritten) clients, and thus reports that are erroneously undeliverable. For example, suppose the server sends a Report-To header like this:
Report-To: { "group": "group_a",
"max_age": 10886400,
"endpoints": [
{ "url": "https://endpoints.com/default" }
] },
{ "group": "group_b",
"max_age": 10886400,
"endpoints": [
{ "url": "https://endpoints.com/preferred",
"priority": 0 },
{ "url": "https://endpoints.com/default",
"priority": 1 }
] }
Then ReportingCache::SetClient() will overwrite the group_a ReportingClient with a new ReportingClient of the same endpoint URL but in group_b, thus leaving group_a with no endpoints. Then let's say a report gets queued up to be sent to group_a. ReportingEndpointManager::FindClientForOriginAndGroup() will not be able to find a client for group_a, thus resulting in ReportingDeliveryAgent skipping the report (and eventually it gets evicted, expired, or garbage collected).
,
Dec 11
Great catch! This is a holdover from an earlier draft of the Reporting spec. Before https://github.com/w3c/reporting/pull/67, each endpoint URL was a top-level element of the Report-To header, and so it was structurally impossible for there to be more than one ReportingClient with the same URL. But now, you're right, this data structure doesn't line up with what you can express in a Report-To header. I think your suggested refactoring is a good possibility. Another would be to make proper classes for each of the named concepts in the spec: clients, endpoint groups, and endpoints. What is currently named ReportingClient is really closer to what's now called "endpoint" in the spec — a single URL, within a single group, for a single origin. If you go down this route, you'd end up with: - rename ReportingClient to ReportingEndpoint - clients is a std::map<url::Origin, ReportingClient> - ReportingClient would contain a std::map<std::string, ReportingEndpointGroup> (i.e., all of the endpoint groups for the origin, keyed by their name) - ReportingEndpointGroup would contain a std::vector<ReportingEndpoint> That might be more invasive of a change than you're looking for, but it would make all of the named concepts in the code line up with the spec!
,
Dec 11
Ah I see, thanks a lot for the context!
,
Dec 12
|
|||
►
Sign in to add a comment |
|||
Comment 1 by chlily@chromium.org
, Dec 7