1032393 - Make sure we honor the 45 minute Safe Browsing expiry

Status: REOPENED

(Toolkit :: Safe Browsing, defect, P5)

Description

[[:mmc] Monica Chew (no longer reading bugmail)]

Bug 1024610 turns it off because we never wrote a gethash responder. There will be one in production in time for 33, though.

Comment 1

[[:mmc] Monica Chew (no longer reading bugmail)]

The purpose of this bug is to ensure that we don't respect stale table entries. We should audit this code regardless of gethash status.

Comment 2

[[:mmc] Monica Chew (no longer reading bugmail)]

The reason we might be respecting stale entries is that gethash requests are currently disabled for mozpub-track-digest256: https://mxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.js#4427

So we could either remove it from urlclassifier.disallow_completions or double-check the logic in the skip completions code in nsUrlClassifierDBService.cpp.

I think we probably *are* respecting stale entries.

Comment 3

[François Marier [:francois]]

Testing steps:

1. Turn TP on
2. Wait for the list to get downloaded
3. Set the update URL to an unreachable host
4. Wait for more than 45 min
5. Is TP still blocking things (now that the list is stale)?

Expected:

TP should not be blocking anything while the list entries are stale.

Comment 4

[Matt Wobensmith [:mwobensmith][:matt:]]

(In reply to [:mmc] Monica Chew (no longer reading bugmail) from comment #2)

> I think we probably *are* respecting stale entries.

My testing says yes, that is correct. We are still blocking things long after list is stale. I tried this twice - once waiting two hours, once overnight - both times, we still block when host is unreachable.

Comment 5

[Steve Workman [:sworkman] (INACTIVE)]

Assigning to François to be tracked by someone.

Comment 6

[François Marier [:francois]]

Note from Matt: it also affects Safe Browsing.

Comment 7

[François Marier [:francois]]

From what I can see, the result that Matt saw in comment 4 only show up when the table is in urlclassifier.disallow_completions.


If we take mozpub-track-digest256 out of that (therefore enabling gethash calls) then it behaves like it should: after 45 minutes, stale entries are not used anymore.

Comment 8

[François Marier [:francois]]

Before we take mozpub-track-digest256 out of disallow_completions, we need to fix https://github.com/mozilla-services/shavar/issues/46.

Comment 9

[François Marier [:francois]]

Another warning: before we start honouring the 45 expiry for lists in urlclassifier.disallow_completions, we need to take the tracking protection list out of there, otherwise a stale list will completely disable TP (which is not currently the behaviour we want to see).

Comment 10

[Gian-Carlo Pascutto [:gcp]]

(In reply to François Marier [:francois] from comment #9)
> Another warning: before we start honouring the 45 expiry for lists in
> urlclassifier.disallow_completions, we need to take the tracking protection
> list out of there, otherwise a stale list will completely disable TP (which
> is not currently the behaviour we want to see).

Make the expiry a per-provider variable?

Comment 11

[François Marier [:francois]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #10)
> Make the expiry a per-provider variable?

Good point. If we set Mozilla lists to expire after 3 hours (for example), it should give them enough time to get updated.

Comment 12

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.