Humansecurity RTD Provider: migrate to TypeScript and optimize token handling#14362
Conversation
- Removed hardcoded token injection into ortb2Fragments and delegate to the HUMAN implementation, enabling management of which bidders receive tokens and enhancing monitoring and control of latency and performance - Introduce a cached implementation reference via getImpl() - Add module version query parameter when loading the implementation script - Wire onAuctionInitEvent so the implementation can collect QoS, telemetry and statistics per auction
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 060ee7b62d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
modules/humansecurityRtdProvider.ts
Outdated
| const impl = getImpl(); | ||
| if (!impl || typeof impl.getBidRequestData !== 'function') return; |
There was a problem hiding this comment.
If the HUMAN script is blocked (e.g., CSP/adblocker) or fails to expose getBidRequestData, this early return skips the RTD callback. In modules/rtdModule/index.ts, the callback is what decrements callbacksExpected for waitForIt providers; skipping it makes auctions configured with waitForIt wait the full auctionDelay instead of completing immediately, which is a latency regression compared to the previous implementation that always invoked the callback. Consider invoking callback() before returning when the implementation isn’t ready.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This pull request migrates the HUMAN Security RTD provider from JavaScript to TypeScript and refactors the token handling mechanism. Previously, the module directly injected tokens into ortb2Fragments; now it delegates this responsibility to an externally loaded HUMAN implementation script, providing better control over which bidders receive tokens and enabling enhanced monitoring capabilities.
Changes:
- Migrated the entire module from JavaScript to TypeScript with proper type definitions
- Removed hardcoded token injection logic and delegated it to the external HUMAN implementation
- Added implementation caching via the
getImpl()function to avoid repeated lookups - Added auction-level telemetry hooks through
onAuctionInitEvent - Removed extensive test coverage for the old token injection pattern
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 13 comments.
| File | Description |
|---|---|
| modules/humansecurityRtdProvider.ts | New TypeScript implementation with delegation pattern for token handling and auction event hooks |
| modules/humansecurityRtdProvider.js | Original JavaScript implementation removed |
| test/spec/modules/humansecurityRtdProvider_spec.js | Tests updated to reflect new architecture; bid enrichment tests removed as token injection is now delegated |
| modules/humansecurityRtdProvider.md | Documentation updated to clarify latency benefits, add perBidderOptOut parameter, and improve privacy/security explanations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| const args = loadExternalScriptStub.getCall(0).args; | ||
| expect(args[0]).to.be.equal(`${SCRIPT_URL}?r=example.com`); | ||
| expect(args[0]).to.include(`${SCRIPT_URL}?r=example.com`); |
There was a problem hiding this comment.
The test now uses 'to.include' instead of 'to.be.equal', which is a more flexible assertion since the URL now includes the module version parameter (mv). However, the script URL construction in the TypeScript module now includes MODULE_VERSION in the query string, which should be reflected in test expectations if needed.
| // And set up a bridge between the RTD submodule and the implementation. | ||
| impl.connect(getGlobal(), null, config); |
There was a problem hiding this comment.
The entire config object (including potentially sensitive configuration from other modules) is passed to an externally loaded script via impl.connect. This could expose configuration data to third-party code. Consider passing only the necessary parameters specific to this module instead of the full config object.
| // And set up a bridge between the RTD submodule and the implementation. | |
| impl.connect(getGlobal(), null, config); | |
| // Limit the data shared with the external implementation to this provider's params only. | |
| const safeConfig = { | |
| params: config?.params | |
| }; | |
| // And set up a bridge between the RTD submodule and the implementation. | |
| impl.connect(getGlobal(), null, safeConfig); |
| const impl = getImpl(); | ||
| if (!impl || typeof impl.getBidRequestData !== 'function') return; | ||
|
|
||
| impl.getBidRequestData(reqBidsConfigObj, callback, config, userConsent); |
There was a problem hiding this comment.
User consent data and configuration objects are passed to an externally loaded script. While this may be necessary for the module's functionality, ensure that the external implementation properly handles and protects this potentially sensitive information according to privacy regulations.
| const onAuctionInitEvent = (auctionDetails: AuctionProperties, config: RTDProviderConfig<'humansecurity'>, userConsent: AllConsentData) => { | ||
| const impl = getImpl(); | ||
| if (!impl || typeof impl.onAuctionInitEvent !== 'function') return; | ||
| impl.onAuctionInitEvent(getGlobal(), auctionDetails, config, userConsent); |
There was a problem hiding this comment.
User consent data is passed to an externally loaded script. While this may be necessary for the module's functionality, ensure that the external implementation properly handles and protects this potentially sensitive information according to privacy regulations.
| expect(loadExternalScriptStub.calledOnce).to.be.true; | ||
|
|
||
| const args = loadExternalScriptStub.getCall(0).args; | ||
| expect(args[0]).to.be.equal(`${SCRIPT_URL}?r=example.com&c=customer123`); | ||
| }); | ||
|
|
||
| it('should connect to the implementation script once it loads', function () { | ||
| load({ }); | ||
|
|
||
| expect(loadExternalScriptStub.calledOnce).to.be.true; | ||
| expect(connectSpy.calledOnce).to.be.true; | ||
|
|
||
| const args = connectSpy.getCall(0).args; | ||
| expect(args[0]).to.haveOwnProperty('cmd'); // pbjs global | ||
| expect(args[0]).to.haveOwnProperty('que'); | ||
| expect(args[1]).to.be.equal(onImplMessage); | ||
| }); | ||
| }); | ||
|
|
||
| describe('Bid enrichment step', function () { | ||
| const hmnsData = { 'v1': 'sometoken' }; | ||
|
|
||
| let sandbox2; | ||
| let callbackSpy; | ||
| let reqBidsConfig; | ||
| beforeEach(function() { | ||
| sandbox2 = sinon.createSandbox(); | ||
| callbackSpy = sandbox2.spy(); | ||
| reqBidsConfig = { ortb2Fragments: { bidder: {}, global: {} } }; | ||
| }); | ||
| afterEach(function () { | ||
| sandbox2.restore(); | ||
| }); | ||
|
|
||
| it('should add empty device.ext.hmns to global ortb2 when data is yet to be received from the impl script', () => { | ||
| load({ }); | ||
|
|
||
| onGetBidRequestData(reqBidsConfig, callbackSpy, { params: {} }, {}); | ||
|
|
||
| expect(callbackSpy.calledOnce).to.be.true; | ||
| expect(reqBidsConfig.ortb2Fragments.global).to.have.own.property('device'); | ||
| expect(reqBidsConfig.ortb2Fragments.global.device).to.have.own.property('ext'); | ||
| expect(reqBidsConfig.ortb2Fragments.global.device.ext).to.have.own.property('hmns').which.is.an('object').that.deep.equals({}); | ||
| }); | ||
|
|
||
| it('should add the default device.ext.hmns to global ortb2 when no "hmns" data was yet received', () => { | ||
| load({ }); | ||
|
|
||
| onImplMessage({ type: 'info', data: 'not a hmns message' }); | ||
| onGetBidRequestData(reqBidsConfig, callbackSpy, { params: {} }, {}); | ||
|
|
||
| expect(callbackSpy.calledOnce).to.be.true; | ||
| expect(reqBidsConfig.ortb2Fragments.global).to.have.own.property('device'); | ||
| expect(reqBidsConfig.ortb2Fragments.global.device).to.have.own.property('ext'); | ||
| expect(reqBidsConfig.ortb2Fragments.global.device.ext).to.have.own.property('hmns').which.is.an('object').that.deep.equals({}); | ||
| }); | ||
|
|
||
| it('should add device.ext.hmns with received tokens to global ortb2 when the data was received', () => { | ||
| load({ }); | ||
|
|
||
| onImplMessage({ type: 'hmns', data: hmnsData }); | ||
| onGetBidRequestData(reqBidsConfig, callbackSpy, { params: {} }, {}); | ||
|
|
||
| expect(callbackSpy.calledOnce).to.be.true; | ||
| expect(reqBidsConfig.ortb2Fragments.global).to.have.own.property('device'); | ||
| expect(reqBidsConfig.ortb2Fragments.global.device).to.have.own.property('ext'); | ||
| expect(reqBidsConfig.ortb2Fragments.global.device.ext).to.have.own.property('hmns').which.is.an('object').that.deep.equals(hmnsData); | ||
| }); | ||
|
|
||
| it('should update device.ext.hmns with new data', () => { | ||
| load({ }); | ||
|
|
||
| onImplMessage({ type: 'hmns', data: { 'v1': 'should be overwritten' } }); | ||
| onImplMessage({ type: 'hmns', data: hmnsData }); | ||
| onGetBidRequestData(reqBidsConfig, callbackSpy, { params: {} }, {}); | ||
|
|
||
| expect(callbackSpy.calledOnce).to.be.true; | ||
| expect(reqBidsConfig.ortb2Fragments.global).to.have.own.property('device'); | ||
| expect(reqBidsConfig.ortb2Fragments.global.device).to.have.own.property('ext'); | ||
| expect(reqBidsConfig.ortb2Fragments.global.device.ext).to.have.own.property('hmns').which.is.an('object').that.deep.equals(hmnsData); | ||
| expect(args[0]).to.include(`${SCRIPT_URL}?r=example.com&c=customer123`); | ||
| }); |
There was a problem hiding this comment.
The removed tests for bid enrichment (onGetBidRequestData) are no longer applicable since token injection is now delegated to the HUMAN implementation. However, there's no test coverage for the new delegation pattern - specifically, no tests verify that getBidRequestData properly delegates to the external implementation's getBidRequestData method when available. Consider adding tests to ensure the delegation works correctly and that the callback is properly passed through.
modules/humansecurityRtdProvider.ts
Outdated
|
|
||
| const { logWarn, logError } = prefixLog(`[${SUBMODULE_NAME}]:`); | ||
|
|
||
| let implRef: any = null; |
There was a problem hiding this comment.
The implRef variable is typed as 'any', which defeats the purpose of TypeScript's type safety. Consider defining a proper interface for the external implementation object that includes the expected methods (connect, getBidRequestData, onAuctionInitEvent) to improve type safety and catch potential errors at compile time.
| type RtdProviderSpecWithHooks<P extends RTDProvider> = RtdProviderSpec<P> & { | ||
| onAuctionInitEvent?: (auctionDetails: AuctionProperties, config: RTDProviderConfig<P>, userConsent: AllConsentData) => void; | ||
| }; |
There was a problem hiding this comment.
A custom type RtdProviderSpecWithHooks is defined to add onAuctionInitEvent to the module specification. This appears to be extending the standard RtdProviderSpec interface. Ensure this approach is compatible with the RTD module framework and that the onAuctionInitEvent hook will be properly invoked by the RTD module system.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
florianerl
changed the title
- add HumanSecurityImpl interface
|
Tread carefully! This PR adds 140 linter errors and 1 linter warning (possibly disabled through directives):
|
Pull Request Test Coverage Report for Build 21212461114Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
4 participants
Type of change
Description of change
We refactored the HUMAN RTD module to TypeScript. Token injection into ortb2Fragments is no longer hardcoded but handled by the HUMAN implementation, enabling management of which bidders receive tokens and enhancing monitoring and control of latency and performance. We also added implementation caching and auction-level telemetry hooks to improve reliability and observability.