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

make the application build w/ PrimeFaces 7 #6281

Merged
merged 1 commit into from
Oct 22, 2019
Merged

Conversation

landreev
Copy link
Contributor

Related Issues

Pull Request Checklist

@coveralls
Copy link

Coverage Status

Coverage increased (+0.005%) to 19.48% when pulling 2e52a65 on 5975-primefaces-7.0 into c42bad4 on develop.

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

The code seems fine but I'm planning on speaking with @djbrooke about the security concerns I have with PrimeFaces. I mentioned a specific cross-site scripting vulnerability in my review.

@@ -277,7 +277,7 @@
<dependency>
<groupId>org.primefaces</groupId>
<artifactId>primefaces</artifactId>
<version>6.2</version>
<version>7.0</version>
Copy link
Member

Choose a reason for hiding this comment

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

7.0.8 is shown at https://www.primefaces.org/downloads/ but my understanding is that we cannot use it without paying for it. There seem to be a variety of bug fixes in https://github.com/primefaces/primefaces/issues?utf8=%E2%9C%93&q=is%3Aissue+label%3A7.0.8 for example. Longer term I think we should not depend on software where that requires you to pay for security updates. primefaces/primefaces#4913 is an example of a cross-site scripting vulnerability that was fixed in PrimeFaces 7.0.5. See also some related discussion about security concerns for open source projects using PrimeFaces in the following places:

@djbrooke
Copy link
Contributor

@pdurbin OK, I'm happy to discuss but is that a reason to hold up this PR? I assume there were many security fixes (and other good stuff) included in 7, so while I'm happy to discuss longer term plans I don't see any reason not to upgrade now.

@djbrooke djbrooke self-assigned this Oct 18, 2019
@pdurbin
Copy link
Member

pdurbin commented Oct 18, 2019

@djbrooke no, there is no need to hold up this PR. We're in an unfortunate situation when it comes to security but what I'd like is a strategy for how to not having cross-site scripting vulnerabilities such as primefaces/primefaces#4913 in Dataverse. Should I create an issue for that specific security vulnerability as an example and we'll talk as a team about how to address it?

@djbrooke
Copy link
Contributor

djbrooke commented Oct 18, 2019

@pdurbin Thanks for letting me know it won't hold up this PR. I'll move it to QA. I think creating an issue for the Primefaces issue is fine, about whether to address it as a one off or to get consensus around a longer term strategy. Sounds like a good tech hours (or other venue) discussion with @scolapasta and other devs.

@pdurbin
Copy link
Member

pdurbin commented Oct 18, 2019

@djbrooke ok, I created https://github.com/IQSS/dataverse-security/issues/9

@djbrooke
Copy link
Contributor

Helping @kcondon with testing for this and I got the message below under "Verbose" in Chrome Dev Tools once on dataverse-internal when loading the dataverse search page. I haven't been able to reproduce it on either dataverse-internal, production, or demo since. Not sure if it's related to this change, I'll let others decide. :)

[Violation] Added non-passive event listener to a scroll-blocking ‘touchstart’ event. Consider marking event handler as ‘passive’ to make the page more responsive. See https://www.chromestatus.com/feature/5745543795965952

Otherwise everything looks good! Nice work!

@kcondon
Copy link
Contributor

kcondon commented Oct 18, 2019

Found this in browser console when editing file metadata from file page. Everything worked fine though:

file.xhtml?persistentId=doi:10.70122/FK2/RTP87H/891HZC&version=DRAFT:1 Uncaught TypeError: Cannot read property 'hide' of undefined
   at HTMLButtonElement.onclick (file.xhtml?persistentId=doi:10.70122/FK2/RTP87H/891HZC&version=DRAFT:1)
onclick @ file.xhtml?persistentId=doi:10.70122/FK2/RTP87H/891HZC&version=DRAFT:1

@mheppler
Copy link
Contributor

mheppler commented Oct 21, 2019

After looking over the 5975-primefaces-7.0 branch locally in Chrome, I was finally able to replicate the warnings and errors reported by @djbrooke and @kcondon above. I was also able to replicate them on demo.

None of these seem to cause any functionality to break. I was able to find a smoking gun related to the Uncaught TypeError on the file pg (PF('blockDatasetForm').hide();) but that is merely trying to hide an element that isn't on the file pg (it's only on the dataset pg). We can address this console error during dataset/file pg redesign efforts.

@mheppler mheppler removed their assignment Oct 21, 2019
@kcondon kcondon merged commit dbb145c into develop Oct 22, 2019
@kcondon kcondon deleted the 5975-primefaces-7.0 branch October 22, 2019 17:16
@djbrooke djbrooke added this to the 4.18 milestone Oct 23, 2019
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.

Upgrade to PrimeFaces 7
6 participants