Skip to content

Commit

Permalink
🐛Fix SRA in no signing exp (#30037)
Browse files Browse the repository at this point in the history
* move body check to 204

* return real response from sra

* sraBlockCallbackHandler test
  • Loading branch information
calebcordry committed Aug 28, 2020
1 parent edce2e8 commit 4c94230
Show file tree
Hide file tree
Showing 4 changed files with 72 additions and 9 deletions.
2 changes: 1 addition & 1 deletion extensions/amp-a4a/0.1/amp-a4a.js
Expand Up @@ -883,7 +883,7 @@ export class AmpA4A extends AMP.BaseElement {
* @return {Promise<?./head-validation.ValidatedHeadDef>}
*/
streamResponse_(httpResponse, checkStillCurrent) {
if (!httpResponse.body) {
if (httpResponse.status === 204) {
this.forceCollapse();
return Promise.reject(NO_CONTENT_RESPONSE);
}
Expand Down
Expand Up @@ -1714,7 +1714,8 @@ export class AmpAdNetworkDoubleclickImpl extends AmpA4A {
headersObj,
done,
sraRequestAdUrlResolvers,
sraUrl
sraUrl,
this.isInNoSigningExp()
);
}
);
Expand Down
42 changes: 35 additions & 7 deletions extensions/amp-ad-network-doubleclick-impl/0.1/sra-utils.js
Expand Up @@ -399,13 +399,15 @@ function serializeItem_(key, value) {
* @param {boolean} done
* @param {!Array<function(?Response)>} sraRequestAdUrlResolvers
* @param {string} sraUrl url of SRA request for error reporting
* @param {boolean=} isNoSigning
*/
export function sraBlockCallbackHandler(
creative,
headersObj,
done,
sraRequestAdUrlResolvers,
sraUrl
sraUrl,
isNoSigning
) {
const headerNames = Object.keys(headersObj);
if (headerNames.length == 1 && isObject(headersObj[headerNames[0]])) {
Expand Down Expand Up @@ -435,12 +437,20 @@ export function sraBlockCallbackHandler(
},
has: (name) => !!headersObj[name.toLowerCase()],
});
const fetchResponse =
/** @type {?Response} */
({
headers,
arrayBuffer: () => tryResolve(() => utf8Encode(creative)),
});

let fetchResponse;
if (isNoSigning) {
const stringifiedHeaders = stringifyHeaderValues(headersObj);
fetchResponse = new Response(creative, {headers: stringifiedHeaders});
} else {
fetchResponse =
/** @type {?Response} */
({
headers,
arrayBuffer: () => tryResolve(() => utf8Encode(creative)),
});
}

// Pop head off of the array of resolvers as the response
// should match the order of blocks declared in the ad url.
// This allows the block to start rendering while the SRA
Expand All @@ -457,3 +467,21 @@ export function sraBlockCallbackHandler(
);
}
}

/**
* Takes any parsed header values from the object that are not strings and
* converts them back to the orginal stringified version.
* TODO above indicates this might get fixed upstream at some point.
* @param {!Object} headersObj
* @return {!Object<string, string>}
*/
function stringifyHeaderValues(headersObj) {
return Object.keys(headersObj).reduce((stringifiedHeaders, headerName) => {
let headerValue = headersObj[headerName];
if (headerValue && typeof headerValue != 'string') {
headerValue = JSON.stringify(headerValue);
}
stringifiedHeaders[headerName] = headerValue;
return stringifiedHeaders;
}, {});
}
Expand Up @@ -800,6 +800,40 @@ describes.realWin('Doubleclick SRA', config, (env) => {
});
});

it('should return a real response in no-siging exp', async () => {
const headerObj = {a: 'b', c: 123};
const slotDeferred = new Deferred();
const sraRequestAdUrlResolvers = [
slotDeferred.resolve,
{
resolve: () => {
throw new Error();
},
},
];
sraBlockCallbackHandler(
creative,
headerObj,
/* done */ false,
sraRequestAdUrlResolvers,
/* sraUrl */ 'http://www.example.com',
/* isNoSigning */ true
);
expect(sraRequestAdUrlResolvers.length).to.equal(1);
const fetchResponse = await slotDeferred.promise;
expect(fetchResponse).to.be.an.instanceof(Response);
expect(fetchResponse.body).to.be.an.instanceof(ReadableStream);
expect(fetchResponse.headers.get('a')).to.equal('b');
expect(fetchResponse.headers.get('c')).to.equal('123');
expect(
fetchResponse.headers.get(RENDERING_TYPE_HEADER.toLowerCase())
).to.equal(XORIGIN_MODE.SAFEFRAME);
expect(fetchResponse.headers.has('unknown')).to.be.false;

const text = await fetchResponse.text();
expect(text).to.equal(creative);
});

it('should handle multiple blocks', () => {
const blocks = [
{
Expand Down

0 comments on commit 4c94230

Please sign in to comment.