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

fetch does not escape strings with special characters #135

Open
LilyStilson opened this issue Aug 19, 2023 · 5 comments
Open

fetch does not escape strings with special characters #135

LilyStilson opened this issue Aug 19, 2023 · 5 comments

Comments

@LilyStilson
Copy link

LilyStilson commented Aug 19, 2023

Bug

The title.

Current behavior

If you are trying to fetch from a remote API some JSON that will contain special characters (including \r\n\t, etc.) the response.json() will return you a string, not an object.

I made a repo and a Cloudflare worker for easier reproduction: LilyStilson/flutterjs_fetch_bug

Expected behavior

fetch properly escapes strings with special characters, parsing does not fail, response.json() is not returning a string, but an object and throws an exception (not fallbacks to response.text()) if it fails to parse JSON.

Remarks

I am not sure whether this is flutter_js issue or QuickJS issue. If it's the latter - I will transfer this issue to them. Also, their library (judging by their website) claims to support fetch and even some additional functions for JSON parsing which are not present in flutter_js.

@LilyStilson
Copy link
Author

Diving a little bit deeper, looks like the problem is not with the fetch polyfill, but with the XMLHttpRequest implementation.
I rewrote the callFetch() function in example like so:

async function callFetch(type) {
    function wrapper(url) {
        return new Promise((resolve, reject) => {
            const req = new XMLHttpRequest();
            req.open('GET', url, true);
            req.onload = () => req.status === 200 ? resolve(req.response) : reject(req.statusText);
            req.send();
        })
    }
    const response = await wrapper("https://flutterjs-test.nightskystudio.workers.dev/" + type);
    let json
    try {
        json = JSON.parse(response);
    } catch (e) {
        json = response;
    }
    return JSON.stringify({
      "result": json,
      "type": typeof(json)
    });
}

Something like a very-very simple wrapper around XMLHttpRequest, and yes, indeed, the problem is with its implementation.

@LilyStilson
Copy link
Author

Looks like I'm talking to myself, but that's okay.

After I went through your code, I think that the problem is with the way you pass values around in XMLHttpRequest code. Somewhere there the original value returned from the http.get is getting lost or re-encoded so that it's impossible to parse later.
The workaround I found is to use sendMessage() to call Dart and use its http package without any additional code on top of it.
Now, the runtime abstraction looks like this:

class JSRuntime {
  final JavascriptRuntime _runtime = getJavascriptRuntime();

  static const _internalCode = """async function callFetch(type) {
    let message = await sendMessage("get", JSON.stringify({"url": "https://flutterjs-test.nightskystudio.workers.dev/" + type}))

    return JSON.stringify(message)
}""";

  JSRuntime() {
    _runtime.evaluate(_internalCode);

    _runtime.onMessage("get", (args) async {
      Uri url = Uri.parse(args["url"]);
      final response = await http.get(url);
      final json = jsonDecode(response.body);
      return json;
    });
  }

   // runtimeEval, runtimeEvalAsync, callFetch

}

Same way we can rewrite fetch function to be basically a call to sendMessage and do all the work in Dart.

@LilyStilson
Copy link
Author

LilyStilson commented Aug 20, 2023

So... I purpose replacing fetch polyfill with this implementation

Javascript

function fetch(url, options) {
    options = options || {};
    return new Promise(async (resolve, reject) => {
        let request = await sendMessage("fetch", JSON.stringify({"url": url, "options": options}))

        const response = () => ({
            ok: ((request.status / 100) | 0) == 2, // 200-299
            statusText: request.statusText,
            status: request.status,
            url: request.responseURL,
            text: () => Promise.resolve(request.responseText),
            json: () => Promise.resolve(request.responseText).then(JSON.parse),
            blob: () => Promise.resolve(new Blob([request.response])),
            clone: response,
            headers: request.headers,
        })

        if (request.ok) resolve(response());
        else reject(response());
    });
}

Dart

_runtime.onMessage("fetch", (args) async {
  Uri url = Uri.parse(args["url"]);
  Map options = args["options"];
  Map<String, String> headers = (options["headers"] as Map<dynamic, dynamic>).map((key, value) => MapEntry("$key", "$value"));
  http.Response response;

  switch(options["method"]) {
    case "GET":
      response = await http.get(url, headers: headers);
      break;
    case "POST":
      response = await http.post(url, headers: headers, body: options["body"]);
      break;
    case "PUT":
      response = await http.put(url, headers: headers, body: options["body"]);
      break;
    case "DELETE":
      response = await http.delete(url, headers: headers);
      break;
    default:
      throw Exception("Invalid method");
  }

  final json = {
    "ok": response.statusCode >= 200 && response.statusCode < 300,
    "status": response.statusCode,
    "statusText": response.reasonPhrase,
    "headers": jsonEncode(response.headers),
    "body": response.body,
    "responseURL": response.request!.url.toString(),
    "responseText": response.body,
  };

  return json;
});

Of course, any improvements to JS/Dart implementation are very welcome. This fixes loss of special characters in response. I won't be creating a pull request for now, since I currently don't have time to find a way to incorporate this code into existing implementation, but everyone who stumbled across the same problem as I did can use this code to fix this.

@moshstudio
Copy link

Thanks for the greate work! Solved \n " .etc escape problems.

@Gieted
Copy link

Gieted commented Apr 11, 2024

The reason to escape character is lost is this line of code, which doesn't escape \ ('\"' in js is just ").

BTW literally nothing is escaped, so the owner of the page we're making requests to could easily perform an RCE attack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants