React-router: Param Types for Routes

[kennethkufluk]

Hi! I'm an engineer at Twitter working on the website.

We use params in Routes like so:
<Route path="/:username/status/:id" />

And then we query our API for matching ids.
https://api.twitter.com/1.1/status/${id}.json

We discovered that it was possible to encode additional path logic into the param matcher, and this could lead to an attacker crafting and sharing a url that makes calls to a different API than was intended. (basically by using uri encoding to add "../")

The obvious solution to this issue is to use patterns with the params.
<Route path="/:username([a-zA-Z_]{1,20})/status/:id(\d+)" />

This solves the immediate problem. However, we have a large codebase and we would want to have a way to enforce pattern usage.

On our side, for now, we've pulled paramTypes into another file, so that all the patterns can be standardized and shared, and enforce usage of that with a lint rule. It's not elegant but it works.

import * as paramTypes from './utils/param-types';
<Route path={`/${paramTypes.screenName}/status/${paramTypes.id}`} />

Perhaps instead this could be formalized with a new prop to Router.

<Router paramTypes={{
    screenName: ParamTypes.string,
    id: ParamTypes.number
  }}
>
  <Route
    path="/:username/status/:id"
  >
    <Tweet />
  </Route>
  <Route
    path="/:screenName/status/:id/:banana" // => error: banana not a valid param type.
  >
    <Tweet />
  </Route>
</Router>

Thanks

[MeiKatz]

Also sounds like a case for validate prop that may come with RRv6:

function validateProps(typeSpecs) {
  return (props) {
    // ... do something like PropTypes.checkPropTypes
  };
}

<Route
  path="/:screenName/status/:id/:banana"
  validate={validateProps({
    screenName: ParamTypes.string,
    id: ParamTypes.number
  })}
>
  <Tweet />
</Route>

[timdorr]

I feel like that's solving it on the wrong side of the equation. Mainly because validate is meant to block the Route matching at all. With a bad parameter, you may want to still match that route, but present an error to the user within its context. E.g., say "Could not load this tweet" within the tweet display screen, rather than falling back to a more generic 404 error. You may even want to record the validation failure to detect attack trends.

Anyways, it would seem like something that would be more useful as a Hook.

const { id, screenName, foo, baz } = useSanitizedParams(Sanitizers.TweetParams)

Then you can decide to recover from the error, do something with it, redirect elsewhere, or something else entirely. It's all up to you at that point. Same basic code, just a different spot.

[kennethkufluk]

With a bad parameter, you may want to still match that route, but present an error to the user within its context.

This is nuanced. We definitely want to match a route where a tweet has been deleted, for example. We'll get an id, the api request will 404 or whatever, we'll show an error message. The user was making a valid attempt to load a page and so we should service that request.

However, I think it would be better to _not_ match for malformed params. We don't want to sanitize the param in every usage, we want to trust it. If you stick a letter in a tweet id, eg "12A34", then it's fine that the route doesn't match and will fall through to the other Routes (which will eventually show our 404 page).

Same basic code, just a different spot.

In a single-person development team, sure. In a team project, this is a security risk - we've moved sanitizing our params from the routes to every usage of the params in the system.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
You can add the fresh label to prevent me from taking any action.