Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(AppFramework): Log malformed protocol values and unify fallback behavior #50099

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

joshtrichards
Copy link
Member

@joshtrichards joshtrichards commented Jan 8, 2025

Summary

This PR:

  • Unifies existing fallback behavior regardless of method used to specify protocol (i.e. always returning http at a minimum).
  • Adds logging whenever the fallback is used so the admin has a chance to fix whatever is causing it.

We currently:

  • Fallback to a sane protocol value (http) when a malformed protocol value is received from a reverse proxy (via X-Forwarded-Proto).
  • Silently fallback, which can make troubleshooting challenging.
  • Trust the value of overwriteprotocol to be reasonable.
  • Have no fallback behavior for malformed overwriteprotocol values (unlike X-Forwarded-Proto).

The fallback is not a default value. It's just means a reasonable protocol is returned. When the fallback is used, it's basically an error. If it wasn't existing behavior, we might consider throwing an exception here instead. This PR just adds similar fallback behavior for malformed overwriteprotocol values and also logs when the fallback is used.

Details:

  • Sanity checks value overwriteprotocol (if any) and when malformed uses same fallback value (http) as we do for X-Forwarded-Proto
  • Adds logging when the server protocol is malformed (regardless of source)
  • Treats upper/mixed case values (i.e. HTTP / HTTPS) as valid for overwriteprotocol (just as it already does for X-Forwarded-Proto)
  • Refactors code slightly (formatting, comments, eliminates unnecessary comparisons, minor structure change)

TODO

  • ...

Checklist

@joshtrichards joshtrichards added bug 3. to review Waiting for reviews php Pull requests that update Php code ♻️ refactor Refactor code (not a bug fix, not a feature just refactoring) labels Jan 8, 2025
@joshtrichards joshtrichards requested review from a team, icewind1991, Altahrim and nfebe and removed request for a team January 8, 2025 19:20
@joshtrichards joshtrichards force-pushed the jtr/fix-appframework-server-proto branch from f2d00fe to 892eb7d Compare January 9, 2025 14:48
@joshtrichards joshtrichards changed the title fix(AppFramework): Unify protocol value fallback behavior and log when used fix(AppFramework): Log malformed protocol values and unify fallback behavior Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug php Pull requests that update Php code ♻️ refactor Refactor code (not a bug fix, not a feature just refactoring)
Projects
None yet
1 participant