Stories
Slash Boxes
Comments

SoylentNews is people

posted by janrinok on Wednesday April 15 2020, @10:11PM   Printer-friendly
from the rushin'-hacks dept.

[20200416_143747 UTC: Update 2: Added an Example to make clear what the problem was, and added 2 headings subsequent to the example to better organize the information. --martyb]
[20200416_005831 UTC; Update 1: Updated title and corrected spelling of balanceTags(). --martyb]

Ooops! Things should be working correctly, now.

tl;dr: Back on March 20th, someone tripped over a bug that appears to be in the balanceTags() routine in our Perl code. I found a way to made a quick fix to prevent its happening again, but the fix was missing a couple steps. I caught and fixed one of them, but only now just handled the other.

Workaround: When writing a comment, writing or editing a journal entry, or when submitting a story, use "DEL" instead of "STRIKE" to make text look like this.

This story is the result of something I learned in the process: properly notify the community of any changes to the site!

Symptom: It all started with a tag (i.e. HTML element) error in this comment in a user's journal where the user coded a <strike> tag, but neglected to provide a matching </strike> tag.

Example: Let's look back to the original comment which manifested this bug. Here's the latter part of it, after being corrected:

Looks who's projecting. Consider your phrase "steal jobs and send them overseas for cheap/free labor" (no such thing as free labor). That helps billions of poor people improve their lives. Yet here you are, selfishly obsessing over your developed world pricing power (with some very unempathic label spewing) rather than display the alleged empathy or morality that you claim to be concerned about.

Your empathy is nonexistent and your morals are bankrupt - definitely not the sort of person I should be taking advice from!

The problem is that there was no closing <strike> after the word "cheap", so it looked like this:

Looks who's projecting. Consider your phrase "steal jobs and send them overseas for cheap/free labor" (no such thing as free labor). That helps billions of poor people improve their lives. Yet here you are, selfishly obsessing over your developed world pricing power (with some very unempathic label spewing) rather than display the alleged empathy or morality that you claim to be concerned about.

Your empathy is nonexistent and your morals are bankrupt - definitely not the sort of person I should be taking advice from!

If that was all that happened it would be ugly, but tolerable. Unfortunately, every single character following it on the page was struck through, too. Not Good™.

Immediate Fix: To my knowledge there was only one way to rectify the immediate issue: manually go into the DB and insert the missing tag. This I was able to do quite quickly, but I still saw a problem.

More to Come: Anyone who saw this comment discussion, either at this moment, or who happened upon it later, would see an opportunity to intentionally leave a hanging tag and thus disfigure the site. Trolls gotta troll. So, I made the fix and noted same in this comment reply.

So, an instance of the problem was fixed, but now what? There's a "proper" way to do it, and there is another way to get the same effect that can be quickly implemented. I chose the latter.

Perl Code: Normally, such HTML errors in a user's comment or journal entry (or an editor's edit of a story!) are caught and handled by a routine in our Perl code: balanceTags(). The code looks though all the tags, with whatever nesting is present, detects where tags do not have a required closing tag,and silently inserts it into the text that makes it into the DB. It's rather hairy code because it also needs to handle: extra closing tags, mis-matched closing tags (e.g.: <b> bold <bold and italic> </b> </i>), mistyped or otherwise non-existent tags, restricting what tags are supported, and custom-created site tags! Whew!

Further, to fix it in the Perl code means going through the process of: checking the code out from GitHub, understanding the code, making the change, compiling the change, testing the change, (after rolling it out to our dev server), and then -- if all looks good -- rolling the change out to our production servers. And, of course, nobody was around at the moment who could support such activities even if it were an easy coding change (and it is not!)

Expediency: I realized there was another approach which would mitigate the problem -- not requiring Perl coding changes -- but could still prevent its recurrence: changing the value of a "Site Variable" (aka "site var").

Rehash Primer: Now I need to step back for a moment and explain a couple things. The code for SoylentNews.org is a fork of ancient Slashcode that was put up on GitHub. Slashcode was implemented using a Model View Controller design. There is a clear demarcation between what is done where and at what level.

Templates: As part of its implementation, the SlashCode implemented "Templates" which generate the HTML pages that get sent to the browser and act as an interface between the code and the user. As far as I know, every page you see on the site comes by way of a template. Each template may, in turn, make use of other templates. Templates can make calls to underlying Perl code. This is where the site implements the heavy lifting of talking to the database (DB), creating e-mails, and other closer-to-the-metal activities. The template language (from personal inspection; I have yet to find an official document as to its syntax and semantics) presents what appears to be a simple, macro-capable language. The templates are stored in the DB and loaded into memory when the site is started. An advantage of this is that changes to templates can be made "on the fly" using a template editor (which is, itself, a template!) There is one caveat: for the changes to take effect, processes on the front-end servers need to be "bounced", i.e. restarted, so the changes are loaded into memory from the (updated) DB.

Site Variables: There are some parameters whose values affect the site's operations: Name of the site, domain name of the site, the name of the Anonymous User account, ... it goes on and on and on. There are no less than 750 site variables! And, as many things that grew beyond their initial construction, there is no simple way to look for what site vars might be appropriate to any given situation. One is just expected to know what they are and what they do and how they do it. Simple enough approach when they first started, I guess. A search capability would be very nice to have, but it will take some coding to make that happen, so it has become just another of the several changes that would be nice to make to the site.

So, back to the matter at hand, I knew about the "approvedtags" site var which lists all tags which are permitted to be used on the site. Sure enough, "STRIKE" was in there! And, I saw that "DEL" was in there, too. Does "DEL" have the same problem? I tried a quick test comment on our development server and it revealed that balanceTags() properly handled a hanging <DEL> without a matching </DEL>. Yay! I removed "STRIKE" from the "approvedtags" list, saved the change, bounced the front-end servers, and breathed a sigh of relief.

All was good, until someone asked in a footnote to a comment why do we still list STRIKE as being a permitted tag for comments? What? I double-checked and verified that "STRIKE" was no longer listed in "approvedtags". What is going on? So, I commenced searching and finally discovered another site var: "approvedtags_visible" which contains the list of tags that is presented to the user as being available. And, sure enough, "STRIKE" was in that list. Grrr! I removed "STRIKE" from "approvedtags_visible", saved the changes, and saw no further issues mentioned there. Finally!

Or so I thought. Did you see what was missed? The site vars were now correct and up-to-date. The changes were saved to the DB. But... those changes existed only in the DB. Still needed to 'bounce' the front end servers for the changes to take effect. So, that entailed a quick SSH to our servers, running the bounce scripts, and verifying that "STRIKE" was truly and properly removed from the tags presented to the user as being available for use, and that anyone trying to use <STRIKE>, anyway, would discover it did not work.

Conclusions: So, here are some lessons learned:

  1. When you want to use a <STRIKE> tag, use <DEL>, instead.
  2. There is no assurance that reporting a problem in the comments will be noticed.
  3. Please report site issues with an e-mail sent to admin (at) soylentnews (dot) org.
  4. In addition to sending an e-mail, mention it in the "#dev" channel of our IRC server.
  5. This particular issue should now be well and truly fixed. Please report any problems you may discover with it.
  6. When communicating changes made to the site, a mention in the comments does not suffice.
  7. --martyb

[Janrinok says: TL:DR Martyb fixed it, OK, OK, I have read it....]


Original Submission

 
This discussion has been archived. No new comments can be posted.
Display Options Threshold/Breakthrough Mark All as Read Mark All as Unread
The Fine Print: The following comments are owned by whoever posted them. We are not responsible for them in any way.
  • (Score: 3, Informative) by maxwell demon on Thursday April 16 2020, @12:52PM (5 children)

    by maxwell demon (1608) on Thursday April 16 2020, @12:52PM (#983552) Journal

    Thanks for the explanation.

    BTW, I now got curious and looked into the balanceTags function on GitHub, and my guess is that to make <strike> work, you would have to add it to the variables %known_tags (line 3016) and %no_block (line 3032).

    Not that it really matters, given that <del> gives the functionality anyway (and even is less to type).

    --
    The Tao of math: The numbers you can count are not the real numbers.
    Starting Score:    1  point
    Moderation   +1  
       Informative=1, Total=1
    Extra 'Informative' Modifier   0  
    Karma-Bonus Modifier   +1  

    Total Score:   3  
  • (Score: 2) by martyb on Thursday April 16 2020, @02:54PM (4 children)

    by martyb (76) Subscriber Badge on Thursday April 16 2020, @02:54PM (#983611) Journal

    Thanks for taking a look and finding that!

    Now, picture yourself on the site, with strike characters running all the way to the end of the page. And knowing there's gotta be that one troll who would see this and run with it, posting a comment with an unclosed strike to every story. And the only way to fix each instance is a manual DB update. Whack-a-troll is not my idea of a good time. When I realized there was a quick fix, I took it and pushed it out ASAP. And, of course this was while a few other things were swirling around at the same time. And as you saw in the write-up, and TMB's comment to this thread, making a coding change under time pressure and rolling it out to the site seemed like a very not good idea.

    Thanks again for looking at the code and coming up with that fix!

    --
    Wit is intellect, dancing.
    • (Score: 2) by maxwell demon on Thursday April 16 2020, @04:16PM (3 children)

      by maxwell demon (1608) on Thursday April 16 2020, @04:16PM (#983651) Journal

      My comment was in no way meant to imply you were lazy or something like that. After all, I didn't even test that this would indeed fix the problem (that would have required me to set up a test server just for this), I just looked at the code and figured this should do it. No guarantee that I didn't overlook something.

      As I wrote, I just got curious, so I looked. And then I thought there's no reason not to share my result.

      --
      The Tao of math: The numbers you can count are not the real numbers.
      • (Score: 2) by martyb on Thursday April 16 2020, @06:01PM (2 children)

        by martyb (76) Subscriber Badge on Thursday April 16 2020, @06:01PM (#983714) Journal

        My comment was in no way meant to imply you were lazy or something like that. After all, I didn't even test that this would indeed fix the problem (that would have required me to set up a test server just for this), I just looked at the code and figured this should do it. No guarantee that I didn't overlook something.

        My bad! In no way did I mean to suggest any shortcoming on your part! In fact, I am grateful for your looking at the code and your suggested fix!

        My intention was to provide some background on why *I* chose to make the fix the way I did!

        And, yes, I cam to the same conclusion as you did WRT the code, under the assumption that the comments were correct and all the other code was functioning as it should. I am by no means conversant with all the two-symbol data structure abstractions that Perl seems to relish!

        And, yes, implementing the change, compiling the code, rebuilding the system, rolling it out, and then testing it... that was going to have to wait for some time when more time and less pressure for a fix is available.

        I humbly apologize for any offense taken; absolutely NONE was intended!

        Thanks again for looking at the code and your suggested fix!

        --
        Wit is intellect, dancing.
        • (Score: 2) by maxwell demon on Thursday April 16 2020, @07:09PM (1 child)

          by maxwell demon (1608) on Thursday April 16 2020, @07:09PM (#983740) Journal

          I humbly apologize for any offense taken; absolutely NONE was intended!

          Don't worry, you haven't offended me. Rather I thought you misunderstood me in a way that would offend you, so I wanted to make clear that it was not meant that way. Sorry for all the misunderstandings I apparently caused.

          --
          The Tao of math: The numbers you can count are not the real numbers.
          • (Score: 2) by martyb on Thursday April 16 2020, @07:24PM

            by martyb (76) Subscriber Badge on Thursday April 16 2020, @07:24PM (#983751) Journal
            Got it. Thanks!
            --
            Wit is intellect, dancing.