Repository navigation
Conversation
PolicyLineToCsv quoted a field holding a line break the same way it quotes a field holding a comma, and the docstring promised the line round-trips through LoadPolicyLine. It cannot: a policy file holds one rule per line and every loader hands LoadPolicyLine a single line at a time, so the closing quote of such a field is never seen. SavePolicy then rewrote the whole policy file with a line Go's CSV reader rejects: parse error on line 1, column 15: extraneous or missing " in quoted-field The rules that were already in the file are lost with it, because the damaged file replaces the previous one. Return an error for such a field instead. Both file-adapter and string-adapter build the full text before touching storage and propagate the error, so the existing policy file is left intact and still loadable. A lone carriage return stays accepted: it does not end the physical line and the rule round-trips unchanged.
Member
|
Thanks for the report and the clear reproduction. This has been fixed directly in aa1ef64: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
persist.PolicyLineToCsvcan write a policy file that no longer loads.Its docstring says quoted fields "round-trip through
LoadPolicyLinewithout corruption". That istrue for commas, quotes and leading spaces. It is not true for a field containing a line break,
which a
sub/obj/actvalue taken from user input, a claim, or a multi-line condition cancarry. CSV has no escape for a newline in this format: a policy file holds one rule per line, and
the loaders (
persist/file-adapter) feedLoadPolicyLineonebufio.Scannerline at a time, sothe closing quote is never seen.
Before - the saved file is:
and reopening it fails outright:
Because
SavePolicyrewrites the whole file in one shot, one bad field costs every rule that wasalready in it.
After - the save is refused before anything is written:
Both
file-adapterandstring-adapteralready propagate the error from this function, and thefile adapter builds the full text before touching storage, so the existing policy file is left
untouched and still loadable.
Fix
Reject a field containing
\nwhile serializing, rather than emitting an unreadable line. Sameshape as
9e2e997for this file: report a clear error instead of letting the CSV machinery faillater. 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 existingTestPolicyLineToCsvtable:TestPolicyLineToCsvLineBreaks:\nand\r\nfields, lone\rand 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 thefile left intact and loadable, or produce a file that reloads with both rules present. On
masterit fails on the second branch; here it takes the first.go test -race ./...: 8 packages okgofmt -l .: emptygolangci-lint run(v2.15.0, as.golangci.ymlrequires): 0 issuesNotes
PolicyLineToCsv(databases, Redis, ...) are unaffected.\ntext would need a matching unescape inLoadPolicyLine, and without it the reloaded value differs from the stored one, so policies stopmatching. Happy to switch if maintainers prefer that.