Throw CryptographicException instead of NullReferenceException when URI attribute is missing in EncryptedXml

[Copilot]

Description

Utils.ExtractIdFromLocalUri dereferences its nullable uri parameter with uri!.Substring(1) without a null guard. When EncryptedXml.DecryptDocument() processes a KeyInfo/RetrievalMethod element that omits the URI attribute, this surfaces as a NullReferenceException — an exception type callers of a cryptography API should never need to handle.

Changes

• Utils.cs: Added null/empty check at the top of ExtractIdFromLocalUri that throws CryptographicException with SR.Cryptography_Xml_UriRequired, consistent with the validation in CipherReference.LoadXml and XmlDecryptionTransform.LoadInnerXml.
• EncryptedXmlTests.cs: Regression test exercising DecryptDocument with a <RetrievalMethod/> element missing its URI attribute.

Reproduction

var doc = new XmlDocument { XmlResolver = null };
doc.LoadXml("<root><EncryptedData xmlns='http://www.w3.org/2001/04/xmlenc#'>" +
    "<KeyInfo xmlns='http://www.w3.org/2000/09/xmldsig#'><RetrievalMethod/></KeyInfo>" +
    "<CipherData><CipherValue>AA==</CipherValue></CipherData></EncryptedData></root>");
new EncryptedXml(doc).DecryptDocument(); // Before: NullReferenceException, After: CryptographicException

[dotnet-policy-service]

Tagging subscribers to this area: @bartonjs, @vcsjones, @dotnet/area-system-security
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

This PR updates XML encryption/decryption code paths to throw a CryptographicException (instead of surfacing a NullReferenceException) when a <RetrievalMethod> element omits the required URI attribute, and adds a regression test to cover the scenario.

Changes:

• Add null/empty guard in Utils.ExtractIdFromLocalUri to throw CryptographicException with SR.Cryptography_Xml_UriRequired.
• Add a regression test validating EncryptedXml.DecryptDocument() throws CryptographicException when <RetrievalMethod/> has no URI.

Show a summary per file

File	Description
src/libraries/System.Security.Cryptography.Xml/src/System/Security/Cryptography/Xml/Utils.cs	Adds early validation to avoid null dereference and throw a crypto-appropriate exception.
src/libraries/System.Security.Cryptography.Xml/tests/EncryptedXmlTests.cs	Adds a regression test exercising the missing-URI RetrievalMethod case.

Copilot's findings

Comments suppressed due to low confidence (1)

src/libraries/System.Security.Cryptography.Xml/src/System/Security/Cryptography/Xml/Utils.cs:334

• The exception message resource SR.Cryptography_Xml_UriRequired says "A Uri attribute is required for a CipherReference element." After this change it can surface from other contexts (e.g., KeyInfo/RetrievalMethod processing via ExtractIdFromLocalUri), which is misleading. Consider switching this helper to a more generic SR (or introducing a generic one) so callers get an accurate error message when a non-CipherReference URI is missing/invalid.

            if (string.IsNullOrEmpty(uri))
                throw new CryptographicException(SR.Cryptography_Xml_UriRequired);

• Files reviewed: 2/2 changed files
• Comments generated: 1

[Copilot]

Copilot's findings

• Files reviewed: 2/2 changed files
• Comments generated: 1

[github-actions]

🤖 Copilot Code Review — PR #127093

Holistic Assessment

Motivation: The PR addresses a real, reproducible bug where a missing URI attribute on a <RetrievalMethod> element causes NullReferenceException during DecryptDocument(). Converting this to a descriptive CryptographicException is clearly the right thing to do.

Approach: The fix is placed in the centralized Utils.ExtractIdFromLocalUri method, which is the right layer. All 6 call sites benefit from the validation, and the error string (SR.Cryptography_Xml_UriRequired) is already used in analogous contexts (CipherReference.LoadXml, XmlDecryptionTransform.LoadInnerXml).

Summary: ✅ LGTM. This is a clean, minimal fix that converts an uninformative NullReferenceException into a proper CryptographicException. The fix is correctly centralized, the test exercises the exact path that was broken, and the change is consistent with existing patterns. A few optional suggestions below.

---

Detailed Findings

✅ Correctness — Fix is well-placed and correct

The string.IsNullOrEmpty(uri) guard covers both the null case (the reported bug) and the empty-string case (which would produce an ArgumentOutOfRangeException from Substring(1)). After the check, the null-forgiving ! on uri is no longer needed and the compiler correctly narrows the type. Verified all 6 callers:

• XmlDecryptionTransform.cs:107, Reference.cs:281, SignedXml.cs:823, EncryptedXml.cs:236 — these already guard against null/empty before calling.
• EncryptedXml.cs:373 and :478 — these pass kiRetrievalMethod.Uri! with no prior null check; the new validation catches these paths.

✅ Test — Regression test exercises the right path

The test constructs a <RetrievalMethod/> element without a URI attribute, which causes KeyInfoRetrievalMethod.Uri to be null (via Utils.GetAttribute returning null). This exercises the GetDecryptionKey call at EncryptedXml.cs:373, confirming the fix works end-to-end. The [SkipOnTargetFramework(NetFramework)] attribute is appropriate since .NET Framework doesn't have this fix.

💡 Suggestion — Remove null-forgiving operators at callers (follow-up)

EncryptedXml.cs lines 373 and 478 still use kiRetrievalMethod.Uri!. Since ExtractIdFromLocalUri now accepts string? and validates internally, the ! operators are unnecessary and slightly misleading (they assert "this isn't null" when it may be). Consider removing them so nullable flow analysis is accurate:

// Line 373 and 478: change from
string idref = Utils.ExtractIdFromLocalUri(kiRetrievalMethod.Uri!);
// to
string idref = Utils.ExtractIdFromLocalUri(kiRetrievalMethod.Uri);

This is a cleanliness improvement, not a correctness issue.

💡 Suggestion — Error message mentions "CipherReference" specifically (pre-existing)

The SR.Cryptography_Xml_UriRequired message reads "A Uri attribute is required for a CipherReference element." This is slightly misleading when the error originates from a RetrievalMethod context. However, this string was already reused in XmlDecryptionTransform and EncryptedReference (non-CipherReference contexts), so this is a pre-existing concern, not something introduced by this PR. A follow-up could generalize the message.

---

Models contributing to this review: Claude Opus 4.6 (primary), GPT-5.3-Codex (sub-agent). Goldeneye sub-agent timed out and did not contribute.

Generated by Code Review for issue #127093 · ◷