fieldKeyParser: handle bad url-encoded prefix key

[alxndrsn]

Related:

• http: handle bad url-encoded Cookie session token #1269
• audit-logs: handle bad url-encoded X-Action-Notes header #1268
• CSRF token: handle failed url-decoding #1265
• formats/odata: handle bad url-encoded subpaths #1270

Closes #1266

[alxndrsn]

@matthew-white is there a more idiomatic way to write this than nesting calls to Option.of()?

[matthew-white]

Right now, prefixKey is either a non-empty Option, or it's null. What if you always made it an Option (an Option.none() instead of null)?

if (match != null) {
  prefixKey = urlDecode(match[1]);
  // ...
} else {
  prefixKey = Option.none();
}

If you did that, then you could set request.fieldKey in the same way as before:

request.fieldKey = Option.of(prefixKey.orElse(queryKey));

Though honestly, the way it was before doesn't feel especially clear to me. There, if prefixKey is not empty, .orElse() will unwrap it, requiring us to rewrap it with Option.of().

Maybe a ternary would be clearer?

request.fieldKey = prefixKey.isDefined() ? prefixKey : queryKey;

I'm not sure that that's necessarily more idiomatic though.

[matthew-white]

Here's another thought. If match != null, why don't we set request.fieldKey immediately and return? Why do we even go on to handle queryKey? I feel like right now, there's technically the possibility that someone could specify both: /v1/key/[some app user token]/projects/1/forms?st=[some public link token]. In that case, request.fieldKey would hold the app user token, while request.originalUrl would be rewritten with the public link token.

[alxndrsn]

Is this line important when there is a query key:

delete request.query.st;

?

[alxndrsn]

There's currently some weird manipulation of originalUrl available when both the query key and the prefix key are provided.

[matthew-white]

There's currently some weird manipulation of originalUrl available when both the query key and the prefix key are provided.

It looks that way to me too. I'd be very happy if we shut that path down. We don't expect a user to specify both.

Also originalUrl seems to inconsistently include /v1, depending on whether a query key or prefix key is used...

I think that's probably just due to how the tests are written. Some tests include /v1 in the url passed to createRequest(), while others don't. I'm pretty sure that versionParser() runs before fieldKeyParser() and will strip off /v1, so fieldKeyParser() shouldn't ever see /v1 in request.url. fieldKeyParser() will still see /v1 in request.originalUrl though, as versionParser() doesn't change that.

Is this line important when there is a query key:

delete request.query.st;

?

If we removed that line, I'm not sure that anything would break. My guess is that the idea was, we've done everything we need to with this query parameter, so nothing downstream ever needs to think about it.

That said, it looks like we retain the st query parameter in request.originalUrl, so maybe there's a little inconsistency in that approach: we don't remove st from everything we could. That might have been a practical decision: it's easy to remove st from request.query, but maybe not as easy to remove it from request.originalUrl.

[alxndrsn]

Rewriting originalUrl at all seems questionable:

This property is much like req.url; however, it retains the original request URL, allowing you to rewrite req.url freely for internal routing purposes.
-https://expressjs.com/en/api.html#req.originalUrl

[alxndrsn]

@matthew-white assertions for st behaviour added at #1295

[alxndrsn]

I think that's probably just due to how the tests are written. Some tests include /v1 in the url passed to createRequest(), while others don't. I'm pretty sure that versionParser() runs before fieldKeyParser() and will strip off /v1, so fieldKeyParser() shouldn't ever see /v1 in request.url.

@matthew-white I've updated #1291 to address this.

[matthew-white]

Rewriting originalUrl at all seems questionable

I also feel a little unsure about rewriting it. It says "original" after all. 😅 Then again, it seems to be working fine as-is.

Looking at the codebase, it looks like we use originalUrl in a number of places. However, my guess is that the only place where we rely on the rewritten originalUrl is lib/resources/forms.js, since that's where the OpenRosa form list and manifest are served. A bunch of OData functionality references originalUrl, but public links that use ?st can't access OData, so I doubt it matters there. It also looks like ?st is explicitly restricted to certain types of actors: authHandler() will return a 403 if a web user tries to use ?st with a session token associated with their account.

If we wanted to do something other than rewriting originalUrl, one option is that we could add a new property (openRosaUrl? urlWithoutSt?). But again, it does seem to be working as-is. It could be that Express provides originalUrl for convenience, so that it can be accessed after url is rewritten, yet also doesn't mind if you rewrite originalUrl.

[matthew-white]

I've marked the PR as a draft again even though it's approved, since we've started discussing some interesting related topics in the comments above.