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.

Sign up for GitHub

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

Fix XMLHttpRequest issues #3237

Merged
merged 3 commits into from
Dec 4, 2021
Merged

Fix XMLHttpRequest issues #3237

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d8baea2
Filter the :Status pseudo-header out when fetching all headers for
perkinslr Dec 2, 2021
0efc78d
Use UTF_8 for lib://URI responses
perkinslr Dec 2, 2021
1e48ba2
Make `fetch` async and handle request.text body returning a promise.
perkinslr Dec 2, 2021
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
4 changes: 3 additions & 1 deletion src/main/java/net/rptools/maptool/client/ui/htmlframe/MTXMLHttpRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,9 @@ public void _getResponseHeaders(JSObject jheaders) {

responseHeaders.forEach(
(key, value) -> {
jheaders.setMember(key, value);
if (!key.equals(":Status")) {
Copy link
Collaborator

@kwvanderlinde kwvanderlinde Dec 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Since this isn't mentioned explicitly in the linked bug reports, I have a couple questions:

  1. What problems does it cause for this pseudo-header to be present?
  2. Are there other pseudo-headers that should also be filtered out to avoid similar problems?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops! That should have been included in the fetch bug report.
:Status is an intentionally invalid HTTP header which lets the macro writer explicitly set the response code. It is also used internally to get the status code from the request processing out to the wrapper layer. It could use an optional, or similar data type, but exposing those to the macro writers would be a bit of a trick. By specification, invalid HTTP headers must not be returned to the client, so it must be stripped out before the headers are returned. If the response is wrapped in a javascript Response object (as the fetch API does), Response's constructor will throw an exception on invalid headers. Note this is different from undefined / extension headers (valid headers must not start with :).

The problem is the header filtering happens when the response body is received. If there is an exception thrown between when the header default values are set and the response body is received, the body-received function is not called, so the headers do not get filtered. When fetch then tries to handle the failed request, it chokes on the invalid header.

So to sum up:

  1. It breaks the fetch API
  2. No.

That said, I think it might still have a problem outside the fetch API on internal server errors. Probably need to refactor it slightly to always explicitly filter the headers without calling the body-received function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, nope, I can't see why the MTXMLHttpRequest::recv doesn't get called before the headers get retrieved. Especially given that the critical onreadystatechange call which triggers the javascript-side header processing happens after the headers should be handled.

jheaders.setMember(key, value);
}
});
}

Expand Down
2 changes: 1 addition & 1 deletion src/main/java/net/rptools/maptool/model/library/url/RequestHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ public static CompletableFuture<String> processRequest(
}
responseHeaders.put(":Status", "200");
try {
c.complete(new String(stream.readAllBytes(), StandardCharsets.UTF_16));
c.complete(new String(stream.readAllBytes(), StandardCharsets.UTF_8));
return c;
} catch (IOException e) {
responseHeaders.put(":Status", "500 Internal Exception");
Expand Down
5 changes: 3 additions & 2 deletions src/main/resources/net/rptools/maptool/client/html5/javascript/XMLHttpRequest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ class XMLHttpRequest {
}


function fetch(target, optionObject) {
async function fetch(target, optionObject) {
let request;
if (target instanceof Request) {
request = target;
Expand All @@ -126,7 +126,8 @@ function fetch(target, optionObject) {
for (let header of request.headers) {
x.setRequestHeader(header[0], header[1])
}
let body = request.text();
let body = await request.text();


let _resolve;
let _reject;
Expand Down