Project not supported due to security issue

You have to become the maintainer to see the security issue.

https://www.drupal.org/node/251466#procedure---own-project---unsupported

Out of curiosity... anyone have an alternative strategy to getting a json path using feeds due to this module being deemed unsupported?

This is a major issue for us as we can not move off this module, can the maintainer of this module share the information for the community to fix this issue????

Bumping priority to max.

@lathan,

can the maintainer of this module share the information for the community to fix this issue????

The simplest answer is no.

If you need further direction, checkout the #security-questions channel in slack.

It is against security teams policy to share the security issue specifics publicly without going through them first. Check out the documentation they provided in the Security Announcement. Specifically this bit

For all the steps below, if realistic, it is suggested to try to resolve the security issue within 30 calendar days after the Security Advisory (SA) is published on Drupal.org. Because if the security issue is not resolved within this period, the Drupal security team may publish the details publicly. At this point, the module will follow the normal unsupported module policies.

And then the original "Contacted by the Security Team" policy.

As you can see if the maintainers were being responsive, it would be against the rules to provide you with the information you seek.

Also, the reason this module got into this state to begin with is because the maintainers (and all the co-maintainers) didn't respond for at least 6 months after contacted by the security team (and I think policy is to attempt contact 3 times). So for the safety of every site using this module during the transitional time, the security issue will not be disclosed publicly.

I have a preliminary fix for the security issue and will be working with my team to test it over the next few days.

Any news on the (preliminary) fix moving onwards and/or an ETA?

Our agency will have to switch over to https://www.drupal.org/project/feeds_ex shortly, but I much rather save that time/effort if there's any good news. :)

I've posted the patch to the private security ticket so it's just waiting on review/revision. I believe there is a new maintainer in the works for the module so things are moving along.

Great news!

Thanks @kevincooper

Feeds extensible parsers

For anyone that plans to switch to Feeds extensible parsers, I just discovered that the instructions on the project page for getting the library without using Composer - more specifically when you want to use the Peekmo/JsonPath library - requires the dev version of Feeds extensible parsers.
I'd like to create a new 7.x release for it, but I first need to investigate what changes were made since 7.x-2.0-beta2 (they weren't all made by me), so I can write proper release notes.

Upgrade path

Please note that Feeds extensible parsers does not provide an upgrade path from Feeds JSONPath parser yet (and also not from Feeds XPath Parser and Feeds QueryPath Parser).
I would like to add that so we can create a stable release for Feeds extensible parsers and get security coverage for it.

Anyone here who wants to help with creating that upgrade path?
Here is the issue for that: #2376645: Please mark all old project as deprecated and provide an upgrade path

I plan to make a start with it, but since I also have enough on my plate for Feeds and related in D8, it might take me a while if I have to do that alone. I think it's about 8 - 12 hours work. On average, I only have about 4 hours time a week for open source work (with most being in my free time).

Feeds extensible parsers might have the same vulnerability

Please also note that since the details of the vulnerability were not shared with me (yet), Feeds extensible parsers could in theory have the same security issue. So I won't make a stable release until I know at least the details of the vulnerability. I've asked the security team for access on the issue and I'm waiting for their reply. When I get access, I'll also review the patch provided by @kevincooper (unless someone beats me before it).

There have been discussions on Slack lately about this issue, I'll post an export of that here as the discussion may soon vanish on Slack.

#security-questions channel, 2019-11-22

#security-questions channel, 2019-11-28

Weekly Feeds meeting in #feeds channel, 2019-11-28

We use this module heavily across over 100+ D7 sites to map JSON feeds from an event management system to events in our sites. Switching this to a different module would be a big endeavor for us. Fixing the security issue in this module and cutting a release so we could update would be much easier since we don't want to spend much time with D7 and this module pretty much just works.

Thanks for all the work on this issue!

I concur with #14.

Is there any thing publicly available yet with regards to the broader community fixing this issue. I have time available for this issue from my project... just need the details so we can get moving on this.

There is currently nothing public yet.

A fix is underway. Please be patient as is worked through and coordinated.

Thanks to @generalredneck and @megachriz for keeping us all up-to-date :)

Been keeping up with the thread, Thanks for taking a look into this

Thanks kevincooper generalredneck

How are we doing on this issue?

@alysaselby
I asked someone to review the latest patch to find any possible disruptiveness people could have when updating.

