Repository: octodns/octodns

Issue:
"Value should be enclosed in quotation marks" when specific record pattern
The octodns-sync --doit (to Route53) fails with the following config:
```yaml
---
'':
  ttl: 600 
  type: TXT 
  values:
    - v=DMARC1\; p=quarantine\; rua=mailto:xxxxxxxxx@xxxxxxxxxxxxxxxxxxx,mailto:xxxxxxxxxx@xxxxxxxxxxxxxxxxxxxxxxxxxx\; ruf=mailto:xxxxxxxx@xxxxxxxxxxxxxxxxxxx,mailto:xxxxxxxxxx@xxxxxxxxxxxxxxxxxxxxxxxxxx\; fo=1\; pct=100\; rf=afrf\;adkim=r\; aspf=r\; ri=43200\;
```
with `InvalidCharacterString (Value should be enclosed in quotation marks)` as:
```
...
2024-08-19T14:36:14  [4339582336] INFO  Plan 
********************************************************************************
* tomo-example.com.
********************************************************************************
* route53 (Route53Provider)
*   Update
*     <TxtRecord TXT 600, tomo-example.com., ['aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\;']> ->
*     <TxtRecord TXT 600, tomo-example.com., ['v=DMARC1\; p=quarantine\; rua=mailto:xxxxxxxxx@xxxxxxxxxxxxxxxxxxx,mailto:xxxxxxxxxx@xxxxxxxxxxxxxxxxxxxxxxxxxx\; ruf=mailto:xxxxxxxx@xxxxxxxxxxxxxxxxxxx,mailto:xxxxxxxxxx@xxxxxxxxxxxxxxxxxxxxxxxxxx\; fo=1\; pct=100\; rf=afrf\;adkim=r\; aspf=r\; ri=43200\;']> (config)
*   Summary: Creates=0, Updates=1, Deletes=0, Existing Records=1
********************************************************************************


2024-08-19T14:36:14  [4339582336] INFO  Route53Provider[route53] apply: making 1 changes to tomo-example.com.
2024-08-19T14:36:14  [4339582336] INFO  Route53Provider[route53] _apply: zone=tomo-example.com., len(changes)=1
2024-08-19T14:36:14  [4339582336] INFO  Route53Provider[route53] _apply:   sending change request for batch of 1 mods, 1 ResourceRecords
Traceback (most recent call last):
  File "/opt/homebrew/bin/octodns-sync", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/opt/homebrew/lib/python3.11/site-packages/octodns/cmds/sync.py", line 62, in main
    manager.sync(
  File "/opt/homebrew/lib/python3.11/site-packages/octodns/manager.py", line 857, in sync
    total_changes += target.apply(plan)
                     ^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/lib/python3.11/site-packages/octodns/provider/base.py", line 298, in apply
    self._apply(plan)
  File "/opt/homebrew/lib/python3.11/site-packages/octodns_route53/provider.py", line 1900, in _apply
    self._really_apply(batch, zone_id)
  File "/opt/homebrew/lib/python3.11/site-packages/octodns_route53/provider.py", line 1911, in _really_apply
    resp = self._conn.change_resource_record_sets(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/lib/python3.11/site-packages/botocore/client.py", line 565, in _api_call
    return self._make_api_call(operation_name, kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/lib/python3.11/site-packages/botocore/client.py", line 1021, in _make_api_call
    raise error_class(parsed_response, operation_name)
botocore.errorfactory.InvalidChangeBatch: An error occurred (InvalidChangeBatch) when calling the ChangeResourceRecordSets operation: [Invalid Resource Record: 'FATAL problem: InvalidCharacterString (Value should be enclosed in quotation marks) encountered with '"v=DMARC1\; p=quarantine\; rua=mailto:xxxxxxxxx@xxxxxxxxxxxxxxxxxxx,mailto:xxxxxxxxxx@xxxxxxxxxxxxxxxxxxxxxxxxxx\; ruf=mailto:xxxxxxxx@xxxxxxxxxxxxxxxxxxx,mailto:xxxxxxxxxx@xxxxxxxxxxxxxxxxxxxxxxxxxx\; fo=1\; pct=100\; rf=afrf\;adkim=r\; aspf=r\; ri=43200\" ";"'']
```

```sh
% octodns-sync --version
octoDNS 1.6.1
```

The cause might be related to how the value is split.
My hypothesis is:
1. The record value `v=DMARC1\; ... aspf=r\; ri=43200\;` has 256 characters.
2. It is split by each 255 characters into two chunks: `v=DMARC1\; ... aspf=r\; ri=43200\` and `;`.
3. The first chunk ends with backslash `\`, so the text delimiter `"` is treated as a double quote `\"` and broken. Or, the second chunk starts without a backslash `\`, so the semicolon`;` is not escaped correctly. 

