DoNotUseInsecureDTDProcessing (CA3075) disappears on the wrong fix

[bkoelman]

Analyzer package

Desktop.Analyzers

Analyzer

CA3075: DoNotUseInsecureDTDProcessing

Repro steps

Next code fragment reports CA3075, which seems appropriate.

        public static NetworkConfigurationFile Load(string path)
        {
            using (XmlReader reader = XmlReader.Create(path, new XmlReaderSettings { CloseInput = true }))
            {
                throw new NotImplementedException();
            }
        }

But when extracting local variable, it is no longer reported. This contradicts the description at https://msdn.microsoft.com/en-us/library/mt661872.aspx, where locals are used in some examples.

        public static NetworkConfigurationFile Load(string path)
        {
            var settings = new XmlReaderSettings { CloseInput = true };
            using (XmlReader reader = XmlReader.Create(path, settings))
            {
                throw new NotImplementedException();
            }
        }

Fixing the original code like this:

        public static NetworkConfigurationFile Load(string path)
        {
            using (XmlReader reader = XmlReader.Create(path, new XmlReaderSettings { CloseInput = true, DtdProcessing = DtdProcessing.Prohibit }))
            {
                throw new NotImplementedException();
            }
        }

does not make the error go away. I'm not sure if something else is needed or the analysis is wrong.

Expected behavior

CA3075 to be reported after extract local; CA3075 to go away after adding DtdProcessing = DtdProcessing.Prohibit.

Actual behavior

Extract local makes the warning go away, while semantically the behavior has not changed. Also it is unclear to me what the correct fix is to make the warning go away.

[mavasani]

@ivanbasov Can you please check if this repros after https://github.com/dotnet/roslyn-analyzers/pull/1327?

[jinujoseph]

@ivanbasov , pls reverify this

[ivanbasov]

Verified that after #1327, the correct fix works. This code does not provide any warning:

        public static NetworkConfigurationFile Load(string path)
        {
            using (XmlReader reader = XmlReader.Create(path, new XmlReaderSettings { CloseInput = true, DtdProcessing = DtdProcessing.Prohibit }))
            {
                throw new NotImplementedException();
            }
        }

However, the incorrect fix still does not provide a warning:

      public static NetworkConfigurationFile Load(string path)
        {
            var settings = new XmlReaderSettings { CloseInput = true };
            using (XmlReader reader = XmlReader.Create(path, settings))
            {
                throw new NotImplementedException();
            }
        }

Expected: Warning Actual: None

MSDN example:

        public void TestMethod(string path)   
        {   
            XmlReaderSettings settings = new XmlReaderSettings(){ DtdProcessing = DtdProcessing.Parse };   
            XmlReader reader = XmlReader.Create(path, settings); // warn   
        }   

also provides no warnings

[mavasani]

@ivanbasov The MSDN example and your sample code does not provide any warning due to lack of dataflow analysis to identify these cases. Let's move this out to a future milestone when we have some flow analysis support.