Skip to content

fix(validation): allow RFC 6749 VSCHAR characters in client_id and client_secret#638

Merged
H2CK merged 3 commits into
H2CK:masterfrom
tony-engineering:fix/vschar-client-credentials
Apr 9, 2026
Merged

fix(validation): allow RFC 6749 VSCHAR characters in client_id and client_secret#638
H2CK merged 3 commits into
H2CK:masterfrom
tony-engineering:fix/vschar-client-credentials

Conversation

@tony-engineering
Copy link
Copy Markdown
Contributor

@tony-engineering tony-engineering commented Apr 5, 2026

Summary

I'm trying to automate OIDC clients provisioning in Nextcloud and hit a wall, took me some time to understand issue. I was generating client id and secrets with the terminal, but realized that the implementation of the oidc plugin is too restrictive. Here is my proposal.

The current validation in oidc:create and SettingsController.addClient() restricts client_id and client_secret to [A-Za-z0-9] characters only.

RFC 6749 (OAuth 2.0) Appendix A https://datatracker.ietf.org/doc/html/rfc6749#appendix-A defines both as *VSCHAR (any printable ASCII, %x20-7E). The only character that must be excluded is : (colon), which is the Basic auth username/password separator per RFC 7617.

The current restriction breaks interoperability with any credential generator that follows standard naming conventions (e.g. myapp-service, UUIDs, hyphenated identifiers). The failure mode is particularly opaque: the CLI rejects the credential at creation time with a non-obvious error message, and if a credential somehow reaches the token endpoint with a non-alphanumeric character, Nextcloud logs a generic Login failed with no indication of the root cause.

Changes

  • Relax the validation regex in lib/Command/Clients/OIDCCreate.php and lib/Controller/SettingsController.php from /^[A-Za-z0-9]{32,64}$/ to VSCHAR minus : and space: /^[\x21-\x39\x3B-\x7E]{32,64}$/
  • Update error messages accordingly
  • Add unit tests for credentials containing -, _, . (should pass) and : (should fail)

Impact

No existing credentials are affected - the alphanumeric subset is fully contained within the new character set. The 32-64 length requirement is preserved as a security floor.

Antoine ROUGEOT and others added 2 commits March 1, 2026 20:39
Copy link
Copy Markdown
Owner

@H2CK H2CK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution. Please have a look at the further comments which are all related to the test execution and solve the problems.
The other changes are in general fine for me.

Comment thread tests/Unit/Controller/SettingsControllerTest.php
Comment thread tests/Unit/BasicAuthBackendTest.php
@H2CK H2CK merged commit 46046fb into H2CK:master Apr 9, 2026
3 checks passed
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.

2 participants