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

Reverting to previous versions of parsedown and parsedown-extra #1921

Closed
gizmecano opened this issue Mar 15, 2018 · 17 comments
Closed

Reverting to previous versions of parsedown and parsedown-extra #1921

gizmecano opened this issue Mar 15, 2018 · 17 comments
Labels

Comments

@gizmecano
Copy link

Due to the recent problems encountered with the new security release of parsedown and parsedown-extra (which introduce some errors in Grav, such as breaking markup for image captions or footnotes), I wonder if it would not be better to revert temporarily (just during the period this bug is not properly resolved) to an efficient previous version (by simply updating the dependencies in composer.json and rebuilding the composer.lock accordingly).

This is more a question than a real suggestion: I don't know all the possible interdependencies, but as the time to solve the problem is unknown and the previous versions were functional, I suppose this little change could be potentially considered.

@rhukster
Copy link
Member

rhukster commented Mar 15, 2018

The problem is that i'm not sure these are considered 'bugs' for the Parsedown library, rather they are exploiting loopholes in the previously unsecured Parsedown handling of elements.

I doubt that there will be a fix in the Parsedown library as any fix, just re-exposes those vulnerabilities. It probably makes more sense to simply move to another solution rather than using markdown based ones.

For example, use image-captions for image captions.. as this still works fine with new Parsedown security fixes.

@gizmecano
Copy link
Author

It probably makes more sense to simply move to another solution rather than using markdown based ones.

I can understand that it would be possible in some cases, but as I specifically chose Grav for its processing using markdown syntax, and more especially for its support of footnotes with the help of smartypants plugin, this problem seriously impact my current project.

I guess that one of the possible manners for me would be to specifically downgrade some of the parsedown components on my own installation, or even to entirely revert to a previous version of Grav where this problem had not yet appeared.

Unless you think that opening an issue on the smartypants plugin directory may, by any chance, have a little possibility of solution?

@rhukster
Copy link
Member

You don't need to revert Grav, or change anything manually. You simply change the Parsedown version to the one before the security fixes (1.6.4) in composer.json and composer update --no-dev to install it. You will have to do this each time you upgrade Grav though, as Grav is bundling the latest stable version.

I can't in good conscience force an older version of Parsedown in Grav core if it exposes security vulnerabilities for the sake of a few 3rd party plugins. It sucks, sure, but you can't mess around with security.

@rhukster
Copy link
Member

You would be better off opening an issue on Parsedown repo, and perhaps ask for an option to disable this security validation if you wish to do so for compatibility reasons. That way you could set an option for markdown, and still be able to stay on the latest version for other features and bugfixes.

@rhukster
Copy link
Member

Actually looks like there are some fixes already in motion for footnotes at least: erusev/parsedown-extra#118

@gizmecano
Copy link
Author

You simply change the Parsedown version to the one before the security fixes (1.6.4) in composer.json and composer update --no-dev to install it.

I have tried to do this, but due to composer.lock file content, this process fails to correctly revert to a previous version. Maybe I made something wrong: I think I have to try it again.

I can't in good conscience force an older version of Parsedown in Grav core if it exposes security vulnerabilities for the sake of a few 3rd party plugins.

I totally understand this point of view, of course. It is exactly why I wrote before this issue seems to be rather a question than an actual suggestion.

You would be better off opening an issue on Parsedown (...)

As there is yet an issue devoted to this (footnotes) problem, I think I will first subscribe to it in order to get a better view of the problem.

(...) ask for an option to disable this security validation if you wish to do so for compatibility reasons

This option would sound like a reasonable proposal indeed.

Thank you for the time spent responding to this thread.

@gizmecano
Copy link
Author

You simply change the Parsedown version to the one before the security fixes (1.6.4) in composer.json and composer update --no-dev to install it.

After changing the version value to 1.6.4 and trying to update dependencies with update --no-dev command, I got now the following error message :

Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - erusev/parsedown 1.6.4 conflicts with roave/security-advisories[dev-master].
    - erusev/parsedown 1.6.4 conflicts with roave/security-advisories[dev-master].
    - Installation request for erusev/parsedown 1.6.4 -> satisfiable by erusev/parsedown[1.6.4].
    - Installation request for roave/security-advisories dev-master -> satisfiable by roave/security-advisories[dev-master].

@gizmecano
Copy link
Author

I wonder if it could be related to this problem, where it is said that composer update command "is now broken for everyone using roave/security-advisories".

@mahagr
Copy link
Member

mahagr commented Mar 16, 2018

In order to use any vulnerable libraries, you need to disable roave/security-advisories. It's there to make sure that you cannot install known vulnerable libraries.

There is no ill effect removing the dependency, except that you will not get informed about vulnerabilities anymore.

@gizmecano
Copy link
Author

So, as stated by @rhukster, who has suggested to downgrade to erusev/parsedown 1.6.4 to resolve the problem, and according to @mahagr, who has indicated that roave/security-advisories can be removed to correctly proceed, I have finally succeed to revert to a dependancies situation where footnotes are no longer broken.

Thanks to all.

@jeremycherfas
Copy link

Any chance of an actual step-by-step of the process, for those of us who are mere users of Grav, rather than developers. I don't mind going into the Terminal, but it would be very useful to have a detailed list of what to do.

@rhukster
Copy link
Member

rhukster commented Mar 20, 2018

Sure:

  1. Edit composer.json in the root of Grav
  2. Remove the security checking dependency:
"roave/security-advisories": "dev-master",`
  1. Edit the parsedown entry to force the last pre-security fixed version:
"erusev/parsedown": "1.6.4",
  1. From terminal run composer update --no-dev

NOTE: If you don't have composer installed, you can use the one bundled with Grav:

bin/composer.phar update --no-dev

Job done.

@jeremycherfas
Copy link

jeremycherfas commented Mar 20, 2018 via email

@jeremycherfas
Copy link

jeremycherfas commented Mar 21, 2018 via email

@rhukster
Copy link
Member

rhukster commented Apr 3, 2018

There's actually a fix in Parsedown dev-master branch that addresses this issue. I've committed a fix using this version but will change it back to a proper version when properly released.

@newmedicine
Copy link

Sorry not quite clear @rhukster does this mean "wait a bit longer an all will be well"?

@rhukster
Copy link
Member

rhukster commented Apr 9, 2018

The fix is in Parsedown 1.8.0-beta1: https://github.com/erusev/parsedown/releases

So the next update of Grav with have the fix.

rhukster added a commit that referenced this issue Apr 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants