getWorkspaceId() contains this todo


  /**
   * {@inheritdoc}
   */
  public function getWorkspaceId(Request $request) {
    $workspace_id = $request->query->get('workspace') ?: NULL;
    if (!$workspace_id && isset($_SESSION['workspace'])) {
      // @todo Review from a security perspective.
      $workspace_id = $_SESSION['workspace'];
    }
    return $workspace_id ?: $this->container->getParameter('workspace.default');
  }

CommentFileSizeAuthor
#41 do_security_review_of-2597464-41.patch44.5 KBtimmillwood
#36 2597464-post-not-get_36.patch42.7 KBjosephdpurcell
#35 2597464-post-not-get_31.patch40.94 KBtimmillwood
#31 interdiff-26-31.txt11.75 KBjosephdpurcell
#31 2597464-post-not-get_31.patch40.94 KBjosephdpurcell
#26 interdiff-25-26.txt10.69 KBjosephdpurcell
#26 2597464-post-not-get_26.patch31.23 KBjosephdpurcell
#25 interdiff-16-24.txt8.48 KBjosephdpurcell
#25 2597464-post-not-get_25.patch27.27 KBjosephdpurcell
#16 interdiff-2597464.txt7.02 KBCrell
#16 2597464-post-not-get.patch19.46 KBCrell
#11 interdiff-2597464.txt1.8 KBCrell
#11 2597464-post-not-get.patch13.02 KBCrell
#5 2597464-post-not-get.patch12.33 KBCrell
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

stevector created an issue. See original summary.

dixon_’s picture

Title: Dos security review of getWorkspaceId() » Do security review of getWorkspaceId()
Priority: Normal » Critical

Bumping as this is security related.

Crell’s picture

My take:

1) workspace needs to be made a global cache context. Which is something you have to do manually in sites/default/services.yml. It's easy, but needs to be documented.

2) GET parameters that change state are totally no. :-) Even if it's just session state.

3) How bad this is depends on how much other workspace access control there is in the system.

I will be working to change this to a POST anyway as part of #2658400: Switching workspaces, per request from Tim.

Crell’s picture

Assigned: Unassigned » Crell

Correction, I'm working on that switch for this issue now. :-)

Crell’s picture

Work in progress. This is also available in my sandbox for merging if you are so inclined: https://www.drupal.org/sandbox/crell/2663020

This replaces the list of links with a list of buttons, each of which is a tiny form. Submitting that form will switch the active workspace. I also am disabling the query parameter switching entirely.

Still todo:

1) Figure out why the layout sucks and my CSS to fix it isn't getting picked up.
2) Switch away from using $_SESSION to a non-super-global-based approach. This could use a little discussion to decide how to force a workspace. Just calling a method on the negotiator seems weird and hard-code-y, but that may be the best option available. Tim, Dixon, thoughts?

Status: Needs review » Needs work

The last submitted patch, 5: 2597464-post-not-get.patch, failed testing.

The last submitted patch, 5: 2597464-post-not-get.patch, failed testing.

The last submitted patch, 5: 2597464-post-not-get.patch, failed testing.

timmillwood’s picture

The patch looks like a good start, not had chance to apply it yet though. Definitely needs tests fixing.

My only experience with Toolbar CSS is Deploy module, so not sure if that gives you some hints http://cgit.drupalcode.org/deploy/tree/deploy.module

Could we store the workspace against the user object? I guess we don't want to allow anonymous users to switch workspace.

timmillwood’s picture

@Crell - I've just committed an update to multiversion add an icon to the "Workspaces" tab.

This may cause a merge conflict, but also add a working multiversion.toolbar.css file.

Crell’s picture

OK, updated patch. The conflict was minimal so no worries there. Fixes a bug, and uses flexbox(!) to make the buttons look like links and stuff. I haven't looked into all of the flexbox fallbacks for various and sundry old browsers, but this works in current Chrome and looks pretty good to me, visually. (Someone who actually specializes in CSS may disagree.)

Addendum: I just realized it breaks in horrific ways when you switch the toolbar to vertical mode, so I'll fix that next.

