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

[TECHNICAL SUPPORT] LPS-125854 Fix XSS validating context and encoding HTML #699

Closed

Conversation

NemethNorbert
Copy link

Hey,
LPS-125854,

I based my work on: 418sec/form#1

First sec issue, let me know if I missed something.

Please review it.
Thanks,

@liferay-continuous-integration
Copy link
Collaborator

CI is automatically triggering the following test suites:

  •     ci:test:relevant
  •     ci:test:sf

@liferay-continuous-integration
Copy link
Collaborator

❌ ci:test:sf - 0 out of 1 jobs passed in 3 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 9abb5412f865531a30c879edc61e94795e419bce

Sender Branch:

Branch Name: LPS-125854
Branch GIT ID: a8e11fe37d2403da700af590d783d08df6803a8d

0 out of 1jobs PASSED
For more details click here.
     [exec] warning "workspace-aggregator-16a800fa-c20e-4608-936f-85f1e420b8a2 > content-dashboard-web > recharts > [email protected]" has unmet peer dependency "react-dom@^15.0.0 || ^16.0.0".
     [exec] warning "workspace-aggregator-16a800fa-c20e-4608-936f-85f1e420b8a2 > questions-web > @apollo/client > [email protected]" has incorrect peer dependency "graphql@^0.9.0 || ^0.10.0 || ^0.11.0 || ^0.12.0 || ^0.13.0 || ^14.0.0".
     [exec] warning "workspace-aggregator-16a800fa-c20e-4608-936f-85f1e420b8a2 > content-dashboard-web > recharts > react-smooth > [email protected]" has unmet peer dependency "react-dom@>=15.0.0".
     [exec] [4/4] Building fresh packages...
     [exec] Done in 6.12s.
     [exec] 
     [exec] > Task :packageRunCheckFormat
     [exec] yarn run v1.13.0
     [exec] \$ liferay-npm-scripts check
     [exec] apps/frontend-js/frontend-js-jquery-web/src/main/resources/META-INF/resources/jquery/form.js: BAD
     [exec] Prettier checked 1 file, 1 file has problems
     [exec] 1 of 3 jobs failed
     [exec] info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
     [exec] error Command failed with exit code 1.
     [exec] 
     [exec] > Task :packageRunCheckFormat FAILED
     [exec] Gradle build finished at 2021-01-14 12:01:11.242.
     [exec] 
     [exec] 
     [exec] FAILURE: Build failed with an exception.
     [exec] 
     [exec] * What went wrong:
     [exec] Execution failed for task ':packageRunCheckFormat'.
     [exec] > Process 'command '/opt/dev/projects/github/liferay-portal/build/node/bin/node'' finished with non-zero exit value 1
     [exec] 
     [exec] * Try:
     [exec] Run with --info or --debug option to get more log output. Run with --scan to get full insights.
     [exec] 
     [exec] * Exception is:
     [exec] org.gradle.api.tasks.TaskExecutionException: Execution failed for task ':packageRunCheckFormat'.
     [exec] 	at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.lambda\$executeIfValid\$1(ExecuteActionsTaskExecuter.java:208)
     [exec] 	at org.gradle.internal.Try\$Failure.ifSuccessfulOrElse(Try.java:263)
     [exec] 	at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.executeIfValid(ExecuteActionsTaskExecuter.java:206)
     [exec] 	at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.execute(ExecuteActionsTaskExecuter.java:187)

@NemethNorbert
Copy link
Author

There are many SF issues in the module that is not connected to my changes. I figured we don't do sourceFormat on this file, am I right?

@liferay-continuous-integration
Copy link
Collaborator

@samuelkong
Copy link

Hi @NemethNorbert
The current fix makes the file very confusing because the file says this is jQuery Form Plugin 3.51.0. However, with the addition of your changes, this file is no longer version 3.51.0.

Ideally, we should upgrade to a version of jQuery Form Plugin which includes the fix. If that's not possible, at least update the version to something like 3.51.0.LIFERAY-PATCHED-ISSUE-586 so that it's a little more obvious that this file includes a patch for jQuery Form issue 586. Thanks.

@julien
Copy link

julien commented Jan 14, 2021

