Skip to content

urls cleanup: Fix buggy isUrlOnRealm function #5452

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

Closed
wants to merge 6 commits into from

Conversation

chrisbobbe
Copy link
Contributor

@chrisbobbe chrisbobbe commented Jul 27, 2022

I'm doing a cleanup of some relevant parts of our URL parsing logic, as I prepare for #5306. Inheriting priority from that.

From start to finish, this would take us from

// TODO: Work out what this does, write a jsdoc for its interface, and
// reimplement using URL object (not just for the realm)
export const isUrlOnRealm = (url: string, realm: URL): boolean =>
  url.startsWith('/') || url.startsWith(realm.toString()) || !/^(http|www.)/i.test(url);

to

// TODO: As of writing, we use this same check in 5 or so other places.
//   Probably should consolidate those so they call this function, or else
//   dissolve this function.
export const isUrlOnRealm = (url: URL, realm: URL): boolean => url.origin === realm.origin;

(Ah, and I think the TODO we're left with in the "after" is probably something we can resolve easily.)

Rather than making that big change in one commit, I've tried to pull out incremental changes so we can see how the behavior changes. As always, please let me know if you'd like me to be more or less incremental, or if there's a better order I can put the commits in. 🙂

We removed the last use of this in aeeb835. With the `URL` API, we
don't need things like this.
If isUrlOnRealm can assume its `url` param is a valid URL string,
then it'll be able to use the nicely defined function isUrlRelative,
which has that requirement. (We'll do that, coming up.)

Thankfully, it seems like its callers do pass well-formed URLs. They
operate on the `href` attribute of links and images in messages,
when one of those is pressed. And Anders says
  ( https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Interpreting.20links.20in.20messages/near/1410864 ):

> The server should be responsible for sending you correct `href`
> attributes.

The new jsdoc seems to match the function's name, and the
requirements of its existing callsites:

- It's used to decide whether to send an authenticated request to
  get a "temporary file URL" (see src/api/tryGetFileTemporaryUrl)
  when the user clicks a link in a message.

- It's used to decide whether to include the auth credential when
  pointing react-native-photo-view's PhotoView component to an image
  on the Web, for the lightbox.

I've written down some false positives and false negatives in the
implementation, on the lines of code responsible for them. We'll fix
those next.
See the previous commit where we started pinning this down: we gave
this function a jsdoc, and commented about some false positives and
false negatives we identified.

Now, use our assumption that the `url` param is a valid URL string,
to use isUrlRelative, which requires that.

isUrlRelative is carefully defined to follow the spec, so
isUrlOnRealm is now much clearer that it does what it says on the
tin, modulo the remaining false-negative bug, which we'll fix next.
getResource contains one of isUrlOnRealm's two callers. We'd like
that to take a URL object, coming up. So, equip this caller to pass
one.
This fixes a false negative; see the removed implementation comment
in the function.

Now this function uses the same check we use in the 5 or so other
places where we check if a URL is on a given realm. As a comment
says, we should probably either reuse this function in all those
cases, or delete it.
@@ -96,7 +96,6 @@ export const tryParseUrl = (url: string, base?: string | URL): URL | void => {
*
* (If relative, we assume callers want to treat it relative to the realm.)
*/
// TODO: Careful! Gives a false positive and false negatives; see comments.
Copy link
Contributor Author

@chrisbobbe chrisbobbe Jul 27, 2022

Choose a reason for hiding this comment

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

Ah, self-nit: one identified false negative remains in this commit (see code comment below), so don't remove this whole TODO just yet.

@gnprice
Copy link
Member

gnprice commented Aug 26, 2022

Thanks again @chrisbobbe for this fix and spotting the consequences of the issue!

I've now pushed f36ccd7 to main, which is a cherry-pick of the stripped-down bugfix that went into release v27.190. So the next step for this PR would be to rebase atop that. The refactors that are in this branch correspond well with what's described in that commit message:

This still isn't a great structure, because it'd be easy for a call
site to change (or a new one to be added) so that the URL gets used
in a way other than by passing to our `new URL`, or with a different
base URL.  Add a TODO to refactor accordingly, and mark the function
as deprecated to discourage new callers.

@chrisbobbe
Copy link
Contributor Author

Closing as superseded by #5492.

@chrisbobbe chrisbobbe closed this Oct 5, 2022
@chrisbobbe chrisbobbe deleted the pr-is-url-on-realm-fixes branch October 5, 2022 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants