4 ms·

This seems odd. I mean, if their code was properly modular, they would have just one place where they "fetchUserIdByName(userName)", which returns one user ID

• eli 13y ago
  Easy to implement in a small project. Not so easy in a large enterprise project likely comprised of multiple interconnected systems.
  • TillE 13y ago
    Not a good excuse. There are plenty of enormous projects that provide only one basic interface to a bit of information. See every operating system API for examples.
    • jzwinck 13y ago
      How about the API to allocate memory in Windows? VirtualAlloc: http://msdn.microsoft.com/en-us/library/windows/desktop/aa366887(v=vs.85).aspx http://msdn.microsoft.com/en-us/library/windows/desktop/aa36... VirtualAllocEx: http://msdn.microsoft.com/en-us/library/windows/desktop/aa366890(v=vs.85).aspx http://msdn.microsoft.com/en-us/library/windows/desktop/aa36... VirtualAllocExNuma: http://msdn.microsoft.com/en-us/library/windows/desktop/aa366891(v=vs.85).aspx http://msdn.microsoft.com/en-us/library/windows/desktop/aa36... It all started out nice and clean I'm sure, but within a few years you start to see many more than one basic interface to some things.
      • brokenparser 13y ago
        And {Global,Heap,Local}{,re}{Alloc,Free,Lock,Unlock}. 4 sets of APIs just to allocate memory. Although Local* and Global* are mapped to Heap for a while now and (IIRC) (un)lock functions don't do anything (but they used to). This BC is still required today.
• dan_manges 13y ago
  Based on their description of the bug, it sounded like the code was modular, but they called the function twice: once when the password reset request was generated, and again when the link in the email was clicked. However, when the link was used, canonical_username was once again applied So after they sent the password reset link, they called "fetchUserIdByName" again, but they passed in a username that had already been canonicalized once. Because of this bug, I wonder if password resets worked at all for users with unicode characters in their names.
  • mmahemoff 13y ago
    If you're saying canonicalise(canonicalise(name)) is not the same as canonicalise(name), that's going to be seriously bug-prone. Idempotence ftw.
    • deleted 13y ago
      [deleted]
    • spicyj 13y ago
      That's exactly what they describe as the cause of the bug. They intended for the function to be idempotent but it wasn't because of a misunderstanding with the Python library spec.
      • Xorlev 13y ago
        Worse, it /did/ work that way in Python 2.4 but Python 2.5 stopped throwing an exception for invalid codepoints which broke the Twisted library which broke their canonicalization function.
    • sanderjd 13y ago
      You should read the article. It prominently features a very interesting description of precisely why their `canonicalise` function turned out to not be idempotent, even though it was meant to be.
    • frogpelt 13y ago
• shawnz 13y ago
  This is what I thought at first. If it's the exact same check (which it should be), why is there any possibility of the answer being different? But the real "bug" here is that they were treating the canonicalized username as if it were just as good as the original username, which is only true if the adjustment function is idempotent as they say. Another possible solution would be to assume the username given to the password reset form is already canonicalized (which would be necessarily true, as far as I understand). EDIT: However this wouldn't solve the other bug that's been discovered here, which is that "ᴮᴵᴳᴮᴵᴿᴰ" is canonicalized differently than "BIGBIRD", thus defeating the purpose of canonicalization (for that particular case) in the first place.
• brown9-2 13y ago
  Sounds like they are using the username as the key in their DBs, which sounds like the ultimate case of any pain: Could the method for computing canonical usernames based on nodeprep.prepare() be salvaged? If not we would be in trouble since we use canonical usernames in various databases so that changing how to derive them in a non-backwards compatible way would be quite costly.
  • tantalor 13y ago
    Not necessarily. It their canonicalization function were idempotent (e.g., the identity), then this database scheme would work well. How else do you map username to user id?
    • brown9-2 13y ago
      Well generally it makes sense to use that userid as a primary key everywhere rather than the username. You only need the mapping of username to userid in one place. Their current scheme also makes it sound hard to change your username.
      • tantalor 13y ago
        Ah I fully agree. I misunderstood your post to say you should never use a user name as a key.
• tantalor 13y ago
  What you're talking about has nothing to do with modularity. You're thinking of DRY. They having nothing to do with each other. DRY tells us the code for looking up a user id by user name should be written in only one place, not that the code should be called from only one place. The mistake was assuming the name->name function was idempotent, because it wasn't. You are right to suggest using a name->id function instead. It would not suffer the same problem because the canonical name should not be stored... it's an implementation detail!
  • Confusion 13y ago
    They having nothing to do with each other. DRY tells us the code for looking up a user id by user name should be written in only one place, not that the code should be called from only one place. If you write 'canonicalize(username)' in eight different places, you are not being DRY. If you need to write 'canonicalize(username)' in eight different places, your code probably doesn't separate responsibilities properly across separate modules. As such, they have a lot to do with each other. After all, if you call code from more than one place, you are writing the calling code in more than one place. Lack of DRYness is about the fact that you are doing so. Lack af modularity is about why you need to do so.