Improve MotW handling and clarify the MSB3821 by JanKrivanek · Pull Request #14015 · dotnet/msbuild

JanKrivanek · 2026-06-10T09:47:11Z

Context

ResXFileRef could lead to adding linked resources - so it should not be processed if 'mark of the web' metadata is present. Going forward - we should not allow any typed entry (as for unknown types the Type.GetType is eventually called, which can probe for assemblies on disk).

Additionaly - clarifying the messaging for MotW - as general guidance is unchanged - users are still responsible for reviewing/trusting the msbuild inputs and env.

IsDangerous only flagged data/metadata nodes that had a mimetype attribute. A ResXFileRef entry uses a type attribute (no mimetype) and later triggers AddLinkedResource, which opens and reads the linked external file. A MotW-marked .resx could therefore bypass the MSB3821 trust check and cause GenerateResource to embed arbitrary build-user-readable file contents into output resources. Fix: also flag the file as dangerous when a data/metadata node has a type attribute referencing ResXFileRef. Additionally, behind ChangeWave 18.8, treat any typed entry on a MotW-marked .resx as untrusted (defense in depth).

Copilot

Pull request overview

This PR tightens GenerateResource’s handling of Mark-of-the-Web (Internet/Restricted zone) inputs to reduce risk from .resx files that can trigger type loading / linked-file processing, and updates MSB3821 text to include secure-usage guidance.

Changes:

Expand “dangerous” .resx/.resw detection to treat ResXFileRef entries as dangerous and (behind ChangeWave 18.8) treat any typed entry as dangerous for MotW-tagged files.
Clarify MSB3821 to instruct users to unblock only if they trust the files and link to MSBuild security best practices.
Update localized XLF source strings and mark translations as needing review.

Show a summary per file

File	Description
src/Tasks/Resources/xlf/Strings.zh-Hant.xlf	Updates MSB3821 source string; marks translation as needing review.
src/Tasks/Resources/xlf/Strings.zh-Hans.xlf	Updates MSB3821 source string; marks translation as needing review.
src/Tasks/Resources/xlf/Strings.tr.xlf	Updates MSB3821 source string; marks translation as needing review.
src/Tasks/Resources/xlf/Strings.ru.xlf	Updates MSB3821 source string; marks translation as needing review.
src/Tasks/Resources/xlf/Strings.pt-BR.xlf	Updates MSB3821 source string; marks translation as needing review.
src/Tasks/Resources/xlf/Strings.pl.xlf	Updates MSB3821 source string; marks translation as needing review.
src/Tasks/Resources/xlf/Strings.ko.xlf	Updates MSB3821 source string; marks translation as needing review.
src/Tasks/Resources/xlf/Strings.ja.xlf	Updates MSB3821 source string; marks translation as needing review.
src/Tasks/Resources/xlf/Strings.it.xlf	Updates MSB3821 source string; marks translation as needing review.
src/Tasks/Resources/xlf/Strings.fr.xlf	Updates MSB3821 source string; marks translation as needing review.
src/Tasks/Resources/xlf/Strings.es.xlf	Updates MSB3821 source string; marks translation as needing review.
src/Tasks/Resources/xlf/Strings.de.xlf	Updates MSB3821 source string; marks translation as needing review.
src/Tasks/Resources/xlf/Strings.cs.xlf	Updates MSB3821 source string; marks translation as needing review.
src/Tasks/Resources/Strings.resx	Updates MSB3821 English resource string with security guidance link.
src/Tasks/GenerateResource.cs	Extends MotW “dangerous” detection to cover `ResXFileRef` and typed entries (ChangeWave-gated).

Copilot's findings

Files reviewed: 15/15 changed files
Comments generated: 2

JanProvaznik · 2026-06-15T09:49:48Z

/review

github-actions · 2026-06-15T09:52:30Z

⚠️ Security scanning failed for Expert Code Review (command). Review the logs for details.

Dependency Management — LGTM

github-actions · 2026-06-15T10:21:33Z

Code Review — Design-Level Findings

🔴 MAJOR: No tests for new `IsDangerous()` behavior (Test Coverage, dim 4)

No test files were changed in this PR. Both new code paths have zero coverage:

.resx with type containing "ResXFileRef" → should return dangerous = true (new unconditional path)
.resx with any type + Wave18_8 enabled → dangerous = true (new wave-gated path)
.resx with any type + Wave18_8 disabled → dangerous = false (wave-off regression guard)
.resx with no type/mimetype → dangerous = false (baseline)