We’re also discussing one thing that could break imports: as a result of fixing the security issue, you’ll get additional validation. Currently, it is possible to use invalid JSONPath configuration and let the import continue. The field where this invalid config would be used would now result in an empty value. With the extra validation, an import would fail on invalid config, but it won’t disrupt other Feeds imports. The question is whether or not that is acceptable or that we should deliberately add code to ignore these errors. The latter means that when you’re getting empty results on an import because of a configuration error it would be hard to find the culprit.

I was thinking about maybe add an option for hiding config errors, but if you need to configure that on every importer after updating, I figured you might as well fix the config error.

I was thinking about maybe add an option for hiding config errors, but if you need to configure that on every importer after updating, I figured you might as well fix the config error.

These should be tabled to another issue. We need to focus on getting this fixed. It's been over two months I've had to keep explaining that this patch is coming and there is no progress here to date.

Let's keep the issues addressed in this patch just to the security fix. Expanding this further to other issues out side this scope just delays the progress of this.

@lathan

It's been over two months I've had to keep explaining that this patch is coming and there is no progress here to date.

One issue is that the fix is not so simple and could cause people having issues after updating. So most of the time is now spent on testing to minimize these possible issues. The reason I brought up adding an extra option is to look for ways to mitigate issues people may have after updating.

I agree on your point that expanding things further delays progress, but the alternative is the risk that some people cannot update easily and need to do some extra work to fix their site afterwards.

Thank you for the update on this. Since I am not a developer, merely a site maintainer, I will let the "big dogs" determine the best approach.

As above, so can the security issue be fixed and the upgrade instructions include details of any potential issues users may experience? People can always revert to the old version, but I'd rather have a secure website asap and any additional issues fixed separately. Of course, none of us know what the actual issue is, so that simply may not be possible :)

@dunx
One of the things to test are the update instructions. I identified that people can come from situation x, y or z and the update instructions for each situation are slightly different. I asked someone to check if the instructions are clear for people for each identified situation.

I and the people I asked to test do have a quite busy schedule. I wonder if I should propose to set a deadline for testing (say two to three weeks from now)? The consequence for if testing isn’t done by then is that some people might experience trouble with or after updating: for some situations additional steps are required and if these are skipped, imports won’t start. I think the error that is logged or displayed then should be understandable and tell the user which step he/she missed. If the error message isn’t clear enough, then it can cost people extra time to figure things out. Would that be an acceptable risk?

Of course: while testing, we might identify other issues/regressions as well. The update instructions are not the only thing to test.

Without understanding the risk of the security concern it's difficult to answer as we don't know how risky it is to leave this module installed. At the moment the module is marked as "Project not supported: This project is no longer supported, and is no longer available for download. Disabling everything included by this project is strongly recommended!'

If site admins are on top of things they will have disabled the module or waiting frantically for a fix. Others won't have noticed, won't have done the research and won't even know there's any issue.

