Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#5011 Fix to set Secure attribute on cookie when SameSite=none #5064

Conversation

goosemanjack
Copy link
Contributor

Type of change

  • [X ] Bugfix

Description of change

This change modifies the storageManager to detect if SameSite=None is used in the set cookie options. When the value "none" is specified for SameSite the browser also requires that the cookie be marked "Secure". This change automatically applies the "Secure" attribute when samesite=none is detected.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie

Tested against DigiTrust ID that sets cookie.
(/integrationExamples/gpt/digitrust_Simple.html) sample page.
Note: this must be run over HTTPS connection to operate correctly.

  • contact email of the adapter’s maintainer
  • official adapter submission

Other Information

Must be run over HTTPS connection to operate correctly. Testing this over HTTP is an invalid scenario.

Copy link
Collaborator

@jaiminpanchal27 jaiminpanchal27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, minor change requested

@@ -64,7 +64,9 @@ export function newStorageManager({gvlid, moduleName, moduleType} = {}) {
if (result && result.valid) {
const domainPortion = (domain && domain !== '') ? ` ;domain=${encodeURIComponent(domain)}` : '';
const expiresPortion = (expires && expires !== '') ? ` ;expires=${expires}` : '';
document.cookie = `${key}=${encodeURIComponent(value)}${expiresPortion}; path=/${domainPortion}${sameSite ? `; SameSite=${sameSite}` : ''}`;
var isNone = (sameSite != null && sameSite.toLowerCase() == 'none')
var secure = (isNone) ? '; Secure' : '';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use const here and for isNone as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made requested change to use const. Thanks.

@bretg bretg requested a review from robertrmartinez April 6, 2020 01:20
@bretg bretg added the needs 2nd review Core module updates require two approvals from the core team label Apr 6, 2020
Copy link
Member

@mkendall07 mkendall07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, although probably should have test coverage

@jsnellbaker jsnellbaker merged commit 8275fc1 into prebid:master Apr 7, 2020
rjvelicaria pushed a commit to openx/Prebid.js that referenced this pull request Apr 9, 2020
…rebid#5064)

* prebid#5011 Fix to set Secure attribute on cookie when SameSite=none

* Minor change to use const instead of var per review request.
redaguermas added a commit to redaguermas/Prebid.js that referenced this pull request Apr 16, 2020
* 'master' of https://github.com/prebid/Prebid.js: (102 commits)
  Marsmedia - Add vastXml and fix id response (prebid#5067)
  PubMatic adapter to support image sync (prebid#5104)
  minor consentManagement fix (prebid#5050)
  fix circle ci failing tests (prebid#5113)
  Add Relaido Adapter (prebid#5101)
  Add new bid adapter for ConnectAd (prebid#4806)
  change payload (prebid#5105)
  Utils updates (prebid#5092)
  Read OpenRTB app objects if set in config + bug fix for when ad units are reloaded (prebid#5086)
  Criteo : added first party data mapping to bidder request (prebid#4954)
  updateAdGenerationManual (prebid#5032)
  New bid adapter: Wipes (prebid#5051)
  Prebid manager analytics utm tags (prebid#4998)
  CRITEO RTUS Integration with Yieldmo Prebid (prebid#5075)
  isSafariBrowser update  (prebid#5077)
  Support min &max duration for onevideo (prebid#5079)
  increment pre version
  Prebid 3.15.0 release
  prebid#5011 Fix to set Secure attribute on cookie when SameSite=none (prebid#5064)
  Prebid adapter for windtalker (prebid#5040)
  ...
iggyfisk pushed a commit to happypancake/Prebid.js that referenced this pull request Jun 22, 2020
…rebid#5064)

* prebid#5011 Fix to set Secure attribute on cookie when SameSite=none

* Minor change to use const instead of var per review request.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LGTM needs 2nd review Core module updates require two approvals from the core team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants