When sniffing for exports, make sure exports is not an HTMLElement #35

Closed
chrisdickinson opened this Issue Jan 23, 2014 · 17 comments

Comments

Projects
None yet
9 participants
@chrisdickinson

Otherwise HTML like so:

<div id="exports"></div>

... can trip up the detection and trigger an exception by attempting to access module.exports, since elements with ids are automatically made globally available by their id value.

if(typeof exports === 'object' && '' + exports === '[object Object]') should fix this issue.

@jrburke

This comment has been minimized.

Show comment
Hide comment
@jrburke

jrburke Jan 29, 2014

Member

I would prefer some other check besides sniffing more on the exports object. What about testing for typeof require === 'function', as that needs to be there for exports to also be there?

Member

jrburke commented Jan 29, 2014

I would prefer some other check besides sniffing more on the exports object. What about testing for typeof require === 'function', as that needs to be there for exports to also be there?

@chrisdickinson

This comment has been minimized.

Show comment
Hide comment
@chrisdickinson

chrisdickinson Jan 29, 2014

You could end up with false positives due to require.js being present.

You could end up with false positives due to require.js being present.

@chrisdickinson

This comment has been minimized.

Show comment
Hide comment
@chrisdickinson

chrisdickinson Jan 29, 2014

The environment:

  1. A UMD-bundled (by browserify) package is introduced via script tag with the intent of introducing a global.
  2. Require.JS is present, but the codebase expects that global to be available.
  3. There is a div with an id of exports on the page.

Currently, this errors because exports is defined and an object, but module is not available.

If require were checked, it would still error because module remains unavailable.

Why would you prefer to avoid inspecting the exports object more closely? Further inspection would eliminate the possible case that both exports and require are id'd on the DOM.

The environment:

  1. A UMD-bundled (by browserify) package is introduced via script tag with the intent of introducing a global.
  2. Require.JS is present, but the codebase expects that global to be available.
  3. There is a div with an id of exports on the page.

Currently, this errors because exports is defined and an object, but module is not available.

If require were checked, it would still error because module remains unavailable.

Why would you prefer to avoid inspecting the exports object more closely? Further inspection would eliminate the possible case that both exports and require are id'd on the DOM.

@jrburke

This comment has been minimized.

Show comment
Hide comment
@jrburke

jrburke Jan 29, 2014

Member

If the UMD bundle is loaded first, requirejs has not been evaluated yet. It seems to me that by checking if (typeof exports === 'object' && typeof require === 'function', that would not match the node/commonjs case since the UMD-wrapped browserify would not be exposing a require()? If there was a an element with ID of require, then it would not be typeof function.

I would prefer some other check to exports.toString(), open to other ideas besides the require check.

Member

jrburke commented Jan 29, 2014

If the UMD bundle is loaded first, requirejs has not been evaluated yet. It seems to me that by checking if (typeof exports === 'object' && typeof require === 'function', that would not match the node/commonjs case since the UMD-wrapped browserify would not be exposing a require()? If there was a an element with ID of require, then it would not be typeof function.

I would prefer some other check to exports.toString(), open to other ideas besides the require check.

@matthewwithanm

This comment has been minimized.

Show comment
Hide comment
@matthewwithanm

matthewwithanm Jan 29, 2014

What about checking module as well?

if (
  typeof module === 'object'
  && !!module // guard against nulls since typeof null === 'object'
  && typeof module.exports === 'object'
  && !!exports // another null guard, if you're worried about this.
) {

What about checking module as well?

if (
  typeof module === 'object'
  && !!module // guard against nulls since typeof null === 'object'
  && typeof module.exports === 'object'
  && !!exports // another null guard, if you're worried about this.
) {
@desandro

This comment has been minimized.

Show comment
Hide comment
@desandro

desandro Mar 17, 2014

I like @chrisdickinson's original suggestion

typeof exports === 'object' && 'exports.toString() === '[object Object]'

I would prefer some other check to exports.toString()

How about

typeof exports === 'object' && exports.toString().indexOf('HTML') === -1

If exports is an element, toString() will return [object HTMLDivElement] or the like.

I like @chrisdickinson's original suggestion

typeof exports === 'object' && 'exports.toString() === '[object Object]'

I would prefer some other check to exports.toString()

How about

typeof exports === 'object' && exports.toString().indexOf('HTML') === -1

If exports is an element, toString() will return [object HTMLDivElement] or the like.

@desandro desandro referenced this issue in desandro/imagesloaded Mar 17, 2014

Closed

Uncaught ReferenceError: module is not defined #137

@matthewwithanm

This comment has been minimized.

Show comment
Hide comment
@matthewwithanm

matthewwithanm Mar 17, 2014

@desandro Why is this preferable to checking module and module.exports? At first blush it seems more fragile IMO.

@desandro Why is this preferable to checking module and module.exports? At first blush it seems more fragile IMO.

@girokon

This comment has been minimized.

Show comment
Hide comment
@girokon

girokon Mar 18, 2014

@chrisdickinson
if exports is an element, another way check its nodeName property:

typeof exports === 'object' && typeof exports.nodeName !== 'string'

girokon commented Mar 18, 2014

@chrisdickinson
if exports is an element, another way check its nodeName property:

typeof exports === 'object' && typeof exports.nodeName !== 'string'
@addyosmani

This comment has been minimized.

Show comment
Hide comment
@addyosmani

addyosmani Jun 19, 2015

Member

@jrburke would you have any opposition to us landing the change proposed by @girokon?

https://twitter.com/mikesherov/status/612009844745347073 cc @mikesherov

Member

addyosmani commented Jun 19, 2015

@jrburke would you have any opposition to us landing the change proposed by @girokon?

https://twitter.com/mikesherov/status/612009844745347073 cc @mikesherov

@jrburke

This comment has been minimized.

Show comment
Hide comment
@jrburke

jrburke Jun 19, 2015

Member

I agree with the twitter thread, for ones that use module.exports focus the tests on that object structure, otherwise the exports checks seem fine. I am not an expert on the proper test for "not a DOM node" but that check seems reasonable. So sounds good to me.

Member

jrburke commented Jun 19, 2015

I agree with the twitter thread, for ones that use module.exports focus the tests on that object structure, otherwise the exports checks seem fine. I am not an expert on the proper test for "not a DOM node" but that check seems reasonable. So sounds good to me.

@Mouvedia

This comment has been minimized.

Show comment
Hide comment
@Mouvedia

Mouvedia Jul 3, 2015

Why about typeof exports === 'object' && !!exports && !exports.attributes?

  1. basic type checking
  2. null protection
  3. http://www.w3.org/TR/REC-DOM-Level-1/level-one-core.html#ID-1950641247

I would advise against using toString because it can be spoofed with Symbol.toStringTag. Concerning nodeName it's not supported by older versions of IE so you would end up with a false positive.

Mouvedia commented Jul 3, 2015

Why about typeof exports === 'object' && !!exports && !exports.attributes?

  1. basic type checking
  2. null protection
  3. http://www.w3.org/TR/REC-DOM-Level-1/level-one-core.html#ID-1950641247

I would advise against using toString because it can be spoofed with Symbol.toStringTag. Concerning nodeName it's not supported by older versions of IE so you would end up with a false positive.

mikesherov added a commit to mikesherov/spin.js that referenced this issue Jul 21, 2015

Update UMD Shim to be resilient to HTMLElement global pollution
Please see https://github.com/umdjs/umd/pull/63/files and umdjs/umd#35 for more information, but
this change prevents an HTML element with an id of `module` from cuasing the UMD shim to incorrectly detect the enivornment as node.

mikesherov added a commit to mikesherov/mustache.js that referenced this issue Jul 22, 2015

Update UMD Shim to be resilient to HTMLElement global pollution
Please see umdjs/umd#63 and umdjs/umd#35 for more information, but
this change prevents an HTML element with an id of `exports` from causing the UMD shim to incorrectly detect the environment as node.

mikesherov added a commit to mikesherov/mustache.js that referenced this issue Jul 22, 2015

Update UMD Shim to be resilient to HTMLElement global pollution
Please see umdjs/umd#63 and umdjs/umd#35 for more information, but
this change prevents an HTML element with an id of `exports` from causing the UMD shim to incorrectly detect the environment as node.
@jacksonrayhamilton

This comment has been minimized.

Show comment
Hide comment
@jacksonrayhamilton

jacksonrayhamilton Aug 26, 2015

typeof exports.nodeName !== 'string' precludes you from having an export called nodeName which is a string.

Wouldn't a check like this be more precise?

typeof exports === 'object' && !/^\[object HTML.*?Element\]$/.test(Object.prototype.toString.call(exports))

It looks like all objects implementing the HTMLElement interface will have a toString representation matching /^\[object HTML.*?Element\]$/. (See the header "Related pages for HTML DOM" on the left navigation for the list of implementors.)

typeof exports.nodeName !== 'string' precludes you from having an export called nodeName which is a string.

Wouldn't a check like this be more precise?

typeof exports === 'object' && !/^\[object HTML.*?Element\]$/.test(Object.prototype.toString.call(exports))

It looks like all objects implementing the HTMLElement interface will have a toString representation matching /^\[object HTML.*?Element\]$/. (See the header "Related pages for HTML DOM" on the left navigation for the list of implementors.)

@mikesherov

This comment has been minimized.

Show comment
Hide comment
@mikesherov

mikesherov Aug 26, 2015

Contributor

@jacksonrayhamilton, correct me if I'm wrong, the UMD shim is intended to wrap code that doesn't reference the export object internally. Furthermore, exports in node is unique on a per file basis, so no two files share an export object, and aren't at risk of pollution from other files.

With that said, the exports and define checks are duck typing at its finest. The nodeName check prevents a false positive from browser global pollution. It's likely "good enough". Your solution prevents a situation in which your using node code that isn't UMD wrapped in a browser environment and also exports a nodeName property as a string. Unlikely.

Contributor

mikesherov commented Aug 26, 2015

@jacksonrayhamilton, correct me if I'm wrong, the UMD shim is intended to wrap code that doesn't reference the export object internally. Furthermore, exports in node is unique on a per file basis, so no two files share an export object, and aren't at risk of pollution from other files.

With that said, the exports and define checks are duck typing at its finest. The nodeName check prevents a false positive from browser global pollution. It's likely "good enough". Your solution prevents a situation in which your using node code that isn't UMD wrapped in a browser environment and also exports a nodeName property as a string. Unlikely.

@mikesherov

This comment has been minimized.

Show comment
Hide comment
@mikesherov

mikesherov Aug 26, 2015

Contributor

I'm curious though to see what @addyosmani thinks though. If users are reporting your issue @jacksonrayhamilton, or if this is just bikeshedding. What if someone reassigns the export object in their node code to an html element? the proposed solution is equally vulnerable.

If a user isn't UMD wrapping all the code being bundled, it's all at risk regardless of whatever duck type checking you do.

Contributor

mikesherov commented Aug 26, 2015

I'm curious though to see what @addyosmani thinks though. If users are reporting your issue @jacksonrayhamilton, or if this is just bikeshedding. What if someone reassigns the export object in their node code to an html element? the proposed solution is equally vulnerable.

If a user isn't UMD wrapping all the code being bundled, it's all at risk regardless of whatever duck type checking you do.

@jacksonrayhamilton

This comment has been minimized.

Show comment
Hide comment
@jacksonrayhamilton

jacksonrayhamilton Aug 26, 2015

What if someone reassigns the export object in their node code to an html element? the proposed solution is equally vulnerable.

I think this would protect against that in all cases (except the case where that exported element is the element with the exports id):

typeof exports === 'object' && (typeof document !== 'object' || document.getElementById('exports') !== exports)

I also wonder whether this element id case is even worth supporting. There are infinitely many more cases where exports could get messed up, many equally as silly as giving a div an id of exports. It is perhaps just as likely that someone will declare a global variable exports as he will make a div with an id of exports.

What if someone reassigns the export object in their node code to an html element? the proposed solution is equally vulnerable.

I think this would protect against that in all cases (except the case where that exported element is the element with the exports id):

typeof exports === 'object' && (typeof document !== 'object' || document.getElementById('exports') !== exports)

I also wonder whether this element id case is even worth supporting. There are infinitely many more cases where exports could get messed up, many equally as silly as giving a div an id of exports. It is perhaps just as likely that someone will declare a global variable exports as he will make a div with an id of exports.

@mikesherov

This comment has been minimized.

Show comment
Hide comment
@mikesherov

mikesherov Aug 27, 2015

Contributor

The difference being that this exact scenario was happening to me and to at least one other person.

I am happy to continue to think up more ways to make this more and more solid, however my personal preference is to leave alone until someone uncovers a real world situation your latest solution addresses that the current doesn't.

Contributor

mikesherov commented Aug 27, 2015

The difference being that this exact scenario was happening to me and to at least one other person.

I am happy to continue to think up more ways to make this more and more solid, however my personal preference is to leave alone until someone uncovers a real world situation your latest solution addresses that the current doesn't.

@Mouvedia

This comment has been minimized.

Show comment
Hide comment
@Mouvedia

Mouvedia Aug 27, 2015

The only problem about the last condition is that on earlier IE you are checking undefined !== 'string'. That's why I prefer !exports.nodeType.

The only problem about the last condition is that on earlier IE you are checking undefined !== 'string'. That's why I prefer !exports.nodeType.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment