5
\$\begingroup\$

My first Firefox add-on puts a red border around insecure links, including plain HTTP links and links with a JavaScript event handler. The code is linted and appears to have good unit and acceptance test coverage. I'm mostly worried about breaking WebExtensions or JavaScript rules or conventions.

manifest.json:

{
  "manifest_version": 2,
  "name": "__MSG_extensionName__",
  "description": "__MSG_extensionDescription__",
  "version": "1.19",
  "author": "Victor Engmark",
  "homepage_url": "https://github.com/l0b0/insecure-links-highlighter",
  "content_scripts": [
    {
      "matches": [
        "<all_urls>"
      ],
      "js": [
        "defaultOptions.js",
        "dom.js",
        "url.js",
        "highlight.js"
      ],
      "all_frames": true
    }
  ],
  "options_ui": {
    "browser_style": true,
    "page": "options.html"
  },
  "permissions": [
    "storage"
  ],
  "icons": {
    "48": "icons/48.png",
    "96": "icons/96.png"
  },
  "default_locale": "en",
  "applications": {
    "gecko": {
      "id": "{da90161a-9c5c-4315-adae-2eedbe24810a}"
    }
  }
}

defaultOptions.js:

(function (exports) {
    exports.defaultOptions = {
        borderColor: 'red',
        elementsWithEventHandlersAreInsecure: true,
        class: 'insecure-links-highlighter-highlighted',
    };
}(this));

dom.js (trimmed most of eventHandlerAttributes for brevity):

(function (exports) {
    'use strict';

    const eventHandlerAttributes = [
        'onabort',
        'onwheel',
    ];

    function commonAncestor(elements) {
        const ancestorLists = elements.map(ancestors);
        let commonAncestorIndex = 0,
            commonAncestor;

        function hasNextAncestor(ancestors) {
            return ancestors[commonAncestorIndex + 1] === commonAncestor;
        }

        do {
            commonAncestor = ancestorLists[0][commonAncestorIndex];
            commonAncestorIndex++;
        } while (ancestorLists.every(hasNextAncestor));

        return commonAncestor;
    }

    function ancestors(element) {
        let ancestors = [element];
        while (element.parentElement !== null) {
            let parent = element.parentElement;
            ancestors.unshift(parent);
            element = parent;
        }
        return ancestors;
    }

    function hasInsecureHrefAttribute(element, protocol) {
        return element.hasAttribute('href') && !isSecureURL(element.getAttribute('href'), protocol);
    }

    function hasNonDefaultEventHandler(element) {
        function hasEventHandler(handlerAttribute) {
            const attribute = element[handlerAttribute];
            return attribute !== undefined && attribute !== null;
        }

        return eventHandlerAttributes.some(hasEventHandler);
    }

    function highlight(element, configuration) {
        if (element.style.cssText !== '') {
            element.style.cssText += '; ';
        }

        element.classList.add(configuration.class);
        element.style.cssText += `border-color: ${configuration.borderColor} !important; border-style: solid !important; border-width: medium !important;`;
    }

    function getLinks(node) {
        return Array.from(node.getElementsByTagName('a'));
    }

    function isElement(node) {
        return node.nodeType === Node.ELEMENT_NODE;
    }

    exports.ancestors = ancestors;
    exports.commonAncestor = commonAncestor;
    exports.getLinks = getLinks;
    exports.hasInsecureHrefAttribute = hasInsecureHrefAttribute;
    exports.hasNonDefaultEventHandler = hasNonDefaultEventHandler;
    exports.highlight = highlight;
    exports.isElement = isElement;
}(this));

highlight.js:

(function () {
    'use strict';
    let configuration;

    function onConfigurationRetrieved(items) {
        configuration = items;
        processAndObserveDocument();
    }

    function processAndObserveDocument() {
        const attributeObserver = new MutationObserver(onAttributeMutation),
            elementObserver = new MutationObserver(onElementMutation);

        processNode(document);

        attributeObserver.observe(document.body, {'attributes': true, 'subtree': true});
        elementObserver.observe(document.body, {'childList': true, 'subtree': true});
    }

    function onAttributeMutation(mutationRecords) {
        mutationRecords.forEach(processAttributeMutationRecord);
    }

    function processAttributeMutationRecord(mutationRecord) {
        if (mutationRecord.attributeName === 'href') {
            highlightInsecureLink(mutationRecord.target);
        }
    }

    function onElementMutation(mutationRecords) {
        const addedElementsList = mutationRecords.map(mutationRecordElements),
            addedElements = [].concat.apply([], addedElementsList);

        processNode(commonAncestor(addedElements));
    }

    function mutationRecordElements(mutationRecord) {
        return Array.from(mutationRecord.addedNodes).filter(isElement);
    }

    function processNode(node) {
        getLinks(node).forEach(highlightInsecureLink);
    }

    function highlightInsecureLink(element) {
        if (
            hasInsecureHrefAttribute(element, location.protocol) ||
            (configuration.elementsWithEventHandlersAreInsecure && hasNonDefaultEventHandler(element))
        ) {
            highlight(element, configuration);
        }
    }

    browser.storage.local.get(defaultOptions).then(onConfigurationRetrieved);
}());

