Closed Bug 1283368 Opened 8 years ago Closed 8 years ago

Implement cookie prefixes spec


(Core :: Networking: Cookies, defect)

Not set



Tracking Status
firefox50 --- fixed


(Reporter: dveditz, Assigned: dveditz)


(Keywords: dev-doc-complete, Whiteboard: [necko-active])


(1 file, 1 obsolete file)

Implement restrictive behavior/requirements for cookies that start with the prefixes __Secure- and __Host- as specified in

Cookies with a name starting with __Secure- must be set with the secure flag and from a secure page. Cookies with a name starting with __Host- must meet the above requirements and in addition must NOT have a domain specified and the path must be "/".

This is already implemented in Chrome 49 and Opera 36
Attached patch Implement cookie prefix spec (obsolete) — Splinter Review
Attachment #8766669 - Flags: review?(mcmanus)
The __Secure- version is less interesting than the more general proposal to extend those protections to all secure cookies in A site that adopts HSTS with includesubdomains from its base domain also effectively gets this protection. But that spec has a long way to go before adoption (web compat worries) and switching an entire domain to HSTS can be non-trivial so this does have a use.

The more interesting __Host- version protects against cookie fixation attacks (should a sibling domain be compromised) that sites don't really have an alternative for at the moment.
Comment on attachment 8766669 [details] [diff] [review]
Implement cookie prefix spec

Review of attachment 8766669 [details] [diff] [review]:

The patch looks good, and the tests seem to cover all the cases in the draft. Thanks!

::: netwerk/test/TestCookie.cpp
@@ +605,5 @@
> +      SetACookie(cookieService, "https://prefixed.test/", nullptr, "__Secure-test=test", nullptr);
> +      SetACookie(cookieService, "https://prefixed.test/", nullptr, "__Host-test=test", nullptr);
> +      GetACookie(cookieService, "https://prefixed.test/", nullptr, getter_Copies(cookie));
> +      rv[1] = CheckResult(cookie.get(), MUST_BE_NULL);
> +      

Trailing whitespace.
Attachment #8766669 - Flags: review+
Comment on attachment 8766669 [details] [diff] [review]
Implement cookie prefix spec

thanks valentin - I'd also like amy to review as she's been looking at cookie implementation issues.

and thanks dan!
Attachment #8766669 - Flags: review?(mcmanus) → review?(amchung)
Whiteboard: [necko-active]
Hi Patrick,
I have traced code from Daniel, and my comment is same as Valentin.
But I need more time to study spec and test, would I review the path first?
Flags: needinfo?(mcmanus)
Comment on attachment 8766669 [details] [diff] [review]
Implement cookie prefix spec

Review of attachment 8766669 [details] [diff] [review]:

I tried some cookies as below:
1. document.cookie = '__Secure-SID=12345; Secure;'
2. document.cookie = '__Secure-SID2=12345;'
3. document.cookie = '__Host-SID3=12345; Secure; Path=/'
4. document.cookie = '__Host-SID4=12345; Secure;; Path=/' 
And I got the right results that following spec on Nightly after applied path.

Attachment #8766669 - Flags: review?(amchung) → review+
Hi Patrick,
I have finished to review the patch after studied spec and tested code.
Flags: needinfo?(mcmanus)
Fixed whitespace issue, carrying over r+
Attachment #8766669 - Attachment is obsolete: true
Attachment #8768184 - Flags: review+
Keywords: checkin-needed
Attachment #8768184 - Flags: checkin?
Pushed by
Implement cookie prefixes spec, r=valentin r=amchung
Keywords: checkin-needed
Attachment #8768184 - Flags: checkin? → checkin+
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.