Skip to content

fix(registry): keep new server ids distinct under sanitize_segment (SBS-880) - #859

Merged
btsouth merged 2 commits into
mainfrom
fix/sanitize-injective-ids
Sep 4, 2026
Merged

btsouth merged 2 commits into
mainfrom
fix/sanitize-injective-ids

Conversation

@btsouth

@btsouth btsouth commented Sep 3, 2026

Copy link
Copy Markdown
Owner

What and why

sanitize_segment maps every character outside [A-Za-z0-9_] to _. That is correct for exposed tool names and wrong as an identity, but the client scope set, PII origin, injection block exemptions and result budgets all key on it. Both id factories emit [a-z0-9-] (one with a literal team_ prefix), so two fresh ids cannot collide, but a hand-edited gh_api beside a new gh-api, or a team server team_acme-crm beside a local server named "Team Acme CRM", become one principal at the gateway.

This is the creation-time half of SBS-880:

  • registry::ids_collide names the invariant: two ids collide when their sanitized forms match, case-insensitively.
  • unique_id, which every factory already goes through (local adds, team sync, profiles, rule sets), now treats a candidate as taken when any existing id collides with it, so it renames the way an exact duplicate already did (gh-api-2, team_acme-crm-2).
  • sanitize_segment documents that it is not an identity and points at the invariant.

Not in this PR: rekeying the five gateway isolation sites on raw ids. Existing collisions in a registry are not rewritten. The gateway's prefix-owner tables already refuse ambiguous prefixes (SBS-866), so that remains the fallback for legacy data.

Testing

  • cargo test --no-default-features --lib (1305 passed; new tests cover legacy underscore ids, the team prefix case, case-insensitivity, and the team sync rename)
  • cargo fmt --check
  • Full CI on this PR

Devin Review

Note

Medium Risk
Changes how new server ids are assigned at creation and during team sync, in an area tied to client scope and security isolation; existing colliding entries are left unchanged.

Overview
Fixes SBS-880 by treating two registry server ids as conflicting when the gateway’s lossy sanitize_segment rewrite would make them the same principal (hyphen vs underscore, case, and the local team-acme-crm vs synced team_acme-crm pattern).

Adds registry::ids_collide (case-insensitive compare of sanitized forms) and changes unique_id—used for local adds, team sync, profiles, and rule sets—to reject candidates that collide under that map, appending numeric suffixes the same way exact duplicates already did. sanitize_segment in router.rs is documented as a charset rewrite, not a stable identity.

Regression tests cover underscore legacy ids, team import rename, and case-insensitivity. Does not rekey existing gateway isolation sites or migrate registries that already contain colliding ids.

Reviewed by Cursor Bugbot for commit 8ba2674. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Fix registry.unique_id to keep new server ids distinct under sanitize_segment

  • Adds registry.ids_collide helper that sanitizes both identifiers and compares them case-insensitively, catching hyphen/underscore equivalence and ASCII case differences
  • Replaces raw-string occupancy checks in registry.unique_id with ids_collide checks, so the allocator suffixes any new id whose sanitized form matches an existing one
  • Adds regression tests in registry.rs and teams.rs covering hyphen/underscore collisions, case differences, team-import collisions, and non-collisions
  • Risk: registry.unique_id now allocates more numeric-suffixed ids than before when existing ids share a sanitized form; callers that assumed exact-string uniqueness may see new suffixed names

Macroscope summarized 8ba2674.

@cursor

cursor Bot commented Sep 3, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_cb431411-f84b-4b5c-96ad-5a09a95db62a)

@coderabbitai

coderabbitai Bot commented Sep 3, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 22 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 2 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: ceacdd2a-ee46-4777-9c23-4e4379d0df01

📥 Commits

Reviewing files that changed from the base of the PR and between aece237 and 8ba2674.

⛔ Files ignored due to path filters (1)
  • CHANGELOG.md is excluded by !**/*.md
📒 Files selected for processing (3)
  • src-tauri/src/registry.rs
  • src-tauri/src/router.rs
  • src-tauri/src/teams.rs

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@btsouth
btsouth force-pushed the fix/sanitize-injective-ids branch from dae95b1 to 8409530 Compare September 3, 2026 23:39

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Devin Review

@cursor

cursor Bot commented Sep 3, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_b03fcc43-0b05-44e7-8873-494a45368346)

…BS-880)

sanitize_segment rewrites an id to the tool-name charset and is lossy: gh-api
and gh_api both become gh_api. It is the right rewrite for exposed tool names,
but the client scope set, the PII origin, injection block exemptions and
result budgets all key on that form, so two registry ids that meet there share
scope, exemptions and pseudonym origins. The two id factories emit constrained
charsets, so fresh ids could not collide with each other, but a hand-edited id
carrying an underscore, or the team_ prefix beside a local name that slugifies
to team-..., could. unique_id now treats a candidate as taken when an existing
id matches it under sanitize_segment, case-insensitively, so local adds and
team syncs both rename the way an exact duplicate already did. Existing
collisions in a registry are not rewritten; the gateway's prefix-owner tables
already refuse ambiguous prefixes.
@btsouth
btsouth force-pushed the fix/sanitize-injective-ids branch from 8409530 to c166dd6 Compare September 3, 2026 23:41
@cursor

cursor Bot commented Sep 3, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_7513dbca-421b-4697-b324-f3fa92a76d22)

@cursor

cursor Bot commented Sep 4, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_d6ab11ae-8009-4437-8384-5ae38583515b)

@btsouth
btsouth merged commit d702073 into main Sep 4, 2026
21 of 22 checks passed
@btsouth
btsouth deleted the fix/sanitize-injective-ids branch September 4, 2026 02:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant