Allowlist lookups fail unsafe in `abuse-enforcement-service`: a transient store error removes enforcement protection from exempted accounts

[keithadler]

Reading through abuse-enforcement-service at c65aa17, I noticed that the allowlist check, which is the mechanism that exempts an account from automated enforcement, treats a store error the same as "not on the allowlist." Because a neighbouring lookup in the same decision block does the opposite, I think this is an inconsistency worth surfacing rather than a deliberate choice.

Observation

The allowlist read collapses two distinct failures to None:

https://github.com/xai-org/x-algorithm/blob/c65aa17/abuse-enforcement-service/service-lib/src/allowlist.rs#L57-L81

let item = match self.client.get(...).await {
    Ok(v) => v?,
    Err(e) => {
        warn!("Manhattan allowlist GET failed: {e}");
        crate::metrics::MANHATTAN_ERRORS_TOTAL.inc();
        return None;                       // transport/store error -> None
    }
};
let entry: AllowlistEntry = serde_json::from_slice(...).ok()?;   // malformed record -> None

Upstream, fetch_user_allowlist maps that None onto AllowlistFacts::default(), i.e. is_allowlisted: false:

https://github.com/xai-org/x-algorithm/blob/c65aa17/abuse-enforcement-service/service-lib/src/strato.rs#L85-L88

and the decision path then proceeds to evaluate enforcement rules instead of taking the user_in_allowlist skip:

https://github.com/xai-org/x-algorithm/blob/c65aa17/abuse-enforcement-service/service-lib/src/lib.rs#L221

So a None from the allowlist can mean either "this account is not exempt" or "we could not determine whether this account is exempt," and the engine cannot tell them apart. Both lead to enforcement proceeding.

Why this looks like a defect rather than an intended choice

In the same decision block, the user-profile and credibility reads handle errors the opposite way:

https://github.com/xai-org/x-algorithm/blob/c65aa17/abuse-enforcement-service/service-lib/src/lib.rs#L256-L257

user: user_res?,   // gizmoduck error -> propagates -> decision aborts -> no enforcement
cred: cred_res?,   // cred error      -> propagates -> decision aborts -> no enforcement

These fail closed: an error aborts the decision, so nothing is enforced on missing data. They also retry, via strato_retry_strategy:

https://github.com/xai-org/x-algorithm/blob/c65aa17/abuse-enforcement-service/service-lib/src/strato.rs#L36-L37

The allowlist read does neither. It has no retry, and its error resolves to "not protected." The one lookup whose purpose is to shield an account from automated action is the one lookup that fails toward action, and it is less resilient than the reads around it.

Impact

When a Manhattan allowlist read errors transiently, or a single stored AllowlistEntry is malformed, an account that is on the allowlist loses its exemption for that evaluation. If it also matches an enforcement rule in the same evaluation, it can be labelled or suspended, up to ActSuspendUser { perm: true }.

The allowlist is a manual operator override, its records carry added_by and reason, so the accounts it protects are exactly those someone deliberately marked as not-to-be-enforced (for example known false-positive-prone or test accounts). Those are the accounts this silently exposes. The only trace is a warn! line and a generic MANHATTAN_ERRORS_TOTAL counter with no entity id, so an incident would be hard to attribute after the fact.

To be precise about scope: this is not "every account gets enforced." The precondition is an account that is both on the allowlist and rule-matching during the window of the store error or bad record. It is triggered by an infrastructure fault or data corruption, not by any external input, so it is a reliability and correctness issue rather than an externally exploitable one.

Suggested direction

Make the exemption read fail the way the rest of the block already fails:

1. Change get_entity to return anyhow::Result<Option<(AllowlistEntry, i64)>> so a store error is distinguishable from a genuine absence, rather than both being None.
2. Give it the same retry wrapper the cred and user reads use (strato_retry_strategy).
3. On an unresolved error, either abort the decision (fail closed, matching user_res? / cred_res?) or explicitly skip enforcement, rather than proceeding as "not allowlisted."
4. Optionally, treat a deserialization failure on a present record as an error rather than absence, so a format change cannot silently drop existing exemptions.

Happy to open a PR along these lines if that is a direction you would accept. I have not done so yet because it changes an error-handling contract (Option to Result) that several call sites depend on, and I would rather confirm the intended failure direction with you first. I also note that abuse-enforcement-service ships without a Cargo.toml in this repository, so I cannot compile or test the change here.

Note

I can only speak to the code as published. If retries or an error-distinguishing cache sit in front of this read in production, neither is visible in this repository, and this can be closed.