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

Add range tests for blob #34384

Merged
merged 1 commit into from
Feb 20, 2023
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
197 changes: 197 additions & 0 deletions fetch/range/blob.any.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,197 @@
// META: script=/common/utils.js

const supportedBlobRange = [
{
name: "A simple blob range request.",
data: ["A simple Hello, World! example"],
type: "text/plain",
range: "bytes=9-21",
content_length: 13,
content_range: "bytes 9-21/30",
result: "Hello, World!",
},
{
name: "A blob range request with no end.",
data: ["Range with no end"],
type: "text/plain",
range: "bytes=11-",
content_length: 6,
content_range: "bytes 11-16/17",
result: "no end",
},
{
name: "A blob range request with no start.",
data: ["Range with no start"],
type: "text/plain",
range: "bytes=-8",
content_length: 8,
content_range: "bytes 11-18/19",
result: "no start",
},
{
name: "A simple blob range request with whitespace.",
data: ["A simple Hello, World! example"],
type: "text/plain",
range: "bytes= \t9-21",
content_length: 13,
content_range: "bytes 9-21/30",
result: "Hello, World!",
},
{
name: "Blob content with short content and a large range end",
data: ["Not much here"],
type: "text/plain",
range: "bytes=4-100000000000",
content_length: 9,
content_range: "bytes 4-12/13",
result: "much here",
},
];

const unsupportedBlobRange = [
{
name: "Blob range request with multiple range values",
data: ["Multiple ranges are not currently supported"],
type: "text/plain",
headers: {
"Range": "bytes=0-5,15-"
}
},
{
name: "Blob range request with multiple range values and whitespace",
data: ["Multiple ranges are not currently supported"],
type: "text/plain",
headers: {
"Range": "bytes=0-5, 15-"
}
},
{
name: "Blob range request with trailing comma",
data: ["Range with invalid trailing comma"],
type: "text/plain",
headers: {
"Range": "bytes=0-5,"
}
},
{
name: "Blob range with no start or end",
data: ["Range with no start or end"],
type: "text/plain",
headers: {
"Range": "bytes=-"
annevk marked this conversation as resolved.
Show resolved Hide resolved
}
},
{
name: "Blob range with invalid whitespace in range #1",
data: ["Invalid whitespace #1"],
type: "text/plain",
headers: {
"Range": "bytes=5 - 10"
}
},
{
name: "Blob range with invalid whitespace in range #2",
data: ["Invalid whitespace #2"],
type: "text/plain",
headers: {
"Range": "bytes=-\t 5"
}
},
{
name: "Blob range request with short range end",
data: ["Range end should be greater than range start"],
type: "text/plain" ,
headers: {
"Range": "bytes=10-5"
}
},
{
name: "Blob range start should be an ASCII digit",
data: ["Range start must be an ASCII digit"],
type: "text/plain" ,
headers: {
"Range": "bytes=x-5"
}
},
{
name: "Blob range should have a dash",
data: ["Blob range should have a dash"],
type: "text/plain" ,
headers: {
"Range": "bytes=5"
}
},
{
name: "Blob range end should be an ASCII digit",
data: ["Range end must be an ASCII digit"],
type: "text/plain" ,
headers: {
"Range": "bytes=5-x"
}
},
{
name: "Blob range should include '-'",
data: ["Range end must include '-'"],
type: "text/plain" ,
headers: {
"Range": "bytes=x"
}
},
{
name: "Blob range should include '='",
data: ["Range end must include '='"],
type: "text/plain" ,
headers: {
"Range": "bytes 5-"
}
},
{
name: "Blob range should include 'bytes='",
data: ["Range end must include 'bytes='"],
type: "text/plain" ,
headers: {
"Range": "5-"
}
},
{
name: "Blob content with short content and a large range start",
data: ["Not much here"],
type: "text/plain",
headers: {
"Range": "bytes=100000-",
}
},
];
Copy link
Member

Choose a reason for hiding this comment

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

This is great! I suggest we add some more invalid cases here to cover more parts of the parser:

  • bytes=x
  • bytes 5-
  • 5-
  • bytes=5-x
  • bytes=x-5

Other places we could add more coverage for is whitespace handling between the various tokens.

Let me know if you'd like some help. I can make some time to push a few tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Other places we could add more coverage for is whitespace handling between the various tokens.

RFC 7233 doesn't seem to indicate a position on whitespace. Is this specified somewhere?

Copy link
Member

Choose a reason for hiding this comment

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

It does although in a convoluted way. See https://httpwg.org/specs/rfc7233.html#rfc.section.1.2 and https://httpwg.org/specs/rfc7233.html#collected.abnf for "normal" ABNF. It seems it allows whitespace in only a few places.

Copy link
Member

Choose a reason for hiding this comment

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

It seems I was wrong here, but I think we should still test cases like:

  • bytes=5 - 10
  • bytes= - \t5

They should probably fail given the RFC, but it seems like implementations are already not following the RFC for whitespace after bytes= so who knows.

Perhaps this is also a thing we can still tighten however.

Copy link
Member Author

Choose a reason for hiding this comment

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

Based on some recent tests WebKit and Blink do seem to allow range specifications with whitespace like this...



supportedBlobRange.forEach(({ name, data, type, range, content_length, content_range, result }) => {
promise_test(async () => {
let blob = new Blob(data, { "type" : type });
return fetch(URL.createObjectURL(blob), {
"method": "GET",
"headers": {
"Range": range
}
}).then(function(resp) {
assert_equals(resp.status, 206, "HTTP status is 206");
assert_equals(resp.type, "basic", "response type is basic");
assert_equals(resp.headers.get("Content-Type"), type, "Content-Type is " + resp.headers.get("Content-Type"));
assert_equals(resp.headers.get("Content-Length"), content_length.toString(), "Content-Length is " + resp.headers.get("Content-Length"));
assert_equals(resp.headers.get("Content-Range"), content_range, "Content-Range is " + resp.headers.get("Content-Range"));
return resp.text();
}).then(function(text) {
assert_equals(text, result, "Response's body is correct");
});
}, name);
});

unsupportedBlobRange.forEach(({ name, data, type, headers }) => {
promise_test(function(test) {
let blob = new Blob(data, { "type" : type });
let promise = fetch(URL.createObjectURL(blob), {
"method": "GET",
"headers": headers,
});
return promise_rejects_js(test, TypeError, promise);
}, name);
});