This is especially important for a security-sensitive code path — a mistyped literal or accidental logic inversion would silently disable the protection with no failing test to catch it. The inner XML-parsing logic can be extracted into a static helper taking a TextReader, making it testable cross-platform without the COM/zone machinery.

Relevant test file: src/Tasks.UnitTests/ResourceHandling/GenerateResource_Tests.cs

🔴 MAJOR: `ChangeWaves.md` missing Wave18_8 entry (ChangeWave Discipline, dim 2)

documentation/wiki/ChangeWaves.md's ### 18.8 section does not document the new behavior. Users whose builds newly fail with MSB3821 on a typed .resx entry have no discoverable path to the MSBUILDDISABLEFEATURESFROMVERSION=18.8 opt-out.

Recommended addition under ### 18.8:

- [GenerateResource: typed ResX data entries (including ResXFileRef) in Mark-of-the-Web files are now blocked (MSB3821); unblock the file or set MSBUILDDISABLEFEATURESFROMVERSION=18.8 to restore prior behavior](https://github.com/dotnet/msbuild/pull/14015)

🟡 MODERATE: No per-trigger diagnostic (Logging & Diagnostics, dim 6)

All three IsDangerous() triggers — mimetype, ResXFileRef, wave-gated typed entry — emit the same generic MSB3821 message. For the Wave18_8 path (new default behavior), users won't know which element caused the failure or that MSBUILDDISABLEFEATURESFROMVERSION=18.8 is the opt-out. Consider propagating a trigger reason out of IsDangerous() so the call site can append a supplemental Log.LogMessage naming the ChangeWave and opt-out path for the wave-gated case.

💬 NIT: Single-letter variable name `s` (Naming, dim 14)

string s = reader.LocalName; (line 987) — consider string localName = reader.LocalName; to make the comparisons self-documenting at each use site.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

patchdiff.githubusercontent.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "patchdiff.githubusercontent.com"

See Network Configuration for more information.

Generated by Expert Code Review (command) for issue #14015 · 2.8K AIC · ⊞ 29.8K ambient context · ◷
_{Comment /review to run again}

github-actions

Expert Review — 24-Dimension Analysis

#	Dimension	Verdict
1	Backwards Compatibility Vigilance	🔴 1 BLOCKING
2	ChangeWave Discipline	🟠 1 MAJOR
4	Test Coverage & Completeness	🟠 1 MAJOR
6	Logging & Diagnostics Rigor	🟡 1 MODERATE
12	Code Simplification	💬 1 NIT
14	Naming Precision	💬 1 NIT

✅ 18/24 dimensions clean.

🔴 BLOCKING — Backwards Compatibility

The ResXFileRef check (line 1004–1006) is unconditional — it has no ChangeWave gate.

This is a hard breaking change: any .resx file downloaded from the internet (MotW applied) that uses the standard WinForms designer pattern of ResXFileRef for image resources will now fail with MSB3821. The immediately adjacent branch (else if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave18_8))) is correctly gated — the ResXFileRef branch appears to have been overlooked. The AllowMOTW registry key is machine-wide and is not a viable per-project workaround.

Fix: Gate both checks behind the same ChangeWave (see inline comment on line 1006).

🟠 MAJOR — Missing items

Test coverage — Both new behaviors (ResXFileRef detection, Wave18_8 typed-entry blocking) have zero test coverage. Add [WindowsOnlyFact]-gated unit tests to GenerateResource_Tests.cs.
ChangeWaves.md — The Wave18_8 section in documentation/wiki/ChangeWaves.md does not document this behavior. Users cannot discover the MSBUILDDISABLEFEATURESFROMVERSION=18.8 opt-out.

The security intent of this PR is correct and valuable — ResXFileRef in MotW-tagged files is a real risk. The fix just needs to respect the ChangeWave discipline that MSBuild uses for all behavioral changes, and needs test coverage to maintain that security guarantee over time.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

patchdiff.githubusercontent.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "patchdiff.githubusercontent.com"

See Network Configuration for more information.

Generated by Expert Code Review (command) for issue #14015 · 2.8K AIC · ⊞ 29.8K ambient context
_{Comment /review to run again}

