Provider.checkToken will no longer treat empty refreshToken strings as invalid tokens and fixed use of __proto__ as identityId in IdentitiesManager

[amydevs]

Description

Provider

The handleClaimIdentity test in tests/identities/IdentitiesManager.test.ts was failing on handleClaimIdentity. This was because it was treating refreshTokens of empty strings as invalid tokens. This is no longer the case.

FAIL  tests/identities/IdentitiesManager.test.ts
  IdentitiesManager
    ✓ IdentitiesManager readiness (85 ms)
    ✓ get, set and unset tokens (with seed=-201149676) (395 ms)
    ✓ start and stop preserves state (with seed=-201149676) (234 ms)
    ✓ fresh start deletes all state (with seed=-201149676) (255 ms)
    ✓ register and unregister providers (39 ms)
    ✓ using TestProvider (with seed=-201149676) (1553 ms)
    ✕ handleClaimIdentity (with seed=1486144507) (939 ms)

  ● IdentitiesManager › handleClaimIdentity (with seed=1486144507)

    Property failed after 1 tests
    { seed: 1486144507, path: "0:0:0:0:0:0:0:0:0:0:0:0:0:3:0:1:1:0:3:0:0:0:1:1:0:1:3:2:0:1", endOnFailure: true }
    Counterexample: ["",{"accessToken":"          ","refreshToken":"","accessTokenExpiresIn":1698190047,"refreshTokenExpiresIn":0}]
    Shrunk 29 time(s)
    Got ErrorProviderUnimplemented:

      64 |
      65 |   public async refreshToken(): Promise<ProviderToken> {
    > 66 |     throw new identitiesErrors.ErrorProviderUnimplemented();
         |           ^
      67 |   }
      68 |
      69 |   public async getAuthIdentityIds(): Promise<Array<IdentityId>> {

      at TestProvider.refreshToken (tests/identities/TestProvider.ts:66:11)
      at TestProvider.refreshToken [as checkToken] (src/identities/Provider.ts:85:25)
      at TestProvider.checkToken [as publishClaim] (tests/identities/TestProvider.ts:152:16)
      at src/identities/IdentitiesManager.ts:272:39
      at constructor_.addClaim (src/sigchain/Sigchain.ts:449:20)
      at withF (node_modules/@matrixai/resources/src/utils.ts:24:12)
      at constructor_.handleClaimIdentity (src/identities/IdentitiesManager.ts:261:5)
      at numRuns (tests/identities/IdentitiesManager.test.ts:361:7)
      at AsyncProperty.run (node_modules/fast-check/lib/check/property/AsyncProperty.generic.js:49:28)
      Hint: Enable verbose mode in order to have the list of all failing values encountered during the run
      at buildError (node_modules/fast-check/lib/check/runner/utils/RunDetailsFormatter.js:131:15)
      at asyncThrowIfFailed (node_modules/fast-check/lib/check/runner/utils/RunDetailsFormatter.js:148:11)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 6 passed, 7 total
Snapshots:   0 total
Time:        4.02 s
Ran all test suites matching /.\/tests\/identities\/IdentitiesManager.test.ts/i.
GLOBAL TEARDOWN
Destroying Global Data Dir: /tmp/polykey-test-global-weOx7R

It is also defined now that any expiry values in AccessToken set to 0, now have their related tokens treated as never-expiring. This was already the case before this PR, but inline comments have been added to document this.

public async checkToken(
    providerToken: ProviderToken,
    identityId?: IdentityId,
  ): Promise<ProviderToken> {
    const now = Math.floor(Date.now() / 1000);
    // this will mean that accessTokenExpiresIn = 0 will be false
    if (
      providerToken.accessTokenExpiresIn &&
      providerToken.accessTokenExpiresIn >= now
    ) {
      if (providerToken.refreshToken == null) {
        throw new identitiesErrors.ErrorProviderUnauthenticated(
          'Access token expired',
        );
      }
      // this will mean that refreshTokenExpiresIn = 0 does not throw
      if (
        providerToken.refreshTokenExpiresIn &&
        providerToken.refreshTokenExpiresIn >= now
      ) {
        throw new identitiesErrors.ErrorProviderUnauthenticated(
          'Refresh token expired',
        );
      }
      return await this.refreshToken(providerToken, identityId);
    }
    return providerToken;
  }

Prototype Pollution

During testing, I noticed two other tests in IdentitiesManager.test.ts failing. This was because of fastcheck sometimes generating a string value of __proto__ for the identityId. The identityId is used as the key for the object that comes out of RocksDB, there is a problem with prototype pollution. This is not only a security issue, but also that JSON.stringify does not serialize the __proto__ property of an object, so certain the provider keys for that identity will never work. This PR uses Object.definePropertyto correctly set the __proto__ property on the providerTokens object.

Issues Related

• CLI Beta Launch Polykey-CLI#40 (comment)
• Fixes Prototype Pollution of getToken and putToken when identityIdis __proto__ #608

Tasks

• 1. Fix conditional check of valid refreshToken
• 2. Inline documentation
• 3. Define implementation for TestProvider.refreshToken
• 4. Add errors to throw when prototype pollution of identityId is attempted

Final checklist

• Domain specific tests
• Full tests
• Updated inline-comment documentation
• Lint fixed
• Squash and rebased
• Sanity check the final build

[ghost]

👇 Click on the image for a new way to code review

Legend

[CMCDragonkai]

If this works, proceed to squash down, no need to keep unnecessary lintfix commits around, and merge in. Are you waiting for something here?

[CMCDragonkai]

https://chat.openai.com/share/caaac780-4de6-4b1a-ab6a-68cd4ed58cd8

This explains that explicitly set __proto__ would be accepted as a property. So why is it a problem?

[CMCDragonkai]

In #608 I've explained how you can support __proto__ as a proper key instead of special casing it. Who knows, somebody might really have a username being __proto__ just to screw with people. This is the modern XKCD:

[CMCDragonkai]

To summarise for safety reasons, if the key is coming from an untrusted source, or a randomised source where you don't know what it could be, there's 2 safe ways of doing this:

1. o = {[untrustedString]: 123 }
2. Object.defineProperty(o, untrustedString, { value: 123, writable: true, enumerable: true, configurable: true }

It is not safe to do o[untrustedString] = 123 although o[untrustedString] can be used to look up the property once it is defined. And of course a literal o = { __proto__: 123 } also does not work!!

[CMCDragonkai]

Can you fix it that way, and add an inline comment describing the reason.

[amydevs]

Can you fix it that way, and add an inline comment describing the reason.

// This has to be done in case the key is `__proto__`.
    // Otherwise, the object will not be correctly serialized.
    // https://github.com/MatrixAI/Polykey/issues/608
    Object.defineProperty(providerTokens, identityId, {
      value: providerToken,
      writable: true,
      enumerable: true,
      configurable: true,
    });