fix: re-enable code challenge support (#1035)

forcedotcom · Mar 11, 2024 · 5786688 · 5786688
1 parent a7c3155
commit 5786688
Show file tree

Hide file tree

Showing 3 changed files with 52 additions and 3 deletions.
diff --git a/src/org/authInfo.ts b/src/org/authInfo.ts
@@ -333,8 +333,10 @@ export class AuthInfo extends AsyncOptionalCreatable<AuthInfo.Options> {
    * @param options The options to generate the URL.
    */
   public static getAuthorizationUrl(options: JwtOAuth2Config & { scope?: string }, oauth2?: OAuth2): string {
-    // Always use a verifier for enhanced security
-    options.useVerifier = true;
+    // Unless explicitly turned off, use a code verifier for enhanced security
+    if (options.useVerifier !== false) {
+      options.useVerifier = true;
+    }
     const oauth2Verifier = oauth2 ?? new OAuth2(options);
 
     // The state parameter allows the redirectUri callback listener to ignore request

diff --git a/src/webOAuthServer.ts b/src/webOAuthServer.ts
@@ -141,6 +141,8 @@ export class WebOAuthServer extends AsyncCreatable<WebOAuthServer.Options> {
     if (!this.oauthConfig.clientId) this.oauthConfig.clientId = DEFAULT_CONNECTED_APP_INFO.clientId;
     if (!this.oauthConfig.loginUrl) this.oauthConfig.loginUrl = AuthInfo.getDefaultInstanceUrl();
     if (!this.oauthConfig.redirectUri) this.oauthConfig.redirectUri = `http://localhost:${port}/OauthRedirect`;
+    // Unless explicitly turned off, use a code verifier as a best practice
+    if (this.oauthConfig.useVerifier !== false) this.oauthConfig.useVerifier = true;
 
     this.webServer = await WebServer.create({ port });
     this.oauth2 = new OAuth2(this.oauthConfig);
@@ -237,6 +239,7 @@ export class WebOAuthServer extends AsyncCreatable<WebOAuthServer.Options> {
       this.logger.debug(`oauthConfig.loginUrl: ${this.oauthConfig.loginUrl}`);
       this.logger.debug(`oauthConfig.clientId: ${this.oauthConfig.clientId}`);
       this.logger.debug(`oauthConfig.redirectUri: ${this.oauthConfig.redirectUri}`);
+      this.logger.debug(`oauthConfig.useVerifier: ${this.oauthConfig.useVerifier}`);
       return authCode;
     }
     return null;

diff --git a/test/unit/webOauthServerTest.ts b/test/unit/webOauthServerTest.ts
@@ -35,11 +35,24 @@ describe('WebOauthServer', () => {
   });
 
   describe('getAuthorizationUrl', () => {
-    it('should return authorization url', async () => {
+    it('should return authorization url with expected (default) params', async () => {
       const oauthServer = await WebOAuthServer.create({ oauthConfig: {} });
       const authUrl = oauthServer.getAuthorizationUrl();
       expect(authUrl).to.not.be.undefined;
       expect(authUrl).to.include('client_id=PlatformCLI');
+      expect(authUrl).to.include('code_challenge=');
+      expect(authUrl).to.include('state=');
+      expect(authUrl).to.include('response_type=code');
+    });
+
+    it('should return authorization url with no code_challenge', async () => {
+      const oauthServer = await WebOAuthServer.create({ oauthConfig: { useVerifier: false } });
+      const authUrl = oauthServer.getAuthorizationUrl();
+      expect(authUrl).to.not.be.undefined;
+      expect(authUrl).to.include('client_id=PlatformCLI');
+      expect(authUrl).to.not.include('code_challenge=');
+      expect(authUrl).to.include('state=');
+      expect(authUrl).to.include('response_type=code');
     });
   });
 
@@ -70,6 +83,37 @@ describe('WebOauthServer', () => {
       const handleSuccessStub = stubMethod($$.SANDBOX, oauthServer.webServer, 'handleSuccess').resolves();
       const authInfo = await oauthServer.authorizeAndSave();
       expect(authInfoStub.save.callCount).to.equal(1);
+      expect(authStub.callCount).to.equal(1);
+      const authCreateOptions = authStub.firstCall.args[0] as AuthInfo.Options;
+      expect(authCreateOptions).have.property('oauth2Options');
+      expect(authCreateOptions).have.property('oauth2');
+      expect(authCreateOptions.oauth2Options).to.have.property('useVerifier', true);
+      expect(authCreateOptions.oauth2Options).to.have.property('clientId', 'PlatformCLI');
+      expect(authCreateOptions.oauth2).to.have.property('codeVerifier');
+      expect(authCreateOptions.oauth2?.codeVerifier).to.have.length.greaterThan(1);
+      expect(authCreateOptions.oauth2).to.have.property('clientId', 'PlatformCLI');
+      expect(authInfo.getFields()).to.deep.equal(authFields);
+      expect(handleSuccessStub.calledOnce).to.be.true;
+    });
+
+    it('should save new AuthInfo without codeVerifier', async () => {
+      redirectStub = stubMethod($$.SANDBOX, WebServer.prototype, 'doRedirect').callsFake(async () => {});
+      stubMethod($$.SANDBOX, WebOAuthServer.prototype, 'executeOauthRequest').callsFake(async () => serverResponseStub);
+      const oauthServer = await WebOAuthServer.create({ oauthConfig: { useVerifier: false } });
+      await oauthServer.start();
+      // @ts-expect-error because private member
+      const handleSuccessStub = stubMethod($$.SANDBOX, oauthServer.webServer, 'handleSuccess').resolves();
+      const authInfo = await oauthServer.authorizeAndSave();
+      expect(authInfoStub.save.callCount).to.equal(1);
+      expect(authStub.callCount).to.equal(1);
+      const authCreateOptions = authStub.firstCall.args[0] as AuthInfo.Options;
+      expect(authCreateOptions).have.property('oauth2Options');
+      expect(authCreateOptions).have.property('oauth2');
+      expect(authCreateOptions.oauth2Options).to.have.property('useVerifier', false);
+      expect(authCreateOptions.oauth2Options).to.have.property('clientId', 'PlatformCLI');
+      expect(authCreateOptions.oauth2).to.have.property('codeVerifier');
+      expect(authCreateOptions.oauth2?.codeVerifier).to.be.undefined;
+      expect(authCreateOptions.oauth2).to.have.property('clientId', 'PlatformCLI');
       expect(authInfo.getFields()).to.deep.equal(authFields);
       expect(handleSuccessStub.calledOnce).to.be.true;
     });