fix: propagate policy parse errors in string adapter LoadPolicy

[zjncs]

Description

stringadapter.Adapter.LoadPolicy discards the error returned by persist.LoadPolicyLine:

for _, str := range strs {
    if str == "" {
        continue
    }
    _ = persist.LoadPolicyLine(str, model)
}

A malformed line (e.g. an unterminated quoted field: p, alice, data1, "read) is therefore silently skipped: LoadPolicy returns nil, the model simply lacks the rule, and Enforce returns false with no error — the caller has no way to notice the typo'd policy.

The file adapter propagates the same error (persist/file-adapter/adapter.go loadPolicyFile returns handler(line, model) errors), and LoadPolicyLine/LoadPolicyArray return errors precisely so callers can report malformed policies (see also #1106). This PR aligns the string adapter with that contract by returning the error.

Testing

• Added Test_LoadPolicyMalformedLine in persist/string-adapter/adapter_test.go: an adapter line with an unterminated quoted field must make LoadPolicy return an error.
  • Before the fix: FAIL — LoadPolicy() error = nil, want a parse error for the unterminated quoted field
  • After the fix: PASS
• All existing tests in the package (Test_KeyMatchRbac, Test_SavePolicyRoundTripWithCommas, Test_StringRbac) still pass, and the full go test ./... suite passes (8 packages, no failures).
• go vet ./persist/string-adapter/ clean.

Behavior change is limited to inputs that previously produced a silently-wrong model; well-formed input loads exactly as before.

---

This PR was prepared with AI assistance (GitHub Copilot / ZCode autonomous agent). All findings were verified manually against the codebase before submission.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[bitflicker64]

Direction is right and the fix is one I would want: discarding that error hides a typo'd rule, and LoadPolicyLine returns an error precisely so callers can act on it. The error-propagation parity with the file adapter that you describe does hold. Two things to sort out before merge, one of them a regression.

loadPolicyFile also does strings.TrimSpace(scanner.Text()) before handing the line to the handler (persist/file-adapter/adapter.go:105), and the string adapter splits on \n and passes the raw line through. That difference was invisible while the error was discarded. Now it turns a previously harmless input into a hard load failure:

"   # a note"  ->  missing required section #     (was: silently skipped)

It also sits next to a pre-existing panic on whitespace-only lines, which str == "" does not catch. That one is not yours and predates the PR, but this change is the natural place to close it, and the same trim does both. Measured on this branch at a2fac65 with go1.27. Suggested inline.

Second, the new test is weaker than it looks. It runs against a bare model.NewModel() with no model text loaded, and on that model a well-formed line errors too:

"p, alice, data1, read"    ->  missing required section p
"p, alice, data1, \"read"  ->  parse error ... extraneous or missing " in quoted-field

So the assertion cannot tell the unterminated quote from an unconfigured model, and it would keep passing if the quoting behaviour regressed outright. Suggested a version that loads a real model and pins the error, inline.

Worth a line in the description rather than a code change: this also makes a g line error out against a model with no g section (missing required section g), which was previously ignored. That looks like the behaviour you want, but it is not covered by "behavior change is limited to inputs that previously produced a silently-wrong model", since that input is well formed. Anyone loading a shared policy string into several models will meet it.

Worth knowing for scope: Enforcer.loadPolicyFromAdapter loads into a scratch copy and only swaps on success (enforcer.go:398-401), so through NewEnforcer and Enforcer.LoadPolicy a returned error leaves the live model untouched. The new error path is only visible to code calling adapter.LoadPolicy on its own model.

[bitflicker64]

Trim before the handler, the way loadPolicyFile does, so the two adapters agree on what a line is and not just on error propagation.

Without it, " # a note" becomes missing required section # where it used to be skipped. It also leaves " " panicking in LoadPolicyArray at key[:1], because str == "" does not catch a whitespace-only line. To be clear, that panic is not introduced by this PR: I reproduced it on the pre-change loop body too. The trim just closes it for free.

Suggested change

	if str == "" {
	continue
	}
	_ = persist.LoadPolicyLine(str, model)
	if err := persist.LoadPolicyLine(str, model); err != nil {
	return err
	}
	if line := strings.TrimSpace(str); line != "" {
	if err := persist.LoadPolicyLine(line, model); err != nil {
	return err
	}
	}

strings is already imported. With this applied, both inputs above load cleanly, the malformed-quote case still errors, gofmt and vet are clean, and go test ./... is green.

[bitflicker64]

Not blocking, just so it is a deliberate choice: returning here leaves model holding every rule parsed before the bad line. The file adapter does the same, so this is consistent rather than new, and Enforcer.loadPolicyFromAdapter loads into a scratch copy and swaps only on success, so the partial model is not reachable through NewEnforcer or Enforcer.LoadPolicy. It is visible only to code calling adapter.LoadPolicy on a model it owns.

[bitflicker64]

This does fail before the change, so it earns its place. But it is close to vacuous: model.NewModel() has no model text loaded, and against that model a well-formed line errors too.

"p, alice, data1, read"    -> missing required section p
"p, alice, data1, \"read"  -> parse error ... extraneous or missing " in quoted-field

The assertion cannot tell those apart, so it would keep passing if the csv quoting behaviour regressed entirely. Loading a real model and pinning the error message fixes both:

Suggested change

	func Test_LoadPolicyMalformedLine(t *testing.T) {
	a := NewAdapter(`p, alice, data1, "read`)
	m := model.NewModel()
	err := a.LoadPolicy(m)
	if err == nil {
	t.Fatal("LoadPolicy() error = nil, want a parse error for the unterminated quoted field")
	}
	}
	func Test_LoadPolicyMalformedLine(t *testing.T) {
	conf := `
	[request_definition]
	r = sub, obj, act
	[policy_definition]
	p = sub, obj, act
	[policy_effect]
	e = some(where (p.eft == allow))
	[matchers]
	m = r.sub == p.sub && r.obj == p.obj && r.act == p.act
	`
	m := model.NewModel()
	if err := m.LoadModelFromText(conf); err != nil {
	t.Fatal(err)
	}
	// A well-formed line must still load against this model, so the failure
	// below can only come from the unterminated quote.
	if err := NewAdapter(`p, alice, data1, read`).LoadPolicy(m); err != nil {
	t.Fatalf("well-formed line should load, got %v", err)
	}
	err := NewAdapter(`p, alice, data1, "read`).LoadPolicy(m)
	if err == nil {
	t.Fatal("LoadPolicy() error = nil, want a parse error for the unterminated quoted field")
	}
	if !strings.Contains(err.Error(), "quoted-field") {
	t.Fatalf("want a csv quoting error, got %v", err)
	}
	}

strings is already imported in this file. I checked this passes on the branch and still fails against the pre-change loop, on the same assertion as before.

[hsluoyz]

Thanks for reporting this, @zjncs! The problem is real: LoadPolicy was silently dropping malformed lines. We fixed it directly in 524f3f2. That commit returns the parse error the same way you proposed, and also trims each line first, like the file adapter does. Without the trim, whitespace-only lines and indented # comments (as pointed out in the review above) would become hard load failures. Since this covers the same fix, I'll close this PR. Thanks again for the contribution!