Hacker News new | comments | show | ask | jobs | submit login

This code is beyond awful - it fails to display, makes endless AJAX requests, and more; here are a few fun tidbits:

1. The code is not encapsulated in an IIFE, so it clobbers any global variables (like 'image_url') in the page, breaking any scripts relying on those variables.

2. The code spends an inordinate time checking if you're running Netscape Navigator 6.

3. Strangely, they include a whole bunch of code allowing the message to be dragged around the window (which is nice) but they don't allow it to be closed. Of course, it closes itself after making a single AJAX request into a black hole, so there's that. Bugs piled on top of each other make this entire message mostly harmless, if it weren't for the variable clobbering & bandwidth usage (see the next item...)

4. Upon load, checkBulletin() is immediately invoked. This does an AJAX call to '/e8f6b078-0f35-11de-85c5-efc5ef23aa1f/aupm/notify.do?dispatch=checkBulletin'. I assume this is to check if the bulletin has changed, to see if there are new messages, or maybe to check if the user has acknowledged the message yet. Unfortunately:

* This URL is relative, which means it will never actually reach its intended target (instead filling your web logs with this request)

* Upon xmlhttp.readystate=4 (request finished, successful or not, so this will change to 4 even on a 404 error), the comcast message is hidden. This means that the entire 'bandwidth exceeded' message will actually be hidden as soon as this request completes, which may be in <500ms, giving the user absolutely no time to see or acknowledge it.

* The author makes an attempt to not continue sending AJAX requests to this URL after a successful attempt, but botches it, so this request is actually sent indefinitely, every 5000ms, while every any page is open. This means every single tab on your system is popping AJAX requests every 5 seconds for the whole month that your account is nearing its quota. This likely brings you over quota pretty quickly if you leave your computer on all day.

That's right, this code causes every page served on your system to pop an AJAX request to the wrong URL every 5 seconds, as long as the tabs are open.

We can sit and argue all day whether or not it's ethical to display messages by injecting code into the DOM, but it is certainly unethical to write such awful javascript that clobbers global variables and drives up bandwidth costs by making AJAX requests to the wrong url every 5 seconds until the cows come home. Whoever wrote this script should be fired.

EDIT: Similarly, back in the dialup days, some ISPs would inject ads into their content. One way this was stopped was to argue that it was not legal for the ISP to charge you for data, then artificially inflate the size of that data by injecting ads. This script is doing just the same in a measurable way by causing these AJAX requests to be run every 5 seconds on every tab in your system.




> * This URL is relative, which means it will never actually reach its intended target (instead filling your web logs with this request)

It likely doesn't matter that the URL is relative. It contains a GUID to be unlikely to resemble any real URL, and it's clear enough that they are capable of deep-packet-inspecting all of your web traffic from the way this is already used, so they likely hijack any request to this URL path within their network to capture its contents, and return a 200.

I don't have Comcast so I can't verify, but it would be interesting for somebody to check whether that URL is masked for all Comcast users.

> That's right, this code causes every page served on your system to pop an AJAX request to the wrong URL every 5 seconds, as long as the tabs are open.

I can only hope that they infinitely hang requests to their special URL in the case that user is under the quota so that this is not true. But if it is true, and they are not perfect about masking the URL (edit: it seems like people below on this thread have seen requests to this URL in their server logs), this could be construed as a DDOS attack by Comcast on every owner of an HTTP server via their own customers.


brokentone comments below that they've seen the urls in their production logs. I don't see it in any of mine but I'd be willing to bet that a company writing JS that bad would probably screw up the rest of the process too.

Surely a class action against Comcast is in order here? They're charging everyone for bandwidth they're not using.


That's likely to happen even if Comcast are using DPI to intercept the requests due to users moving between Comcast and other internet connections.


I can confirm that I saw a large number of these urls show up in my logs as well.


> Whoever wrote this script should be fired.

Not to mention the potential GPL violation for cut-n-pasting brainjar code.

http://www.brainjar.com/terms.asp


Oh, the irony of Comcast being guilty of copyright infringement....


Can we get them on 6 strikes?


More like 6 Million strikes ;)


I love how the top comment expresses outrage, not that Comcast is injecting JS into people's sessions, but that it's poorly written JS.


This is Hacker News. We take our code seriously.


This is the Internet. We take other people's code even more seriously :-)


Nobody is allowed to see my code. It's not allowed.


There are a lot of things in this code that make me think that it was written by someone for whom JavaScript is not their main language - but probably the most glaring example is the use of `new Object()`. I've never seen anyone with more than 3 days JS experience use the Object constructor over a literal.


Oh, it's just awful. It's worse than just "not knowing JavaScript." This is code from someone who has no idea how to program:

    function Browser() {
        var ua, s, i;
        this.isIE = false;
        this.isNS = false;
        this.version = null;
        ua = navigator.userAgent;
        s = "MSIE";
        if ((i = ua.indexOf(s)) >= 1) {
            this.isIE = true;
            this.version = parseFloat(ua.substr(i + s.length));
            return;
        }
        s = "Netscape6/";
        if ((i = ua.indexOf(s)) >= 0) {
            this.isNS = true;
            this.version = parseFloat(ua.substr(i + s.length));
            return;
        }
        s = "Gecko";
        if ((i = ua.indexOf(s)) >= 0) {
            this.isNS = true;
            this.version = 6.1;
            return;
        }
    }
But it's not just these people. Code like this is everywhere! Here's what I ran into on www.safeco.com today (NSFL!):

    function setupAddress(frm, i, clickevent) {
        if (frm["USERESADDASMAILINGMAIN" + i].checked) {
            if (frm.NEWRESIDENCEADDRESS1.value == "" && frm.NEWRESIDENCEADDRESS2.value == "") {
                alert("Resident address must be entered for this option.");
                frm.NEWRESIDENCEADDRESS1.focus();
                frm["USERESADDASMAILINGMAIN" + i].checked = false;
            }
            if (frm.NEWRESIDENCEADDRESS1.value == "" && frm.NEWRESIDENCEADDRESS2.value != "") {
                FieldSwap(document.frmMain.NEWRESIDENCEADDRESS1, document.frmMain.NEWRESIDENCEADDRESS2);
            }
            frm["NEWMAILINGADDRESS1" + i].value = frm.NEWRESIDENCEADDRESS1.value;
            frm["NEWMAILINGADDRESS1" + i].disabled = true;
            frm["NEWMAILINGADDRESS1" + i].onfocus = frm["NEWMAILINGADDRESS1" + i].blur;
            frm["NEWMAILINGADDRESS2" + i].value = frm.NEWRESIDENCEADDRESS2.value;
            frm["NEWMAILINGADDRESS2" + i].disabled = true;
            frm["NEWMAILINGADDRESS2" + i].onfocus = frm["NEWMAILINGADDRESS2" + i].blur;
            frm["NEWMAILINGCITY" + i].value = frm.NEWRESIDENCECITY.value;
            frm["NEWMAILINGCITY" + i].disabled = true;
            frm["NEWMAILINGCITY" + i].onfocus = frm["NEWMAILINGCITY" + i].blur;
            frm["NEWMAILINGSTATE" + i].value = frm.NEWRESIDENCESTATE.value;
            frm["NEWMAILINGSTATE" + i].disabled = true;
            frm["NEWMAILINGSTATE" + i].onfocus = frm["NEWMAILINGSTATE" + i].blur;
            frm["NEWMAILINGZIPCODE" + i].value = frm.NEWRESIDENCEZIPCODE.value;
            frm["NEWMAILINGZIPCODE" + i].disabled = true;
            frm["NEWMAILINGZIPCODE" + i].onfocus = frm["NEWMAILINGZIPCODE" + i].blur;
            if (i != 0) {
                frm["EXPLANATIONVEH" + i].value = "";
                frm["EXPLANATIONVEH" + i].disabled = true;
                frm["EXPLANATIONVEH" + i].onfocus = frm["EXPLANATIONVEH" + i].blur;
            }
        } else {
            frm["NEWMAILINGADDRESS1" + i].disabled = false;
            frm["NEWMAILINGADDRESS1" + i].onfocus = null;
            frm["NEWMAILINGADDRESS2" + i].disabled = false;
            frm["NEWMAILINGADDRESS2" + i].onfocus = null;
            frm["NEWMAILINGCITY" + i].disabled = false;
            frm["NEWMAILINGCITY" + i].onfocus = null;
            frm["NEWMAILINGSTATE" + i].disabled = false;
            frm["NEWMAILINGSTATE" + i].onfocus = null;
            frm["NEWMAILINGZIPCODE" + i].disabled = false;
            frm["NEWMAILINGZIPCODE" + i].onfocus = null;
            if (i != 0) {
                frm["EXPLANATIONVEH" + i].disabled = false;
                frm["EXPLANATIONVEH" + i].onfocus = null;
            }
            if (clickevent) {
                frm["NEWMAILINGADDRESS1" + i].value = '';
                frm["NEWMAILINGADDRESS2" + i].value = '';
                frm["NEWMAILINGCITY" + i].value = '';
                frm["NEWMAILINGSTATE" + i].value = '';
                frm["NEWMAILINGZIPCODE" + i].value = '';
            }
        }
    }


