-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Allow MFA devices with /
in names to be deleted from UI
#11011
Conversation
/
in names to be deleted/
in names to be deleted from UI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change fixes the problem with /webapi/mfa/token/:token/devices/:devicename
endpoint, but does not address other endpoints with exact same problem e.g. /webapi/sites/:site/desktops/:desktopName
.
I believe the proper fix should be done at the library level, e.g. by forking httprouter
and applying julienschmidt/httprouter#209 which was raised to address this exact issue, or in any other way which ensures syntax :param
works with encoded parameter values.
Doing so will also work towards resolving #10576
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally agreed with @Tener that we should try to fix this by properly handling escaped characters
DeviceName: p.ByName("devicename"), | ||
DeviceName: p.ByName("devicename")[1:], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this change do?
_, err = clt.Delete(ctx, pack.clt.Endpoint("webapi", "mfa", "token", privilegeToken, "devices", enc)) | ||
require.NoError(t, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we're here testing this, any cases where there should be an error? Can you try to delete a device that doesn't exist, or you don't have permissions for?
I went ahead and tested (#11068) that the comprehensive fix appears to be to:
What do you think? To be clear, I think we want to reference our own fork of |
@probakowski @Tener Hey guys, if we're going in a different direction like @Tener mentioned, should this one be closed? |
Yeah, I think this makes sense. The tests are definitely useful to show the issue is fixed though. |
Yes, that makes sense, I'll close this one |
Currently MFA devices with
/
in names can't be removed from UI becausehttprouter
can't match correct route as it sees additional path segments despite:devicename
being correctly escaped (go doesn't treat encoded reserved characters as different from unescaped, deviating from RFC).This change fixes that by using catch-all route instead, which captures all characters past
/device
.