Skip to content

fix: exact version match in accept header validation#1753

Closed
mmagician wants to merge 1 commit intorelease/v0.14.0-alphafrom
mmagician-claude/fix-accept-header-prerelease
Closed

fix: exact version match in accept header validation#1753
mmagician wants to merge 1 commit intorelease/v0.14.0-alphafrom
mmagician-claude/fix-accept-header-prerelease

Conversation

@mmagician
Copy link
Contributor

@mmagician mmagician commented Mar 5, 2026

Summary

The AcceptHeaderLayer::new constructs a semver Comparator with patch: None and pre: Prerelease::default() (empty), effectively creating a version requirement like =0.14. While the patch: None correctly allows any patch version through (so 0.14.0 and 0.14.1 are both accepted), the empty pre-release causes semver's VersionReq::matches to reject all pre-release versions. Per semver matching rules, a comparator without a pre-release component never matches versions that have one.

This means during the alpha release cycle, no client can pass version negotiation - a client at 0.14.0-alpha.3 is rejected even by its own node at 0.14.0-alpha.3.

Fix

The VersionReq alone can't express "match major.minor with any patch, but require exact pre-release" because of how semver treats pre-release comparisons. Instead, we:

  1. Keep the VersionReq as-is for major.minor matching (with patch: None, pre: empty)
  2. Store the expected pre-release tag separately in expected_pre
  3. In negotiate, strip the pre-release from the client version before checking VersionReq, then check the pre-release tag separately for an exact match

This gives us the desired compatibility rules:

  • Patch versions are compatible: 0.14.0 and 0.14.1 are both accepted by a 0.14.0 node
  • Pre-release tags must match exactly: alpha.3 only matches alpha.3, not alpha.1 or beta.3
  • Stable and pre-release are not compatible: a 0.14.0 client is rejected by a 0.14.0-alpha.3 node and vice versa

Test changes

  • Added version_prerelease_rejected_by_stable case (a stable server rejects pre-release clients)
  • Added a prerelease test module that creates a server at 0.14.0-alpha.3 and verifies:
    • Exact pre-release match passes
    • Different patch with same pre-release tag passes (0.14.1-alpha.3)
    • Different pre-release number (alpha.1) is rejected
    • Different pre-release tag (beta.3) is rejected
    • Stable version (0.14.0) is rejected

Context

Discovered while running integration tests for the miden-client release/v0.14.0-alpha branch. The client (at 0.14.0-alpha.1) was rejected by the test node (built from miden-node-rpc at 0.14.0-alpha.3) with:

accept header validation failed: server rejected request
(client version: 0.14.0-alpha.1, genesis commitment: none)

See: https://github.com/0xMiden/miden-client/actions/runs/22726117886/job/65902524781?pr=1862

The AcceptHeaderLayer was constructing a semver comparator with patch: None
and pre: Prerelease::default() (empty), which created a requirement like =0.14.
This had two problems:

1. It allowed any patch version (e.g. a client at 0.14.1 could connect to a 0.14.0 node)
2. It rejected ALL pre-release versions, since semver matching rules exclude
   pre-releases when the comparator has no pre-release component

This meant that during the alpha release cycle, no client could ever pass version
negotiation - a client at 0.14.0-alpha.1 was rejected by a node at 0.14.0-alpha.3,
and even a client at 0.14.0-alpha.3 was rejected by its own node.

The fix includes the full version (patch + pre-release) in the comparator, so the
node requires an exact version match. For example, a node at 0.14.0-alpha.3
now accepts only clients at 0.14.0-alpha.3.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@mmagician mmagician force-pushed the mmagician-claude/fix-accept-header-prerelease branch from afb79eb to 11a09a6 Compare March 5, 2026 17:32
Copy link
Collaborator

@igamigo igamigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I guess another potential fix could've been to strip the alpha suffix from the client's side, but this sounds like it's more correct even if it incurs another release

@sergerad
Copy link
Collaborator

sergerad commented Mar 5, 2026

Makes sense I'm just not certain we want this behaviour:

Pre-release tags must match exactly: alpha.3 only matches alpha.3, not alpha.1 or beta.3

Might be more helpful to require exact match on alpha/beta and ignore the number?

@bobbinth
Copy link
Contributor

bobbinth commented Mar 5, 2026

Might be more helpful to require exact match on alpha/beta and ignore the number?

I think this may be preferable as well. Basically, we'd have 0.14.0-alpha.3 node accept 0.14.0-alpha.x client (0.14.0-alpha.1, 0.14.0-alpha.2 etc.). This would allow us to evolve client and node codebases somewhat independently (e.g., a bugfix in the node does not need to trigger re-deployment of the client and vice-versa). And if we did want to make them incompatible - we could use beta, gamma etc.

@Mirko-von-Leipzig
Copy link
Collaborator

I think this indicates that its time to version the gRPC schema separately since its largely more stable than the number of versions we'll be releasing elsewhere.

The main thing preventing this is the protocol types which we raw encode. Since we effectively inherit its stability as well. But we could also pin the version used in the grpc crate(s) to get some stability guarantees.


Given that ^^ should be our target (~soonish), I think we can just do something lenient for the alpha stuff so that it gets out of they way. Basically what @bobbinth and @sergerad suggest -- match on the alpha but ignore the number suffix.

@mmagician
Copy link
Contributor Author

I think this may be preferable as well. Basically, we'd have 0.14.0-alpha.3 node accept 0.14.0-alpha.x client (0.14.0-alpha.1, 0.14.0-alpha.2 etc.). This would allow us to evolve client and node codebases somewhat independently

We discussed both options in the sync meeting and the current approach (of exact match on both alpha AND x) was what we all agreed on to implement to unblock us with alpha - unless I misunderstood / missed something?

In any case, the other approach is relatively simple as well, I opened another PR targeting the same branch to compare: #1755

@mmagician mmagician added the rpc Related to the RPC component label Mar 6, 2026
@igamigo
Copy link
Collaborator

igamigo commented Mar 6, 2026

We can close this after #1755 got merged, right?

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

Labels

rpc Related to the RPC component

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants