Skip to content

Implement PKCE support - #164

Open
Doridian wants to merge 2 commits into
donetick:developfrom
Doridian:feat/pkce-support
Open

Implement PKCE support#164
Doridian wants to merge 2 commits into
donetick:developfrom
Doridian:feat/pkce-support

Conversation

@Doridian

Copy link
Copy Markdown

Based upon #162, otherwise merge conflicts are annoying

This is the frontend part of PKCE support.

Fixes donetick/donetick#739

@tatezach

Copy link
Copy Markdown

This PR fixes a real, reproducible bug that isn't mentioned in its title, so I want to document it here — it may be worth pulling the generateRandomString change out as a standalone fix if PKCE review takes longer.

The current state generation is too short for spec-compliant providers. On main:

const randomState = Math.random().toString(32).substring(5)

Math.random().toString(32) returns 0. followed by a variable number of base-32 digits; stripping five characters leaves roughly seven to nine. Authelia enforces minimum_parameter_entropy: 8 by default and rejects anything shorter:

error=invalid_state
error_description=The state is missing or does not have enough characters and is
therefore considered too weak. Request parameter 'state' must be at least be 8
characters long to ensure sufficient entropy.
&state=nso2l4v

That state has seven characters. Because the length varies per attempt, login succeeds sometimes and fails other times, which makes it look like a flaky provider or reverse proxy rather than a client bug. The only workaround today is minimum_parameter_entropy: -1, which Authelia explicitly discourages in its own documentation.

This PR's generateRandomString(16) fixes it properly: 22 base64url characters, and crypto.getRandomValues instead of Math.random(). The latter matters independently of length — state is the CSRF protection for the authorization code flow, and Math.random() is not cryptographically secure.

For the record, this was reported in #17 back in March 2025 with the identical Authelia error and a six-character state. That issue was closed as completed, but the code on main was never changed, so the behaviour has persisted since then.

One review note: Uint8Array.prototype.toBase64() is very recent and not available in older WebViews. Since the same code path runs inside Capacitor on Android and iOS, it may be worth using a manual hex or base64url conversion instead, so the auth flow doesn't break on older devices:

const randomState = Array.from(data, b => b.toString(16).padStart(2, '0')).join('')

Happy to test a build against Authelia v4.39 on web, Android and iOS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature request] OAuth2 PKCE support

2 participants