1111848 - remove nsISiteSecurityService.shouldIgnoreHeaders and implementation

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler]]

As far as I can tell, nsISiteSecurityService.shouldIgnoreHeaders isn't used anymore (somewhere along the line of refactoring to get HPKP out the door, we pulled that functionality directly into the processing of headers but never removed the unused/duplicated code).

This basically involves:

* removing the declaration in nsISiteSecurityService
* (including updating the uuid)
* removing the implementation in nsSiteSecurityService

Comment 1

[David Erceg]

Attachment: bug-1111848.diff

Comment 2

[kunal jain]

I am new to open source and I am willing to work on this bug if the bug is unsolved.
please give me some work to start from.

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8540020 [details] [diff] [review]
bug-1111848.diff

Review of attachment 8540020 [details] [diff] [review]:
-----------------------------------------------------------------

Great! r=me with removing the (now) unnecessary #include (see comment).

::: security/manager/boot/src/nsSiteSecurityService.cpp
@@ -972,5 @@
> -                                           bool* aResult)
> -{
> -  nsresult rv;
> -  bool tlsIsBroken = false;
> -  nsCOMPtr<nsISSLStatusProvider> sslprov = do_QueryInterface(aSecurityInfo);

I believe you can also remove the #include for nsISSLStatusProvider.h now (but not nsISSLStatus).

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler]]

(In reply to kunal jain from comment #2)
> I am new to open source and I am willing to work on this bug if the bug is
> unsolved.
> please give me some work to start from.

Thanks, Kunal, but it looks like David has already done the necessary work. If you're interested, bug 1077954 is another [good first bug] related to security.

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Sid - just a heads-up: this might affect ForceTLS (the implementation of nsISiteSecurityService now checks the trustworthiness of the nsISSLStatus of the connection that's attempting to set the header, so the separate ShouldIgnoreHeaders function is no longer necessary).

Comment 6

[David Erceg]

Attachment: Remove nsISSLStatusProvider.h include

Thanks for the quick turnaround David! I've updated the patch to remove the nsISSLStatusProvider.h include.

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8540549 [details] [diff] [review]
Remove nsISSLStatusProvider.h include

Review of attachment 8540549 [details] [diff] [review]:
-----------------------------------------------------------------

Great!

Comment 8

[David Erceg]

Thanks! How do I go about getting the patch committed? From the docs, it seems it needs to be submitted to the try server first? (I don't have access to the try server, so can't push the patch myself).

Comment 9

[Sid Stamm [:geekboy or :sstamm]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=69a0e041f373

I don't think it *really* needs a try run since it's code removal, but can't hurt.  If this looks good, then you can set the checkin-needed keyword on this bug and a kind sheriff will check it in (or I'll check back and maybe check it in for you).

Comment 10

[Sid Stamm [:geekboy or :sstamm]]

Yep, try looked good.  Great job!

Landed on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/178d18db618d

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/178d18db618d

Comment 12

[Jorge Villalobos [:jorgev] (he/him)]

Adding addon-compat just to document it, but it looks like only Sid's add-on is affected :)