WordPress.org

Make WordPress Core

Opened 3 months ago

Closed 5 weeks ago

Last modified 5 weeks 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 3 months ago.
Screenshot
51941.patch (1.6 KB) - added by engahmeds3ed 7 weeks ago.
check if application name exists before for this user
error_step_3.PNG (17.0 KB) - added by Boniu91 6 weeks ago.
51941-unique-name-before-after.gif (9.6 MB) - added by hellofromTonya 6 weeks 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 6 weeks ago.
Sanitize name before and after applying PR 898
51941-name-all-whitespaces.gif (7.6 MB) - added by hellofromTonya 6 weeks ago.
Name that is all whitespaces: before and after applying PR 898
51941-schema-minLength.gif (5.9 MB) - added by hellofromTonya 6 weeks ago.
Name field schema minLength = 3: before and after applying PR 898

Change History (37)

@Boniu91
3 months ago

Screenshot

#1 @hellofromTonya
3 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.


7 weeks ago

#3 @hellofromTonya
7 weeks ago

  • Keywords needs-patch good-first-bug added

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

@engahmeds3ed
7 weeks ago

check if application name exists before for this user

#4 @metalandcoffee
6 weeks 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.


6 weeks ago

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


6 weeks 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
6 weeks 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.


6 weeks ago

#9 @xkon
6 weeks 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
6 weeks ago

#10 @Boniu91
6 weeks 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
6 weeks ago

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

#12 @francina
6 weeks 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.


6 weeks ago

#14 @xkon
6 weeks 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
6 weeks 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
6 weeks 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
6 weeks ago

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

@hellofromTonya
6 weeks ago

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

@hellofromTonya
6 weeks ago

Sanitize name before and after applying PR 898

#18 @hellofromTonya
6 weeks ago

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

  • Updates schema to include:
    • minLength of 1
    • 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.

Last edited 5 weeks ago by hellofromTonya (previous) (diff)

#19 @hellofromTonya
6 weeks 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
6 weeks ago

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

@hellofromTonya
6 weeks ago

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

#20 @prbot
6 weeks 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
5 weeks 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
5 weeks 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.


5 weeks ago

#25 @Boniu91
5 weeks 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
5 weeks 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 5 weeks ago by paaljoachim (previous) (diff)

#27 @hellofromTonya
5 weeks 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
5 weeks 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
5 weeks 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
5 weeks 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.