An infinite loop in ViewsJoin could cause denial of service

Thanks a ton for creating the issue!

This is the equivalent of the D7 fix for D8. The failing test simulates a missing join.

+++ b/core/modules/views/tests/src/Unit/ManyToOneHelperTest.php
@@ -0,0 +1,94 @@
+    $join->leftTable = 'left_table';

What about calling it "non_existing_left_table". Given that should we also test what happens on a non existing right_table?

@dawehner yeah that makes sense. Changed the wording.

Adding a test for the right_table seems out of scope here, fancy enough to get some explicit testing for ManyToOneHelper as it is.

Thank you @Lendude, especially for pinging me!

How does the infinite loop occur? The test-only patch errors on trying to get a property of a non-object. Are we just hiding an error with this change?

Setting back to "needs review" so someone can answer #9.

@alexpott you are right on both counts, we were just hiding the error and the test scenario wasn't the correct one to trigger the infinite loop.

Added a test for the infinite loop and instead of just quietly passing the error by, we now throw an exception when this happens, after all there is something seriously wrong when you run into this. But maybe that's a bit of overkill?

To test if we are inside a loop I've taken a page from \Drupal\views\Plugin\views\query\Sql::ensurePath which already protects against that.

This is indeed much better!

+++ b/core/modules/views/src/Exception/InvalidViewsDataException.php
@@ -0,0 +1,10 @@
+/**
+ * Thrown when invalid views data is encountered.
+ */
+class InvalidViewsDataException extends \Exception {

Can we document somehow how to resolve this for people? Like a table pointing to itself?

@dawehner yeah that makes sense, something like this?

Re-rolled #13 for 9.4.

+++ b/core/modules/views/src/ManyToOneHelper.php
@@ -57,10 +58,19 @@ public function getField() {
+   * @param null $join
+   * @param null $alias
+   *
+   * @return string

These need to confirm to our documentation standards. So we need a comment explaining each one. Also I think $alias is a string and $join will be a \Drupal\views\Plugin\views\join\JoinPluginInterface

Resolved CS issues

Attached patch against #23

Attached patch against #26

This should fix some of the fails and get rid of the unrelated changes.

Updated the IS a bit

This doesn't apply cleanly to 10.1.x, so we'll likely need two patches.

There seems to be a fair bit of duplication between the three test methods, especially around the handler/relationship setup - perhaps we could refactor some of that into a helper method to make it easier to maintain, as we're already at the rule of three here.

But other than that, the code looks good to me.

Moved to MR and rerolled

Hiding patches for clarity.

Started the issue summary update but not as familiar with the issue so left TBD in sections that I couldn't answer. Is the proposed solution correct? Steps to reproduce?

Updated the IS

Thanks. Not sure because this is testing an infinite loop but when I tried to run the test-only feature it got hung up but did see an "E" in the output to show the failure.

So clearly test coverage is there haha.

Looking at the code change seems to appropriately catch the issue and throws the new exception.

Marking it but do new exceptions need a CR?

I've added some comments to the MR.

Addressed feedback. Not sure if the minor scope creep for php-stan is better than updating the base line and fixing it later, but it seems pretty minor so went for the fix

Feedback around the return null appears to have been addressed.

I'm triaging RTBC issues. I read the IS and the comments and the MR. I didn't find any unanswered questions or other work to do.

However, I left one small suggestion for a comment in the MR. So setting back to NW for someone to check that suggestion.