url.js:

(function (exports) {
    'use strict';

    const protocolPrefixRegex = new RegExp('^[a-z]+://'),

        // Known secure protocols handled by the browser
        internalSecureProtocols = ['https'],

        // Presumed secure since they are handled externally (see network.protocol-handler.external.[protocol])
        externallyHandledProtocols = ['mailto', 'news', 'nntp', 'snews'],

        // Presumed secure, commonly handled by add-ons or externally
        expectedExternallyHandledProtocols = ['tel'],

        secureProtocols = [].concat(
            internalSecureProtocols,
            externallyHandledProtocols,
            expectedExternallyHandledProtocols
        );

    function isSecureURL(url, protocol) {
        const urlProtocol = url.split(':', 1)[0].toLowerCase();

        if (secureProtocols.includes(urlProtocol)) {
            return true;
        }

        return !hasExplicitProtocol(url) && (protocol === 'file:' || protocol === 'https:');
    }

    function hasExplicitProtocol(url) {
        return protocolPrefixRegex.test(url.toLowerCase());
    }

    exports.hasExplicitProtocol = hasExplicitProtocol;
    exports.isSecureURL = isSecureURL;
}(this));
\$\endgroup\$
  • \$\begingroup\$ @Jamal The title and tag were very deliberate, because this is a cross-browser add-on. It works at least in Chromium without modification, for example. \$\endgroup\$ – l0b0 May 10 '18 at 0:49
1
\$\begingroup\$

Bloat

I started reviewing the code, most of it was good with a quick glance, I was going to comment on the long variable names and suggest you consider using common abbreviations to make the code more readable.

As I looked deeper I started to find more and more bloat. There is a point where code granularity is a burden on the source quality. One line functions called from only one location is not a good way to use functions. It make the code harder to follow, especial that the names are so long they all look the same and need to be read to find, rather than a quick scan.

The example

(function (exports) {
    'use strict';

    const protocolPrefixRegex = new RegExp('^[a-z]+://'),

        // Known secure protocols handled by the browser
        internalSecureProtocols = ['https'],

        // Presumed secure since they are handled externally (see network.protocol-handler.external.[protocol])
        externallyHandledProtocols = ['mailto', 'news', 'nntp', 'snews'],

        // Presumed secure, commonly handled by add-ons or externally
        expectedExternallyHandledProtocols = ['tel'],

        secureProtocols = [].concat(
            internalSecureProtocols,
            externallyHandledProtocols,
            expectedExternallyHandledProtocols
        );

    function isSecureURL(url, protocol) {
        const urlProtocol = url.split(':', 1)[0].toLowerCase();

        if (secureProtocols.includes(urlProtocol)) {
            return true;
        }

        return !hasExplicitProtocol(url) && (protocol === 'file:' || protocol === 'https:');
    }

    function hasExplicitProtocol(url) {
        return protocolPrefixRegex.test(url.toLowerCase());
    }

    exports.hasExplicitProtocol = hasExplicitProtocol;
    exports.isSecureURL = isSecureURL;
}(this));

It was not until I read it that I realized most of it is completely unneeded.

With identical functionality is the following

;(function (exports) {
    'use strict';
    const secure = ['https', 'mailto', 'news', 'nntp', 'snews', 'tel'];
    const protoPrefix = /^[a-z]+:\/\//;

    exports.isSecureURL = (url, protocol) => {
        url = url.toLowerCase();
        if (secure.includes(url.split(':', 1)[0])) { return true }
        return !protoPrefix.test(url) && (protocol=== 'file:' || protocol === 'https:');
    }
}(this));

The rest is similar with many redundancies and bad use of functions (too granular)

\$\endgroup\$
  • \$\begingroup\$ I'd still keep names like secureProtocols for clarity and maintainability, and I disagree that pulling out things like hasExplicitProtocol is a bad idea (for the same reason), but joining those variables might be an option. I still want to make sure a maintainer understands why nntp is considered "safe", when it isn't (or rather wasn't) normally served via an end-to-end encrypted channel. I know the JavaScript norm is functions with many lines, but I strongly consider that an antipattern. \$\endgroup\$ – l0b0 May 9 '18 at 19:41
  • \$\begingroup\$ @l0b0 The place for technical information regarding a product is not in the code. It is in the product documentation that provides a indexed, cross referenced, single, authoritative spec, (including details regarding abstractions and reasoning). The anti-pattern is splitting spec, the danger is conflict between two sources, which is highly likely if more than one coder is editing source. Or worse if the source is all there is you can not guarantee the information you have in-bedded add-hock within the code will be maintained. Source code is not an information store.. \$\endgroup\$ – Blindman67 May 9 '18 at 22:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.