Skip to content

Four broken fixtures in uts/rest/unit — two make their tests unsatisfiable, two encode superseded specification text #528

Description

@owenpearson

Summary

Four fixtures in uts/rest/unit carry values that cannot do what the surrounding test says they do. Two of them (RSP5/decode-cipher-channel-7, RSL6b/unrecognized-encoding-preserved-0) make their tests unsatisfiable for any conforming SDK: the data in the fixture is truncated in one case and not decodable in the other. The other two (stats.md, types/options_types.md) are stale rather than unsatisfiable — they encode shapes that features.md has since replaced, and each also documents a small coverage or hygiene gap worth closing.

Line references are against d9a04ca; paths are relative to uts/.


1. presence/rest_presence.md:1225 — the cipher fixture is a truncated copy of a real interop fixture

rest/unit/RSP5/decode-cipher-channel-7 sets up:

cipher_key = base64_decode("WUP6u0K7MXI5Zeo0VppPwg==")

# Encrypted data for {"secret":"data"}
encrypted_data = "HO4cYSP8LybPYBPZPHQOtuD53yrD3YV3NBoTEYBh4U0="

encrypted_data decodes to 32 bytes: a 16-byte IV plus exactly one AES-CBC block. The stated plaintext {"secret":"data"} is 17 bytes, which PKCS#7-pads to two blocks. So the fixture is one block short of being able to hold the value its comment claims, whatever the key.

Decrypting it with the fixture's own key shows what is actually in that block:

key = base64.b64decode("WUP6u0K7MXI5Zeo0VppPwg==")   # 16 bytes
buf = base64.b64decode("HO4cYSP8LybPYBPZPHQOtuD53yrD3YV3NBoTEYBh4U0=")   # 32 bytes
AES.new(key, AES.MODE_CBC, buf[:16]).decrypt(buf[16:])
  -> b'{"example":{"jso'

The final byte is 0x6f (111, o), not a PKCS#7 pad length in 1–16, so unpadding fails as well. The block is a truncated prefix of a longer plaintext, and not the one in the comment.

The original. This is the first 43 characters of an existing interop fixture in ably-common, with an = appended. test-resources/crypto-data-128.json (at 18c920d) declares, at the top of the file:

"key": "WUP6u0K7MXI5Zeo0VppPwg==",
"iv": "HO4cYSP8LybPYBPZPHQOtg==",

— the same key the UTS fixture uses, and an IV that is byte-for-byte the first 16 bytes of the UTS ciphertext. Line 41 of that file is:

        "data": "HO4cYSP8LybPYBPZPHQOtuD53yrD3YV3NBoTEYBh4U0N1QXHbtkfsDfTspKeLQFt",

with "encoding": "json/utf-8/cipher+aes-128-cbc/base64" — the same encoding string the UTS mock returns — and an encoded sibling whose data is {"example":{"json":"Object"}}.

Checks against the submodule:

Check Result
real[:43] + "=" equals the UTS fixture true
UTS fixture decodes to 32 bytes; real decodes to 48 true
UTS fixture bytes are a byte-prefix of the real ciphertext true
crypto-data-128.json iv equals the first 16 bytes of both true
Real value decrypts and unpads cleanly {"example":{"json":"Object"}}, pad byte 3

So the fixture's comment, # Encrypted data for {"secret":"data"}, is wrong twice over: wrong plaintext and a ciphertext truncated to one block of it.

Why it was not caught. The assertions are:

ASSERT result.items[0].data IS Object/Map
# Decryption applied based on cipher+aes-128-cbc encoding

A type-only assertion. Even given a correct fixture this test would not distinguish {"secret":"data"} from anything else that happens to deserialize to a map — which is precisely why the truncation survived.

Proposed fix. Adopt the real interop fixture (HO4cYSP8LybPYBPZPHQOtuD53yrD3YV3NBoTEYBh4U0N1QXHbtkfsDfTspKeLQFt), correct the comment to {"example":{"json":"Object"}}, and assert the decrypted value:

ASSERT result.items[0].data == { "example": { "json": "Object" } }

Sourcing the fixture from crypto-data-128.json also keeps the unit spec in step with the RSL6a1 interop fixtures rather than carrying an independent copy.


2. encoding/message_encoding.md:405 — invalid base64, and the assertions are the negation of RSL6b

rest/unit/RSL6b/unrecognized-encoding-preserved-0 (:388) returns:

"data": "encrypted-data-here",
"encoding": "custom-encryption/base64",

and asserts:

# base64 should be decoded, but custom-encryption is unrecognized
ASSERT message.encoding == "custom-encryption"
# Data should be base64-decoded but not further processed
ASSERT message.data IS bytes  # Result of base64 decode

