Support for passing appId to IsolationSession upon sandbox provision - #802
Support for passing appId to IsolationSession upon sandbox provision#802Dom Giandinoto (daamenik) wants to merge 3 commits into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Adds app-scoped IsolationSession provisioning through PFN resolution and AddUserAsync2.
Changes:
- Resolves empty or absent
appIdvalues from the caller’s PFN. - Updates IsolationSession bindings and provisioning flows.
- Revises related documentation and tests.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/Cargo.toml |
Enables Windows package identity APIs. |
src/core/wxc_common/src/models.rs |
Documents resolved appId behavior. |
src/backends/isolation_session/common/src/app_id.rs |
Implements PFN resolution. |
src/backends/isolation_session/common/src/lib.rs |
Registers the resolver module. |
src/backends/isolation_session/common/src/manager.rs |
Uses AddUserAsync2. |
src/backends/isolation_session/common/src/state_aware.rs |
Resolves state-aware provision IDs. |
src/backends/isolation_session/common/src/one_shot.rs |
Adds default PFN detection. |
src/backends/isolation_session/common/src/error.rs |
Updates operation diagnostics. |
src/backends/isolation_session/bindings/src/bindings.rs |
Regenerates Preview API bindings. |
external/windows-sdk/isolation-session/GENERATION_INFO.toml |
Updates binding provenance date. |
docs/isolation-session/state-aware-typescript.md |
Documents TypeScript behavior. |
docs/isolation-session/state-aware-rust.md |
Documents Rust lifecycle behavior. |
.github/copilot-instructions.md |
Updates backend architecture guidance. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if rc != ERROR_INSUFFICIENT_BUFFER || length == 0 { | ||
| return None; |
There was a problem hiding this comment.
Ignoring this; if there's some kind of error when detecting the PFN, we should just continue.
| // user is live rather than re-activating to find out. | ||
| let (provisioned, _manager) = | ||
| IsolationSessionManager::add_user().map_err(map_lifecycle_error)?; | ||
| IsolationSessionManager::add_user(app_id.as_deref()).map_err(map_lifecycle_error)?; |
| /// Resolved at provision before the OS call: a non-empty value is used | ||
| /// verbatim; an empty string or an absent value opts into PFN | ||
| /// auto-detection (the caller's Package Family Name becomes `PFN:<pfn>`, or | ||
| /// the original value is kept when the caller is unpackaged). The resolved | ||
| /// value is passed to `IsoSessionOps::AddUserAsync2` and carried verbatim |
b546b66 to
1eb1aaf
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (8)
src/backends/isolation_session/common/src/state_aware.rs:80
- The resolved value is not necessarily the identity “actually used”: when the host gate chooses legacy
AddUserAsync, no app ID reaches the OS, but this value is still encoded in thesandboxId. Clarify that distinction so later code does not treat the payload as proof of app association.
// Resolve the caller-supplied `appId` into the value passed to
// `AddUserAsync2`: a non-empty id verbatim, or `PFN:<pfn>` for a
// packaged caller that supplied the default. The resolved value is what
// rides inside the `sandboxId`, so later phases (and any future OS
// consumer) see the identity actually used, not the pre-resolution
// request.
src/core/wxc_common/src/models.rs:271
- This contract states that every resolved value is passed to
AddUserAsync2, but provisioning can select legacyAddUserAsyncand omit it. Document the host-dependent fallback, especially because the value still appears insandboxIdeven when no OS association was created.
/// Resolved at provision before the OS call: a non-empty value is used
/// verbatim; an empty string or an absent value opts into PFN
/// auto-detection (the caller's Package Family Name becomes `PFN:<pfn>`, or
/// the original value is kept when the caller is unpackaged). The resolved
/// value is passed to `IsoSessionOps::AddUserAsync2` and carried verbatim
/// into the `sandboxId`.
src/backends/isolation_session/common/src/manager.rs:55
Err(_)is not limited to the documented “unknown feature” case: transport, activation, and other WinRT failures are also converted into “unsupported.” That can make provisioning continue through legacyAddUserAsyncand silently drop an explicitly requestedappId. Treat only the expectedE_INVALIDARGas an old-host signal and propagate other probe failures.
fn app_scoped_supported_from(level: windows_core::Result<i32>) -> bool {
matches!(level, Ok(level) if level > 0)
src/backends/isolation_session/common/src/error.rs:26
- This operation label is also used by the legacy fallback in
manager.rs, so failures fromAddUserAsyncare now reported asIsoSessionOps.AddUserAsync2. Preserve distinct operation labels and select the matching one with the chosen overload (or use an intentionally method-neutral label) so error envelopes and telemetry identify the API that actually failed.
pub(crate) const ADD_USER: &str = "IsoSessionOps.AddUserAsync2";
docs/isolation-session/state-aware-typescript.md:50
- This says the resolved ID is always passed to
AddUserAsync2and represents the identity actually used, butmanager.rsfalls back toAddUserAsyncon unsupported hosts and does not pass any app ID. Document that host gate here so TypeScript consumers do not assume the returnedSandboxIdproves OS-side app association.
| `appId` | string | absent | Optional identifier for the calling application — the Package Family Name for a packaged app, any string otherwise — associating the provisioned agent user with its owning app. **Resolved at provision by the native layer:** a non-empty value is used verbatim; an empty string (or omitting the field) opts into PFN auto-detection, where the calling process's Package Family Name becomes `PFN:<pfn>` (or the original value is kept when the caller is unpackaged). The resolved value is passed to the OS `AddUserAsync2` overload and carried inside the returned `SandboxId` so later phases recover the identity actually used without the caller re-supplying it. Validated structurally only (no control characters, at most 256 characters); rejections surface as `MxcError` with `code: 'policy_validation'`. Beyond PFN substitution, whitespace and case are preserved exactly, and on an unpackaged host an explicitly supplied empty string is a **distinct** value from omitting the field. Provision-phase only — it is fixed for the sandbox's lifetime, and the `IsolationSessionStartConfig` type rejects it at compile time. |
docs/isolation-session/state-aware-rust.md:60
- The host-gating paragraph above explicitly allows legacy
AddUserAsync, so these fields are not always returned byAddUserAsync2. Mention both overloads to keep the metadata contract internally consistent.
| `agentUserName` | string | The OS-assigned agent account name returned by `AddUserAsync2`, also carried inside the `sandboxId` payload where it serves as the addressing key for every post-provision phase. Format is OS-internal and not stable across builds. |
| `agentUserSid` | string | The security identifier (SID) of the agent user, returned by `AddUserAsync2`. Diagnostic only. |
docs/isolation-session/state-aware-rust.md:356
- On compatibility hosts the code mints users with legacy
AddUserAsync, notAddUserAsync2. Include the fallback here so this concurrency claim matches the implemented host gate.
Distinct `sandboxId`s map to distinct OS agent users (each `AddUserAsync2`
src/backends/isolation_session/common/src/manager.rs:122
- The method documentation promises that
app_idis passed toAddUserAsync2, while the implementation below can call legacyAddUserAsyncwithout it. Describe the gated behavior here so callers understand that the argument is best-effort on older hosts.
/// The OS interface takes an app id plus an optional enterprise account
/// name and token. MXC resolves `app_id` (see [`super::app_id`]) and passes
/// it to the app-scoped [`AddUserAsync2`] overload, with empty strings for
/// the enterprise account name and token — which selects a local agent
/// user. `app_id` is the already-resolved value (`PFN:<pfn>` for a packaged
/// caller that supplied the default, a caller-chosen id verbatim, or empty);
/// resolution happens in the caller so the same value can be recorded in the
/// `sandboxId`.
adpa-ms
left a comment
adpa-ms
left a comment
There was a problem hiding this comment.
[ReviewAgent] Independent review by three reviewers on different model families, each covering the whole change; every delegated claim re-verified against code before inclusion. Read-only — no builds or test runs, so the author's validation results are relied upon rather than checked. Verdict: sign off with findings. Nothing blocking.
Findings that can't be anchored inline (files this PR doesn't touch, but which it makes wrong):
src/core/wxc_common/src/wire.rs:574-577— still describesappIdas merely "Carried inside thesandboxId", with no mention of resolution or the OS call. This text is emitted verbatim intoschemas/dev/mxc-config.schema.0.8.0-dev.json, which is what config authors see via$schema. Becausewire.rsdidn't change, the schema doesn't drift from its source — socheck-schema-codegen.jsstill passes and CI cannot catch this one.sdk/node/src/state-aware-types.ts:46-59— the shipped JSDoc still says "Carried verbatim" and "Nothing consumes it yet". This PR updatedstate-aware-typescript.md, which describes this surface, but not the surface's own documentation.sandbox_id.rs:74-75and:99-104— "carried only — nothing in MXC consumes it today", and "MXC is a pass-through carrier here". Note:81-84is correct as written and needs no change.
Verified correct, recorded so it isn't re-litigated: the PFN: colon cannot corrupt the sandboxId (base64url payload; only the first colon is structural); the IsoSessionFeature constant renames preserve their numeric values and have zero references anywhere in src/; AddUserAsync's arity is unchanged, so the fallback call site is unaffected; treating an empty/absent appId as opting into PFN detection matches the API's own sentinel semantics; validation ordering is safe in both directions; the Win32 buffer protocol (sizing call, allocation, terminator trim) is correct; and the seven inline resolver tests plus the feature-level test cover the decision table well.
One product-level note: on a packaged host there is no way to express "no app association" — an empty value and an omitted one both opt into PFN scoping, so every input except a non-empty literal yields a PFN-scoped registration. Flagging it in case that matters for a caller who wants an unassociated agent user.
| Some(app_id) if !app_id.is_empty() => Some(app_id.to_string()), | ||
| // Empty or absent: prefer the caller's PFN when packaged, otherwise | ||
| // preserve the original (empty or absent) value verbatim. | ||
| _ => match pfn { | ||
| Some(pfn) if !pfn.is_empty() => Some(format!("{PFN_PREFIX}{pfn}")), | ||
| _ => app_id.map(str::to_string), | ||
| }, |
There was a problem hiding this comment.
[ReviewAgent] These two arms produce different registration identities for the same app, and the API uses whichever value it receives verbatim.
The contract: a non-empty appId is used as-is; an empty (or omitted) one is what opts into PFN-scoped registration. So the PFN: prefix on line 48 is part of the identity, not decoration — and a packaged caller who passes the bare PFN Contoso.App_8wekyb3d8bbwe is registered under the literal Contoso.App_8wekyb3d8bbwe, which is a different registration from the PFN:Contoso.App_8wekyb3d8bbwe this line produces for that same app.
That bare form is what every example in this repo shows (docs/schema.md:251, mxc-state-aware-sandbox-api.md:1600,1906,1921, four SDK tests) and what state-aware-rust.md:53 instructs packaged callers to pass.
Worth stating explicitly wherever appId is documented: to obtain PFN-scoped registration, pass "" / omit the field, or pass the literal PFN:<pfn>. A bare PFN is a distinct, non-PFN-scoped identity. The examples should then be updated to whichever form is intended.
There was a problem hiding this comment.
I'll update the documentation to explicitly state that if the app is packaged, the user should provide the PFN in the format "PFN:<pfn>"
| | Field | Type | Default | Description | | ||
| |---|---|---|---| | ||
| | `appId` | string \| absent | absent | Optional identifier for the calling application. For a **packaged** application this is the Package Family Name; for an unpackaged one it may be any string. Carried verbatim inside the `sandboxId` (see below) so later phases recover it without the caller re-supplying it. **Nothing consumes it today** — it is accepted now so a future OS contract that acts on the calling application's identity does not require a breaking change. Validated **structurally only** (no control characters; at most 256 characters) — MXC is a pass-through carrier here and does not judge what a valid application identity looks like, so enforcing a PFN grammar would risk rejecting forms a future OS API accepts. Preserved verbatim: no trimming, no case folding, no normalisation. An explicitly-supplied **empty string is a distinct value from absent** and round-trips as such (a future OS API may assign it meaning, and MXC never synthesizes an empty string the caller did not send); JSON `null` is a second spelling of absent. Rejections surface as `policy_validation` from `validate_provision`, before any OS call. The wire path is `experimental.isolation_session.provision.appId`. | | ||
| | `appId` | string \| absent | absent | Optional identifier for the calling application, associating the provisioned agent user with its owning app. For a **packaged** application this is the Package Family Name; for an unpackaged one it may be any string. **Resolution at provision:** a non-empty value is used verbatim; an **empty string or absent** value opts into PFN auto-detection — MXC reads the calling (`wxc-exec`) process's Package Family Name and substitutes `PFN:<pfn>`, or, when the caller is unpackaged (or the lookup fails), keeps the original value unchanged (`""` stays `""`, absent stays absent). The **resolved** value is what is passed to `IsoSessionOps::AddUserAsync2` and carried inside the `sandboxId` (see below), so later phases recover the identity actually used without the caller re-supplying it. **Host gating:** the app-scoped `AddUserAsync2` overload is used only when the host advertises `IsoSessionFeature::AppScopedRegistration` via `GetFeatureLevel` (a positive level); on older hosts — where `GetFeatureLevel` returns a non-positive level or rejects the unknown feature value with an error — MXC falls back to the legacy `AddUserAsync`, which carries no `appId`, so the agent user is provisioned without app association (the resolved value is still recorded in the `sandboxId`). Validated **structurally only** (no control characters; at most 256 characters) — MXC does not judge what a valid application identity looks like, so enforcing a PFN grammar would risk rejecting forms a future OS API accepts. Beyond PFN substitution there is no trimming, case folding, or normalisation. An explicitly-supplied **empty string is a distinct value from absent** on an unpackaged host and round-trips as such; JSON `null` is a second spelling of absent. Rejections surface as `policy_validation` from `validate_provision`, before any OS call. The wire path is `experimental.isolation_session.provision.appId`. | |
There was a problem hiding this comment.
[ReviewAgent] This row tells a packaged caller that appId "is the Package Family Name" — i.e. a bare PFN — but a bare PFN and PFN:<pfn> are two different registration identities, and only the latter (or an empty/omitted value) is PFN-scoped. Suggest documenting both accepted spellings here.
The same wording appears in state-aware-typescript.md, docs/schema.md:251 and docs/state-aware-lifecycle/mxc-state-aware-sandbox-api.md:1600,1906,1921.
There was a problem hiding this comment.
I'll update the documentation to explicitly state that if the app is packaged, the user should provide the PFN in the format "PFN:<pfn>"
| let async_op = if app_scoped_supported_from( | ||
| ops.GetFeatureLevel(IsoSessionFeature::AppScopedRegistration), | ||
| ) { | ||
| ops.AddUserAsync2( | ||
| &HSTRING::from(app_id.unwrap_or_default()), | ||
| &HSTRING::new(), | ||
| &HSTRING::new(), | ||
| ) | ||
| } else { | ||
| ops.AddUserAsync(&HSTRING::new(), &HSTRING::new()) | ||
| } | ||
| .map_err(|e| transport_err(op::ADD_USER, "call failed", &e))?; |
There was a problem hiding this comment.
[ReviewAgent] On this branch the appId cannot be passed — the 2-arg AddUserAsync has no such parameter — yet state_aware.rs:93-96 still encodes the resolved value into the sandboxId. The id then asserts an app association this path did not make.
Provision has to keep succeeding on these hosts, so this isn't a request to fail. But the recorded value shouldn't disagree with what happened: consider having add_user return the identity actually applied and encoding that (None here), so the sandboxId can't over-claim.
Separately, app_scoped_supported_from maps any Err to "not supported", while the doc comment above justifies only the documented unknown-feature rejection. A transport failure on the probe is currently indistinguishable from an old host — worth separating "the host says no" from "the query failed".
| pub(super) mod op { | ||
| pub(crate) const ACTIVATE: &str = "IsoSessionOps.ActivateInstance"; | ||
| pub(crate) const ADD_USER: &str = "IsoSessionOps.AddUserAsync"; | ||
| pub(crate) const ADD_USER: &str = "IsoSessionOps.AddUserAsync2"; |
There was a problem hiding this comment.
[ReviewAgent] This constant now reports AddUserAsync2 for both branches, including when the fallback actually invoked the 2-arg AddUserAsync — and it covers the whole ladder, since manager.rs:151-154 and the result-property reads reuse it for "call failed" and "wait failed".
The module doc immediately above states the contract this breaks: "Interface-qualified names of the API operations this backend invokes … the values that reach the wire as error.operation." On a legacy host this groups failures under an API that was deliberately not called, which is misleading precisely where the fallback is otherwise invisible. Suggest selecting the operation constant alongside the overload.
| // `appId` rides inside the id so later phases recover it without the | ||
| // caller re-supplying it. Nothing consumes it yet; it is carried for a |
There was a problem hiding this comment.
[ReviewAgent] Both halves of this comment are falsified by this PR. "Nothing consumes it yet" — AddUserAsync2 consumes it now. And the justification continuing on lines 91-92 ("the caller already has the value it supplied") no longer holds when the value was auto-detected: the caller supplied nothing, and has no supported way to learn what was used — the payload after iso: is documented as backend-private, and state-aware-typescript.md:60-63 states the SandboxId is opaque and "nothing in the SDK parses past the iso: prefix".
Suggest echoing the resolved/applied appId in IsolationSessionProvisionMetadata, and updating this comment plus the matching justification in state-aware-rust.md:63-64.
| fn current_package_family_name() -> Option<String> { | ||
| // Sizing call: a null buffer yields ERROR_INSUFFICIENT_BUFFER plus the | ||
| // required length (in characters, including the terminator) for a packaged | ||
| // process, or APPMODEL_ERROR_NO_PACKAGE for an unpackaged one. | ||
| let mut length: u32 = 0; | ||
| let rc = unsafe { GetCurrentPackageFamilyName(&mut length, Some(PWSTR::null())) }; | ||
| if rc != ERROR_INSUFFICIENT_BUFFER || length == 0 { | ||
| return None; | ||
| } | ||
|
|
||
| let mut buffer = vec![0u16; length as usize]; | ||
| let rc = unsafe { GetCurrentPackageFamilyName(&mut length, Some(PWSTR(buffer.as_mut_ptr()))) }; | ||
| if rc != ERROR_SUCCESS { | ||
| return None; | ||
| } | ||
|
|
||
| // `length` counts the null terminator; trim it before decoding. | ||
| let chars = (length as usize).saturating_sub(1).min(buffer.len()); | ||
| Some(String::from_utf16_lossy(&buffer[..chars])) | ||
| } |
There was a problem hiding this comment.
[ReviewAgent] current_package_family_name() returns None for any failure, not only the no-package case — so a packaged caller hitting a transient lookup error takes the unpackaged path and MXC passes "" onward.
Since "" is what opts into PFN-scoped registration, the API then resolves it and registers the caller's PFN — while the sandboxId records "". The registration is right; MXC's record of it is wrong. Same over/under-claiming theme as the fallback path.
At minimum, log the non-no-package failures so this is diagnosable; ideally distinguish them.
| pub(crate) fn resolve_app_id(app_id: Option<&str>) -> Option<String> { | ||
| resolve_app_id_with(app_id, current_package_family_name().as_deref()) | ||
| } |
There was a problem hiding this comment.
[ReviewAgent] current_package_family_name() is an argument expression, so it is evaluated eagerly — the lookup runs on every provision, including when app_id is Some("explicit") and the result is discarded at line 44. Harmless today, but branching on presence before consulting the detector would make the code visibly do the lookup only when it's needed, and it's the same restructure as any other edit here.
…assed for packaged callers as appId
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
src/backends/isolation_session/common/src/error.rs:26
op::ADD_USERis also used for the legacy fallback atmanager.rs:148-151. Consequently, failures fromAddUserAsyncare reported and grouped in telemetry asIsoSessionOps.AddUserAsync2, obscuring which API actually failed. Define separate operation values and select the one matching the chosen branch.
pub(crate) const ADD_USER: &str = "IsoSessionOps.AddUserAsync2";
| "properties": { | ||
| "appId": { | ||
| "description": "Optional application identifier for the calling application. For a packaged application this is the Package Family Name; for an unpackaged one it may be any string. Carried inside the `sandboxId` so later lifecycle phases can recover it without the caller re-supplying it.", | ||
| "description": "Optional identifier for the calling application.\n\n**A packaged application must supply its Package Family Name in the form `PFN:<packageFamilyName>`** (for example `PFN:Contoso.App_8wekyb3d8bbwe`) — the literal prefix `PFN:` followed by the PFN. A non-empty value is used verbatim. An unpackaged application may pass any string.\n\nAlternatively, a packaged caller may pass an empty string or omit the field to opt into best-effort PFN auto-detection: MXC reads the calling process's Package Family Name and substitutes `PFN:<pfn>` for it. On an unpackaged caller the empty/absent value is kept unchanged.\n\nResolution happens at provision, before the OS call; the resolved value is passed to `IsoSessionOps::AddUserAsync2` to associate the provisioned agent user with its owning app, and is carried inside the `sandboxId` so later lifecycle phases recover the identity actually used without the caller re-supplying it.", |
| } else { | ||
| ops.AddUserAsync(&HSTRING::new(), &HSTRING::new()) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/backends/isolation_session/common/src/manager.rs:55
- This treats every
GetFeatureLevelfailure as an unsupported old host, although the contract described above only identifiesE_INVALIDARGas the compatibility signal. A transient RPC/service/access failure can therefore be swallowed and provisioning can continue through legacyAddUserAsync, silently dropping the requested app association. Return aResult<bool, _>, map onlyE_INVALIDARGtoOk(false), and propagate other failures fromadd_user.
fn app_scoped_supported_from(level: windows_core::Result<i32>) -> bool {
matches!(level, Ok(level) if level > 0)
src/backends/isolation_session/common/src/error.rs:26
- This operation label is now inaccurate on the documented compatibility path: when app-scoped registration is unavailable,
manager.rscalls legacyAddUserAsync, but every call, wait, and result error is still emitted asIsoSessionOps.AddUserAsync2. Track which overload was selected (or use an overload-neutral provisioning operation) so telemetry and diagnostics identify the operation that actually failed.
pub(crate) const ADD_USER: &str = "IsoSessionOps.AddUserAsync2";
src/core/wxc_common/src/wire.rs:592
- This documentation promises that the resolved ID is passed to
AddUserAsync2and represents the identity actually used, butmanager.rsdeliberately falls back toAddUserAsyncon unsupported hosts and records the resolved ID even though no app association was made. Document that host gate and fallback here; because this rustdoc generates the schema and wire types, also regenerate those artifacts and align the public TypeScript documentation, which currently repeats the unconditional guarantee.
/// Resolution happens at provision, before the OS call; the resolved value
/// is passed to `IsoSessionOps::AddUserAsync2` to associate the provisioned
/// agent user with its owning app, and is carried inside the `sandboxId` so
/// later lifecycle phases recover the identity actually used without the
/// caller re-supplying it.
📖 Description
Update sandbox-provision APIs to take in an
appId. If""is passed as theappId, attempt to detect the calling process's Package Family Name (PFN) and usePFN:<pfn>instead. If calling process is unpackaged, useappIdverbatim.Use the
AddUserAsync2API from the IsoSessionOps Preview API instead ofAddUserAsync().🔗 References
🔍 Validation
✅ Checklist
Cargo.lock, thedependency-feed-checkcheck passes (see docs/pull-requests.md)📋 Issue Type
GitHub Actions runs the PR validation build automatically. The ADO pipeline
(
MXC-PR-Build) is the Azure version of the PR pipeline, kept in parity with the GitHubActions build; it runs on merge to
main, and Microsoft reviewers with write access can trigger iton a PR with
/azp run. See docs/pull-requests.md.If the
dependency-feed-checkcheck fails on a new dependency, the crate must be added tothe feed before the PR can pass. See docs/pull-requests.md
for the steps.
Microsoft Reviewers: Open in CodeFlow
Microsoft Reviewers: Open in CodeFlow