WordPress.org

Make WordPress Core

Opened 10 months ago

Closed 8 months ago

Last modified 8 months ago

#51941 closed enhancement (fixed)

Unique names for Application Password

Reported by: Boniu91 Owned by: boniu91
Milestone: 5.7 Priority: normal
Severity: normal Version: 5.6
Component: Application Passwords Keywords: has-patch needs-testing has-unit-tests
Focuses: administration Cc:

Description

The Application Password names should be unique. It might be worth mentioning that the field is not sanitized in any way.

Attachments (7)

image (5).png (47.3 KB) - added by Boniu91 10 months ago.
Screenshot
51941.patch (1.6 KB) - added by engahmeds3ed 8 months ago.
check if application name exists before for this user
error_step_3.PNG (17.0 KB) - added by Boniu91 8 months ago.
51941-unique-name-before-after.gif (9.6 MB) - added by hellofromTonya 8 months ago.
Before and After applying PR 898: Unique name handling, admin notice, and HTTP 409 status
51941-sanitize-name-before-after.gif (5.4 MB) - added by hellofromTonya 8 months ago.
Sanitize name before and after applying PR 898
51941-name-all-whitespaces.gif (7.6 MB) - added by hellofromTonya 8 months ago.
Name that is all whitespaces: before and after applying PR 898
51941-schema-minLength.gif (5.9 MB) - added by hellofromTonya 8 months ago.
Name field schema minLength = 3: before and after applying PR 898

Change History (37)

@Boniu91
10 months ago

Screenshot

#1 @hellofromTonya
10 months ago

  • Component changed from Users to Application Passwords
  • Milestone changed from Awaiting Review to 5.7

Hello @Boniu91,

Thank you for identifying this during today's 5.6 testing scrub as well as creating this ticket.

Moving into 5.7 per @TimothyBlynJacobs comment on Slack:

We don’t enforce unique names and pagination is not currently implemented. Both are 5.7 things. Would be great to open separate tickets for each.

This ticket was mentioned in Slack in #core-restapi by hellofromtonya. View the logs.


8 months ago

#3 @hellofromTonya
8 months ago

  • Keywords needs-patch good-first-bug added

Timothy suggests applying a patch in WP_Application_Passwords::create_new_application_password here.

@engahmeds3ed
8 months ago

check if application name exists before for this user

#4 @metalandcoffee
8 months ago

  • Keywords has-patch needs-testing needs-unit-tests added; needs-patch good-first-bug removed

Thank you for the patch, @engahmeds3ed!

This ticket was mentioned in Slack in #core-restapi by hellofromtonya. View the logs.


8 months ago

This ticket was mentioned in PR #898 on WordPress/wordpress-develop by hellofromtonya.


8 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

Trac ticket: https://core.trac.wordpress.org/ticket/51941

  • Updates existing test
  • Adds new unit tests for WP_Application_Passwords::create_new_application_password
  • Adds new unit tests for WP_Application_Passwords:: user_application_name_exists

Note: new tests follows the organizational and naming convention in the handbook.

#7 @hellofromTonya
8 months ago

@Boniu91 Can you retest this please with the PR applied and share results?

This ticket was mentioned in Slack in #core by francina. View the logs.


8 months ago

#9 @xkon
8 months ago

Would it be beneficial/ok to adjust the scope of this ticket as well to also add sanitization and not allowing seemingly empty names by using 1 space character?

@Boniu91
8 months ago

#10 @Boniu91
8 months ago

Tested steps:

  1. Go to the Users > Profile
  2. Add a new Application Password test- Created correctly
  3. Add a new Application Password called test again- Notification appears, we're not able to
  4. Add a new Application Password called test2- Worked as expected
  5. Revoke one Application Password- It disappears
  6. Revoke all Application Password- They disappear

@hellofromTonya In step 3 I'm getting an error in the JS console:

#11 @francina
8 months ago

Tested as per @Boniu91 steps, works for me on macOS Big Sur 11.1 and Safari 14.0.2

#12 @francina
8 months ago

  • Owner set to boniu91
  • Status changed from new to assigned

This ticket was mentioned in Slack in #core-restapi by hellofromtonya. View the logs.


8 months ago

#14 @xkon
8 months ago

I had a look as discussed on slack during testing earlier today and as @Boniu91 mentions in the ticket there's no validation. Everything passes as is in the DB and we result on instances like s:4:"name";s:29:"<script>alert('hey')</script>"; s:4:"name";s:18:"<?php echo 'test';" so on so forth.

I'm not entirely sure but @TimothyBlynJacobs I guess it wouldn't hurt to pass the $args['name'] via a sanitize_text_field() in create_new_application_password() to keep the db clean, correct?

This would also help with other chars as / or (space) that allows applications with visually "no name" to be added.

Since it was mentioned and it's a small change as it seems maybe we can add it here as well directly.

Thanks to @pbiron as well for cross checking :) .

#15 @TimothyBlynJacobs
8 months ago

@xkon We escape everywhere on output, right? But yeah that seems sensible. Make sure to also catch update_application_password.

For empty, or visually empty, strings I think we should also express that in the schema. We can set a minLength keyword and use the pattern keyword to restrict entering just whitespace characters.

#16 @xkon
8 months ago

@TimothyBlynJacobs yes we do use esc_html on outputs, but we'll have to adjust the JS to avoid showing possible returned &lt; and other characters.

@hellofromTonya would you like to update your PR for the changes mentioned in comments 14 & 15? I see that you already pushed a trim() also in the PR but if we pass the Name via sanitize_text_field it won't be needed afaic.

