💢 Perform update to Angular 17

[bkimminich]

In order to stay up to date, Juice Shop needs to get updated to Angular 16. There is an initial attempt available on the angular16 branch, but it fails from some used modules not being compatible with the Ivy compiler. Angular removed the backward compatible compiler which worked with these older modules, and now Ivy is the only compiler available from Angular 16 forward.

We probably need to decide for some modules if we want to inline them or replace them with more up-to-date alternatives. At least for the ng-simple-slideshow we need to make sure that replacing them does not accidentally break XSS challenges.

Recommended Links

• https://pwning.owasp-juice.shop/companion-guide/latest/part3/contribution.html
• https://pwning.owasp-juice.shop/companion-guide/latest/part3/codebase.html#_client_tier
• https://update.angular.io/?l=3&v=15.0-16.0

[AnmolSirola]

@bkimminich Are there specific strategies or patterns you recommend for effectively inline-replacing older modules to ensure seamless integration with Ivy?

Appreciate any guidance as I'm keen to contribute/take on this issue :)

[EbonyAdder]

If any additional assistance is needed, I would also be happy to help. Thanks.

[bkimminich]

@bkimminich Are there specific strategies or patterns you recommend for effectively inline-replacing older modules to ensure seamless integration with Ivy?

Appreciate any guidance as I'm keen to contribute/take on this issue :)

We didn't have to do this up until now, as everything was always backward compatible with NGCC. The official recommendation is to ask library maintainers to switch to Ivy, but since that doesn't help with abandoned libs, we're on our own so to speak to find a lib replacement or recreate them in our own codebase as modules/components.

[bkimminich]

If any additional assistance is needed, I would also be happy to help. Thanks.

@EbonyAdder, this totally could be tackled by more than one person, so how about I invite both @AnmolSirola and you to our next developer meeting on Jan 17th and we can come up with a battle plan? I'll ping you on Slack for the invitation.

[martinakraus]

Hey together,

is this still needed? I'm quite experienced with Angular and I have upgraded already a couple of Angular app to v17 even.
Would love to help

[bkimminich]

Hey together,

is this still needed? I'm quite experienced with Angular and I have upgraded already a couple of Angular app to v17 even.
Would love to help

Hi! Yes, this is still an open task and we'd love to get your help with it! 👍

[martinakraus]

Sure :)
So how should I proceed? There is already an Angular 16 branch - should I take this one over or create a new branch an using the old one as a reference for what was already done.

Also since May v18 will be released does it make sense to update to v17 already as well? (of course first to 16) but then to 17 as well?

[bkimminich]

The existing branch was more an experiment I did when I bumped into the GCC vs Ivy 🧱 ... 😅

Please feel free to ignore that one. And of course going all the way to the latest version would be ideal! 🤩

[martinakraus]

Sure I'll give it a try :) :)

[martinakraus]

@bkimminich ok I was able to upgrade to v16 (after everything is fixed I'll upgrade to v17) and build it without errors.
BUT some packages need to be replaced (due to not supported Ivy versions etc.):

• ng-simple-slideshow -> I will include ng-gallery which has even kind of the same API: https://www.npmjs.com/package/ng-gallery

• ngx-text-diff: -> Do we have a version compatible with Ivy? ABenassi87/ngx-text-diff#71
  Still need to look for a good replacement

• ng-mat-search-bar -> I might just implement this thing on my own with material-input

Then another thing: Your fork: Anuglar2-qrcode also creates issues. I forked it already from you and just thinking about migrating this to Ivy as well - would that be ok with you?

[bkimminich]

Wow, you're fast! 🤩 ... The Angular QR Code library fork is part of a hacking challenge, so if we'd inline it as an Ivy module, we will need to pick another "victim" for our typosquatting challenge. That's not a big deal, so please feel free to replace it. You can base it on the original lib, not my fork, as I only made this for the typosquatting challenge.

[martinakraus]

@bkimminich for v16 I have created a PR:
#2195
If you want you can already take a look :).
I'll update now to v17 - shouldn't be that much effort (hopefully) now

I would recommend updating angular/material in another separate branch since it affects MAAANY files :D

[bkimminich]

