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

RFC: JavaScriptCore-safe bundle URLs #646

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Changes from 4 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
64 changes: 64 additions & 0 deletions proposals/0642-JavaScriptCore-safe-bundle-URLs.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
---
title: JavaScriptCore-safe bundle URLs
authors:
- Rob Hogan (@robhogan)
- Moti Zilberman (@motiz88)
date: 2023-05-02
---

# RFC0642: JavaScriptCore-safe bundle URLs

## Summary

Add support for a query-string-free URL format to bundlers, so that symbolication (etc.) is not broken by JavaScriptCore error stack manipulation introduced in https://github.com/WebKit/WebKit/commit/71985aa1520560154a329e5b54159fff3173cf22 (first appearing in iOS 16.4).

## Basic example

`http://localhost:8080/project/index.bundle;&platform=ios`

Should be supported by Metro (and other bundlers) equivalently and in addition to the current query-string form:

`http://localhost:8080/project/index.bundle?platform=ios`

## Motivation

Recent versions of JavaScriptCore strip query strings from source URLs in stack traces (`Error.prototype.stack`).

To implement features such as Metro's `/symbolicate` endpoint, we must be able to derive (a close approximation of) the source that was executed the client. To do so correctly we need to know the build parameters the client used originally. Those parameters being stripped (and defaults used instead) has manifested as obscure resolution errors for React Native users, on both Expo and the community CLI ([1](https://github.com/facebook/react-native/issues/36794), [2](https://github.com/expo/expo/issues/22008)).

Ideally, for maximal compatibility with debuggers (etc.) that may intercept the raw error stack, the source URLs in the error stack should be sufficient, and should be valid URLs, without any post-processing or external context.

## Detailed design

Use a character sequence from the "reserved" set specified in RFC 3986, namely `;&`, as an alternative delimiter in place of `?`, and percent-encode any `?` characters appearing within the subsequent query string, so that the result is a valid [`path`](https://www.rfc-editor.org/rfc/rfc3986#section-3.3).

If the client sends a URL in this format, Metro should respond as if `?` was seen in place of the alternative delimiter, with the following exceptions:
Copy link
Author

Choose a reason for hiding this comment

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

@motiz88 I think we need to specify what "in this format" means here. For example, what format is this?

`/path/with-;&-unencoded/index.bundle?platform=ios`

If we interpret it as a "URL of mixed format" as suggested below, then we'll break the specific edge case we claim to support, because clients are not presently required to percent-encode this sequence within a bundle request path (and in fact, don't). In practice, we could ensure that React Native escapes that sequence in future, but it'd still technically be a semver-breaking change to Metro, albeit in a narrow edge case.

It might be more practical to consider an input to be "in this format" if it contains both a ;& within the path, and no query component.

Another possibility is to use a character sequence that can't appear in (normalised) POSIX paths - there aren't many, but maybe //?

- Wherever the bundle itself encodes its own URL, source map URL, or the URLs of other bundles on the same server (such as for lazy bundling), those should use the alternative format for any included query strings *if and only if* the original bundle request used the alternative format. Otherwise, standard query strings starting with `?` should be emitted.
- URLs of mixed format with `;&` preceding `?`, i.e. `.../path;&foo=bar?foo=baz&bar=quux` should be interpreted leniently, as if they were given fully in the alternative format, i.e. `.../path;&foo=bar&foo=baz&bar=quux`.

Because the alternative delimiter is made of reserved characters, it does not create ambiguity with standard URLs and may be escaped by percent-encoding for use as a param value, path component, etc. In particular, this supports the edge case of a valid POSIX file path containing the substring `;&` being the subject of a bundle request.

## Drawbacks

- The proposed URL format is unfamiliar, and standard libraries and tools won't be able to encode/decode build parameters into/from URLs in this format. An exposed API or package would help mitigate.
- Maintenance/complexity of supporting an additional URL format.
- Forking of bundle URL generation in React Native by platform/engine, assuming we use this new format on as narrow a range of target engines as possible.

## Alternatives
Copy link

@tido64 tido64 May 9, 2023

Choose a reason for hiding this comment

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

Not entirely familiar with all the bits involved, but I have a couple of questions:

  • What prevents Safari from also stripping out ;& for security reasons?
  • Have you considered embedding a comment similar to //# sourceMappingURL= for source maps?

Copy link
Author

@robhogan robhogan May 9, 2023

Choose a reason for hiding this comment

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

  • What prevents Safari from also stripping out ;& for security reasons?

Nothing in theory, but my understanding is that query strings (which of course aren't fundamentally less secure than path components) are commonly used to append tracking or other sensitive information in a web context during redirection. From what I can gather (though the security bugzilla is private), the JSC change was to plug quite a specific hole where stack consumers could end up being sent the sensitive data added by a separate redirection service. Those services would have to motivation to make that private data leakable, and couldn't append ;& in the general case anyway because it will usually break routing.

I've also been in touch with Apple folks about building an opt-out for controlled, non-browser use cases like RN - they're open to that because the disruption to our use case was completely unintentional, so I don't think we're getting into an arms race here - just a workaround for a temporary situation.

  • Have you considered embedding a comment similar to //# sourceMappingURL= for source maps?

Metro already adds these, but JavaScriptCore doesn't use them when populating error stacks. The stacks coming from the engine only use the sourceURL provided explicitly to the JSEvaluateScript call (docs).


### Encode+decode within React Native runtime
Encode the source URL passed to JavaScriptCore to a format that won't be stripped, and need not be a URL (e.g., base64-encode the entire URL), catch errors within the runtime close to where they are thrown, and post-process stacks to decode and restore the original URLs before sending a `/symbolicate` request.

Pros:
- No changes required to Metro, fix closely coupled to specific issue and may be removed if/when it becomes unnecessary.

Cons:
- Relies on being able to catch and decode the stack before consumption - not a solution for all potential consumers of error stacks, (eg., errors caught within user code, uses of `new Error().stack`, remote debugging with Safari).

## Adoption strategy

Metro and React Native patch releases will be required. No change is required from users.

## How we teach this

React Native users don't need to be aware of this explicitly. This RFC is intended to communicate the change with maintainers of CLIs and bundlers that may need to make changes to accommodate new URLs.