6f24cc0632
In some cases, the `rule.adoc` at root of a rule is never included anywhere and thus is dead code. It's a maintenance cost by itself, but also it misses opportunities to inline code that seems used by two documents when in fact only one document is actually rendered. And this missed opportunity, in turn, stops us from applying the correct language tag on the code samples.
87 lines
2.5 KiB
Plaintext
87 lines
2.5 KiB
Plaintext
== Why is this an issue?
|
|
|
|
Mutable collections are those whose state can be changed. For instance, ``++Array++`` and ``++List<T>++`` are mutable, but ``++System.Collections.ObjectModel.ReadOnlyCollection<T>++`` and ``++System.Collections.Immutable.ImmutableList<T>++`` are not. Mutable collection class members should not be returned to a caller or accepted and stored directly. Doing so leaves you vulnerable to unexpected changes in your class state.
|
|
|
|
|
|
Instead use and store a copy of the mutable collection, or return an immutable collection wrapper, e.g. ``++System.Collections.ObjectModel.ReadOnlyCollection<T>++``.
|
|
|
|
|
|
Note that you can't just return your mutable collection through the ``++IEnumerable<T>++`` interface because the caller of your method/property could cast it down to the mutable type and then change it.
|
|
|
|
|
|
This rule checks that mutable collections are not stored or returned directly.
|
|
|
|
=== Noncompliant code example
|
|
|
|
[source,csharp]
|
|
----
|
|
class A
|
|
{
|
|
private List<string> names = new List<string>();
|
|
|
|
public ICollection<string> Names => names; // Noncompliant
|
|
|
|
public IEnumerable<string> GetNames() // Noncompliant
|
|
{
|
|
return names;
|
|
}
|
|
|
|
public void SetNames(List<string> strings)
|
|
{
|
|
this.names = strings; // Noncompliant
|
|
}
|
|
}
|
|
----
|
|
|
|
=== Compliant solution
|
|
|
|
[source,csharp]
|
|
----
|
|
class A
|
|
{
|
|
private List<string> names = new List<string>();
|
|
private ReadOnlyCollection<string> readOnlyNames = new ReadOnlyCollection<string>(names);
|
|
|
|
public ICollection<string> Names => readOnlyNames; // Return a collection wrapper
|
|
|
|
public IEnumerable<string> GetNames()
|
|
{
|
|
names.ToList(); // Make a copy
|
|
}
|
|
|
|
public void SetNames(List<string> strings)
|
|
{
|
|
this.names.Clear();
|
|
this.names.AddRange(strings); // Make a copy
|
|
}
|
|
}
|
|
----
|
|
|
|
== Resources
|
|
|
|
* https://cwe.mitre.org/data/definitions/374[MITRE, CWE-374] - Passing Mutable Objects to an Untrusted Method
|
|
* https://cwe.mitre.org/data/definitions/375[MITRE, CWE-375] - Returning a Mutable Object to an Untrusted Caller
|
|
|
|
|
|
ifdef::env-github,rspecator-view[]
|
|
|
|
'''
|
|
== Implementation Specification
|
|
(visible only on this page)
|
|
|
|
include::../message.adoc[]
|
|
|
|
'''
|
|
== Comments And Links
|
|
(visible only on this page)
|
|
|
|
=== on 15 Mar 2017, 16:13:35 Amaury Levé wrote:
|
|
\[~ann.campbell.2] I changed a bit this rule to focus only on collections for C# as ``++Date++`` is immutable for us.
|
|
|
|
=== on 15 Mar 2017, 18:47:09 Ann Campbell wrote:
|
|
Works for me [~amaury.leve]
|
|
|
|
include::../comments-and-links.adoc[]
|
|
|
|
endif::env-github,rspecator-view[]
|