Fix scope leak: Generate internal extensions for non-public methods and types#48
Fix scope leak: Generate internal extensions for non-public methods and types#48timonkrebs wants to merge 1 commit into
Conversation
…ation Co-authored-by: timonkrebs <11026852+timonkrebs@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Pull request overview
This PR tightens the source generator’s accessibility decisions to prevent leaking non-public symbols into generated public duck-typing extension wrappers (avoiding CS0050/CS0051 and reducing unintended API surface).
Changes:
- Expands accessibility evaluation (
IsEffectivelyPublic) to inspect arrays/pointers, generic type arguments, and method signatures (parameters/return types). - Updates
CandidateAnalyzersotargetIsPublicis only true when the target, original method, and all ducked types are effectively public; otherwise extensions fall back tointernal. - Adds new unit tests covering scenarios where internal members/types should force internal extension wrappers.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/NTypeForge.Generator.Tests/AccessibilityLeakTests.cs | Adds tests intended to ensure internal members/types produce internal extension wrappers. |
| src/NTypeForge.SourceGenerator/CandidateAnalyzer.cs | Deepens “effectively public” checks and uses them to decide whether to emit public vs internal extension classes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| switch (symbol) | ||
| { | ||
| case IArrayTypeSymbol array: | ||
| return IsEffectivelyPublic(array.ElementType); | ||
| case IPointerTypeSymbol pointer: | ||
| return IsEffectivelyPublic(pointer.PointedAtType); | ||
| case INamedTypeSymbol named: | ||
| if (!named.TypeArguments.All(IsEffectivelyPublic)) return false; | ||
| return IsContainingTypePublic(named); | ||
| case IMethodSymbol method: | ||
| if (method.DeclaredAccessibility != Accessibility.Public) return false; | ||
| if (!IsEffectivelyPublic(method.ReturnType)) return false; | ||
| if (!method.Parameters.All(p => IsEffectivelyPublic(p.Type))) return false; | ||
| if (!method.TypeArguments.All(IsEffectivelyPublic)) return false; | ||
| return IsContainingTypePublic(method); | ||
| default: | ||
| return IsContainingTypePublic(symbol); | ||
| } |
| var text = GeneratorTestHarness.GetGeneratedText(source); | ||
| Assert.Contains("internal static class", text); |
| var text = GeneratorTestHarness.GetGeneratedText(source); | ||
| Assert.Contains("internal static class", text); |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5fd0b433e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| targetIsPublic: IsEffectivelyPublic(target) | ||
| && duckedArgs.All(a => IsEffectivelyPublic(a.ArgType) && IsEffectivelyPublic(a.UnderlyingType) && IsEffectivelyPublic(a.InterfaceType)) | ||
| && (originalMethod == null || IsEffectivelyPublic(originalMethod)), |
There was a problem hiding this comment.
When the same public target has both a public-safe duck site and a non-public site, this per-candidate flag can still leak the non-public site because ProxyEmitter.EmitExtensions groups by target and chooses the class modifier from only the first item after EmitOrderKey sorting. For example, a public Target.A(IPublic) plus an internal Target.Z(IInternal) sorts A first, emits a public static class, and then the generated public Z(InternalImpl ...) method either exposes the internal method or hits CS0051. The target-level accessibility needs to be the minimum over all generated members for that target, not whichever candidate sorts first.
Useful? React with 👍 / 👎.
| if (!named.TypeArguments.All(IsEffectivelyPublic)) return false; | ||
| return IsContainingTypePublic(named); |
There was a problem hiding this comment.
For a nested type whose enclosing type is constructed with a non-public argument, such as Outer<Internal>.Inner, the non-public argument lives on named.ContainingType, not on Inner's own TypeArguments when Inner has no arity. This can mark the type as effectively public and emit a public extension method with Outer<Internal>.Inner in its signature, still producing inconsistent-accessibility errors. The publicness check needs to recurse through constructed containing types, not only check their declared accessibility.
Useful? React with 👍 / 👎.
| if (!IsEffectivelyPublic(method.ReturnType)) return false; | ||
| if (!method.Parameters.All(p => IsEffectivelyPublic(p.Type))) return false; | ||
| if (!method.TypeArguments.All(IsEffectivelyPublic)) return false; |
There was a problem hiding this comment.
When a public generic method is ducked from generic code, the constructed candidate can have an outer type parameter as its return type, a passthrough parameter type, or a method type argument; falling through to IsContainingTypePublic treats ITypeParameterSymbol as non-public because its declared accessibility is NotApplicable. That downgrades otherwise public forwarding extensions to internal even though the emitted signature declares its own generic type parameter and exposes no internal type, so consumers in another assembly lose the generated public overload for valid public generic APIs.
Useful? React with 👍 / 👎.
🚨 Severity: MEDIUM
💡 Vulnerability: The source generator indiscriminately emitted
public static classextensions for duck typing. If the target type, its generic arguments, the original method's parameters, or the ducked argument itself wereinternalorprivate, this would causeCS0050orCS0051compilation errors by leaking internal scope references into public APIs. It violated the principle of least privilege.🎯 Impact: Projects with internal encapsulation boundaries could fail to compile when using duck typing or inadvertently expose internal architectures into wider scope boundaries through public proxy wrappers.
🔧 Fix: Upgraded
IsEffectivelyPublicto deeply evaluate arrays, generic type arguments, andIMethodSymbolparameters and return types. Updated theCandidateAnalyzerto enforcetargetIsPubliconly if all associated members are fully public. If not, it falls back safely to aninternal static class.✅ Verification: Added tests verifying that internal types produce
internalwrappers and that cognitive complexity thresholds are not violated.PR created automatically by Jules for task 13907236352264035335 started by @timonkrebs