Skip to content

Allows import to succeed when realm names include whitespaces#11964

Closed
DGuhr wants to merge 3 commits intokeycloak:mainfrom
DGuhr:iss11921_fix_whitespaces_in_realmimport
Closed

Allows import to succeed when realm names include whitespaces#11964
DGuhr wants to merge 3 commits intokeycloak:mainfrom
DGuhr:iss11921_fix_whitespaces_in_realmimport

Conversation

@DGuhr
Copy link
Copy Markdown
Contributor

@DGuhr DGuhr commented May 12, 2022

cc @pedroigor @vmuzikar @abstractj As said, this does the trick, but I am not entirely sure about this solution (alternative could be to e.g. encode the URLs before giving them to the validation), so draft state for now. Test will follow, too. Could you please give me your thoughts on this one? Fixes it for both, wildfly and quarkus.

Closes #11921

@DGuhr DGuhr requested review from pedroigor and vmuzikar May 12, 2022 14:46
@vmuzikar
Copy link
Copy Markdown
Contributor

vmuzikar commented May 12, 2022

@DGuhr I'm sorry if I missed it, but is this a regression? If so, did you find the root cause?

abstractj
abstractj reviewed May 12, 2022
View reviewed changes
Comment thread services/src/main/java/org/keycloak/validation/DefaultClientValidationProvider.java Outdated
@DGuhr
Copy link
Copy Markdown
Contributor Author

DGuhr commented May 12, 2022

@vmuzikar from the answer in the issue it seems to be a regression from Keycloak 6, yes. Last PR that touched these was titled KEYCLOAK-14306 OIDC redirect_uri allows dangerous schemes resulting in potential XSS and seems to come from you, the title is part of why I am not sure this is the right solution ;)

@vmuzikar
Copy link
Copy Markdown
Contributor

vmuzikar commented May 12, 2022

@DGuhr Thank you for the clarification!

IMHO loosening the validation in URLs in general is not the way to go. We could maybe instead try to sanitize the URLs on Realm Import, before any validation happens.

@DGuhr
Copy link
Copy Markdown
Contributor Author

DGuhr commented May 12, 2022 •
edited
Loading

@vmuzikar updated the PR now to sanitize it at the right point, at least for the --import-realm at startup for quarkus. have to find the spots for wildfly / the actual import command, and I think sanitizeClientUrlsForImport will go to the RealmRepresentation itself as a method. But this solution now worked perfectly for realms with one or multiple whitespaces inside, and should have no side effects hopefully. more tomorrow.

edit: Ah hum, now the redirect uri field in the client will actually have the %20 inside... hum. Have to check if that actually causes side effects. Nevertheless, gotten a bit late. tomorrow.

@DGuhr DGuhr mentioned this pull request May 13, 2022
Closed
@DGuhr DGuhr added the status/hold PR should not be merged. On hold for later. label May 13, 2022
@DGuhr DGuhr closed this May 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abstractj abstractj abstractj left review comments

@vmuzikar vmuzikar Awaiting requested review from vmuzikar

@pedroigor pedroigor Awaiting requested review from pedroigor

Assignees

No one assigned

Labels

status/hold PR should not be merged. On hold for later.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Validation for realm name not containing space OLD DESCRIPTION: Realm import fails with "Base URL is not a valid URL; A redirect URI is not a valid URI"

3 participants

@DGuhr @vmuzikar @abstractj