Skip to content

Harden reporter telemetry privacy projection - #5990

Open
Sean Larkin (TheLarkInn) wants to merge 5 commits into
mainfrom
copilot/reporter-r4a-telemetry-privacy
Open

Harden reporter telemetry privacy projection#5990
Sean Larkin (TheLarkInn) wants to merge 5 commits into
mainfrom
copilot/reporter-r4a-telemetry-privacy

Conversation

@TheLarkInn

Copy link
Copy Markdown
Member

Summary

  • collect producer package identities and protocol metadata only from public envelopes
  • project lifecycle values only from public envelopes while preserving explicitly allowlisted diagnostic codes/categories from local-sensitive diagnostics
  • ignore secret diagnostics and add mixed-order regression coverage for public, local-sensitive, and secret first-party/extension events

Review context

关注-up to #5867 (comment).

Threat model

TelemetrySubscriber observes canonical events before reporter filtering. A private extension or internal plugin can therefore place a private package name/version in envelope source metadata even when the event is classified local-sensitive or secret. This change treats source/protocol metadata and lifecycle payload fields as telemetry-safe only for public envelopes. Extension payloads remain outside the projection, secret events contribute no values, and the existing field-level allowlist for public diagnostic code/category data remains intact.

Validation

  • rushx _phase:build
  • ESLint on the changed telemetry and test files
  • rushx _phase:test (293 tests passed)
  • rush change --verify

Non-goals

  • wiring reporter aggregates into the live rush-lib upload stack
  • changing local reporter redaction or storage behavior
  • broadening reporter privacy classification semantics outside this telemetry projection

Part of #5977

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: d6318e80-5da9-4858-a147-817e8692f10e
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: d6318e80-5da9-4858-a147-817e8692f10e
@TheLarkInn
Sean Larkin (TheLarkInn) enabled auto-merge (squash) August 28, 2026 03:37
@TheLarkInn

Copy link
Copy Markdown
Member Author

Independent review found one mixed-privacy regression in the initial patch: secret diagnostic envelopes were dropping the explicitly public diagnostic code/category fields. Commit e7d7c41 restores those allowlisted fields while continuing to exclude non-public producer identity, protocol, lifecycle, parameters, and other payload values.

Local validation: reporter clean build, all 293 reporter tests, focused telemetry tests, and rush change --verify. Squash auto-merge is enabled; awaiting required CI and approval.

@TheLarkInn

Copy link
Copy Markdown
Member Author

Combined deep review of current head e7d7c4136b.

MEDIUM TelemetrySubscriber.ts:53-72 accepts diagnostic code and category from non-public envelopes without validating either value or bounding cardinality. A malformed producer can place sensitive or unbounded strings into uploaded telemetry.

Required fix: validate codes against the registry, restrict categories to known values or other, and cap distinct values before collecting them.

Validate non-public diagnostic codes against the registry, bucket unknown categories, and bound retained telemetry dimensions deterministically.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: d6318e80-5da9-4858-a147-817e8692f10e
@TheLarkInn

Copy link
Copy Markdown
Member Author

Fixed in 8888b3d6333059e1e3ee651ef3a3400c14200e2c: non-public diagnostic codes now require runtime shape validation and central registry membership, categories are reduced to known values or other, and diagnostic dimensions use deterministic 20-entry budgets with registered codes prioritized. Added foreign-envelope regressions for token/path values, valid local-sensitive and secret fields, mixed privacy ordering, and cardinality floods.

Validated reporter build with TypeScript, ESLint, and API Extractor; focused TelemetrySubscriber tests; all 296 reporter tests; rush check; and rush change --verify. Auto-merge remains disabled pending the next four-review round.

@TheLarkInn

Copy link
Copy Markdown
Member Author

Four-review rerun on 8888b3d633 confirms the original diagnostic validation issue is fixed. Three actionable findings remain:

  1. HIGH Public foreign events can overwrite the session protocol version. Record protocol ownership only from the primary session.
  2. MEDIUM producerVersions is unbounded and accepts unrestricted child-controlled strings. Add deterministic cardinality and length budgets, with trusted producers prioritized.
  3. MEDIUM A diagnostic with mixed public and secret parameters can still be stamped public and admit producer identity. Gate producer metadata on effective payload sensitivity, not only the envelope label.

Useful low follow-ons include exact category-cap handling, category derivation for non-public diagnostics, deterministic category ordering, and explicit child-diagnostic attribution. Auto-merge remains disabled pending another fix and review round.

Keep protocol metadata root-owned, gate mixed-privacy diagnostics, and bound producer attribution with trusted deterministic retention.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: d6318e80-5da9-4858-a147-817e8692f10e
@TheLarkInn

Copy link
Copy Markdown
Member Author

Fixed the round-two findings in a3505aec1ed738c7afe5315110ecabe72074bf75: protocol metadata is root-owned; producer versions are capped at 20 entries and 256 characters with trusted namespace priority and deterministic retention; and mixed public plus secret lifecycle diagnostics no longer admit producer or protocol metadata. Non-public categories now come from the registered code, category insertion respects the exact cap, and category keys serialize in sorted order. Safe child diagnostics remain attributed, while sensitive child producer identity is excluded.

Validated the reporter build with TypeScript, ESLint, and API Extractor; 15 focused TelemetrySubscriber tests; all 299 reporter tests; rush check; and rush change --verify. Auto-merge remains disabled pending the next four-review round.

@TheLarkInn

Copy link
Copy Markdown
Member Author

Four-review rerun on a3505aec1e confirms all prior telemetry fixes. One MEDIUM issue remains: trusted producer priority is inferred from child-supplied package-name prefixes. A foreign child can spoof @microsoft/ names and evict genuine root producers from the bounded set.

Required fix: derive priority from trusted provenance such as root-session ownership, not namespace text. Add spoofed-prefix flood coverage. Auto-merge remains disabled.

Derive bounded producer retention priority from parent-session provenance instead of child-controlled package namespaces.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: d6318e80-5da9-4858-a147-817e8692f10e
@TheLarkInn

Copy link
Copy Markdown
Member Author

Fixed the final producer-priority finding in 70a28dd51353fc7579c7483c97e327c82eff6f98: bounded producer retention now protects parent-session producers based on provenance, not package namespace text. Foreign children remain attributable within the count and length budgets, but spoofed @microsoft/ and @rushstack/ names cannot evict parent-owned entries.

Added root-first and root-last spoofed-prefix floods in forward and reverse order, oversized parent and child values, and child attribution coverage. Reporter build, ESLint, API Extractor, all 299 reporter tests, rush check, and rush change --verify pass. Auto-merge remains disabled pending final review.

@TheLarkInn

Copy link
Copy Markdown
Member Author

Final combined review of 70a28dd513 found no critical, high, or medium actionable issue. All recorded privacy, validation, cardinality, protocol ownership, mixed-privacy, and producer provenance findings are fixed.

Dismissible low follow-ons: always derive a registered public diagnostic category from the registry, expose aggregate truncation indicators, and defensively handle impossible non-positive hard-coded budgets. This head is low-only and eligible for normal repository gates.

@TheLarkInn

Copy link
Copy Markdown
Member Author

Ian Clanton-Thuon (@iclanton) David Michon (@dmichon-msft) This independent privacy root is full-CI green, low-only after repeated review, and auto-merge enabled. Approval is the only remaining gate, and it blocks the AI qualification rollout prerequisite.

注册 for free to join this conversation on GitHub. Already have an account? 登录 to comment

标签

None yet

项目

Status: Needs triage

Development

Successfully merging this pull request may close these issues.

1 participant