Still to do: Make it not use $_SESSION directly if at all possible. I need to see where we ended up on that front for what the preferred alternative is.

The idea behind using lots of little forms here is that it allows us to organize, layout, or show only subsets of the list in other locations if appropriate. This approach allows those other cases to organize the mini forms however makes sense, including putting them into a scroll bar or similar.

Status: Needs review » Needs work

The last submitted patch, 11: 2597464-post-not-get.patch, failed testing.

The last submitted patch, 11: 2597464-post-not-get.patch, failed testing.

The last submitted patch, 11: 2597464-post-not-get.patch, failed testing.

timmillwood’s picture

We will also need passing test ;-)

Crell’s picture

New patch fixes the vertical layout, and removes the test for the block that I just deleted.

Now here's the problem. With this change, the only way to change the active workspace is via a form, which we place in the toolbar. That breaks any tests that used the query parameter to switch workspaces, obviously. I'm trying to update those tests, but there's a problem: The toolbar doesn't seem to be available to drupalPostForm(), even when the toolbar module is enabled. Presumably that's because it is rendered via Javascript. However, there's no other API than "mess with the session directly" for setting the workspace for more than a single request.

That suggests we need to add such an API, but also that we need to expose it somehow in a test-friendly way. I am unclear how to do that safely.

While I was at it, I also noticed the WorkspaceSwitcherInterface, which is built around exposing the old GET links. That should assumedly go away, too, but in favor of what I'm not entirely certain. Perhaps repurpose it to show the buttons? Does that mean we bring back the block I just killed, and make it use the buttons instead? Maybe, I'm not sure.

Tim, let me know what direction you want to take here. Unfortunately I have only one day left before I go to India so there's a limit to what I can do in that time.

Status: Needs review » Needs work

The last submitted patch, 16: 2597464-post-not-get.patch, failed testing.

The last submitted patch, 16: 2597464-post-not-get.patch, failed testing.

The last submitted patch, 16: 2597464-post-not-get.patch, failed testing.

josephdpurcell’s picture

Workspaces appear to be non-functional when JavaScript is disabled; the "Workspaces" link goes to the home page. However, it appears much of the rest of the navigation still works.

One option would be to create a page that "Workspaces" goes to and shows the list of workspaces there, with forms, and that the web test be for the test on this page. The assumption in this scenario is that the Toolbar forms would function similarly to this new page.

josephdpurcell’s picture

Worth noting, I don't see any other navigation items that change the state of the system on click. They are all strictly for navigation.

I am unaware of any other modules that would benefit from having quick access to state-modifying actions in the nav. If there are not, the effort of modifying the test framework to support testing JavaScript generated forms may prove little value in the long term.

Alternatively, we could leave this as is, and simply write a kernel test for the form. However, I don't see any examples of that.

