Skip to content

Surface vetted-connector branding to Internet Identity, bound to the authorization session - #200

Open
aterga wants to merge 6 commits into
mainfrom
feat/cimd-connector-branding
Open

aterga wants to merge 6 commits into
mainfrom
feat/cimd-connector-branding

Conversation

@aterga

@aterga aterga commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator

What

This is the imcp2 side of client branding. Internet Identity's consent screen can then show which vetted product is asking for a connect, instead of granting II accounts to an anonymous "some bridge". It complements the CIMD registration mode (#191).

Design

  • Server-curated connector table in the new src/branding module. It covers ChatGPT, Claude, Cursor, Grok, Perplexity and Google Antigravity.
    • It is keyed on the session's validated redirect_uri vendor, never on the client-supplied client_name/logo_uri. Open DCR takes all callers, so client-supplied identity is attacker-controlled.
    • Anyone may register a vetted redirect, but the path-pinned allow-list delivers the code only to that vendor's callback. The redirect vendor is therefore a sound, server-determined proxy for product identity.
    • Only a redirect admitted by a compiled-in DEFAULT_ALLOWED_REDIRECTS entry is branded (redirect_uri_on_default_allow_list). An OAUTH_ALLOWED_REDIRECT_PREFIXES entry works for redirects but is never branded, even when it's on a vendor's domain, so a vendor's name only ever rests on the reviewed callback paths in this repo.
    • A test holds the curated domains equal to the compiled-in allow-list vendors.
  • Session-bound metadata. GET {issuer}/branding?state=<state> → { name, logo, verified }, CORS-open. Every response, including the 404, is no-store.
    • II passes the state it already holds from the connect link.
    • The server looks up that pending, unexpired connect and derives the connector from the redirect that authorize validated.
    • Nothing in the connect-link fragment asserts branding. The fragment is craftable by any native client that drives the navigation, so a slug there would let a loopback session claim to be "verified Claude".
    • A loopback or unlisted session gets 404, as does an unknown, missing, expired or malformed state. They are indistinguishable, so the endpoint is no session oracle.
    • The lookup is read-only and cannot disturb the handshake.
  • Static logo. GET {issuer}/branding/{slug}/logo → SVG with nosniff, a cache header, and a sandboxing CSP (default-src 'none'; style-src 'unsafe-inline'; sandbox). It returns 404 outside the curated set.
    • II must render it via <img> and never inline it, because an inlined SVG can run script.
    • The CSP covers the case where the logo URL is opened top-level, where it would otherwise be an SVG document on the issuer origin. A test keeps every bundled logo free of script, event handlers, embedded HTML and external references.
  • One host rule. Redirect validation, the allow-list env parser, the CIMD trust match and branding all use host_key + host_is_or_under, so they cannot disagree about a vendor.
    • For example, https://claude.ai./… (accepted by validation) brands as Claude.
    • A redirect that validation would refuse never resolves to a connector.

Logos are placeholders

The bundled logo is an original, neutral "verified connector" mark. It is deliberately not a reproduction of any vendor's logo, because that is a trademark and licensing matter this repo shouldn't decide. Each vendor's licensed mark can go into its Connector::logo; the factual product names ship now.

Not yet visible: needs II's counterpart

II has to:

  • fetch /branding?state= from the validated callback origin, using the one state it parsed from the link for both the lookup and the callback;
  • show the verified treatment only for imcp2 issuer origins that II itself trusts.

Until that ships, the endpoints are inert.

Tests

  • Unit:
    • redirect → connector resolution, including subdomains, a trailing dot and uppercase;
    • rejection of look-alike apexes, loopback, unlisted hosts, http, redirects validation refuses, and operator-added entries on a vendor's domain;
    • bundled logos are static SVG (no script, handlers, embedded HTML or external references);
    • closed slug lookup;
    • curated domains equal to allow-list domains.
  • Unit, branding_is_bound_to_the_session: a vetted session → 200; native → 404; unknown or missing → 404; expired → 404; the lookup leaves the pending connect untouched.
  • II link: it carries no connector param.
  • Router:
    • The DCR round-trip test drives branding over HTTP: authorize → state from II's link → /branding?state= → 200 Claude + no-store for both claude.ai spellings, and 404 for the loopback session. It is folded into the one accepting-registration test so it can't race the shared store file.
    • No session → 404 with CORS and no-store; the old unbound /branding/{slug} metadata route is gone; the logo is served as SVG with nosniff and the sandboxing CSP.
  • Full workspace green; no new clippy warnings in the root crate.

🤖 Generated with Claude Code

Implements the imcp2 side of the client-branding extension
(docs/scoping-client-branding.md, PR #103): let II show WHICH vetted
product is asking for a connect, instead of an anonymous "some bridge".

- New `branding` module: a server-curated table of vetted connectors
  (ChatGPT, Claude, Cursor, Grok, Perplexity, Google Antigravity), keyed on
  the request's already-validated `redirect_uri` vendor — never on the
  client-supplied `client_name`/`logo_uri`, which open DCR makes
  attacker-controlled (rendering them would be a consent-phishing gift). A
  connect that doesn't resolve to a vetted web vendor — including every
  loopback (native-app) redirect — keeps the status-quo anonymous screen.
- `authorize` resolves the connector for the validated redirect and passes
  its slug on the II connect link as `&connector=<slug>` (additive fragment
  param; an II that doesn't know it ignores it).
- Two issuer-rooted GET endpoints II fetches same-origin with the
  #4091-validated callback: `/branding/{slug}` (name + absolute logo URL +
  `verified`, `no-store`) and `/branding/{slug}/logo` (SVG, `nosniff`).
  Both 404 outside the curated set so the path can't probe.

The bundled logos are ORIGINAL neutral placeholders (a generic "verified
connector" mark), deliberately not reproductions of vendor logos — each
vendor's own licensed mark is dropped into its `Connector::logo` later.
Actually rendering this needs II's counterpart change (parse the slug,
fetch these endpoints, render); until then it is inert and harmless.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@aterga
aterga requested review from a team and a balanced review from Copilot September 23, 2026 21:30

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Branding is not bound to its authorization session, allowing an attacker-crafted fragment to claim another vetted connector.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity · 1 Medium severity

Open (2)
What changed in this PR

Adds vetted connector branding for Internet Identity consent screens.

Changes:

  • Maps validated vendor redirects to curated connector branding.
  • Adds branding metadata/logo endpoints.
  • Passes optional connector slugs through II connect URLs with tests.
File Description
src/​branding.rs Defines branding catalog and endpoints.
src/​auth.rs Resolves branding during authorization.
src/​lib.rs Registers branding routes.
crates/​imcp2-core/​src/​iiconnect.rs Adds connector URL parameter.
crates/​imcp2-local/​src/​login/​mod.rs Omits branding for local login.
tests/​routers.rs Tests branding endpoints.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/branding.rs Outdated
Comment thread src/branding.rs Outdated
The first cut let the connect-link fragment carry `&connector=<slug>` and
served `GET {issuer}/branding/{slug}` unconditionally. The fragment is
craftable by any native client that drives the navigation, so a loopback
session could have presented itself to Internet Identity as "verified
Claude".

Branding is now answered per session: II asks `GET {issuer}/branding?state=`
with the `state` it already holds, and the server derives the connector from
that pending, unexpired connect's validated `redirect_uri`. Nothing in the
fragment asserts branding (the connector param is gone from `ii_mcp_url`),
and a loopback or unlisted session gets the same 404 as an unknown one. The
lookup is read-only, so it cannot disturb the handshake.

Branding and redirect validation now share one host rule (`host_key` +
`host_is_or_under`), so a trailing-dot `claude.ai.` redirect, which
validation accepts, brands as Claude instead of reading as anonymous; the
allow-list parser and the CIMD trust match use the same helpers. A redirect
validation would refuse never resolves to a connector, and a test holds the
curated domains equal to the compiled-in allow-list vendors.

The DCR router test now drives the 200 path over HTTP (authorize -> the
`state` in II's link -> `/branding?state=`), including `no-store` and both
claude.ai spellings.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 24, 2026 12:04
@aterga aterga changed the title Surface vetted-connector branding to Internet Identity (name + logo) Surface vetted-connector branding to Internet Identity, bound to the authorization session Sep 24, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The implemented session-bound API conflicts with the documented Internet Identity integration contract.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity · 1 Low severity

Open (2)
Resolved since last review (2)

Comment thread src/lib.rs
Comment thread src/branding.rs Outdated
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 24, 2026 12:09

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

The implementation is securely session-bound and well tested; only a minor public-rustdoc omission remains.

Review effort: Balanced
Findings: None

Resolved since last review (2)
Previously missed (1)

In code that hasn't changed since last review

Low severity Update mcp_router rustdoc for branding routes and fallback scope

src/​lib.rs:341

The public mcp_router rustdoc at lines 231–244 still lists only the OAuth and metadata routes and says “everything else” reaches the MCP fallback. This new public route makes that contract inaccurate; document both branding endpoints (including their CORS behavior) and qualify the fallback as applying only to unmatched paths.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 24, 2026 13:13

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

The implementation matches the revised session-bound design and is covered by focused unit and router tests.

Review effort: Balanced
Findings: None

connector_for_redirect accepted anything the effective allow-list admits,
so an OAUTH_ALLOWED_REDIRECT_PREFIXES entry on a vendor's domain (another
path on claude.ai, say) was shown as that vendor, contrary to the docs.
Branding now requires a redirect admitted by a compiled-in
DEFAULT_ALLOWED_REDIRECTS entry, under validation's own rules
(redirect_uri_on_default_allow_list, sharing hosted_redirect_admitted with
redirect_uri_permitted), so a vendor's name only ever rests on the reviewed
callback paths in this repo.

The branding 404 is now no-store like the 200 it stands in for, and the logo
carries a sandboxing CSP: opened top-level it is an SVG document on the
issuer origin, where <img> isolation does not apply. A test keeps every
bundled logo free of script, event handlers, embedded HTML, and external
references, for when licensed vendor marks replace the placeholder.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 24, 2026 14:53

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

The session binding, redirect vetting, caching, routing, and SVG safeguards are coherent and well tested.

Review effort: Balanced
Findings: None

The logo test banned a handful of literal substrings, so a spaced
`href = "https://…"` or a quoted `url('https://…')` got through. It now
strips whitespace first and requires every href, src, and url() target to
be an internal `#…` reference, and also rejects XHTML, animation, and DTDs.
It checks itself against a set of unsafe SVGs, spaced and quoted spellings
included, so the guard can't quietly weaken when licensed vendor marks
replace the placeholder.

The module doc now says what branding vouches for: where the authorization
code is delivered, not who started the connect.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 24, 2026 15:03
aterga added a commit that referenced this pull request Sep 24, 2026
The first draft put the product slug in the connect-link fragment
(`&connector=<slug>`) and served an unbound `GET {issuer}/branding/{slug}`.
The fragment is written by whoever drives the browser, so that let a
consent screen vouch for a product the server never tied to the connect.

The contract now matches what #200 implements: II asks
`GET {issuer}/branding?state=` and the server answers from the connect's
validated redirect, with nothing about branding in the link. The revision
also:

- states precisely what branding vouches for (where the authorization code
  is delivered, not who started the connect or which vendor account ends up
  holding the grant) and the residual risks that follow;
- spells out the II side as numbered requirements: issuer derivation from
  the validated callback, response acceptance, one callback and one state
  for everything, an exact-origin trust list gating the whole treatment,
  and safe rendering (CSP, <img>, wording);
- notes that branding covers only compiled-in callbacks, never operator
  allow-list entries, and documents the logo hardening;
- replaces line-number citations with symbol references.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

Branding is securely session-bound, consistently validated, and comprehensively tested.

Review effort: Balanced
Findings: None

This branch has not been deployed

No deployments
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.

2 participants