JavaScript parsing error on inline href attribute of <a> tag.

[rustyconover]

When testing out AssetGraph or even using bringhome npm module it seems there is an escaping problem with inline JavaScript.

Example to trigger the error:

bringhome https://www.atncorp.com/x-sight4k-pro-day-night-rifle-scope-3-14x\?utm_source\=email+marketing+Mailigen\&utm_campaign\=Thor+LT+Refurb+%2B+Rebate\&utm_medium\=email -o foo

ParseError: Parse error in inline JavaScript in https://www.atncorp.com/x-sight4k-pro-day-night-rifle-scope-3-14x?utm_source=email+marketing+Mailigen&utm_campaign=Thor+LT+Refurb+%2B+Rebate&utm_medium=email Unexpected token (1:8)

When adding additional debug to emit the JavaScript that is attempting to be parsed the text that is attempting to be parsed is:

function()%20{%20return%20false;%20}

The tag that causes this problem is:

<a href="javascript:function() { return false; }" class="device active">

It seems that spaces or most other content in the href attribute have been URIEncoded, which is a bit odd.

[papandreou]

Hmm, yeah, we should clearly percent-decode those javascript: urls before attempting to parse them: 72e0293

However, that doesn't fully fix this case, because function() { return false; } isn't a valid FunctionDeclaration. I wonder if there's an implicit (function () { return )() around it the url? I've googled a bit, and can't really find anything about how it's supposed to be interpreted.

Maybe the workaround in #1136 (comment) will allow you to get past it? Looks like you can use bringhome --silent to wire it up that way. TBH that probably deserves its own switch, or maybe we should make even it the default. I'm sure wget also operates in a "best effort" mode in cases like this 😅 . WDYT?

[rustyconover]

The escaping change works. Thank you.

I didn't write this HTML so I'm not sure what people were thinking with this bit of JavaScript. This is just content I'm encountering out in the wild. It just one example of millions.

You're likely right, I shouldn't care much if the JavaScript is valid or not, but I think the approach of just emitting warnings could be better. I may want to have those warnings/errors be stored as part of the graph rather than just emitted at construction time. Are there any provisions to do that?

Rusty

[papandreou]

I think the approach of just emitting warnings could be better. I may want to have those warnings/errors be stored as part of the graph rather than just emitted at construction time. Are there any provisions to do that?

Most of the warnings (maybe all? 😅 ) include an asset property, so you can gather them up by asset outside the graph by registering a warn handler early on:

const warningsByAsset = new Map();
assetGraph.on('warn', err => {
  if (err.asset) {
    let warningsForAsset = warningsByAsset.get(err.asset);
    if (!warningsForAsset) {
      warningsByAsset.set(err.asset, (warningsForAsset = []));
    }
    warningsForAsset.push(err);
  } else {
    // Do something with error that doesn't point to a specific asset
  }
});

... You can obviously also put them on the asset or graph instance itself if that's handier.