Awesome, thanks! We'll check it out asap! 👍

[martinakraus]

An update to v17 has also landed.
It's in the same PR :)

[bkimminich]

Wow! I'll look at it latest on Saturday when I'm back from a business trip. 🚀

[bkimminich]

@martinakraus, I'm moving any remaining issue discussions here so we don't need to always go back to the closed PR.

I noticed that the minification is not happening on the main.js any more. When I run locally npm install --production, the exact same error from the CI/CD will also occur locally. Maybe you can take a look at this? My assumption is, that some default behavior changed when it comes to dev/prod build. In the old Angular 15.x world (see develop branch) when doing npm install I will get the minified JS frontend code. This is exactly the behavior we need to keep, because many users launch Juice Shop from source, and some challenges rely on the frontend code being minified. Non-minified the challenges would be too easy.

https://update.angular.io/?l=3&v=15.0-17.0 doesn't give me any hint why the behavior has changed from minified-by-default to non-minified.

[bkimminich]

The remaining issues come from the Angular configuration. In dev mode all seems to work fine, in production mode all goes up in 🔥. I tried different configuration flag permutations, but none was without errors unfortunately. The Angular migration guide didn't help me find the issue either.

[martinakraus]

Ok I'm not quite sure.
In v12 Angular switched the default build mode to prod.
I tried npm run build back then without errors so unfortunately I'm not sure what kind of build config is used for CI/CD.
Buuut let me dive into this!
I'm having a workshop tomorrow and Thursday and will check latest on Friday what you have done so far :)
I really really want to know whats going on 😅

[bkimminich]

Ok I'm not quite sure.
In v12 Angular switched the default build mode to prod.
I tried npm run build back then without errors so unfortunately I'm not sure what kind of build config is used for CI/CD.
Buuut let me dive into this!
I'm having a workshop tomorrow and Thursday and will check latest on Friday what you have done so far :)
I really really want to know whats going on 😅

Just npm install --production should do the trick, that's what the CI/CD does.

[martinakraus]

Ok I'm not quite sure.
In v12 Angular switched the default build mode to prod.
I tried npm run build back then without errors so unfortunately I'm not sure what kind of build config is used for CI/CD.
Buuut let me dive into this!
I'm having a workshop tomorrow and Thursday and will check latest on Friday what you have done so far :)
I really really want to know whats going on 😅

Just npm install --production should do the trick, that's what the CI/CD does.

Ah I have actually a guess.
I moved some things from dependecies to devDependencies and only dependencies will be installed with - -production flag.

Additionally I read on the docs

The npm install command will install the devDependencies along other dependencies when run inside a package directory, in a development environment (the default).

In version 8.x and above use --omit=dev flag to install only regular dependencies:

npm install --omit=dev 

This will install only dependencies, and not devDependencies, regardless of the value of the NODE_ENV environment variable.

[martinakraus]

Yep - it is totally related to my moving dependency things.
Reverting it fixes the issue with not finding custom-webpack but there is still some small issue - Buuut Im getting there :)

Btw npm does print a warning: so maybe we could switch that in CI/CD as well:

npm WARN config production Use --omit=dev instead.

[bkimminich]

I think we tried that once, but that isn't recognized by Angular iirc, so let's first try to get back to "works as before" and then we can still consider optimizing.

[martinakraus]

So with this package,json:
#2207

The npm install --production

(sorry for over-optimizing 😅)

So the npm install --production went through on the docker smoke test.
There are some other things failing, but is this all related to the change of this package.json?

[martinakraus]