@NemethNorbert if I run yarn checkFormat in the frontend-js-jquery-web module in the master branch I see many errors, (21 errors, 0 warnings in total) but the error that you're seeing in the test report is about the form.js file being formatted incorrectly (an error from prettier).

It might be worth fixing those (eslint) errors too and if you need help with that feel free to ask.

@NemethNorbert
Copy link
Author

@samuelkong, the latest version is not including this fix. I will update the file based on your suggestions.

@julien I give it a go and make an SF commit as well.

Thanks,

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:stable - 9 out of 9 jobs passed

✔️ ci:test:relevant - 23 out of 23 jobs passed in 1 hour 33 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 9abb5412f865531a30c879edc61e94795e419bce

Copied in Private Modules Branch:

Branch Name: master-private
Branch GIT ID: 15218ef33680a1fe233ea812e8d097cfcb0feb42

ci:test:stable - 9 out of 9 jobs PASSED
9 Successful Jobs:
ci:test:relevant - 23 out of 23 jobs PASSED
23 Successful Jobs:
For more details click here.

@liferay-continuous-integration
Copy link
Collaborator

@julien
Copy link

julien commented Jan 15, 2021

@NemethNorbert just wanted to let you know that in the master branch (with commit 9abb5412f865 when running yarn checkFormat in the frontend-js-jquery-web module I get 72 errors .

I'm going to look if something has changed regarding formatting or linting in the module just to make sure we're not missing something.

@NemethNorbert
Copy link
Author

Thanks @julien for looking into that. I also run an SF on 7.2.x and I found 18errors that can be found on master as well. I think this file is not transpired and we ignored the SF errors on it so far.

@samuelkong I updated the file with the suggested version change

Thank you,

@julien
Copy link

julien commented Jan 15, 2021

@NemethNorbert I don't think it has to do with transpiling but rather linting and formatting.

In LPS-100905, we added linting and formatting support for "older" modules, and it seems the original PR was this one, which seems to have been forwarded here: in that PR I don't see any CI errors concerning formatting or linting - I'm going to keep on searching

@julien
Copy link

julien commented Jan 15, 2021

@NemethNorbert what's interesting is that the .eslintignore file has an entry for the frontend-js-jquery-web module, so I wonder why it's not ignoring it (.prettierignore doesn't but that's not surprising since it only has to do with formatting)

@NemethNorbert
Copy link
Author

NemethNorbert commented Jan 15, 2021

Hey @julien I mentioned transpiling as our SF rules forces arrow functions on master, that would be a no go in this case.

About the ignore files, that's indeed interesting. At least we know we intended to ignore these files when it comes to linting or formatting

@julien
Copy link

julien commented Jan 15, 2021

@NemethNorbert my bad, I got it all wrong.

If you run yarn checkFormat from the (top) modules directory (which is what CI is doing and not from the modules/apps/frontend-js/frontend-js-jquery-web directory), you'll see that there are no errors related to linting only about formatting (because npm-scripts reads the top .eslintignore file and ignores our frontend-js-jquery-web module as expected).

What I did was run yarn format from the top modules directory and prettier formatted the
/modules/apps/frontend-js/frontend-js-jquery-web/src/main/resources/META-INF/resources/jquery/form.js file like this

diff --git a/modules/apps/frontend-js/frontend-js-jquery-web/src/main/resources/META-INF/resources/jquery/form.js b/modules/apps/frontend-js/frontend-js-jquery-web/src/main/resources/META-INF/resources/jquery/form.js
index 9d9844c52fa6..823d3aa40915 100644
--- a/modules/apps/frontend-js/frontend-js-jquery-web/src/main/resources/META-INF/resources/jquery/form.js
+++ b/modules/apps/frontend-js/frontend-js-jquery-web/src/main/resources/META-INF/resources/jquery/form.js
@@ -244,9 +244,12 @@
                                callbacks.push(function (data) {
                                        var fn = options.replaceTarget ? 'replaceWith' : 'html';

-                                       // Validate `data` through `HTML encoding` when passed `data` is passed
-                                       // to `html()`, as suggested in https://github.com/jquery-form/form/issues/464
-                                       fn == 'html' ? data = $.parseHTML($("<div>").text(data).html()) : '';
+                                       // Validate `data` through `HTML encoding` when passed `data` is passed
+                                       // to `html()`, as suggested in https://github.com/jquery-form/form/issues/464
+
+                                       fn == 'html'
+                                               ? (data = $.parseHTML($('<div>').text(data).html()))
+                                               : '';

                                        $(options.target)[fn](data).each(oldSuccess, arguments);
                                });
@@ -1082,7 +1085,7 @@
                                        $.parseJSON ||
                                        function (s) {

-                                               // Arise an error resolvable including jquery instead of
+                                               // Arise an error resolvable including jquery instead of
                                                // making a new function using unsanitized inputs

                                                window.console.error('jquery.parseJSON is undefined');

So if you do that, commit the change and push, the SF tests should pass

@NemethNorbert
Copy link
Author

Thanks @julien I pushed the SF commit :)

@julien
Copy link

julien commented Jan 15, 2021

ci:test:sf

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:stable - 9 out of 9 jobs passed

✔️ ci:test:relevant - 23 out of 23 jobs passed in 2 hours 39 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 7d640f4b27841520daf470b08dbe8683c2f2e033

Copied in Private Modules Branch:

Branch Name: master-private
Branch GIT ID: 2b89e6be0ae0f6e12dec390930d3e9525006e687

ci:test:stable - 9 out of 9 jobs PASSED
9 Successful Jobs:
ci:test:relevant - 23 out of 23 jobs PASSED
23 Successful Jobs:
For more details click here.

@liferay-continuous-integration
Copy link
Collaborator

All required test suite(s) passed.
Forwarding pullrequest to brianchandotcom.

@liferay-continuous-integration
Copy link
Collaborator

Pull request has been successfully forwarded to brianchandotcom#97915

@liferay-continuous-integration
Copy link
Collaborator

@jbalsas
Copy link

jbalsas commented Jan 21, 2021

Reopening based on Brian's comment

@NemethNorbert I'm not fond this

  • version: 3.51.0-2014.06.20
  • version: 3.51.0.LIFERAY-PATCHED-ISSUE-586
    First, it doesn't work for more than one patch issue. Second, I have no idea what the patch was. If you look at our other patched files in modules/third-party, we use actual patch files.

I'm OK with using a patch file in build.gradle

See archived/opensocial-portlet/patches/LPS-21488.patch

@NemethNorbert, thinking about it, we don't even add form.js by default, so a solution for master could be to simply remove it and document it's no longer included by default and let customers decide if they want it or not.

We will still need to figure out a way to patch it in other branches. I'm not even sure this is actually safe to do based on the Copyright header. Maybe we shouldn't be hosting this at all!

Could you please open an inbound licensing JIRA issue so this can be evaluated?

@jbalsas jbalsas reopened this Jan 21, 2021
@NemethNorbert
Copy link
Author

Reopening based on Brian's comment

@NemethNorbert I'm not fond this

  • version: 3.51.0-2014.06.20
  • version: 3.51.0.LIFERAY-PATCHED-ISSUE-586
    First, it doesn't work for more than one patch issue. Second, I have no idea what the patch was. If you look at our other patched files in modules/third-party, we use actual patch files.

I'm OK with using a patch file in build.gradle
See archived/opensocial-portlet/patches/LPS-21488.patch

@NemethNorbert, thinking about it, we don't even add form.js by default, so a solution for master could be to simply remove it and document it's no longer included by default and let customers decide if they want it or not.

We will still need to figure out a way to patch it in other branches. I'm not even sure this is actually safe to do based on the Copyright header. Maybe we shouldn't be hosting this at all!

Could you please open an inbound licensing JIRA issue so this can be evaluated?

@jbalsas , could you open this licensing ticket and follow up on this? I don't know what type of ticket you are talking about neither what would be the problem I should describe there.

Thank you

@jbalsas
Copy link

jbalsas commented Jan 21, 2021

Hey @NemethNorbert, for external code (3rd party) we should usually file an "Inbound License" type of issue in our "INTERNAL - FOSS Licensing Issues" in JIRA to make sure that including the source code is compatible with our license.

I was worried because I didn't see that it's actually dual licensed as MIT-GPL

Dual licensed under the MIT and GPL licenses.

I think that means it's okay.

As I said, I think we should simply get rid of this library in master.

In 7.3 this is bundled but not available by default and is only added to the page if JQuery is reenabled from Control Panel > System Settings > Third Party. It would be great if for those other versions we could simply wait for Security Fix for Cross-site Scripting (XSS) - huntr.dev to be merged so we can stay on a non-patched version.

If we want to do a quick fix for older versions, I would indeed suggest that we either go the patch route that Brian suggests or absorb the code (if possible). I would ask Legal again to see if this is okay or not.

@NemethNorbert
Copy link
Author

NemethNorbert commented Jan 21, 2021

Hey @NemethNorbert, for external code (3rd party) we should usually file an "Inbound License" type of issue in our "INTERNAL - FOSS Licensing Issues" in JIRA to make sure that including the source code is compatible with our license.

I was worried because I didn't see that it's actually dual licensed as MIT-GPL

Dual licensed under the MIT and GPL licenses.

I think that means it's okay.

As I said, I think we should simply get rid of this library in master.

In 7.3 this is bundled but not available by default and is only added to the page if JQuery is reenabled from Control Panel > System Settings > Third Party. It would be great if for those other versions we could simply wait for Security Fix for Cross-site Scripting (XSS) - huntr.dev to be merged so we can stay on a non-patched version.

If we want to do a quick fix for older versions, I would indeed suggest that we either go the patch route that Brian suggests or absorb the code (if possible). I would ask Legal again to see if this is okay or not.

Hey @jbalsas , I intend to backport this security fix till 7.1. As this issue is a reported security vulnerability, I don't think we can wait for the above mentioned fix to be merged and released (its already sitting there for more than 6months now, while the fix itself is accepted). In that case we would also be looking forward to upgrade our jQuery form.js to the latest version that might present its own set of problems.

I would like to do a quick fix for older versions, could you create a solution for me with the recommended patch route? I don't even know where to start with that. I would take care of all the necessary backports after that.

@jbalsas
Copy link

jbalsas commented Jan 21, 2021

I would like to do a quick fix for older versions, could you create a solution for me with the recommended patch route? I don't even know where to start with that. I would take care of all the necessary backports after that.

Hey @NemethNorbert, we can look at it, but it will take some time since we're pressed on other fronts at the moment.

@NemethNorbert
Copy link
Author

I would like to do a quick fix for older versions, could you create a solution for me with the recommended patch route? I don't even know where to start with that. I would take care of all the necessary backports after that.

Hey @NemethNorbert, we can look at it, but it will take some time since we're pressed on other fronts at the moment.

I really appreciate it @jbalsas
I could also create a quick hotfix with the solution here to the customer, so we are not pushed by that side for now. It would not be the patched direction, but for a quick solution to our customer I think it can be accepted. Do you agree with this approach?
However even with the hotfix provided, we are expected to deal with security issues in a timely manner. I'm not trying to be pushy, just shooting straight.

Thank you for your help

@NemethNorbert
Copy link
Author

hey @liferay-frontend,
Could you let me know what do you think about providing an SDH from the fix that is available on this PR? Also I am following the Draft PR created by Wincent, could you give me an estimate when can we have a final solution?

Thanks,

@wincent
Copy link

wincent commented Feb 5, 2021

@NemethNorbert: I pinged some folks on a thread in the team Slack (private link, but including it here for those who can read it anyway) to get some opinions on whether the draft PR is the best approach (or at least an adequate one); will let you know once we reach a conclusion.

@wincent
Copy link

wincent commented Feb 8, 2021

@NemethNorbert: We concluded that we're just going to fork the project into our monorepo and patch it there. I'm going to do that today, so closing this one for now; I created an issue in the monorepo for you or anybody else who is interested to have something they can subscribe to to track progress on this one.

@wincent
Copy link

wincent commented Feb 8, 2021

See: #772

@wincent
Copy link

wincent commented Feb 11, 2021

@NemethNorbert — Upstream PR got merged: brianchandotcom#98619

Of course, I put the wrong LPS in the commit subject line and PR title (LPS-125853 instead of LPS-125854), just to make traceability interesting 🤦‍♂️...

so-sorry

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants