Problem/Motivation

This is the default REST resource configuration:

langcode: en
status: true
dependencies:
  module:
    - basic_auth
    - hal
    - node
id: entity.node
plugin_id: 'entity:node'
granularity: resource
configuration:
  methods:
    - GET
    - POST
    - PATCH
    - DELETE
  formats:
    - hal_json
  authentication:
    - basic_auth

If you read it, you'd think it result in

  1. a Node REST resource being provisioned
  2. which is accessible via the GET, POST, PATCH and DELETE methods
  3. can be read/modified in the application/hal+json format
  4. and requires HTTP Basic Authentication

This is almost correct. The gotcha is in point 4: anybody is always allowed to use the cookie authentication mechanism, because it is a global authentication provider
This is correct! See #12.

Proposed resolution

Either:

  1. Modify the meaning of _auth: only things listed there are allowed, global authentication providers are not also allowed (global authentication providers then only apply to routes without the _auth route option)
  2. Introduce a new _auth_only route option that exhibits the above behavior, and modify \Drupal\rest\Routing\ResourceRoutes::getRoutesForResourceConfig() to use that route option instead

Add explicit test coverage proving this.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#12 2817745-12.patch2.99 KBwim leers

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers’s picture

Priority: Normal » Major
Issue tags: +Security improvements

Actually, since this affects security, I think this is major.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

I agree, being more explicit is always a win. Changing the behaviour of existing configuration keys is tricky though. If you are honest about things, even configuration is sort of an API. Can we change it?

wim leers’s picture

Maybe it's once again possible to provide an upgrade path to ensure consistent behavior. We managed to do #2308745: Remove rest.settings.yml, use rest_resource config entities without breaking BC (i.e. without breaking configuration, i.e. without changing the behavior that depends on configuration — behavior was the same before and after). So perhaps we can find a way to do so here too.

dawehner’s picture

Maybe it's once again possible to provide an upgrade path to ensure consistent behavior. We managed to do #2308745: Remove rest.settings.yml, use rest_resource config entities without breaking BC (i.e. without breaking configuration, i.e. without changing the behavior that depends on configuration

Well I'm actually referring to the config structure itself. You cannot really provide a BC layer I guess unless you override the config object.

wim leers’s picture

I meant in #6 that #2308745: Remove rest.settings.yml, use rest_resource config entities also changed the config structure itself. We managed to provide an upgrade path for that. Hence why I think it's once again possible to provide an update path to ensure consistent behavior.

wim leers’s picture

To be clear: in the update path, we'd automatically add all global authentication providers (cookie) to the configuration, to ensure we don't break existing sites.

wim leers’s picture

#2808233: REST 403 responses don't tell the user *why* access is not granted: requires deep Drupal understanding to figure out landed recently. Which included the interdiff at #2808233-84: REST 403 responses don't tell the user *why* access is not granted: requires deep Drupal understanding to figure out. That added the following test coverage to \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase::testGet():

    $request_options[RequestOptions::HEADERS]['REST-test-auth'] = '1';

    // DX: 403 when attempting to use unallowed authentication provider.
    $response = $this->request('GET', $url, $request_options);
    $this->assertResourceErrorResponse(403, 'The used authentication method is not allowed on this route.', $response);

    unset($request_options[RequestOptions::HEADERS]['REST-test-auth']);

IOW: this already added some of the test coverage we need. But it's testing a non-global authentication provider What this issue is about, is ensuring that even global authentication providers are treated like this.

wim leers’s picture

Title: A REST resource's "auth" configuration is highly confusing: global authentication providers like "cookie" are allowed even when not listed » Add test coverage to prove that REST resource's "auth" configuration is also not allowing global authentication providers like "cookie" when not listed
Category: Bug report » Task
Priority: Major » Normal
Issue summary: View changes
Status: Active » Needs review
Issue tags: -DrupalWTF, -Security improvements, -Needs update path, -Needs update path tests
StatusFileSize
new2.99 KB

I worked on test coverage today to prove that global authentication providers are always allowed, even when they're not explicitly listed in a RestResourceConfig entity's configuration. Not being listed there causes it to also not be listed in a REST route's _auth option.

But… it turns out that actually the configuration is being respected after all! Did I really make the wrong observation way back when I opened this issue?

The ultimate test:

  1. Install D8 Standard install profile.
  2. Create an article: /node/1
  3. Install the HAL, Basic Auth and REST modules.
  4. While logged in, try GET http://d8/node/1?_format=hal_json. You get an error response.
  5. Now modify rest.resource.entity.node, and add cookie to the auth key (by default, only basic_authis listed).
  6. While logged in, try GET http://d8/node/1?_format=hal_json. You get a HAL+JSON response.

Tested this with both 8.2.6 and 8.3.x. It works as expected.

In other words:

  1. this is not a WTF
  2. this is not a security improvement
  3. this is already working as expected!
  4. hence this is a normal task, not a major bug
  5. this does not need an upgrade path
  6. … all this needs, is test coverage to prove it, to ensure we don't ever introduce a bug that changes this :)
tedbow’s picture

Status: Needs review » Reviewed & tested by the community

The test looks good. Into it and it is the correct error message for the authentication not being applicable for the route.

RTBC!

  • cilefen committed d20187f on 8.4.x
    Issue #2817745 by Wim Leers: Add test coverage to prove that REST...
cilefen’s picture

Committed d20187f and pushed to 8.4.x. Thanks! We are entering a commit freeze so we can cherry-pick this to 8.3.x as early as Thursday.

  • cilefen committed d9be246 on 8.3.x
    Issue #2817745 by Wim Leers: Add test coverage to prove that REST...
cilefen’s picture

Status: Reviewed & tested by the community » Fixed

Cherry-picked as d9be246 and pushed to 8.3.x. Thanks!

wim leers’s picture

Thanks!

Status: Fixed » Closed (fixed)

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