Implementing POST in login, redirecting to previous page with NEXT parameter

[simon-spier0]

To allow user to be redirected to previous or specific page after login, django has default native parameter NEXT for that. When I want to implement it, this is the way:

login.html template:

<form method="get" action="{% url 'django_auth_adfs:login' %}">{% csrf_token %}
  <input type="hidden" name="next" value="{{ next }}">
  <button type="submit" class="btn btn-info"><i class="fa-brands fa-windows"></i> Log in with ADFS</button>
</form>

It works fine but OWASP scanner flags it as XSLT injection medium priority warning.

What I did then:

1. Changed form method GET to POST:

<form method="post" action="{% url 'django_auth_adfs:login' %}">{% csrf_token %}
  <input type="hidden" name="next" value="{{ next }}">
  <button type="submit" class="btn btn-info"><i class="fa-brands fa-windows"></i> Log in with ADFS</button>
</form>

2. Added post view:

class OAuth2LoginView(View):
    def get(self, request):
        return redirect(provider_config.build_authorization_endpoint(request))

    def post(self, request):
        return redirect(provider_config.build_authorization_endpoint(request))

3. Added the NEXT url from POST in config:

    def build_authorization_endpoint(self, request, disable_sso=None, force_mfa=False):
        self.load_config()
        redirect_to = request.POST.get(REDIRECT_FIELD_NAME, None)
        if not redirect_to:
            redirect_to = request.GET.get(REDIRECT_FIELD_NAME, None)
        if not redirect_to:
            redirect_to = django_settings.LOGIN_REDIRECT_URL
	...

Now, POST support is added but OWASP still detects it as XSLT injection. When I removed/disallowed the method get() in OAuth2LoginView, OWASP doesn't detect it anymore.

My question is if you can add even the POST support in login to this library. 🙂

Thanks.

Upvote & Fund

• We're using Polar.sh so you can upvote and help fund this issue.
• We receive the funding once the issue is completed & confirmed by you.
• Thank you in advance for helping prioritize & fund our backlog.

[simon-spier0]

Any updates? Will POST be implemented? GET is not secure enough. 🙂

[tim-schilling]

Hi @simon-spier0, would you be interested in opening a PR with the solution? We'd need to do the get restriction via deprecation most likely.

[simon-spier0]

Hi,
I finally had a moment, so I opened PR. 🙂

[tim-schilling]

I'm sorry @simon-spier0, I did not understand this issue well from the start. I'm not a fan of this change. The only thing that's being protected is the value of next because it's not included in the querystring. However, it's immediately known when the requests come back. I think this is a case where a security analysis tool is identifying something that's possibly a vulnerability, but in actuality it shouldn't be.

[JonasKs]

I'm also not sure what the security vulnerability it could possibly be, because we have next as a url query parameter?

According to the docs, there is a concern around query parameters for sensitive information, but this isn't confidential information.

[simon-spier0]

I understand that the next parameter isn’t sensitive and using GET shouldn't pose a real security risk, especially with proper validation. My intent was to enhance security and avoid false positives from tools like OWASP scanner, which flagged the use of GET as a potential issue.

However, I see your point. Would it make sense to support both GET and POST methods? Having the option to use POST could be useful for securely sending the next parameter without exposing it in the URL.

Would it be acceptable to keep the new POST method in the code without deprecating the GET method? This way, both options would be available for developers. 🙂

[JonasKs]

Thanks for the clarification and for the PR you've opened 😊
Since Microsoft don't see GET as a security concern either, and have documentation on how to use GET, I'd prefer not having a breaking change just because of false positive OWASP scans. It'll cause work for all our users.
In other words, I do not want to remove GET, but I personally don't mind adding POST. Let's wait for @tim-schilling to give his opinion!

[tim-schilling]

I'm fine with adding support for POST for the views.

Any concern comes around specifying the data (right now it's just the redirect path) and potentially having to support checking request.POST then request.GET. And making sure our documentation properly communicates what is expected.

[simon-spier0]

Thank you both for your feedback! I've updated the pull request based on our discussion:

• GET method: I've kept the GET method intact to avoid any breaking changes.
• POST method: Added support for POST requests as an additional option.
• Documentation: I updated the documentation to include examples for both GET and POST methods, so users have the flexibility to choose based on their needs.
• GET vs. POST handling: The handling of request.POST vs. request.GET has already been adjusted in a previous commit.

Let me know if there’s anything else you’d like to see changed. 😉