Problem/Motivation

I set an allowed route to /user/ and didn't set any URL parameters. Once logged in, it gave me a 403 error because the url of the page displayed is /user/42?check_logged_in=1 (...with a query string).

Steps to reproduce

  1. Install with composer and enable version 2.0.0-beta1 on Drupal 10
  2. Add a new restricted route with these parameters
    • Route name or path: /user/
    • Request methods: GET, POST
    • URL parameters: Leave it empty
    • Ips: Enter your ip
    • Operation: Allow access
    • Check Enabled
  3. In your browser, open a new private page and go to /user/login of your site. You should see it.
  4. Log in
  5. You should have a 403 error if there's a quey string in the url.
    • Otherwise, go to /admin/people and edit any user. The url should be /user/%/edit?destination=/admin/people and you should have a 403 error.

Proposed resolution

Logically, if I don't set any URL parameters, it should accept any query string passed to the URL. And looking at the code I think that's what it was supposed to do, but a logic error broke the condition.

I believe the problem comes from the condition present on line 256 of RestrictIpService.php : if (!$params && !$query_string) {

In fact, it could be if (!$params || ($params && !$query_string)) { to check if URL parameters is not set, and otherwise if it's the case, to check if a query string is not passed to the page.

In addition, none of this code has test coverage, so that should be added.

Remaining tasks

Review the submitted merge request.

CommentFileSizeAuthor
#2 restrict_route_by_ip-3405569.patch544 bytesfranckylfs

Issue fork restrict_route_by_ip-3405569

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:

About issue forks

Comments

FranckyLFS created an issue. See original summary.

franckylfs’s picture

franckylfs commented
StatusFileSize
new restrict_route_by_ip-3405569.patch544 bytes
solideogloria’s picture

solideogloria commented
Status: Active » Needs work

if (!$params || !$query_string) { is exactly the same as if (!$params || ($params && !$query_string)) {, so that would be shorter if it works.

solideogloria opened merge request !4

solideogloria’s picture

solideogloria commented
Status: Needs work » Needs review

Please review the merge request. It works the same way you described, and it fixes the issue for me.

solideogloria’s picture

solideogloria commented
Priority: Normal » Major

Marking this issue as Major, since it would prevent most potential uses of this module altogether.

franckylfs’s picture

franckylfs commented

Thanks! I approve, but I think I can't do it on git.drupalcode.org. Sorry :(

solideogloria’s picture

solideogloria commented
Status: Needs review » Reviewed & tested by the community

A maintainer has to do that part.

solideogloria’s picture

solideogloria commented
Assigned: Unassigned » solideogloria
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Working on creating tests.

solideogloria’s picture

solideogloria commented
Status: Needs work » Needs review

The requestAffected() function didn't have any tests for it at all! I added test coverage for the entire function, not just for the changed functionality.

It's a good thing I spent time last week reading about how phpspec/prophecy works, because the module's tests use it a lot.

solideogloria’s picture

solideogloria commented
Title: Route allowed without specified parameter is blocked when a parameter is passed in the url » Route allowed without specified parameter is blocked when a parameter is passed in the URL. Add test coverage as well.
solideogloria’s picture

solideogloria commented
Issue tags: -Needs tests
solideogloria’s picture

solideogloria commented
Related issues: +#3447382: Add .gitlab-ci.yml

Adding as a related issue, since I fixed its failing phpunit test here.

solideogloria’s picture

solideogloria commented
Issue summary: View changes
solideogloria’s picture

solideogloria commented
Title: Route allowed without specified parameter is blocked when a parameter is passed in the URL. Add test coverage as well. » Allow if no params in URL or none in configuration. Add test coverage as well.

Shorter title.

solideogloria’s picture

solideogloria commented
Assigned: solideogloria » Unassigned
solideogloria’s picture

solideogloria commented

Still waiting on review.

tyfoo’s picture

tyfoo commented

I just bumped into this issue as well. At least for me, leaving the params option empty should mean that we don't care about matching the query string.

So instead of 

if (!$params && !$query_string) {
...
}

We could just have

if (empty($params)) {
...
}

What do you think?

We are also trying to restrict the /user/ route, but URLs like /user/1/edit?destination=/admin/people aren't working correctly because of the query string.

solideogloria’s picture

solideogloria commented

Did you try the patch from the MR? The MR uses if (!$params || !$query_string) {, which should also work.

tyfoo’s picture

tyfoo commented

Yeah, the MR does work for my case. I wonder about the logic still, though. If a user specifies params, they probably want to make sure the request params match right?

!$params || !$query_string would work for cases where no params were specified, but also where params were specified but no query string is present.

What do you think? I could see the argument either way, really.

solideogloria’s picture

solideogloria commented

Same. I don't have that use case personally, so I don't really know either way, to know which is better.

I think that argument makes sense, though. The tests will also need to be updated. Specifically this part, since we would be changing it to not be affected if there were no query params given but the route restriction has query params specified:

    $this->request->getMethod()->willReturn('GET');
    $request_affected = $this->restrictIpService->requestAffected();
    $this->assertEquals(TRUE, $request_affected, 'Request affected for method match and empty params.');
solideogloria’s picture

solideogloria commented

Review the changes now. I added more tests for more use cases.

solideogloria’s picture

solideogloria commented

The changes here will not apply once #3524482: 2.0.x Drupal 11 compatibility. is merged. It will need to be rerolled.

However, that issue is for D11 compatibility, so please review and test it, and then we can reroll this issue.

solideogloria’s picture

solideogloria commented
Related issues: +#3524482: 2.0.x Drupal 11 compatibility.

Comment removed. It was for another issue.

solideogloria’s picture

solideogloria commented
Version: 2.0.0-beta1 » 2.0.0-beta2
very_random_man’s picture

very_random_man commented
Status: Needs review » Reviewed & tested by the community

I was having a problem where GET parameters were resulting in 403 when they weren't explicitly specified in the config. The MR fixes this.

Thanks!

solideogloria’s picture

solideogloria commented

@very_random_man Please also review #3524482: 2.0.x Drupal 11 compatibility. if you can.