Could I suggest pressing on, but including a message displayed during the upgrade to read the release notes if they experience issues after upgrading (for those that don't read them in advance... not me your honour...) and what to do if those issues are experienced.

@dunx
I'm assuming that not everyone will read the release notes. That's why I think it's important to have clear error messages appearing on the site.

About the risk: I wish I could tell you more about this without giving it all away. :(

I understand why you can't say anything, but it makes it impossible for us mere mortals to have any understanding of the risk. It needs to be your call for sure.

I still think a message at upgrade time warning about possible issues and more info in the notes is sufficient, but that's just my opinion.

I'd rather it was secure with possible issues, rather than insecure. On our site it's still running against advice from d.o. Only you know whether that's a bad idea.

I agree with dunx. I too have to keep this module operational and as a site admin - I will read all of the instructions and proceed with caution with respect to implementing the upgrade.

Thanks again for your work on this.

Just a suggestion; if this patch makes a "major update" release (eg v1 > v2) then the SemVer implication is that there you are making incompatible API changes. You should never blindly update a major version without expecting things to potentially break.

Failing that, getting this fantastic work into a 2.x-dev release so we (who have been educated via this thread to expect breakage) can give it a whirl and potentially feedback any critical issues we find to you while your testing team find time to read the docs? Although I do appreciate this may be problematic in terms of the security issues being fixed.

What can we do to help you?

@MegaChris - Any further updates on the testing and was there a deadline proposed? The community at large can also test as well, especially if it is either an alpha or development release. Trust, you'll hear of any issues from the community as well as potential workarounds or fixes; this has been extended so long it's got quite a bit of attention..

@kevgrob
We've proposed a deadline, not sure if we can make that date still. For that I need to try harder to avoid the news, if you know what I mean. I think we all need to do the testing in our free time - I at least. To be honoust, this item is only third on my priority list right now (I'm hoping that others find time to test as else I would review/test my own patch). Multilingual Feeds and the process plugin UI for Feeds Migrate are higher on my list. I'd like to have these two done before Drupal 9 is released, which happens soon. The D8 version of Feeds is also not ready for Drupal 9 yet and cannot be made ready right now because I decided to keep supporting Drupal 8.7 until the next release. And I'm hoping to include the multilingual feature in the next release - which is nearly finished.

So in short, this is taking longer on my side because I'd like to have Feeds and all related modules ready for Drupal 9 in time.

Just to throw this out there... none of us went through the official workflow to be granted maintainership of this module. I started that over here: https://www.drupal.org/project/feeds_jsonpath_parser/issues/3122318. Anyone else willing to take on maintainership is encouraged to post there.

Good things to come with a new release.

So after waiting for a while, we've got a release that reworks the dependencies of the module. Make sure to check out the Project page, the release notes, and updated security advisory. Good luck and post new issues if you have any trouble.

woot woot - just saw that red turn to yellow. Hooray.

I've tested this in production, seems to work well. Thanks for the work on getting a secure release out there!

Yeah for those wondering, The security team only pushes the "security" flag on releases that get an official SA. It's their call. This release due to the amount of time the vulnerability was out (specifically 30 days beyond the "Unspported" date), allows them decide to not apply this flag. Given that, I suspect the fact that it's an already public security issue related to the library we were using, and that we were allowed to push the release on a off day due to my own personal medical issues, they decided to forgo marking this as a security release. The security team was integral to this fix and helping us keep yall safe so I'm ok with their decision.

That said, I can't stress enough that upgrading to 1.2 is important if you plan to stay on this module.

I gave credit in the commit, But Shout out to KevinCooper whom found the issue and a big thank you to MegaChriz who kept the momentum going and held each team member accountable. Without him, Feeds wouldn't be as strong.

Thanks all for the support!

For those who also use Feeds extensible parsers (or switched to that module), this module had the same security vulnerability and new releases have been made for it (on April 9).

https://www.drupal.org/project/feeds_ex/releases/7.x-1.0-rc1
https://www.drupal.org/project/feeds_ex/releases/8.x-1.0-alpha3

Hi MegaChriz, I have updated the module from 7.x-1.0 => 7.x-1.2 and removed the existing jsonpath library and replaced it with the library referenced in the release notes.

TEST: In my case, the site I am working with obtains event node information from the Trumba calendaring system as JSON. I deleted one of the event nodes in Drupal that I verified was in the JSON, and performed the import again, however it doesn't recreate the new record as it did before. If I revert back to the old library and 7.x-1.0 module version, and perform the above test, the node is recreated. Outside of this module, only feeds_tamper is used for modifying content as well as running the hook_feeds_jsonpath_parser_filter function to make a few changes. Any ideas on what could be causing this issue?

@kevgrob
My only guess is that you may have jsonpath configuration that isn’t valid for the new library. The flow/jsonpath library validates your config while the old library ignored config errors and just returned an empty value for those source fields.

You could check your JSONPath configuration with the JSONPath Expression Tester. That tester uses the flow/jsonpath library, though I see it doesn’t provide an option for the latest version of it yet.

You can also check how your source looks like after parsing with the Feeds Import Preview module. This module can sometimes help with finding configuration errors.

@MegaChriz
Thanks for the quick response. I have been poking around the module, as well as a custom module I had written for restructuring the generated array from the JSON, which used the hook_feeds_jsonpath_parser_filter(array &$item, FeedsSource $source) function. FYI: The $item variable in the function declaration, stored a single item in the previous versions, but now it stores all items, so any logic based on parsing the array structure will probably be broken.

Also, while testing in FeedsJSONPathParser.inc, on line 82, $all_items, gets a multidimensional array in the form $all_items[0] = [ 0 => item 1, 1 => item2, 2 => item3 ... ]. In testing if I use the reset function on all_items, to just grab an array of items, vs. an array of array of items, then I can actually start to get the expected nodes to be imported vs the message "There are no new nodes." It would appear there are various calls expecting one object ($item), but they are receiving multiple, e.g. protected function parseSourceElement($item, $query, $source) on line 170, should only receive an array with one item, but executing the following dpm($item), it shows all items.

Not sure what others are getting, but it's not even pulling down any nodes for me without changes to the code.

@kevgrob
Let's please follow direction in #38 and make a new issue. Feel free to link it to this one as related. This one is about status on the security issue which is fixed.

That said... thanks for taking such a detailed look into the code. While I'm out on paternity leave I would appreciate any further help you are willing to give.

Shocking that the people in this thread with the knowledge of this issue, would not release the security breach for the community to fix.

@lathan
What do you mean? A new release with the security fix was made over a month ago! :)
The security issue was in the old library and we fixed that by removing support for that library (goesner/jsonpath) and adding support for a different library instead: flow/jsonpath.

It was a community decision and the security teams right to not post a security issue publicly. By not releasing it publicly, we were following the regulations installed by the security team. The Security team did not make the issue public on their own so we had no right to make it public ourselves per policy. Not that I"m throwing this on their shoulders, but this is the way it happens for every security bug for every project that you see on wednesday afternoons. If you have further questions about the security team policy around publicly announcing security issues, please check out the documentation in #2, #7 and here: https://www.drupal.org/security-team/report-issue. It may help you clear up some of shock you are feeling as you may understand we were following the rules.

if you have more questions about the process and reasoning for keeping details hush for coordinated releases, feel free to talk to the security team in #security-questions in slack.

But as MegaChriz pointed out 1.1 was the security release on April 7th and mentioned in #37. Check out the following comments to learn about the security team's decision to not flag it "security"

@generalredneck none the less we maintain some high profile sites that can not simply jump ship from this module. We have asked multiple times for any info and were told to wait and so we did. The suggestion to simply move to another module is not feasible in our circumstance. As the sites we inherited were written by php developers with very poor knowledge of Drupal. This module is "hardwired" into 90% of the sites.

We are in the process of moving to D8. Yet have a number of sites still waiting their turn to make it on to that stack.

Leaving this security whole open, and not disclosing the issue to the border community even if they have the time and resources to fix this issue. Simply is disgusting.

Im sitting in meetings with clients asking me how to fix this, and I have no answer. As I don't event know the issue, so all I can tell them is their sites are at risk because the Drupal Community and guidelines.

A) fail to release details on the issue, so we can fix it.
B) have ridiculous rules around security issues.

So here we sit with our hands tied behind our backs, having a client paying 10,000's of dollars for a website we are unable to maintain and fix security issues because of 'politics'.

Hopefully the people who decide on these guidelines will fit the bill when we loose this client because of their incompetence.

The fix has been released and is available in all releases of feeds_jsonpath_parser >= 7.x-1.1.

The people working on this issue have gone above and beyond what is expected of anyone working in a volunteer capacity on an open source project.

I applaud them for their discretion on the issue while they worked towards a fix.

I urge you to upgrade to 7.x-1.2.

So here we sit with our hands tied behind our backs, having a client paying 10,000's of dollars for a website we are unable to maintain and fix security issues because of 'politics'.

Sitting on your hands is untrue. If a person chooses to... well yeah. Those of us whom get the security risk info go through the regulations (that keep sites safe by not disclosing the issue) to gain access to the security issue. Additionally, this security issue was also found by a fellow Drupal developer just like you. So it's not beyond the realm of possibility (and one of the several ways to obtain access to the security issue) to find it (or another one) and contribute.

That said, I can't tell if you are still discussed because we took some time to update the module between the security advisory and April or if you still think that there hasn't been a fix?

There has and it was released in April. Its not to move to another module... it's to update to version 1.2. If you want further support we are suggesting you change modules as this one was marked as minimally maintained back in 2015.

That said, I appreciate your candid feedback, but the rules are put in place to keep the community safe. Not to make it easy.

I highly encourage you to walk through the steps to contribute to rescuing an unsupported module in the future and put your paying clients money to good use. This is my 3rd module.

As for what to tell your clients. If they want to know what the risk is, you can tell them its unsupported due to the unresponsiveness of the original maintainers and ask if they would like you to pursue you finding/fixing the issue and/or taking over the module (because that's exactly what I did) and be honest with them... let them time box it... if you didn't find a solution encourage them to spend the money to move off of it and make sure they know the risks of staying on it. If they make the call to stay on it... you did your duty and you can take it to your own time (like we did) to finish fixing it or leave it to others.

Something you can do though... if 30 days pass beyond the unsupported security announcement in the future... hit up the security team and ask for a public issue to be made.

I hope this all helps.