Not sure if this should be in the common library. This opens this api up to read every private string property. When this was designed that would be a huge security issue. (Times have changed, but not sure this api should as well)

@jeffhandley @eiriktsarpalis @ericstj @krwq do you know the history of the affected APIs? a concern is raised by @rhuijben in the comment #111130 (comment).

Not sure if this should be in the common library. This opens this api up to read every private string property. When this was designed that would be a huge security issue. (Times have changed, but not sure this api should as well)

It's not about the property to be public, it's only about the ResourceType, that might be only internal and not public. But since you need to have access to the type that is assigned to the ResourceType, I don't see that it might be a security issue. But I'm also not fully sure about this either.

@uo1 reading the code

GetRuntimeProperty will work only if the property is public property. Implicitly indicating the type containing the property must be public too. Right?

@uo1 reading the code

runtime/src/libraries/System.ComponentModel.Annotations/src/System/ComponentModel/DataAnnotations/LocalizableString.cs

Line 126 in 38366f0

var property = _resourceType.GetRuntimeProperty(_propertyValue);

GetRuntimeProperty will work only if the property is public property. Implicitly indicating the type containing the property must be public too. Right?

@tarekgh I just checked it and you are right, that the property needs to be public, but it does not matter, which access modifier the type has. Even if _resourceType is assigned with a type that is only private or internal the method GetRuntimeProperty will still return a valid PropertyInfo if the property exists and is public.

Even if _resourceType is assigned with a type that is only private or internal the method GetRuntimeProperty will still return a valid PropertyInfo if the property exists and is public.

Yes, I understand that. I was trying to figure out if checking the resource type in the code to be public was done intentionally for specific reason or it was just early check to avoid calling reflection. I don't know the history to tell.

Generally, I do not see a significant issue if we allow using the internal types with the DisplayAttribute. It is kind of breaking change but not major breaking. Also, I am not seeing this is a security issue either. Users can still call reflection with the internal types to get the property value if they want.

I'll not object to accepting this change if others do not have any concerns.

@jeffhandley @eiriktsarpalis @ericstj @krwq @GrabYourPitchforks

@uo1 Why do you keep force-pushing the changes? Please avoid doing that so the reviewers don't have to repeat the review process.

@uo1 Why do you keep force-pushing the changes? Please avoid doing that so the reviewers don't have to repeat the review process.

The pipelines keep failing with different errors that are not related to the changes in this PR. I just rebased the PR a few times to start the pipeline again, but it's still not successful. I will not rebase the PR anymore. Hopefully someone will take care if that problem.

@uo1 that's not necessary to be green - look at the details of Build Analysis — .NET Result Analysis and you only need to review that failures are known. If failures are unrelated to your changes we can still merge the PR. With the traffic we have in this repo it's really hard to keep checks green at all times especially with all configurations on different OSes. If you're changing fairly deterministic things most configurations will fail on any error.

Thanks @uo1 for providing the change!