Skip to content

fix(compiler-cli): disallow template access to private members of base classes #71053

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
JeanMeche wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix(compiler-cli): disallow template access to private members of base classes #71053

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -669,14 +669,15 @@ export class TemplateTypeCheckerImpl implements TemplateTypeChecker {
const fileRecord = this.state.get(sfPath)!;

const typeCheckProgram = this.programDriver.getProgram();
const typeChecker = typeCheckProgram.getTypeChecker();

const diagnostics: (ts.Diagnostic | null)[] = [];
if (fileRecord.hasInlines) {
const inlineSf = getSourceFileOrError(typeCheckProgram, sfPath);
diagnostics.push(
...typeCheckProgram
.getSemanticDiagnostics(inlineSf)
.map((diag) => convertDiagnostic(diag, fileRecord.sourceManager)),
.map((diag) => convertDiagnostic(diag, fileRecord.sourceManager, typeChecker)),
);
}

Expand All @@ -687,7 +688,9 @@ export class TemplateTypeCheckerImpl implements TemplateTypeChecker {
const filteredDiagnostics = this.filterShimDiagnostics(shimSf, semanticDiagnostics);

diagnostics.push(
...filteredDiagnostics.map((diag) => convertDiagnostic(diag, fileRecord.sourceManager)),
...filteredDiagnostics.map((diag) =>
convertDiagnostic(diag, fileRecord.sourceManager, typeChecker),
),
);
diagnostics.push(...shimRecord.genesisDiagnostics);

Expand Down Expand Up @@ -757,14 +760,15 @@ export class TemplateTypeCheckerImpl implements TemplateTypeChecker {
const shimRecord = fileRecord.shimData.get(shimPath)!;

const typeCheckProgram = this.programDriver.getProgram();
const typeChecker = typeCheckProgram.getTypeChecker();

const diagnostics: (TemplateDiagnostic | null)[] = [];
if (shimRecord.hasInlines) {
const inlineSf = getSourceFileOrError(typeCheckProgram, sfPath);
diagnostics.push(
...typeCheckProgram
.getSemanticDiagnostics(inlineSf)
.map((diag) => convertDiagnostic(diag, fileRecord.sourceManager)),
.map((diag) => convertDiagnostic(diag, fileRecord.sourceManager, typeChecker)),
);
}

Expand All @@ -773,7 +777,9 @@ export class TemplateTypeCheckerImpl implements TemplateTypeChecker {
const filteredDiagnostics = this.filterShimDiagnostics(shimSf, semanticDiagnostics);

diagnostics.push(
...filteredDiagnostics.map((diag) => convertDiagnostic(diag, fileRecord.sourceManager)),
...filteredDiagnostics.map((diag) =>
convertDiagnostic(diag, fileRecord.sourceManager, typeChecker),
),
);
diagnostics.push(...shimRecord.genesisDiagnostics);

Expand Down Expand Up @@ -1784,8 +1790,9 @@ export class TemplateTypeCheckerImpl implements TemplateTypeChecker {
function convertDiagnostic(
diag: ts.Diagnostic,
sourceResolver: TypeCheckSourceResolver,
typeChecker: ts.TypeChecker,
): TemplateDiagnostic | null {
if (!shouldReportDiagnostic(diag)) {
if (!shouldReportDiagnostic(diag, typeChecker)) {
return null;
}
return translateDiagnostic(diag, sourceResolver);
Expand Down Expand Up @@ -2004,9 +2011,10 @@ function getDeprecatedSuggestionDiagnostics(
return [];
}

const typeChecker = program.getTypeChecker();
const tsDiags = tsLs.getSuggestionDiagnostics(path).filter(isDeprecatedDiagnostics);
const commonTemplateDiags = tsDiags.map((diag) => {
return convertDiagnostic(diag, fileRecord.sourceManager);
return convertDiagnostic(diag, fileRecord.sourceManager, typeChecker);
});

const elementTagDiags = getTheElementTagDeprecatedSuggestionDiagnostics(
Expand Down
66 changes: 58 additions & 8 deletions packages/compiler-cli/src/ngtsc/typecheck/src/diagnostics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,26 +7,75 @@
*/
import ts from 'typescript';

import {getTokenAtPosition} from '../../util/src/typescript';
import {TemplateDiagnostic} from '../api';
import {makeTemplateDiagnostic} from '../diagnostics';

import {getSourceMapping, TypeCheckSourceResolver} from './tcb_util';

/**
* This function will the check the source text of the TCB instead of doing a more expensive AST traversal (via getTokenAtPosition)
* Determines whether a TS2341 ("Property 'x' is private and only accessible within class 'Y'")
* diagnostic occurred on a property access on the component/directive's `this` context for a
* private member declared on that component/directive class itself (as opposed to an inherited
* private member from a base class or a private member on a narrowed subtype).
*/
function isAccessOnThis(text: string, start: number): boolean {
return (
text.substring(start - 5, start) === 'this.' || text.substring(start - 7, start) === '(this).'
);
function isPrivatePropertyOnHostThis(
sf: ts.SourceFile,
start: number,
typeChecker: ts.TypeChecker,
): boolean {
const node = getTokenAtPosition(sf, start);
if (!ts.isPropertyAccessExpression(node.parent) || node.parent.name !== node) {
return false;
}

let receiver: ts.Expression = node.parent.expression;
while (ts.isParenthesizedExpression(receiver) || ts.isNonNullExpression(receiver)) {
receiver = receiver.expression;
}
if (receiver.kind !== ts.SyntaxKind.ThisKeyword) {
return false;
}

const thisSymbol = typeChecker.getSymbolAtLocation(receiver);
if (thisSymbol === undefined) {
return false;
}
const hostDeclarations: readonly ts.Node[] | undefined = typeChecker
.getTypeOfSymbol(thisSymbol)
.getSymbol()?.declarations;
if (hostDeclarations === undefined || hostDeclarations.length === 0) {
return false;
}

const propSymbol = typeChecker.getSymbolAtLocation(node);
if (propSymbol?.declarations === undefined || propSymbol.declarations.length === 0) {
return false;
}

let hasPrivateDeclaration = false;
for (const decl of propSymbol.declarations) {
if ((ts.getCombinedModifierFlags(decl) & ts.ModifierFlags.Private) !== 0) {
hasPrivateDeclaration = true;
const declaringClass = ts.isParameter(decl) ? decl.parent?.parent : decl.parent;
if (declaringClass === undefined || !hostDeclarations.includes(declaringClass)) {
return false;
}
}
}

return hasPrivateDeclaration;
}

/**
* Determines if the diagnostic should be reported. Some diagnostics are produced because of the
* way TCBs are generated; those diagnostics should not be reported as type check errors of the
* template.
*/
export function shouldReportDiagnostic(diagnostic: ts.Diagnostic): boolean {
export function shouldReportDiagnostic(
diagnostic: ts.Diagnostic,
typeChecker: ts.TypeChecker,
): boolean {
const {code} = diagnostic;
if (code === 6133 /* $var is declared but its value is never read. */) {
return false;
Expand All @@ -38,8 +87,9 @@ export function shouldReportDiagnostic(diagnostic: ts.Diagnostic): boolean {
return false;
} else if (code === 2341 /* Property 'X' is private and only accessible within class */) {
if (diagnostic.file !== undefined && diagnostic.start !== undefined) {
// Here we're discarding private property reads error to allow them to be used in template expressions
if (isAccessOnThis(diagnostic.file.text, diagnostic.start)) {
// Discard private property access errors when accessing a private member declared on the
// component/directive class itself via `this` in a template or host binding expression.
if (isPrivatePropertyOnHostThis(diagnostic.file, diagnostic.start, typeChecker)) {
return false;
}
}
Expand Down
215 changes: 215 additions & 0 deletions packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -778,6 +778,221 @@ class TestComponent {
`TestComponent.html(1, 14): Property 'a' is private and only accessible within class 'Model'.`,
]);
});

it('disallows access to private members of a parent class while allowing own private and parent protected members', () => {
const messages = diagnose(
`<button (click)="parentMethod(); childMethod(); parentSetter = 'a'">{{ parentProp }} {{ parentParam }} {{ parentGetter }} {{ childProp }} {{ childParam }} {{ parentProtected }}</button>`,
`
export class Parent {
private parentProp = 'parent';
private get parentGetter(): string { return 'getter'; }
private set parentSetter(v: string) {}
protected parentProtected = 'protected';
constructor(private parentParam: string) {}
private parentMethod(): void {}
}

export class TestComponent extends Parent {
private childProp = 'child';
constructor(private childParam: string) {
super(childParam);
}
private childMethod(): void {}
}
`,
);

expect(messages).toEqual([
`TestComponent.html(1, 18): Property 'parentMethod' is private and only accessible within class 'Parent'.`,
`TestComponent.html(1, 49): Property 'parentSetter' is private and only accessible within class 'Parent'.`,
`TestComponent.html(1, 72): Property 'parentProp' is private and only accessible within class 'Parent'.`,
`TestComponent.html(1, 89): Property 'parentParam' is private and only accessible within class 'Parent'.`,
`TestComponent.html(1, 107): Property 'parentGetter' is private and only accessible within class 'Parent'.`,
]);
});

it('disallows access to private members across multi-level inheritance', () => {
const messages = diagnose(
`{{ grandParentPrivate }} {{ parentPrivate }} {{ childPrivate }} {{ grandParentProtected }}`,
`
export class GrandParent {
private grandParentPrivate = 1;
protected grandParentProtected = 2;
}

export class Parent extends GrandParent {
private parentPrivate = 3;
}

export class TestComponent extends Parent {
private childPrivate = 4;
}
`,
);

expect(messages).toEqual([
`TestComponent.html(1, 4): Property 'grandParentPrivate' is private and only accessible within class 'GrandParent'.`,
`TestComponent.html(1, 29): Property 'parentPrivate' is private and only accessible within class 'Parent'.`,
]);
});

it('handles private members on generic parent and generic child classes', () => {
const messages = diagnose(
`{{ parentPrivate }} {{ childPrivate }} {{ parentProtected }}`,
`
export class Parent<T> {
private parentPrivate!: T;
protected parentProtected!: T;
}

export class TestComponent<T> extends Parent<T> {
private childPrivate!: T;
}
`,
);

expect(messages).toEqual([
`TestComponent.html(1, 4): Property 'parentPrivate' is private and only accessible within class 'Parent<T>'.`,
]);
});

it('disallows access to private members from a mixin base class', () => {
const messages = diagnose(
`{{ basePrivate }} {{ mixinPrivate }} {{ ownPrivate }}`,
`
type Constructor<T = {}> = new (...args: any[]) => T;

export class Base {
private basePrivate = 'base';
}

function WithMixin<TBase extends Constructor>(Ctor: TBase) {
return class MixinClass extends Ctor {
private mixinPrivate = 'mixin';
};
}

export class TestComponent extends WithMixin(Base) {
private ownPrivate = 'own';
}
`,
);

expect(messages).toEqual([
`TestComponent.html(1, 4): Property 'basePrivate' is private and only accessible within class 'Base'.`,
`TestComponent.html(1, 22): Property 'mixinPrivate' is private and only accessible within class 'MixinClass'.`,
]);
});

it('handles getter and setter with divergent visibility on parent and child classes', () => {
const messages = diagnose(
`<button (click)="parentProp = 'a'; ownProp = 'b'">{{ parentProp }} {{ ownProp }}</button>`,
`
export class Parent {
get parentProp(): string { return ''; }
private set parentProp(v: string) {}
}

export class TestComponent extends Parent {
get ownProp(): string { return ''; }
private set ownProp(v: string) {}
}
`,
);

expect(messages).toEqual([
`TestComponent.html(1, 18): Property 'parentProp' is private and only accessible within class 'Parent'.`,
]);
});

it('handles optional chaining, non-null assertion, and parentheses on explicit this for private members', () => {
const messages = diagnose(
`{{ this.own }} {{ this?.own }} {{ this!.own }} {{ (this).own }} {{ this.parent }} {{ this?.parent }} {{ this!.parent }} {{ (this).parent }}`,
`
export class Parent {
private parent = 'parent';
}

export class TestComponent extends Parent {
private own = 'own';
}
`,
);

expect(messages).toEqual([
`TestComponent.html(1, 73): Property 'parent' is private and only accessible within class 'Parent'.`,
`TestComponent.html(1, 92): Property 'parent' is private and only accessible within class 'Parent'.`,
`TestComponent.html(1, 111): Property 'parent' is private and only accessible within class 'Parent'.`,
`TestComponent.html(1, 131): Property 'parent' is private and only accessible within class 'Parent'.`,
]);
});

it('handles type narrowing of this with private members', () => {
const messages = diagnose(
`@if (isSub()) { {{ subPrivate }} {{ ownPrivate }} } @if (hasExtra()) { {{ extra }} {{ ownPrivate }} } @if (ownNullable !== null) { {{ ownNullable.value }} }`,
`
export class TestComponent {
private ownPrivate = 'own';
private ownNullable: {value: string} | null = null;

isSub(): this is SubComponent {
return true;
}

hasExtra(): this is { extra: string } {
return true;
}
}

export class SubComponent extends TestComponent {
private subPrivate = 'sub';
}
`,
);

expect(messages).toEqual([
`TestComponent.html(1, 20): Property 'subPrivate' is private and only accessible within class 'SubComponent'.`,
]);
});

it('allows access to own private members when component class merges with an interface', () => {
const messages = diagnose(
`{{ ownPrivate }} {{ parentPrivate }}`,
`
export class Parent {
private parentPrivate = 'parent';
}

export interface TestComponent {
extraProp: string;
}

export class TestComponent extends Parent {
private ownPrivate = 'own';
}
`,
);

expect(messages).toEqual([
`TestComponent.html(1, 21): Property 'parentPrivate' is private and only accessible within class 'Parent'.`,
]);
});

it('disallows access to private members on another instance of the same component class', () => {
const messages = diagnose(
`{{ other.secret }}`,
`
export class TestComponent {
private secret = 'shh';
other!: TestComponent;
}
`,
);

expect(messages).toEqual([
`TestComponent.html(1, 10): Property 'secret' is private and only accessible within class 'TestComponent'.`,
]);
});
});

describe('method call spans', () => {
Expand Down
Loading
Loading