If it's easier we can split the extra changes mentioned on a new task and I can also work on them tomorrow, to commit this "as is" since tests are OK :).

#17 @hellofromTonya
8 months ago

@xkon Already working on PR updates to account for both empty/whitespace and sanitizing/escaping.

@hellofromTonya
8 months ago

Before and After applying PR 898: Unique name handling, admin notice, and HTTP 409 status

@hellofromTonya
8 months ago

Sanitize name before and after applying PR 898

#18 @hellofromTonya
8 months ago

@TimothyBlynJacobs @xkon Updated PR 898 to include the following:

  • Updates schema to include:
    • minLength of 3
    • pattern to exclude a string with all whitespaces (empty string)
  • sanitize_text_field( $args['name'] ) when creating a new password
  • Adds HTTP 409 for WP_Error unique name
  • Adds HTTP 400 for WP_Error empty name
  • Adds test cases for changes

Ready for your code review. Also adding gifs to show before and after for testing.

Version 0, edited 8 months ago by hellofromTonya (next)

#19 @hellofromTonya
8 months ago

@Boniu91 Yes, display a console error when an error occurs is the expected behavior. However, I adjusted the HTTP status codes:

  • HTTP 400 for empty name
  • HTTP 409 for non-unique name

@hellofromTonya
8 months ago

Name that is all whitespaces: before and after applying PR 898

@hellofromTonya
8 months ago

Name field schema minLength = 3: before and after applying PR 898

#20 @prbot
8 months ago

hellofromtonya commented on PR #898:

we need to make sure to apply this same level of validation/sanitization in update_application_password which allows updating the name for an App.

@TimothyBJacobs Good catch! Addressed this in commit https://github.com/WordPress/wordpress-develop/pull/898/commits/d20b4083b688a765697d715e469ff29fba555bd5. Commit https://github.com/WordPress/wordpress-develop/pull/898/commits/1576e78cc8e9aa43d1ab8f3a0f17549efd720ee1 cleans up my isolation ticket test.

#21 @TimothyBlynJacobs
8 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 50030:

App Passwords: Improve validation and sanitization of the application name.

Application names are now required to be unique and cannot contain solely whitespace characters. Additionally, invalid characters are now stripped from the application name using sanitize_text_field().

Props Boniu91, hellofromTonya, engahmeds3ed, xkon, francina.
Fixes #51941.

#23 @SergeyBiryukov
8 months ago

In 50057:

Docs: Update documentation for WP_Application_Passwords::application_name_exists_for_user() per the documentation standards.

Follow-up to [50030].

See #51941.

This ticket was mentioned in Slack in #core by monikarao. View the logs.


8 months ago

#25 @Boniu91
8 months ago

@hellofromTonya When trying to add a space as an Application Name, we're getting the following error:
Invalid parameter(s): name

I think it should be more clear for the users.

To reproduce:

  1. Go to the Users > Profile
  2. Try to add space (white sign) in the Application Password Name field
  3. Invalid parameter(s): name is displayed

#26 @paaljoachim
8 months ago

Testing the latest patch.

Go to the Users > Profile

  1. Add a new Application Password test- Created correctly

Works

  1. Add a new Application Password called test again- Notification appears, we're not able to

I am seeing this message: Each application name should be unique.

  1. Add a new Application Password called test2- Worked as expected

I added test2

  1. Revoke one Application Password- It disappears

Works. Revoked test2. Message: Application password revoked.

  1. Revoke all Application Password- They disappear

Works. Revoked test. Message Application password revoked.

In Chrome developer tools Console I see this error:

POST http://localhost:8889/index.php?rest_route=/wp/v2/users/1/application-passwords&_locale=user 409 (Conflict)                                           jquery.js?ver=3.5.1:10099 
send @ jquery.js?ver=3.5.1:10099
ajax @ jquery.js?ver=3.5.1:9682
jQuery.ajax @ jquery-migrate.js?ver=3.3.2:305
apiRequest @ api-request.js?ver=5.7-alpha-50000-src:22
(anonymous) @ application-passwords.js?ver=5.7-alpha-50000-src:49
dispatch @ jquery.js?ver=3.5.1:5429
elemData.handle @ jquery.js?ver=3.5.1:5233

Refreshing the page the error went away.

Last edited 8 months ago by paaljoachim (previous) (diff)

#27 @hellofromTonya
8 months ago

Thanks @Boniu91 for retesting. Yes, a space (a series of spaces) is an empty string, which is now an invalid name.

And you're right, the error is generic:

Invalid parameter(s): name

The error comes from the schema.

What do you think would make the error message more understandable for users?

#28 @TimothyBlynJacobs
8 months ago

@paaljoachim That is expected behavior.

Regarding the error message, JSON Schema doesn't let us set custom error messages at the moment. That's something I'm looking to change. But the more specific error message, %1$s does not match pattern %2$s., wouldn't be any clearer.

While ideally we would be able to provide a cleaner error message, IMO it is sufficiently obvious to the user that an empty application name is invalid.

#29 @Boniu91
8 months ago

@hellofromTonya @TimothyBlynJacobs If it's not possible to find the reason why the pattern is not matching, maybe it would be good to output something like:
Application Password Name is not correct, empty text fields, spaces, etc. are not allowed

#30 @TimothyBlynJacobs
8 months ago

Well that's the thing, we can't specify a custom error message to be used when a particular schema keyword, in this case pattern, fails to validate. This is how all REST API endpoints operate, so I don't think it is worth doing manual introspection just for this use case. When we add support for custom error messages we would certainly apply it here as well.

Note: See TracTickets for help on using tickets.