The same error occurs with the following value:
```
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\; a\;
```
It has 256 characters (`a`x250 + `\; a\;`). So, I think it is split into two chunks (`aaa...aaa\` + `;`) where the issue is the first chunk ends with a backslash or the second chunk starts without a backslash.

A workaround is to adjust the text length. In my case, I have added a space in between the tokens from `... rf=afrf;adkim=r; ...` to `... rf=afrf; adkim=r; ...` and the sync succeeded.

I think the issue does not happen often, but thought this info is beneficial for others if they see the same issue.

Discussion:
@ross: If I'm understanding correctly I believe this is a bug splitting as you point out and not a problem with quoting.

Route53 uses chunked_value https://github.com/octodns/octodns-route53/blob/53342d3e53a78718e253ed87f6ec2c162f85e8fb/octodns_route53/provider.py#L301 so points to the problem being in octoDNS core's chunked value. I don't remember doing anything to handle escapes in it so it's very likely it does not.

> I think the issue does not happen often, but thought this info is beneficial for others if they see the same issue.

Definitely needs fixing. Thanks for the digging/debugging to track down the problem. Will stick this on the TODO list.

Solution patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 53bb57e..82a298d 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -3,6 +3,8 @@
 * MetaProcessor.include_extra to add support for arbitrary extra values to be
   set on the meta record.
 * Correctly handled quoted svcparams when parsing SVCB/HTTPS rdata text
+* Fix handling of chunked values (TXT, SPF) when escaped characters land at the
+  split boundaries, don't split escapes from their following chars
 
 ## v1.9.1 - 2024-06-21 - What's in a name
 
diff --git a/octodns/record/chunked.py b/octodns/record/chunked.py
index 3f088e3..a07acfd 100644
--- a/octodns/record/chunked.py
+++ b/octodns/record/chunked.py
@@ -13,10 +13,20 @@ class _ChunkedValuesMixin(ValuesMixin):
 
     def chunked_value(self, value):
         value = value.replace('"', '\\"')
-        vs = [
-            value[i : i + self.CHUNK_SIZE]
-            for i in range(0, len(value), self.CHUNK_SIZE)
-        ]
+        vs = []
+        i = 0
+        n = len(value)
+        # until we've processed the whole string
+        while i < n:
+            # start with a full chunk size
+            c = min(self.CHUNK_SIZE, n - i)
+            # make sure that we don't break on escape chars
+            while value[i + c - 1] == '\\':
+                c -= 1
+            # we have our chunk now
+            vs.append(value[i : i + c])
+            # and can step over if
+            i += c
         vs = '" "'.join(vs)
         return self._value_type(f'"{vs}"')
 


Test patch:
diff --git a/tests/test_octodns_record_chunked.py b/tests/test_octodns_record_chunked.py
index 30592cc..4d7bbc0 100644
--- a/tests/test_octodns_record_chunked.py
+++ b/tests/test_octodns_record_chunked.py
@@ -4,8 +4,9 @@
 
 from unittest import TestCase
 
-from octodns.record.chunked import _ChunkedValue
+from octodns.record.chunked import _ChunkedValue, _ChunkedValuesMixin
 from octodns.record.spf import SpfRecord
+from octodns.record.txt import TxtValue
 from octodns.zone import Zone
 
 
@@ -67,3 +68,50 @@ class TestChunkedValue(TestCase):
             ['non ASCII character in "Déjà vu"'],
             _ChunkedValue.validate('Déjà vu', 'TXT'),
         )
+
+    zone = Zone('unit.tests.', [])
+
+    # some hacks to let us work with smaller sizes
+    class Base:
+        def __init__(self, *args, **kwargs):
+            pass
+
+    class SmallerChunkedMixin(_ChunkedValuesMixin, Base):
+        CHUNK_SIZE = 8
+        _type = 'TXT'
+        _value_type = TxtValue
+
+        def __init__(self, values):
+            super().__init__(None, None, {'values': values})
+
+    def test_splitting(self):
+
+        for value, expected in (
+            # shorter
+            ('0123', '"0123"'),
+            # exact
+            ('01234567', '"01234567"'),
+            # simple
+            ('0123456789', '"01234567" "89"'),
+            # 1 extra
+            ('012345678', '"01234567" "8"'),
+            # escape in the middle
+            ('01234\\;56789', '"01234\\;5" "6789"'),
+            # escape before the boundary
+            ('012345\\;6789', '"012345\\;" "6789"'),
+            # escape after the boundary
+            ('01234567\\;89', '"01234567" "\\;89"'),
+            # escape spanning the boundary
+            ('0123456\\;789', '"0123456" "\\;789"'),
+            # multiple escapes at the boundary
+            ('012345\\\\;6789', '"012345" "\\\\;6789"'),
+            # exact size escape
+            ('012345\\;', '"012345\\;"'),
+            # spanning ending
+            ('0123456\\;', '"0123456" "\\;"'),
+        ):
+            sc = self.SmallerChunkedMixin(value)
+            self.assertEqual([expected], sc.chunked_values)
+
+        sc = self.SmallerChunkedMixin(['0123456789'])
+        self.assertEqual(['"01234567" "89"'], sc.chunked_values)