Project

General

Profile

Bug #1989

Blogs can be created with subdomain names > 63 characters, breaking stuff

Added by Sarah Morgano over 6 years ago. Updated over 6 years ago.

Status:
Resolved
Priority name:
Normal
Category name:
BuddyPress (misc)
Target version:
Start date:
2012-07-11
Due date:
% Done:

0%

Estimated time:

Description

Robert Duncan reported being associated with the following blog: schoolofpublicaffairshighereducationexecutivecertificateprogramineducationalresearch.commons.gc.cuny.edu, but notes that he never joined the group it is associated with: Higher Education Certificate Program at Baruch.

I joined the group to do some testing and noticed a couple of things:

  • The group blog is a dead link.
  • When you leave the group, the group blog is still listed under "my sites."

Robert would like to be removed from the blog. He also noted that many member of the Games group also have the blog listed under their profiles (though I'm not sure if they joined the group).

I am not sure who created the group, but they don't even have a real name, just "School of Public Affairs Office of Executive Programs." Seems a little strange.

bp-subdomain-maxlength-REV1.diff (1.1 KB) bp-subdomain-maxlength-REV1.diff Dominic Giglio, 2012-09-24 11:19 PM
wp-subdomain-maxlength-REV1.diff (1.25 KB) wp-subdomain-maxlength-REV1.diff Dominic Giglio, 2012-09-24 11:19 PM
Long_Subdomains_Screen_Shot.png (113 KB) Long_Subdomains_Screen_Shot.png Dominic Giglio, 2012-09-24 11:19 PM
Screen_Shot_2012-10-08_at_11.18.39_PM.png (133 KB) Screen_Shot_2012-10-08_at_11.18.39_PM.png Dominic Giglio, 2012-10-08 11:21 PM

History

#1 Updated by Matt Gold over 6 years ago

  • Category name set to BuddyPress (misc)
  • Status changed from New to Assigned
  • Assignee set to Boone Gorges
  • Target version set to 1.3.17

#2 Updated by Boone Gorges over 6 years ago

  • Assignee changed from Boone Gorges to local admin

There appears to be a more general issue with the length of subdomain names, which is probably at the root of the problem here. 63 characters appears to be the upper limit:

Resolves (63) - http://abcdefghijklmnopqrstuvwxyabcdefghijklmnopqrstuvwxyabcdefghijklm.cdev.gc.cuny.edu
Doesn't resolve (64) - http://abcdefghijklmnopqrstuvwxyabcdefghijklmnopqrstuvwxyabcdefghijklmn.cdev.gc.cuny.edu

It looks like this might be a DNS-level issue (see 2.3.1 at http://www.faqs.org/rfcs/rfc1035.html). I don't know enough about it to say whether or not the restriction can be subverted at the CUNY level. André, could you give some feedback on this?

If we can't work around it at the hosting level, we'll probably have to go back, find the instances of these broken URLs, and fix them somehow (in which case this ticket should be assigned back to me for the moment).

#3 Updated by local admin over 6 years ago

  • Assignee changed from local admin to Boone Gorges

André, could you give some feedback on this?

Sorry, you can't haz > 63 chars domain name (RFC 1034, section 3.1)

#4 Updated by Boone Gorges over 6 years ago

  • Subject changed from Higher Education Certificate Program at Baruch Group Blog to Blogs can be created with subdomain names > 63 characters, breaking stuff
  • Assignee changed from Boone Gorges to Dominic Giglio
  • Target version changed from 1.3.17 to 1.5
  • Severity set to Normal

OK, thanks, André. Strange that WP doesn't prevent this out of the box. I did a quick search on '63' and '64' through WP's codebase and didn't find any such blocks anywhere (though I could have missed something).

So I guess we have to come up with a remediation strategy:
1) Enforce the 63 character limit on the front end, in all places where blogs can be created. This includes:
- Dashboard > Sites > Add New
- Blogs > Add New
- Groups > Groupblog > Create new blog
2) In instances where a blog with a supersized domain name has already been created, do the following:
- Truncate the domain in wp_blogs to something <= 63 characters
- Find all places in the database where the blog is mentioned by domain name (not sure if there are any - maybe in wp_bp_groups_groupmeta, where the groupblog association happens) and swap the name out, as appropriate.

Since this is not a regression (it's been this way forever), it's not urgent to fix at this moment. If we can sneak it into 1.4, it'd be great, but I don't think it's going to happen. Dom, I've assigned the task to you. Please prepare the items in (1) in a topic branch, and write a script for (2) (which you can keep under version control as well of course). Try to do all of (1) in cac-functions.php (or as a plugin) but if you find a place where something should really be fixed upstream in BP-Groupblog or BP or WP, let me know and we can also submit some patches.

#5 Updated by Dominic Giglio over 6 years ago

Just for the fun of it, I'm gonna answer in reverse order:

2) In instances where a blog with a supersized domain name has already been created

- I think I'm just gonna take some of the relevant code from domain_change.phpsh and rework it so we can pull long subdomains and truncate them throughout the DB. But I need a little more time with this. Will update again once I've got something up and running.

1) Enforce the 63 character limit on the front end, in all places where blogs can be created

- Groups > Groupblog > Create new blog: I've created a new topic branch where I've updated the JS that I wrote to alert users to blog names that are in excess of 15 characters. It now checks if the length is also beyond 63 characters and displays a separate warning. It also hides the "Next" button so a user cannot proceed until they've reduced the number of characters.

- Blogs > Add New: This was already taken care of in the existing code. In /bp-nelo/blogs/create.php the template calls cac_show_blog_signup_form(). That function is defined in bp-custom and calls cac_blogs_signup_blog(). The input element that it outputs has a maxlength="50" attribute. Which prevents a user from entering a subdomain blog name in excess of 50 characters. Let me know if you'd like me to change this to 63. This input has the same ID as the input for creating a new group blog so my JS also works on this page. It will warn the user when a blog name exceeds 15 characters.

- Dashboard > Sites > Add New: I think the simplest fix for this would actually be a small patch submitted to WP core. The file responsible for rendering the inputs for a new site is /wp-admin/network/site-new.php. A small patch that adds maxlength="(50 or 63)" to the input on line 126 would not only fix our issue but it would benefit the entire WP community as a whole.

Let me know if you want me to put together a diff and open a ticket on trac.

#6 Updated by Dominic Giglio over 6 years ago

Boone,

It's been a while since we've talked about subdomains but I wanted to get something done on this cause I think we can squeeze it somewhere into 1.4.x.

This update is an extension of my last one, so please re-read that if you haven't been on this page in a while.

I've created a single file plugin that adds a settings page to the Network Sites menu to display blogs with subdomains larger than 63 characters. It doesn't actually fix the problem, but it should be able to give us an idea of how many blogs we have that are violating this limit.

As I mentioned in my previous update, the js I wrote to warn users of blog names that are longer than 15 characters has been expanded to help prevent blog names from exceeding 63 characters going forward.

I've also created (and attached) two patches that add maxlength="63" to the BP and WP inputs that allow users to create blogs/sites on a subdomain install. This should provide further protection for us and the rest of the community going forward. Let me know if you'd like me to move the js I wrote for our site into the BP patch so it could possibly become part of core as well. This will be my first WP patch on trac so let me know if you see anything wrong with the patch or if I should expand the changes to encompass more feature changes or updates.

While the plugin I wrote doesn't actually "fix" the problem, it will allow us to identify the problem domains, and if there are only a handful, correct them individually. The plugin links to each blog's Network Sites page where the domain can be truncated by hand. If it turns out that we have a tremendous amount of problem domains, I will build into the plugin the ability to just click a button to trim the appropriate characters off the end of each domain in the DB.

Here are the commits for my 'long-subdomains' topic branch: https://github.com/castiron/cac/commits/long-subdomains

I've also attached a screenshot of what the plugin looks like when it's activated.

#7 Updated by Boone Gorges over 6 years ago

  • Target version changed from 1.5 to 1.4.8

Thanks for digging this one back up, Dom :)

