[BUG] Redirect code is wrong in example controller.xq for POST requests

[adamretter]

In the README.md an example controller.xq file is given here:

(: if no valid token, redirect to SAML auth :)
if (exsaml:is-enabled() and not(exsaml:check-valid-saml-token()))
then (
    let $debug := exsaml:log('info', "controller: no valid token, redirect to SAML auth")
    let $return-path := "/exist/apps" || $exist:controller || $exist:path
    return
        <dispatch xmlns="http://exist.sourceforge.net/NS/exist">
            <redirect url="{exsaml:build-authnreq-redir-url($return-path)}">
                <set-header name="Cache-Control" value="no-cache, no-store" />
                <set-header name="Pragma" value="no-cache" />
            </redirect>
        </dispatch>

Unfortunately in the provided example, the <redirect url="{exsaml:build-authnreq-redir-url($return-path)}"> will always produce a HTTP 302 Response Code. This is incorrect:

1. A HTTP 302 Response Code should only be used when the incoming HTTP Request uses a HTTP GET Method.
2. If the incoming HTTP Request uses a POST Method, then the correct HTTP Response Code for a redirect should instead be a HTTP 303. This causes the client to rewrite the redirect from a POST to a GET.

[adamretter]

eXist-db's URL Rewrite facility does not provide a mechanism for controlling the Redirect Code at present, until that is possible added in future, a workaround could be something like:

(: if no valid token, redirect to SAML auth :)
if (exsaml:is-enabled() and not(exsaml:check-valid-saml-token()))
then (
    let $debug := exsaml:log('info', "controller: no valid token, redirect to SAML auth")
    let $return-path := "/exist/apps" || $exist:controller || $exist:path
    let $response-status :=
        if (request:get-method() eq "GET")
        then
          302
        else
          303
    let $response-location := exsaml:build-authnreq-redir-url($return-path)
    return
        (
          response:set-status-code($response-status),
          response:set-header("Location", $response-location),
          response:set-header("Cache-Control", "no-cache, no-store"),
          response:set-header("Pragma", "no-cache")
        )

[chakl]

@adamretter Have you tested that? I would be surprised if it worked.

[adamretter]

I would be surprised if it worked.

May I ask why that is?

[chakl]

I would be surprised if it worked.

May I ask why that is?

No. You're the one claiming it works, so pls show.

• the example code with HTTP 302 works in all existdb-saml installations I have access to.
• you propose a change to this - you should have good reasons for a change that might break multiple installations.
• you should at least have tested this yourself - raising theroretical problems without any attempt to validate them does not give credit to your "bug" reports.
• I think this 303 discussion is pure nonsense
• so pls put up or shut up

[adamretter]

You're the one claiming it works

@chakl Yes. My questions was to try and understand why you wrote "I would be surprised if it worked". My suggested change is a small and straightforward one. From your language, I understood you to be dismissive of the change as you believed that it would not work. Is that not the case? If you don't think it would work, I would like to understand why, which is why I wrote "May I ask why that is?".

the example code with HTTP 302 works in all existdb-saml installations I have access to.

I am not disputing that what is there may work.

you propose a change to this - you should have good reasons for a change that might break multiple installations.

Simply because something is working mostly, does not of course mean that it is correct. We have been debugging a situation where we are using this existdb-saml module with Microsoft Azure.

To help us understand the problem, we have been re-reading the 5.1.2 SP-Initiated SSO: Redirect/POST Bindings section of the OASIS specification: Security Assertion Markup Language (SAML) V2.0 Technical Overview. The specification clearly states: The SP sends an HTTP redirect response to the browser (HTTP status 302 or 303).

When to use a 302 or 303 is clearly set out in the HTTP 1.1 specification from the IETF, see:

1. https://www.rfc-editor.org/rfc/rfc9110.html#name-302-found
2. https://www.rfc-editor.org/rfc/rfc9110.html#name-303-see-other

you should at least have tested this yourself - raising theroretical problems without any attempt to validate them does not give credit to your "bug" reports.

At no point have I indicated that I have not tested this. This is a conclusion that you have drawn by yourself.
To be clear, I have tested this!

I think this 303 discussion is pure nonsense

From reading the relevant specifications, it would seem that the authors of the OASIS SAML specification and the HTTP 1.1 specification might disagree with you.

so pls put up or shut up

I find your language to be very inflammatory. I am contributing issues and fixes to this project in good faith. I do not believe the language I have used in this bug report should cause offence to yourself or anyone else, and neither was it intended to do so.
@chakl Can I please remind you that all eXist-db projects are covered by a Contributor Covenant to try and encourage people to feel welcome to contribute.

[chakl]

@adamretter sorry for the late reply, just returned from vacation. Pls give me a moment to check recent communication.

I find your language to be very inflammatory.

Pls accept my sincere apologies for this. I had assumed you're a native speaker who knows that schoolyard quip "put up or shut up", meaning "show what you have instead of talking about it".
I used that phrase because I was under the impression that you were just theorizing, instead of showing a practical use case, and proof that your change works.

I still don't see a use case for this. I'll address this in another message.
Thnx for referring to the spec, I'll re-read to make sure we're in the same boat.

[chakl]

@adamretter Your initial statement is:

Unfortunately in the provided example, the will always produce a HTTP 302 Response Code. This is incorrect:

Nope. That works exactly as intended, so I call this correct.

A HTTP 302 Response Code should only be used when the incoming HTTP Request uses a HTTP GET Method.

Agree. The common use case is GET /exist/apps/foobar, which gets a 302 "pls auth at IDP" redirect if the user is not authorized yet. Nothing wrong here.

If the incoming HTTP Request uses a POST Method, then the correct HTTP Response Code for a redirect should instead be a HTTP 303. This causes the client to rewrite the redirect from a POST to a GET.

That's where you lost me. What problem or use case does that solve? Why should the client rewrite POST to GET? The result of this code path is a single round-trip between user's browser and IDP, with the IDP POSTing the SP endpoint (which is a different code path).

Ok, maybe I'm just colorblind.. Can you show curl traces with headers that demonstrate the problem that your proposed change addresses, and traces that show what's being solved?

I'd rather remove than add rarely used code paths, so pls show a practical use case for your change request.

[adamretter]

Pls accept my sincere apologies for this.

@chakl Accepted. Thank you.

I had assumed you're a native speaker

I am British, and I am a native English speaker.

who knows that schoolyard quip "put up or shut up"

I am aware of it, and it is considered rude (e.g. https://en.wikipedia.org/wiki/Wikipedia:Put_up_or_shut_up#But_%22put_up_or_shut_up%22_is_rude!)

Let's move forward...

[adamretter]

Unfortunately in the provided example, the will always produce a HTTP 302 Response Code. This is incorrect:

Nope. That works exactly as intended, so I call this correct.

Then allow me to rephrase my comment to add further clarity. By "This is incorrect", I perhaps should have written more clearly: "This is incorrect according to the relevant international technical standards, i.e.: (a) OASIS specification: Security Assertion Markup Language (SAML) V2.0 Technical Overview, and (b) IETF RFC 9110 HTTP Semantics (i.e. HTTP 1.1)"

Agree. The common use case is GET /exist/apps/foobar, which gets a 302 "pls auth at IDP" redirect if the user is not authorized yet. Nothing wrong here.

In SAML, the User Agent performs a HTTP POST from a HTML Form to the Service Provide (eXist-db). The SP responds by sending a redirect to the User Agent. The problem is that eXist-db in the code for this module sends a HTTP 302. As you recognised above, a 302 should only be used to redirect from a HTTP GET. It is incorrectly used here to redirect from a HTTP POST.

That's where you lost me. What problem or use case does that solve? Why should the client rewrite POST to GET?

I am not sure what you mean by "client". Any server receiving a HTTP POST that wants to redirect the client, needs to send a 303 (at present the existdb-saml module always sends a 302).

If we want this existdb-saml module to be compliant with the relevant international technical standards, and I can't imagine why we wouldn't, then we need to fix this.

I'd rather remove than add rarely used code paths

This isn't rare. This occurs on every SAML authentication between UA-SP-IDP.

[chakl]

Then allow me to rephrase my comment to add further clarity. By "This is incorrect", I perhaps should have written more clearly: "This is incorrect according to the relevant international technical standards, i.e.: (a) OASIS specification: Security Assertion Markup Language (SAML) V2.0 Technical Overview, and (b) IETF RFC 9110 HTTP Semantics (i.e. HTTP 1.1)"

Ah, I should have responded to your other msg as well..

The specification clearly states: The SP sends an HTTP redirect response to the browser (HTTP status 302 or 303)

existdb-saml currently sends 302, I don't see a violation of (a) OASIS specification at all. As for (b) IETF RFC 9110, I still don't see where this applies, except in a scenario that I think you make up.

In SAML, the User Agent performs a HTTP POST from a HTML Form to the Service Provide (eXist-db). The SP responds by sending a redirect to the User Agent. The problem is that eXist-db in the code for this module sends a HTTP 302. As you recognised above, a 302 should only be used to redirect from a HTTP GET. It is incorrectly used here to redirect from a HTTP POST.

Uhm, no. You have this wrong.

The User Agent performs a HTTP POST from a HTML Form to the Identity Provider (remote IDP). After successful auth, the IDP POSTs to the configured SAML SP endpoint (called /SAML2SP in the example). In that code branch, the IDP answer gets validated, and if it passes, that code branch is responsible to send the user agent to the initially requested resource. That's a completely different code branch than the one you're talking about.

If we want this existdb-saml module to be compliant with the relevant international technical standards, and I can't imagine why we wouldn't, then we need to fix this.

Yes. And if it ain't broke, don't fix it.

This isn't rare. This occurs on every SAML authentication between UA-SP-IDP.

Well, rare for me, as I never experienced issues running that code for years on a few high profile sites.
And I still don't understand what issue you are trying to solve, other than perceived applicable standards compliance.

Put up some traces? :)

[adamretter]

In SAML, the User Agent performs a HTTP POST from a HTML Form to the Service Provide (eXist-db). The SP responds by sending a redirect to the User Agent. The problem is that eXist-db in the code for this module sends a HTTP 302. As you recognised above, a 302 should only be used to redirect from a HTTP GET. It is incorrectly used here to redirect from a HTTP POST.

Uhm, no. You have this wrong.

@chakl I don't think so. Please see the attached diagram from the OASIS specification for SAML. In my text above, I am specifically referring to what they have conveniently labelled as step 6 and step 7 in their diagram.