feat: oidc authorize post#823
Conversation
dosubot
Bot
added
the
size:M
📝 WalkthroughWalkthroughThis PR implements support for POST-based OAuth2/OIDC authorization requests. It adds a new Changes
Sequence DiagramsequenceDiagram
participant Client as Browser/Client
participant Middleware as UI Middleware
participant Controller as OIDC Controller
participant Backend as Authorization Backend
Client->>Middleware: POST /authorize<br/>(form-encoded params)
Middleware->>Middleware: Detect POST to /authorize
Middleware->>Controller: c.Next() - Forward to handler
Controller->>Controller: AuthorizePseudoPost:<br/>Read request body
Controller->>Controller: Extract params from body
Controller->>Client: 302 Redirect to GET /authorize<br/>with query parameters
Client->>Backend: GET /authorize?scope=...&client_id=...
Backend-->>Client: Authorization response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/controller/oidc_controller_test.go (1)
872-874: Avoid asserting the full redirect URL string.This couples the test to query ordering and exact escaping. Parsing
Locationand asserting the individual parameters would make the test less brittle.Refactor sketch
location := recorder.Header().Get("Location") assert.NotEmpty(t, location) - assert.Equal(t, "https://tinyauth.example.com/authorize?client_id=some-client-id&code_challenge=some-challenge&code_challenge_method=plain&nonce=some-nonce&redirect_uri=https%3A%2F%2Ftest.example.com%2Fcallback&response_type=code&scope=openid&state=some-state", location) + + redirectURL, err := url.Parse(location) + require.NoError(t, err) + assert.Equal(t, "https://tinyauth.example.com/authorize", redirectURL.Scheme+"://"+redirectURL.Host+redirectURL.Path) + + params := redirectURL.Query() + assert.Equal(t, "some-client-id", params.Get("client_id")) + assert.Equal(t, "some-challenge", params.Get("code_challenge")) + assert.Equal(t, "plain", params.Get("code_challenge_method")) + assert.Equal(t, "some-nonce", params.Get("nonce")) + assert.Equal(t, "https://test.example.com/callback", params.Get("redirect_uri")) + assert.Equal(t, "code", params.Get("response_type")) + assert.Equal(t, "openid", params.Get("scope")) + assert.Equal(t, "some-state", params.Get("state"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/oidc_controller_test.go` around lines 872 - 874, The test currently asserts the entire Location string (the lines using recorder.Header().Get("Location") and assert.Equal(... full URL ...)), which is brittle; change it to parse the Location with net/url (url.Parse), assert the scheme/host/path or base URL separately, then parse query parameters (url.Query()) and assert individual keys like client_id, code_challenge, code_challenge_method, nonce, redirect_uri, response_type, scope, and state have the expected values; this removes coupling to query ordering and exact escaping while keeping the same expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/oidc_controller.go`:
- Around line 202-211: AuthorizePseudoPost currently appends the raw POST body
into the redirect URL (in AuthorizePseudoPost), exposing large/signed request
payloads and breaking POST semantics; instead, persist the POST parameters
server-side and redirect with a short opaque reference (e.g., a generated state
ID) only. Change AuthorizePseudoPost to read and validate the body, generate a
secure random key (state), store the full form/payload in a server-side store or
session keyed by that state (or set a secure, HttpOnly cookie referencing it),
then redirect to fmt.Sprintf("%s/authorize?state=%s",
controller.oidc.GetIssuer(), state). Ensure you use the same storage lookup
later when handling the GET authorize flow and add error handling for storage
failures.
In `@internal/middleware/ui_middleware.go`:
- Around line 56-60: The current middleware forwards any POST whose path starts
with "authorize" (strings.HasPrefix(path, "authorize")), which erroneously
matches "/authorized" and "/authorize-anything"; change the condition to match
only the exact "/authorize" route used by the OIDC controller (e.g., replace the
HasPrefix check with an equality check for the full path string such as path ==
"/authorize" or path == "authorize" consistent with how path is set), keeping
the same behavior of calling c.Next() and return when matched so only POST
/authorize is forwarded to the downstream handler defined in the OIDC
controller.
---
Nitpick comments:
In `@internal/controller/oidc_controller_test.go`:
- Around line 872-874: The test currently asserts the entire Location string
(the lines using recorder.Header().Get("Location") and assert.Equal(... full URL
...)), which is brittle; change it to parse the Location with net/url
(url.Parse), assert the scheme/host/path or base URL separately, then parse
query parameters (url.Query()) and assert individual keys like client_id,
code_challenge, code_challenge_method, nonce, redirect_uri, response_type,
scope, and state have the expected values; this removes coupling to query
ordering and exact escaping while keeping the same expectations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e11543d-bce0-487f-8172-3e1be8253af7
📒 Files selected for processing (6)
frontend/vite.config.tsinternal/bootstrap/router_bootstrap.gointernal/controller/oidc_controller.gointernal/controller/oidc_controller_test.gointernal/middleware/ui_middleware.gointernal/service/oidc_service.go
| // Pseudo handler that will just redirect to get in frontend then back to backend | ||
| func (controller *OIDCController) AuthorizePseudoPost(c *gin.Context) { | ||
| body, err := io.ReadAll(c.Request.Body) | ||
| if err != nil { | ||
| tlog.App.Error().Err(err).Msg("Failed to read request body") | ||
| c.Redirect(http.StatusFound, fmt.Sprintf("%s/authorize", controller.oidc.GetIssuer())) | ||
| return | ||
| } | ||
| redirectUrl := fmt.Sprintf("%s/authorize?%s", controller.oidc.GetIssuer(), body) | ||
| c.Redirect(http.StatusFound, redirectUrl) |
There was a problem hiding this comment.
This turns authorize POST back into a GET.
Line 210 serializes the entire form body into the Location header, so large or signed request payloads still hit URL-length limits and end up in browser history/logs. That defeats the main reason clients use POST on the authorization endpoint. Keep the authorize parameters server-side across the login redirect instead of rebuilding them into the URL.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/controller/oidc_controller.go` around lines 202 - 211,
AuthorizePseudoPost currently appends the raw POST body into the redirect URL
(in AuthorizePseudoPost), exposing large/signed request payloads and breaking
POST semantics; instead, persist the POST parameters server-side and redirect
with a short opaque reference (e.g., a generated state ID) only. Change
AuthorizePseudoPost to read and validate the body, generate a secure random key
(state), store the full form/payload in a server-side store or session keyed by
that state (or set a secure, HttpOnly cookie referencing it), then redirect to
fmt.Sprintf("%s/authorize?state=%s", controller.oidc.GetIssuer(), state). Ensure
you use the same storage lookup later when handling the GET authorize flow and
add error handling for storage failures.
There was a problem hiding this comment.
To be honest I don't believe that storing on the backend makes sense, it's way too much code for something that will be used extremely rarely. Even when it's used I am not familiar with any URL limit.
There was a problem hiding this comment.
@steveiliop56, that's a fair point — for standard authorization parameters the URL stays well within practical browser/server limits (~8 KB), and the certification test PASSED with 43/43 successes, which is the real validation here.
The one narrow edge case would be a large request JWT (RFC 9101 / JARM), since those can be several KB on their own. But since you've already added the Request field to AuthorizeRequest and the conformance suite accepts the current approach, the complexity of server-side storage clearly isn't justified. I'll drop the suggestion.
✏️ Learnings added
Learnt from: steveiliop56
Repo: tinyauthapp/tinyauth PR: 823
File: internal/controller/oidc_controller.go:202-211
Timestamp: 2026-04-28T12:09:48.523Z
Learning: In `internal/controller/oidc_controller.go` (tinyauth project, Go/Gin), `AuthorizePseudoPost` intentionally converts a POST body into redirect URL query parameters. The maintainer has decided this is acceptable: server-side parameter storage is considered too complex for this rarely-used code path, and the OIDCC certification test `oidcc-ensure-post-request-succeeds` passes with this approach.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| // For OIDC post authentication, we need to redirect the POST to /authorize to the backend | ||
| if method == http.MethodPost && strings.HasPrefix(path, "authorize") { | ||
| c.Next() | ||
| return | ||
| } |
There was a problem hiding this comment.
Match only /authorize here.
strings.HasPrefix(path, "authorize") also forwards paths like /authorized and /authorize-anything, but the only downstream route added is POST /authorize in internal/controller/oidc_controller.go Line 78. Those requests now bypass the UI/static handler and fall into a 404 instead.
Suggested fix
- if method == http.MethodPost && strings.HasPrefix(path, "authorize") {
+ if method == http.MethodPost && path == "authorize" {
c.Next()
return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // For OIDC post authentication, we need to redirect the POST to /authorize to the backend | |
| if method == http.MethodPost && strings.HasPrefix(path, "authorize") { | |
| c.Next() | |
| return | |
| } | |
| // For OIDC post authentication, we need to redirect the POST to /authorize to the backend | |
| if method == http.MethodPost && path == "authorize" { | |
| c.Next() | |
| return | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/middleware/ui_middleware.go` around lines 56 - 60, The current
middleware forwards any POST whose path starts with "authorize"
(strings.HasPrefix(path, "authorize")), which erroneously matches "/authorized"
and "/authorize-anything"; change the condition to match only the exact
"/authorize" route used by the OIDC controller (e.g., replace the HasPrefix
check with an equality check for the full path string such as path ==
"/authorize" or path == "authorize" consistent with how path is set), keeping
the same behavior of calling c.Next() and return when matched so only POST
/authorize is forwarded to the downstream handler defined in the OIDC
controller.
|
Never mind this is a mess. |
1 participant
Summary by CodeRabbit
New Features
/authorizeendpoint with form-encoded parameter handling and HTTP redirects/authorizerequests to backendTests
/authorizeendpoint with form-encoded parameters and redirect validation