Closed Bug 1441989 Opened 2 years ago Closed 9 months ago

Remove dead code from ContentVerifier.cpp

Categories

(Core :: DOM: Security, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: ursula, Assigned: keeler)

References

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file)

Remote newtab was using ContentVerifier.cpp since it was loading a remote resource, and that project has long been dead. The removal of the code has started in bug 1355166 for the files in browser/components/newtab, but there is leftover content verifying stuff that we no longer need. To quote Franziskus on bug 1355166:

The only two services (I know of) using content signatures right now are blocklist updates and normandy. They both use the CS service directly. So I think ContentVerifier.cpp can go completely. To make that work there are changes to nsHTTPChannel necessary.
However, before doing that it might be advisable to check whether this is used anywhere else. I don't think it is and a quick check doesn't reveal any callers of LoadInfo::SetVerifySignedContent (and without it content signature verification is not invoked from nsHTTPChannel). The LoadInfo bits can be removed as well then as it can't really be used anymore.
This is touching a couple different parts but I think it's all dead code right now and should probably be removed.
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]

Adam, are you still working on this? (fwiw, from what I can tell nothing is using ContentVerifier).

Blocks: 1541942
Flags: needinfo?(adam.kasztenny)

Oops, no I'm not. I haven't had time recently... or for the past year. Can we just delete ContentVerifier altogether?

Flags: needinfo?(adam.kasztenny)

My view is that a) ContentVerifier doesn't have tests b) it's dead code anyway and c) its presence is complicating efforts to improve ContentSignatureVerifier (see e.g. bug 1534600). Removing it isn't quite as simple as hg rm, but it also wouldn't be that difficult. If you're not available to do this, I'll probably pick it up in the next few weeks or so.

Makes sense. I probably won't have spare time for a few weeks.

ContentVerifier has been dead code since bug 1355166 (which, incidentally, means
it has no tests). Its presence is preventing improvements to
ContentSignatureVerifier (see e.g. bug 1534600), so this patch removes it.
As a result, the nsILoadInfo attributes verifySignedContent and enforceSRI are
also unused, so this patch removes those as well.

Assignee: adam.kasztenny → dkeeler
Priority: P2 → P1
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8292a59d1957
remove ContentVerifier r=baku,mayhemer
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.