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

Add CSRF protection to workspace settings forms #84

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add CSRF protection to workspace settings forms #84

wants to merge 2 commits into from

Conversation

yogur
Copy link

@yogur yogur commented Nov 1, 2023

Problem

Forms that change workspace settings are vulnerable to CSRF. A successful CSRF attack can lead to changing workspace user access list and to making private workspaces public.

The vulnerability can be reproduced on the form for changing user access by following these steps:

  1. Copy the following code and save it in a new html file:
<html>
  <body>
    <form action="http://localhost:8080/workspace/1/users" method="POST">
      <input type="hidden" name="writeUsers" value="structurizr&#13;&#10;csrf&#45;write&#64;gmail&#46;com&#13;&#10;" />
      <input type="hidden" name="readUsers" value="csrf&#45;read&#64;gmail&#46;com&#13;&#10;" />
      <input type="submit" value="Submit request" />
    </form>
    <script>
      history.pushState('', '', '/');
      document.forms[0].submit();
    </script>
  </body>
</html>
  1. Change the workspace ID number in form action from 1 to an ID number of an existing workspace.
  2. Open the html file in a browser that's already authenticated to structurizr and click the submit button.
  3. Note how the list of users that have access to the workspace has been updated.

Fix

The changes in the pull request add CSRF protection to seven forms.

I built the code locally and tested the fix to make sure it works.

@simonbrowndotje
Copy link
Contributor

Thanks for the PR. Unfortunately the || uri.startsWith("/workspace") test is too wide in scope, and breaks other POST requests (e.g. /workspace/1/lock). Perhaps you could add more specific tests (e.g. uri.endsWith("/users"), etc)?

@yogur
Copy link
Author

yogur commented Nov 17, 2023

I wasn't aware of this.
Before I do any changes to the code, do you have any concerns with using regex to match paths? So for example for the path /workspace/123/images/delete, the test would be uri.matches("/workspace/\\d+/images/delete").

@yogur
Copy link
Author

yogur commented Aug 28, 2024

Hello @simonbrowndotje,

It has been a while since I created this PR. Can you please review.

I pushed a commit that uses a regex for more specific matching. I tested locally. The post requests are matched and are blocked if the CSRF is not present or inaccurate.

@yogur
Copy link
Author

yogur commented Nov 14, 2024

Hi @simonbrowndotje,

Pinging you again in case you missed my latest message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yogur @simonbrowndotje