Skip to content

prebidAdapter secure support#1655

Merged
mkendall07 merged 1 commit intomasterfrom
feature/prebidServer-secure
Oct 6, 2017
Merged

prebidAdapter secure support#1655
mkendall07 merged 1 commit intomasterfrom
feature/prebidServer-secure

Conversation

@bretg
Copy link
Contributor

@bretg bretg commented Oct 3, 2017

Type of change

  • Bugfix
  • Feature
  • New bidder adapter
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other

Description of change

Passing the 'secure' flag from s2sConfig through to Prebid Server

Other information

Could use a better unit test -- but that stands true of this module already... looks like there's not a good suite of checks that s2sConfig values are sent through to server. e.g. accountId, timeout_millis.

@bretg bretg requested a review from mkendall07 October 3, 2017 19:01
@mkendall07
Copy link
Contributor

@bretg
Prebid server already detects this, any reason to have it set here? https://github.com/prebid/prebid-server/blob/e0df370deef5aa4467619efbe553a0c53d34206f/pbs/pbsrequest.go#L275

@bretg
Copy link
Contributor Author

bretg commented Oct 3, 2017

@mkendall07 - good to know it's at least partly covered. That certainly lowers the priority, but might still be a useful lever -- depends on the referrer that PBS receives. There are are two scenarios where the secure flag needs to be passed:

  1. secure page, secure iframe
  2. non-secure page, secure iframe - yes, we've seen this.

if the referrer is overridden to the page level (as it should be), then PBS doesn't know whether it's in scenario 2.

Copy link
Contributor

@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.

Needs update to prebid.js

tid: bidRequest.tid,
max_bids: config.maxBids,
timeout_millis: config.timeout,
secure: config.secure,
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I think the current PR is what we want -- if it's added to the object.assign in prebid.js as a default, then Prebid Server's URL check is always bypassed. The behavior of the code here is that when the s2sConfig contains a secure override, it's sent, otherwise PBS detects.

@mkendall07 mkendall07 merged commit 42282f5 into master Oct 6, 2017
@matthewlane matthewlane deleted the feature/prebidServer-secure branch October 6, 2017 23:54
rmloveland pushed a commit to prebid/prebid.github.io that referenced this pull request Oct 16, 2017
vzhukovsky added a commit to aol/Prebid.js that referenced this pull request Oct 25, 2017
….30.0 to aolgithub-master

* commit '5a8d2bf93ee15071a78e24ac976103cacf3c6021': (35 commits)
  Added changelog entry.
  Prebid 0.30.1 Release
  Remove undefined variable usage (prebid#1662)
  fixes bug for IE when invalid value passed to parse (prebid#1657)
  Aliasbidder fix (prebid#1652)
  prebidAdapter secure support (prebid#1655)
  Increment pre version
  Prebid 0.30.0 Release
  Add native param support to mediaTypes (prebid#1625)
  PulsePoint Lite adpater changes (prebid#1630)
  Appnexus ast unittest updates (prebid#1654)
  Support aspect ratio specification for native images (prebid#1634)
  Revert changes for switch between client side and server side. (prebid#1653)
  rubicon converted to bidderFactory (prebid#1624)
  Add JSDoc for `pbjs.getAllWinningBids` (prebid#1566)
  Add ignore-loader to handle .md files (prebid#1646)
  fixed PBS cookie syncs (prebid#1637)
  Add placementId request param to Yieldmo bid adapter (prebid#1632)
  Adxcg analytics adapter (prebid#1599)
  Add publisher sub-id support to the Criteo adapter (prebid#1629)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments