We found and fixed a rare race condition in our session handling

It is funny how they make it seem like they are super cautious. When in fact they had no other choice than do it when you rephrase the problem as " there were maybe some people who got the admin right of the github account of some of our customers"

They are lucky this wasn't much, much worse.

Why does the error reporting service need to call the web thread back anyway?

Yet another case where immutable-everything would have prevented this hairball from accreting in the first place.

Sobering thought for me is how probable it is that a similar issue might be present in other platforms. The title calls it "rare" but given (a) the number of complex-enough web/app platforms in use today, (b) just the endless number of ways/infra permutations for "doing x", and (c) the size of a typical company's codebase and dependencies, could it be a more common occurrence than we think?

Total anecdote, up for you to believe: Back in April 2016, one of my then-housemates bought a brand-new Macbook Air. A week into his ownership, he exclaims surprise that he is logged-in to another Facebook account, someone we are totally not acquainted with. I borrow his MBA and try to investigate but, alas, I lack expertise for it. The only relevant thing I can eke out is that they both went to the same event a week or so ago and probably shared a Wifi AP, where a "leak" could've happened. Or maybe a hash collision in FB's side?

I would've reported it to FB but then I don't have enough details; with the two conjectures I have, I'm basically holding water with a sieve. But now the thought that someone I totally don't know might end up logged-in in my FB account is a possibility that bothers me. And I have no idea how to prevent it other than minimizing what could be compromised (and no, "Get off FB!" just isn't in the cards, I'm sorry).

Kudos for Github for investigating and fixing this issue. Another thought that occurred to me while writing this is how many users of other platforms might be filing issues like this but can't provide enough detail and so their reports are eventually marked as "Could Not Reproduce". Not exactly leaky sessions but just general security mishaps that they would struggle to describe to support.

What we know based on this report:

- This bug was live in production for at least 3 days.

- It was common enough to happen numerous times in the wild.

- GitHub says that they identified some specific user sessions that may have been impacted by this bug by analyzing their logs.

What we do not know:

- How many leaked user sessions were identified in the followup investigation?

- Is GitHub confident that they identified all leaked user sessions?

- Was there any evidence of malicious use of these sessions?

- Did GitHub notify the users whose sessions were leaked?

- How long/for how many requests could these sessions have been valid between their leak and when they were definitively revoked on the 8th?

- Is it possible that any private user data was leaked? If so, what data?

Impressed with GH folks in finding this, though I wonder how many more of these types of bugs are out there in Ruby libs. It'd be nice to make this more difficult to do, a la FP languages, by making it really intentional to modify state. Maybe in Ruby 4??

I believe that this could be addressed with an architecture that favored single-threaded processes that can handle a single request at a time. Said processes would be numerous, have a (best-effort) low memory footprint, and be pooled under some master process written in a concurrent/performant manner.

The result wouldn't be necessarily pretty or resource-efficient (especially in terms of RAM), but it also can be seen as a consequence of Ruby's requirements for safety.

An org like GH almost certainly can afford using more/beefier servers it it means that entire classes of bugs will go away.

ANY time ruby 1.8 raised a timeout, it was silently swallowed, and Unicorn returned the adjacent request session_id. We had to constantly defend against any code that might hit the native timeout.

So they already conceded that their code is fundamentally not thread-safe and they decided to let it slide.

It's never a good idea to perform multithreaded operations within the web server, unless you have a strong knowledge of its internal workings. For instance, if you miss manage threads in an IIS web application you can end up having your web requests queued since the web server has by default a fixed number of worker threads to serve web requests.

From their bug description it's clear that ruby on rails is no exception to this rule. In such cases where background processing is required due to some condition triggered by a web request it's best to stick with a propper queue system [1]

[1] https://thomasvilhena.com/2019/07/using-queues-to-offload-we...

Immutable data forces you to design your system in a different way and remove layers of complexity.

This type of thing is why PHP's shared-nothing model is vastly undervalued IMO.

Does PHP have the ability to use threads if you really need it? Yes.

Are they generally a PITA to use? Yes.

Is this a problem for the vast, vast, vast majority of projects? Not in the slightest.

Having a separate invocation of the runtime for each request is a feature, not a failing.

But I don't see the old behavior described as "allocates one single Ruby Hash that is then cleared (using Hash#clear) between each request"...even after poking around in http_request.rb and other places. I reads like the pre-patched version would just re-use the hash as-is, only overwriting/adding if it saw a new key or value, but not deleting any keys.

[1] https://yhbt.net/unicorn-public/66A68DD8-83EF-4C7A-80E8-3F1F...

Who in the heck thought this would be a good idea? For the record, back when Desk.com existed, I also saw strange session issues that were probably complex and irreproducible race conditions as well.

