dw_dev_training | Bug 858 fix walkthrough: Private messages strip (rather than escape) HTML

This is a walkthrough for how I tackled Bug 858 - Private messages strip (rather than escape) HTML. I'm not a very experienced DW developer, so you're going to see me do a lot of bumbling around as I try to figure things out. But, I'm going to document all of my wrong turns and head scratchings in the hopes that seeing the whole process laid out from start to finish might help people who aren't sure how to go about things. I will note ahead of time that some of the issues I run into won't happen to people on Dreamhacks--they're due to my setup not being completely right.

To be honest, this walkthrough is a bit advanced and best suited for people already comfortable with the command line and coding. I'm going to do another one soon and hope that it doesn't go as awkwardly as this one did.

Table of contents:

Preparation

First, I assign the bug to myself on Bugzilla by putting my email into the assigned field, setting the status to "ASSIGNED", and committing. If I decide later that I can't do the bug, then I can put "nobody@dreamwidth.org" instead. But, for now, people will know that this is a bug I'm working on.

Now, I'm one of the people who works in the Mercurial repository of dw-free and then syncs the live version of the code back with the code in the repository. Because of this, I go into the dw-free repository to start out with:

cd cvs/dw-free

I check for any patches I might already have applied or been working on. I did one recently--maybe I didn't unapply it yet. So I run this command:

hg qapplied

No applied patches are listed. If there were, I would have used hg qpop until my patch queue was clean. I want to be working on a clean slate.

Next, I update my Dreamwidth install. I want to be working on the latest copy of the Dreamwidth code. The instructions I use to do this are on the Dev Maintenance wiki page.

Now I make a new patch for the changes I am about to make with a name that will let me know what it's about:

hg qnew message_escape_html

Finding the relevant code to make changes on

Now I have to find out where the private message code is. I do an rgrep for the phrase "private message" to start out with.

dw@li53-94:~/cvs/dw-free$ rgrep "private message" * bin/upgrading/en.dat:userlinkbar.sendmessage.title=Send a private message to this user bin/upgrading/en.dat:userlinkbar.sendmessage.title.self=Send a private message to yourself bin/upgrading/en.dat:userlinkbar.sendmessage.title.cantsendmessage=You cannot send a private message to this user bin/upgrading/en.dat:userlinkbar.sendmessage.title.loggedout=You must be logged in to send a private message to this user cgi-bin/ljprotocol.pl: return fail($err, 305) if $u->statusvis eq 'S'; # suspended cannot send private messages cgi-bin/DW/Logic/ProfilePage.pm: # private message htdocs/inbox/compose.bml.text:.suspended.cannot.send=Suspended accounts are not allowed to send private messages

The line bolded up there looks suspiciously like where I want to be--it's talking about not sending a private message if the account is suspended. I bet that line is in the function that sends messages. So I open it up in vi and search for the phrase "private message".

vi cgi-bin/ljprotocol.pl /private message

There are two places where the strip_html function is being used:

470 my $subject_text = LJ::strip_html($req->{'subject'}); 471 return fail($err, 208, 'subject') 472 unless LJ::text_in($subject_text); 473 474 # strip HTML from body and test encoding and length 475 my $body_text = LJ::strip_html($req->{'body'}); 476 return fail($err, 208, 'body') 477 unless LJ::text_in($body_text);

Now, LJ probably has an escape HTML function already, but what is it? We know from the bug report that other places on the site are escaping HTML. We can go look at those places and see what function there are using. But I'm going to see if I can try and grep it out of the code first instead of trying to figure out where the support code is. I quit vim by entering :q and make a guess:

rgrep "escape_html" *

Gets nothing. The escape function must not be named that. However, what are the functions that end in html? We can grep for html next to an opening parentheses to get a good approximation.

rgrep "html(" *

This gives us a LOT of matches, including all of the strip_html function calls. There are also lots of LJ::ehtml functions:

htdocs/editjournal.bml: $ret .= " <i>" . LJ::ehtml($subj) . "</i>"; htdocs/editjournal.bml: my $event = LJ::ehtml(LJ::durl($res{"events_${i}_event"})); htdocs/editjournal.bml: my $event = LJ::ehtml($POST{'event'}); htdocs/editjournal.bml: $spellcheck_html = $s->check_html(\$event); htdocs/misc/blockedlink.bml: return LJ::ehtml($GET{url}); htdocs/misc/suggest_qotd.bml: my $questions = LJ::ehtml($POST{questions}); htdocs/inbox/compose.bml: my $msg_subject_text = LJ::strip_html($POST{'msg_subject'}); htdocs/inbox/compose.bml: my $msg_body_text = LJ::strip_html($POST{'msg_body'});

Maybe ehtml is the function I am looking for! But how do I check? I know that functions are defined in Perl with "sub FUNCTIONNAME", so I'll try searching for that:

dw@li53-94:~/cvs/dw-free$ rgrep "sub ehtml" * cgi-bin/LJ/S2.pm:sub ehtml cgi-bin/ljtextutil.pl:sub ehtml src/jbackup/jbackup.pl:sub ehtml {

I found it! I go check and see what's around the function:

vi cgi-bin/ljtextutil.pl /sub ehtml

And find this function:

145 # 146 # name: LJ::ehtml 147 # class: text 148 # des: Escapes a value before it can be put in HTML. 149 # args: string 150 # des-string: string to be escaped 151 # returns: string escaped. 152 # 153 sub ehtml 154 { 155 # fast path for the commmon case: 156 return $_[0] unless $_[0] =~ /[&\"\'<>]/; 157 158 # this is faster than doing one substitution with a map: 159 my $a = $_[0]; 160 $a =~ s/\&/&/g; 161 $a =~ s/\"/"/g; 162 $a =~ s/\'/&\#39;/g; 163 $a =~ s//>/g; 165 return $a; 166 } 167 *eall = \&ehtml; # old BML syntax required eall to also escape BML. not anymore.

This is the function I am looking for!

Making the changes

I open up the cgi-bin/ljprotocol.pl file in the repository again and change both of the strip_htmls to ehtmls:

470 my $subject_text = LJ::ehtml($req->{'subject'}); 471 return fail($err, 208, 'subject') 472 unless LJ::text_in($subject_text); 473 474 # strip HTML from body and test encoding and length 475 my $body_text = LJ::ehtml($req->{'body'}); 476 return fail($err, 208, 'body') 477 unless LJ::text_in($body_text);

I save my changes and quit. Then I run the command that saves the current state of my patch:

hg qrefresh

Testing

I go and update the live code to the code in my repository and restart the server:

dw@li53-94:~$ cd $LJHOME dw@li53-94:~$ bin/cvsreport.pl -sync -cvsonly dw@li53-94:~$ sudo /etc/init.d/apache2 restart

Now my server should be running my new changed code.

I need to test my changes. The bug report specifically mentions a case in which the HTML gets stripped: for <3 with a > after it. I'm going to make a test case for this. Since I made both the title and body of the message escaped instead of stripped, I should test them both. I use my system account to send a test message to the bunnies test account on my account:

Subject: Test <3 >!

This is a test of HTML stripping.

Test <3 >!

I then go to the sent box where my sent message should be saved, but I don't see it! Maybe there's a worker responsible for that that I don't know about. I should check and see bunnies' mailbox. But I've forgotten the password to bunnies. Going to the page to reset it throws up this error:

[Error: To use reCAPTCHA you must get an API key from http://recaptcha.net/api/getkey at '/home/dw/htdocs/lostinfo.bml' line 25 @ li53-94]

I'm too lazy to set up reCAPTCHA at the moment, I just want to disable it! A cursorary search of etc/config.pl for CAPTCHA only brings up:

772 #$CAPTCHA_MOGILEFS = 1; # turn this on to put captchas in MogileFS

I know that MogileFS is a filesystem LJ uses for storing lots of individual little files like icons, but I haven't set that up on my system, and it has nothing to do with reCAPTCHA, so it's not what I'm looking for.

So I go to the file /home/dw/htdocs/lostinfo.bml to see what is around that code:

25 if (LJ::is_enabled("recaptcha")) { 26 my $c = Captcha::reCAPTCHA->new; 27 $ret .= $c->get_options_setter({ theme => 'white' }); 28 $ret .= $c->get_html( LJ::conf_test($LJ::RECAPTCHA{public_key}), '', $LJ::IS_SSL ) . " \n"; 29 }

It checks to see if recaptcha is enabled before it throws that error. I want to disable that! So I go back into etc/config.pl and find this array of stuff that is disabled:

177 # turns these from 0 to 1 to disable parts of the site that are 178 # CPU & database intensive or that you simply don't want to use 179 %DISABLED = ( 180 adult_content => 0, 181 blockwatch => 1, 182 'community-logins' => 0, 183 directory => 0, 184 eventlogrecord => 1, 185 feedster_search => 0, 186 free_create => 1, 187 'interests-findsim' => 0, 188 memories => 0, 189 opt_findbyemail => 1, 190 payments => 0, 191 schools => 1, 192 'show-talkleft' => 0, 193 'stats-recentupdates' => 0, 194 'stats-newjournals' => 0, 195 tellafriend => 0, 196 esn_archive => 1, 197 );

So I add recaptcha to it and set it to be disabled:

190 payments => 0, 191 recaptcha => 1, 192 schools => 1,

I make a note: recaptcha should exist in the base config file itself in the repository, although it should probably be enabled by default. But I shouldn't put it in this patch anyway, because we don't want to mix up multiple objectives for the same patch.

I restart the server again:

sudo /etc/init.d/apache2 restart

I can access the page again!

Success. Your password has been mailed.

I note that's bad site copy text. We don't actually mail the password, we email instructions to reset the password. But I know the translation system is a shambling horror of the deep, so I go and double check the site copy live on Dreamwidth itself. It's the same. I make a note to notify the sitecopy team of this.

I reset the password for my test account. The next page on my development server also has something funny with it:

[missing string Success]

I'm not inclined to track this one down at this time, but make a note to check it on the Dreamwidth site itself later. I now use the top login box to log in. Awkwardly, I get forwarded to the same change password page as before, and it now says:

Change Password
Error

One or more errors occurred processing your request. Please go back, correct the necessary information, and submit your data again.

* You've already used this link to reset your password. If you need to change your password again, you'll need to request a new password reset email.

That's a bad practice and probably another thing that should get changed. I make another note. I'm now logged into my test account that I just sent a message to, but bunnies has no messages in its inbox! That's really weird; when I send a message from bunnies back to system it tells me:

Compose Message
Your message has been sent successfully.

From here you can:

* Send a new message
* Return to Inbox
* Return Home

I want to check and see how the message actually gets sent. I go back to cgi-bin/ljprotocol.pl to look at that function for clues:

494 my @msg; 495 BML::set_language('en'); # FIXME 496 497 foreach my $to (@to) { 498 my $tou = LJ::load_user($to); 499 return fail($err, 100, $to) 500 unless $tou; 501 502 my $msg = LJ::Message->new({ 503 journalid => $u->userid, 504 otherid => $tou->userid, 505 subject => $subject_text, 506 body => $body_text, 507 parent_msgid => defined $req->{'parent'} ? $req->{'parent'} + 0 : undef, 508 userpic => $req->{'userpic'} || undef, 509 }); 510 511 push @msg, $msg 512 if $msg->can_send(\@errors); 513 } 514 return fail($err, 203, join('; ', @errors)) 515 if scalar @errors; 516 517 foreach my $msg (@msg) { 518 $msg->send(\@errors); 519 } 520 521 return { 'sent_count' => scalar @msg, 'msgid' => [ grep { $_ } map { $_->msgid } @msg ], 522 (@errors ? ('last_errors' => \@errors) : () ), 523 };

I wonder what that FIXME is there for. Put it on my list of followup items. Looks like it creates a LJ::Message object, and calls the send function on it. I want to find the code for this object. Now, I know that the file I'm looking for is probably in cgi-bin/LJ somewhere and sure enough there is a Message.pm, but somebody who didn't know that could do an rgrep "Message" *. I look at the cgi-bin/LJ/Message.pm and don't really see any helpful clues, so I go ask in #dw_kindergarten.

[3:18pm] foxfirefey: Sooo is there any worker that I need to set up to have sending private messages work? [3:19pm] foxfirefey: Right now, it just tells me that I'm successful when I send one, but the messages never actually appear anywhere after. [3:20pm] exor674: foxfirefey: yeah [3:21pm] exor674: the... esn-fired-event most likely [3:21pm] foxfirefey: Okay, thanks

So, I set up the Schwartz based on The Schwartz Setup wiki page. Then I run:

$LJHOME/bin/worker-manager --debug

On my command line, but I get an error:

Can't locate YAML.pm in @INC (@INC contains: /etc/perl /usr/local/lib/perl/5.10.0 /usr/local/share/perl/5.10.0 /usr/lib/perl5 /usr/share/perl5 /usr/lib/perl/5.10 /usr/share/perl/5.10 /usr/local/lib/site_perl .) at /home/dw/bin/worker-manager line 14.
BEGIN failed--compilation aborted at /home/dw/bin/worker-manager line 14.

Looks like I don't have a needed package installed. I do a search for YAML on the Ubuntu site and find a package called libyaml-perl that's probably what I need. I install it:

sudo apt-get install libyaml-perl

(Afterwards, I make sure to add the libyaml-perl package to Dreamwidth Scratch Installation: Installing necessary packages, so other people with scratch installations won't run into the same mistake.)

This time, I run the worker-manager so that it directs all output to /dev/null (>&) so it doesn't get printed to my terminal, and have it run in the background (&).

$LJHOME/bin/worker-manager --debug >& /dev/null &

Now I go back and test sending my messages. Finally! Message sending works! But, my change didn't take. My happy <3 > still gets stripped. What's going wrong?

Troubleshooting

Maybe there is something in LJ::Message that strips HTML? I go to look at the code that creates a new Message.

12 sub new { 13 my ($class, $opts) = @_; 14 15 my $self = bless {}; 16 17 # fields 18 foreach my $f (qw(msgid journalid otherid subject body type parent_msgid 19 timesent userpic)) { 20 $self->{$f} = delete $opts->{$f} if exists $opts->{$f}; 21 } 22 23 # unknown fields 24 croak("Invalid fields: " . join(",", keys %$opts)) if (%$opts); 25 26 # Handle renamed users 27 my $other_u = LJ::want_user($self->{otherid}); 28 if ($other_u && $other_u->is_renamed) { 29 $other_u = $other_u->get_renamed_user; 30 $self->{otherid} = $other_u->{userid}; 31 } 32 33 my $journalid = $self->{journalid} || undef; 34 my $msgid = $self->{msgid} || undef; 35 36 $self->set_singleton; # only gets set if msgid and journalid defined 37 38 return $self; 39 } 40 41 *load = \&new;

I don't really see anything that would affect my subject or message here. And a search of "strip_html" in this file comes up with nada. Since I don't know object oriented Perl really well, I use this opportunity to get a bit more familiar with some of the stuff I see here. I look up what the bless function does, since I don't recognize it. Evidently that is telling Perl to treat the anonymous hash reference (see: Hash HowTo for more background on hashes and hash references, and perlref - Perl references and nested data structures for more info on making anonymous hash references) as a LJ::Message object. Then, it goes through all of the items in the qw and if they exist, it sets the value for that in our LJ::Message $self and deletes it from the options passed in. The function uses croak to die if there are any values left in $opts, because that means somebody is passing in an argument that isn't recognized. It does a couple other settings and then returns the $self variable as a new LJ::Message. None of this really seems to affect the contents of body or subject, though.

I want to know if this stuff is getting stripped before it hits the database or if it gets into the database but is displayed stripped. So I decide to go into the database and see what they look like as saved messages. To make a really horrible analogy, a database is like a set of spreadsheets. A table in the database is like an individual spreadsheet, and every table has different columns it is composed of. A row in a table corresponds to an individual entry.

mysql -u dw -p dw;

To get a list of the tables in the database, I use this command:

show tables;

I scan down the table names and figure these ones look promising:

| usermsg | | usermsgprop | | usermsgproplist | | usermsgtext |

I think that the text of the messages is what I'm interested in, and those will be in usermsgtext. So I look at the columns for that table:

mysql> show columns from usermsgtext; +-----------+------------------+------+-----+---------+-------+ | Field | Type | Null | Key | Default | Extra | +-----------+------------------+------+-----+---------+-------+ | journalid | int(10) unsigned | NO | PRI | NULL | | | msgid | int(10) unsigned | NO | PRI | NULL | | | subject | varchar(255) | YES | | NULL | | | body | blob | NO | | NULL | | +-----------+------------------+------+-----+---------+-------+ 4 rows in set (0.04 sec)

So, every entry for this table in the database has those four pieces of data. It looks like the unique identifier for each entry is the journalid and the msgid together (based on the "PRI" equalling "primary" in the "Key" of the descriptive table above). msgid probably hooks back up into some other table of messages.

I'm going to look at the content of those pieces of data for last five message texts sent, so I will do a SELECT statement for the last five messages, ordered from biggest (and thus I assume lastest) msgid to smallest.

mysql> select * from usermsgtext order by msgid desc limit 5; +-----------+-------+---------+--------+ | journalid | msgid | subject | body | +-----------+-------+---------+--------+ | 1 | 7 | Test ! | Test ! | | 11 | 7 | Test ! | Test ! | | 1 | 6 | Test ! | Test ! | | 11 | 6 | Test ! | Test ! | | 1 | 5 | Moo! | Moo? | +-----------+-------+---------+--------+ 5 rows in set (0.00 sec)

Interesting things to note here:

* Each message has *two* copies. One for the journal that sends it, and one for the journal that receives it.
* The HTML is still getting stripped before it enters the database. That means it's not an issue with fetching the message for display.

I can now exit the database, having found what I wanted:

exit;

At this point, I kind of want to make sure that ehtml is doing what I think it should do. I know there is a nifty program by

exor674 that emulates a console with all of the DW Perl stuff loaded. I set it up according to the Dev Tools wiki page. Then I can run the ljconsole program:

$LJHOME/bin/ljconsole > my $test = LJ::strip_html("Test <3 >"); $VAR1 = 'Test '; > my $test2 = LJ::ehtml("Test <3 >"); $VAR1 = 'Test <3 >';

Well, strip_html certainly kills my <3 > but ehtml is definitely stripping it and replacing the < and > with their appropriate character entity references. Now I'm wondering what happens during the creation and sending of a message. Is that where this is occuring? I look at the code for making a new message in the function I originally changed and make up an equivalent. I used the journal ids 1 and 11, because those are the IDs in the database table above, so they refer to the system and bunnies accounts. I set the parent message id to undefined and the userpic to undefined, since the code says those are okay if no data exists for those items. I set the body and subject to what I assume their content would be coming out from the ehtml function used above.

> $msg = LJ::Message->new({ journalid => 1, otherid => 11, subject => "Test <3 >", body => "Test <3 >", parent_msgid => undef, userpic => undef }); $VAR1 = bless( { 'userpic' => undef, 'body' => 'Test <3 >', 'otherid' => 11, 'subject' => 'Test <3 >', 'journalid' => 1, 'parent_msgid' => undef }, 'LJ::Message' );

It appears I've successfully created a Message object! Now I want to test and see what the content of the body is:

> $msg->body; $VAR1 = 'Test &lt;3 &gt;';

Oh dear. Somewhere along the line, the content has been escaped again from what I entered. My <s have been changed into &gt;s. That's not what I wanted at all. I try and send it anyway to see what happens, using the send function I saw used in sendmessage:

> $msg->send; Can't locate object method "new" via package "LJ::Event::UserMessageSent" (perhaps you forgot to load "LJ::Event::UserMessageSent"?) at /home/dw/cgi-bin/LJ/Message.pm line 72, line 3.

I guess I need to load that library specifically.

> use LJ::Event::UserMessageSent; > $msg->send; $VAR1 = 1;

Looks like this has successfully sent a message! It arrives and just like it showed before, the subject and body have been overescaped. I go to the web interface and send another message just like it from the site, and receive another message that's been stripped instead of overescaped. I go back into the database to see the actual contents of these messages:

mysql> select * from usermsgtext order by msgid desc limit 5; +-----------+-------+-----------------+-----------------+ | journalid | msgid | subject | body | +-----------+-------+-----------------+-----------------+ | 1 | 10 | Test ! | Test ! | | 11 | 10 | Test ! | Test ! | | 1 | 9 | Test <3 > | Test <3 > | | 11 | 9 | Test <3 > | Test <3 > | | 1 | 8 | Test &apm;lt;3 > | Test <3 > | +-----------+-------+-----------------+-----------------+ 5 rows in set (0.00 sec)

The last one I sent is stripped. But, interestingly enough, the actual entries in the database aren't overescaped.

But now I'm curious. What if I create a message without any escaping or stripping at all? Would that balance out the escaping that's happening afterwards?

> $msg = LJ::Message->new({ journalid => 1, otherid => 11, subject => "Test2 <3 >", body => "Test2 <3 >", parent_msgid => undef, userpic => undef }); $VAR1 = bless( { 'userpic' => undef, 'body' => 'Test2 <3 >', 'otherid' => 11, 'subject' => 'Test2 <3 >', 'journalid' => 1, 'parent_msgid' => undef }, 'LJ::Message' ); > use LJ::Event::UserMessageSent; > $msg->body; $VAR1 = 'Test2 <3 >'; > $msg->send; $VAR1 = 1;

Oh goody! It appears properly escaped in the Inbox now.

And in the database it looks like:

mysql> select * from usermsgtext order by msgid desc limit 2; +-----------+-------+------------+------------+ | journalid | msgid | subject | body | +-----------+-------+------------+------------+ | 1 | 11 | Test2 <3 > | Test2 <3 > | | 11 | 11 | Test2 <3 > | Test2 <3 > | +-----------+-------+------------+------------+ 2 rows in set (0.00 sec)

This is closer to what we want. I go back to the source code file for LJ::Message. I look for instances of ehtml to explain the overescaping and find them:

267 sub subject_raw { 268 my $self = shift; 269 return $self->_row_getter("subject", "msgtext"); 270 } 271 272 sub subject { 273 my $self = shift; 274 return LJ::ehtml($self->subject_raw) || "(no subject)"; 275 } 276 277 sub body_raw { 278 my $self = shift; 279 return $self->_row_getter("body", "msgtext"); 280 } 281 282 sub body { 283 my $self = shift; 284 return LJ::ehtml($self->body_raw); 285 }

So that's where my overescaping comes from. Now I'm thinking it's time to take away the ehtml changes I made.

cd cvs/dw-free vi cgi-bin/ljprotocol.pl

470 my $subject_text = $req->{'subject'}; 471 return fail($err, 208, 'subject') 472 unless LJ::text_in($subject_text); 473 474 # strip HTML from body and test encoding and length 475 my $body_text = $req->{'body'}; 476 return fail($err, 208, 'body') 477 unless LJ::text_in($body_text);

I take out that invalid comment while I'm at it. Then I save the file in vi, and update my changes to my patch and refresh the code the server is running:

hg qrefresh cd $LJHOME bin/cvsreport.pl -sync -cvsonly sudo /etc/init.d/apache2 restart

The message is still stripped, though. This is expected: I was still having stripping problems initially. It's happening before it hits the database, too:

mysql> select * from usermsgtext order by msgid desc limit 2; +-----------+-------+---------+--------+ | journalid | msgid | subject | body | +-----------+-------+---------+--------+ | 1 | 12 | Test3 | Test3 | | 11 | 12 | Test3 | Test3 | +-----------+-------+---------+--------+ 2 rows in set (0.00 sec)

So, now I'm going to trace through the saving of a message to see if I can catch where this is happening. I first look at the send function of LJ::Message:

43 sub send { 44 my $self = shift; 45 my $errors = shift; 46 47 return 0 unless ($self->can_send($errors)); 48 49 # Set remaining message properties 50 # M is the designated character code for Messaging counter 51 my $msgid = LJ::alloc_global_counter('M') 52 or croak("Unable to allocate global message id"); 53 $self->set_msgid($msgid); 54 $self->set_timesent(time()); 55 56 # Send message by writing to DB and triggering event 57 if ($self->save_to_db) { 58 $self->_send_msg_event; 59 $self->_orig_u->rate_log('usermessage', $self->rate_multiple) if $self->rate_multiple; 60 return 1; 61 } else { 62 return 0; 63 } 64 }

I don't see anything here that affects the message subject or body, but there is a save_to_db function that might be doing this before it saves the object. (I end up trying to find out why there are underscores at the beginning of certain function names; my googling helps me find out it's a convention for designating a private class method. That means it's not supposed to be used by code outside of the class.)

76 # Write message data to tables while ensuring everything completes 77 sub save_to_db { 78 my ($self) = @_; 79 80 die "Missing message ID" unless ($self->msgid); 81 82 my $orig_u = $self->_orig_u; 83 my $rcpt_u = $self->_rcpt_u; 84 85 # Users on the same cluster share the DB handle so only a single 86 # transaction will exist 87 my $same_cluster = $orig_u->clusterid eq $rcpt_u->clusterid; 88 89 # Begin DB Transaction 90 my $o_rv = $orig_u->begin_work; 91 my $r_rv = $rcpt_u->begin_work unless $same_cluster; 92 93 # Write to DB 94 my $orig_write = $self->_save_sender_message; 95 96 # Already inserted in _save_sender_message, when sending to yourself 97 my $rcpt_write = $orig_u->equals( $rcpt_u ) ? 1 : $self->_save_recipient_message; 98 99 if ($orig_write && $rcpt_write) { 100 $orig_u->commit; 101 $rcpt_u->commit unless $same_cluster; 102 return 1; 103 } else { 104 $orig_u->rollback; 105 $rcpt_u->rollback unless $same_cluster; 106 return 0; 107 } 108 109 }

I don't see anything here that changes the message subject or body, but this is calling another two functions that do the saving for the sender and the recipient.

111 sub _save_sender_message { 112 my ($self) = @_; 113 114 return $self->_save_db_message('out'); 115 } 116 sub _save_recipient_message { 117 my ($self) = @_; 118 119 return $self->_save_db_message('in'); 120 }

Both of them call another function, _save_db_message.

121 sub _save_db_message { 122 my ($self, $type) = @_; 123 124 # Message is being sent or received 125 # set userid and otherid as appropriate 126 my ($u, $userid, $otherid); 127 if ($type eq 'out') { 128 $u = $self->_orig_u; 129 $userid = $self->journalid; 130 $otherid = $self->otherid; 131 } elsif ($type eq 'in') { 132 $u = $self->_rcpt_u; 133 $userid = $self->otherid; 134 $otherid = $self->journalid; 135 } else { 136 croak("Invalid 'type' passed into _save_db_message"); 137 } 138 139 return 0 unless $self->_save_msg_row_to_db($u, $userid, $type, $otherid); 140 return 0 unless $self->_save_msgtxt_row_to_db($u, $userid); 141 return 0 unless $self->_save_msgprop_row_to_db($u, $userid); 142 143 return 1; 144 }

I know that the _save_msgtxt_row_to_db is probably the function that is handling the subject and body saving for that table with a similar name I've been looking at to see what's in the database.

170 sub _save_msgtxt_row_to_db { 171 my ($self, $u, $userid) = @_; 172 173 my $sql = "INSERT INTO usermsgtext (journalid, msgid, subject, body) " . 174 "VALUES (?,?,?,?)"; 175 176 $u->do($sql, 177 undef, 178 $userid, 179 $self->msgid, 180 $self->subject_raw, 181 $self->body_raw, 182 ); 183 if ($u->err) { 184 warn($u->errstr); 185 return 0; 186 } 187 188 return 1; 189 }

This is a dead end. I've already seen subject_raw and body_raw, and they don't do any stripping. Now I'm thinking that the stripping must be happening before the subject and body ever hit the sendmessage function. It makes a lot of sense, because when I'm sending a message from the console program using the Message object, it doesn't strip. I should have thought of this before! So, now I'm going to try and find where the arguments are passed in to the page. I go look at the URL of the compose page; it is at inbox/compose.bml. I know that the .bml files for this will be in htdocs:

cd htdocs/inbox

And I open up compose.bml. I search for a use of the sendmessage function, and there isn't one. This is awkward. Why would that function be there if it wasn't being used for this page!? I go back to the main directory of the repository to do an exploratory search. Where does sendmessage get used?

dw@li53-94:~/cvs/dw-free$ rgrep "sendmessage" * bin/upgrading/deadphrases.dat:general /userinfo.bml.sendmessage.body bin/upgrading/en.dat:userlinkbar.sendmessage=Send Message bin/upgrading/en.dat:userlinkbar.sendmessage.title=Send a private message to this user bin/upgrading/en.dat:userlinkbar.sendmessage.title.self=Send a private message to yourself bin/upgrading/en.dat:userlinkbar.sendmessage.title.cantsendmessage=You cannot send a private message to this user bin/upgrading/en.dat:userlinkbar.sendmessage.title.loggedout=You must be logged in to send a private message to this user cgi-bin/ljprotocol.pl: if ($method eq "sendmessage") { return sendmessage(@args); } cgi-bin/ljprotocol.pl:sub sendmessage cgi-bin/DW/Logic/UserLinkBar.pm: text_ml => 'userlinkbar.sendmessage', cgi-bin/DW/Logic/UserLinkBar.pm: title_ml => 'userlinkbar.sendmessage.title', cgi-bin/DW/Logic/UserLinkBar.pm: $link->{title_ml} = 'userlinkbar.sendmessage.title.self' if $u->equals( $remote ); cgi-bin/DW/Logic/UserLinkBar.pm: $link->{class} = "sendmessage"; cgi-bin/DW/Logic/UserLinkBar.pm: $link->{title_ml} = $remote ? 'userlinkbar.sendmessage.title.cantsendmessage' : 'userlinkbar.sendmessage.title.loggedout'; cgi-bin/DW/Logic/UserLinkBar.pm: $link->{class} = "sendmessage_disabled"; htdocs/userinfo.bml.text:.label.sendmessage=Send Message htdocs/userinfo.bml.text:.sendmessage.body2=<a [[aopts]]>Send [[user]] a text message</a><br />on his/her cellphone/pager. htdocs/stc/gradation/gradation-vertical.css: .profile_sendvgift_disabled, .profile_sendmessage_disabled, htdocs/stc/profile.css: .profile_sendvgift_disabled, .profile_sendmessage_disabled, htdocs/stc/blueshift/blueshift.css: .profile_sendvgift_disabled, .profile_sendmessage_disabled, htdocs/stc/celerity/celerity.css: .profile_sendvgift_disabled, .profile_sendmessage_disabled, htdocs/js/contextualhover.js: var sendmessage = document.createElement("a"); htdocs/js/contextualhover.js: sendmessage.href = data.url_message; htdocs/js/contextualhover.js: sendmessage.innerHTML = "Send message"; htdocs/js/contextualhover.js: message.appendChild(sendmessage);

It's...not actually getting called anywhere in Perl code. But now I'm noticing the line emphasized up above.

163 sub do_request 164 { 165 # get the request and response hash refs 166 my ($method, $req, $err, $flags) = @_; 167 168 # if version isn't specified explicitly, it's version 0 169 if (ref $req eq "HASH") { 170 $req->{'ver'} ||= $req->{'version'}; 171 $req->{'ver'} = 0 unless defined $req->{'ver'}; 172 } 173 174 $flags ||= {}; 175 my @args = ($req, $err, $flags); 176 177 my $r = eval { BML::get_request() }; 178 $r->notes->{codepath} = "protocol.$method" 179 if $r && ! $r->notes->{codepath}; 180 181 if ($method eq "login") { return login(@args); } 182 if ($method eq "getfriendgroups") { return getfriendgroups(@args); } 183 if ($method eq "getfriends") { return getfriends(@args); } 184 if ($method eq "friendof") { return friendof(@args); } 185 if ($method eq "checkfriends") { return checkfriends(@args); } 186 if ($method eq "getdaycounts") { return getdaycounts(@args); } 187 if ($method eq "postevent") { return postevent(@args); } 188 if ($method eq "editevent") { return editevent(@args); } 189 if ($method eq "syncitems") { return syncitems(@args); } 190 if ($method eq "getevents") { return getevents(@args); } 191 if ($method eq "editfriends") { return editfriends(@args); } 192 if ($method eq "editfriendgroups") { return editfriendgroups(@args); } 193 if ($method eq "consolecommand") { return consolecommand(@args); } 194 if ($method eq "getchallenge") { return getchallenge(@args); } 195 if ($method eq "sessiongenerate") { return sessiongenerate(@args); } 196 if ($method eq "sessionexpire") { return sessionexpire(@args); } 197 if ($method eq "getusertags") { return getusertags(@args); } 198 if ($method eq "getfriendspage") { return getfriendspage(@args); } 199 if ($method eq "getinbox") { return getinbox(@args); } 200 if ($method eq "sendmessage") { return sendmessage(@args); } 201 if ($method eq "setmessageread") { return setmessageread(@args); } 202 if ($method eq "addcomment") { return addcomment(@args); } 203 204 $r->notes->{codepath} = "" 205 if $r; 206 207 return fail($err,201); 208 }

I'm now suspecting that ljprotocol.pl is a file meant for AJAX calls--the kind of thing that lets you do something on a webpage without refreshing the page, like tracking a post, using Javascript to communicate back with the server. But there's no comment at the top of this file to confirm my suspicions. And there don't really appear to even be any uses in the .js files in my grep above; the one in the contextual hover doesn't do a no-page-reload send message, it just inserts a link to send a message. So in short, there is a function in the code that I don't know where it gets called. But if it ever does, it's a good thing that I found it and changed it anyway, because otherwise we'd end up with another bug when some HTML got unexpectedly stripped during sending messages. The duplicate functionality between this code and the code on compose.bml makes me a little nervous though. I know it would be better if all of the code that is the same was in one function, but I don't think architectural changes like that are in the scope of this bug, and I also don't think I have the experience to handle them. So I'm just going to make the change in both places and make a question in my bug report about this when I submit the patch.

Still, the code I'm looking for will be after the form gets submitted, so I look at the code after the statement that checks to see if stuff has been submitted:

41 if (LJ::did_post()) {

And it's there, just like it was in sendmessage:

49 my $msg_subject_text = LJ::strip_html($POST{'msg_subject'}); 50 push @errors, "Invalid text encoding for message subject" 51 unless LJ::text_in($msg_subject_text); 52 53 # strip HTML from body and test encoding and length 54 my $msg_body_text = LJ::strip_html($POST{'msg_body'}); 55 push @errors, "Invalid text encoding for message body" 56 unless LJ::text_in($msg_body_text);

I take away the strip_htmls. Once again, I refresh my patch, sync my code, and restart the server:

hg qrefresh cd $LJHOME bin/cvsreport.pl -sync -cvsonly sudo /etc/init.d/apache2 restart

I perform my favorite test again and glory of glories, it actually works this time on the site. But now I'm noticing something else in my email notification of the message. I have my emails set to be plain text, so no escaping should be necessary. Yet the email is now telling me:

Hi bunnies,

system sent you a message on Memewidth.

The message is: Test4 <3 >
Test4 <3 >

The title of the message is escaped even though I have plaintext emails. But the body isn't. I need to track down where this is. The subject/message of HTML emails is going to need to be escaped, and for plain text it shouldn't be. I also note that I would actually like the message subject to be in the subject of the email. Right now it says "system sent you a message", but I want it to say "Message from system: SUBJECT" instead or something like that. I make a note of that to check and see if it would make a good suggestion if it hasn't been suggested already.

Now, I don't know much about the ESN system. It handles the notifications; ESN stands for Event Subscription Notification. But I remember seeing something about sending a message while playing around with LJ::Message and that package it was complaining about missing when I was using it on the console. I go look at LJ::Message and send again and they lead me to:

66 sub _send_msg_event { 67 my ($self) = @_; 68 69 my $msgid = $self->msgid; 70 my $ou = $self->_orig_u; 71 my $ru = $self->_rcpt_u; 72 LJ::Event::UserMessageSent->new($ou, $msgid, $ru)->fire; 73 LJ::Event::UserMessageRecvd->new($ru, $msgid, $ou)->fire; 74 }

I'm concerned about the received message; as far as I know sending a message doesn't trigger an email. I open cgi-bin/LJ/Event/UserMessageRecvd.pm and find a relevant function pretty close to the top:

31 sub _as_email { 32 my ($self, $u, $is_html) = @_; 33 34 my $lang = $u->prop('browselang'); 35 my $msg = $self->load_message; 36 my $other_u = $msg->other_u; 37 my $sender = $other_u->user; 38 my $inbox = "$LJ::SITEROOT/inbox/"; 39 $inbox = "<a href=\"$inbox\">" . LJ::Lang::get_text($lang, 'esn.your_inbox') . "</a>" if $is_html; 40 41 my $vars = { 42 user => $is_html ? ($u->ljuser_display) : ($u->user), 43 subject => $msg->subject, 44 body => $is_html ? $msg->body : $msg->body_raw , 45 sender => $is_html ? ($other_u->ljuser_display) : ($other_u->user), 46 postername => $other_u->user, 47 sitenameshort => $LJ::SITENAMESHORT, 48 inbox => $inbox, 49 };

Looks like the problem is everything else except the subject has a conditional based on whether or not the email is HTML. I make changes that look like this:

43 subject => $is_html ? $msg->subject : $msg->subject_raw, 44 body => $is_html ? $msg->body : $msg->body_raw, 45 sender => $is_html ? ($other_u->ljuser_display) : ($other_u->user),

Time for another refresh and test:

hg qrefresh cd $LJHOME bin/cvsreport.pl -sync -cvsonly sudo /etc/init.d/apache2 restart

But the test doesn't work! I'm still getting emails with escaped subjects. Then I think that I've started the web server side, but the messages are probably being sent by the worker. And I haven't restarted that, so it's probably still running off of the old code. I bring worker-manager to the foreground, kill it, and restart it:

fg Ctrl-C $LJHOME/bin/worker-manager --debug >&/dev/null &

I run my test again and find sweet, sweet victory. My plaintext email now has the subject not escaped, and I know that it's still escaped for HTML email.

Patch evaluation and clean up

I want to get a summary of all the changes to the code I've made. I need to check them over according to the Programming Guidelines.

cd cvs/dw-free hg qdiff

Gives me a diff of my changes. Lines that I've changed or added have a + in front of them. The original lines or lines that I've deleted have a - in front of them.

diff -r 4f64074511e9 cgi-bin/LJ/Event/UserMessageRecvd.pm --- a/cgi-bin/LJ/Event/UserMessageRecvd.pm Mon May 25 01:18:17 2009 +0800 +++ b/cgi-bin/LJ/Event/UserMessageRecvd.pm Mon May 25 02:53:34 2009 +0000 @@ -40,8 +40,8 @@ my $vars = { user => $is_html ? ($u->ljuser_display) : ($u->user), - subject => $msg->subject, - body => $is_html ? $msg->body : $msg->body_raw , + subject => $is_html ? $msg->subject : $msg->subject_raw, + body => $is_html ? $msg->body : $msg->body_raw, sender => $is_html ? ($other_u->ljuser_display) : ($other_u->user), postername => $other_u->user, sitenameshort => $LJ::SITENAMESHORT, diff -r 4f64074511e9 cgi-bin/ljprotocol.pl --- a/cgi-bin/ljprotocol.pl Mon May 25 01:18:17 2009 +0800 +++ b/cgi-bin/ljprotocol.pl Mon May 25 02:53:34 2009 +0000 @@ -467,12 +467,11 @@ my @errors; - my $subject_text = LJ::strip_html($req->{'subject'}); + my $subject_text = $req->{'subject'}; return fail($err, 208, 'subject') unless LJ::text_in($subject_text); - # strip HTML from body and test encoding and length - my $body_text = LJ::strip_html($req->{'body'}); + my $body_text = $req->{'body'}; return fail($err, 208, 'body') unless LJ::text_in($body_text); diff -r 4f64074511e9 htdocs/inbox/compose.bml --- a/htdocs/inbox/compose.bml Mon May 25 01:18:17 2009 +0800 +++ b/htdocs/inbox/compose.bml Mon May 25 02:53:34 2009 +0000 @@ -46,12 +46,12 @@ if ($mode eq 'send') { # strip HTML from subject and test encoding - my $msg_subject_text = LJ::strip_html($POST{'msg_subject'}); + my $msg_subject_text = $POST{'msg_subject'}; push @errors, "Invalid text encoding for message subject" unless LJ::text_in($msg_subject_text); # strip HTML from body and test encoding and length - my $msg_body_text = LJ::strip_html($POST{'msg_body'}); + my $msg_body_text = $POST{'msg_body'}; push @errors, "Invalid text encoding for message body" unless LJ::text_in($msg_body_text); my ($msg_len_b, $msg_len_c) = LJ::text_length($msg_body_text);

You can see that this change has only affected about six lines of code, all of which were easy changes! It was finding them and testing them that was the difficult part.

To check for whitespace (no extraneous whitespace, no tabs, etc), I use this trick:

hg qdiff | cat -et

It shows me where endlines are with a $. (They'll be one space away from the start of the line in the diff, where the +/-s show up.) If there is a tab, I can see it annotated with ^I. My whitespace looks good overall. No extra whitespace at the end of lines, I'm not using tabs. But I notice that some of the older code surrounding my code doesn't have space around parenthesis. Additionally, some of them are quoting hash literals. Since it's old code, I can leave as is if I'm matching my new stuff with the old style. Because a lot of this code feels really crufty (with the duplicate functionality and need of English stripping) and I suspect it will be cleaned up further later, I don't want to go through the effort of bringing it up to our standards right now. I add a few comments instead: one to annotate that sendmessage is about sending private messages from one account to another, and another to replace the comment I deleted with the parts that are still relevant.

Submitting the patch

I make one last check that all my changes are in my patch:

hg qrefresh

Then I make sure that there haven't been updates that conflict with my patch since I started making it:

hg qpop # pop my the patch off of the code hg pull # pull any changes from the main repository hg update # apply any changes hg qpush # put my patch back onto the code

There haven't been any updates, so my patch goes back on cleanly. I copy my patch to my home directory to make it easier to fetch to my home computer:

cp .hg/patches/message_escape_html ~/message_escape_html.patch

I fetch the file from my server. People using Windows can use a program like WinSCP for this, but I'm on a Mac, so I am using a command line program called "scp" in Terminal. I have an entry in my .ssh/config file that gives me a shorthand name for my server information:

Host dwdev Hostname dwdev.memewidth.org user dw

And I can use SCP command line program to fetch the file I just made with the command:

scp dwdev:message_escape_html.patch .

(Overall, the command for scp goes SERVER:PATH/TO/SERVERFILE PATCH/TO/DOWNLOAD.)

Now, I go back to Bug 858 on Bugzilla. I click the "add an attachment" link and put the file path in the File box. For description, I enter "Escapes HTML instead of stripping it in private messages". I check the patch box. I think this patch is ready to be committed if it gets through review, so I put both of those flags to "?". Now it's time to describe the changes I've made and any concerns and questions I might have:

I've changed stripping to escaping for both the subject and body of private messages. Due to this change, I've also changed when the subject of a private message is sent raw in an email, which is for plaintext emailers.

Sidenotes: A lot of the code I was working with looked like it needed to be English stripped. Additionally, the duplicated functionality between compose.bml and ljprotocol.pl for sending messages worries me.

Now it is up to others to review the patch themselves, to make sure that I'm making the right changes. If it's acceptable, they'll commit it. If not, they'll tell me what to fix.

Hindsight

In hindsight, I should have started out at compose.bml and worked my way back from there. It would have been a lot faster! I knew that's where new private messages were composed, so it would have been trivial to find.

And once I tested saving an unescaped message to the database, I didn't need to trace the process in LJ::Message. If I was thinking properly, I would have known the stripping wasn't happening when it got saved to the database, because the unstripped message saved just fine. I could have gone straight from there to compose.bml.

And, I feel like I should mention that a lot of these issues are due to struggling with my own Dreamhack setup not being right. I don't think Dreamhacks would come with some of these issues, like the reCAPTCHA problem, The Schwartz and workers not running, or YAML not being installed.

Patch Acceptance

After reviewing my patch and leaving notes on the bug ticket,

afuna decides my patch is okay and commits it to the repository. When this happens, it shows up as a post in

changelog.

Now that my patch has been accepted, I don't need to have it in my patch queue anymore. So I go and unapply it and delete it:

cd cvs/dw-free
hg qpop
hg qdelete message_escape_html

Lessons Summary

* I've learned how to disable the "recaptcha" part of the system.
* The Schwartz is needed for the successful sending of messages.
* If I make changes to event code, I should restart the worker-manager for those changes to take effect.
* YAML needs to be installed for The Schwartz, and I've added that to the list of Perl packages to install.
* LJ::ehtml escapes HTML from a string, LJ::strip_html strips HTML from a string.
* cgi-bin/ljprotocol.pl contains functions probably used for AJAX calls
* bless is used to make an anonymous hash reference into an object
* usermsgtext is the database table that contains the contents of messages, like the subject and body. There are two copies: one for the person who sent it and one for the person who received it. The primary key for that table is the combination of those two values.
* I can use the ljconsole script to help me test and debug code.
* A class function that starts with _ means that it is a private function.

Notes Summary

* 'recaptcha' needs to be added to the DISABLED array in the config.pl in the repository. It should probably be disabled by default, since it takes extra set up.
* We need to change the site copy text for resetting the password from "Your password has been mailed." to something more appropriate, like "Instructions on resetting your password have been mailed."
* I was missing the password reset success string, but I've gone back and doublechecked it on Dreamwidth and it's there, so it must be something with my install. Case closed.
* If you then log in from the change password reset page with the confirmation code, it takes you to an error page. This should be more user friendly.
* sendmessage in ljprotocol.pl has a FIXME in it with no other explanation.
* There's an awkward amount of duplicate code for sending a message: the code in compose.bml and the code in ljprotocol.pl. It would be nice if this was consolidated as much as possible.
* ljprotocol.pl and compose.bml both are in need of English stripping.

I hope this has been a useful walkthrough of the process of bug fixing and banging your head against things until they work. If anybody has questions about the contents here, or suggestions about things I should have done differently, feel free to comment!