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.

Sign up for GitHub

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

Jump to bottom

fieldKeyParser: handle bad url-encoded prefix key #1267

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

fieldKeyParser: handle bad url-encoded prefix key #1267

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
afb0240
fieldKeyParser: handle bad uri-encoded prefix key
Nov 4, 2024
f18b63a
Merge branch 'master' into fieldkey-handle-bad-uri-encoding
Nov 9, 2024
dc73779
use urlDecode()
Nov 9, 2024
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
12 changes: 8 additions & 4 deletions lib/http/middleware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
// we would like for the entire requestpath to run within a single Promise context,
// so that we can manage the database transaction without awkward contortions.

const { urlDecode } = require('../util/http');
const Problem = require('../util/problem');
const Option = require('../util/option');

Expand All @@ -40,10 +41,13 @@ const versionParser = (request, response, next) => {
// TODO: we should probably reject as usual if multiple auth mechs are used
// at once but that seems like a corner of a corner case here?
const fieldKeyParser = (request, response, next) => {
let prefixKey;
const match = /^\/key\/([^/]+)\//.exec(request.url);

const prefixKey = Option.of(match).map((m) => decodeURIComponent(m[1]));
prefixKey.ifDefined(() => { request.url = request.url.slice(match[0].length - 1); });
if (match != null) {
prefixKey = urlDecode(match[1]);
if (prefixKey.isEmpty()) return next(Problem.user.authenticationFailed());
request.url = request.url.slice(match[0].length - 1);
}

const queryKey = Option.of(request.query.st);
queryKey.ifDefined((token) => {
Expand All @@ -53,7 +57,7 @@ const fieldKeyParser = (request, response, next) => {
request.originalUrl = `/v1/key/${token.replace(/\//g, '%2F')}${request.originalUrl.slice(3)}`;
});

request.fieldKey = Option.of(prefixKey.orElse(queryKey));
request.fieldKey = Option.of(Option.of(prefixKey).orElse(queryKey));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@matthew-white matthew-white Nov 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@matthew-white matthew-white Nov 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this line important when there is a query key:

delete request.query.st;

?

Copy link
Contributor Author

@alxndrsn alxndrsn Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.


next();
};
Expand Down
10 changes: 10 additions & 0 deletions test/unit/http/middleware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,16 @@ describe('middleware', () => {
});
});

it('should return error for unparsable percent-encoded prefix keys', (done) => {
const request = createRequest({ url: '/key/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa%eaaa!aaaaaaaaaaaaaaaaaa/users/23' });
fieldKeyParser(request, null, (error) => {
error.should.be.a.Problem();
error.problemCode.should.equal(401.2);
error.message.should.equal('Could not authenticate with the provided credentials.');
done();
});
});

it('should pass through any query key content', (done) => {
const request = createRequest({ url: '/v1/users/23?st=inva|id' });
fieldKeyParser(request, null, () => {
Expand Down