Note that in Elixir/Phoenix, this class of bug is literally impossible, thanks to immutability-all-the-way-down. Turing bless it.

To prevent these kind of problems we have a few approaches, but the main way is to prevent shared mutable state. To do this we have a custom C# code analyzer (source here: https://github.com/Brightspace/D2L.CodeStyle/tree/master/src... , but it's not documented for external consumption at this point... ImmutableDefinitionChecker is the main place to look.) It goes like this:

  [Immutable]
  public class Foo : Bar {
    // object is a scary field type, but "new object()"
    // is always an immutable value.
    private readonly object m_lock = new object();
  
    // we'll check that ISomething is [Immutable] here
    private readonly ISomething m_something;
  
    // Fine because this lambdas in initializers can't
    // close over mutable state that we wouldn't
    // otherwise catch.
    private readonly Action m_abc = () => {};
  
    // see the constructor
    private readonly Func<int, int> m_xyz;
  
    // danger: not readonly
    private int m_zzz;

    // danger: arrays are always mutable
    private readonly int[] m_array1 new[]{ 1, 2, 3 };

    // ok
    private readonly ImmutableArray<int> m_array2
      = ImmutableArray.Create( 1, 2, 3 );
  
    public Foo( ISomething something ) {
      m_something = something;
  
      // ok: static lambdas can't close over mutable state
      m_xyz = static _ => 3;
  
      // danger: lambdas can in general close over mutable
      // state.
      int i = 0;
      m_xyz = x => { i += x; return i; };
    }
  }

Any class that were to extend Foo (as a base class) will be required to be [Immutable] as well. There's a decent number of aspects to this analysis (e.g. a generic type can be "conditionally immutable" -- ImmutableArray<int> is, but ImmutableArray<object> is not), check the source if you're interested.

We require all static (global) variables be immutable, and any "singleton" type (of which we have tens of thousands if I remember correctly) also must be.

Another important thing we do to is to cut off any deferred boot-up code from accessing customer data (via a thread-local variable that is checked before access through the data layer). This prevents accidentally caching something for a particular customer inside, say, a persistent Lazy<T> (or the constructor for a singleton, etc.)

We've adapted our code base to be very strict about this over the last few years and it discovered many obscure thread-safety bugs and doubtlessly prevented many from happening since.

Also another reason not to mix the main application code with the authentication/authorization code. Setting security cookies should be done by dedicated code in a dedicated process to avoid this kind of interaction, and the load-balancer layer should filter the setting of security cookies from non-security backends to enforce it.

There are of course safe ways to do all of this kind of stuff, but there are _so many_ unsafe ways to do things, and accidentally share things across requests. At least in the Django world I think ASGI is going to make it more likely that stuff is done the right way but there's a big learning curve there.

Shared memory environments are very unsafe and we should be working towards a memory model where data that belongs to different organizations can never get mixed. This isn't impossible, but it requires new programming languages that allow us to express which memory areas can't get mixed up.

We've got to start designing our backend infrastructure differently such that this entire class of memory errors can't ever occur.

It saved me more than once in similar situations with several threads.

I'm not judging the choices of technology, but I'm thinking it was easier for them to buy Github outright then try to compete with it. I assume Github still operates independently, with engineers who are familiar with this stack.

> We initially thought this to be an internal reporting problem only and that we would see some data logged for an otherwise unrelated request from the background thread. Though inconsistent, we considered this safe since each request has its own request data and Rails creates a new controller object instance for each request.

I mean this is like an SNL skit: "though inconsistent, we considered this safe" -- inconsistency, no matter where it's happening, literally means your code isn't safe. Like, jeez... this is GITHUB we're talking about. I mean, I give money to GitHub -- this is pretty serious and we should really stop with the "omg I love these detailed post-mortems!" stuff. A billion-dollar company knowingly having code that's not thread safe in their flagship product and going "eh, it's probably fine" is bonkers to me.

Github Actions has had many problems that affect many people this year alone. While the missed deadlines and stress for people can't be quantified by Github, they are happening.

I'd urge Github to take ongoing service interruptions in Github Actions more seriously. I hope Github will take a stronger stance on providing customer communication on migration timelines between Packages and Container Registry.

Just because a bug or service interruption doesn't risk random user session exposure doesn't mean it isn't important to communicate about and describe how the business is working to prevent it from happening again.

I find it ironic that the largest repository of open source code is closed source and and owned by the largest closed source software company on the planet, yet they somehow still "believe in transparency."

That's like McDonald's "transparently" recalling burgers because of a chemical contamination, and then when you ask what else is in the burgers they tell you it's a secret.