VASTClient not throwing error or emitting wrong error code for invalid wrapper VAST

ccampbell5 commented 1 month ago

Hi, I'm using version 6.0.1 and noticing inconsistent behavior with the VASTClient in how it processes wrapper VASTs that contain errors. The following sample code contains two wrapper VASTs; expected vs actual behavior is indicated in comments. (Just swap the test constant.) Also attaching the package.json file. package.json

The expects303Error VAST is a two-layer wrapper that eventually points to an empty VAST document: <VAST xsi:noNamespaceSchemaLocation="vast.xsd" version="3.0"/>. According to the docs, the error code for "No VAST response after one or more Wrappers" is 303. However, no 'VAST-error' event is emitted for this VAST.
The expectsThrowError wrapper VAST points to an unsupported 1.0 VAST document; in this case, my understanding is that the VASTClient's parseVAST method should return a rejected Promise since the root element is not 'VAST'. However, the resulting promise is not rejected, and the parser emits a 'VAST-error' with the 301 (timeout) code. (If you follow the VASTAdTagURI link and copy the VAST 1.0 XML from there, then pass it to parseVAST, then the expected rejected Promise is returned.)

import { VASTClient } from "@dailymotion/vast-client";
import xmlDom from "@xmldom/xmldom";
import fetch from "node-fetch";

const client = new VASTClient();

client.getParser().on("VAST-error", (err) => {
  console.log("VAST-error: ", err);
});

const getXML = (text) => {
  const parser = new xmlDom.DOMParser({
    errorHandler: (level, msg) => {
      throw new Error(`[Level: ${level}]: ${msg}`);
    },
  });

  const xml = parser.parseFromString(text, "text/xml");
  const err = xml
    ?.querySelector?.("parsererror")
    ?.querySelector?.("div")?.textContent;

  if (err) {
    throw new Error(err);
  }

  return xml;
};

// Expected:
// Should emit a 303 error since
// https://pubads.g.doubleclick.net/gampad/ads?iu=/21775744923/external/single_ad_samples&sz=640x480&cust_params=sample_ct%3Dredirecterror&ciu_szs=300x250%2C728x90&gdfp_req=1&output=vast&unviewed_position_start=1&env=vp&impl=s&nofb=1&correlator=
// will emit a 303 error

// Actual: No error is emitted
const expects303Error = `<VAST version="4.0" xmlns="http://www.iab.com/VAST">
<Ad id="20011" sequence="1" conditionalAd="false">
  <Wrapper followAdditionalWrappers="true" allowMultipleAds="1" fallbackOnNoAd="0">
    <AdSystem version="4.0">iabtechlab</AdSystem>
    <Error>http://example.com/error</Error>
    <Impression id="Impression-ID">http://example.com/track/impression</Impression>
    <Creatives>
      <Creative id="5480" sequence="1" adId="2447226">
        <CompanionAds>
          <Companion id="1232" width="100" height="150" assetWidth="250" assetHeight="200" expandedWidth="350" expandedHeight="250" apiFramework="VPAID" adSlotID="3214" pxratio="1400">
            <StaticResource creativeType="image/png">
              <![CDATA[https://www.iab.com/wp-content/uploads/2014/09/iab-tech-lab-6-644x290.png]]>
            </StaticResource>
            <CompanionClickThrough>
              <![CDATA[https://iabtechlab.com]]>
            </CompanionClickThrough>
          </Companion>
        </CompanionAds>
      </Creative>
    </Creatives>
    <VASTAdTagURI>
      <![CDATA[https://pubads.g.doubleclick.net/gampad/ads?iu=/21775744923/external/single_ad_samples&sz=640x480&cust_params=sample_ct%3Dredirecterror&ciu_szs=300x250%2C728x90&gdfp_req=1&output=vast&unviewed_position_start=1&env=vp&impl=s&nofb=1&correlator=]]>
    </VASTAdTagURI>
  </Wrapper>
</Ad>
</VAST>
`;

