Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEATURE] Properties File Escaping #214

Merged
merged 10 commits into from
Jul 29, 2019
11 changes: 10 additions & 1 deletion lib/middleware/MiddlewareManager.js
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,16 @@ class MiddlewareManager {
await this.addMiddleware("discovery", {
mountPath: "/discovery"
});
await this.addMiddleware("serveResources");
await this.addMiddleware("serveResources", {
wrapperCallback: (serveResourcesModule) => {
return ({resources}) => {
return serveResourcesModule({
resources,
tree: this.tree
});
};
}
});
await this.addMiddleware("serveThemes");
await this.addMiddleware("versionInfo", {
mountPath: "/resources/sap-ui-version.json",
Expand Down
8 changes: 1 addition & 7 deletions lib/middleware/serveIndex.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@ const log = require("@ui5/logger").getLogger("server:middleware:serveIndex");
const mime = require("mime-types");
const parseurl = require("parseurl");

const rProperties = /\.properties$/;

const KB = 1024;
const MB = KB * KB;
const GB = KB * KB * KB;
Expand All @@ -15,11 +13,7 @@ const GB = KB * KB * KB;
* @returns {string} mime type
*/
function getMimeType(resource) {
if (rProperties.test(resource.getPath())) {
return "text/plain;charset=ISO-8859-1";
} else {
return mime.lookup(resource.getPath()) || "application/octet-stream";
}
return mime.lookup(resource.getPath()) || "application/octet-stream";
}

/**
Expand Down
38 changes: 19 additions & 19 deletions lib/middleware/serveResources.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,11 @@ function isFresh(req, res) {
* @param {module:@ui5/fs.AbstractReader} resources.all Resource collection which contains the workspace and the project dependencies
* @returns {Function} Returns a server middleware closure.
*/
function createMiddleware({resources}) {
return function serveResources(req, res, next) {
const pathname = parseurl(req).pathname;
resources.all.byPath(pathname).then(function(resource) {
function createMiddleware({resources, tree: project}) {
return async function serveResources(req, res, next) {
try {
const pathname = parseurl(req).pathname;
const resource = await resources.all.byPath(pathname);
if (!resource) { // Not found
next();
return;
Expand All @@ -36,22 +37,21 @@ function createMiddleware({resources}) {
log.verbose("\n" + treeify.asTree(resource.getPathTree()));
}

let type;
let charset;
const resourcePath = resource.getPath();
const type = mime.lookup(resourcePath) || "application/octet-stream";
const charset = mime.charset(type);
if (rProperties.test(resourcePath)) {
// Special handling for *.properties files which are encoded with charset ISO-8859-1.
type = "text/plain";
charset = "ISO-8859-1";
} else {
type = mime.lookup(resourcePath) || "application/octet-stream";
// Special handling for *.properties files escape non ascii characters.
const nonAsciiEscaper = require("@ui5/builder").processors.nonAsciiEscaper;
const propertiesFileSourceEncoding = project && project.resources && project.resources.configuration && project.resources.configuration.propertiesFileSourceEncoding;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we missed something here 🤔

project is the root project. However, resources are taken from resources.all. So we rather need to look for the project on the resource. Just like we do in the lbt Bundler

We also need to add a test for this: A dependency tree of two projects, one with UTF-8 encoding and another one with ISO-8859-1, making sure that both are being processed correctly (stubs should be sufficient, no actual comparison of the results)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be resolved with: #219

const encoding = nonAsciiEscaper.getEncodingFromAlias(propertiesFileSourceEncoding || "ISO-8859-1");
await nonAsciiEscaper({
resources: [resource], options: {
encoding
}
});
}

if (!res.getHeader("Content-Type")) {
if (!charset) {
charset = mime.charset(type);
}

res.setHeader("Content-Type", type + (charset ? "; charset=" + charset : ""));
}

Expand All @@ -71,7 +71,7 @@ function createMiddleware({resources}) {
// UTF-8 anyways.
// Also, only process .library, *.js and *.json files. Just like it's done in Application-
// and LibraryBuilder
if (charset === "UTF-8" && rReplaceVersion.test(resourcePath)) {
if ((!charset || charset === "UTF-8") && rReplaceVersion.test(resourcePath)) {
if (resource._project) {
stream = stream.pipe(replaceStream("${version}", resource._project.version));
} else {
Expand All @@ -80,9 +80,9 @@ function createMiddleware({resources}) {
}

stream.pipe(res);
}).catch((err) => {
} catch (err) {
next(err);
});
}
};
}

Expand Down
14 changes: 7 additions & 7 deletions test/lib/server/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,29 +67,29 @@ test("Get resource from application.a with replaced version placeholder (/versio
});
});

test("Get resource from application.a (/i18n/i18n.properties) with correct charset 'ISO-8859-1'", (t) => {
test("Get resource from application.a (/i18n/i18n.properties) with correct content-type", (t) => {
return request.get("/i18n/i18n.properties").then((res) => {
if (res.error) {
t.fail(res.error.text);
}
t.deepEqual(res.statusCode, 200, "Correct HTTP status code");
t.deepEqual(res.headers["content-type"], "text/plain; charset=ISO-8859-1", "Correct content type and charset");
t.deepEqual(Buffer.from(res.text, "latin1").toString(), "showHelloButtonText=Say Hello!", "Correct response");
t.deepEqual(res.headers["content-type"], "application/octet-stream", "Correct content type");
t.deepEqual(res.body.toString(), "showHelloButtonText=Say Hello!", "Correct response");
});
});

test("Get resource from application.a (/i18n/i18n_de.properties) with correct encoding 'ISO-8859-1'", (t) => {
test("Get resource from application.a (/i18n/i18n_de.properties) with correct content-type'", (t) => {
return request.get("/i18n/i18n_de.properties")
.responseType("arraybuffer")
.then((res) => {
if (res.error) {
t.fail(res.error.text);
}
t.deepEqual(res.statusCode, 200, "Correct HTTP status code");
t.deepEqual(res.headers["content-type"], "text/plain; charset=ISO-8859-1",
"Correct content type and charset");
t.deepEqual(res.headers["content-type"], "application/octet-stream",
RandomByte marked this conversation as resolved.
Show resolved Hide resolved
"Correct content type");

t.deepEqual(res.body.toString("latin1"), "showHelloButtonText=Say ä!", "Correct response");
t.deepEqual(res.body.toString(), "showHelloButtonText=Say \\u00e4!", "Correct response");
// Because it took so long to figure this out I keep the below line. It is equivalent to the deepEqual above
// t.deepEqual(res.body.toString("latin1"), Buffer.from("showHelloButtonText=Say \u00e4!", "latin1").toString("latin1"),
// "Correct response");
Expand Down
2 changes: 1 addition & 1 deletion test/lib/server/middleware/serveIndex.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ test.serial("Check if index for files is created", (t) => {
end: function(content) {
t.regex(content, /<td title="1024 Bytes">1\.00 KB<\/td>\s*<td><a href="\/myFile1\.meh">myFile1\.meh<\/a><\/td>\s*<td>application\/octet-stream<\/td>/);
t.regex(content, /<td title="1048576 Bytes">1\.00 MB<\/td>\s*<td><a href="\/myFile2\.js">myFile2\.js<\/a><\/td>\s*<td>application\/javascript<\/td>/g);
t.regex(content, /<td title="1073741824 Bytes">1\.00 GB<\/td>\s*<td><a href="\/myFile3\.properties">myFile3\.properties<\/a><\/td>\s*<td>text\/plain;charset=ISO-8859-1<\/td>/g);
t.regex(content, /<td title="1073741824 Bytes">1\.00 GB<\/td>\s*<td><a href="\/myFile3\.properties">myFile3\.properties<\/a><\/td>\s*<td>application\/octet-stream<\/td>/g);
resolve();
},
};
Expand Down
153 changes: 153 additions & 0 deletions test/lib/server/middleware/serveResources.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,153 @@
const test = require("ava");
const sinon = require("sinon");
const resourceFactory = require("@ui5/fs").resourceFactory;
const serveResourcesMiddleware = require("../../../../lib/middleware/serveResources");
const writeResource = function(writer, path, size, stringContent) {
const statInfo = {
ino: 0,
ctime: new Date(),
mtime: new Date(),
size: size,
isDirectory: function() {
return false;
}
};
const resource = resourceFactory.createResource({path, buffer: Buffer.from(stringContent, "latin1"), statInfo});
// stub resource functionality in order to be able to get the Resource's content. Otherwise it would be drained.
sinon.stub(resource, "getStream").returns({
pipe: function() {
}
});
return writer.write(resource).then(() => {
return resource;
});
};
const fakeResponse = {
writeHead: function(status, contentType) {},
getHeader: function(string) {},
setHeader: function(string, header) {}
};

test.afterEach.always((t) => {
sinon.restore();
});


test.serial("Check if properties file is served properly", (t) => {
t.plan(4);

const readerWriter = resourceFactory.createAdapter({virBasePath: "/"});

return writeResource(readerWriter, "/myFile3.properties", 1024 * 1024, "key=titel\nfame=straße").then((resource) => {
const setStringSpy = sinon.spy(resource, "setString");
const middleware = serveResourcesMiddleware({
resources: {
all: readerWriter
},
tree: {
resources: {
configuration: {
propertiesFileSourceEncoding: "ISO-8859-1"
}
}
}
});

const response = fakeResponse;

const setHeaderSpy = sinon.spy(response, "setHeader");
const req = {
url: "/myFile3.properties",
headers: {}
};
const next = function(err) {
throw new Error(`Next callback called with error: ${err.message}`);
};
return middleware(req, response, next).then((o) => {
return resource.getString();
}).then((content) => {
t.is(content, `key=titel
fame=stra\\u00dfe`);
t.is(setHeaderSpy.callCount, 2);
t.is(setStringSpy.callCount, 1);
t.is(setHeaderSpy.getCall(0).lastArg, "application/octet-stream");
});
});
});

test.serial("Check if properties file is served properly with UTF-8", (t) => {
t.plan(4);

const readerWriter = resourceFactory.createAdapter({virBasePath: "/"});

return writeResource(readerWriter, "/myFile3.properties", 1024 * 1024, "key=titel\nfame=straße").then((resource) => {
const setStringSpy = sinon.spy(resource, "setString");
const middleware = serveResourcesMiddleware({
resources: {
all: readerWriter
},
tree: {
resources: {
configuration: {
propertiesFileSourceEncoding: "UTF-8"
}
}
}
});

const response = fakeResponse;

const setHeaderSpy = sinon.spy(response, "setHeader");
const req = {
url: "/myFile3.properties",
headers: {}
};
const next = function(err) {
throw new Error(`Next callback called with error: ${err.message}`);
};
return middleware(req, response, next).then((o) => {
return resource.getString();
}).then((content) => {
t.is(content, `key=titel
fame=stra\\ufffde`);
t.is(setHeaderSpy.callCount, 2);
t.is(setStringSpy.callCount, 1);
t.is(setHeaderSpy.getCall(0).lastArg, "application/octet-stream");
});
});
});

test.serial("Check if properties file is served properly without property setting", (t) => {
t.plan(4);

const readerWriter = resourceFactory.createAdapter({virBasePath: "/"});

return writeResource(readerWriter, "/myFile3.properties", 1024 * 1024, "key=titel\nfame=straße").then((resource) => {
const setStringSpy = sinon.spy(resource, "setString");
const middleware = serveResourcesMiddleware({
resources: {
all: readerWriter
}
});

const response = fakeResponse;

const setHeaderSpy = sinon.spy(response, "setHeader");
const req = {
url: "/myFile3.properties",
headers: {}
};
const next = function(err) {
throw new Error(`Next callback called with error: ${err.stack}`);
};
return middleware(req, response, next).then((o) => {
return resource.getString();
}).then((content) => {
t.is(content, `key=titel
fame=stra\\u00dfe`);
t.is(setHeaderSpy.callCount, 2);
t.is(setStringSpy.callCount, 1);
t.is(setHeaderSpy.getCall(0).lastArg, "application/octet-stream");
});
});
});