feat(ngCookies): support sameSite option #16544

m-amr · 2018-05-01T15:00:24Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

feature for issue #16543

What is the current behavior? (You can also link to an open issue here)

What is the new behavior (if this is a feature change)?

support sameSite option for ngCookie

Does this PR introduce a breaking change?
no

Please check if the PR fulfills these requirements

The commit message follows our guidelines
Fix/Feature: Docs have been added/updated
Fix/Feature: Tests have been added; existing tests pass

Other information:

Closes angular#16543 Closes angular#16544

Narretz · 2018-05-04T18:44:13Z

src/ngCookies/cookieWriter.js

@@ -14,6 +14,11 @@
 function $$CookieWriter($document, $log, $browser) {
  var cookiePath = $browser.baseHref();
  var rawDocument = $document[0];
+  var sameSiteValidValues = ['lax', 'strict'];


How big is the chance that another option is added to this? This validation would prevent us from supporting this seemlessly.

Narretz · 2018-05-04T18:46:40Z

src/ngCookies/cookieWriter.js

@@ -33,6 +38,7 @@ function $$CookieWriter($document, $log, $browser) {
    str += options.domain ? ';domain=' + options.domain : '';
    str += expires ? ';expires=' + expires.toUTCString() : '';
    str += options.secure ? ';secure' : '';
+    str += (options.SameSite && isSameSiteValueValid(options.SameSite)) ? ';SameSite=' + options.SameSite : '';


If the option is invalid we should probably throw - otherwise you get a false sense of security. Based on my comment above, I would simply skip the validation, but add a note to the docs that as time of writing, "lax" and "strict" are allowed values.

Closes angular#16543 Closes angular#16544

m-amr · 2018-05-04T19:12:32Z

@Narretz
Thanks for your feedback
I have removed the validation maybe we can add the validation later in angular-hint
by decorating the $$CookieWriter to validate the option value.

also I have add more information to the docs.
please review it again.
Thanks

Closes angular#16543 Closes angular#16544

Narretz · 2018-05-14T16:38:48Z

Hi @m-amr can you please make the following two final changes:

change the option to camelCase sameSite to match all other options
change the text to (with line breaks at 100 chars):

prevents the browser from sending the cookie along with cross-site requests. Accepts the values `lax` and `strict`. See the [OWASP Wiki](https://www.owasp.org/index.php/SameSite) for more info. Note that as of May 2018, not all browsers support `SameSite`, so it cannot be used as a single measure against Cross-Site-Request-Forgery (CSRF) attacks.

m-amr · 2018-05-14T19:47:55Z

hi @Narretz

Thanks for your feedback.

regards to this point
change the option to camelCase sameSite to match all other options

if we change SameSite to camelCase. the cookieReader will fail to get the value because
it will not find sameSite in camelCase.
https://github.com/angular/angular.js/blob/master/src/ng/cookieReader.js#L51

so to use camelCase we need to edit the cookieReader to map SameSite to sameSite

            name = (name === "SameCase")?'sameCase': name;
            lastCookies[name] = safeDecodeURIComponent(cookie.substring(index + 1));

what do you think ?

Closes angular#16543 Closes angular#16544

Narretz · 2018-05-15T19:50:53Z

Hi @m-amr you are right, that is a pickle. However I realized just now that the domain names are case-insensitive (that's why the other keys are lowercase). So we should simply lowercase everything - the property name in options object, and the string that is actually written to the cookie

Closes angular#16543 Closes angular#16544

m-amr · 2018-05-16T04:36:38Z

@Narretz
Thanks for your feedback.
I have applied the requested changes.

please review it again.

Narretz · 2018-05-17T09:43:55Z

Hi @m-amr, almost there. Just change sameSite to samesite everywhere. Completely lowercase. Sorry if I wasn't clear about this before.

Closes angular#16543 Closes angular#16544

m-amr · 2018-05-17T16:06:50Z

Hi @Narretz ,
I have applied your change request.
Can you please review it again ?

Thanks

Closes angular#16543 Closes angular#16544 Closes angular#16544

googlebot added the cla: yes label May 1, 2018

m-amr force-pushed the support_same_site_cookies branch from 164d546 to cbf09b3 Compare May 1, 2018 20:09

m-amr added a commit to m-amr/angular.js that referenced this pull request May 1, 2018

feat(ngCookies): support SameSite option

cbf09b3

Closes angular#16543 Closes angular#16544

m-amr force-pushed the support_same_site_cookies branch from cbf09b3 to 284ea94 Compare May 1, 2018 20:42

m-amr added a commit to m-amr/angular.js that referenced this pull request May 1, 2018

feat(ngCookie): support SameSite option

284ea94

Closes angular#16543 Closes angular#16544

Narretz suggested changes May 4, 2018

View reviewed changes

Narretz added this to the 1.7.x milestone May 4, 2018

Narretz added type: feature component: ngCookies labels May 4, 2018

m-amr force-pushed the support_same_site_cookies branch from 284ea94 to 83e1b3e Compare May 4, 2018 18:54

m-amr added a commit to m-amr/angular.js that referenced this pull request May 4, 2018

feat(ngCookie): support SameSite option

83e1b3e

Closes angular#16543 Closes angular#16544

m-amr force-pushed the support_same_site_cookies branch from 83e1b3e to 1249c3b Compare May 4, 2018 19:10

m-amr added a commit to m-amr/angular.js that referenced this pull request May 4, 2018

feat(ngCookie): support SameSite option

1249c3b

Closes angular#16543 Closes angular#16544

m-amr force-pushed the support_same_site_cookies branch from 1249c3b to 3c8b926 Compare May 4, 2018 19:12

m-amr added a commit to m-amr/angular.js that referenced this pull request May 4, 2018

feat(ngCookie): support SameSite option

3c8b926

Closes angular#16543 Closes angular#16544

m-amr force-pushed the support_same_site_cookies branch from 3c8b926 to 10c24b3 Compare May 4, 2018 19:13

m-amr added a commit to m-amr/angular.js that referenced this pull request May 4, 2018

feat(ngCookie): support SameSite option

10c24b3

Closes angular#16543 Closes angular#16544

m-amr force-pushed the support_same_site_cookies branch from 10c24b3 to 84609bd Compare May 4, 2018 19:15

m-amr added a commit to m-amr/angular.js that referenced this pull request May 4, 2018

feat(ngCookie): support SameSite option

84609bd

Closes angular#16543 Closes angular#16544

petebacondarwin approved these changes May 14, 2018

View reviewed changes

petebacondarwin self-assigned this May 14, 2018

m-amr force-pushed the support_same_site_cookies branch from 84609bd to 3073055 Compare May 14, 2018 19:57

m-amr added a commit to m-amr/angular.js that referenced this pull request May 14, 2018

feat(ngCookie): support SameSite option

3073055

Closes angular#16543 Closes angular#16544

m-amr force-pushed the support_same_site_cookies branch from 3073055 to de8d8c1 Compare May 14, 2018 19:58

m-amr added a commit to m-amr/angular.js that referenced this pull request May 14, 2018

feat(ngCookie): support SameSite option

de8d8c1

Closes angular#16543 Closes angular#16544

m-amr force-pushed the support_same_site_cookies branch from de8d8c1 to 2304e2e Compare May 14, 2018 20:00

m-amr added a commit to m-amr/angular.js that referenced this pull request May 14, 2018

feat(ngCookie): support sameSite option

2304e2e

Closes angular#16543 Closes angular#16544

m-amr force-pushed the support_same_site_cookies branch from 2304e2e to 56bd612 Compare May 16, 2018 04:31

m-amr added a commit to m-amr/angular.js that referenced this pull request May 16, 2018

feat(ngCookie): support sameSite option

56bd612

Closes angular#16543 Closes angular#16544

m-amr force-pushed the support_same_site_cookies branch from 56bd612 to f05af0e Compare May 16, 2018 04:33

m-amr added a commit to m-amr/angular.js that referenced this pull request May 16, 2018

feat(ngCookie): support sameSite option

f05af0e

Closes angular#16543 Closes angular#16544

feat(ngCookie): support sameSite option

c211b52

Closes angular#16543 Closes angular#16544

m-amr force-pushed the support_same_site_cookies branch from f05af0e to c211b52 Compare May 17, 2018 16:05

Narretz approved these changes May 17, 2018

View reviewed changes

Narretz merged commit 10a229c into angular:master May 17, 2018

Narretz pushed a commit to Narretz/angular.js that referenced this pull request May 18, 2018

feat(ngCookie): support sameSite option

2aabf1f

Closes angular#16543 Closes angular#16544 Closes angular#16544

dependencies bot mentioned this pull request Jun 8, 2018

Update 6 dependencies from npm JetBrains/ring-ui#281

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ngCookies): support sameSite option #16544

feat(ngCookies): support sameSite option #16544

m-amr commented May 1, 2018 •

edited

Loading

Narretz May 4, 2018

Narretz May 4, 2018

m-amr commented May 4, 2018 •

edited

Loading

Narretz commented May 14, 2018 •

edited

Loading

m-amr commented May 14, 2018 •

edited

Loading

Narretz commented May 15, 2018

m-amr commented May 16, 2018

Narretz commented May 17, 2018

m-amr commented May 17, 2018

feat(ngCookies): support sameSite option #16544

feat(ngCookies): support sameSite option #16544

Conversation

m-amr commented May 1, 2018 • edited Loading

Narretz May 4, 2018

Choose a reason for hiding this comment

Narretz May 4, 2018

Choose a reason for hiding this comment

m-amr commented May 4, 2018 • edited Loading

Narretz commented May 14, 2018 • edited Loading

m-amr commented May 14, 2018 • edited Loading

Narretz commented May 15, 2018

m-amr commented May 16, 2018

Narretz commented May 17, 2018

m-amr commented May 17, 2018

m-amr commented May 1, 2018 •

edited

Loading

m-amr commented May 4, 2018 •

edited

Loading

Narretz commented May 14, 2018 •

edited

Loading

m-amr commented May 14, 2018 •

edited

Loading