Skip to content
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.

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

KEYCLOAK-8541: Admin console uses vulnerable AngularJS #5679

Merged
merged 1 commit into from
Nov 1, 2018

Conversation

ssilvert
Copy link
Contributor

No description provided.

@ssilvert ssilvert self-assigned this Oct 26, 2018
@ssilvert ssilvert requested a review from stianst October 26, 2018 13:04
@ssilvert
Copy link
Contributor Author

@keycloak-ci-bot test

@keycloak-ci-bot
Copy link

@ssilvert Job is scheduled

@stianst stianst requested a review from vmuzikar October 29, 2018 13:50
Copy link
Contributor

@vmuzikar vmuzikar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've build the distribution from your branch. Using Chrome's "incognito mode", I've logged in into the Admin Console. I can see that angular.min.js is loaded but looking at the header it's still v1.6.6.

@ssilvert Could you please review?

@ssilvert
Copy link
Contributor Author

Glad you caught that. Looks like we now have two node_modules directories. It's possible that Stian added the second one by mistake, but it could have been intentional.

I'm going to update the other one and be done for now. I'll create a separate JIRA to investigate.

@ssilvert
Copy link
Contributor Author

ssilvert commented Oct 30, 2018

Now I see what he did. The second version is a "clean" directory with only the stuff that belongs in the distro. We really need to pull that stuff out automatically so we don't have two copies of the same file in the repo. But again, that's a task for another day. See KEYCLOAK-8700.

@ssilvert
Copy link
Contributor Author

@vmuzikar You should be able to retest this now.

@stianst
Copy link
Contributor

stianst commented Oct 31, 2018

I remember now - you need to run mvn clean install -P npm-update in themes. That'll update the clean node_modules.

I agree at some point we need to automate this. Probably move to just downloading everything from NPM. There's one thing I don't like about that and it's that I would have to run a Maven build before running KeycloakServer from IDE. I can easily see that becoming out of sync as at least personally I rarely do a Maven build locally for Keycloak.

@vmuzikar
Copy link
Contributor

@ssilvert I suppose you've updated the files in the other node_modules directory "manually".
For the consistency, shouldn't we instead use the approach @stianst mentioned - mvn clean install -P npm-update?

@ssilvert
Copy link
Contributor Author

I don't think it makes a difference. The result is exactly the same.

@vmuzikar
Copy link
Contributor

Ok, if we're sure the result is the same, I'll proceed with the testing...

Copy link
Contributor

@vmuzikar vmuzikar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now, thanks.

@ssilvert
Copy link
Contributor Author

@keycloak-ci-bot test

@keycloak-ci-bot
Copy link

@ssilvert Job is scheduled

@stianst stianst merged commit d0f75c8 into keycloak:master Nov 1, 2018
@ssilvert ssilvert deleted the kc8541-ngJS-vulnerable branch November 1, 2018 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants