Guard Before the Payload: Requirements Are Global-Write Only
A cross-module document checklist could be rewritten by any account holding a service write permission. The guard now refuses before reading the payload.
TL;DR
Permission gate was too narrow. Added scope check to write path. Guard now runs before validation.
I grepped for the write permission across every handler in KotaPortal and opened UpdateItems in the document requirements service. The first statement after the signature was a payload validation. Not a scope check. The validation ran against whatever the caller had sent, and only once it passed did the service reach for a repository. That ordering bothered me more than the missing check.
The checklist behind FRM-01 and FRM-02 decides which cross-module borrowing requests are allowed through. If those rows are wrong, requests get approved that should never have reached an approver. Any account holding layanan:write could rewrite them. The permission gate was doing exactly what it was told; the problem was that the job it had been given was too small for what the endpoint controls.
The permission was right, the scope was missing
The read/write permission sets were not the weak point. That separation held up fine. What had no representation anywhere in the write path was the bidang dimension: an officer trusted with writes inside one bidang could rewrite a document that governs all of them. Least privilege is not satisfied by a verb-level permission when the resource is not verb-level in nature [2].
The same source puts it plainly: access should be denied by default, and it should not depend on the caller meeting some other condition first [2]. A write guard that only trips when the payload is well-formed is not a default deny. It is a conditional deny with an attacker-controlled precondition.
Guard before validation, before the repository
Commit ebe34a2 moved the decision to the top of the service. Two additions: a sentinel ErrForbidden, and a five-line guard between the entry point and everything else.
func guardWrite(bidang string) error {
if scope.IsGlobal(bidang) {
return nil
}
return ErrForbidden
}The handler now pulls middleware.BidangFrom(r.Context()) and hands it into svc.UpdateItems, so the service never has to trust a bidang supplied by the request body. The guard runs before the form validation block and before any repository method call. That placement is the whole point.
Validation is work performed on behalf of the caller. Refusing is not. If validation runs first, an unauthorized caller learns which fields the schema expects, one error message at a time, on a resource they are not allowed to touch. Ordering the guard above validation means the scope decision never waits for a valid payload, and the refusal costs nothing but a comparison.
Returning a sentinel instead of a preformatted error keeps the split clean. The service states the outcome; the HTTP layer decides what that outcome looks like on the wire. guardWrite has no opinion about status codes and does not need one.
What the refusal says, and what it withholds
respondError gained one case: errors.Is(err, ErrForbidden) maps to 403 FORBIDDEN with a static detail string. The server understood the request and declined to fulfill it, which is precisely what 403 means [5]. The failure is tied to application logic rather than to anything malformed in the request [6], and a client repeating the call unchanged will get the same answer [6].
The detail is a constant: the document requirements are managed only by the dinas admin. A code comment next to it says the requester value is never echoed. That comment is the part I would defend hardest in review. A dynamic detail naming the caller's bidang would turn a refusal into a probe: send the write, read back which scope you actually hold, adjust. Constants cannot be mined.
Reads stayed open on purpose. List still crosses bidang boundaries, because an officer verifying a submission needs to consult the checklist even when the rows were authored elsewhere. Locking reads down would have broken the verification workflow to fix a write problem. The permission gate itself is unchanged; only the write path gained a floor.
The test that keeps this honest is TestUpdateItemsGlobalOnly. It loops three values into the guard: budaya, olahraga, and the empty string. For each, it asserts two things, and the second is the interesting one. It checks for ErrForbidden, then checks that repo.calls == 0. The fake repository increments a counter on every method, so any deny path that still touched storage fails the suite. Without that counter the test would pass on a guard sitting in the wrong place.
Integration tests on the happy paths now pass middleware.BidangAll, which keeps them exercising the success route rather than accidentally asserting a 403 that happens to look like a pass.
Review approved the commit with one informational note. A malformed JSON body still produces a 400 from the decode step before the service can return 403. Decode order reveals nothing about authorization state, only about request shape, so it is not an oracle. I accepted the note and left the order alone.
The five lines are not what fixed this. Their position is. Every write handler I touch now gets the same read: what is the cheapest true statement this function can make, and does it come before the expensive ones.
Sumber: