Refactor (another one) #47

RReverser · 2018-08-15T11:20:00Z

Primary changes:

Upgraded dependencies (in particular, those with vulnerabilities).
Fixed code to match new xo lints after upgrade (const and let instead of var, arrow functions where possible, destructuring etc.).
Autoformatted code with Prettier (now an option in xo).
Added automatic lint and formatting of staged files on commit with husky.
Replaced Bluebird with native async functions
Added filenames to UglifyJS and PostCSS invocations for better error reporting (previously URL wasn't reported so it was hard to figure out where error occurs).
Added helper for consistent URL parsing in CSS (previously URLs for imports and those in properties were parsed differently, one with custom regex and another with PostCSS value parser).
Preserving any data URIs and not just those encoded as base64.
Added reporting of UglifyJS warnings (sometimes useful for debugging, corresponds to warning level if set via CLI).
Simplified external collapser functions to avoid repeating URLs and log message code.
Switched to rely on Content-Type provided by the server instead of trying to guess it with mmmagic - the latter previously led to various detection bugs even when server served the image correctly, and generally guessing mime types is more dangerous (as seen in history of X-Content-Type-Options: nosniff).
Added printing of final HTML when collapsing with a CLI tool so that it can be saved for inspection.
Switched logs to process.stderr instead of process.stdout so that they don't get mixed with the content.
Added graceful recovery from UglifyJS errors (just because UglifyJS can't parse something, it doesn't mean that that JS is necessary invalid, and so it's better to log it but move on with collapsing).
Improved HTTP resiliency by adding limit to parallel requests (for now hardcoded to 8), request retries (hardcoded to 5) and HTTP cache to avoid repeated fetches.
Switched to parse5-html-rewriting-stream for HTML parsing. PostHTML uses htmlparser2 which has worse compliance with spec, and, in particular, doesn't decode/encode text and attributes, which currently means 1) collapsify would try to use raw attributes for validation against forbidden regex, but then decode them before sending request (this might lead to fetching forbidden resources) and 2) when constructing data-URI, potentially invalid attribute characters would not get escaped (this might lead to XSS on collapsed pages). Also this allows to process HTML in a streaming fashion, modifying only the parts we're actually interested in.

Using https://github.com/sgilroy/async-await-codemod to upgrade codebase.

It's now required for correct URL resolution.

- Add limit of maximum 8 parallel requests. - Add 5 retries in case of request failure. - Add cache to use while fetching the page.

parse5 has much better compliance with spec than htmlparser2 used in PostHTML, performs attribute decoding as per spec and allows to granularly rewrite only parts of HTML we're interesting in while preserving the surrounding formatting.

terinjokes · 2018-08-20T23:53:59Z

lib/collapsers/binary.js

 }

-function collapse(buf) {
-  return base64Utils.encode(buf);
+function collapse(body, opts) {


All of the collapse functions should be asynchronous for consistency.

We anyway await them, which supports both sync and async versions with no differences, but yeah, I guess I could do it.

terinjokes · 2018-08-20T23:57:17Z

lib/collapsers/html.js

-        .use(require('../plugins/posthtml-flatten-style')(opts))
-        .use(require('../plugins/posthtml-flatten-script')(opts))
-        .process(String(buf));
+function collapse(body, opts) {


This returns a Promise, but it's not entirely clear right now. Mind also making this async?

terinjokes · 2018-08-21T00:02:26Z

lib/collapsers/binary.js

-
-  return opts.fetch(url).then(collapse);
+async function external({fetch, resourceLocation: url}) {
+  const {contentType, body} = await fetch(url);


Servers don't always return content-types browsers accept for dataURIs. It might be better to prefer a list for common file extensions, and fall back to the content-type if unknown.

No, I definitely don't want to "guess" content-type again, it's a very insecure practice, and there is a reason browsers don't do that. We shouldn't change how it treats the content just by server-side extensions - if real browser wouldn't show the image on original HTML because of incorrect Content-Type, it shouldn't do that on the resulting HTML either.

Servers don't always return content-types browsers accept for dataURIs.

Can you provide an example of content-type that would work on a network response but not dataURI (after removing spaces)? From what I've seen, anything should work; moreover, as per spec, it's optional and even data URIs without any content-type like data:,123 are perfectly valid.

I'm definitely not saying we should guess, that was a mistake that was going to be the first thing I removed if I ever touched this again.

I'll have to dig through my archives to see if I can find the cases I found before.

terinjokes · 2018-08-21T00:03:45Z

test/plugins/parse5-flatten-image.js

@@ -0,0 +1,46 @@
+'use strict';


I'd love to eventually see this with snapshot tests instead.

Definitely not part of this PR :)

This was intended as "nitpick"/":heart:"

terinjokes · 2018-08-21T00:07:25Z

bin/cli.js

+        digits: 2
+      })
+    );
+    process.stdout.write(output);


This was avoided because the output can get rather large. If you must, accept a flag to enable.

I usually just redirect output to a file, but yeah, flag sounds good to me.

terinjokes · 2018-08-21T00:09:36Z

lib/plugins/parse5-flatten-script.js

@@ -0,0 +1,76 @@
+'use strict';
+const {resolve} = require('url');
+const logger = require('bole')('posthtml-flatten-script');


Your logger name isn't matching the rest of collapsify.

That name was in posthtml-flatten-script and was just moved over here, but I agree it should've been consistent (or at least renamed).

Fixed (decided to log to collapsers:html group).

terinjokes · 2018-08-21T00:10:28Z

lib/utils/httpclient.js

-    url = he.decode(url);
+  const client = got.extend({
+    headers: {
+      'user-agent': `Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10; rv:33.0) Gecko/20100101 Firefox/33.0 Collapsify/${VERSION} node/${


nitpick: Firefox 33 is old now. Might be worth updating.

RReverser · 2018-08-22T10:52:04Z

Resolved comments (except for data URI) and bumped package version.

We can just read from package.json directly.

terinjokes · 2018-08-22T15:25:11Z

package.json

@@ -11,7 +11,8 @@
  "contributors": [
    "Christopher Joel <chris@scriptolo.gy> (http://scriptolo.gy)",
    "Terin Stock <terinjokes@gmail.com> (http://terinstock.com)",
-    "Andrew Galloni <andrew@cloudflare.com>"
+    "Andrew Galloni <andrew@cloudflare.com>",
+    "Ingvar Stepanyan <me@rreverser.com> (https://rreverser.com)"


I'm so sorry.

RReverser added 30 commits August 15, 2018 11:45

npm audit fix

3058fad

npm audit fix --force

4ec9af1

npm update

ab33108

Run xo --fix to autofix new lints

2d36085

Autoformat code with Prettier

5fbf368

Construct options object from argv

2ccc128

Fix various lint errors manually

6d4ba0c

Add automatic xo --fix on commit

a36275d

Upgrade deps where possible w/o breaking

6b2c623

Upgrade postcss, fix tests

0f38fe7

Upgrade cssnano, fix tests

517d11a

Upgrade uglify-js, fix tests

c4a43e8

Update husky and istanbul to RC versions

8938ea1

Auto-rewrite .then(...) to async-await

f57ddab

Using https://github.com/sgilroy/async-await-codemod to upgrade codebase.

Bluebird.all -> Promise.all

402b053

Bluebird.resolve -> Promise.resolve

0490977

Bluebird.reject -> Promise.reject

4ae8fe9

Bluebird.promisify -> util.promisify

bb899a5

Bluebird.try -> async functions

07293b0

Remove Bluebird as no longer necessary

77be7f6

Add from option to PostCSS invocation

101a739

It's now required for correct URL resolution.

Consistently parse URLs in CSS

ab0cefb

Detect any data URIs, not just base-64

31e6aec

Replace .match with .startsWith if can

6d3172a

Promise.{resolve, reject} -> async fn

df9d6c7

Add UglifyJS warning reporting

1d0eb0a

Avoid duplication of URL in .external calls

e9f6c2d

.test -> .includes

8edb411

Use Content-Type instead of mmmagic

c5572ef

Fix leftover CLI bugs; print HTML

952d67a

RReverser added 5 commits August 15, 2018 12:06

Centralise "fetching" log messages

6f5dd89

Improve HTTP resiliency

08485d0

- Add limit of maximum 8 parallel requests. - Add 5 retries in case of request failure. - Add cache to use while fetching the page.

Switch to parse5 for HTML processing

c1bdb32

parse5 has much better compliance with spec than htmlparser2 used in PostHTML, performs attribute decoding as per spec and allows to granularly rewrite only parts of HTML we're interesting in while preserving the surrounding formatting.

Extract HTML rewriter to utils

80fc65d

Removed Node.js 8.x from Travis

8869297

RReverser requested review from terinjokes, inikulin and dotjs August 15, 2018 11:32

dotjs approved these changes Aug 15, 2018

View reviewed changes

terinjokes suggested changes Aug 21, 2018

View reviewed changes

RReverser added 4 commits August 22, 2018 11:40

Make all collapse funcs async for consistency

196db23

Log script flattening to collapsers:html group

a05cc03

Update User-Agent to FF 60.0 ESR

3f8ab6b

Bump package version

060aa52

This comment has been minimized.

Sign in to view

RReverser added 4 commits August 22, 2018 12:15

Remove versionify script

f48536c

We can just read from package.json directly.

Add tmp folder to gitignore

e17eaf9

Fix VERSION echo in Makefile

1f79a07

Use Node for reading version from JSON

f03dbb2

RReverser force-pushed the ingvar/refactor branch from 3abf72d to f03dbb2 Compare August 22, 2018 11:15

RReverser added 6 commits August 22, 2018 12:18

Make CLI output HTML to a given path

60338bd

Add myself to contributors

4ce6baf

Fix import order

46e0275

Upgrade build to work with modern cfsetup

338e32e

Switch to npm ci for building/testing

95a38c0

Fix fpm output path

e976ac0

terinjokes approved these changes Aug 22, 2018

View reviewed changes

terinjokes reviewed Aug 22, 2018

View reviewed changes

RReverser merged commit d0c515a into cloudflare:master Aug 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor (another one) #47

Refactor (another one) #47

RReverser commented Aug 15, 2018

terinjokes Aug 20, 2018

RReverser Aug 22, 2018

RReverser Aug 22, 2018

terinjokes Aug 20, 2018

RReverser Aug 22, 2018

RReverser Aug 22, 2018

terinjokes Aug 21, 2018

RReverser Aug 22, 2018

terinjokes Aug 22, 2018

terinjokes Aug 21, 2018

RReverser Aug 22, 2018

terinjokes Aug 22, 2018

terinjokes Aug 21, 2018

RReverser Aug 22, 2018

RReverser Aug 22, 2018

terinjokes Aug 21, 2018

RReverser Aug 22, 2018 •

edited

RReverser Aug 22, 2018

terinjokes Aug 21, 2018

RReverser commented Aug 22, 2018

This comment has been minimized.

terinjokes Aug 22, 2018

Refactor (another one) #47

Refactor (another one) #47

Conversation

RReverser commented Aug 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RReverser Aug 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RReverser commented Aug 22, 2018

This comment has been minimized.

Choose a reason for hiding this comment

RReverser Aug 22, 2018 •

edited