Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The false option of the pkceMethod init parameter for the JavaScript adapter is ignored #28335

Closed
1 of 2 tasks
mstichweh opened this issue Apr 1, 2024 · 2 comments · Fixed by #28347
Closed
1 of 2 tasks
Assignees
Labels
area/adapter/javascript kind/bug Categorizes a PR related to a bug kind/regression priority/blocker Highest Priority. Has a deadline and it blocks other tasks release/24.0.3 release/25.0.0 team/ui
Milestone

Comments

@mstichweh
Copy link

Before reporting an issue

  • I have read and understood the above terms for submitting issues, and I understand that my issue may be closed without action if I do not follow them.

Area

adapter/javascript

Describe the bug

Since version 24 where "S256" is the default for the PKCE method, it can not be disabled anymore. Even the new false option does not have any effect. So "S256" is always set as PKCE method.

Version

24.0.1

Regression

  • The issue is a regression

Expected behavior

It should be again possible to disable PKCE by setting the pkceMethod option to false, as described within the documentation.

Actual behavior

The false option of the pkceMethod init parameter is ignored. The check for false is missing in the source code:

if (initOptions.pkceMethod) {
    if (initOptions.pkceMethod !== "S256") {
        throw new TypeError(`Invalid value for 'pkceMethod', expected 'S256' but got '${initOptions.pkceMethod}'.`);
    }
    kc.pkceMethod = initOptions.pkceMethod;
} else {
    kc.pkceMethod = "S256";
}

It should be:

if (initOptions.pkceMethod) {
    if (initOptions.pkceMethod !== "S256") {
        throw new TypeError(`Invalid value for 'pkceMethod', expected 'S256' but got '${initOptions.pkceMethod}'.`);
    }
    kc.pkceMethod = initOptions.pkceMethod;
} else if (initOptions.pkceMethod !== false) {
    kc.pkceMethod = "S256";
}

How to Reproduce?

  1. Create a new Keycloak JavaScript adapter
  2. Provide false for the pkceMethod option when calling the init method
  3. For example call the createLoginUrl method
  4. The resulting URL will include the parameter code_challenge_method=S256 but it should not

Anything else?

No response

@mstichweh mstichweh added kind/bug Categorizes a PR related to a bug status/triage labels Apr 1, 2024
@jonkoops jonkoops self-assigned this Apr 2, 2024
@keycloak-github-bot keycloak-github-bot bot added kind/regression priority/blocker Highest Priority. Has a deadline and it blocks other tasks and removed status/triage action/priority-regression labels Apr 2, 2024
@jonkoops jonkoops added action/priority-regression and removed priority/blocker Highest Priority. Has a deadline and it blocks other tasks kind/regression labels Apr 2, 2024
@keycloak-github-bot keycloak-github-bot bot added this to the 25.0.0 milestone Apr 2, 2024
@keycloak-github-bot keycloak-github-bot bot added kind/regression priority/blocker Highest Priority. Has a deadline and it blocks other tasks and removed action/priority-regression labels Apr 2, 2024
jonkoops added a commit to jonkoops/keycloak that referenced this issue Apr 2, 2024
Closes keycloak#28335

Signed-off-by: Jon Koops <jonkoops@gmail.com>
jonkoops added a commit to jonkoops/keycloak that referenced this issue Apr 2, 2024
Closes keycloak#28335

Signed-off-by: Jon Koops <jonkoops@gmail.com>
@jonkoops
Copy link
Contributor

jonkoops commented Apr 2, 2024

Thanks for the report @mstichweh. I've created a PR to resolve this issue (#28347). Is there any practical reason why you would want to not have PKCE enabled?

@mstichweh
Copy link
Author

Thanks for the PR! I'm currently working on a Capacitor login solution and so doing the Access Token exchange by hand with a POST request. In the moment it seems to be the easier way to disable PKCE, but it still do not work, so we will see what the final solution will be ;-)

jonkoops added a commit to jonkoops/keycloak that referenced this issue Apr 2, 2024
Closes keycloak#28335

Signed-off-by: Jon Koops <jonkoops@gmail.com>
jonkoops added a commit to jonkoops/keycloak that referenced this issue Apr 9, 2024
Closes keycloak#28335

Signed-off-by: Jon Koops <jonkoops@gmail.com>
jonkoops added a commit to jonkoops/keycloak that referenced this issue Apr 9, 2024
Closes keycloak#28335

