Unique names for Application Password

[Boniu91]

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

[Boniu91]

Screenshot

[hellofromTonya]

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.

[hellofromTonya]

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

[engahmeds3ed]

check if application name exists before for this user

[metalandcoffee]

Thank you for the patch, @engahmeds3ed!

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.

[hellofromTonya]

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

[xkon]

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]

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:

[francina]

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

[xkon]

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 :) .

[TimothyBlynJacobs]

@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.

[xkon]

@TimothyBlynJacobs yes we do use esc_html on outputs, but we'll have to adjust the JS to avoid showing possible returned < 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 :).

[hellofromTonya]

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

[hellofromTonya]

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

[hellofromTonya]

Sanitize name before and after applying PR 898

[hellofromTonya]

@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.

[hellofromTonya]

@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]

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

[hellofromTonya]

Name field schema minLength = 3: before and after applying 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.

[TimothyBlynJacobs]

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.

Merged in ​https://github.com/WordPress/wordpress-develop/commit/e1c98a744b410dd9f31ee80d3f1064fc9d433cd1.

[SergeyBiryukov]

In 50057:

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

Follow-up to [50030].

See #51941.

[Boniu91]

@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

[paaljoachim]

Testing the latest patch.

Go to the Users > Profile

1. Add a new Application Password test- Created correctly

Works

2. 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.

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

I added test2

4. Revoke one Application Password- It disappears

Works. Revoked test2. Message: Application password revoked.

5. 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.

[hellofromTonya]

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?

[TimothyBlynJacobs]

@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.

[Boniu91]

@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

[TimothyBlynJacobs]

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.