-
Notifications
You must be signed in to change notification settings - Fork 146
fix: Logout calls localhost in prod; standardise API base resolution #1201
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
Conversation
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1201 +/- ##
==========================================
+ Coverage 83.86% 83.87% +0.01%
==========================================
Files 67 68 +1
Lines 2906 2908 +2
Branches 366 367 +1
==========================================
+ Hits 2437 2439 +2
Misses 409 409
Partials 60 60 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Please can a @finos/git-proxy-maintainers review this for me - thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andy_pols, sorry for the delay, I'm travelling. I hope to test this against an LDAP server, but that sometimes take a while to arrange.
There are a few additional VITE_API_URI references that could be replaced with API_BASE:
- /src/ui/services/auth.js
- /src/ui/services/config.js
- /src/ui/services/git-push.js
- /src/ui/services/repo.js
i.e. the UI services that haven't been converted to TS.
Otherwise this looks good to me! Thanks for taking it on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just wondering if we can use the API_BASE
instead of envs in git-proxy-cli/index.js
:
const { GIT_PROXY_UI_HOST: uiHost = 'http://localhost', GIT_PROXY_UI_PORT: uiPort = 8080 } =
process.env;
const baseUrl = `${uiHost}:${uiPort}`;
No because the proxy and the API are separate and may not have the same DNS (we have different DNS's domain names for the proxy and the app). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we can't (shouldn't?) use API_BASE
in the JS UI services until converted to TS, I think this is good to go.
Summary
This PR fixes a production logout failure caused by the client defaulting to http://localhost:3000 when VITE_API_URI is unset.
Root Cause
In
src/ui/components/Navbars/DashboardNavbarLinks.tsx
we have:Because
VITE_API_URI
isn’t set in prod, the code hit thelocalhost
fallback.Different areas of the code use different patterns, leading to inconsistent behaviour:
src/ui/views/Login/Login.tsx
:src/ui/services/user.ts
:Note: in Vite client code we should prefer
import.meta.env.VITE_API_URI
overprocess.env.*
but that does not work until we upgrade typescript config to use ES Modules (and not "CommonJS").Fix
What this PR changes
relative URLs by default
, with an optional env override.localhost
andlocation.origin
fallbacks from call sites.