-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
feat(client): improved basepath detection #2320
Changes from all commits
f285a33
5ec8da5
e2c3c25
e53e31b
4d77a53
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
import parseUrl from "../../lib/parse-url" | ||
|
||
// https://stackoverflow.com/questions/48033841/test-process-env-with-jest | ||
describe("parseUrl() tests", () => { | ||
const OLD_ENV = process.env | ||
|
||
beforeEach(() => { | ||
jest.resetModules() // Most important - it clears the cache | ||
process.env = { ...OLD_ENV } // Make a copy | ||
}) | ||
|
||
afterAll(() => { | ||
process.env = OLD_ENV // Restore old environment | ||
}) | ||
|
||
test("when on client side and NEXT_PUBLIC_NEXTAUTH_URL is defined, correctly returns baseUrl and basePath", () => { | ||
process.env.NEXT_PUBLIC_NEXTAUTH_URL = "http://localhost:3000/api/v1/auth" | ||
|
||
const { baseUrl, basePath } = parseUrl() | ||
|
||
expect(baseUrl).toBe("http://localhost:3000") | ||
expect(basePath).toBe("/api/v1/auth") | ||
}) | ||
|
||
test("when on client side and NEXT_PUBLIC_NEXTAUTH_URL is not defined, falls back to default values", () => { | ||
const { baseUrl, basePath } = parseUrl() | ||
|
||
expect(baseUrl).toBe("http://localhost:3000") | ||
expect(basePath).toBe("/api/auth") | ||
}) | ||
|
||
test("when on server side, correctly parses any given URL", () => { | ||
let url = "http://localhost:3000" | ||
let { baseUrl, basePath } = parseUrl(url) | ||
|
||
expect(baseUrl).toBe("http://localhost:3000") | ||
expect(basePath).toBe("/api/auth") | ||
|
||
url = "http://localhost:3000/api/v1/authentication/" | ||
// this semi-colon is needed, otherwise js thinks the string | ||
// above is a function | ||
;({ baseUrl, basePath } = parseUrl(url)) | ||
Comment on lines
+39
to
+42
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I understand we don't need to re-assign const url1 = "http://localhost:3000"
const { baseUrl: baseUrl1, basePath: basePath1 } = parseUrl(url1)
// ...
const url2 = "http://localhost:3000"
const { baseUrl: baseUrl2, basePath: basePath2 } = parseUrl(url2) I think will make this case a bit nicer to read 👍🏽 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wanted to do this but decided against it. But it seems like it would actually be better this way. |
||
|
||
expect(baseUrl).toBe("http://localhost:3000") | ||
expect(basePath).toBe("/api/v1/authentication") | ||
|
||
url = "https://www.mydomain.com" | ||
;({ baseUrl, basePath } = parseUrl(url)) | ||
|
||
expect(baseUrl).toBe("https://www.mydomain.com") | ||
expect(basePath).toBe("/api/auth") | ||
|
||
url = "https://www.mydomain.com/api/v3/auth" | ||
;({ baseUrl, basePath } = parseUrl(url)) | ||
|
||
expect(baseUrl).toBe("https://www.mydomain.com") | ||
expect(basePath).toBe("/api/v3/auth") | ||
}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We probably want to add test cases for the unhappy paths as well 🙏🏽 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup. That would probably make more sense once we have refactored parseUrl() to throw errors. |
||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,7 +56,7 @@ export function useSession(options = {}) { | |
|
||
React.useEffect(() => { | ||
if (requiredAndNotLoading) { | ||
const url = `/api/auth/signin?${new URLSearchParams({ | ||
const url = `${_apiBaseUrl()}/signin?${new URLSearchParams({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we could use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure on this one. As far as I understand, I would leave it on the core team to make decisions on this and refactor accordingly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, I didn't realise There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mahieyin-rahmun could you move this change to a different PR please? 🙏🏽 |
||
error: "SessionRequired", | ||
callbackUrl: window.location.href, | ||
})}` | ||
|
@@ -339,6 +339,17 @@ function _apiBaseUrl() { | |
logger.warn("NEXTAUTH_URL", "NEXTAUTH_URL environment variable not set") | ||
} | ||
|
||
if ( | ||
process.env.NODE_ENV === "development" && | ||
!/^http:\/\/localhost:\d+$/.test(process.env.NEXTAUTH_URL) && | ||
!process.env.NEXT_PUBLIC_NEXTAUTH_URL | ||
) { | ||
logger.warn( | ||
"NEXT_PUBLIC_NEXTAUTH_URL", | ||
`NEXTAUTH_URL is set to "${process.env.NEXTAUTH_URL}" instead of the default "http://localhost:3000", and NEXT_PUBLIC_NEXTAUTH_URL is not set. Client side path detections will fail.` | ||
) | ||
} | ||
|
||
// Return absolute path when called server side | ||
return `${__NEXTAUTH.baseUrlServer}${__NEXTAUTH.basePathServer}` | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,23 +5,36 @@ | |
* supporting a default value, so a simple split is sufficent. | ||
* @param {string} url | ||
*/ | ||
export default function parseUrl (url) { | ||
export default function parseUrl(url) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function is also called server-side, and the logic currently works because there we pass it I think we should move that logic inside, hence whenever calling export default function parseBaseUrl(url) {
// ..
if (process.env.NEXTAUTH_URL) {
if (!process.env.NEXT_PUBLIC_NEXTAUTH_URL) {
throw new Error("[next-auth] Trying to use a custom `NEXTAUTH_URL` without setting `NEXT_PUBLIC_NEXTAUTH_URL`, client-side redirects won't work") Now that we're modifying this function, I think we should:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, currently, it feels like it's dangling between both client and server.
Agreed. |
||
// Default values | ||
const defaultHost = 'http://localhost:3000' | ||
const defaultPath = '/api/auth' | ||
|
||
if (!url) { url = `${defaultHost}${defaultPath}` } | ||
const defaultHost = "http://localhost:3000" | ||
const defaultPath = "/api/auth" | ||
|
||
if (!url) { | ||
if (process.env.NEXT_PUBLIC_NEXTAUTH_URL) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have the impression we should move this to v4 and change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is intended to be part of v4, otherwise it would be a big breaking change, IMO. I made the pull request against the I like the idea of having a single variable. |
||
/* | ||
if the user has defined NEXT_PUBLIC_NEXTAUTH_URL, which should be mandatory | ||
if they are using a different path other than 'http://localhost:3000' | ||
in NEXTAUTH_URL variable in their .env file, use that variable instead, | ||
otherwise, the clientside path detection will fail. | ||
*/ | ||
url = process.env.NEXT_PUBLIC_NEXTAUTH_URL | ||
} else { | ||
// fallback to default values, user should see a warning if NEXTAUTH_URL is different | ||
// and NEXT_PUBLIC_NEXTAUTH_URL is not set | ||
url = `${defaultHost}${defaultPath}` | ||
} | ||
} | ||
// Default to HTTPS if no protocol explictly specified | ||
const protocol = url.startsWith('http:') ? 'http' : 'https' | ||
const protocol = url.startsWith("http:") ? "http" : "https" | ||
|
||
// Normalize URLs by stripping protocol and no trailing slash | ||
url = url.replace(/^https?:\/\//, '').replace(/\/$/, '') | ||
url = url.replace(/^https?:\/\//, "").replace(/\/$/, "") | ||
|
||
// Simple split based on first / | ||
const [_host, ..._path] = url.split('/') | ||
const [_host, ..._path] = url.split("/") | ||
const baseUrl = _host ? `${protocol}://${_host}` : defaultHost | ||
const basePath = _path.length > 0 ? `/${_path.join('/')}` : defaultPath | ||
const basePath = _path.length > 0 ? `/${_path.join("/")}` : defaultPath | ||
Comment on lines
+29
to
+37
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Out of curiosity, is this something that could be accomplished using Something like: let baseUrl = defaultHost;
let basePath = defaultPath;
try {
url = new URL(url, url.startsWith('http') ? undefined : defaultHost);
baseUrl = url.origin;
basePath = url.pathname.length ? url.pathname : defaultPath;
} catch (e) {} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting PoV. I will try and test this out. Looks like this could work, with some modifications. |
||
|
||
return { baseUrl, basePath } | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,17 @@ if (!process.env.NEXTAUTH_URL) { | |
logger.warn("NEXTAUTH_URL", "NEXTAUTH_URL environment variable not set") | ||
} | ||
|
||
if ( | ||
process.env.NODE_ENV === "development" && | ||
!/^http:\/\/localhost:\d+$/.test(process.env.NEXTAUTH_URL) && | ||
!process.env.NEXT_PUBLIC_NEXTAUTH_URL | ||
) { | ||
logger.warn( | ||
"NEXT_PUBLIC_NEXTAUTH_URL", | ||
`NEXTAUTH_URL is set to "${process.env.NEXTAUTH_URL}" instead of the default "http://localhost:\${PORT}", and NEXT_PUBLIC_NEXTAUTH_URL is not set. Client side path detections will fail.` | ||
) | ||
} | ||
Comment on lines
+23
to
+32
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In my opinion, we should move this logic to On the other hand, we should be throwing errors in both cases rather than warnings as this misconfiguration, I understand, is critical? 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I agree on this. I could not think of a better message, so I left it as warning, but it should definitely be treated as error. |
||
|
||
/** | ||
* @param {import("next").NextApiRequest} req | ||
* @param {import("next").NextApiResponse} res | ||
|
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.
I think there's no need for comments here, the code is pretty self-descriptive 👍🏽
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.
Makes sense.