Skip to content

The edit form one bidang should never have opened

Adityo Guni Waluyo

Role checks answer who may manage staff, never which row they may touch. A row-ownership guard closes the gap.

TL;DR

Someone edited a staff record from a different bidang. The issue was caused by a lack of object-level authorization. A guard function was added to prevent this.

# The edit form one bidang should never have opened

The QA account belonged to a single bidang. It existed to test that bidang's screens and nothing else. Then someone pasted the staff edit URL of a different bidang into the address bar, changed the id in the path, and the form loaded: name, email, role, status. No warning, no redirect. The save button worked too.

The module did have role checks. Admin may manage staff, staff may manage nobody, and a middleware rejected every request without an admin session. Those checks passed. The test that would have caught this was never written, because nobody had framed it as something that could break.

The first guess was that the role check covered it. An admin is an admin, so an admin session may edit any staff record. That holds right up to the moment the CMS stops having one admin. KotaPortal runs per bidang: each division has its own operators, its own staff rows, its own queue of citizen requests. Admin rights inside a bidang are not admin rights over the whole installation.

OWASP names the pattern. Broken Object Level Authorization sits at position one of the API Security Top 10 for 2023 [3], and on the web side it folds into Broken Access Control, A01:2021 [2]. CWE-639 calls this specific shape Horizontal Authorization: two users hold the same privilege level, and each must still be prevented from reaching the other's resources [4]. The definition matters because it removes the excuse. Nobody had more privilege than they were entitled to. They had the right role pointed at the wrong row.

The guard runs after the load

The first repair is a function called guardTargetBidang. It runs after the record is fetched from the repository and before any mutation touches it, on both Update and Delete. Placement after the load is deliberate: the guard needs the record's bidang to compare against the actor's, and a check written before the fetch has nothing to compare. Placement in the service layer rather than in the handler means every entry path passes it, including the ones added next quarter.

The second repair is the collection. Staff listing is filtered to the actor's bidang, so the repository List and Count both take a bidang parameter.

A guarded detail endpoint in front of an unguarded list is half a fix. The list is where an operator learns which ids exist, and an id you can see is an id you will try.

Third, the error shape. The CMS returns one static ErrForbidden whether the target does not exist or exists under another bidang. The trade-off is written into the pull request instead of being accepted silently. Collapsing both cases removes a 403-versus-404 oracle, at the cost of being vague to a legitimate operator who mistypes an id. Residual risk is rated LOW, because staff ids are sequential and enumerable in a handful of requests regardless.

What the guard does not cover

Citizen accounts stay exempt. They carry role user, they belong to no bidang, and cross-bidang access to them is intended: a request filed in one division may be read by staff helping with it. A blanket guard would have broken the support workflow the CMS exists to serve.

Four things worth keeping. A role check answers who you are, never which row you may touch. A list endpoint without the same filter is a broken-object surface of its own [1]. The guard belongs in the service layer, after the load, so no path skips it. And an untested guard is one refactor away from deletion.

The guard did not arrive alone. The same change carries a 157-line unit test file: an admin of one bidang attempts Update and Delete on another bidang's accounts, the superadmin and global actors pass through, citizen accounts stay untouched by the guard. Repository and handler tests were adjusted in the same commit. The test is what keeps the guard there through the next refactor.

[1] OWASP Authorization Cheat Sheet

[2] OWASP Top 10 2021: A01 Broken Access Control

[3] OWASP API Security Top 10 2023: API1 Broken Object Level Authorization

[4] CWE-639: Authorization Bypass Through User-Controlled Key

Related articles