My inclination would be to do the following:

  • Make the "Workspaces" button in the nav go to /admin/structure/workspaces
  • Add a "Select Workspace" action to the list of buttons under the "Operations" column
  • Write a test that navigates to the /admin/structure/workspaces route and tests the "Select Workspace" button there
  • Assume that test passing means the navigation dropdown (as seen in the latest patch, #16) will also function

Thoughts?

josephdpurcell’s picture

There are two challenges with this that I see:

  1. If an action for selecting the active workspace is added to the /admin/structure/workspaces page, there should also be some indication of which is the active workspace
  2. If the same form is processing submissions from the toolbar and /admin/structure/workspaces, and if the completion of the form submission means the user goes to different success pages, that will need accommodations; perhaps this is easy to do
josephdpurcell’s picture

Per discussion with Tim, will will proceed by making Multiversion's workspace switching accessible even when JavaScript is disabled. This gives us the benefit of testability, and accessibility for the user.

To do this, we will:

  • Make the "Workspaces" element in the nav link to /admin/structure/workspaces
  • Add a "Select Workspace" action to the list of buttons under the "Operations" column at /admin/structure/workspaces
  • Add an indicator of which is the current workspace at /admin/structure/workspaces (what type of indication is TBD)
  • Completion of the workspace switching form should redirect the user back to the page from which they came
  • Write a test that navigates to the /admin/structure/workspaces route and tests the "Select Workspace" button there; assume that test passing means the navigation dropdown (as seen in the latest patch, #16) will also function

One thing to figure out: What happens if the user switches workspaces and they are on a node that is only accessible in one workspace?

timmillwood’s picture

Switching workspaces should always take the user to the homepage, just as it does now from the current implementation.

Also just to check, the current implementation is staying the same when Javascript is enabled, the alternative from the workspace list page is an alternative that works without javascript.

Also for the current workspace indicator, I think something quick for now, then we can iterate on it, maybe an "Active" column with a tick in it, or something.

josephdpurcell’s picture

Here is a patch of where I'm at:

  • The nav bar now links to the workspaces page, so when js is disabled it will go there
  • The workspaces page now has a column "Status" that shows "Active" or "Inactive" for each workspace
  • The workspaces page now has an action under "Operations" for inactive workspaces that says "Set Active", which takes you to the activation form
  • There is now an activation form at /admin/structure/workspaces/{id}/activate

Todos:

  1. Move workspace switching to a service, and don't use $_SESSION to handle it
  2. Clean up old workspace switching logic
  3. Consolidate form usage from the toolbar and the activation form to at least use the same service for workspace switching, even if they don't have the same test
  4. Modify test to use the activation form
  5. Clean up @todo's

Here is a screenshot: https://s3.amazonaws.com/uploads.hipchat.com/51684/970789/sMARdt0HgMtv7O...

Any early feedback is welcome. I plan to continue this next week.

josephdpurcell’s picture

This takes care of 1 and 4 from #25, but still need to complete the rest of todos, primarily removing old logic and consolidating code.

Additionally, we need to submit a patch to Workspaces module for any changes that should be migrated over there.

Setting to "Needs review" just to see if tests pass.

Status: Needs review » Needs work

The last submitted patch, 26: 2597464-post-not-get_26.patch, failed testing.

The last submitted patch, 26: 2597464-post-not-get_26.patch, failed testing.

The last submitted patch, 26: 2597464-post-not-get_26.patch, failed testing.

timmillwood’s picture

Tests are currently failing because DrupalCI pulls in Drupal 8.1.x and Multiversion currently only works with 8.0.x

josephdpurcell’s picture

Hmm... k. If that is the case, this patch may very well fail too. I've made some major changes, which will likely need discussion. I'm going to submit it anyway to see if tests do in fact pass.

Status: Needs review » Needs work

The last submitted patch, 31: 2597464-post-not-get_31.patch, failed testing.

The last submitted patch, 31: 2597464-post-not-get_31.patch, failed testing.

The last submitted patch, 31: 2597464-post-not-get_31.patch, failed testing.

timmillwood’s picture

A couple of nit picks, I think we should add font-weight: normal; to the toolbar buttons, and remove/override box-shadow on button hover.

I think we discussed keeping the switcher block, just updating it to have the toolbar like buttons in it.

Not sure I like the two steps needed to set active workspace from the workspace collection list, but not sure there's a way around it. I do like having the ability to set active from there.

josephdpurcell’s picture

The BlockTest is giving me fits because we're no longer dealing with GET requests. Typical WebTestBase works well when you don't need to do any interactions, like clicking.

My thought is to use BrowserTestBase and trigger a form submission on the block (since the block now has a series of forms instead of links). Yet, I'm having trouble getting that to succeed. I'm attaching the patch anyway. I'll follow up tomorrow.

josephdpurcell’s picture

Status: Needs review » Needs work

The last submitted patch, 36: 2597464-post-not-get_36.patch, failed testing.

The last submitted patch, 36: 2597464-post-not-get_36.patch, failed testing.

The last submitted patch, 36: 2597464-post-not-get_36.patch, failed testing.

timmillwood’s picture

Status: Needs review » Needs work

The last submitted patch, 41: do_security_review_of-2597464-41.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review

The last submitted patch, 25: 2597464-post-not-get_25.patch, failed testing.

The last submitted patch, 25: 2597464-post-not-get_25.patch, failed testing.

The last submitted patch, 25: 2597464-post-not-get_25.patch, failed testing.

timmillwood’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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