The plugin looks great - very useful. Please go ahead and put it directly in the 1.4.x branch, so that we can get a sense sooner rather than later of the kind of remediation that will be required. (As an aside, you can now probably understand now just how annoying it is to build tables in the WP back end. There's a core class WP_List_Table which should, in theory, be easily extensible for this sort of purpose, but in practice it is harder than just building the markup yourself. Groan. I wrote a couple plugins to help me to do this sort of thing - not necessarily relevant for this implementation, but you might find them interesting/helpful down the road. Google "Boone's Pagination" and "Boone's Sortable Columns".)

The JS and markup changes look great too. I'm thinking we should probably move these changes to a 1.4.x release too, since we want to start preventing incorrect blog domains as soon as possible (and this is, arguably, a bugfix rather than an enhancement). I'll put it into 1.4.8, but it can be punted if it's holding up the show (or put in earlier, if it won't hold up your other work on those milestones - I'll leave it up to you).

The BP patch looks good. Please open a BP Trac ticket and briefly describe the problem. The first thing I'll do when it's up is to look back through the log (using Trac's nice 'Annotate' feature, or git blame) to see when the maxlength attr was set to 50 in the first place - I want to make sure there's no good reason for it being the way it is. So, if you feel like doing this before posting the Trac ticket to give some background, you may - but I'll probably do it myself either way, just so that I have a full understanding myself :)

As for the WP ticket, a couple of suggestions:
- Consider removing the CSS adjustment. Your patch is more likely to be accepted if it just does one small thing
- Search through WP's Trac to see whether this has ever been discussed. This is a tricky one to search, but some keywords might be '63' and 'maxlength' and 'blog[domain]'.
Please be sure to link to the WP ticket when you've posted it, as I'd like to add myself to the CC list.

Thanks for your work on this, Dom!

#8 Updated by Dominic Giglio over 6 years ago

Boone,

I'm in the middle of taking care of all the to do's in your last update, but I have a quick question about proper merging again, so I don't mess anything up:

This long-subdomains topic branch has been around for a while. I'm pretty sure it was originally branched off of master. I merged master back into it before begining to work on it again a few days ago. Here is the output of checking out 1.4.x and merging long-subdomains into it:

[ Voyager-humanshell ~/sites/wordpress/commons/cac (1.4.x) ]
 $ git merge long-subdomains 
Auto-merging wp-content/plugins/bp-custom.php
Merge made by the 'recursive' strategy.
 wp-content/plugins/bp-custom.php                                       |   2 +-
 wp-content/plugins/bp-reply-by-email/bp-rbe-core.php                   |   5 +-
 wp-content/plugins/bp-reply-by-email/includes/bp-rbe-extend-bpdocs.php | 541 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 wp-content/plugins/bp-reply-by-email/loader.php                        |  23 ++++
 wp-content/plugins/cac-livestream/balloons.jpg                         | Bin 0 -> 535652 bytes
 wp-content/plugins/cac-livestream/jwplayer.js                          |   1 +
 wp-content/plugins/cac-livestream/livestream.php                       |  65 +++++++++++
 wp-content/plugins/cac-livestream/player.swf                           | Bin 0 -> 113505 bytes
 wp-content/plugins/cac-long-subdomains.php                             | 114 +++++++++++++++++++
 wp-content/themes/bp-nelo/_inc/js/cac-custom-js.js                     |  34 +++++-
 10 files changed, 779 insertions(+), 6 deletions(-)
 create mode 100644 wp-content/plugins/bp-reply-by-email/includes/bp-rbe-extend-bpdocs.php
 create mode 100644 wp-content/plugins/cac-livestream/balloons.jpg
 create mode 100644 wp-content/plugins/cac-livestream/jwplayer.js
 create mode 100644 wp-content/plugins/cac-livestream/livestream.php
 create mode 100644 wp-content/plugins/cac-livestream/player.swf
 create mode 100644 wp-content/plugins/cac-long-subdomains.php

I'm only asking because of the references to RBE and Livestream. I haven't pushed to the repo yet incase there is something wrong here. Does it look like I've merged something from master into 1.4.x that isn't supposed to be there yet? If everything looks OK I'll push to the repo and finish opening WP and BP trac tickets. If not I'll just git -D my local 1.4.x and then check a new 1.4.x out from origin so I know it's where it's supposed to be. Then you and I can talk about how to get the stuff out of long-subdomains that we need in 1.4.x.

#9 Updated by Dominic Giglio over 6 years ago

Also,

I read through WP_List_Table on the WP Codex Class Reference. It's nice to know it's there, but you're right. Looks like more work than just hard-coding the html structure. I guess if you're building a large plugin that tries to integrate more fully with the admin it would make sense to subclass so you can keep your code cleaner and more modular.

You might want to checkout Piklist, it was created by the WordPress NYC meetup coordinator (Steve Bruner) and a friend of his. It's still pretty new, but I think it might become really popular. It basically abstracts all of the internal WP Admin related functionality (widgets, settings, plugins) into a plugin that you can use to create features and functionality off of. It's an interesting idea and I think really needed for less advanced devs.

I've also create the two tickets...

BP: http://buddypress.trac.wordpress.org/ticket/4560
WP: http://core.trac.wordpress.org/ticket/21994

Finally, I think leaving this in 1.4.8 for now is fine. It will give us some time to get some feedback from the two tickets.

#10 Updated by Boone Gorges over 6 years ago

Does it look like I've merged something from master into 1.4.x that isn't supposed to be there yet?

Yeah. The RBE stuff looks fine, but the cac-livestream stuff shouldn't be in the 1.4.x branch yet https://github.com/castiron/cac/tree/1.4.x/wp-content/plugins I'm guessing that you were on the cac-livestream branch when you created the long-subdomains branch.

Here's what I suggest. Get a new local version of the 1.4.x branch, and use git cherry-pick to get the relevant commits off of the long-subdomains branch (there's only a couple of them, right?).

Piklist looks interesting, thanks for the link!

BP and WP tickets look great. I left a comment on the BP one.

Finally, I think leaving this in 1.4.8 for now is fine. It will give us some time to get some feedback from the two tickets.

Sounds great.

#11 Updated by Dominic Giglio over 6 years ago

OK, first of all, git cherry-pick might be the coolest thing ever invented!! :-)

Just an FYI: it was the merge of master back into the long-subdomains branch that brought cac-livestream into the picture. I merged master into long-subdomains because 1.) I thought that was where I originally branched off of and 2.) I wanted to make sure I was resuming my testing on long-subdomains with the latest codebase. I'm only mentioning it here so you know that cac-livestream is already in master now. If you look at the cac-livestream issue, it's been moved into 1.4.6, so I think it's being included in Monday's scheduled release. No?

The cac-livestream branch was merged into master on 9/15/12 in this commit: 291bc6944

I took your advice and dumped my local 1.4.x branch and checked out a clean one from the repo. I then git cherry-picked the three commits associated with the updated JS (and HTML maxlength attrs) for long subdomain warnings and the new plugin that displays sites with long subdomains in the network admin. I've also removed the long-subdomains topic branch from the repo on Github. For any future work related to subdomains I will just create a new topic branch.

long-subdomains commits in 1.4.x: https://github.com/castiron/cac/commits/1.4.x

#12 Updated by Boone Gorges over 6 years ago

Sounds great! Thanks, Dom.

If the changes are actually already in the 1.4.x branch, should we just move the target version back to 1.4.6? Is there some other aspect of the ticket that we're waiting for?

#13 Updated by Dominic Giglio over 6 years ago

In comment 10, we decided to leave it open for a little bit until we get some feedback on the patches:

Finally, I think leaving this in 1.4.8 for now is fine. It will give us some time to get some feedback from the two tickets.

Sounds great.

If you'd rather move it to 1.4.6 and close it (so we can open new tickets later if needed) I'm OK with that.

#14 Updated by Boone Gorges over 6 years ago

Oh right. I guess I lean toward putting this ticket into 1.4.6 - the milestone should reflect the release where the code first goes live. But it doesn't matter much.

#15 Updated by Dominic Giglio over 6 years ago

Boone,

It's only been 10 days but I was just looking at the BP/WP tickets and I see that the BP ticket looks as though DJPaul added a "commit" keyword to the ticket. Does that mean it's been accepted? And the WP ticket has had some conversation but no resolution. As this is my first WP core patch, should I be commenting there trying to push things along or just maintain patience for now?

#16 Updated by Boone Gorges over 6 years ago

Dom - The BP patch looks fine. It'll be committed sooner or later. Can't say much about the WP stuff - they have their own priority system. I'd suggest waiting it out a bit more.

#17 Updated by Dominic Giglio over 6 years ago

No prob, I just wanted to check and see if there was something I was supposed to be doing but wasn't. :-)

#18 Updated by Dominic Giglio over 6 years ago

Boone,

According to my long subdomains network plugin, we have only three group blogs with names that exceed 63 characters.

There should be no problem fixing them. Should we notify these groups to request alternate names?

I've attached a screenshot showing the blogs in question.

#19 Updated by Boone Gorges over 6 years ago

Should we notify these groups to request alternate names?

Sounds good to me. Dom, do you want to reach out to the admins of these blogs? (You should be able to find the admin emails at Network Admin > Sites > [find the site] > Edit.) I'd recommend that instead of just asking for them to set new names, you actually suggest a new name, and say that you're going to change the name of the site unless you hear otherwise by a certain date. (The fact is that these sites have probably never worked, so it's likely that the admins of the groups are not independently motivated with respect to their groupblog, and in fact probably set it up on accident. So they may not respond, or indeed even understand the request.) Feel free to post a draft of your email here, if you'd like.

#20 Updated by Dominic Giglio over 6 years ago

My email would go something like this:

Dear [ADMIN],

Recent updates to The CUNY Academic Commons site have uncovered and corrected a situation where the URL of a group blog could be created in such a way as to prevent the site from rendering correctly in a web browser. Unfortunately your group: [GROUP_NAME] was affected by this issue. To complete the solution that corrects this, we will be renaming your group blog's address to [NEW_BLOG_ADDRESS] on 10/21/2012. If you would prefer a different name for your group blog please contact us prior to 10/18/2012.

[COMMONS_CONTACT_INFO]

The Commons development team apologizes for this inconvenience and remains committed to identifying and preventing issues such as these whenever possible.

Sincerely

The CUNY Academic Commons Development Team

#21 Updated by Boone Gorges over 6 years ago

Thumbs up, Dom. One request: can you say "we will be changing your blog's address to..." and "If you would prefer a different address..."? I want to avoid the word "name", since "name" can refer to a couple different kinds of things. Other than that, go ahead and send. And thanks!

#22 Updated by Matt Gold over 6 years ago

I think it looks great, too, Dom. Please change the apology line so that it reads:

The Commons development team apologizes for this inconvenience and thanks you for your use of the Commons. Please don't hesitate to get in touch with any questions or concerns you might have.

#23 Updated by Dominic Giglio over 6 years ago

Thanks for the suggestions, here's the final version:


Dear [ADMIN],

Recent updates to The CUNY Academic Commons site have uncovered and corrected a situation where the URL of a group blog could be created in such a way as to prevent the site from rendering correctly in a web browser. Unfortunately your group: [GROUP_NAME] was affected by this issue. To complete the solution that corrects this, we will be changing your blog's address to [NEW_BLOG_ADDRESS] on 10/21/2012. If you would prefer a different address for your group blog please contact us prior to 10/18/2012.

[COMMONS_CONTACT_INFO]

The Commons development team apologizes for this inconvenience and thanks you for your use of the Commons. Please don't hesitate to get in touch with any questions or concerns you might have.

Sincerely

The CUNY Academic Commons Development Team


The only thing I need to know is what you'd like me to insert into [COMMONS_CONTACT_INFO]

#24 Updated by Matt Gold over 6 years ago

Thanks, Dom -- it looks good. I'd just add an email address to the deadline:

please contact us at prior to 10/18/2012.

#25 Updated by Dominic Giglio over 6 years ago

Thanks Matt, that's what I was waiting on, I'll get the emails out tomorrow.

#26 Updated by Dominic Giglio over 6 years ago

Matt, Boone,

I just wanted to run my new, proposed domain names past you before sending the emails. I think the two of you may be more familiar with the groups in question than I.

1.) queenscollegesecondaryeducationandyouthservicestechnologycommittee.commons.gc.cuny.edu
This is the blog for the group: Queens College, Secondary Education and Youth Services Technology Committee

My proposed url would be: queenscollegesecondaryedandyouthservicestechcommittee.commons.gc.cuny.edu

2.) councilofcunychairshealthphysicaleducationrecreationdancegeronotologynutritionalsciences.commons.gc.cuny.edu
This is the blog for the group: Council of CUNY Chairs: Health, Physical Education, Recreation, Dance, Geronotology & Nutrition

