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

Reject "%2F" as an invalid sequence in simp messaging usernames #23836

Closed
wants to merge 1 commit into from

Conversation

@njlaw
Copy link

njlaw commented Oct 19, 2019

Current State

Since messaging destinations are delimited by "/", the existing code performs a simple string replacement of "/" -> "%2F" in usernames. This allows usernames which contain "/" to be used and have messaging destinations parsed correctly. See SimpMessagingTemplate.java and DefaultUserDestinationResolver.java

Issue with Current State

Usernames that already contain "%2F" end up with that character sequence converted to a "/" by DefaultUserDestinationResolver. E.g., abc%2Fabc > abc/abc; however, abc/abc is not the username, so destination matching fails.

Here is an example test that fails, resulting in

org.springframework.messaging.simp.user.DefaultUserDestinationResolverTests > handleMessageEncodedUserNameWithPercentTwoEff() FAILED
--
org.opentest4j.AssertionFailedError at DefaultUserDestinationResolverTests.java:205
 
717 tests completed, 1 failed

:spring-messaging:testorg.springframework.messaging.simp.user.DefaultUserDestinationResolverTests » handleMessageEncodedUserNameWithPercentTwoEff() (0.044s)
org.opentest4j.AssertionFailedError: 

Expecting:
 <0> (instead resolves to 0 destinations)
to be equal to:
 <1> (should resolve to a single destination)
but was not.

(Gradle build scan)

Possible Solutions

  1. Use a custom encoding scheme that accounts both for "/" and whatever "/" encodes to in the scheme.
  2. Use a known encoding scheme like application/x-www-form-urlencoded or URL-safe Base64 (RFC 4648) and either:
    (a) Encode all usernames
    (b) Encode only usernames that contain characters that need to be encoded and flag the username as encoded so that only encoded usernames are decoded

Sample Solution

A sample solution is included in this pull request using 2(b).

Outstanding Questions

Should an explicit encoding be provided for String > byte[] and byte[] > String? If so, what? UTF-8? Or just let the behavior be undefined if running Java in an environment where the default encoding does not support a particular character?

Other Alternatives

  • Document "%2F" as not supported in a username and add a runtime check for invalid user names instead of the current behavior of just not matching message destinations correctly

Any feedback is appreciated!

As a note, I recreated this pull request since I didn't branch my fork of the code first or squash commits as recommended by the contribution guidelines.

Replaces the simple string replacement scheme of "/" -> "%2F" which
breaks when "%2F" is in the original username with a conditional
URL-safe Base64 scheme.
@rstoyanchev

This comment has been minimized.

Copy link
Contributor

rstoyanchev commented Nov 8, 2019

Hi, thanks for raising the issue. I would prefer to reject %2F as an invalid sequence. I presume you have such a case. Are those generated names or how did they end up with that?

@njlaw

This comment has been minimized.

Copy link
Author

njlaw commented Nov 9, 2019

I agree that rejecting %2F as invalid is a reasonable option since it is almost definitely a corner case.

In our case it came up because the site we are developing is multi-tenanted and we want the username to represent a unique user-tenant combination, so the username in Spring is a combination of the tenant’s OAuth2 issuer and their subject. The issuer includes slashes and may contain our chosen delimiter, so we were URL encoding it before appending the delimiter and subject.

I’m not very familiar with the codebase here, but if you can point me in the right direction, I’m glad to add a rejection message or assertion for the character sequence %2F.

@rstoyanchev

This comment has been minimized.

Copy link
Contributor

rstoyanchev commented Nov 11, 2019

Okay thanks for elaborating.

if you can point me in the right direction, I’m glad to add a rejection message or assertion for the character sequence %2F.

This could probably be rejected at the point of trying to subscribe to a user destination. It could also be checked and rejected when trying to send a message in SimpMessagingTemplate just before replacing "/" with "%2F".

@rstoyanchev rstoyanchev self-assigned this Nov 11, 2019
@rstoyanchev rstoyanchev added this to the 5.2.2 milestone Nov 11, 2019
@rstoyanchev rstoyanchev changed the title Handle %2F in simp messaging usernames Reject "%2F" as an invalid sequence in simp messaging usernames Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.