Skip to content

[BUG] Core: move record_index to OutboundRoute from config #264

Description

@solus161

Context

PR #260 introduces a breaking change involving ConnectorConfig: the config now holds a new attr record_index. The purpose of this design is for record_index could be carried from collect_outbound_routes to WsBusSink.publish() then ClientManager.broadcast() could gate message base on clients' permission bitmap.

However, this design proves unoptimal due to several reasons:

  • The extra record_index key in OutboundRoute.config could create a bug where the outbound route is constructed with record_index whose value divert from the record registration order. When record_index reaches OutboundRoute.config as encoded String, there are two record_key in that config and when ConnectorConfig::from_query(&config) a record_key branch needed to construct back record_key into ConnectorConfig. This make record_key a reserved keyword in from_query and we want to avoid that. The current flow has no error simply because the correct record_key lands as last item in config, and that's not strict enough;
  • The index reaches broadcasting by taking a detour from usize -> String (carried in OutboundRoute.config) -> usize when reaching broadcasting. This could be avoided;

Proposal

  • Add pub record_index to OutboundRoute. ConnectorConfig keep record_index;
  • Change the assignment of record_index: in pump_sink, after ConnectorConfig::from_query(&config), set ConnectorConfig.record_index to Some(record_index). This way, users could not override assigned record indexes by passing in a record_index param when sending request. This is the only way to fix the security leak and it is better than changing the Connector::publish signature as 1) that change will propagate to at least six call sites; 2) not all call sites have an use for such index;
  • Remove record_index branch from ConnectorConfig::from_query so the reserved keyword issue solved;
  • Add #[non_exhaustive] to ConnectorConfig, since it's already a breaking change;
  • Connector::publish stays the same;
  • Rewrite collect_outbound_routes_preserves_record_order to check list_records()[route.record_index].record_key == key instead of parsing the config pair;

Tests

  • record_index that is wrongly passed when building a outbound route will be overridden by the correct id, clients having valid grant could still receive message;

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

    Labels

    bugSomething isn't working🏗️ coreCore engine work

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions