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.

Sign up for GitHub

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

Jump to bottom

FIX IntegrationTestTrait failing with FormProtector when debug disabled #17490

Open
wants to merge 1 commit into
base: 5.x
Choose a base branch
from
Open

FIX IntegrationTestTrait failing with FormProtector when debug disabled #17490

wants to merge 1 commit into from

Conversation

kolorafa
Copy link
Contributor

When doing a test post request to a endpoint with enabled form protector while having debug disabled, then the form protector will always fail.

Example of failing test:

    public function testFormProtector() {
        Configure::write('debug', false);
        $this->enableSecurityToken();
        $this->enableCsrfToken();
        $this->post('/path/with/enabled/form/protector`, ['some' => 'data']);
        $this->assertResponseCode(302 /** or any other code like 200 */ );
    }

The test result:

1) App\Test\TestCase\Service\User\UserLoginTest::testFormProtector
Failed asserting that `302` matches response status code `400`.

The culprit code that is failing the request:

        if (!Configure::read('debug') && isset($formData['_Token']['debug'])) {
            $this->debugMessage = 'Unexpected `_Token.debug` found in request data';

            return null;
        }

If the debug is disabled then the debug field should not be set.

But not only FormProtector::buildTokenData is always setting the debug, but on top of it the IntegrationTestTrait doesn't care about debug and always additionally set/override this variable.

When printing the form, this field in FormHelper is skipped/not included if not in debug.

@dereuromark
Copy link
Member

Is there a use case of running tests without debug mode? I wasnt aware of that.

@dereuromark dereuromark added this to the 5.0.4 milestone Dec 20, 2023
@kolorafa
Copy link
Contributor Author

kolorafa commented Dec 20, 2023 •
edited
Loading

In my case, I want to test the production error page, yes I could make a hidden page that is somehow disabling FormProtector on that request and/or manually trigger showing error page then test it that way.

For example if the FormProtection kicks in, I was thinking of making a test to verify that it looks correct and show what's expected, especially not the debug page.

I'm also reviewing the currently failing test, in \Cake\TestSuite\IntegrationTestTrait::_addTokens the code adds those fields essentially unlocking all provided data, so I'm thinking how it was failing before if I only removed the debug from the request that was not supposed to be there in the first place based on the FormProtector

@kolorafa
Copy link
Contributor Author

And also all my tests in CI/CD run with debug set to false (probably by accident, but that reflects more the way it's going to run in production)

@kolorafa
Copy link
Contributor Author

Is there a use case of running tests without debug mode? I wasnt aware of that.

Do I understand it correctly, that the cakephp CI/testsuite also run with debug mode not enabled?
As my "remove debug field" should only kick in when it's set to false.
And the line I removed is just putting some random text, not valid protection token ?

Or I'm missing something in those test cases?

Thanks

markstory
markstory reviewed Dec 20, 2023
View reviewed changes
Copy link
Member

@markstory markstory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good to me. It would be good to have a test added so that we don't regress this in the future.

@kolorafa
Copy link
Contributor Author

I can add test for that case over the weekend.

@ADmad
Copy link
Member

ADmad commented Dec 21, 2023

There's also a failing test which seems related to this change.

@kolorafa
Copy link
Contributor Author

kolorafa commented Dec 21, 2023 •
edited
Loading

The failing test is also very strange, as I don't understand how that test could pass in the first place, by the logic if you don't provide unlocked field, the logic automatically is unlocking all fields provided as input, because in \Cake\TestSuite\IntegrationTestTrait::_addTokens if you don't set the second $lock it's actually triggering unlockField on those fields, so by logic both testPostSecuredFormUnlockedFieldsFails and testPostSecuredFormUnlockedFieldsWithSet should result in the same request, so I don't know how the first one did fail with error 400 before, unless I'm not seeing something (as I didn't debug it yet).

Can look more into it over weekend.

@markstory markstory modified the milestones: 5.0.4, 5.0.5 Dec 28, 2023
@markstory markstory modified the milestones: 5.0.5, 5.0.6 Jan 28, 2024
@markstory markstory modified the milestones: 5.0.6, 5.0.7 Mar 9, 2024
@markstory markstory modified the milestones: 5.0.7, 5.0.8 Apr 6, 2024
@markstory markstory modified the milestones: 5.0.8, 5.0.9 May 11, 2024
@markstory markstory modified the milestones: 5.0.9, 5.0.10 Jun 24, 2024
@markstory markstory modified the milestones: 5.0.10, 5.0.11 Jul 28, 2024
@markstory markstory modified the milestones: 5.0.11, 5.1.0, 5.1.1 Sep 13, 2024
@markstory markstory modified the milestones: 5.1.1, 5.1.2 Oct 4, 2024
@markstory markstory modified the milestones: 5.1.2, 5.1.3 Nov 10, 2024
@dereuromark
Copy link
Member

Ping @kolorafa

@markstory markstory modified the milestones: 5.1.3, 5.1.4 Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markstory markstory markstory left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
5.1.5
Development

Successfully merging this pull request may close these issues.
4 participants
@kolorafa @dereuromark @ADmad @markstory