Issue fork webform-3269936

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

cilefen created an issue. See original summary.

cilefen’s picture

Status: Active » Needs review
feyp’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @cilefen. The MR looks good and works well for me. All tests pass. I'll go ahead and RTBC this.

I think we should create a follow-up issue to discuss how to solve this issue in a more general way. I'd like to highlight a comment by @xjm made in a discussion in the #security-questions Slack channel:

The whole pinning to a specific version in contrib is not recommended and sort of bends security team policy if it prevents the site owner from updating the library

I don't think we should block on that for this issue, since this is a major issue blocking people from applying the security update right now and I think it would need some thought on how to best approach this, but I definitely think we should look into this. If we can't get rid of the pinning to a specific version we'll have the same issue again sooner or later. If that was the case, the whole Composer merge plugin stuff might not be the right approach to maintaining libraries after all and we might need to remove support for this feature entirely or at least stop recommending it as a possible way to maintain libraries in favor of one of the other solutions already provided by Webform module.

cilefen’s picture

You took the words right out of my mouth about needing a conversation about this. I think the instructions about Webform posted in https://www.drupal.org/sa-core-2022-005 are incorrect and do not work in these cases. It should actually only link to Webform's instructions page.

feyp’s picture

I think the instructions about Webform posted in https://www.drupal.org/sa-core-2022-005 are incorrect and do not work in these cases. It should actually only link to Webform's instructions page.

I agree. The instructions might work, if you already set up library management in a certain way, but it didn't work for our setup for example. Since the command will probably return a success message or something, users might think they fixed the vulnerability when in fact they didn't. The documentation page has the whole picture and I think just linking to that is a good idea.

taran2l’s picture

hi @jrockowitz, I think it's time for a more elegant solution ... there were discussions #3088336: Is composer.libraries.json compatible with the Composer path repository configuration? and #2912387: Stop using wikimedia/composer-merge-plugin before.

Let's create a followup

cilefen’s picture

A fundamental problem is that these libraries have to be downloaded from a specific url which contains a version number. So any solution has to solve that.

taran2l’s picture

I would go with a separate optional module with own composer.json specifying libraries from assets.packagist.org meaning that it MUST be added to the root composer.json beforehand. If @jrockowitz believes in the idea, I can contribute

cilefen’s picture

Why would it necessarily have to be added to the root composer.json?

taran2l’s picture

taran2l’s picture

Why would it necessarily have to be added to the root composer.json?

Those JS packages do not exist in packagist.org / Drupal packagist. Composer won't be able to find them.

jrockowitz’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.