NUAAwqy opened a new pull request, #1769:
URL: https://github.com/apache/casbin/pull/1769
## What
`persist.PolicyLineToCsv` can write a policy file that no longer loads.
Its docstring says quoted fields "round-trip through `LoadPolicyLine`
without corruption". That is
true for commas, quotes and leading spaces. It is not true for a field
containing a line break,
which a `sub`/`obj`/`act` value taken from user input, a claim, or a
multi-line condition can
carry. CSV has no escape for a newline in this format: a policy file holds
one rule per line, and
the loaders (`persist/file-adapter`) feed `LoadPolicyLine` one
`bufio.Scanner` line at a time, so
the closing quote is never seen.
```go
_, _ = e.AddPolicy("bob", "first\nsecond", "write")
_ = e.SavePolicy() // returns nil
// reopen the same model + policy file
```
Before - the saved file is:
```
p, alice, data1, read
p, bob, "first
second", write
```
and reopening it fails outright:
```
parse error on line 1, column 15: extraneous or missing " in quoted-field
```
Because `SavePolicy` rewrites the whole file in one shot, one bad field
costs every rule that was
already in it.
After - the save is refused before anything is written:
```
invalid policy field "first\nsecond": a policy file holds one rule per line,
so a field cannot contain a line break
```
Both `file-adapter` and `string-adapter` already propagate the error from
this function, and the
file adapter builds the full text before touching storage, so the existing
policy file is left
untouched and still loadable.
## Fix
Reject a field containing `\n` while serializing, rather than emitting an
unreadable line. Same
shape as `9e2e997` for this file: report a clear error instead of letting
the CSV machinery fail
later. A lone carriage return is deliberately still accepted - it does not
end a physical line and
the rule round-trips unchanged, which the test pins.
## Test
`persist/persist_test.go`, next to the existing `TestPolicyLineToCsv` table:
- `TestPolicyLineToCsvLineBreaks`: `\n` and `\r\n` fields, lone `\r` and an
ordinary quoted field,
so the check is that a line break is refused and everything else is not.
- `TestSavePolicyWithLineBreakRule`: end to end in a temp dir - save must
either error with the
file left intact and loadable, or produce a file that reloads with both
rules present. On
`master` it fails on the second branch; here it takes the first.
- `go test -race ./...`: 8 packages ok
- `gofmt -l .`: empty
- `golangci-lint run` (v2.15.0, as `.golangci.yml` requires): 0 issues
## Notes
- Adapters that do not serialize through `PolicyLineToCsv` (databases,
Redis, ...) are unaffected.
- I chose refusing over escaping. Escaping the break as `\n` text would need
a matching unescape in
`LoadPolicyLine`, and without it the reloaded value differs from the
stored one, so policies stop
matching. Happy to switch if maintainers prefer that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
