Authentication examples assumes all promises a resolved in request order

[torsv454]

In the FAQ the recommended approach for managing the identity of the user during the processing of the request is:

app.use(async (ctx, next) => {
  // do whatever checks to determine the user ID
  ctx.state.userId = userId;
  await next();
  delete ctx.state.userId; // cleanup
  });

But according to the documentation context.state is "A record of application state.". This is also evident in the code in context.ts (L90) where the state in all context instances are the same object as the application state.

 constructor(...) {
    ...
this.state = app.state;

So by default the state object is shared across all request which means if the routes or any middlewares contains asynchronous code where the promises are executed out of order you will not have the state you expect (i.e. a request specific state). You can see this if you implement a route which waits for a promise which is resolved after a second invocation completes together with a middleware which follows the authentication example from the FAQ. For example:

import { Application, Router, Context } from "https://deno.land/x/oak/mod.ts";

export type Continuation = () => Promise<void>;

let resolve: () => void;
let reject;
const promise = new Promise((res, rej) => {
  resolve = res;
  reject = rej;
});

const dump = (ctx: Context) => {
  console.log(
    `The id ${ctx.state.userId} should be equal to ${ctx.request.url.pathname}.`
  );
};

const sayHello = async (ctx: Context) => {
  dump(ctx);

  // do something async, e.g. db, fs, etc.
  if (ctx.request.url.pathname === "/1") {
    await promise;
  }
  dump(ctx);

  if (ctx.request.url.pathname === "/2") {
    resolve();
  }

  ctx.response.body = `Result: ${ctx.state.userId} should be equal to ${ctx.request.url.pathname}.`;
};

// And this is from the Oak  FAQ (https://oakserver.github.io/oak/FAQ).
interface MyState {
  userId: string;
}

const app = new Application<MyState>();

app.use(async (ctx, next) => {
  // do whatever checks to determine the user ID
  // We'll take the path as the userid to prove the point
  ctx.state.userId = ctx.request.url.pathname;
  await next();
  // delete ctx.state.userId;
  ctx.state.userId = "whatever";
});

const router = new Router();
router
  .get("/1", sayHello) //
  .get("/2", sayHello);

app.use(router.routes());

await app.listen({ port: 1993 });

Then executing in 3 separate terminals in order:

deno run --allow-net main.ts

And

curl -X GET http://localhost:1993/1

And

curl -X GET http://localhost:1993/2

Which yields:

The id /1 should be equal to /1.
The id /2 should be equal to /2.
The id /2 should be equal to /2.
The id /2 should be equal to /1. // <--- This is not as intended. 

To achieve a request specific state that is stable across the entire lifetime of the request regardless of resolution order of any promises involved, which is what you need for the authentication pattern to work, you need to introduce a middleware which does something along the lines of:

async (ctx: Context, next: Continuation) => {
  ctx.state = {}; // or new whatever.
  await next();
};

Am I missing something obvious here or are the authentication examples broken since the state object is shared across all requests and therefore cannot be used to store the userid / session id.

[kaleidawave]

I agree, storing request specific information to a single global variable is terrible. Especially for something as important as user sessions as your examples show a big security risk. I'm not sure if oak processes requests concurrently but that would cause problems with this method as well.

I am currently running oak with user sessions. The current JS best practise for associating data to an object is using WeakMap rather than assigning to its instance. This is something like I have currently:

const _ctxToUserIDMap: WeakMap<Context, string> = new WeakMap();

function getUserIDFromOakRequest(ctx: Context): string {
    if (_ctxToUserIDMap.has(ctx)) {
        return _ctxToUserIDMap.get(ctx);
    } else {
        const userID = /** get user id. as ctx is scoped you have access to its headers, cookies etc */
        _ctxToUserIDMap.set(ctx, userId);
        return userId;
    }
}

app.use((ctx: Context) => {
    const userID = getUserIDFromOakRequest(ctx);
    /** */
});

And this works because ctx is the same object throughout middleware but unique to that network request (I think??). So what ever middleware is doing it can still get the user without going back to the databases etc. Using WeakMap rather than Map means that the ctx objects can be garbage collected. and this works with TypeScript. Prevents collisions with a possible other library that uses ctx.state.userID. And should be very fast by using maps. And the getUserIDFromOakRequest can be asynchronous. Just need to import and use this function where ever you need access to the user. And I can't see any concurrency or other problems here.

I think this is the model Oak should be pushing.

For now the example in the docs should be removed. and ctx.state should be renamed to ctx.appState to prevent confusion. @kitsonk

Deno and Oak are still not recommended for production so I don't this as a critical security issues.

[torsv454]

:+1: Thanks for your detailed reply. Then it wasn't something obvious I missed at least.

A WeakMap approach using either Context or the Request object should be fine. Unfortunately I saw that some of the middlewares linked from the middleware page suffer from the same problem as the examples, e.g. https://github.com/denjucks/session/blob/master/frameworks/oak.ts.

[cmd-johnson]

Coming from Koa and without looking too deep into Oak's documentation, having a single State object shared between each and every request goes right past all my expectations. This just sounds like an accident waiting to happen (which I guess already did happen, seeing the examples making the same assumption).

At best it's a bug that's difficult to track down, because it only happens with requests happening in parallel. At worst, it's a gaping security hole allowing attackers to impersonate others.

I like the simplicity of just having a request state that's used across middlewares. Using a WeakMap as mentioned in the other comments works, but gets repetitive if you have multiple middlewares that all are interested in the currently logged in user.

Seeing how similar Oak and Koa are in other regards, I'm in favor of re-purposing the ctx.state parameter to an object that's unique for every request, without the risk of leaking data between requests. The application state can still be accessed through ctx.app.state if needed. I guess it'd also be possible to just a requestState property to the Context class, but having both state and requestState on the same context doesn't really less confusing (although that would work around introducing a breaking change).

You could also add some kind of defaultRequestState property to the application that's copied to create each request's initial state value.

If you want, I'll gladly create a PR for whichever approach you like best.

[kaleidawave]

The problem is that its a design issue with Oak rather than a internal issue.

I'm in favor of re-purposing the ctx.state parameter to an object that's unique for every request, without the risk of leaking data between requests.

I believe this is already possible through just defining a property to request e.g. ctx.request["user"] = someUser. And I am not really sure how and where defaultRequestState would be useful.

At best it's a bug that's difficult to track down, because it only happens with requests happening in parallel. At worst, it's a gaping security hole allowing attackers to impersonate others.

👍

I would advocate for a registerSource method that exists on Application

const app = new Application();

app.registerSource<IUser>("user", (ctx) => getUserFromCTX(ctx));

app.use(async (ctx) => {
    console.log(ctx.requestState.user)
});

Under the hood it would add the method to some property on the application. The application would expose a object under ctx.requestState that is a bunch of getters that are created with Object.defineProperty. The value of this in the get execution would be ctx. And it would do the whole WeakMap thing under the hood. Kind of hard to describe, I will work on a PR in the next coming days to show it.

[cmd-johnson]

I believe this is already possible through just defining a property to request e.g. ctx.request["user"] = someUser. And I am not really sure how and where defaultRequestState would be useful.

I'm not sure if a defaultRequestState is really needed either, but it may be nice to have if you have a properly typed state so you don't have to make all properties optional and spam your code with lots of ?? and || or === undefined, but I don't think that's a strong argument.

Monkey-patching the Context or Request or Response object sure is possible, but I'd love to have nicely typed generics for my request state.

I would advocate for a registerSource method that exists on Application

Under the hood it would add the method to some property on the application. The application would expose a object under ctx.requestState that is a bunch of getters that are created with Object.defineProperty. The value of this in the get execution would be ctx. And it would do the whole WeakMap thing under the hood. Kind of hard to describe, I will work on a PR in the next coming days to show it.

I think I get the idea.

I'd already be happy if requestState would just be set to an empty object of type Partial<RequestState> for every incoming request and I could populate the values using middlewares.

What would be the benefit of using your registerSource method over just using middleware to populate the requestState?

[kaleidawave]

Implemented the registerSource suggestion at #250. The benefits are it is a bit stricter than just assigning data in middleware. Being built on getters it will only compute properties when they are called. Currently ambivalent on it and recognise it does not really fit into the whole middleware design.

On the other hand a state property on the Request object would be straightforward to implement + including types so that is probably a better suggestion at this stage. The only thing about it being Partial<RequestState> is that you would have to tell TypeScript that the property exists... but that should be fine.

[thesmart]

I have some thoughts on these two points:

Monkey-patching the Context or Request or Response object sure is possible, but I'd love to have nicely typed generics for my request state.

I believe this is already possible through just defining a property to request e.g. ctx.request["user"] = someUser. And I am not really sure how and where defaultRequestState would be useful.

As a user, I'd really appreciate keeping "kernel space" (data provided by Oak) separate from "user space" (data provided by middleware). For example, keeping ctx.request as a pristine (and ideally immutable) representation of the client's request so that there is no pollution and related unpredictability as a request traverses to down-stack middlewares.

I see ctx.state as an Application-scoped "global" user-memory space. Missing is a per-request user-memory space that ought to be kept separate from the ctx.request. Thus I introduce ctx.locals in #284. Otherwise, there is currently no type-safe and concurrency-safe way for middleware to store data in the stack. The benefit of storing such data in the stack is that it is naturally garbage collected w/o concern of leaking sensitive data between requests.