Implement access grants for OAuth #130

ThisIsMissEm · 2025-03-09T21:04:07Z

This is still a work in progress, but gives an idea of how the code changes. I haven't yet implemented PKCE in here, though the database supports it.

ThisIsMissEm

Left some informative comments.

Code coverage for src/oauth.tsx is 56.47%, and for the other parts:

 hollo/src/oauth         |   83.18 |    79.16 |   85.71 |   83.18 |                                              
  constants.ts           |     100 |      100 |     100 |     100 |                                              
  helpers.ts             |   78.16 |    57.14 |      80 |   78.16 | 40-59,92-98,131-134                          
  middleware.ts          |   98.52 |    93.33 |     100 |   98.52 | 61                                           
  validators.ts          |   66.66 |       50 |     100 |   66.66 | 11-17

ThisIsMissEm · 2025-03-09T21:05:18Z

src/db.ts

+// This is necessary for passing a transaction into a function:
+export type Transaction = PgTransaction<
+  PostgresJsQueryResultHKT,
+  typeof schema,
+  ExtractTablesWithRelations<typeof schema>
+>;


Apparently drizzle doesn't export a type for this, which is weird.

ThisIsMissEm · 2025-03-09T21:06:17Z

src/oauth.test.ts

+      t.assert.equal(
+        response.headers.get("Location"),
+        "custom://oauth_callback?error=access_denied&error_description=The+resource+owner+or+authorization+server+denied+the+request.",
+      );


I could maybe assert here that the count of access grants is zero?

ThisIsMissEm · 2025-03-09T21:09:00Z

src/oauth.tsx

+          // Revoke the access grant:
+          await tx
+            .update(accessGrants)
+            .set({
+              revokedAt: new Date(),
+            })
+            .where(eq(accessGrants.token, accessGrantToken));


Currently I'm setting revokedAt here, which means our database will have loads of now useless access grants in it. It would be a reasonable decision to just delete the access grant from the database in this case.

@dahlia would like your thoughts here, a trade-off is that by not setting revoked and just deleting the access grant, you can't detect authorization code reuse attacks, where the authorization code is reused and consequently all created access tokens from that authorization code should be invalidated (grants, this would imply having some relationship between access grants and access tokens)

ThisIsMissEm · 2025-03-09T21:11:14Z

src/oauth.tsx

@@ -272,18 +292,9 @@ app.post("/token", cors(), async (c) => {
      401,
    );
  }
-  if (form.scope?.some((s) => !application.scopes.includes(s))) {


scope is only required for client_credentials grant type.

ThisIsMissEm · 2025-03-09T21:19:11Z

src/oauth/middleware.test.ts

+  it(
+    "Returns an error if the access token is not valid",
+    { plan: 3 },
+    async (t: TestContext) => {
+      const response = await app.request("/endpoint", {
+        method: "GET",
+        headers: {
+          // Forces the Access Token code path:
+          Authorization: "Bearer foo^bar",
+        },
+      });
+
+      t.assert.equal(response.status, 401, "Should return 401");
+      t.assert.equal(response.headers.get("content-type"), "application/json");
+
+      const json = await response.json();
+
+      t.assert.equal(json.error, "invalid_token");
+    },
+  );


This code path technically doesn't exist anymore, so we could remove this test.

src/schema.ts

ThisIsMissEm · 2025-03-09T21:23:43Z

package.json

+    "test": "pnpm run migrate:test && tsx --env-file-if-exists=.env.test --test --test-concurrency=1",
+    "test:ci": "tsx --test --test-reporter=@reporters/github --test-reporter-destination=stdout --test-reporter=spec --test-reporter-destination=stdout --test-concurrency=1",


We needed to switch to concurrency of 1 for now, as the database tests clean the database, which causes issues if multiple tests are running concurrently.

ThisIsMissEm · 2025-03-09T21:24:53Z

package.json

    "check": "tsc && biome check .",
    "check:ci": "tsc && biome ci --reporter=github .",
+    "check:coverage": "c8 --check-coverage --lines 100 pnpm test -- --experimental-test-coverage --test-reporter=lcov --test-reporter-destination=coverage/tmp/lcov.info",


For now, this is just to give an idea of where we stand, but we may want to make it part of CI to check code coverage once we get it high enough.

Could probably also already include it as like a PR comment or something.

pnpm-lock.yaml

ThisIsMissEm · 2025-03-09T21:32:44Z

Correction, oauth coverage is, I had some dead code:

 hollo/src               |   71.03 |    67.47 |   23.28 |   71.03 |           
  oauth.tsx              |   56.47 |       40 |      80 |   56.47 | ...9,418-427,433-441,444-452,455-463,479-485 
 hollo/src/oauth         |   90.99 |    79.16 |     100 |   90.99 |                                              
  constants.ts           |     100 |      100 |     100 |     100 |                                              
  helpers.ts             |    90.9 |    57.14 |     100 |    90.9 | 71-77,110-113                                
  middleware.ts          |   98.52 |    93.33 |     100 |   98.52 | 61                                           
  validators.ts          |   66.66 |       50 |     100 |   66.66 | 11-17

src/schema.ts

… type flow Mastodon apparently supports passing scopes to /oauth/token endpoint

ThisIsMissEm · 2025-03-10T19:40:08Z

@dahlia I've just been working on the access tokens for FIRES, and came across something interesting, which is still using a database value for the "secret values", but using a construction of ${tokenBytes}.${hmac(tokenBytes, secretKey)} (where both values are base64 urlsafe). This allows using the database for the token storage, whilst also easily filtering out completely invalid tokens using cryptography.

Might be an idea to adopt that. I've also come across some interesting stuff for including CRC32's at the end of the token, such that secret scanning tools can easily detect the secret is a "valid secret": https://docs.github.com/en/code-security/secret-scanning/secret-scanning-partnership-program/secret-scanning-partner-program#identify-your-secrets-and-create-regular-expressions

ThisIsMissEm commented Mar 9, 2025

View reviewed changes

ThisIsMissEm force-pushed the feat/implement-access-grants branch 2 times, most recently from fd5e2e6 to 9481a4b Compare March 9, 2025 21:39

dahlia reviewed Mar 10, 2025

View reviewed changes

src/schema.ts Outdated Show resolved Hide resolved

src/schema.ts Outdated Show resolved Hide resolved

ThisIsMissEm added 8 commits March 10, 2025 17:06

Add schema for OAuth Access Grants table

95fb36b

Add test coverage for tokenRequired middleware

8ecc1cc

Add script for checking code coverage

898d454

Implement Access Grants usage

3375653

Remove incorrect scope check in authorization grant flow

94a9cf9

Remove dead code

8d1b629

Remove overtly strict check on scopes during authorization_code grant…

f732189

… type flow Mastodon apparently supports passing scopes to /oauth/token endpoint

Fix issues created after changing the database schema

66e347e

ThisIsMissEm force-pushed the feat/implement-access-grants branch from ec19c6c to 66e347e Compare March 10, 2025 16:08

Rename access_grants.token to access_grants.code

1686c63

ThisIsMissEm mentioned this pull request Apr 1, 2025

fix: Add OPTIONS method to /oauth/token CORS policy to support preflight requests #138

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement access grants for OAuth #130

Implement access grants for OAuth #130

ThisIsMissEm commented Mar 9, 2025

ThisIsMissEm left a comment

ThisIsMissEm Mar 9, 2025

ThisIsMissEm Mar 9, 2025

ThisIsMissEm Mar 9, 2025

ThisIsMissEm Mar 10, 2025

ThisIsMissEm Mar 9, 2025

ThisIsMissEm Mar 9, 2025

ThisIsMissEm Mar 9, 2025

ThisIsMissEm Mar 9, 2025

ThisIsMissEm commented Mar 9, 2025

ThisIsMissEm commented Mar 10, 2025

		"test": "pnpm run migrate:test && tsx --env-file-if-exists=.env.test --test --test-concurrency=1",
		"test:ci": "tsx --test --test-reporter=@reporters/github --test-reporter-destination=stdout --test-reporter=spec --test-reporter-destination=stdout --test-concurrency=1",

Implement access grants for OAuth #130

Are you sure you want to change the base?

Implement access grants for OAuth #130

Conversation

ThisIsMissEm commented Mar 9, 2025

ThisIsMissEm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThisIsMissEm commented Mar 9, 2025

ThisIsMissEm commented Mar 10, 2025