> This is code from someone who has no idea how to program

That's a quite strong assertion. What's wrong with your first example? I can think of very few criticisms (s isn't needed for example) but there's lots of things they did well:

- It follows the best practices for an OO constructor (doesn't return the object, just sets properties of `this`)

- All temporary variables are local. No global pollution (besides the "Browser" function itself, but because you're quoting it out of contect, I can't tell if even that's local or not)

- Degrades gracefully (everything is null) instead of picking a default incorrect choice

Sure, I would have written it differently, but so would everyone else here.

As for your second example, sure, it's not great, but I can sort of imagine some sleep-deprived developer coding up that to interop with some auto-generated DOM elements from an old PHP script left behind by a forgotten intern. We need more context here.


I was foaming at the mouth a bit, wasn't I?

All your points are well taken, and a better analysis of the code by far than my hasty reaction.

So what was bothering me about the first example? Probably the repetition of the indexOf() tests, combined with one of the indexOf() tests being >= 1 and the rest >= 0.

But you're right, it's not nearly as bad as I made it out to be.

Since I've put my foot in my mouth, I guess I'll put my money there too and show how I might have done it. If I were doing UA detection at all, that is:

    function Browser() {
        function is( ua, result ) {
            var start = navigator.userAgent.indexOf( ua );
            if( start < 0 ) return false;
            result.version = result.version ||
                parseFloat( navigator.userAgent.substr( start + ua.length ) );
            return result;
        }
        return(
            is( 'MSIE', { isIE: true } ) ||
            is( 'Netscape6/', { isNS: true } ) ||
            is( 'Gecko', { isNS: true, version: 6.1 } ) ||
            {}
        );
    }
But that fails on one of your points, since it returns an object instead of setting properties of 'this'. It's also less flexible - what if one of the tests needed more than a simple string comparison? At least it's simpler?

So who am I to criticize? :-)

On the second example, it's not just that function - the entire web page is full of similar code. Here's another snippet:

    addressCheckMsg="";
    if(!type)
    {
        iLen = line1.value.length;
        for(i=0; (i<4) && (i<iLen); i++)
        {
            var ch = line1.value.substring(0,i+3);
            chUpper=ch.toUpperCase();
            switch(chUpper)
            {
                case 'PO BOX':
                    addressCheckMsg += "     Resident address can not be a P.O. Box.\n";
                    i=iLen;
                    break;
                case 'P.O. BOX':
                    addressCheckMsg += "     Resident address can not be a P.O. Box.\n";
                    i=iLen;
                    break;
                case 'P. O. BOX':
                    addressCheckMsg += "     Resident address can not be a P.O. Box.\n";
                    i=iLen;
                    break;
                case 'P O BOX':
                    addressCheckMsg += "     Resident address can not be a P.O. Box.\n";
                    i=iLen;
                    break;
                case 'POB':
                    addressCheckMsg += "     Resident address can not be a P.O. Box.\n";
                    i=iLen;
                    break;
                default:
                    break;
            }           
        }
    }
Yikes. I'd better not say more or I'll start foaming again... :-)


Your first example has nice trickery in it but I prefer the original version. I think it is important to keep it simple.

Compressed code is often not the best way to do it; adding a few lines of verbosity can reduce the time it takes to understand the code to a fraction while sacrificing very little in terms of performance.


I disagree about the first one. I think that is easier to understand, easier to add more options and less prone to errors.


It's good that this discussion can actually be had on here civilly. Too often I see vitriol and pedantic disagreement on HN for no reason other than the swinging of the e-peen.


actually in this code, "this" refers to the window object. it means it's same as window.version.


I thought that for a moment, but it isn't so. They do call the Browser() function as a constructor with 'new Browser()', so 'this' is the object it's constructing.


I'm curious to read the criticism on code.


Ethical stuff aside, I can't imagine hiring someone to actually produce code THIS bad. Where the hell did they find the coder to make this?


They're all over the place. People just starting out. It could've been an intern fresh out of college. It could've been someone who just never graduated beyond copy-and-paste-from-StackOverflow. It could've been written by a person who never did web development before and was just told to make it work.

The little HN/Twitter/Reddit "awesome programmer" bubble is just that... a bubble. It's easy for us to forget that lots of people write lots of bad, untested code all day long. As much as it frustrates me, lots of people code who don't care about code - it's just their job.


"People just starting out."

Ha, you give them too much credit.

This code is from a 10-year veteran "consultant," probably charging over $200/hour, brought on by the Global Services company hired by the Consulting Agency that Comcast brought in to assist in completing the critical time-sensitive project as quickly as possible.

It was also deemed a great success, and presentations were made about how effective it was, how smart the manager who hired the consulting agency is, and how skilled the global services contractors were who implemented it were, all only 2 weeks behind schedule—a new record for a project of this scope.

That manager got a promotion and is now VP of something or other. He sleeps like a baby and makes 100 times more than you.


This is exactly who wrote this code. I nearly accepted a job with one of Comcast's major consulting partners. My first hint should have been 2 technical interviews in which they were impressed that I used linux... and couldn't tell me a thing about what their day to day looked like. "Oh it's always different"

When I was issued my company laptop, the software had been installed by hand (OS and all). I offered to setup an imaging system for them... but the "IT guy" from the "IT consulting firm" wasnt exactly sure what that was and needed to find out who to get approval from first...


Hey, hey, now. I did include people who never moved on from copy-and-paste too.


They understand that "good code" is code that delivers a lot of value to the person who needs it. Why hate on them for that? You just sound jealous that they make more than you do.


"Good code" that delivers a lot of value and is high quality and maintainable is still better!

I'm not jealous. They don't make more than me. I said they make more than you. And I'm not hating—I'm just telling it exactly like it is, because I understand it, and it's insane, like the truth tends to be when you have huge amounts of power and money being controlled by puny incompetent humans.


Maybe he has had the pleasure of being someone that gets to maintain that "good" code. I know I have.. and at Comcast no less.


That's the whole point. Code is not meant to serve the people who maintain it. Maintainability is only a concern once lack of such starts impacting your actual customers. If writing ugly code and fixing it up later is necessary in order to get shit out the door, why is that bad?


So because it satisfies the suits, he should reserve passing judgement? Try again; he is a programmer, not a suit. Hint: there exist many seperate but equally valid systems for judging worth/merit/quality.

Also, even for a suit, "Maintainability is only a concern once lack of such starts impacting your actual customers." is only true if by "actual customers" you mean shareholders. If you really want to get down to it and make an obnoxious out of place point, you can technically fuck over the customers all you want so long as doing so does not actually hurt the business (meaning: hurt the shareholders). Bonus points for figuring out how this could be done by a consulting company.


If you are a money-chasing robot, perhaps. Most businesses care at least a little about making a good product/satisfying their customers. That's good. Making life easy for your employees at the expense of your customers? That's bad.


Because if you want to be a software developer in the long term, you need to prefer the long term alternative in most cases.

A typical example is, "If we don't get something out the door, we'll be out of business. 'Shit' is something that can be shipped quickly, therefore we must ship 'shit'."

But companies that ship 'shit' generally go out of business anyway. Either their customers find it unappealing and leave, or ongoing maintenance quickly becomes so difficult and expensive that the product can not improve except by being rewritten under new management.

With something like a secure website (or script injected into arbitrary websites by a large ISP) the severity of the security vulnerabilities that tend to result from "shipping shit" often you only get one or two chances as a company.


How is that relevant? Comcast is not a software company. Shipping something that works and then never touching it again is exactly what they want.


>They're all over the place. People just starting out. It could've been an intern fresh out of college. It could've been someone who just never graduated beyond copy-and-paste-from-StackOverflow. It could've been written by a person who never did web development before and was just told to make it work.

I'm an intern, just moving past S.O. copy-pasta jobs and generally get scared at what the hacker news crowd might say about my code... seeing this caliber of shit get pushed live by a major ISP is almost comical, if an admitted novice such as myself can see that it should be a sign as to the ineptitude of our current crop of ISPs.


The code on your GitHub, for the most part, seems fine.

One thing I can say is don't use exec[1] if you can avoid it:

    $string = 'rm /var/www/Giftest/*.gif';
    exec($string);
While there's nothing * technically* wrong, it's platform specific and I think it would be better to use PHP's unlink[2] function. Also, sorry if this is wrong, I haven't looked at the regex but it seems your parsing YouTube URLs? Have you looked at oEmbed[3] - it may be an easier way to accomplish what your doing? You can use it with json_decode[4] to get an object.

[1] https://github.com/Machtap/GiffyTube/blob/master/download.ph...

[2] http://php.net/manual/en/function.unlink.php

[3] http://apiblog.youtube.com/2009/10/oembed-support.html

[4] http://php.net/manual/en/function.json-decode.php


would you be willing to discuss this further? No contact in your profile.


Sorry for the delay in getting back to you. Check your emails.


The type of programmer who writes code like this never wonders whether their code could be better or not. So don't worry, just by being self-aware enough to ask the question you put yourself on a higher level.


One thing I have learned over the years: It is easy to write "this is crap code" over a lot of production code I have seen. But making it better, writing consistently great code in the usual environment is much harder.

Don't let the macho attitude of HN infect you too much - a lot of people here (and elsewhere) are great in criticizing others.


+1. When you have whole pile of pretty bad code to maintain, it is very difficult to make the fixes significantly better within the time you have to make the fix. Usually significant improvements would require extensive refactoring which is feasible or sensible in surprisingly few cases.

Though, I have to admit, bitching about other peoples' code is fun.


You should be scared... what's with the hardcoded login info exposed on github?

https://github.com/Machtap/_ctv/blob/master/_www/model/commo...


The database doesn't accept external connections, out of curiosity, what is the proper way to pass connection credentials?


at a minimum:

- keep config variables in a separate file that is in your .gitignore and won't get pushed to github.

- keep config file outside of any web accessible directory in case the file renders in plaintext for some reason.

Regardless of db only accepting local connections - an attacker is one step closer to dumping the db.


Hrm, is there really a problem if the test data on my development server were to get dumped? It's not like those credentials or the accounts stored in the db carry over when this gets deployed to production, nor will the changes for production ever come close to my github.

Still very valuable things to be aware of in future situations where the above might not apply, thank you very much.


Keep it up - keep moving up and learning more stuff. Be awesome. Don't worry too much about what other people think of your code, worry just enough that it pushes you to write better code. :)


I agree, they're everywhere - but it doesn't explain this well-written RFC[1] to accompany the code.

[1] http://tools.ietf.org/html/rfc6108


From the RFC:

> R3.1.1. Must Only Be Used for Critical Service Notifications Additional Background: The system must only provide critical notifications, rather than trivial notifications. An example of a critical, non-trivial notification, which is also the primary motivation of this system, is to advise the user that their computer is infected with malware, that their security is at severe risk and/or has already been compromised, and that it is recommended that they take immediate, corrective action NOW.

So much for that.


Probably written by a different person.


You know, there is nothing wrong in picking up code from stack overflow. If you are very efficient in picking up good, well written snippets that fit the style of the project and work without debugging and any time waste - more power to you.

Personally, I consider google search (and stack overflow) as an extension of my development environment and I'd recommend using it and melding your dev env with google search as much as possible. It really helps and speeds things up.


I agree - there's nothing wrong with picking up code from SO if you trust it and understand it. But that takes experience .

I've come across lots of situations where the accepted answer isn't the best answer.

So I'm talking about knowing vs. cargo-culting.


Honest question: Why is everyone so cynical and brash here?

Is it because we don't like Comcast?


> Honest question: Why is everyone so cynical and brash here?

It's a rockstar developer thing. You wouldn't understand.



Unpaid/underpaid interns!


That moment you see document.write()'ing style into the document... Yikes.


soooo is there a browser plugin to block stuff like this yet?


noscript.


Actually, I dont' think noscript/notscript will block this. It blocks loaded javascript, not inline stuff.


Any chance you can elaborate a bit? I'm not as familiar with how JS is loaded and exactly what noscript does as I should be.


I wondered why I was seeing `checkBulletin()` in my logs.


canceled my services with comcast because of this, plus FIOS has an excellent offer around my area! :)




Guidelines | FAQ | Support | API | Security | Lists | Bookmarklet | DMCA | Apply to YC | Contact

Search: