WordPress.org

Making WordPress.org

Opened 2 years ago

Last modified 4 months ago

#1462 reopened defect

Broken Gravatar on Trac

Reported by: SergeyBiryukov Owned by:
Milestone: Priority: normal
Component: Trac Keywords:
Cc:

Description

See #WP29640.

The ticket reporter's username is "let me see...". It's not urlencoded in the profile link, but the link still redirects to the correct URL:

https://profiles.wordpress.org/let me see...https://profiles.wordpress.org/let-me-see/

However, the Gravatar URL is not redirected and displays a broken image:
//wordpress.org/grav-redirect.php?user=let me see...&s=48

Attachments (1)

1462.patch (4.3 KB) - added by ocean90 2 years ago.

Download all attachments as: .zip

Change History (22)

#1 follow-up: @Otto42
2 years ago

That URL seems to work for me. I see his gravatar.

#2 in reply to: ↑ 1 @SergeyBiryukov
2 years ago

Replying to Otto42:

That URL seems to work for me. I see his gravatar.

I see nginx's "403 Forbidden" page when opening the Gravatar URL from ticket description.

#4 @SergeyBiryukov
2 years ago

I'm testing with the latest Chrome and Firefox on Windows.

If I had to guess, it might have to do with a dotorg sandbox.

#5 @swissspidy
2 years ago

I get the same 403 error on OS X.

Works: Safari
Broken: Chrome, Firefox, Opera

#6 follow-up: @ocean90
2 years ago

This is a known issue, the spaces are breaking HiDPI avatars too.

#7 in reply to: ↑ 6 @SergeyBiryukov
2 years ago

Replying to ocean90:

This is a known issue, the spaces are breaking HiDPI avatars too.

I think it's the dots rather than spaces:

  • https://wordpress.org/grav-redirect.php?user=let-me-see...&s=48 has the same issue (403).
  • https://wordpress.org/grav-redirect.php?user=let-me-see&s=48 works as expected.

#8 @ocean90
2 years ago

Both. :)

#9 @SergeyBiryukov
2 years ago

Should we use the sanitized username that Profiles redirect to, i.e. let-me-see instead of let me see...?

@ocean90
2 years ago

#10 @ocean90
19 months ago

In 3140:

Trac: Use user's nicename for gravatars and profile links.

Adds wporg_sanitize_user_nicename() which simulates bb_sanitize_with_dashes() from bbPress.

See #1462.

#11 @ocean90
19 months ago

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

#12 @stephdau
13 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

@ocean90 : I think dot (.) is missing from the regex in r3140 (str = re.sub(r'[^%a-z0-9\x80-\xff _-]', '', str) ), leading to the user henry.wright to get the Gravatar for henrywright on Trac: https://href.li/?https://core.trac.wordpress.org/ticket/38486

The generated grav redirect URL is:
https://href.li/?https://wordpress.org/grav-redirect.php?user=henrywright&s=96

Leading to the wrong hash:
https://secure.gravatar.com/avatar/0f6a8d63a73d65a3e5418b3040bc9b25?s=96&d=retro

This was reported to us (Automattic) by the user.

Looks like the Trac integration should match what grav-redirect.php is itself doing: [^a-z0-9_. @-].

#13 follow-up: @ocean90
13 months ago

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

@stephdau The regex is fine because dots are not allowed in user_nicename. That this will produce a wrong gravatar for a few people is a known issue and can be fixed case-by-case, see https://wordpress.slack.com/archives/meta-tracdev/p1477221873000365 for a similar report.

#14 @stephdau
13 months ago

Hmmm, ok then.

#15 in reply to: ↑ 13 ; follow-up: @mdawaffe
13 months ago

Replying to ocean90:

@stephdau The regex is fine because dots are not allowed in user_nicename. That this will produce a wrong gravatar for a few people is a known issue and can be fixed case-by-case, see https://wordpress.slack.com/archives/meta-tracdev/p1477221873000365 for a similar report.

Trying to look things up by user_nicename from Trac seems broken.

  • grav-redirect.php looks up the user by user_login first, then falls back to user_nicename.
  • user_login is what we display publicly on Trac (as far as I can tell).
  • Trac's wporg_sanitize_user_nicename() cannot determine a user_nicename from a user_login. Only DB access can do that. For example, in this case (as in the one referenced at https://wordpress.slack.com/archives/meta-tracdev/p1477221873000365), there is a user_nicename "conflict": user_login=henry.wright, user_nicename=henrywright v. user_login=henrywright, user_nicename=henrywright-1. In the case discussed on Slack, both accounts were (supposedly) owned by the same person, so we could edit the DB at will. That's not always going to be the case.

In this specific instance, if grav-redirect.php looked things up by user_nicename first (then fell back to user_login), we'd get the correct Gravatar, but that will just break things in some other way (if user_login=henrywright creates a Trac ticket, for example).

It seems like we should just look things up by user_login all the time: it's what Trac uses internally and publicly and is always predictable.

#16 @dd32
13 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I'm going to echo @mdawaffe here

We should encode user_logins if needed, and if .. is causing issue, we should either change affected user_logins (as harsh as that sounds), disable that check for this URL, or find a way to encode it (unfortunately it looks like %2e triggers the check too).

I'd rather have this broken for users with insane user_logins than have it broken for those wish clashing user_nicenames - there's about 20k users with slugs which don't match tracs current slug generation, about 500 with .. in their logins, not going to check spaces but they're easily dealt with by encoding them.

Last edited 13 months ago by dd32 (previous) (diff)

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


9 months ago

#18 @henry.wright
9 months ago

If you take a look at the "Notifications" section below, you'll see my Gravatar is displayed (I'm watching this ticket). Contrast that with my photo on this comment. Not sure if that helps but thought I'd mention.

Last edited 9 months ago by henry.wright (previous) (diff)

#19 @mdawaffe
9 months ago

@dd32, in lieu of a real fix, can you work your DB magic on @henry.wright's accounts? (Can we verify both accounts belong to the same person somehow?)

Account 1:
user_login=henry.wright
user_nicename=henrywright

Account 2:
user_login=henrywright
user_nicename=henrywright-1

He says:

I use Account 1 so would like to keep that. Account 2 can be deleted if that makes things easier. Perhaps Account 1 could end up looking like this?

user_login=henrywright
user_nicename=henrywright

#20 in reply to: ↑ 15 @mdawaffe
9 months ago

Replying to mdawaffe:

In the case discussed on Slack, both accounts were (supposedly) owned by the same person

Sorry - I didn't see https://wordpress.slack.com/archives/C0C89GD35/p1477230539000375 earlier. I didn't mean to imply you weren't being careful :)

This ticket was mentioned in Slack in #meta by sergey. View the logs.


4 months ago

Note: See TracTickets for help on using tickets.