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.

[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. 😉

[simon-spier0]

Is there a plan to merge this? 🙂

[tim-schilling]

I apologize @simon-spier0. I've had a lot on my plate and this project tends to get de-prioritized when that happens. I appreciate your work and thank you for bringing this up again.

[simon-spier0]

I wanted to ask if there's a plan to release a new version of the library that includes my fix. Thank you for your response. 🙂

[JonasKs]

Hi!
Yes! I can look into a release this week if Tim don't beat me to it☺️

[simon-spier0]

Great, thank you! I have tested it, and it works, so I think we can close this and #280. 🙂