nsStrictTransportSecurityService should use Principal to access the PermissionManager

RESOLVED FIXED in mozilla17

Status

()

Core
Security
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

(Blocks: 1 bug)

Trunk
mozilla17
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(blocking-basecamp:+)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 643986 [details] [diff] [review]
Patch
Attachment #643986 - Flags: review?(bsmith)
(Assignee)

Updated

5 years ago
Depends on: 769583
Comment on attachment 643986 [details] [diff] [review]
Patch

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

::: security/manager/boot/src/nsStrictTransportSecurityService.cpp
@@ +602,5 @@
>  
>      // Use whatever we ended up with, which defaults to UNKNOWN_ACTION.
>      return NS_OK;
>  }
> +

namespace {

@@ +603,5 @@
>      // Use whatever we ended up with, which defaults to UNKNOWN_ACTION.
>      return NS_OK;
>  }
> +
> +/* static */ nsresult

remove /* static */

@@ +615,5 @@
> +      do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv);
> +   NS_ENSURE_SUCCESS(rv, rv);
> +
> +   return securityManager->GetNoAppCodebasePrincipal(aURI, aPprincipal);
> +}

} // unnamed namespace.

::: security/manager/boot/src/nsStrictTransportSecurityService.h
@@ +124,5 @@
> +  /**
> +   * Returns a principal (aPrincipal) corresponding to aURI.
> +   * This is used to interact with the permission manager.
> +   */
> +  static nsresult GetPrincipalForURI(nsIURI* aURI, nsIPrincipal** aPrincipal);

Remove this declaration. This does not need to be a member function of nsSTSS.
Attachment #643986 - Flags: review?(bsmith) → review+
Attachment #643986 - Flags: superreview?(honzab.moz)
Comment on attachment 643986 [details] [diff] [review]
Patch

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

::: security/manager/boot/src/nsStrictTransportSecurityService.cpp
@@ +608,5 @@
> +GetPrincipalForURI(nsIURI* aURI, nsIPrincipal** aPrincipal)
> +{
> +   // The permission manager wants a principal but don't actually check a
> +   // permission but a data we saved in the permission manager so we are good by
> +   // creating a no-app codebase principal and send it to the permission manager.

Replace this comment with this one:

// We want all apps to share HSTS state, so this is one
// of the few places where we do not silo persistent
// state by extended origin.
Comment on attachment 643986 [details] [diff] [review]
Patch

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

This should, from fairness, land (and also merge) after the patch for bug 760307 since this one is much more simpler.  But depends on priorities.

::: security/manager/boot/src/nsStrictTransportSecurityService.cpp
@@ +614,5 @@
> +   nsCOMPtr<nsIScriptSecurityManager> securityManager =
> +      do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv);
> +   NS_ENSURE_SUCCESS(rv, rv);
> +
> +   return securityManager->GetNoAppCodebasePrincipal(aURI, aPprincipal);

typo: aPprincipal

What exactly GetNoAppCodebasePrincipal does?  It probably still just sits in some patch yet landed that is not listed in deps, so I cannot do the proper review here.
Attachment #643986 - Flags: superreview?(honzab.moz)
(Assignee)

Comment 4

5 years ago
(In reply to Honza Bambas (:mayhemer) from comment #3)
> Comment on attachment 643986 [details] [diff] [review]
> Patch
> 
> Review of attachment 643986 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This should, from fairness, land (and also merge) after the patch for bug
> 760307 since this one is much more simpler.  But depends on priorities.
> 
> ::: security/manager/boot/src/nsStrictTransportSecurityService.cpp
> @@ +614,5 @@
> > +   nsCOMPtr<nsIScriptSecurityManager> securityManager =
> > +      do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv);
> > +   NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +   return securityManager->GetNoAppCodebasePrincipal(aURI, aPprincipal);
> 
> typo: aPprincipal
> 
> What exactly GetNoAppCodebasePrincipal does?  It probably still just sits in
> some patch yet landed that is not listed in deps, so I cannot do the proper
> review here.

It's like getCodebasePrincipal(), see bug 758258. Basically, we have getAppCodebasePrincipal() and getNoAppCodebasePrincipal() to get a principal with app info or not. GetNoAppCodebasePrincipal() is exactly siimilar to getCodebasePrincipal() because it has no app info. However, we are going to remove getCodebasePrincipal() to make sure people know what they are doing.
(Assignee)

Comment 5

5 years ago
Comment on attachment 643986 [details] [diff] [review]
Patch

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

I fixed the typo and apply bsmith review comments locally.
Attachment #643986 - Flags: superreview?(honzab.moz)
Mounir, can you please add dep bug where GetNoAppCodebasePrincipal() has been introduced?
(Assignee)

Updated

5 years ago
Depends on: 758258
blocking-basecamp: --- → +
(In reply to Mounir Lamouri (:mounir) from comment #5)
> I fixed the typo and apply bsmith review comments locally.

Can you please submit the newer patch?
(Assignee)

Comment 8

5 years ago
Created attachment 644701 [details] [diff] [review]
Patch
Attachment #644701 - Flags: superreview?(honzab.moz)
(Assignee)

Updated

5 years ago
Attachment #643986 - Attachment is obsolete: true
Attachment #643986 - Flags: superreview?(honzab.moz)
Attachment #644701 - Flags: superreview?(honzab.moz) → superreview+
(Assignee)

Updated

5 years ago
Attachment #644701 - Flags: checkin+
(Assignee)

Updated

5 years ago
Flags: in-testsuite-
Target Milestone: --- → mozilla17

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/09d63c143799
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.