Add manifest-exists rule and make other improvements

.sonarrc

@@ -8,6 +8,7 @@
     "formatter": "json",
     "rules": {
         "lang-attribute": "warning",
+        "manifest-exists": "warning",
         "manifest-file-extension": "warning",
         "no-double-slash": "warning"
     }

src/lib/collectors/jsdom.ts

@@ -33,8 +33,7 @@ const debug = require('debug')('sonar:collector:jsdom');
 import * as logger from '../util/logging';
 import { readFileAsync } from '../util/misc';
 import { Sonar } from '../sonar'; // eslint-disable-line no-unused-vars
-import { Collector, CollectorBuilder, ElementFoundEvent, FetchResponse, URL } from '../types'; // eslint-disable-line no-unused-vars
-
+import { Collector, CollectorBuilder, ElementFoundEvent, NetworkData, URL } from '../types'; // eslint-disable-line no-unused-vars
 // ------------------------------------------------------------------------------
 // Defaults
 // ------------------------------------------------------------------------------
@@ -61,13 +60,14 @@ const builder: CollectorBuilder = (server: Sonar, config): Collector => {
 
     request = pify(request, { multiArgs: true });
 
-    /** Loads a url that uses the `file://` protocol taking into account if the host is Windows or *nix */
-    const _fetchFile = async (target: URL): Promise<FetchResponse> => {
+    /** Loads a url that uses the `file://` protocol taking into
+     *  account if the host is `Windows` or `*nix` */
+    const _fetchFile = async (target: URL): Promise<NetworkData> => {
         let targetPath = target.path;
 
-        /* targetPath in windows is like /c:/path/to/file.txt
-            readFileAsync will prepend c: so the final path will be:
-            c:/c:/path/to/file.txt which is not valid */
+        /* `targetPath` on `Windows` is like `/c:/path/to/file.txt`
+           `readFileAsync` will prepend `c:` so the final path will
+           be: `c:/c:/path/to/file.txt` which is not valid */
         if (path.sep === '\\' && targetPath.indexOf('/') === 0) {
             targetPath = targetPath.substr(1);
         }
@@ -77,12 +77,13 @@ const builder: CollectorBuilder = (server: Sonar, config): Collector => {
         return {
             body,
             headers: null,
-            originalBody: null
+            originalBody: null,
+            statusCode: null
         };
     };
 
     /** Loads a url (`http(s)`) combining the customHeaders with the configured ones for the collector */
-    const _fetchUrl = async (target: URL, customHeaders?: object): Promise<FetchResponse> => {
+    const _fetchUrl = async (target: URL, customHeaders?: object): Promise<NetworkData> => {
         let req;
         const href = typeof target === 'string' ? target : target.href;
 
@@ -98,12 +99,13 @@ const builder: CollectorBuilder = (server: Sonar, config): Collector => {
 
         return {
             body,
-            headers: response.headers
+            headers: response.headers,
+            statusCode: response.statusCode
             // Add original compressed bytes here (originalBytes)
         };
     };
 
-    const _fetchContent = async (target: URL | string, customHeaders?: object): Promise<FetchResponse> => {
+    const _fetchContent = async (target: URL | string, customHeaders?: object): Promise<NetworkData> => {
         let parsedTarget = target;
 
         if (typeof parsedTarget === 'string') {

src/lib/rule-context.ts

@@ -42,8 +42,8 @@ export class RuleContext {
     }
 
     /** A useful way of making requests */
-    get fetchContent() {
-        return this.sonar.fetchContent;
+    fetchContent(target, headers?) {
+        return this.sonar.fetchContent(target, headers);
     }
 
     /** Finds the exact location in the page's HTML for a match in an element */

src/lib/rules/manifest-exists/manifest-exists.md

@@ -0,0 +1,24 @@
+# Require a web app manifest file (manifest-exists)
+
+##  Rule Details
+
+This rule warns against not providing a
+[web app manifest](https://www.w3.org/TR/appmanifest) file.
+
+To ensure a web app manifest file is provided, the rule basically
+checks:
+
+ * if the web app manifest file is specified correctly in the
+   page (i.e. the page contains a single, valid declaration such
+   as: `<link rel="manifest" href="site.webmanifest">`)
+
+ * if the declared web app manifest file is actually accessible
+   (i.e. the request doesn't result in a 404, 500, etc.)
+
+In the context of [progressive web
+apps](https://en.wikipedia.org/wiki/Progressive_web_app), this
+rule is important as providing a web app manifest file is essential.
+
+## Resources
+
+* [Web App Manifest Specification](https://www.w3.org/TR/appmanifest)

src/lib/rules/manifest-exists/manifest-exists.ts

@@ -0,0 +1,117 @@
+/**
+ * @fileoverview Check if a single web app manifest file is specified,
+ * and if that specified file is accessible.
+ */
+
+// ------------------------------------------------------------------------------
+// Requirements
+// ------------------------------------------------------------------------------
+
+import * as url from 'url';
+
+import { Rule, RuleBuilder, ElementFoundEvent } from '../../types'; // eslint-disable-line no-unused-vars
+import { RuleContext } from '../../rule-context'; // eslint-disable-line no-unused-vars
+
+const debug = require('debug')('sonar:rules:manifest-exists');
+
+// ------------------------------------------------------------------------------
+// Public
+// ------------------------------------------------------------------------------
+
+const rule: RuleBuilder = {
+    create(context: RuleContext): Rule {
+
+        let manifestIsSpecified = false;
+
+        const manifestWasSpecified = () => {
+
+            // If no web app manifest file was specified when
+            // the parsing of the page ended, emit an error.
+
+            if (!manifestIsSpecified) {
+                context.report(null, null, 'Web app manifest not specified');
+            }
+        };
+
+        const manifestExists = async (data: ElementFoundEvent) => {
+            const { element, resource } = data;
+
+            if (element.getAttribute('rel') === 'manifest') {
+
+                // Check if we encounter more than one
+                // <link rel="manifest"...> declaration.
+
+                if (manifestIsSpecified) {
+                    context.report(resource, element, 'Web app manifest already specified');
+
+                    return;
+                }
+
+                manifestIsSpecified = true;
+
+                // Check if a web app manifest file is specified,
+                // and if the specified file actually exists.
+                //
+                // https://w3c.github.io/manifest/#obtaining
+
+                const manifestHref = element.getAttribute('href');
+                let manifestURL = '';
+
+                // Check if `href` doesn't exist or it has the
+                // value of empty string.
+
+                if (!manifestHref) {
+                    context.report(resource, element, `Web app manifest specified with invalid 'href'`);
+
+                    return;
+                }
+
+                // If `href` exists and is not an empty string, try
+                // to figure out the full URL of the web app manifest.
+
+                if (url.parse(manifestHref).protocol) {
+                    manifestURL = manifestHref;
+                } else {
+                    manifestURL = url.resolve(resource, manifestHref);
+                }
+
+                // Try to see if the web app manifest file actually
+                // exists and is accesible.
+
+                try {
+                    const { statusCode } = await context.fetchContent(manifestURL);
+
+                    // If it's not a local file (has statusCode === null),
+                    // check also if the status code is `200`.
+
+                    if (statusCode && statusCode !== 200) {
+                        context.report(resource, element, `Web app manifest file could not be fetched (status code: ${statusCode})`);
+                    }
+
+                // Check if fetching/reading the file failed.
+
+                } catch (e) {
+                    debug('Failed to fetch the web app manifest file');
+                    context.report(resource, element, `Web app manifest file request failed`);
+                }
+
+            }
+        };
+
+        return {
+            'element::link': manifestExists,
+            'traverse::end': manifestWasSpecified
+        };
+    },
+    meta: {
+        docs: {
+            category: 'PWA',
+            description: 'Provide a web app manifest file',
+            recommended: true
+        },
+        fixable: 'code',
+        schema: []
+    }
+};
+
+module.exports = rule;

src/lib/types.ts

@@ -36,14 +36,15 @@ export interface Collector {
     /** The headers from the response if applicable */
     headers: object;
     /** A way for you to make requests if needed */
-    fetchContent(target: URL | string, customHeaders?: object): Promise<FetchResponse>;
+    fetchContent(target: URL | string, customHeaders?: object): Promise<NetworkData>;
 }
 
 /** The response of fetching an item using the request of a collector */
-export interface FetchResponse {
+export interface NetworkData {
     body: string;
     headers: object;
-    originalBytes?: Uint8Array // TODO: is this the right type?
+    originalBytes?: Uint8Array, // TODO: is this the right type?
+    statusCode: number
 }
 
 export interface Config {

src/tests/helpers/rule-runner.ts

@@ -1,14 +1,12 @@
+import * as jsdom from 'jsdom';
 import * as path from 'path';
+import * as pify from 'pify';
+import * as sinon from 'sinon';
 
 import { test, ContextualTestContext } from 'ava'; // eslint-disable-line no-unused-vars
-import { Rule, RuleBuilder, ElementFoundEvent } from '../../lib/types'; // eslint-disable-line no-unused-vars
+import { Rule, RuleBuilder, ElementFoundEvent, NetworkData } from '../../lib/types'; // eslint-disable-line no-unused-vars
 import { RuleTest } from './rule-test-type'; // eslint-disable-line no-unused-vars
 
-
-import * as jsdom from 'jsdom';
-import * as pify from 'pify';
-import * as sinon from 'sinon';
-
 import { readFile } from '../../lib/util/misc';
 import { findProblemLocation } from '../../lib/util/location-helpers';
 
@@ -21,19 +19,21 @@ const getDOM = async (filePath) => {
 
 test.beforeEach((t) => {
     const ruleContext = {
+        async fetchContent() {
+            throw new Error('Request failed');
+        },
         findProblemLocation: (element, content) => {
             return findProblemLocation(element, { column: 0, line: 0 }, content);
         },
-
         report: sinon.spy()
     };
 
     t.context.rule = ruleBuilder.create(ruleContext);
-    t.context.report = ruleContext.report;
+    t.context.ruleContext = ruleContext;
 });
 
 test.afterEach((t) => {
-    t.context.report.reset();
+    t.context.ruleContext.report.reset();
 });
 
 /** Creates an event for HTML fixtures (`element::` events) */
@@ -49,7 +49,10 @@ const getHTMLFixtureEvent = async (event): Promise<null | ElementFoundEvent> =>
     const elementType = eventNameParts[1];
     const elements = window.document.querySelectorAll(elementType);
     const elementIndex = eventNameParts.length === 3 ? parseInt(eventNameParts[2]) : 0;
-    const eventData = <ElementFoundEvent>{ element: elements[elementIndex] };
+    const eventData = <ElementFoundEvent>{
+        element: elements[elementIndex],
+        resource: event.fixture
+    };
 
     return Promise.resolve(eventData);
 };
@@ -71,21 +74,37 @@ const getFixtureEvent = async (event): Promise<Object> => {
 
 /** Runs a test for the rule being tested */
 const runRule = async (t: ContextualTestContext, ruleTest: RuleTest) => {
+    const ruleContext = t.context.ruleContext;
     const { events, report } = ruleTest;
 
     for (const event of events) {
         const eventData = await getFixtureEvent(event);
+        const eventName = event.name.split('::')
+            .slice(0, 2)
+            .join('::');
+
+        if (event.responses) {
+            ruleContext.fetchContent = sinon.stub();
 
-        t.context.rule[event.name](eventData);
+            event.responses.forEach((response, i) => { // eslint-disable-line no-loop-func
+                ruleContext.fetchContent.onCall(i).returns(Promise.resolve(response));
+            });
+        }
+
+        await t.context.rule[eventName](eventData);
     }
 
     if (!report) {
-        t.true(t.context.report.notCalled);
+        t.true(ruleContext.report.notCalled);
+
+        return;
+    } else if (ruleContext.report.notCalled) {
+        t.fail(`report method should have been called`);
 
         return;
     }
 
-    const reportArguments = t.context.report.firstCall.args;
+    const reportArguments = ruleContext.report.firstCall.args;
 
     t.is(reportArguments[2], report.message);
     t.deepEqual(reportArguments[3], report.position);

src/tests/helpers/rule-test-type.ts

@@ -1,4 +1,5 @@
 import { ProblemLocation } from '../../lib/types';
+import { NetworkData } from '../../lib/types';
 
 /** An event to fire while testing rules */
 export interface TestEvent {
@@ -9,7 +10,9 @@ export interface TestEvent {
      */
     name: string,
     /** The path to the fixture to use when sending the event */
-    fixture: string
+    fixture: string,
+    /** The response data that should be returned */
+    responses?: Array<NetworkData>
 }
 
 export interface Report {

src/tests/lib/rules/manifest-exists/fixtures/manifest-specified-and-file-does-exist.html

@@ -0,0 +1,10 @@
+<!doctype html>
+<html lang="en">
+    <head>
+        <title>test</title>
+        <link rel="manifest" href="site.webmanifest">
+    </head>
+    <body>
+
+    </body>
+</html>

src/tests/lib/rules/manifest-exists/fixtures/manifest-specified-and-file-does-not-exist.html

@@ -0,0 +1,10 @@
+<!doctype html>
+<html lang="en">
+    <head>
+        <title>test</title>
+        <link rel="manifest" href="does-not-exist.webmanifest">
+    </head>
+    <body>
+
+    </body>
+</html>

src/tests/lib/rules/manifest-exists/fixtures/manifest-specified-as-full-url.html

@@ -0,0 +1,10 @@
+<!doctype html>
+<html lang="en">
+    <head>
+        <title>test</title>
+        <link rel="manifest" href="https://example.com/site.webmanifest">
+    </head>
+    <body>
+
+    </body>
+</html>