carlos / platform Public

objstore+console: three more tests that read a Deny as the Deny

CI caught one of these and the other two were sitting behind it, silent.
I had reported the previous commit as passing every package; the run I
read exited 0 but predated the change. That is the same mistake as the
one this branch has already paid for once — trusting an exit code
instead of the thing it was supposed to have covered.

TestScanEnforceWritesTheDenyToThePolicy asserted that granting the store
wrote NO deny and that `enforce on` wrote one, both by searching the
policy for `"Effect":"Deny"`. The anti-forgery pair is unconditional, so
the first assertion now failed and the test was right to fail: it was
measuring the wrong thing and only noticed because the wrong thing
changed.

The two beside it are worse, because they are POSITIVE assertions:

  TestGrantWritesDenyOnHealthyStore    "did not write the deny"
  TestGrantClearsDenyWhenEnforceIsCleared  "setup: the deny was never written"

Both would now pass with the enforcement hoist entirely removed — the
anti-forgery Denys satisfy the search on their own. A green test
asserting nothing, in the exact place the hoist is the only thing being
tested. Nothing failed, which is why they needed looking for rather than
waiting for.

All three key on s3:ExistingObjectTag now, which is what "enforcement"
actually means in a policy document. Mutation-tested rather than
asserted: with the hoist in walk.go disabled so rec.Scanning.Enforce
never reaches EnsureUser, both tests fail with their own messages. The
old assertions would have passed.

And two of them gain the mirror they were missing — that turning
enforcement off does NOT take the anti-forgery Denys with it. A store
that has stopped denying infected reads is exactly where a forged clean
tag would sit unnoticed longest.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PEkJh1ESqiA84quAHJV4K6
Paul Campbell pushed by paul@keymail.dev 6f2b74ff498d51a74302090b209e1262c811755a parent a9941d0
2 files changed, +26 −9
  • internal/console/apiscan_test.go +19 −5
  • internal/objstore/enforce_test.go +7 −4
diff --git a/internal/console/apiscan_test.go b/internal/console/apiscan_test.go
index d45bca02d..0dffa612f 100644
--- a/internal/console/apiscan_test.go
+++ b/internal/console/apiscan_test.go
@@ -217,22 +217,36 @@ func TestScanEnforceWritesTheDenyToThePolicy(t *testing.T) {
t.Fatalf("scan grant = %d %v", status, out)
}
user := objstore.UserName(fx.sqid, "titogo")
- if strings.Contains(fake.PolicyFor(user), `"Effect":"Deny"`) {
- t.Fatal("enabling the engine alone wrote a deny — enforcement is a separate switch")
+
+ // The ENFORCEMENT deny specifically, which is the one keyed on the verdict
+ // tag. Every policy also carries the two anti-forgery Denys, unconditional
+ // and unrelated to this switch, so `"Effect":"Deny"` — what this test used
+ // to look for — no longer tells the two states apart. It would now report
+ // enforcement on from the moment the store was granted.
+ const enforcementDeny = "s3:ExistingObjectTag"
+
+ if strings.Contains(fake.PolicyFor(user), enforcementDeny) {
+ t.Fatal("enabling the engine alone wrote an enforcement deny — enforcement is a separate switch")
}
if status, out := postJSON(t, scanURL(fx, "titogo")+"/enforce", map[string]any{"on": true}, token); status != http.StatusOK {
t.Fatalf("enforce on = %d %v", status, out)
}
- if !strings.Contains(fake.PolicyFor(user), `"Effect":"Deny"`) {
+ if !strings.Contains(fake.PolicyFor(user), enforcementDeny) {
t.Fatalf("enforce on never reached the policy:\n%s", fake.PolicyFor(user))
}
if status, out := postJSON(t, scanURL(fx, "titogo")+"/enforce", map[string]any{"on": false}, token); status != http.StatusOK {
t.Fatalf("enforce off = %d %v", status, out)
}
- if strings.Contains(fake.PolicyFor(user), `"Effect":"Deny"`) {
- t.Fatalf("enforce off left a live deny standing:\n%s", fake.PolicyFor(user))
+ if strings.Contains(fake.PolicyFor(user), enforcementDeny) {
+ t.Fatalf("enforce off left a live enforcement deny standing:\n%s", fake.PolicyFor(user))
+ }
+ // And turning enforcement off must not have taken the anti-forgery Denys
+ // with it: a store that has stopped denying infected reads is exactly
+ // where a forged clean tag would sit unnoticed longest.
+ if !strings.Contains(fake.PolicyFor(user), "s3:RequestObjectTagKeys") {
+ t.Fatalf("enforce off dropped the anti-forgery denys:\n%s", fake.PolicyFor(user))
}
}
diff --git a/internal/objstore/enforce_test.go b/internal/objstore/enforce_test.go
index 9e64f6de0..bc3bb6193 100644
--- a/internal/objstore/enforce_test.go
+++ b/internal/objstore/enforce_test.go
@@ -179,8 +179,11 @@ func TestGrantWritesDenyOnHealthyStore(t *testing.T) {
"bab", "titogo", "op@example.com", "", time.Now().UTC()); err != nil {
t.Fatal(err)
}
- if !strings.Contains(p.PolicyFor(user), `"Effect":"Deny"`) {
- t.Fatalf("re-walking a healthy store did not write the deny — the hoist is not working:\n%s",
+ // s3:ExistingObjectTag, not `"Effect":"Deny"`: every policy carries the two
+ // anti-forgery Denys now, so the looser check would pass with the hoist
+ // entirely broken — a green test asserting nothing.
+ if !strings.Contains(p.PolicyFor(user), "s3:ExistingObjectTag") {
+ t.Fatalf("re-walking a healthy store did not write the enforcement deny — the hoist is not working:\n%s",
p.PolicyFor(user))
}
}
@@ -208,8 +211,8 @@ func TestGrantClearsDenyWhenEnforceIsCleared(t *testing.T) {
"bab", "titogo", "op@example.com", "", time.Now().UTC()); err != nil {
t.Fatal(err)
}
- if !strings.Contains(p.PolicyFor(user), `"Effect":"Deny"`) {
- t.Fatal("setup: the deny was never written")
+ if !strings.Contains(p.PolicyFor(user), "s3:ExistingObjectTag") {
+ t.Fatal("setup: the enforcement deny was never written")
}
stored, etag, err = Get(ctx, s, "bab", "titogo")