[abhiramshaji-cyber]

Fixes #195

A sign in request sent as a POST form to /authorize was rejected for a missing redirect_uri; now the form values are read the same as query values.

What this code is for

serveAuthorize in server/authorize.go handles /authorize, the first step of an OAuth sign in. A relying party sends the user there with client_id, redirect_uri, state, nonce, scope and PKCE values. It checks the client and redirect, asks WhoIs who the user is, stores an AuthRequest, and redirects back with a code.

Before this PR

A relying party signs a user in by sending POST /authorize with client_id and redirect_uri in an application/x-www-form-urlencoded body.

1. serveAuthorize checks r.Method and allows POST.
2. It reads parameters with uq := r.URL.Query(), which only holds the query string.
3. The query string is empty, so uq.Get("redirect_uri") returns "".
4. The user gets 400 invalid_request - must specify redirect_uri.

After this PR

Same relying party, same POST /authorize.

1. serveAuthorize checks r.Method and allows POST.
2. It calls r.ParseForm() and reads parameters from r.Form, which holds the body values and the query values.
3. uq.Get("redirect_uri") and uq.Get("client_id") return the values from the body.
4. The client and redirect checks pass and the user is redirected back with a code.

A GET /authorize with parameters in the query string goes through the same steps and works as before.

The diagram

flowchart LR

classDef box fill:#eef1ff,stroke:#4c5bd4,color:#111827
classDef bad fill:#ffd6d6,stroke:#cc0000,color:#111827,stroke-width:3px
classDef good fill:#c8f0c8,stroke:#008800,color:#111827,stroke-width:3px

subgraph BEFORE["BEFORE: POST form gets 400"]
direction TB
  B1["POST /authorize with form body"] --> B2["serveAuthorize allows POST"]
  B2 --> B3["uq := r.URL.Query()"]
  B3 --> B4{"redirect_uri in query?"}
  B4 -->|"POST body ignored"| B5["400 must specify redirect_uri"]
  B4 -->|"GET with query"| B6["redirect with code"]
end

subgraph AFTER["AFTER: POST form gets a code"]
direction TB
  A1["POST /authorize with form body"] --> A2["serveAuthorize allows POST"]
  A2 --> A3["r.ParseForm()"]
  A3 --> A4{"THE CHANGE: uq := r.Form"}
  A4 -->|"POST body read"| A5["redirect with code"]
  A4 -->|"GET with query"| A6["redirect with code, same as before"]
end

B1 ~~~ A1

class B1,B2,B3,B6 box
class A1,A2,A3,A6 box
class B4,B5 bad
class A4,A5 good

linkStyle 3 stroke:#cc0000,stroke-width:5px
linkStyle 8 stroke:#008800,stroke-width:5px

style BEFORE fill:#fff5f5,stroke:#cc0000,color:#111827
style AFTER fill:#f3fff3,stroke:#008800,color:#111827

Loading

The change

In serveAuthorize, uq := r.URL.Query() becomes a call to r.ParseForm() followed by uq := r.Form. Every later uq.Get(...) call is unchanged. When r.ParseForm() returns an error, the handler returns 400 invalid_request with failed to parse form, the same way server/ui.go handles it.

server/authorize_test.go adds usePOSTForm and rawBody to TestServeAuthorize, with rows for a POST form that issues a code and keeps state and nonce, a POST form without redirect_uri, and a POST body that cannot be parsed.

What changes for a user

• Clients that send /authorize as a POST form now complete sign in.
• GET /authorize with a query string works as before.
• If a value is sent in both the body and the query string, uq.Get returns the body value, because r.Form lists body values first.
• A request whose body or query string cannot be parsed, such as an invalid % escape, now gets 400 invalid_request instead of having the bad pair dropped. Taken on purpose, so a malformed request is reported instead of half read.

[abhiramshaji-cyber]

@mikeodr would you be able to approve the Linux CI run for this? It is waiting on maintainer approval, and the change addresses #195.

[abhiramshaji-cyber]

Hope you're doing well, @mikeodr! It's me again from my note on 10/28 about the Linux CI run, which is still waiting whenever you have a moment, and I know you've been busy keeping tsidp's Go and Nix bits fresh, so there's truly no rush. If anything else would help, or you'd like the change shaped differently, I'm happy to rework it. I saw #209 just landed for offline_access, so I'm curious whether making tsidp work smoothly with more off the shelf OIDC clients is where the project is heading, and whether there's an area like that where outside help is most useful. Any tips on how you like contributions to tsidp to come in would be really welcome too, thank you!