fix(oauth): omit blank pkce from url when not supported

[uphillcheddar]

Description

This change fixes an issue with OAuth providers that do not support blank (empty) PKCE strings, such as AWS Cognito. The original code would send an empty code_challenge and code_challenge_method to the OAuth provider, which would cause these providers to return an error. This change modifies the code to only send these parameters if the OAuth provider explicitly supports PKCE. this has been a problem for my instance since 1.132.0 . this issue is discussed in this thread by another user. #18482 (comment)

Fixes # (issue)

How Has This Been Tested?

This change has been tested manually with AWS Cognito.

• Test A: Verified that the code_challenge and code_challenge_method parameters are not sent to the OAuth provider when the provider does not support PKCE.
• Test B: Verified that the code_challenge and code_challenge_method parameters are sent to the OAuth provider when the provider does support empty PKCE strings.

Screenshots (if appropriate)

Checklist:

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation if applicable
• I have no unrelated changes in the PR.
• I have confirmed that any new dependencies are strictly necessary.
• I have written tests for new code (if applicable)
• I have followed naming conventions/patterns in the surrounding code
• All code in src/services/ uses repositories implementations for database calls, filesystem operations, etc.
• All code in src/repositories/ is pretty basic/simple and does not have any immich specific logic (that belongs in src/services/)

Please describe to which degree, if any, an LLM was used in creating this pull request.

I used an LLM to verify the spelling/grammar of this pull request description. LLM was not used for the actual code changes as they were relatively simple (adding a conditional check to the url builder)

[danieldietzler]

You can keep the old syntax and simply return undefined in the or case of the ternary instead.

[uphillcheddar]

i tried it with undefined:

const url = buildAuthorizationUrl(client, {
      redirect_uri: redirectUrl,
      scope: config.scope,
      state,
      code_challenge: client.serverMetadata().supportsPKCE() ? codeChallenge : undefined,
      code_challenge_method: client.serverMetadata().supportsPKCE() ? 'S256' : undefined,
    }).toString();

but that gives me some typing errors

"Type 'string | undefined' is not assignable to type 'string'.
Type 'undefined' is not assignable to type 'string'.ts(2322)
(property) code_challenge_method: string | undefined"

would you accept something like this ?

    const url = buildAuthorizationUrl(client, {
      redirect_uri: redirectUrl,
      scope: config.scope,
      state,
      ...(client.serverMetadata().supportsPKCE() && {code_challenge: codeChallenge, code_challenge_method: 'S256'}),
    }).toString();
    return { url, state, codeVerifier };
  }

[danieldietzler]

I guess it doesn't really matter but I personally do prefer that, yes. FYI I just looked at the RFC and code_challenge is required per the spec https://datatracker.ietf.org/doc/html/rfc7636#section-4.3, so I'd be hesitant omitting that from the response

[uphillcheddar]

The spec that you pointed to is for the pkce extension specifically, and not oauth2 as a whole. Meaning IF you are using pkce then you must absolutely provide those keys.

however my change is for setups that are not using pkce please see the below from section 5 (emphasis mine)

"Server implementations of this specification may accept OAuth 2.0 clients that do not implement this extension. If the "code_verifier" is not received from the client in the Authorization Request, servers supporting backwards compatibility revert to the OAuth 2.0 [RFC6749] protocol without this extension."

let me know if you are good with it or not and i can edit my merge

thanks
-cheddar

---

EDIT / PS:

for additional info/clarity the client.serverMetadata().supportsPKCE() uses the .well-known/openid-configuration metadata file to determine if a provider supports PKCE.

Here is a cognito example :

Since the "code_challenge_methods_supported" property is omitted, it means the provider does not support PKCE. Therefore, the supportsPKCE() function in the openid-client library will correctly return false. But right now the code is ignoring the metadata and sending the (blank) code challenge

{
  "authorization_endpoint": "https://login.domain.com/oauth2/authorize",
  "end_session_endpoint": "https://login.domain.com/logout",
  "id_token_signing_alg_values_supported": [
    "RS256"
  ],
  "issuer": "https://cognito-idp.us-east-1.amazonaws.com/us-east-1_XXXXXXXX",
  "jwks_uri": "https://cognito-idp.us-east-1.amazonaws.com/us-east-1_XXXXXXXX/.well-known/jwks.json",
  "response_types_supported": [
    "code",
    "token"
  ],
  "revocation_endpoint": "https://login.domain.com/oauth2/revoke",
  "scopes_supported": [
    "openid",
    "email",
    "phone",
    "profile"
  ],
  "subject_types_supported": [
    "public"
  ],
  "token_endpoint": "https://login.domain.com/oauth2/token",
  "token_endpoint_auth_methods_supported": [
    "client_secret_basic",
    "client_secret_post"
  ],
  "userinfo_endpoint": "https://login.domain.com/oauth2/userInfo"
}

In my opinion, the current behavior of sending a blank code_challenge is incorrect and violates the spec. Since a blank string does not meet the minimum length or entropy requirements outlined in 4.1 and 4.2. and Cognito is correct to reject the request as invalid because it's a malformed PKCE request.

the code should be binary:

• if pkce is supported according to the metadata send a valid, correctly-formatted code_challenge
• if pkce is not supported according to the metadata: Omit the code_challenge and code_challenge_method parameters entirely from the authorization request.

[danieldietzler]

The spec that you pointed to is for the pkce extension specifically

Oh damn it, should've read more closely, apologies! Thanks for the very elaborate answer. :)

[jrasm91]

I'd probably prefer moving the options to a variable and then doing if(pkce) { add properties to object }

[uphillcheddar]

I'd probably prefer moving the options to a variable and then doing if(pkce) { add properties to object }

Can I get some guidance on my next steps? My initial commit was assembling the string as you discribed, but Daniel requested the update to the spread method. I'm happy to update as needed just need to know the right method to use

Thanks

[jrasm91]

That's too funny lol