My proposed url would be: healthphysedrecreationdancegeronotologynutritionalsciences.commons.gc.cuny.edu

3.) schoolofpublicaffairshighereducationexecutivecertificateprogramineducationalresearch.commons.gc.cuny.edu
This is the blog for the group: Higher Education Certificate Program at Baruch

My proposed url would be: baruchhighereducationcertificateprogram.commons.gc.cuny.edu

I'm wondering if I should be trying to go for acronyms instead of just shortening them to less than 63 characters?

#27 Updated by Raymond Hoh over 6 years ago

Just throwing in my two cents here, but I would definitely go with acronyms. See if the organization already has such a short name in their communications.

For example, the Queens College, Secondary Education and Youth Services Technology Committee already goes by SEYS:
http://www.qc.cuny.edu/Academics/Degrees/Education/SEYS/Pages/default.aspx

#28 Updated by Dominic Giglio over 6 years ago

Great point Ray.

Here's my revised proposals:

1.) queenscollegesecondaryeducationandyouthservicestechnologycommittee.commons.gc.cuny.edu

Becomes: seystechnologycommitte.commons.gc.cuny.edu

2.) councilofcunychairshealthphysicaleducationrecreationdancegeronotologynutritionalsciences.commons.gc.cuny.edu

Becomes: councilofcunychairs.commons.gc.cuny.edu

3.) schoolofpublicaffairshighereducationexecutivecertificateprogramineducationalresearch.commons.gc.cuny.edu

Becomes: spahighereducationcertificateprogram.commons.gc.cuny.edu

Look a little better?

#29 Updated by Matt Gold over 6 years ago

Better, but let's go even shorter/more focused:

1.) queenscollegesecondaryeducationandyouthservicestechnologycommittee.commons.gc.cuny.edu

Becomes: qcseys.commons.gc.cuny.edu (a quick google search turned up the fact that they use SEYS on the QC website: http://www.qc.cuny.edu/Academics/Degrees/Education/SEYS/Pages/default.aspx? )

2.) councilofcunychairshealthphysicaleducationrecreationdancegeronotologynutritionalsciences.commons.gc.cuny.edu

Becomes: cunyhealthchairs.commons.gc.cuny.edu

-- this is a group with one member, btw

3.) schoolofpublicaffairshighereducationexecutivecertificateprogramineducationalresearch.commons.gc.cuny.edu

Becomes: higheredexecutivecertificate.commons.gc.cuny.edu

#30 Updated by Dominic Giglio over 6 years ago

I like all of your updates Matt. I just wanted to bring up one more thought:

The SEYS group is specifically for the Technology Committee, I thought maybe we'd want to specify that in the URL?

qcseystechcommittee.commons.gc.cuny.edu

Maybe?

#31 Updated by Matt Gold over 6 years ago

Sure -- thanks.

#32 Updated by Dominic Giglio over 6 years ago

Great, notification emails with revised urls going out tomorrow.

#33 Updated by Matt Gold over 6 years ago

Thanks, Dom.

#34 Updated by Dominic Giglio over 6 years ago

Emails have been sent.

Matt, Boone, I BCC'd you on each of the 3 emails.

Please let me know if any responses come in to the address as I do not have access to that account.

#35 Updated by Matt Gold over 6 years ago

Great -- thanks, Dom, and will do.

#36 Updated by Dominic Giglio over 6 years ago

Since we haven't heard back regarding the notification emails, and the deadline indicated in my emails was the 20th, I've updated the three blogs that exceeded 63 characters.

Boone,

Since the two tickets:

BP: http://buddypress.trac.wordpress.org/ticket/4560
WP: http://core.trac.wordpress.org/ticket/21994

are still open, should we just keep punting this issue?

#37 Updated by Dominic Giglio over 6 years ago

  • Target version changed from 1.4.8 to 1.4.9

#38 Updated by Boone Gorges over 6 years ago

  • Status changed from Assigned to Resolved
  • Target version changed from 1.4.9 to 1.4.8

Excellent. Thanks, Dom.

Since the problem is more or less fixed for CAC, let's mark this ticket Resolved. We can make a note here if/when the upstream patches are committed.

Also available in: Atom PDF