Signed-off-by: Jon Koops <jonkoops@gmail.com>
jonkoops added a commit that referenced this issue Apr 9, 2024
Closes #28335

Signed-off-by: Jon Koops <jonkoops@gmail.com>
stianst pushed a commit to stianst/keycloak that referenced this issue Apr 16, 2024
Closes keycloak#28335

Signed-off-by: Jon Koops <jonkoops@gmail.com>
stianst added a commit that referenced this issue Apr 16, 2024
* Limit requests sent through session status iframe (#132)

Closes #116

Signed-off-by: Jon Koops <jonkoops@gmail.com>

* Perform exact string match if redirect URI contains userinfo, encoded slashes or parent access (#131)

Closes keycloak/keycloak-private#113
Closes keycloak/keycloak-private#134

Signed-off-by: rmartinc <rmartinc@redhat.com>
Co-authored-by: Stian Thorgersen <stianst@gmail.com>

* Validate Saml URLs inside DefaultClientValidationProvider (#135)

Closes keycloak/keycloak-private#62

Signed-off-by: rmartinc <rmartinc@redhat.com>

* Avoid the same userSessionId after re-authentication (#136)

Closes #69

Signed-off-by: Giuseppe Graziano <g.graziano94@gmail.com>

* Better management of domains in TrustedHostClientRegistrationPolicy (#139)

Closes keycloak/keycloak-private#63

Signed-off-by: rmartinc <rmartinc@redhat.com>

* Secondary factor bypass in step-up authentication (#143)

closes #34

Signed-off-by: mposolda <mposolda@gmail.com>

* Restrict the token types that can be verified when not using the user info endpoint (#146)

Closes #47

Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>

Conflicts:
	core/src/main/java/org/keycloak/util/TokenUtil.java
	testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/ClientTokenExchangeTest.java

* Resolve the user federation link as null when decorating the user profile metadata in the LDAP provider (#147)

Closes #28100

Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>

* Align isEnabled in MSAD mappers to how other properties are processed in UserAttributeLDAPStorageMapper (#148)

- user model is updated by onImport with the enabled/disabled status of the LDAP user
- a config option always.read.enabled.value.from.ldap was introduced, in synch to what we have in UserAttributeLDAPStorageMapper
- isEnabled checks the flag to decide if it should always retrieve the value from LDAP, or return the local value.
- setEnabled first updates the LDAP tx, and then calls the delegate to avoid issue #24201

Closes #26695
Closed #24201

Signed-off-by: Stefan Guilhen <sguilhen@redhat.com>
(cherry picked from commit 2ca59d4)

* Ignore all links to GitHub when checking external links in docs due to rate limiting issues (#151)

Closes #28330

Signed-off-by: stianst <stianst@gmail.com>

* Make sure attribute metadata from user storage providers are added only for the provider associated with a federated user (#150)

Closes #28248

Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>

Conflicts:
	docs/documentation/upgrading/topics/changes/changes-24_0_3.adoc

* Allow `false` to be set for `pkceMethod` option (#28347) (#152)

Closes #28335

Signed-off-by: Jon Koops <jonkoops@gmail.com>

* fix: adds a test and permissions for cache configmap (#153)

closes: #28638

Signed-off-by: Steve Hawkins <shawkins@redhat.com>

---------

Signed-off-by: Jon Koops <jonkoops@gmail.com>
Signed-off-by: rmartinc <rmartinc@redhat.com>
Signed-off-by: Giuseppe Graziano <g.graziano94@gmail.com>
Signed-off-by: mposolda <mposolda@gmail.com>
Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>
Signed-off-by: stianst <stianst@gmail.com>
Signed-off-by: Steve Hawkins <shawkins@redhat.com>
Co-authored-by: Jon Koops <jonkoops@gmail.com>
Co-authored-by: Ricardo Martin <rmartinc@redhat.com>
Co-authored-by: Giuseppe Graziano <g.graziano94@gmail.com>
Co-authored-by: Marek Posolda <mposolda@gmail.com>
Co-authored-by: Pedro Igor <pigor.craveiro@gmail.com>
Co-authored-by: Stefan Guilhen <sguilhen@redhat.com>
Co-authored-by: Steven Hawkins <shawkins@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/adapter/javascript kind/bug Categorizes a PR related to a bug kind/regression priority/blocker Highest Priority. Has a deadline and it blocks other tasks release/24.0.3 release/25.0.0 team/ui
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants