Set enable_reporting to false #995

nikolowry · 2021-03-09T21:06:10Z

enable_reporting has been enabled since v83.0.4103.46, 65bd5c1.

This PR sets it back to false and adds ungoogled-chromium-Fix-building-without-enabling-reporting.patch
to fix build issues. See ungoogled-software/ungoogled-chromium#1416 for more details.

Added `ungoogled-chromium-Fix-building-without-enabling-reporting.patch` to fix build issues. See the following PR for more details: ungoogled-software/ungoogled-chromium#1416

csagan5 · 2021-03-09T21:19:50Z

Nice finding, but what is the impact of enable_reporting being set to true? Is any undesired connection happening?

csagan5 · 2021-03-09T21:20:51Z

It was enabled in v83 because of COEP: https://bugs.chromium.org/p/chromium/issues/detail?id=887967

nikolowry · 2021-03-09T21:36:45Z

@csagan5 I thought it was just an oversight setting it to true. I'll dig into the linked Chromium issue and report back in the next few hours.

Setting enable_reporting=true shouldn't negatively effect privacy-related functionality due to the domain substitution patches, correct? The domain substitution is a catch-all to prevent unnoticed tracking features from creeping into builds, right?

Regardless, I'll report back after I review.

wchen342 · 2021-03-09T21:39:20Z

From what I read COEP reporting and the policy itself are two separate things? I had a look at the navigation and render host part and there is nothing hidden behind the reporting flag.

nikolowry · 2021-03-09T22:17:36Z

@csagan5 isn't the point of these lines

bromite/build/patches/ungoogled-chromium-Fix-building-without-enabling-reporting.patch

Lines 1 to 24 in 9824847

    
           --- a/content/browser/BUILD.gn 
        
           +++ b/content/browser/BUILD.gn 
        
           @@ -1150,6 +1150,10 @@ source_set("browser") { 
        
                "net/browser_online_state_observer.cc", 
        
                "net/browser_online_state_observer.h", 
        
                "net/cookie_store_factory.cc", 
        
           +    "net/cross_origin_embedder_policy_reporter.cc", 
        
           +    "net/cross_origin_embedder_policy_reporter.h", 
        
           +    "net/cross_origin_opener_policy_reporter.cc", 
        
           +    "net/cross_origin_opener_policy_reporter.h", 
        
                "net/network_errors_listing_ui.cc", 
        
                "net/network_errors_listing_ui.h", 
        
                "net/network_quality_observer_impl.cc", 
        
           @@ -2796,10 +2800,6 @@ source_set("browser") { 
        
              if (enable_reporting) { 
        
                sources += [ 
        
           -      "net/cross_origin_embedder_policy_reporter.cc", 
        
           -      "net/cross_origin_embedder_policy_reporter.h", 
        
           -      "net/cross_origin_opener_policy_reporter.cc", 
        
           -      "net/cross_origin_opener_policy_reporter.h", 
        
                  "net/reporting_service_proxy.cc", 
        
                  "net/reporting_service_proxy.h", 
        
                ]

to not make COEP/COOP-support dependent on enable_reporting being true?

@Ahrotahn explains why the other changes are needed in this comment ungoogled-software/ungoogled-chromium#1416 (comment).

I haven't had any issues today after building v89 with enable_reporting=false and the ungoogled-chromium-Fix-building-without-enabling-reporting.patch.

csagan5 · 2021-03-09T22:27:58Z

isn't the point of these lines to not make COEP/COOP-support dependent on enable_reporting being true?

Yes; but as far as I know no reporting of any other type is happening right now; it would be nice to confirm/deny that.

I haven't had any issues today after building v89 with enable_reporting=false and the ungoogled-chromium-Fix-building-without-enabling-reporting.patch.

Yes, but for all changes we need to have a reason to make them and then document it.

nikolowry · 2021-03-09T22:43:53Z

Yes; but as far as I know no reporting of any other type is happening right now; it would be nice to confirm/deny that.

Any suggestions on a reliable way to profile and test requests?

Yes, but for all changes we need to have a reason to make them and then document it.

I'll be the first to admit I have very limited knowledge of Chromium's net architecture -- hopefully someone more familiar can chime in on whether enable_reporting=true causes negative effects.

Feel free to close this or keep it open until we have more context to make an informed decision.

csagan5 · 2021-03-09T22:48:35Z

Any suggestions on a reliable way to profile and test requests?

None other than Wireshark; see also #471

By following the code it should be possible to understand what reporting does (or not).

(bromite/bromite#995)

uazo · 2021-03-17T07:42:24Z

ah, I didn't notice, thanks @csagan5
so it is not only active, but also with cookies!

uazo · 2021-03-17T08:18:17Z

By following the code it should be possible to understand what reporting does (or not).

I got there by checking something else, but here it is explained:
https://source.chromium.org/chromium/chromium/src/+/master:services/network/expect_ct_reporter.cc;l=116;drc=70365c5dbd0628df048dcc4e81740b3932411574
however, it is not the only point that uses that call:
https://source.chromium.org/chromium/chromium/src/+/master:services/network/network_context.cc;l=2241;drc=70365c5dbd0628df048dcc4e81740b3932411574;bpv=1;bpt=1

Any suggestions on a reliable way to profile and test requests?

can this be enough?
https://source.chromium.org/chromium/chromium/src/+/master:services/network/expect_ct_reporter_unittest.cc

csagan5 · 2021-03-18T22:18:55Z

so it is not only active, but also with cookies!

are cookies there by design of the certificate reporting (necessary) or they are unnecessary and a privacy violation?
certificate reporting is currently happening in Bromite for websites that have an invalid certificate and specify the headers for certificate reporting?
is invalid certificate reporting to be considered a privacy-invasive feature for the user? Can it be used/misused in such sense?
the patch in this PR would allow to still have COEP/COOP but no reporting, correct?

(3) is the most important aspect here

uazo · 2021-03-19T07:09:26Z

are cookies there by design of the certificate reporting (necessary) or they are unnecessary and a privacy violation?

forget it, a more careful check allowed me to understand that cookies are NOT active.

regarding the other points, personally I think that any request not authorized by the user should never be made.

csagan5 · 2021-03-19T07:39:57Z

I still do not have a resolution for the other questions (2,4)

regarding the other points, personally I think that any request not authorized by the user should never be made.

The requests are sent to the same site the user is visiting, not a third-party. What are the conditions under which these requests are sent?

uazo · 2021-03-19T07:49:49Z

not a third-party

should it be verified or are you sure?

What are the conditions under which these requests are sent?

the documentation and code must be studied and the test cases verified, it can be a long thing.

csagan5 · 2021-03-19T07:53:43Z

should it be verified or are you sure?

I am sure; it must be the same domain; feel free to double-check!

the documentation and code must be studied and the test cases verified, it can be a long thing.

Sure; but it is supposedly a feature for the protection of users (in case they are under MiTM or similar security problems), at the moment I am not sure how it can be abused against users.

uazo · 2021-03-19T08:07:11Z

ok, i'll check.
in any case the correct build flag to disable this, appears to be IS_CT_SUPPORTED

csagan5 · 2021-03-20T09:00:15Z

in any case the correct build flag to disable this, appears to be IS_CT_SUPPORTED

Related #985, where we enabled PartitionExpectCTStateByNetworkIsolationKey

uazo · 2021-06-23T16:00:59Z

is invalid certificate reporting to be considered a privacy-invasive feature for the user?

...
it can’t be the case that this general benefit be allowed to take priority over the ability 
of a user to individually opt-out of such a system
...
User agents MUST allow users to disable reporting with some reasonable amount 
of granularity in order to maintain the priority of constituencies espoused in [HTML-DESIGN-PRINCIPLES].

https://w3c.github.io/reporting/#disable

so, there should be a flag that deactivates it, but there isn't

csagan5 · 2021-06-24T20:19:05Z

https://w3c.github.io/reporting/#disable

so, there should be a flag that deactivates it, but there isn't

Since adding a chrome:// flag does not seem possible I will merge this PR and we go without the feature. Thoughts?

nikolowry · 2021-06-24T23:55:50Z

@csagan5 should I update this PR with the latest patch that's located at https://github.com/Eloston/ungoogled-chromium/blob/a9eb6fd87445465a2e1881f7fc6e718fd17db606/patches/core/ungoogled-chromium/fix-building-without-enabling-reporting.patch?

uazo · 2021-06-25T07:41:01Z

Since adding a chrome:// flag does not seem possible

why you say that?

I will merge this PR and we go without the feature.

I don't know, the alternative would be put a flag and act only on https://source.chromium.org/chromium/chromium/src/+/master:services/network/network_context.cc;l=2241;drc=70365c5dbd0628df048dcc4e81740b3932411574 however the code must be checked.

uazo · 2021-06-26T06:19:07Z

I came up with another idea.

I don't know if you are aware of traffic annotator script, (i think the result).

the idea could be to create an ui that allows the user to select which ones he wants to activate.
for implementation I guess the point to act is simple_url_loader: when blocked, we return a network error, a bit like the replacement of the url done by Automated-domain-substitution.patch, theoretically at that point we could even do without it, simplifying the build process.

also theoretically it would also be possible to act in DefineNetworkTrafficAnnotation(), by modifying the call in such a way that the compiler communicates new annotators to us going into error.

about the implementation, I don't think it's impossible, a bit 'of doubts about what to return in case it is blocked, because I would not want there were infinite retries, but just check.

csagan5 · 2021-06-26T17:03:11Z

Since adding a chrome:// flag does not seem possible

why you say that?

I say that because it is currently a build flag, just assuming that it would be complicated at runtime. But if we have already covered similar complexity/functionality and you think we can have it as a flag it would be better.

I don't know if you are aware of traffic annotator script, (i think the result).

I know about traffic annotations but I have never checked the scripts used during tests for them.

the idea could be to create an ui that allows the user to select which ones he wants to activate.
for implementation I guess the point to act is simple_url_loader: when blocked, we return a network error, a bit like the replacement of the url done by Automated-domain-substitution.patch

Before doing this we should survey what are the traffic annotations and if there is an use-case for enabling/disabling some. If not, this would not be necessary.

As for the error: the one returned when ad-blocking is also fine.

theoretically at that point we could even do without it, simplifying the build process.

The domain substitution patch changes strings, which end up in multiple places. Even when they end up in URLs being loaded, they are not always via simple_url_loader and they do not have a shared annotation.

about the implementation, I don't think it's impossible, a bit 'of doubts about what to return in case it is blocked, because I would not want there were infinite retries, but just check.

For this specific PR #995 I suggest a flag, for the traffic annotations we can study it separately in a new issue.

@csagan5 should I update this PR with the latest patch that's located at https://github.com/Eloston/ungoogled-chromium/blob/a9eb6fd87445465a2e1881f7fc6e718fd17db606/patches/core/ungoogled-chromium/fix-building-without-enabling-reporting.patch?

@nikolowry if we go with the flag then the reporting code would be always compiled but disabled at runtime, so I suggest to hold on that for now.

csagan5 · 2021-09-26T09:50:53Z

I am going to try building with enable_reporting=false and use the latest ungoogled-chromium patch as needed; having a flag here would be nice, since user might want to be sending this information to the domain they are willingly visiting, but I also would like to speed up the resolution of issues/PRs.

nikolowry · 2021-09-26T21:27:14Z

I'll do the same tonight and update the patch in the PR. Feel free to cool if you beat me to it @csagan5

csagan5 · 2021-09-26T21:50:03Z

@nikolowry not necessary, I was already going to merge the PR after verifying that it does not introduce bugs.

csagan5 · 2021-10-13T22:12:46Z

In v94 there is now a feature flag that basically achieves this: chromium/chromium@329e4bb

Perhaps we could stick to that feature flag alone instead of the patch; @nikolowry @uazo feel free to investigate.

nikolowry · 2021-10-13T22:37:33Z

@csagan5 there's been two commits targeted for v94 in Ungoogled's fix-building-without-enabling-reporting.patch that appear to be more than just the result of updating via quilt:

Maybe the commit author, @Ahrotahn, could weigh in with their thoughts?

uazo · 2021-10-14T12:11:13Z

that basically achieves this

no, it does not seem
from what I read, the kDocumentReporting flag only activates the modification of the url by reading a header in the response, it would have been more correct to call it kParsedReportingEndpointsHeader.

I also checked the current master and the flag still does just that.

Ahrotahn · 2021-10-14T13:17:50Z

That was my understanding as well, but I haven't looked into it since that update. It seemed like they were still adding more changes at the time.

Those changes to the reporting patch were made because kDocumentReporting is gated behind the ENABLE_REPORTING buildflag elsewhere, but they added that section later and forgot to include the buildflag there as well.

csagan5 · 2021-10-14T18:31:36Z

Yes, they forgot to gate the kDocumentReporting behind the build flag, I have already fixed this in the upcoming v94 patches.

from what I read, the kDocumentReporting flag only activates the modification of the url by reading a header in the response, it would have been more correct to call it kParsedReportingEndpointsHeader.

I also checked the current master and the flag still does just that.

Ok, then we still need this patch.

Set enable_reporting to false

9824847

Added `ungoogled-chromium-Fix-building-without-enabling-reporting.patch` to fix build issues. See the following PR for more details: ungoogled-software/ungoogled-chromium#1416

deemru added a commit to deemru/Chromium-Gost that referenced this pull request Mar 10, 2021

fix enable_reporting=false

7a8bdce

(bromite/bromite#995)

csagan5 mentioned this pull request Mar 17, 2021

Flag to allow user-installed certificates #921

Closed

csagan5 merged commit 07d13a3 into bromite:master Sep 26, 2021

uazo mentioned this pull request Nov 6, 2021

Disable "Conversion Measurement API" #1531

Closed

csagan5 mentioned this pull request Dec 2, 2021

Check Certificate Transparency and reporting #1554

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set enable_reporting to false #995

Set enable_reporting to false #995

nikolowry commented Mar 9, 2021

csagan5 commented Mar 9, 2021

csagan5 commented Mar 9, 2021

nikolowry commented Mar 9, 2021

wchen342 commented Mar 9, 2021 •

edited

nikolowry commented Mar 9, 2021

csagan5 commented Mar 9, 2021

nikolowry commented Mar 9, 2021 •

edited

csagan5 commented Mar 9, 2021

uazo commented Mar 17, 2021

uazo commented Mar 17, 2021

csagan5 commented Mar 18, 2021

uazo commented Mar 19, 2021

csagan5 commented Mar 19, 2021

uazo commented Mar 19, 2021

csagan5 commented Mar 19, 2021

uazo commented Mar 19, 2021

csagan5 commented Mar 20, 2021

uazo commented Jun 23, 2021

csagan5 commented Jun 24, 2021

nikolowry commented Jun 24, 2021

uazo commented Jun 25, 2021

uazo commented Jun 26, 2021

csagan5 commented Jun 26, 2021

csagan5 commented Sep 26, 2021

nikolowry commented Sep 26, 2021

csagan5 commented Sep 26, 2021

csagan5 commented Oct 13, 2021

nikolowry commented Oct 13, 2021

uazo commented Oct 14, 2021

Ahrotahn commented Oct 14, 2021

csagan5 commented Oct 14, 2021

Set enable_reporting to false #995

Set enable_reporting to false #995

Conversation

nikolowry commented Mar 9, 2021

csagan5 commented Mar 9, 2021

csagan5 commented Mar 9, 2021

nikolowry commented Mar 9, 2021

wchen342 commented Mar 9, 2021 • edited

nikolowry commented Mar 9, 2021

csagan5 commented Mar 9, 2021

nikolowry commented Mar 9, 2021 • edited

csagan5 commented Mar 9, 2021

uazo commented Mar 17, 2021

uazo commented Mar 17, 2021

csagan5 commented Mar 18, 2021

uazo commented Mar 19, 2021

csagan5 commented Mar 19, 2021

uazo commented Mar 19, 2021

csagan5 commented Mar 19, 2021

uazo commented Mar 19, 2021

csagan5 commented Mar 20, 2021

uazo commented Jun 23, 2021

csagan5 commented Jun 24, 2021

nikolowry commented Jun 24, 2021

uazo commented Jun 25, 2021

uazo commented Jun 26, 2021

csagan5 commented Jun 26, 2021

csagan5 commented Sep 26, 2021

nikolowry commented Sep 26, 2021

csagan5 commented Sep 26, 2021

csagan5 commented Oct 13, 2021

nikolowry commented Oct 13, 2021

uazo commented Oct 14, 2021

Ahrotahn commented Oct 14, 2021

csagan5 commented Oct 14, 2021

wchen342 commented Mar 9, 2021 •

edited

nikolowry commented Mar 9, 2021 •

edited