From 0f08c85938a670bd6424874906a8a647a0c6161e Mon Sep 17 00:00:00 2001 From: Rodrigo Fernandes Date: Tue, 26 Nov 2019 09:44:09 +0000 Subject: [PATCH] refactor: Unify escaping --- src/__tests__/diff2html-tests.ts | 2 +- src/__tests__/printer-utils-tests.ts | 59 +++++++--- src/__tests__/utils-tests.ts | 57 ++------- src/line-by-line-renderer.ts | 166 +++++++++++++-------------- src/render-utils.ts | 28 +++-- src/side-by-side-renderer.ts | 62 +++++----- src/utils.ts | 14 --- 7 files changed, 188 insertions(+), 200 deletions(-) diff --git a/src/__tests__/diff2html-tests.ts b/src/__tests__/diff2html-tests.ts index d53af91..6cdc6e7 100644 --- a/src/__tests__/diff2html-tests.ts +++ b/src/__tests__/diff2html-tests.ts @@ -299,7 +299,7 @@ describe("Diff2Html", () => { ' $a="
\n' + " $a=\"
- 1.1.9: Fix around ubuntu's inability to cache promises. [#877](https://github.com/FredrikNoren/ungit/pull/878)\n" + " - 1.1.8:\n" + - "@@ -11,7 +10,7 @@ $a=\"
- 1.1.9: Fix around ubuntu's inability to cache promises. [#8\n" + + "@@ -11,7 +10,7 @@ $a="<table><tr><td>- 1.1.9: Fix around ubuntu's inability to cache promises. [#8\n" + " - 1.1.7:\n" + " - Fix diff flickering issue and optimization [#865](https://github.com/FredrikNoren/ungit/pull/865)\n" + " - Fix credential dialog issue [#864](https://github.com/FredrikNoren/ungit/pull/864)\n" + diff --git a/src/__tests__/printer-utils-tests.ts b/src/__tests__/printer-utils-tests.ts index 04e8102..6452beb 100644 --- a/src/__tests__/printer-utils-tests.ts +++ b/src/__tests__/printer-utils-tests.ts @@ -1,17 +1,50 @@ -import * as renderUtils from "../render-utils"; +import { escapeForHtml, getHtmlId, filenameDiff, diffHighlight } from "../render-utils"; import { DiffStyleType, LineMatchingType } from "../types"; describe("Utils", () => { + describe("escapeForHtml", () => { + it("should escape & with &", () => { + const result = escapeForHtml("&"); + expect(result).toEqual("&"); + }); + it("should escape < with <", () => { + const result = escapeForHtml("<"); + expect(result).toEqual("<"); + }); + it("should escape > with >", () => { + const result = escapeForHtml(">"); + expect(result).toEqual(">"); + }); + it('should escape " with "', () => { + const result = escapeForHtml('"'); + expect(result).toEqual("""); + }); + it("should escape ' with '", () => { + const result = escapeForHtml("'"); + expect(result).toEqual("'"); + }); + it("should escape / with /", () => { + const result = escapeForHtml("/"); + expect(result).toEqual("/"); + }); + it("should escape a string containing HTML code", () => { + const result = escapeForHtml(`Search 'Diff2Html'`); + expect(result).toEqual( + "<a href="/search?q=diff2html">Search 'Diff2Html'</a>" + ); + }); + }); + describe("getHtmlId", () => { it("should generate file unique id", () => { - const result = renderUtils.getHtmlId({ + const result = getHtmlId({ oldName: "sample.js", newName: "sample.js" }); expect("d2h-960013").toEqual(result); }); it("should generate file unique id for empty hashes", () => { - const result = renderUtils.getHtmlId({ + const result = getHtmlId({ oldName: "sample.js", newName: "sample.js" }); @@ -21,49 +54,49 @@ describe("Utils", () => { describe("getDiffName", () => { it("should generate the file name for a changed file", () => { - const result = renderUtils.filenameDiff({ + const result = filenameDiff({ oldName: "sample.js", newName: "sample.js" }); expect("sample.js").toEqual(result); }); it("should generate the file name for a changed file and full rename", () => { - const result = renderUtils.filenameDiff({ + const result = filenameDiff({ oldName: "sample1.js", newName: "sample2.js" }); expect("sample1.js → sample2.js").toEqual(result); }); it("should generate the file name for a changed file and prefix rename", () => { - const result = renderUtils.filenameDiff({ + const result = filenameDiff({ oldName: "src/path/sample.js", newName: "source/path/sample.js" }); expect("{src → source}/path/sample.js").toEqual(result); }); it("should generate the file name for a changed file and suffix rename", () => { - const result = renderUtils.filenameDiff({ + const result = filenameDiff({ oldName: "src/path/sample1.js", newName: "src/path/sample2.js" }); expect("src/path/{sample1.js → sample2.js}").toEqual(result); }); it("should generate the file name for a changed file and middle rename", () => { - const result = renderUtils.filenameDiff({ + const result = filenameDiff({ oldName: "src/really/big/path/sample.js", newName: "src/small/path/sample.js" }); expect("src/{really/big → small}/path/sample.js").toEqual(result); }); it("should generate the file name for a deleted file", () => { - const result = renderUtils.filenameDiff({ + const result = filenameDiff({ oldName: "src/my/file.js", newName: "/dev/null" }); expect("src/my/file.js").toEqual(result); }); it("should generate the file name for a new file", () => { - const result = renderUtils.filenameDiff({ + const result = filenameDiff({ oldName: "/dev/null", newName: "src/my/file.js" }); @@ -73,7 +106,7 @@ describe("Utils", () => { describe("diffHighlight", () => { it("should highlight two lines", () => { - const result = renderUtils.diffHighlight("-var myVar = 2;", "+var myVariable = 3;", false, { + const result = diffHighlight("-var myVar = 2;", "+var myVariable = 3;", false, { matching: LineMatchingType.WORDS }); @@ -89,7 +122,7 @@ describe("Utils", () => { }); }); it("should highlight two lines char by char", () => { - const result = renderUtils.diffHighlight("-var myVar = 2;", "+var myVariable = 3;", false, { + const result = diffHighlight("-var myVar = 2;", "+var myVariable = 3;", false, { diffStyle: DiffStyleType.CHAR }); @@ -105,7 +138,7 @@ describe("Utils", () => { }).toEqual(result); }); it("should highlight combined diff lines", () => { - const result = renderUtils.diffHighlight(" -var myVar = 2;", " +var myVariable = 3;", true, { + const result = diffHighlight(" -var myVar = 2;", " +var myVariable = 3;", true, { diffStyle: DiffStyleType.WORD, matching: LineMatchingType.WORDS, matchWordsThreshold: 1.0 diff --git a/src/__tests__/utils-tests.ts b/src/__tests__/utils-tests.ts index 4548cf6..9023c26 100644 --- a/src/__tests__/utils-tests.ts +++ b/src/__tests__/utils-tests.ts @@ -1,4 +1,4 @@ -import { escapeForRegExp, escapeForHtml, unifyPath, hashCode } from "../utils"; +import { escapeForRegExp, unifyPath, hashCode } from "../utils"; describe("Utils", () => { describe("escapeForRegExp", () => { @@ -12,53 +12,20 @@ describe("Utils", () => { }); }); - describe("escapeForHtml", () => { - it("should escape & with &", () => { - const result = escapeForHtml("&"); - expect(result).toEqual("&"); - }); - it("should escape < with <", () => { - const result = escapeForHtml("<"); - expect(result).toEqual("<"); - }); - it("should escape > with >", () => { - const result = escapeForHtml(">"); - expect(result).toEqual(">"); - }); - it('should escape " with "', () => { - const result = escapeForHtml('"'); - expect(result).toEqual("""); - }); - it("should escape ' with '", () => { - const result = escapeForHtml("'"); - expect(result).toEqual("'"); - }); - it("should escape / with /", () => { - const result = escapeForHtml("/"); - expect(result).toEqual("/"); - }); - it("should escape a string containing HTML code", () => { - const result = escapeForHtml(`Search 'Diff2Html'`); - expect(result).toEqual( - "<a href="/search?q=diff2html">Search 'Diff2Html'</a>" - ); + describe("unifyPath", () => { + it("should unify windows style path", () => { + const result = unifyPath("\\Users\\Downloads\\diff.html"); + expect(result).toEqual("/Users/Downloads/diff.html"); }); + }); - describe("unifyPath", () => { - it("should unify windows style path", () => { - const result = unifyPath("\\Users\\Downloads\\diff.html"); - expect(result).toEqual("/Users/Downloads/diff.html"); - }); + describe("hashCode", () => { + it("should create consistent hash for a text piece", () => { + const string = "/home/diff2html/diff.html"; + expect(hashCode(string)).toEqual(hashCode(string)); }); - - describe("hashCode", () => { - it("should create consistent hash for a text piece", () => { - const string = "/home/diff2html/diff.html"; - expect(hashCode(string)).toEqual(hashCode(string)); - }); - it("should create different hash for different text pieces", () => { - expect(hashCode("/home/diff2html/diff1.html")).not.toEqual(hashCode("/home/diff2html/diff2.html")); - }); + it("should create different hash for different text pieces", () => { + expect(hashCode("/home/diff2html/diff1.html")).not.toEqual(hashCode("/home/diff2html/diff2.html")); }); }); }); diff --git a/src/line-by-line-renderer.ts b/src/line-by-line-renderer.ts index 685b90b..307494d 100644 --- a/src/line-by-line-renderer.ts +++ b/src/line-by-line-renderer.ts @@ -1,8 +1,7 @@ -import * as utils from "./utils"; import HoganJsUtils from "./hoganjs-utils"; import * as Rematch from "./rematch"; import * as renderUtils from "./render-utils"; -import { DiffFile, DiffBlock, DiffLine, LineType } from "./types"; +import { DiffFile, DiffLine, LineType } from "./types"; export interface LineByLineRendererConfig extends renderUtils.RenderConfig { renderNothingWhenEmpty?: boolean; @@ -72,53 +71,6 @@ export default class LineByLineRenderer { }); } - // TODO: Make this private after improving tests - makeColumnLineNumberHtml(block: DiffBlock): string { - return this.hoganUtils.render(genericTemplatesPath, "column-line-number", { - CSSLineClass: renderUtils.CSSLineClass, - blockHeader: utils.escapeForHtml(block.header), - lineClass: "d2h-code-linenumber", - contentClass: "d2h-code-line" - }); - } - - // TODO: Make this private after improving tests - makeLineHtml( - isCombined: boolean, - type: renderUtils.CSSLineClass, - content: string, - oldNumber?: number, - newNumber?: number, - possiblePrefix?: string - ): string { - const lineNumberTemplate = this.hoganUtils.render(baseTemplatesPath, "numbers", { - oldNumber: oldNumber || "", - newNumber: newNumber || "" - }); - - let lineWithoutPrefix = content; - let prefix = possiblePrefix; - - if (!prefix) { - const lineWithPrefix = renderUtils.deconstructLine(content, isCombined); - prefix = lineWithPrefix.prefix; - lineWithoutPrefix = lineWithPrefix.content; - } - - if (prefix === " ") { - prefix = " "; - } - - return this.hoganUtils.render(genericTemplatesPath, "line", { - type: type, - lineClass: "d2h-code-linenumber", - contentClass: "d2h-code-line", - prefix: prefix, - content: lineWithoutPrefix, - lineNumber: lineNumberTemplate - }); - } - // TODO: Make this private after improving tests generateEmptyDiff(): string { return this.hoganUtils.render(genericTemplatesPath, "empty-diff", { @@ -127,47 +79,20 @@ export default class LineByLineRenderer { }); } - // TODO: Make this private after improving tests - processLines(isCombined: boolean, oldLines: DiffLine[], newLines: DiffLine[]): string { - let lines = ""; - - for (let i = 0; i < oldLines.length; i++) { - const oldLine = oldLines[i]; - const oldEscapedLine = utils.escapeForHtml(oldLine.content); - lines += this.makeLineHtml( - isCombined, - renderUtils.toCSSClass(oldLine.type), - oldEscapedLine, - oldLine.oldNumber, - oldLine.newNumber - ); - } - - for (let j = 0; j < newLines.length; j++) { - const newLine = newLines[j]; - const newEscapedLine = utils.escapeForHtml(newLine.content); - lines += this.makeLineHtml( - isCombined, - renderUtils.toCSSClass(newLine.type), - newEscapedLine, - newLine.oldNumber, - newLine.newNumber - ); - } - - return lines; - } - // TODO: Make this private after improving tests generateFileHtml(file: DiffFile): string { - const distance = Rematch.newDistanceFn( - (e: DiffLine) => renderUtils.deconstructLine(e.content, file.isCombined).content + const matcher = Rematch.newMatcherFn( + Rematch.newDistanceFn((e: DiffLine) => renderUtils.deconstructLine(e.content, file.isCombined).content) ); - const matcher = Rematch.newMatcherFn(distance); return file.blocks .map(block => { - let lines = this.makeColumnLineNumberHtml(block); + let lines = this.hoganUtils.render(genericTemplatesPath, "column-line-number", { + CSSLineClass: renderUtils.CSSLineClass, + blockHeader: block.header, + lineClass: "d2h-code-linenumber", + contentClass: "d2h-code-line" + }); let oldLines: DiffLine[] = []; let newLines: DiffLine[] = []; @@ -242,8 +167,7 @@ export default class LineByLineRenderer { for (let i = 0; i < block.lines.length; i++) { const diffLine = block.lines[i]; - const { prefix, content: line } = renderUtils.deconstructLine(diffLine.content, file.isCombined); - const escapedLine = utils.escapeForHtml(line); + const { prefix, content } = renderUtils.deconstructLine(diffLine.content, file.isCombined); if ( diffLine.type !== LineType.INSERT && @@ -256,7 +180,7 @@ export default class LineByLineRenderer { lines += this.makeLineHtml( file.isCombined, renderUtils.toCSSClass(diffLine.type), - escapedLine, + content, diffLine.oldNumber, diffLine.newNumber, prefix @@ -265,7 +189,7 @@ export default class LineByLineRenderer { lines += this.makeLineHtml( file.isCombined, renderUtils.toCSSClass(diffLine.type), - escapedLine, + content, diffLine.oldNumber, diffLine.newNumber, prefix @@ -286,4 +210,70 @@ export default class LineByLineRenderer { }) .join("\n"); } + + // TODO: Make this private after improving tests + makeLineHtml( + isCombined: boolean, + type: renderUtils.CSSLineClass, + content: string, + oldNumber?: number, + newNumber?: number, + possiblePrefix?: string + ): string { + const lineNumberTemplate = this.hoganUtils.render(baseTemplatesPath, "numbers", { + oldNumber: oldNumber || "", + newNumber: newNumber || "" + }); + + let lineWithoutPrefix = content; + let prefix = possiblePrefix; + + if (!prefix) { + const lineWithPrefix = renderUtils.deconstructLine(content, isCombined); + prefix = lineWithPrefix.prefix; + lineWithoutPrefix = lineWithPrefix.content; + } + + if (prefix === " ") { + prefix = " "; + } + + return this.hoganUtils.render(genericTemplatesPath, "line", { + type: type, + lineClass: "d2h-code-linenumber", + contentClass: "d2h-code-line", + prefix: prefix, + content: lineWithoutPrefix, + lineNumber: lineNumberTemplate + }); + } + + // TODO: Make this private after improving tests + processLines(isCombined: boolean, oldLines: DiffLine[], newLines: DiffLine[]): string { + let lines = ""; + + for (let i = 0; i < oldLines.length; i++) { + const oldLine = oldLines[i]; + lines += this.makeLineHtml( + isCombined, + renderUtils.toCSSClass(oldLine.type), + oldLine.content, + oldLine.oldNumber, + oldLine.newNumber + ); + } + + for (let j = 0; j < newLines.length; j++) { + const newLine = newLines[j]; + lines += this.makeLineHtml( + isCombined, + renderUtils.toCSSClass(newLine.type), + newLine.content, + newLine.oldNumber, + newLine.newNumber + ); + } + + return lines; + } } diff --git a/src/render-utils.ts b/src/render-utils.ts index b823fbe..740f7b2 100644 --- a/src/render-utils.ts +++ b/src/render-utils.ts @@ -1,6 +1,6 @@ import * as jsDiff from "diff"; -import { unifyPath, escapeForHtml, hashCode } from "./utils"; +import { unifyPath, hashCode } from "./utils"; import * as rematch from "./rematch"; import { LineMatchingType, DiffStyleType, LineType, DiffLineParts, DiffFile, DiffFileName } from "./types"; @@ -75,6 +75,21 @@ function prefixLength(isCombined: boolean): number { return isCombined ? 2 : 1; } +/** + * Escapes all required characters for safe HTML rendering + */ +// TODO: Test this method inside deconstructLine since it should not be used anywhere else +export function escapeForHtml(str: string): string { + return str + .slice(0) + .replace(/&/g, "&") + .replace(//g, ">") + .replace(/"/g, """) + .replace(/'/g, "'") + .replace(/\//g, "/"); +} + /** * Deconstructs diff @line by separating the content from the prefix type */ @@ -82,7 +97,7 @@ export function deconstructLine(line: string, isCombined: boolean): DiffLinePart const indexToSplit = prefixLength(isCombined); return { prefix: line.substring(0, indexToSplit), - content: line.substring(indexToSplit) + content: escapeForHtml(line.substring(indexToSplit)) }; } @@ -209,11 +224,11 @@ export function diffHighlight( return { oldLine: { prefix: line1.prefix, - content: escapeForHtml(line1.content) + content: line1.content }, newLine: { prefix: line2.prefix, - content: escapeForHtml(line2.content) + content: line2.content } }; } @@ -248,11 +263,10 @@ export function diffHighlight( const highlightedLine = diff.reduce((highlightedLine, part) => { const elemType = part.added ? "ins" : part.removed ? "del" : null; const addClass = changedWords.indexOf(part) > -1 ? ' class="d2h-change"' : ""; - const escapedValue = escapeForHtml(part.value); return elemType !== null - ? `${highlightedLine}<${elemType}${addClass}>${escapedValue}` - : `${highlightedLine}${escapedValue}`; + ? `${highlightedLine}<${elemType}${addClass}>${part.value}` + : `${highlightedLine}${part.value}`; }, ""); return { diff --git a/src/side-by-side-renderer.ts b/src/side-by-side-renderer.ts index 1149c14..3766da7 100644 --- a/src/side-by-side-renderer.ts +++ b/src/side-by-side-renderer.ts @@ -1,4 +1,3 @@ -import * as utils from "./utils"; import HoganJsUtils from "./hoganjs-utils"; import * as Rematch from "./rematch"; import * as renderUtils from "./render-utils"; @@ -47,20 +46,10 @@ export default class SideBySideRenderer { return this.hoganUtils.render(genericTemplatesPath, "wrapper", { content: diffsHtml }); } - // TODO: Make this private after improving tests - generateEmptyDiff(): FileHtml { - return { - right: "", - left: - this.hoganUtils.render(genericTemplatesPath, "empty-diff", { - contentClass: "d2h-code-side-line", - CSSLineClass: renderUtils.CSSLineClass - }) || "" - }; - } - // TODO: Make this private after improving tests makeFileDiffHtml(file: DiffFile, diffs: FileHtml): string { + if (this.config.renderNothingWhenEmpty && Array.isArray(file.blocks) && file.blocks.length === 0) return ""; + const fileDiffTemplate = this.hoganUtils.template(baseTemplatesPath, "file-diff"); const filePathTemplate = this.hoganUtils.template(genericTemplatesPath, "file-path"); const fileIconTemplate = this.hoganUtils.template(iconsBaseTemplatesPath, "file"); @@ -83,21 +72,21 @@ export default class SideBySideRenderer { } // TODO: Make this private after improving tests - makeSideHtml(blockHeader: string): string { - return this.hoganUtils.render(genericTemplatesPath, "column-line-number", { - CSSLineClass: renderUtils.CSSLineClass, - blockHeader: utils.escapeForHtml(blockHeader), - lineClass: "d2h-code-side-linenumber", - contentClass: "d2h-code-side-line" - }); + generateEmptyDiff(): FileHtml { + return { + right: "", + left: this.hoganUtils.render(genericTemplatesPath, "empty-diff", { + contentClass: "d2h-code-side-line", + CSSLineClass: renderUtils.CSSLineClass + }) + }; } // TODO: Make this private after improving tests generateSideBySideFileHtml(file: DiffFile): FileHtml { - const distance = Rematch.newDistanceFn( - (e: DiffLine) => renderUtils.deconstructLine(e.content, file.isCombined).content + const matcher = Rematch.newMatcherFn( + Rematch.newDistanceFn((e: DiffLine) => renderUtils.deconstructLine(e.content, file.isCombined).content) ); - const matcher = Rematch.newMatcherFn(distance); const fileHtml = { right: "", @@ -183,8 +172,7 @@ export default class SideBySideRenderer { for (let i = 0; i < block.lines.length; i++) { const diffLine = block.lines[i]; - const { prefix, content: line } = renderUtils.deconstructLine(diffLine.content, file.isCombined); - const escapedLine = utils.escapeForHtml(line); + const { prefix, content } = renderUtils.deconstructLine(diffLine.content, file.isCombined); if ( diffLine.type !== LineType.INSERT && @@ -197,14 +185,14 @@ export default class SideBySideRenderer { fileHtml.left += this.generateSingleLineHtml( file.isCombined, renderUtils.toCSSClass(diffLine.type), - escapedLine, + content, diffLine.oldNumber, prefix ); fileHtml.right += this.generateSingleLineHtml( file.isCombined, renderUtils.toCSSClass(diffLine.type), - escapedLine, + content, diffLine.newNumber, prefix ); @@ -213,7 +201,7 @@ export default class SideBySideRenderer { fileHtml.right += this.generateSingleLineHtml( file.isCombined, renderUtils.toCSSClass(diffLine.type), - escapedLine, + content, diffLine.newNumber, prefix ); @@ -233,6 +221,16 @@ export default class SideBySideRenderer { return fileHtml; } + // TODO: Make this private after improving tests + makeSideHtml(blockHeader: string): string { + return this.hoganUtils.render(genericTemplatesPath, "column-line-number", { + CSSLineClass: renderUtils.CSSLineClass, + blockHeader: blockHeader, + lineClass: "d2h-code-side-linenumber", + contentClass: "d2h-code-side-line" + }); + } + // TODO: Make this private after improving tests processLines(isCombined: boolean, oldLines: DiffLine[], newLines: DiffLine[]): FileHtml { const fileHtml = { @@ -251,8 +249,8 @@ export default class SideBySideRenderer { let newPrefix; if (oldLine) { - const { prefix, content: line } = renderUtils.deconstructLine(oldLine.content, isCombined); - oldContent = utils.escapeForHtml(line); + const { prefix, content } = renderUtils.deconstructLine(oldLine.content, isCombined); + oldContent = content; oldPrefix = prefix; } else { oldContent = ""; @@ -260,8 +258,8 @@ export default class SideBySideRenderer { } if (newLine) { - const { prefix, content: line } = renderUtils.deconstructLine(newLine.content, isCombined); - newContent = utils.escapeForHtml(line); + const { prefix, content } = renderUtils.deconstructLine(newLine.content, isCombined); + newContent = content; newPrefix = prefix; } else { newContent = ""; diff --git a/src/utils.ts b/src/utils.ts index 986bb72..649f23a 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -30,20 +30,6 @@ export function escapeForRegExp(str: string): string { return str.replace(regex, "\\$&"); } -/** - * Escapes all required characters for safe HTML rendering - */ -export function escapeForHtml(str: string): string { - return str - .slice(0) - .replace(/&/g, "&") - .replace(//g, ">") - .replace(/"/g, """) - .replace(/'/g, "'") - .replace(/\//g, "/"); -} - /** * Converts all '\' in @path to unix style '/' */