// Expected: Should throw an error since the root tag of the XML is not 'VAST'
// Actual: Parses successfully and emits a 301 (wrapper timeout)
const expectsThrowError = `
<VAST version="4.0" xmlns="http://www.iab.com/VAST">
<Ad id="20011" sequence="1" conditionalAd="false">
  <Wrapper followAdditionalWrappers="true" allowMultipleAds="1" fallbackOnNoAd="0">
    <AdSystem version="4.0">iabtechlab</AdSystem>
    <Error>http://example.com/error</Error>
    <Impression id="Impression-ID">http://example.com/track/impression</Impression>
    <Creatives>
      <Creative id="5480" sequence="1" adId="2447226">
        <CompanionAds>
          <Companion id="1232" width="100" height="150" assetWidth="250" assetHeight="200" expandedWidth="350" expandedHeight="250" apiFramework="VPAID" adSlotID="3214" pxratio="1400">
            <StaticResource creativeType="image/png">
              <![CDATA[https://www.iab.com/wp-content/uploads/2014/09/iab-tech-lab-6-644x290.png]]>
            </StaticResource>
            <CompanionClickThrough>
              <![CDATA[https://iabtechlab.com]]>
            </CompanionClickThrough>
          </Companion>
        </CompanionAds>
      </Creative>
    </Creatives>
    <VASTAdTagURI>
      <![CDATA[https://raw.githubusercontent.com/InteractiveAdvertisingBureau/VAST_Samples/master/VAST%201-2.0%20Samples/vast1Nonlinear.xml    ]]>
    </VASTAdTagURI>
  </Wrapper>
</Ad>
</VAST>`;

const test = expectsThrowError;

export function replaceUrlCorrelator(url) {
  const newURI = new URL(url);
  newURI.searchParams.set(
    "correlator",
    `${Math.floor(Math.random() * Date.now())}`
  );
  return newURI.toString();
}

async function urlHandlerGet(url, options) {
  const urlWithCorrelator = replaceUrlCorrelator(url);
  const response = await fetch(urlWithCorrelator, {
    ...options,
    credentials: options.withCredentials ? "include" : "omit",
  });
  const text = await response.text();

  return {
    document: getXML(text),
    original: text,
    status: response.status,
  };
}

const options = {
  resolveAll: true,
  wrapperLimit: 10,
  allowMultipleAds: true,
  urlHandler: {
    get: async (url, options) => {
      try {
        const result = await urlHandlerGet(url, options);
        console.log("Result from URL Handler for URL: ");
        console.log(url);
        return {
          xml: result.document,
          details: {
            byteLength: result.original.length,
            statusCode: result.status,
            rawXml: result.original,
          },
        };
      } catch (err) {
        console.log("Error from URL Handler for URL: ");
        console.log(url);
        console.log(err);
        return {
          error: err,
        };
      }
    },
  },
};

const parse = async () => {
  const vastXml = getXML(test);

  client
    .parseVAST(vastXml, options)
    .then((res) => {
      console.log("Parsed!! ");
      console.log(res);
    })
    .catch((err) => {
      console.log("Error! ", err);
    });
};

parse();

ZacharieTFR commented 3 weeks ago

Thanks for raising the issue, we will look at it asap 😉

ZacharieTFR commented 2 weeks ago

Hello, I've looked at the issues:

About expects303Error use case: I found out that without companions ads in the root vast, the 303 error is triggered as expected. Whereas with companion, it's not triggered anymore, it come from this condition https://github.com/dailymotion/vast-client-js/blob/5d148f75770aaca03edf64c0739d9523fad06293/src/parser/vast_parser.js#L443 Since companions are inside creative node, ad.creatives won't be empty meaning the condition will not be met. We need to filter them out to know if we got an empty vast in the end.
About expectsThrowError use case: here are the culprits https://github.com/dailymotion/vast-client-js/blob/5d148f75770aaca03edf64c0739d9523fad06293/src/parser/vast_parser.js#L420 that prevent rejecting the error and https://github.com/dailymotion/vast-client-js/blob/5d148f75770aaca03edf64c0739d9523fad06293/src/parser/vast_parser.js#L418 setting the wrong error code, it should be 102: VAST version of response not supported

We're going to fix it, but if you'd like a quick fix, feel free to contribute by opening a pull request! We'll be happy to look into it !

dailymotion / vast-client-js

VASTClient not throwing error or emitting wrong error code for invalid wrapper VAST #476