"encrypted-data-here" is not valid base64. protocol.md:256 fixes the alphabet — "The base64 encoding used is RFC4648" — and the two - characters are not in it. Discarding them leaves 17 data characters, and 17 ≡ 1 (mod 4), which is the one residue class no valid base64 string can occupy: a 4-character group encodes 3 bytes and the only permitted partial groups are 2 and 3 characters. It is therefore not repairable by adding padding either; there is no base64 string of any length whose alphabet characters number 1 more than a multiple of 4. The base64 decode the test asserts succeeds cannot succeed.

It also inverts its own spec point. features.md:347:

(RSL6b) If, for example, incompatible encryption details are provided or invalid Base64 is detected in the message payload, an error message will be sent to the logger, but the message will still be delivered with last successful decoding and the encoding field. For example, if a message had a decoding of "utf-8/cipher+aes-128-cbc/base64", and the payload was successfully Base64 decoded but the payload could not be decrypted because the CipherParam details were not configured, the message would be delivered with a binary payload and an encoding with the value "utf-8/cipher+aes-128-cbc".

"Invalid Base64 is detected in the message payload" is named by RSL6b as the failure branch. On that branch the correct outcome is data == "encrypted-data-here" (unchanged) and encoding == "custom-encryption/base64" (the full residual encoding, base64 included), plus a log entry. Both of the test's assertions are the negation of that: it demands the decode succeed and the base64 component be consumed.

Why it survived. The behaviour is decoder-leniency-dependent. Node's Buffer.from(s, 'base64') silently discards the out-of-alphabet characters and the orphan 17th character, returning 14 bytes of garbage rather than throwing:

> const b = Buffer.from('encrypted-data-here', 'base64')
> b.length                      // 14
> b.toString('base64')          // 'encrypted+data+herc='   (does not round-trip)
> Buffer.isBuffer(b)            // true

A literal derivation in ably-js would therefore satisfy both assertions vacuously: Buffer.isBuffer(...) is true of 14 garbage bytes, and encoding would be 'custom-encryption' because the decode "succeeded". In practice ably-js did not derive it literally — test/uts/rest/unit/encoding/message_encoding.test.ts silently substitutes a valid payload (Buffer.from('encrypted-data').toString('base64')) and keeps the spec's assertions. Either way the fixture never had to be correct for that suite to go green, which is the point.

It did find a real bug, once investigated. In ably-python, this exact input caused binascii.Error to escape Message.from_encoded rather than being logged and the message delivered with its residual encoding — a genuine RSL6b non-compliance, being fixed separately in that SDK. A broken fixture surfaced a real defect, but only because it was investigated rather than trusted.

Proposed fix — two tests, not one. The current section conflates the success and failure branches of RSL6b:

  1. Keep this test as the unrecognized-encoding test and give it a valid payload: ZW5jcnlwdGVkLWRhdGEtaGVyZQ== (base64 of encrypted-data-here). The existing assertions then hold, and message.data == bytes("encrypted-data-here") can be asserted rather than just its type.
  2. Add a separate test for the RSL6b failure branch, with a deliberately invalid payload, asserting delivery with the last successful decoding — data unchanged and encoding == "custom-encryption/base64". That branch currently has no coverage anywhere in uts/rest/unit.

3. stats.md:29-46 — the fixture predates specification version 2.1

The RSC6a/returns-paginated-stats-0 fixture is:

stats_data = [
  {
    "intervalId": "2024-01-01:00:00",
    "unit": "hour",
    "all": {
      "messages": {"count": 100, "data": 5000},
      "all": {"count": 100, "data": 5000}
    }
  },
  ...
]

features.md:1697-1708 deletes that whole family of clauses:

  • (TS12d) This clause has been deleted. It was valid up to and including specification version 2.1.
  • (TS12e) This clause has been deleted. It was valid up to and including specification version 2.1.

(and identically for TS12f through TS12o)

and features.md:1694 replaces them with a single flat map:

  • (TS12r) entries (property present in the JSON) - a Dict<String, int> containing statistics entries

The IDL agrees — features.md:2755-2762 has no all and no nested counters:

    class Stats: // TS12
      intervalId: String // TS12a
      intervalTime: Time // TS12p (calculated client-side)
      unit: Stats.IntervalGranularity // TS12c
      inProgress: String? // TS12q
      entries: Dict<String, Int> // TS12r
      schema: String // TS12s
      appId: String // TS12t

This is a fixture problem, not an SDK problem. ably-python's ably/types/stats.py:18-31 reads exactly entries, unit, intervalId, inProgress, appId and schema, and derives interval_time from intervalId per TS12p. It is a correct TS12r implementation; it simply ignores the fixture's all key.

Narrowing the claim: the test is not unsatisfiable. Nothing asserts all — the assertions (stats.md:65-80) only touch intervalId, unit, items.length, and the request method and path — so a conforming post-2.1 SDK passes it, as both ably-python and ably-js do (the latter having copied the stale fixture verbatim). The two real costs are:

  • The fixture is a worked example of a shape the specification deleted, in the file a reader consults for what a Stats response looks like.
  • entries appears nowhere in stats.md. TS12r — the only clause that now describes statistics data — has no coverage in the unit suite at all.

