Skip to content

fix(openidc): include request port in redirect_uri#13081

Open
shreemaan-abhishek wants to merge 6 commits intoapache:masterfrom
shreemaan-abhishek:fix/port-missing-from-redirecturi
Open

fix(openidc): include request port in redirect_uri#13081
shreemaan-abhishek wants to merge 6 commits intoapache:masterfrom
shreemaan-abhishek:fix/port-missing-from-redirecturi

Conversation

@shreemaan-abhishek
Copy link
Copy Markdown
Contributor

@shreemaan-abhishek shreemaan-abhishek commented Mar 10, 2026

Description

This PR: #12551, introduced a change that would overwrite X-Forwarded-Host with ctx.var.host which is just the hostname without the port.

When lua-resty-openidc sees a redirect_uri starting with /, it tries to reconstruct the full URL by checking headers in this order:

  1. Forwarded header → usually not set
  2. X-Forwarded-Host → now set to just localhost (no port) by handle_x_forwarded_headers
  3. ngx.var.http_host → never reached

Before v3.14.0, X-Forwarded-Host wasn't forcibly overwritten, so openidc would fall through to ngx.var.http_host
which includes the port (e.g., localhost:8888), and everything worked.

Fix:

Instead of handing a relative path to lua-resty-openidc and letting it reconstruct the URL (where it can lose the
port), we now build the full absolute redirect_uri directly in the plugin using ctx.var.http_host. This is the
original Host header from the client, which preserves the port as the client sees it critical for Docker port
mappings where the external port differs from the container port.

Fixes #12970

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

Signed-off-by: Abhishek Choudhary <shreemaan.abhishek@gmail.com>
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. bug Something isn't working labels Mar 10, 2026
@Baoyuantop
Copy link
Copy Markdown
Contributor

Hi @shreemaan-abhishek, could you describe the root cause of the problem and the current solution?

@shreemaan-abhishek
Copy link
Copy Markdown
Contributor Author

@Baoyuantop, I have added that in the description.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses a regression in the openid-connect plugin where auto-generated redirect_uri could lose the client-facing port (notably after the X-Forwarded-Host sanitization changes from #12551). It does so by constructing an absolute redirect_uri directly in the plugin rather than relying on lua-resty-openidc to reconstruct it from forwarded headers.

Changes:

  • Build an absolute redirect_uri in apisix/plugins/openid-connect.lua using request-derived scheme/host plus the computed redirect path.
  • Add regression tests to ensure auto-generated redirect_uri preserves a non-default port from the Host header and behaves consistently when Host has no port.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
apisix/plugins/openid-connect.lua Changes auto-generation of redirect_uri from a relative path to an absolute URL to preserve client-facing port.
t/plugin/openid-connect3.t Adds tests validating the generated redirect_uri contents, including port-preservation scenarios.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Signed-off-by: Abhishek Choudhary <shreemaan.abhishek@gmail.com>
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Mar 13, 2026
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 11 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

shreemaan-abhishek and others added 2 commits March 13, 2026 09:18
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Abhishek Choudhary <shreemaan.abhishek@gmail.com>
Copy link
Copy Markdown
Member

@moonming moonming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @shreemaan-abhishek, thank you for fixing the redirect_uri port issue in openid-connect!

Including the request port in the redirect_uri is important for deployments where the gateway runs on non-standard ports or behind proxies that forward port information via X-Forwarded-Host. With 5 reviews, this looks well-discussed. Will review in detail shortly.

moonming
moonming previously approved these changes Mar 16, 2026
local scheme
local host

if trusted_addr.is_trusted(ctx.var.realip_remote_addr) then
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this logic is duplicated with handle_x_forwarded_headers. For the current issue, I believe we should fix it in handle_x_forwarded_headers by adding the port to X-Forwarded-Host. The port can be omitted for 80/443, while other non-standard ports should be included in X-Forwarded-Host. I've checked the relevant information, and this is the conventional practice.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise, we need to figure out how to correctly obtain the external access address of the gateway for every function involving the acquisition of the gateway access address, and this is exactly what the X-Forwarded-Host is for. We should fix the issue at the root cause instead of making fixes wherever the X-Forwarded-Host is used.

Signed-off-by: Abhishek Choudhary <shreemaan.abhishek@gmail.com>
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Mar 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: APISIX 3.14.0+ OpenID Connect redirect drops request port (regression vs 3.13.0)

5 participants