Skip to content

Commit

Permalink
fix: etag middleware corrupted binary stream responses
Browse files Browse the repository at this point in the history
(see ringo/ringojs#439) to construct the etag md5 digest it copies the response body into an array of byte arrays/byte strings, but `Stream.forEach()` re-uses a single binary buffer [1], so if the streamed response exceeds 8192 bytes and thus be split up into multiple buffers the body created by etag middleware would essentially consist of different byte arrays all containing the same bytes. the fix is to use `Binary.prototype.slice()` to copy the body part - this is expensive and effectively thwarts the use of `response.stream()`, but it's what the etag middleware did before #409 too, and at least it doesn't corrupt binary responses anymore.

[1] https://github.com/ringo/ringojs/blob/master/modules/io.js#L48
  • Loading branch information
grob committed Nov 6, 2021
1 parent a1bad9b commit 675fe5a
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 4 deletions.
12 changes: 8 additions & 4 deletions lib/middleware/etag.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,11 @@
* @fileOverview Middleware for conditional HTTP GET request based on
* response body message digests.
*
* The response body must implement a digest() method for this middleware to work.
* If the response body doesn't implement a `digest()` method this middleware
* creates a base 16 encoded MD5 digest of it. Note that for binary responses
* this is a memory expensive operation, as it needs to copy the body contents
* to create the digest. Especially for streamed JSGI responses the body should
* contain a `digest()` method.
*/

const strings = require('ringo/utils/strings');
Expand Down Expand Up @@ -32,7 +36,7 @@ exports.middleware = function etag(next, app) {
let {status, headers, body} = res;

if (status === 200) {
let etags, etag;
let etags, etag, binBody;
let header = request.headers["if-none-match"];
if (header) {
etags = header.split(",").map(function(s) {return s.trim() });
Expand All @@ -43,9 +47,9 @@ exports.middleware = function etag(next, app) {
etag = body.digest();
} else {
// we can't rely on body having map(), so we fake it with forEach()
const binBody = [];
binBody = [];
body.forEach(function(part) {
binBody.push(binary.toByteString(part));
binBody.push((part instanceof Binary) ? part.slice() : binary.toByteString(part));
});
etag = digest(binBody);
}
Expand Down
25 changes: 25 additions & 0 deletions test/middleware/etag_test.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
const assert = require("assert");
const {Headers} = require("ringo/utils/http");
const fs = require("fs");
const {Arrays} = java.util;
const response = require("ringo/jsgi/response");
const {MemoryStream} = require("io");

const {Application} = require("../../lib/stick");

Expand Down Expand Up @@ -33,6 +37,27 @@ exports.testBasicMD5 = function() {
assert.equal(headers.get("etag"), "1234567890");
};

exports.testBinaryStreamResponse = function() {
const file = module.resolve("./fixtures/ringo-drums.png");
const app = new Application();
app.configure("etag", "route");
app.get("/", function() {
return response.stream(fs.open(file, {binary: true}), "image/png");
});
const result = app({
method: 'GET',
headers: {},
env: {},
pathInfo: '/'
});
const headers = Headers(result.headers);
assert.equal(headers.get("content-type"), "image/png");
assert.equal(headers.get("etag"), "0C15DE3FBC9D7A7B936F98147CD7FDCB");
const body = new MemoryStream();
result.body.forEach(part => body.write(part));
assert.isTrue(Arrays.equals(body.content, fs.read(file, {binary: true})));
};

if (require.main === module) {
require("system").exit(require("test").run(module.id));
}
Binary file added test/middleware/fixtures/ringo-drums.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.

0 comments on commit 675fe5a

Please sign in to comment.