Comment by kentonv

Hi, I'm the author of the library. (Or at least, the author of the prompts that generated it.)

> I’m also an expert in OAuth

I'll admin I think Neil is significantly more of an expert than me, so I'm delighted he took a pass at reviewing the code! :)

I'd like to respond to a couple of the points though.

> The first thing that stuck out for me was what I like to call “YOLO CORS”, and is not that unusual to see: setting CORS headers that effectively disable the same origin policy almost entirely for all origins:

I am aware that "YOLO CORS" is a common novice mistake, but that is not what is happening here. These CORS settings were carefully considered.

We disable the CORS headers specifically for the OAuth API (token exchange, client registration) endpoints and for the API endpoints that are protected by OAuth bearer tokens.

This is valid because none of these endpoints are authorized by browser credentials (e.g. cookies). The purpose of CORS is to make sure that a malicious website cannot exercise your credentials against some other website by sending a request to it and expecting the browser to add your cookies to that request. These endpoints, however, do not use browser credentials for authentication.

Or to put in another way, the endpoints which have open CORS headers are either control endpoints which are intentionally open to the world, or they are API endpoints which are protected by an OAuth bearer token. Bearer tokens must be added explicitly by the client; the browser never adds one automatically. So, in order to receive a bearer token, the client must have been explicitly authorized by the user to access the service. CORS isn't protecting anything in this case; it's just getting in the way.

(Another purpose of CORS is to protect confidentiality of resources which are not available on the public internet. For example, you might have web servers on your local network which lack any authorization, or you might unwisely use a server which authorizes you based on IP address. Again, this is not a concern here since the endpoints in question don't provide anything interesting unless the user has explicitly authorized the client.)

Aside: Long ago I was actually involved in an argument with the CORS spec authors, arguing that the whole spec should be thrown away and replaced with something that explicitly recognizes bearer tokens as the right way to do any cross-origin communications. It is almost never safe to open CORS on endpoints that use browser credentials for auth, but it is almost always safe to open it on endpoints that use bearer tokens. If we'd just recognized and embraced that all along I think it would have saved a lot of confusion and frustration. Oh well.

> A more serious bug is that the code that generates token IDs is not sound: it generates biased output.

I disagree that this is a "serious" bug. The tokens clearly have enough entropy in them to be secure (and the author admits this). Yes, they could pack more entry per byte. I noticed this when reviewing the code, but at the time decided:

1. It's secure as-is, just not maximally efficient. 2. We can change the algorithm freely in the future. There is not backwards-compatibility concern.

So, I punted.

Though if I'd known this code was going to get 100x more review than anything I've ever written before, I probably would have fixed it... :)

> according to the commit history, there were 21 commits directly to main on the first day from one developer, no sign of any code review at all

Please note that the timestamps at the beginning of the commit history as shown on GitHub are misleading because of a history rewrite that I performed later on to remove some files that didn't really belong in the repo. GitHub appears to show the date of the rebase whereas `git log` shows the date of actual authorship (where these commits are spread over several days starting Feb 27).

> I had a brief look at the encryption implementation for the token store. I mostly like the design! It’s quite smart.

Thank you! I'm quite proud of this design. (Of course, the AI would never have come up with it itself, but it was pretty decent and filling in the details based on my explicit instructions.)