September 1, 2026

When Your Sort Function Lies to You: How a Broken Comparator Silently Bypassed Security Rules in…

Some vulnerabilities come from missing input validation. Some come from a forgotten authentication check. This one came from something far…

By k3tu

4 min read

Some vulnerabilities come from missing input validation. Some come from a forgotten authentication check. This one came from something far more mundane: a single Less() function that violated the basic mathematical rules a sort comparator is supposed to follow, and quietly let denied actions slip through as allowed.

What Is Allstar

Allstar is an OSSF (Open Source Security Foundation) project that enforces organization-wide security policies on GitHub repositories. One of its features is an "action policy," which lets administrators write rules that allow, require, or deny specific GitHub Actions. When configured correctly, a well-placed deny rule should always win over a lower-priority allow rule. That guarantee is exactly what broke.

The Bug, in Plain Terms

Allstar evaluates action policy rules in priority order: critical, then high, then medium, then low. Within the same priority tier, allow and require rules are supposed to be checked before deny rules. To make that happen, Allstar sorts its rule list before evaluating it, using a custom comparator function called Less().

Here's the code that shipped:

go

func (s sortableRules) Less(i, j int) bool {
    if s[i].priorityInt < s[j].priorityInt {
        return true
    }
    if s[i].Method != "deny" {   // no priority guard
        return true
    }
    return false
}

The second if statement is the problem. It fires whenever the rule at index i isn't a deny rule, no matter what priority that rule actually has relative to the rule it's being compared against. The tiebreaker that's only supposed to matter within a priority tier ends up overriding priority itself across tiers.

Why That Breaks Everything

Go's sort.Interface requires a comparator to satisfy some basic mathematical properties, the same ones any correct ordering relation needs: irreflexivity (an item can never be "less than" itself) and asymmetry (if A is less than B, then B cannot also be less than A). This comparator violates both.

Take a low-priority allow rule and a high-priority deny rule. Priority numbers work in reverse here (lower number means higher priority), so a deny rule at priority 1 should always sort before an allow rule at priority 2. But run both directions through Less():

Less(allow-P2, deny-P1):
  2 < 1              -> false
  "allow" != "deny"  -> true
  returns: true
Less(deny-P1, allow-P2):
  1 < 2              -> true
  returns: true

Both directions return true. That's not just wrong, it's logically impossible in a valid ordering. When you hand Go's sort.Sort a comparator like that, the standard library makes no promises about what comes out. In practice, what came out depended entirely on the order the rules were written in the config file.

The Real-World Consequence

Allstar evaluates rules top to bottom and stops at the first match. So if a bad sort silently placed a lower-priority allow rule ahead of a higher-priority deny rule, the allow rule would fire first, the action would be permitted, and the deny rule further down the list would never even run. No error. No warning. No GitHub issue created flagging the violation. Just a policy that looks correct on paper and does something else entirely at runtime.

Worse, the failure mode tracks almost perfectly with normal human behavior. An administrator reading Allstar's own documentation ("rules are applied in order of priority") would naturally write their most important rule, the deny rule, first in the file. That completely reasonable authoring choice is exactly the one that triggers the bypass.

Why the Existing Test Didn't Catch It

Allstar had a test covering "deny higher priority than allow." It listed the allow rule first and the deny rule second in the test's rule slice. Because of how Go's sort algorithm happens to walk through a two-element slice in that specific order, the buggy comparator produced the correct result purely by coincidence. Swap the order of the two rules in the test (deny first, allow second, the natural way someone would actually write it) and the test fails immediately: the allow rule wins and the deny rule is never enforced.

That's a good reminder that a passing test only proves your code works for the exact input you tested, not that the underlying logic is sound.

The Fix

The corrected comparator adds the priority guard that was missing:

go

func (s sortableRules) Less(i, j int) bool {
    if s[i].priorityInt != s[j].priorityInt {
        return s[i].priorityInt < s[j].priorityInt
    }
    // Same priority tier only: allow/require sorts before deny
    iIsDeny := s[i].Method == ruleMethodDeny
    jIsDeny := s[j].Method == ruleMethodDeny
    return !iIsDeny && jIsDeny
}

Now the method-based tiebreak only ever applies when the two rules are already at the same priority tier, exactly the case it was meant for. Priority differences are resolved first and always win.

Affected Package and Versions

Who Was Affected

The bypass only mattered for a specific configuration pattern: organizations mixing deny rules and allow/require rules across different priority tiers in the same rule group, with the deny rule listed before the allow rule. Organizations using only deny rules, only allow rules, or a single priority tier throughout were never at risk, since the broken branch either never fires or can't produce an inconsistent result in those cases.

The Bigger Lesson

This bug is classified as CWE-670, "Always-Incorrect Control Flow Implementation," and it's a solid case study in a few recurring security lessons:

  • Comparator correctness is a security property, not just a code-quality nicety. A sort function that violates basic ordering axioms doesn't just produce "slightly wrong" output, it produces undefined behavior, and undefined behavior in a security control is a bypass waiting to happen.
  • Passing tests can hide the exact behavior they're meant to verify. A single input ordering gave false confidence for what was likely years of production use.
  • Documentation can describe the intended behavior perfectly while the implementation quietly does something else. The comment right above the sort call even said "first by priority, second by method," which was true in the code's intent and false in its execution.
  • Silent failures are the worst kind. No crash, no error log, no alert. Just a security policy that looks enforced and isn't.

It's a two-branch function. It's also a reminder that security bugs don't need to be exotic to be dangerous, sometimes they're just a missing equality check away from a total control bypass.

Reported against the ossf/allstar project. Root cause identified in pkg/policies/action/action.go, lines 648-657. Classified as CWE-670, CVSS 3.1 score 4.3 (Medium severity).

Reference: