🛡️ Sentinel: [MEDIUM] Fix Scope Leakage via Incomplete Accessibility Check#52
🛡️ Sentinel: [MEDIUM] Fix Scope Leakage via Incomplete Accessibility Check#52timonkrebs wants to merge 1 commit into
Conversation
The source generator's `IsEffectivelyPublic` logic failed to deeply inspect compound types such as arrays, pointers, and generic type arguments. Consequently, a public generic type parameterized with an internal type (e.g. `PublicGeneric<InternalClass>`) was incorrectly considered fully public. This could lead to scope leakage by emitting `public` extension classes for types that logically should be treated as `internal`, resulting in CS0050/CS0051 visibility errors when compiling generated code. This fix ensures `IsEffectivelyPublic` recursively checks `ElementType` for Arrays, `PointedAtType` for Pointers, and `TypeArguments` for Generic Types to ensure no internal types are inadvertently leaked. 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 hardens the source generator’s accessibility analysis so it doesn’t treat compound types (arrays, pointers, generic type arguments) as effectively public when they reference non-public types, preventing scope leakage that can cause CS0050/CS0051 in generated code.
Changes:
- Updated
IsEffectivelyPublicto treatAccessibility.NotApplicableas acceptable for wrapper/compound types. - Added recursive inspection of array element types, pointer pointed-at types, and generic type arguments.
- Documented the vulnerability/learning/prevention details in
.jules/sentinel.md.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/NTypeForge.SourceGenerator/CandidateAnalyzer.cs | Updates effective-public accessibility computation to recurse into compound types. |
| .jules/sentinel.md | Adds a Sentinel entry describing the scope leakage scenario and the intended prevention approach. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| switch (type.TypeKind) | ||
| { | ||
| case TypeKind.Array: | ||
| return IsEffectivelyPublic(((IArrayTypeSymbol)type).ElementType); | ||
| case TypeKind.Pointer: | ||
| return IsEffectivelyPublic(((IPointerTypeSymbol)type).PointedAtType); | ||
| } |
![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: 68229f6087
ℹ️ 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".
| foreach (var typeArg in namedType.TypeArguments) | ||
| { | ||
| if (!IsEffectivelyPublic(typeArg)) return false; |
There was a problem hiding this comment.
For nested constructed targets such as PublicOuter<InternalType>.Inner, this loop only checks the innermost type's TypeArguments while the loop above checks the containing type's DeclaredAccessibility only. The internal argument on the constructed containing type is therefore never visited, so TargetIsPublic can remain true and ProxyEmitter will still emit a public static class with an internal receiver type, reproducing the CS0050/CS0051 leak this patch is meant to prevent. Recurse through each ContainingType's type arguments as well before returning public.
Useful? React with 👍 / 👎.
Fix IsEffectivelyPublic to properly evaluate compound types
The source generator's
IsEffectivelyPubliclogic failed to deeplyinspect compound types such as arrays, pointers, and generic type
arguments. Consequently, a public generic type parameterized with an
internal type (e.g.
PublicGeneric<InternalClass>) was incorrectlyconsidered fully public. This could lead to scope leakage by emitting
publicextension classes for types that logically should be treated asinternal, resulting in CS0050/CS0051 visibility errors when compilinggenerated code.
This fix ensures
IsEffectivelyPublicrecursively checksElementTypefor Arrays,
PointedAtTypefor Pointers, andTypeArgumentsfor GenericTypes to ensure no internal types are inadvertently leaked.
PR created automatically by Jules for task 12150536405441857384 started by @timonkrebs