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

security improvement #4

Merged
merged 1 commit into from Feb 1, 2019

Conversation

Projects
None yet
5 participants
@VasylRudnitskyi
Copy link
Contributor

VasylRudnitskyi commented Jan 18, 2019

No description provided.

@VasylRudnitskyi VasylRudnitskyi requested a review from markwahl-msft Jan 18, 2019

@msftclas

This comment has been minimized.

Copy link

msftclas commented Jan 18, 2019

CLA assistant check
All CLA requirements met.

@VasylRudnitskyi VasylRudnitskyi requested a review from ntonyho Jan 23, 2019

@markwahl-msft

This comment has been minimized.

Copy link
Contributor

markwahl-msft commented Jan 30, 2019

Acknowledgement to finder of issue T. Sluijter.

@tsluyter

This comment has been minimized.

Copy link

tsluyter commented Jan 31, 2019

Acknowledgement to finder of issue T. Sluijter.

Thanks for the acknowledgement, I appreciate it!

I've gone over the modifications to roles.js and I don't really see changes that would fix the XSS I'd found. But maybe that's because Javascript isn't my forté. I'll give it another look.

@VasylRudnitskyi

This comment has been minimized.

Copy link
Contributor Author

VasylRudnitskyi commented Feb 1, 2019

      > Acknowledgement to finder of issue [T. Sluijter](https://www.kilala.nl).

Thanks for the acknowledgement, I appreciate it!
I've gone over the modifications to roles.js and I don't really see changes that would fix the XSS I'd found. But maybe that's because Javascript isn't my forté. I'll give it another look.

We have changed rendering of user input value from 'as html' to 'as text', this made by next changes:
"columnDefs": [
{ targets: [0, 5, 6], render: $.fn.dataTable.render.text() },
],
Where targets is numbers of column which represent values provided by user as 'Display Name', 'Description' or 'Justification'

@tsluyter

This comment has been minimized.

Copy link

tsluyter commented Feb 1, 2019

@VasylRudnitskyi

This comment has been minimized.

Copy link
Contributor Author

VasylRudnitskyi commented Feb 1, 2019

      EDIT: removed the comment. Let’s discuss the rest privately.

This fix make save users who use this specific sample,
for described in your removed comment issue I've opened bug, as it out of scope of PAM Portal sample.
For communication or in case of any questions please mail me v-varudn@microsoft.com

@tsluyter

This comment has been minimized.

Copy link

tsluyter commented Feb 1, 2019

I will email you. I have already submitted the bug/vulnerability in question

@ntonyho

This comment has been minimized.

Copy link
Collaborator

ntonyho commented Feb 1, 2019

If you consider the systems being independent, each system should sufficiently protect itself without making assuming on the data from external systems

The web app should not trust the data from external systems including but not limited to SQL, active directory.

Otherwise, I will just put JavaScript as my display name in my active directory user / group object

@markwahl-msft markwahl-msft merged commit 9fec5c4 into master Feb 1, 2019

1 check passed

license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment