Repository navigation
fix(runtime): seven self-contained audit findings - #181
Merged
Merged
Conversation
`resource_instances.kind` was written with `format!("{:?}")` and read by a
hand-written match missing the `ExternalService` arm. The reconciler
persists those rows, so `find_instance` on one returned a
`FromSqlConversionFailure` instead of the instance — latent today because
only tests call it, but a persisted row the reader cannot parse is a trap
waiting for the first production caller.
Writing now goes through an exhaustive `resource_kind_str`, so a new
variant stops compiling rather than silently becoming unreadable, and a
round-trip test covers every variant — including that the spelling still
matches Debug, since the column already holds rows written that way.
…failure `l[rt.started.state-methods]` says the deadline argument must be a positive integer and that zero or absent uses the state's default; `l[rt.stop]` defers to the same rule. All five call sites went through `d.max(0) as u64`, so zero became `Some(0)` — which `elapsed >= d` trips on the very next pass, the exact opposite of the documented fallback. A script writing `ready(0)` for the default 30s got "Barrier deadline of 0s exceeded" about two seconds later. Negatives now throw rather than clamping to zero: the spec calls for a positive integer, and the defs layer's rule is to refuse malformed input rather than coerce it into meaning something else. Two existing tests used a zero deadline as a way to force immediate expiry, so both asserted the behaviour this changes. `barrier_deadline_zero_expires_on_second_pass` was testing the bug itself and now asserts the default is taken; `rt_stop_deadline_is_enforced` still tests what it meant to, using the smallest real deadline and letting it elapse.
`l[volume.write.validation]` forbids a path that escapes the volume root *after canonicalisation*. The implementation rejected any path containing a `..` component at all, so `/etc/../etc/conf` — which resolves safely inside the volume — was refused. Resolution is textual, which is right for this check: it runs at definition time, where the volume does not exist yet and a symlink inside it could not be followed anyway. The spec contradicted itself. `l[rt.write]` paraphrased the rule as "must not contain `..` components" while claiming to restate `l[volume.write.validation]`, which is where the stricter reading came from; the paraphrase now matches the rule it cites. `rt_write_rejects_path_traversal` asserted the old message mentioned `'..'`. The path it uses still escapes and is still refused, so the test keeps its meaning and checks the new wording.
`challenge_record_name("*.example.com")` produced
`_acme-challenge.*.example.com`. RFC 8555 places a wildcard's DNS-01
challenge at the base domain — `_acme-challenge.example.com` — and no zone
will hold a name with a `*` label in the middle, so the provider call
failed and wildcard issuance could never complete.
Nothing rejects a wildcard hostname on the way in, and `build_csr`
deliberately preserves wildcard SANs, so this path is reachable by design
rather than by misuse. Only a leading `*.` is stripped; a `*` anywhere
else is not a wildcard.
`r[image.pin.expiry]` says expirations "are cleared whenever a pin's reference is observed to be valid again for the owning app". An explicit `rt.warm_images` is exactly that observation, but `upsert_pin` used `ON CONFLICT DO NOTHING`, so an `expires_at` stamped earlier by the post-update reconciliation rule stayed put — and the re-warmed image was swept on a later tick as though the app had stopped referring to it. `DO UPDATE` names only `expires_at`. `pinned_at` records when the pin was first taken and belongs to the insert; naming no more than this writer owns is what the repo's rule about unscoped row overwrites asks for.
An endpoint with only AAAA records on a host without IPv6 egress reported `NeedsNat64ButDisabled`, the only `UnroutableReason` there was. The reconciler then told the operator the endpoint "require[s] NAT64 but NAT64 is not active" — even when NAT64 was active, and it could not have helped either way: NAT64 translates v6 to v4, and what is missing here is v6 egress. `NoIpv6Egress` carries that case, the reason travels with the host into the fault set, and the two get sentences naming the remedy each actually needs. `dns_aaaa_only_without_v6_egress_is_unroutable` asserted the old reason. The endpoint is still unroutable, which is what the test was for, so it keeps its meaning and now names the right cause.
… as absent `apply_schema` built its submitted map with `v.as_str()` inside a `filter_map`, so a schema-declared param supplied as a JSON number or bool was skipped as though it had not been sent. If the field had a default, the caller's value was silently replaced by it; if it was required with no default, the caller was told a required field was missing — when it was present and merely the wrong type. Both answers name the wrong problem, which is the point: a value that is present but unusable is not an absence. It is an error now, and the rejected call leaves `params` untouched.
Contributor
Code Coverage OverviewLanguages: TypeScript, Rust TypeScript / code-coverage/vitestThe overall line coverage in commit fecc7c0 in the Rust / code-coverage/rustThe overall line coverage in commit fecc7c0 in the Show a line coverage summary of the most impacted files.
Updated |
Merged
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
1 participant
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.
🤖 Seven trivial findings from the logic-bug audit in the runtime and defs layers, one commit each.
ExternalServicerows could not be read back.resource_instances.kindwas written withformat!("{:?}")and parsed by a hand-written match missing that arm, sofind_instanceon a row the reconciler writes returned a conversion failure. Writing now goes through an exhaustiveresource_kind_str, so a new variant stops compiling instead of silently becoming unreadable, and a round-trip test covers every variant — including that the spelling still matches Debug, since the column already holds rows written that way.A zero deadline meant "fail immediately" instead of the default.
l[rt.started.state-methods]says zero or absent uses the state's default andl[rt.stop]defers to it, but all five sites went throughd.max(0) as u64, soready(0)failed about two seconds in with "Barrier deadline of 0s exceeded". Negatives now throw rather than clamping to zero.volume.writerefused..paths that stay inside the root. The rule is "must not escape after canonicalisation"; the code rejected the component outright, so/etc/../etc/confwas refused. The spec contradicted itself here —l[rt.write]paraphrased the rule as "must not contain..components" while claiming to restatel[volume.write.validation], which is where the stricter reading came from — so the paraphrase now matches the rule it cites.Wildcard ACME issuance could never complete.
challenge_record_name("*.example.com")produced_acme-challenge.*.example.com; RFC 8555 places a wildcard's challenge at the base domain, and no zone holds a name with a*label in the middle. Nothing rejects a wildcard on the way in andbuild_csrdeliberately preserves wildcard SANs, so this was reachable by design.Re-warming an image left a pending pin expiry in place.
r[image.pin.expiry]clears an expiration once the reference is observed valid again, butupsert_pinusedON CONFLICT DO NOTHING, so a re-warmed image was still swept later.DO UPDATEnames onlyexpires_at—pinned_atbelongs to the insert, and naming no more than this writer owns is what the unscoped-overwrite rule asks for.NAT64 was blamed for a missing IPv6 egress. An AAAA-only endpoint on a host without v6 egress reported the only reason there was, so the operator was told NAT64 was inactive even when it was on — and it could not have helped, since it translates v6 to v4.
NoIpv6Egresscarries that case and the reason travels with the host into the fault set.A non-string schema param was treated as absent.
apply_schemafiltered onv.as_str(), so a submitted JSON number was silently replaced by the field's default, or reported as a missing required field. Both name the wrong problem: a value that is present but unusable is not an absence.Three existing tests asserted behaviour these changes correct, and all three keep their original intent:
barrier_deadline_zero_expires_on_second_passwas testing the bug itself, and now asserts the default is taken.rt_stop_deadline_is_enforcedused a zero deadline as a way to force expiry; it uses the smallest real deadline and lets it elapse.dns_aaaa_only_without_v6_egress_is_unroutableasserted the wrong reason; the endpoint is still unroutable and the test now names the right cause.Two findings from this subsystem are not here, both on cards rather than forced into the sweep.
/forwards/startport validation needs a pod traversal that does not exist yet (V4). Instance GC skipping the active desired state needs keep-set membership the GC cannot see — it is computed at reconcile time and not persisted (X4).