…testable helper - Extract the .resx/.resw content danger scan into a static, platform-agnostic ResourceFileContentsAreDangerous(TextReader) helper so it is unit-testable on all TFMs (the surrounding zone/COM check remains full-framework only). - Add ResourceFileTrustTests covering: plain string (not dangerous), mimetype (dangerous), ResXFileRef (dangerous even with Wave18_8 disabled), typed entry with Wave18_8 enabled (dangerous) and disabled (not dangerous). - Document the new MotW/typed-entry behavior and the MSBUILDDISABLEFEATURESFROMVERSION=18.8 opt-out under ChangeWaves.md 18.8.

JanProvaznik

it should be changewave 18.9

Add Wave18_9 to ChangeWaves and gate the GenerateResource MotW typed-entry detection behind it; update tests and ChangeWaves.md (### 18.9 section, MSBUILDDISABLEFEATURESFROMVERSION=18.9 opt-out). ResXFileRef detection remains unconditional.

adressed

JanKrivanek added 3 commits June 10, 2026 11:43

Update the MotW warning to clarify msbuild security guidance for inputs

c684553

Update the guidance

a69b830

Copilot AI review requested due to automatic review settings June 10, 2026 09:47

Copilot started reviewing on behalf of JanKrivanek June 10, 2026 09:47 View session

Copilot AI reviewed Jun 10, 2026

View reviewed changes

Comment thread src/Tasks/GenerateResource.cs Outdated

Comment thread src/Tasks/GenerateResource.cs Outdated

Address PR review: use cached LocalName and break early once dangerous

ab55888

JanKrivanek enabled auto-merge June 10, 2026 11:11

github-actions Bot previously requested changes Jun 15, 2026

View reviewed changes

Comment thread src/Tasks/GenerateResource.cs Outdated

Comment thread src/Tasks/GenerateResource.cs Outdated

Comment thread src/Tasks/GenerateResource.cs Outdated

JanKrivanek and others added 2 commits June 16, 2026 14:44

Merge branch 'main' into dev/jankrivanek/resx-impr

5cfa20c

JanProvaznik requested changes Jun 16, 2026

View reviewed changes

JanProvaznik requested a review from ViktorHofer June 16, 2026 12:47

JanProvaznik approved these changes Jun 16, 2026

View reviewed changes

JanProvaznik disabled auto-merge June 17, 2026 08:29

JanProvaznik merged commit a1930ad into main Jun 17, 2026
14 checks passed

JanProvaznik deleted the dev/jankrivanek/resx-impr branch June 17, 2026 08:30

This was referenced Jun 18, 2026

[release/10.0.4xx] Source code updates from dotnet/msbuild dotnet/dotnet#7262

Merged

[main] Source code updates from dotnet/msbuild dotnet/dotnet#7265

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve MotW handling and clarify the MSB3821#14015

Improve MotW handling and clarify the MSB3821#14015
JanProvaznik merged 7 commits into
mainfrom
dev/jankrivanek/resx-impr

JanKrivanek commented Jun 10, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

JanProvaznik commented Jun 15, 2026

Uh oh!

github-actions Bot commented Jun 15, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jun 15, 2026

Uh oh!

github-actions Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JanProvaznik left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

JanKrivanek commented Jun 10, 2026

Context

Related

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Copilot's findings

Uh oh!

Uh oh!

Uh oh!

JanProvaznik commented Jun 15, 2026

Uh oh!

github-actions Bot commented Jun 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jun 15, 2026

Code Review — Design-Level Findings

🔴 MAJOR: No tests for new IsDangerous() behavior (Test Coverage, dim 4)

🔴 MAJOR: ChangeWaves.md missing Wave18_8 entry (ChangeWave Discipline, dim 2)

🟡 MODERATE: No per-trigger diagnostic (Logging & Diagnostics, dim 6)

💬 NIT: Single-letter variable name s (Naming, dim 14)

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Expert Review — 24-Dimension Analysis

🔴 BLOCKING — Backwards Compatibility

🟠 MAJOR — Missing items

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JanProvaznik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Jun 15, 2026 •

edited

Loading

🔴 MAJOR: No tests for new `IsDangerous()` behavior (Test Coverage, dim 4)

🔴 MAJOR: `ChangeWaves.md` missing Wave18_8 entry (ChangeWave Discipline, dim 2)

💬 NIT: Single-letter variable name `s` (Naming, dim 14)