How do you run the production build?
So basically it's just ng build: https://angular.io/guide/deployment
But you have some additional parameters, so I ran npm run frontend:build actually without errors :(

[bkimminich]

I just run npm install in the main folder, which then triggers on develop

ng build --aot --configuration production --output-hashing=none --vendor-chunk=true --source-map=false

and on martinakraus-master

ng build --output-hashing=none --vendor-chunk=true --source-map=false

which is a non-prod build. If you try --configuration production, you should see all the errors I mentioned. I even tried to include the passed parameters into the config in angular.json, but that doesn't help either.

[martinakraus]

I see - there seems to be something a bit wrong with the configuration. Usually ng build is already prod build, which has aot: true already set.
Nevertheless - I'm able to fix these errors as well.
Seems like aot in v16 doesn't like to have this huge AppModule anymore - so turning the related components into standalone will fix this.
I can do this later.

[martinakraus]

Indeed "defaultConfiguration": "production" wasn't set in the angular.json with that it isn't actually necessary to add the parameters --aot --configuration production --output-hashing=none --vendor-chunk=true --source-map=false
since these configurations can be set for production build in angular.json:

           "optimization": true,
              "outputHashing": "none",
              "sourceMap": false,
              "namedChunks": false,
              "aot": true,
              "extractLicenses": true,
              "vendorChunk": true,
              "buildOptimizer": true

[bkimminich]

Hi @martinakraus, I think I've pulled all the command line arguments into the production config and force its use during ng build. This is as far as I get before ❌❌❌...❌ happens, see https://github.com/juice-shop/juice-shop/actions/runs/8427747979.

[martinakraus]

I'm so sorry😢 sometimes life happens and work keeps crushing me.

As mentioned moving to standalone components seems to resolve the issue on aot compilation. It's not that big of a deal. It just needs to be done carefully with some time.
I'm sitting a looong train ride on Monday.
Then I finally have time to solve this hopefully.
I'm really sorry didn't want to have this hanging for so long. But we will get there 😊

[bkimminich]

No worries, I just removed that label because I thought that the current state of Angular-affairs is not really something for a beginner to pick up... 😁

I was coincidentally just playing around with standalone components:

Subject: [PATCH] Switch wallet component to standalone
---
Index: frontend/src/main.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/frontend/src/main.ts b/frontend/src/main.ts
--- a/frontend/src/main.ts	(revision f908e09f53df3e87ba1084f9fc19e95f2f70ae10)
+++ b/frontend/src/main.ts	(date 1712325178765)
@@ -8,10 +8,15 @@
 
 import { AppModule } from './app/app.module'
 import { environment } from './environments/environment'
+import { WalletComponent } from './app/wallet/wallet.component'
+import { bootstrapApplication } from '@angular/platform-browser'
 
 if (environment.production) {
   enableProdMode()
 }
 
+bootstrapApplication(WalletComponent)
+  .catch((err: Error) => console.log(err))
+
 platformBrowserDynamic().bootstrapModule(AppModule)
   .catch((err: Error) => console.log(err))
Index: frontend/src/app/app.module.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/frontend/src/app/app.module.ts b/frontend/src/app/app.module.ts
--- a/frontend/src/app/app.module.ts	(revision f908e09f53df3e87ba1084f9fc19e95f2f70ae10)
+++ b/frontend/src/app/app.module.ts	(date 1712324836017)
@@ -117,7 +117,6 @@
 import { SavedAddressComponent } from './saved-address/saved-address.component'
 import { ChallengeStatusBadgeComponent } from './challenge-status-badge/challenge-status-badge.component'
 import { OrderCompletionComponent } from './order-completion/order-completion.component'
-import { WalletComponent } from './wallet/wallet.component'
 import { WalletService } from './Services/wallet.service'
 import { OrderHistoryComponent } from './order-history/order-history.component'
 import { OrderHistoryService } from './Services/order-history.service'
@@ -201,7 +200,6 @@
     PurchaseBasketComponent,
     PrivacyPolicyComponent,
     ChallengeStatusBadgeComponent,
-    WalletComponent,
     OrderHistoryComponent,
     DeliveryMethodComponent,
     PhotoWallComponent,
Index: frontend/src/app/wallet/wallet.component.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/frontend/src/app/wallet/wallet.component.ts b/frontend/src/app/wallet/wallet.component.ts
--- a/frontend/src/app/wallet/wallet.component.ts	(revision f908e09f53df3e87ba1084f9fc19e95f2f70ae10)
+++ b/frontend/src/app/wallet/wallet.component.ts	(date 1712325090070)
@@ -7,11 +7,17 @@
 import { WalletService } from '../Services/wallet.service'
 import { UntypedFormControl, Validators } from '@angular/forms'
 import { Router } from '@angular/router'
+import { TranslateModule } from '@ngx-translate/core'
+import { MatFormFieldModule } from '@angular/material/form-field'
+import { MatCardModule } from '@angular/material/card'
+import { MatIconModule } from '@angular/material/icon'
 
 @Component({
+  standalone: true,
   selector: 'app-wallet',
   templateUrl: './wallet.component.html',
-  styleUrls: ['./wallet.component.scss']
+  styleUrls: ['./wallet.component.scss'],
+  imports: [TranslateModule, MatFormFieldModule, MatCardModule, MatIconModule]
 })
 export class WalletComponent implements OnInit {
   public balance: string

I was down to one remaining compiler error for that component at least... But I'm not entirely sure this is "right" and would result in a running application or if it would just compile and then the disaster happens at runtime.

[bkimminich]

⏰ Getting this issue resolved with community support would be amazing! I currently consider this the sole blocker for releasing v17.0.0, which is otherwise ready-to-go. It just doesn't have enough incompatible changes to really warrant a major release, so getting the Angular 17.x migration done would be really nice as added value. Aaaaalso, coincidentally the version numbers of Juice Shop and Angular would align...

[bkimminich]

I might have gotten us to a point where the remaining broken things are "just" the UI styles (see screenshot) and the fact that the mat-slider on the Customer Feedback screen is no longer working...

[thomasbreland]

I upgraded an old Angular application by like four versions before, and this was pretty recent. I think I can help. I have never contributed to open source before, so I am reading the contribution guidelines for the project now.

[thomasbreland]

I am caught up on reading and have forked the repository.

I might have gotten us to a point where the remaining broken things are "just" the UI styles (see screenshot) and the fact that the mat-slider on the Customer Feedback screen is no longer working...

Have these changes been committed somewhere, or should I start working from 'develop'?

[bkimminich]

I am caught up on reading and have forked the repository.

I might have gotten us to a point where the remaining broken things are "just" the UI styles (see screenshot) and the fact that the mat-slider on the Customer Feedback screen is no longer working...

Have these changes been committed somewhere, or should I start working from 'develop'?

The working branch is martinakraus-master for the Angular 17 migration, and I pushed it, yes. The CI/CD did not like that at all... But locally it almost works, except for the UI trouble.

[thomasbreland]

The working branch is martinakraus-master for the Angular 17 migration, and I pushed it, yes. The CI/CD did not like that at all... But locally it almost works, except for the UI trouble.

I will do my best.

[bkimminich]

The working branch is martinakraus-master for the Angular 17 migration, and I pushed it, yes. The CI/CD did not like that at all... But locally it almost works, except for the UI trouble.

I will do my best.

Just taking a look with a fresh perspective is already worth a lot... 🙏

As the CI/CD is currently in shambles, I ran the frontend unit tests locally, and there are some issues with several components apart from the Customer Feedback screen. Also problems with the About Us page, the code snippet dialog and some others. All in all my local result of npm run test is currently

Chrome Headless 124.0.6367.60 (Windows 10): Executed 640 of 644 (53 FAILED) (skipped 4) (10.478 secs / 5.85 secs)

[thomasbreland]

I'm seeing 54 failed on my end; much of what you have described though. I am investigating; I think I had a similar issue with npm packages when I did the migration at my last job.

[thomasbreland]

When running npm test I received many "Error: NG01203: No value accessor for form control unspecified name attribute." It appears from my limited testing that specifying ngDefaultControl for UntypedFormControls may resolve this issue. Of course, I will need to test this again running the application and with e2e tests. Mentioning here because you may already know whether this is an acceptable solution or if a more tailored approach will be needed for each UntypedFormControl, but I will continue investigating.

I received another error suggesting that ngOnInit () should specify return type void, i.e., ngOnInit (): void.

While investigating NullInjectorError: No provider for TranslateService! I found that TranslateModule.forRoot() was not included, nor a translateService mock created, for about component as it was for contact component. I am trying my best to make things consistent, though there may be a more efficient way to configure these tests that I am yet unaware of.

I am down to 46 failures, but I see many instances of those same errors described above.

[thomasbreland]

If ng generate @angular/material:mdc-migration was run at any point, it does not appear that all components were selected. Running that for slider, making a few small changes listed on https://material.angular.io/guide/mdc-migration#slider , and changing the import from MatSlider to MatSliderModule seems to have the slider working again.

I apologize if this is too verbose for this thread, this is just my first time contributing to open source as I mentioned before, and I want to make sure that these findings are documented in a timely manner so that we are not needlessly duplicating effort. I will continue applying my findings to other components and working to resolve these errors.

[bkimminich]

This is by no means too verbose, it's super helpful! 🙏

I wasn't even considering the possibility that the script might have overlooked some existing Material component... 😅

[thomasbreland]

I went down a rabbit hole with npm packages and then got a little distracted. There will have to be some legacy-peer-dependencies, I'm sure - as there already are - so I'm doing a bit of a technical triage of the situation and deferring as much of that as I can justify while I get the UI cleaned up and tests running.

With the last migration I did, I had to do a small patch on a dependency with the help of https://www.npmjs.com/package/patch-package, and that might be acceptable for this use-case if it comes down to it; but hopefully it won't - I'm just mentioning it here as a last-resort option for anything we cannot otherwise resolve in a timely manner. Even @angular-material-extensions/password-strength - which is currently maintained - only officially supports up to Angular 15, though others online have had luck getting it running with 17.

The routing should probably be updated at some point - in the tests as well - so I'm looking into that a little bit.

I will continue on it tomorrow; it is currently 1 AM in my timezone, so I should get to sleep.

[thomasbreland]

I still need to finish fixing the UI, but I have been trying to get these unit tests running. I made a couple of commits to my fork here, for your review: https://github.com/thomasbreland/juice-shop/tree/2173_breland

The CodeSnippetComponent and ScoreBoardPreviewComponent tests seem to be the only ones having issues; 25 total test failures, but really just the same two recurring issues. I am of course very new to this repo, so I want to make sure I that really understand what is going on before I start editing tests/mocks/spies - lest the tests only confirm existing behavior.

---

In score-board.component.spec.ts

TypeError: Cannot read properties of undefined (reading 'showFlagsInNotifications')
            at templateFn (ng:///ChallengeCardComponent.js:239:91)

This seems to be defined with a spy of ConfigurationService elsewhere when tested, e.g., challenge-solved-notification.component.spec.ts:

configurationService.getApplicationConfiguration.and.returnValue(of({ ctf: { showFlagsInNotifications: true } }))

---

In code-snippet.component.spec.ts

TypeError: Cannot read properties of undefined (reading 'split')
            at CodeAreaComponent.call [as ngOnInit] (src/app/code-area/code-area.component.ts:

The line referenced occurs within the ngOnInit function:
this.lineMarkers = this.code.split('\n').map((line, lineIndex) => {

The variable is declared in code-area.component.ts:

@Input('code')
  public code: string = ''

---

@angular-devkit/architect 0.1703.5 (cli-only)
@angular-devkit/core 17.3.5 (cli-only)
@angular-devkit/schematics 17.3.5 (cli-only)
@schematics/angular 17.3.5 (cli-only)
typescript 4.6.4
Node.js 20.12.2

[thomasbreland]

After being stuck on the tests or a while, I decided to focus on the UI for a bit.

Running the ng generate @angular/material:mdc-migration for the rest of the component types yielded 46 instances of this TODO throughout various scss files
/*TODO(mdc-migration): The following rule targets internal classes of X that may no longer apply for the MDC version where X includes the following:

• button
• card
• checkbox
• dialog
• form-field
• select
• tabs (table)

I also noticed that it missed renaming one css element, but I fixed that and committed those changes to my fork. Now I have to figure out which internal classes no longer exist, and I'm also trying to get the search bar to better match the old ng-mat-search-bar, which has led me to refactor the scss files to bring those more up-to-date; there seem to have been some side-effects with typography not being defined in the themes, even though that should be "optional".

---

A number of the modules have also been renamed to xModule, where x includes - but is not necessarily limited to - the following:

• MatButton
• MatCard
• MatFormField
• MatIcon
• MatInput

I have made some progress with the cards locally, but there is still work to be done.

[bkimminich]

Wow, I really wasn't expecting this to become that big of an endeavor... 😨

If you'd like to see the CI/CD results for your branch, feel free to just open a PR (in draft mode for example), and then most of the tests will trigger whenever you push something new into the PR's underlying branch.

[thomasbreland]

I did a bit more of the MDC migration, took a break, and came back to it. I want to make sure that everything is consistent and that I'm really understanding the differences between the old and new components before I start committing more changes that I might soon revert. Still, here are some things that might be interesting.

Avoid defining custom styles that would affect the size or internal layout of the component, such as padding, height, width, or overflow. You can specify display: none to hide a component, but avoid specifying any other display value. Overriding these properties can break components in unexpected ways as the internal styles change between releases.
- https://material.angular.io/guide/customizing-component-styles

Sometimes there just isn't a way around it, but I'm trying to minimize unnecessary changes.

Card padding

The element itself does not add any padding around its content. This allows developers to customize the padding to their liking by applying padding to the elements they put in the card.

In many cases developers may just want the standard padding specified in the Material Design spec. In this case, the , , and sections can be used.
- https://material.angular.io/components/card/overview#card-padding

The impact of this was immediately obvious. I think the easiest refactor would be to just place the card contents in a div with set padding, which should hopefully avoid breaking changes in the future, but I'm trying a few different ways and I'm open to suggestions. No way around it, we will need a serious code review when the time comes, but hopefully this is enough information to help if anyone else has strong opinions about the style code and would like to give it a shot. Still, I will keep working on it and reporting my findings.

Other observations:

• mat-form-field: min-width is now 0 by default, which seems to be what made the select drop-downs on the score-board nearly disappear
• mat-button: "white-space: nowrap;" is omitted, e.g., "Hide disabled challenges" button
• button contents (img/span) do not vertical-align by default, e.g. home button / logo

---

The last commit I pushed to my fork was just changes to the scss material theme. A side effect of typography not being defined in the styles was that certain custom properties expected by Material were not defined, like "--mdc-filled-text-field-container-color" and "--mdc-filled-text-field-disabled-container-color" which determine the background color of e.g., the search bar.

[thomasbreland]

I have opened my first ever draft PR. The test failures mentioned in #2173 (comment) still remain, unfortunately. I finished migrating the cards and tables, and I've included a checklist for the remaining issues I'm working on.

• Verify internal classes for the following MDC components:
  • button
  • checkbox
  • dialog
  • form-field
  • select
• Verify styling / parameters for the following MDC modules:
  • MatButtonModule
  • MatFormFieldModule
  • MatIconModule
  • MatInputModule
• Update MatIconButton to MatButtonModule + MatIconModule as per https://material.angular.io/components/button/overview
• mat-form-field: set "min-width" (or set column width/percentage for "options-group")
• mat-button: set "white-space: nowrap;"
• Notifications: vertical-align text middle
• Home button/logo: vertical-align content middle
  • Check if issue occurs on other buttons
• Fix search bar styling
• Fix navigation menu styling
• Fix header on tracking page
  • Check if this issue occurs elsewhere
• Make sure help popup is functioning correctly

There also seems to be a bit of styling that is no longer in use, but that might best be handled as a separate task. Most of these things should be pretty simple, it's just a matter of verifying that there are no side-effects and keeping the SCSS succinct.

[thomasbreland]

I am unable to recreate the errors occurring in the installation step of the pipeline locally. Looking at the pipeline, I notice the following...

Run actions/setup-node@5e21ff4d9bc1a8cf6de233a3057d20ec6b3fb69d
Found in cache @ C:\hostedtoolcache\windows\node\20.13.1\x64
Environment details
  node: v20.13.1
  npm: 10.5.2
  yarn: 1.22.22

...even while the pipeline step indicates Node.js 18; this might be worth looking into. It is possible that we need to update the pipeline actions themselves. It might also help to bring back the package-lock.json and use 'npm ci' instead of 'npm i' to ensure that the pipeline is using the exact same package versions. (I'm guessing that clearing the cache would not help - considering that we never call https://github.com/actions/cache - but it is another troubleshooting suggestion that I have run across.)