System.Security.Cryptography.Xml Fix NullReferenceException with PropagatedNamespaces

[krwq]

Please see XmlDsigC14NTransformTest.PropagatedNamespaces for repro. Copy:

            XmlDocument doc = new XmlDocument();
            doc.AppendChild(doc.CreateElement("foo", "urn:foo"));
            doc.DocumentElement.AppendChild(doc.CreateElement("bar", "urn:bar"));
            Assert.Equal(String.Empty, doc.DocumentElement.GetAttribute("xmlns:f"));
            XmlDsigExcC14NTransform t = new XmlDsigExcC14NTransform();
            t.LoadInput(doc);
            t.PropagatedNamespaces.Add("f", "urn:foo");
            t.PropagatedNamespaces.Add("b", "urn:bar");
            using (Stream s = t.GetOutput() as Stream)
            using (StreamReader streamReader = new StreamReader(s, Encoding.UTF8))
            {
                string result = streamReader.ReadToEnd();
                Assert.Equal(result,
                    "<foo xmlns=\"urn:foo\"><bar xmlns=\"urn:bar\"></bar></foo>");
                Assert.Equal("urn:foo", doc.DocumentElement.NamespaceURI);
            }

We need to figure if @anthonylangsworth fix is the correct one and port the fix to netfx.
original fix: krwq/corefx@55f4b87

As part of this issue we need to ensure that the test is correctly interacting with PropagatedNamespaces (please see full commit with change including the changes made to the test)

[karelz]

@krwq why was it moved from 2.0?

[krwq]

@karelz - this is not too common scenario considering this bug existed on Desktop for some time and I haven't seen reports on it. We currently want to focus on having all Desktop scenarios working and if there is enough time we can take any additional fixes. We should likely re-triage after we finish

[karelz]

Sounds good. BTW: I wasn't pushing back, just wanted to understand the 'why' :)

[krwq]

Moving it back to 2.0.0 milestone. @tintoy @anthonylangsworth - I think we will need to make this fix slightly faster. I've noticed that http://www.w3.org/2002/07/decrypt#XML is basically not working because of this. (Algorithm description: https://www.w3.org/TR/xmlenc-decrypt#sec-xml-processing). I've tried adding a tests for the class but as I started writing it I've noticed that we pretty much always throw NRE when calling GetOutput on the transform

[anthonylangsworth]

What is the goal, @krwq ? If we cannot change the implementation, is the goal to create workarounds with what remains?

[krwq]

@anthonylangsworth we can change implementation, the only difference when changing implementation is that we need to make those changes in separate PRs and file issues for them so that we can track porting to desktop and so that the change is really easy to review during triage. I think if you've opened PR with 2 lines change which would enable your test and fix we will be able to take it, @danmosemsft - is that correct or am I too confused?

[danmoseley]

If it's a code bug -- we are not porting fixes to Desktop at the moment unless they are quite serious problems. At some future point we'll presumably go through issues labeled "netfx-port-consider" like this but it's not happening right now. So I wouldn't go to any particular effort to open lots of such issues (or any necessarily). We have made hundreds of fixes in Core over the last 2 years that would be port candidates and we don't attempt to track them. I would fix any bug in the Core copy, if it's not a prohibited breaking change of course.

If it's a test failure against desktop -- we do want our tests to pass against Desktop -- that helps prove that we are compatible (it is to test core not desktop that we run tests in this way). We either tweak the test so it expects different behavior on desktop, or we break out a new test just for desktop, or we disable the test for desktop whichever seems best.

Does this help? Not sure of the exact question.

[krwq]

@danmosemsft thank you, that's exactly what I wanted to know! @anthonylangsworth tl;dr; we can make a fix as long as we disable the test on net45. Fixing NullReferenceException was never considered breaking

[anthonylangsworth]

@krwq @danmosemsft Thanks. I'll fix it over the weekend.

[anthonylangsworth]

@krwq This should be closed as the fix was merged.

[karelz]

Fixed in dotnet/corefx#17020

Thanks @anthonylangsworth for reminder!

[danmoseley]

netfx tests re-enabled now this is in