Proposed fix. Replace the all blocks with entries maps, e.g. "entries": {"messages.all.all.count": 100, "messages.all.all.data": 5000}, and add assertions on result.items[0].entries so TS12r is actually exercised. intervalTime, inProgress, appId and schema are likewise untested if coverage is being extended anyway.


4. types/options_types.md:254-256 — pre-REC1 hostnames, which the fix commit passed over

The TO/endpoint-affects-host-0 test case table reads:

| ID | Endpoint | Expected Rest Host |
|----|----------|--------------------|
| 1 | (none/production) | `rest.ably.io` |
| 2 | `"test"` | `test-rest.ably.io` |
| 3 | `"custom-env"` | `custom-env-rest.ably.io` |

Against features.md:38 (REC1a):

  • (REC1a) The primary domain is main.realtime.ably.net unless overridden by specifying an endpoint option or, optionally, any of the deprecated options environment, restHost, realtimeHost.

and features.md:43 (REC1b4):

  • (REC1b4) Otherwise, the endpoint option is a production routing policy ID of the form [id], and the primary domain is [id].realtime.ably.net.

So the three rows should be main.realtime.ably.net, test.realtime.ably.net and custom-env.realtime.ably.net.

The detail worth recording: commit 551080a "Fix endpoint values in UTS test specs" did touch this exact table, but changed only the routing-policy id and left the legacy domain shape in place:

@@ -252,7 +252,7 @@ Tests that endpoint option affects default hosts.
 | ID | Endpoint | Expected Rest Host |
 |----|----------|--------------------|
 | 1 | (none/production) | `rest.ably.io` |
-| 2 | `"sandbox"` | `sandbox-rest.ably.io` |
+| 2 | `"test"` | `test-rest.ably.io` |
 | 3 | `"custom-env"` | `custom-env-rest.ably.io` |

That commit's own message says "Update host assertions in unit tests to match (test.realtime.ably.net etc.)", and it did so in request_endpoint.md in the same commit:

-ASSERT mock_http.captured_requests[0].url.host == "sandbox.realtime.ably.net"
+ASSERT mock_http.captured_requests[0].url.host == "test.realtime.ably.net"

The options_types.md table was missed because it is a prose table rather than an ASSERT.

Mitigating, and it should be said: the "Expected Rest Host" column is never asserted by any test. The only assertion in the section is options_types.md:272:

ASSERT options.endpoint == test_case.endpoint

and the section's own Note says so ("The actual host resolution may be tested at the HTTP client level. This test verifies the option is stored correctly."). No SDK's derived test reads the column. So today this is cosmetic — but it is the table a reader copies from when writing a host-resolution test, and it currently teaches the pre-REC1 shape.

Proposed fix. Update the three rows to the REC1a/REC1b4 forms. Separately worth considering: either drop the column, since it is asserted nowhere, or move the section to one that does assert resolved hosts (request_endpoint.md already has that machinery).


Cross-SDK comparison

Derived-test status for the four fixtures. ably-cocoa has derived only TimeTests.swift from rest/unit, and ably-pubsub-java has derived no REST unit specs, so neither has an opinion on any of these.

Fixture ably-python ably-pubsub-js
RSP5/decode-cipher-channel-7 derived; re-encrypted the fixture for the comment's stated plaintext and asserts the decrypted value it.skip — "TODO: Implement when cipher infrastructure is available"
RSL6b/unrecognized-encoding-preserved-0 derived; substituted ZW5jcnlwdGVkLWRhdGEtaGVyZQ== derived; substituted Buffer.from('encrypted-data').toString('base64')
RSC6a/returns-paginated-stats-0 derived; stale fixture copied verbatim, passes (all unread) derived; stale fixture copied verbatim, passes
TO/endpoint-affects-host-0 derived; host column unused derived; host column unused

The pattern across the first two rows is that both SDKs that attempted them independently concluded the fixture was unusable and silently repaired it in their own derived test, without the specification being changed.

Proposed fix (summary)

# File Change
1 presence/rest_presence.md:1224-1225 Adopt the full crypto-data-128.json line-41 ciphertext, correct the comment to {"example":{"json":"Object"}}, assert the decrypted value rather than its type
2 encoding/message_encoding.md:405 Use ZW5jcnlwdGVkLWRhdGEtaGVyZQ== here; add a separate RSL6b failure-branch test with a deliberately invalid payload asserting delivery with residual encoding
3 stats.md:29-46 Replace the all blocks with TS12r entries maps and assert entries
4 types/options_types.md:254-256 main.realtime.ably.net / test.realtime.ably.net / custom-env.realtime.ably.net

Items 1 and 2 are the ones that block a conforming SDK today. Items 3 and 4 are mechanical.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions