build: Enable tsec checks for critical packages. #43108

uraj · 2021-08-10T23:18:28Z

uraj · 2021-08-10T23:23:08Z

@josephperrott This is the PR that enables TT static analysis for the core package. Could you take a look?

google-cla · 2021-08-11T14:43:24Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

IgorMinar

Thanks @uraj for sending us this PR! I'm supportive of adding tsec to our repo.

Overall I like where this is heading, I left a few comments primarily asking for clarification on your intention here.

If we could change the PR to run these tests via bazel, then that would be better, but I can't easily estimate how much more work that would be. @josephperrott might have a better understanding. Maybe running it via the new https://bazelbuild.github.io/rules_nodejs/ that autogenerate bazel rules for npm binaries (see the babel example in that link) would make it trivial if we can collect all the right inputs easily.

Please take a look at the other comments. Thank you again.

IgorMinar · 2021-08-11T18:03:02Z

packages/zone.js/lib/tsconfig-tsec.json

+    "outDir": ".tsec",
+    "types": ["node"]
+  },
+  "files": ["zone.ts"]


is it right that you included just the zone.ts, shouldn't files under browser/* and others be included as well?

The original plan is to only check core, which only depends zone.d.ts but not anything in browser/* (in terms of type checking). Do we actually want to check browser/*? If so, I can include it as one of the to-be-checked packages.

packages/tsconfig-build.json

packages/tsconfig-tsec.json

IgorMinar · 2021-08-11T18:03:02Z

packages/core/tsconfig-tsec.json

+  "extends": "../tsconfig-tsec-base.json",
+  "compilerOptions": {
+    "composite": true,
+    "outDir": ".tsec"


I'm not too keen on writing these files into the working directory. I see that it's .gitignored but it still feels icky.

how difficult is it to run tsec under bazel?

Tsetse (the checker engine used by tsec) used to be part of @bazel/typescript, but the support was discontinued due to the TS team being understaffed.

I tried to hack this a bit, and it turns out to need quite some work. To effectively manage dependencies with bazel, all tsconfig-tsec.json files need to be generated rather than being hand-written. The TypeScript compiler provided by bazel has a few augmentations to make this easier, but tsec is not that different from the vanilla tsc.

I can discuss with my TL whether we should support using tsec with bazel in the future (I have a prototype that can handle TS libraries with only node_modules dependencies). For now I think we have two options as temporary compromises:

Make tsec place all the generated declaration files at a location outside of the working directory, e.g., /tmp/tsec or ${PROJECT_ROOT}/.tsec.

Stop using project references so that I can turn on noEmit. That way tsec won't generate any files in the working directory, at the cost of loosing incremental compilation. This, however, may not be feasible, due to zone.js/lib producing an global-scoped file zone.d.ts (without any exports), which is tricky to handle in a noEmit configuration.

What do you think?

I was able to do certain amount of bazel integration. There are several caveats though.

tsconfig-tsec.json files are still not auto-generated and have to be maintained manually to make it consistent with BUILD.bazel. The maintenance cost should be low though.

The bazel rule tsec_test does not check transitive dependencies, so each dependency needs to have its own tsec_test. This is hard to fix, unless we can have some upstream patches for the vanilla ts_library bazel rule.

packages/localize/tsconfig-tsec.json

packages/tsec-exemption.json

google-cla · 2021-08-11T19:41:38Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla · 2021-08-11T20:53:06Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla · 2021-08-11T20:56:35Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

uraj · 2021-09-13T17:18:53Z

@devversion I filled google/tsec#23 to track Windows compatibility.

AndrewKushnir · 2021-09-13T19:15:24Z

@josephperrott there is an indication that this PR needs g3 presubmit and it looks like only **/tsconfig-tsec.json files are synced (see presubmit CL). Do we need those files in g3 of should we exclude them from the sync? Adding the "cleanup" label to this PR for now.

uraj · 2021-09-13T19:21:43Z

@AndrewKushnir I think they can be excluded.

AndrewKushnir · 2021-09-13T19:24:38Z

@uraj thanks for the comment. Could you please update the exclude list in https://github.com/angular/angular/blob/master/.github/angular-robot.yml#L42 to reflect that (and also the file in g3 that is mentioned at https://github.com/angular/angular/blob/master/.github/angular-robot.yml#L35)? That should eliminate the need to have a presubmit for this PR.

Those files are for tsec_test only and therefore irrelevant to google3.

uraj · 2021-09-13T19:34:18Z

@AndrewKushnir Updated.

AndrewKushnir · 2021-09-13T19:38:51Z

Thanks for the update @uraj. I've updated g3 status for this PR and removed the "cleanup" label.

AndrewKushnir · 2021-09-13T21:45:17Z

Merge-assistance: it's ok to proceed with the current set of reviews, since its just testing changes around everything and not changing the code in those areas.

Introduce two new bazel rules: tsec_test and tsec_config, for describing the tsec checks and the tsconfig file needed for such checks, respectively. Currently, tsec_test only checks the srcs of a ts_library or ng_module. It does not check direct or transitive dependencies. Also, tsconfig files need to be manually maintained to make sure tsec can read all necessary input (including global symbols). PR Close #43108

This is to replace the implicitly created ts_library_forwared rules and keep changes related to tsec/bazel integration solely in tools/tsec.bzl. PR Close #43108

Upgrade tsec version form 0.1.8 to 0.1.9. The new version has a minor bug fix. PR Close #43108

Those files are for tsec_test only and therefore irrelevant to google3. PR Close #43108

tsec is a static analyzer that discovers Trusted Types violations. Deploy tsec to make sure there will be no TT regression in several critical packages, including core, platform-browser, platform-server and their dependencies. Existing violations have been reviewed and exempted in packages/tsec-exemption.json. Future changes to the exemption list requires security review. PR Close angular#43108

Introduce two new bazel rules: tsec_test and tsec_config, for describing the tsec checks and the tsconfig file needed for such checks, respectively. Currently, tsec_test only checks the srcs of a ts_library or ng_module. It does not check direct or transitive dependencies. Also, tsconfig files need to be manually maintained to make sure tsec can read all necessary input (including global symbols). PR Close angular#43108

) This is to replace the implicitly created ts_library_forwared rules and keep changes related to tsec/bazel integration solely in tools/tsec.bzl. PR Close angular#43108

Upgrade tsec version form 0.1.8 to 0.1.9. The new version has a minor bug fix. PR Close angular#43108

) Those files are for tsec_test only and therefore irrelevant to google3. PR Close angular#43108

tsec is a static analyzer that discovers Trusted Types violations. Deploy tsec to make sure there will be no TT regression in several critical packages, including core, platform-browser, platform-server and their dependencies. Existing violations have been reviewed and exempted in packages/tsec-exemption.json. Future changes to the exemption list requires security review. PR Close angular#43108

Introduce two new bazel rules: tsec_test and tsec_config, for describing the tsec checks and the tsconfig file needed for such checks, respectively. Currently, tsec_test only checks the srcs of a ts_library or ng_module. It does not check direct or transitive dependencies. Also, tsconfig files need to be manually maintained to make sure tsec can read all necessary input (including global symbols). PR Close angular#43108

) This is to replace the implicitly created ts_library_forwared rules and keep changes related to tsec/bazel integration solely in tools/tsec.bzl. PR Close angular#43108

Upgrade tsec version form 0.1.8 to 0.1.9. The new version has a minor bug fix. PR Close angular#43108

) Those files are for tsec_test only and therefore irrelevant to google3. PR Close angular#43108

angular-automatic-lock-bot · 2021-10-14T16:10:47Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

google-cla bot added the cla: yes label Aug 10, 2021

pullapprove bot requested review from AndrewKushnir, atscott, filipesilva and JiaLiPassion Aug 10, 2021

uraj marked this pull request as draft Aug 10, 2021

AndrewKushnir requested a review from IgorMinar Aug 10, 2021

google-cla bot added cla: no and removed cla: yes labels Aug 11, 2021

uraj marked this pull request as ready for review Aug 11, 2021

IgorMinar reviewed Aug 11, 2021

View changes

IgorMinar added action: cleanup target: major labels Aug 11, 2021

IgorMinar reviewed Aug 11, 2021

View changes

packages/tsec-exemption.json Show resolved Hide resolved

Loading

pullapprove bot requested a review from alan-agius4 Aug 11, 2021

uraj force-pushed the tsec branch from 7bb9063 to 48fc316 Aug 11, 2021

uraj changed the title ~~feat(core): Enable tsec checks for @angular/core.~~ feat(dev-infra): Enable tsec checks for critical packages. Aug 11, 2021

uraj force-pushed the tsec branch from 48fc316 to d6c031e Aug 11, 2021

google-cla bot added cla: yes and removed cla: no labels Aug 11, 2021

uraj changed the title ~~feat(dev-infra): Enable tsec checks for critical packages.~~ feat: Enable tsec checks for critical packages. Aug 12, 2021

uraj force-pushed the tsec branch from d6c031e to fce16fb Aug 12, 2021

uraj changed the title ~~feat: Enable tsec checks for critical packages.~~ build: Enable tsec checks for critical packages. Aug 12, 2021

uraj requested a review from IgorMinar Aug 12, 2021

AndrewKushnir added the comp: security label Aug 12, 2021

AndrewKushnir added action: cleanup action: presubmit labels Sep 13, 2021

AndrewKushnir removed the action: presubmit label Sep 13, 2021

build: Exclude tsconfig-tsec.json files from sync with g3

cdfb069

Those files are for tsec_test only and therefore irrelevant to google3.

AndrewKushnir added action: merge-assistance and removed action: cleanup labels Sep 13, 2021

AndrewKushnir closed this in 509031c Sep 13, 2021

AndrewKushnir added a commit that referenced this issue Sep 13, 2021

build: Upgrade tsec to 0.1.9 (#43108)

006bf65

Upgrade tsec version form 0.1.8 to 0.1.9. The new version has a minor bug fix. PR Close #43108

AndrewKushnir added a commit that referenced this issue Sep 13, 2021

build: Exclude tsconfig-tsec.json files from sync with g3 (#43108)

5ccc9e8

Those files are for tsec_test only and therefore irrelevant to google3. PR Close #43108

TeriGlover added a commit to TeriGlover/angular that referenced this issue Sep 16, 2021

build: Upgrade tsec to 0.1.9 (angular#43108)

2994385

Upgrade tsec version form 0.1.8 to 0.1.9. The new version has a minor bug fix. PR Close angular#43108

TeriGlover added a commit to TeriGlover/angular that referenced this issue Sep 16, 2021

build: Exclude tsconfig-tsec.json files from sync with g3 (angular#43108

b2cbec3

) Those files are for tsec_test only and therefore irrelevant to google3. PR Close angular#43108

TeriGlover added a commit to TeriGlover/angular that referenced this issue Sep 22, 2021

build: Upgrade tsec to 0.1.9 (angular#43108)

780b619

Upgrade tsec version form 0.1.8 to 0.1.9. The new version has a minor bug fix. PR Close angular#43108

TeriGlover added a commit to TeriGlover/angular that referenced this issue Sep 22, 2021

build: Exclude tsconfig-tsec.json files from sync with g3 (angular#43108

b5e25c0

) Those files are for tsec_test only and therefore irrelevant to google3. PR Close angular#43108

angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 14, 2021

Oct	NOV	Dec
	12
2020	2021	2022

angular / angular Public

build: Enable tsec checks for critical packages. #43108

build: Enable tsec checks for critical packages. #43108

uraj commented Aug 10, 2021 •

edited

Loading

uraj commented Aug 10, 2021

google-cla bot commented Aug 11, 2021

IgorMinar left a comment

IgorMinar Aug 11, 2021

uraj Aug 11, 2021 •

edited

Loading

IgorMinar Aug 11, 2021

uraj Aug 11, 2021 •

edited

Loading

uraj Aug 13, 2021

google-cla bot commented Aug 11, 2021

google-cla bot commented Aug 11, 2021

google-cla bot commented Aug 11, 2021

uraj commented Sep 13, 2021

AndrewKushnir commented Sep 13, 2021

uraj commented Sep 13, 2021

AndrewKushnir commented Sep 13, 2021

uraj commented Sep 13, 2021

AndrewKushnir commented Sep 13, 2021

AndrewKushnir commented Sep 13, 2021

angular-automatic-lock-bot bot commented Oct 14, 2021

angular / angular Public

build: Enable tsec checks for critical packages. #43108

build: Enable tsec checks for critical packages. #43108

Conversation

uraj commented Aug 10, 2021 • edited Loading

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

uraj commented Aug 10, 2021

google-cla bot commented Aug 11, 2021

IgorMinar left a comment

IgorMinar Aug 11, 2021

Choose a reason for hiding this comment

uraj Aug 11, 2021 • edited Loading

Choose a reason for hiding this comment

IgorMinar Aug 11, 2021

Choose a reason for hiding this comment

uraj Aug 11, 2021 • edited Loading

Choose a reason for hiding this comment

uraj Aug 13, 2021

Choose a reason for hiding this comment

google-cla bot commented Aug 11, 2021

google-cla bot commented Aug 11, 2021

google-cla bot commented Aug 11, 2021

uraj commented Sep 13, 2021

AndrewKushnir commented Sep 13, 2021

uraj commented Sep 13, 2021

AndrewKushnir commented Sep 13, 2021

uraj commented Sep 13, 2021

AndrewKushnir commented Sep 13, 2021

AndrewKushnir commented Sep 13, 2021

angular-automatic-lock-bot bot commented Oct 14, 2021

uraj commented Aug 10, 2021 •

edited

Loading

uraj Aug 11, 2021 •

edited

Loading

uraj Aug 11, 2021 •

edited

Loading