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

RFC: Improve the handing of baseURL to be more intuitive #797

Closed
5 tasks
zoey-kaiser opened this issue Jul 13, 2024 · 3 comments · Fixed by #913
Closed
5 tasks

RFC: Improve the handing of baseURL to be more intuitive #797

zoey-kaiser opened this issue Jul 13, 2024 · 3 comments · Fixed by #913
Labels
enhancement An improvement that needs to be added rfc Request for Comments: Needs discussion

Comments

@zoey-kaiser
Copy link
Member

Describe the feature

The current logic used to determine if the auth.baseURL is internal or external is misleading and hard to understand. The main issue is that the module needs external URLs to be configured in a very specific way to properly identify it as an external URL.

Examples taken from #782 by @coreyshuman:

Example 1 - Incorrect URL

The sign in path becomes: http://base-url/api/auth/http://api-url/login

auth: {
  baseURL: 'http://base-url',
  provider: {
    endpoints: {
      signIn: {path: 'http://api-url/login'},
    },
}

Example 2 - Incorrect URL

Add the ending slash on baseURL. The sign in path becomes: http://base-url/http://api-url/login

auth: {
  baseURL: 'http://base-url/',
  provider: {
    endpoints: {
      signIn: {path: 'http://api-url/login'},
    },
}

Example 3 - Incorrect URL

Delete baseURL. The sign in path becomes: http://localhost:3000/api/auth/http://api-url/login

auth: {
  baseURL: '',
  provider: {
    endpoints: {
      signIn: {path: 'http://api-url/login'},
    },
}

Example 4 - Correct URL

The sign in path becomes: http://api-url/login, allowing me to use my external endpoint.

auth: {
  baseURL: 'http://api-url/',
  provider: {
    endpoints: {
      signIn: {path: 'login'},
    },
}

In #796 we updated the docs to better explain this behaviour, however we should look into improving this, to make it less complex.

How would you implement this?

Some potential ways to better determine if a URL is internal or external:

  • Using schema (https or http) to signify a URL is external
  • Ignoring trailing and leading / inside the baseURL and endpoints and then composing and adding them as needed, to better infer the URLs, instead of just combining them

More suggestions are welcome!

Additional information

  • Would you be willing to help implement this feature?

Provider

  • AuthJS
  • Local
  • Refresh
  • New Provider
@zoey-kaiser zoey-kaiser added enhancement An improvement that needs to be added pending An issue waiting for triage rfc Request for Comments: Needs discussion and removed pending An issue waiting for triage labels Jul 13, 2024
@coreyshuman
Copy link

Hello Zoey, thank you for opening this RFC!

I think an important improvement for this is to ensure that an absolute URL is used as-is and does not attempt to prepend the baseURL. Looking at the $fetch implementation, it uses withBase to intelligently append a path and base URL: https://github.com/unjs/ufo/blob/387d01aa02d58384bb59efbd47bc8f2d6320c247/src/utils.ts#L226

This method performs the edge-case handling for:

  • URL already has a protocol / schema
  • base path has extra trailing slash
  • duplicate path in the base and URL paths

This could be implemented by replacing joinURL with withBase in url.ts:

return joinURL(base, path)

This would have the added benefit that someone could use different servers for each endpoint in their configuration, in an extreme edge use-case. For example:

endpoints: {
  signIn: {path: 'https://auth-server/login'},
  getSession: { path: 'https://session-server/session' },
}

We would want to consider how this change may break existing configurations, but I think it would provide a much more intuitive behavior.

Pending further discussion and ideas, I'm happy to contribute a PR for this.

@zoey-kaiser
Copy link
Member Author

zoey-kaiser commented Jul 19, 2024

I made a quick demo to test out serval options of a baseURL and a path to ensure that the following conditions are still meet:

  • URLs are correctly composed regardless of where and how / are placed
  • Internal URLs are still called internally, without any protocol or host

The best solution I came up with was to use withBase as @coreyshuman pointed out!

function joinMethodTwo(base: string, path: string) {
  return withBase(path, base)
}

Screenshot 2024-07-19 at 17 14 16

@tpkee
Copy link

tpkee commented Aug 8, 2024

Referring to issue #840, I believe the URL should not be automatically formatted by the library. Instead, the library should validate the URL and return an error if it's invalid—or at the very least, log a warning. Currently, no error is reported, making debugging quite challenging.

To address this, I propose the following approach:

function formatUrl (base: string = ‘’, path: string) {
 let host = base
 if (!base) {
  host = location.origin
 }
 try {
   const url = new URL(path, host) // will throw an error if it can’t form a proper URL
   return url.href
  } catch (err) {
   // return the error or handle it
  }
}

One potential drawback of this approach is that if the user passes another URL as the endpoint’s path (e.g., https://example.com/donde/), the base URL won't be applied. However, I don't see this as a significant issue. In some cases, the user might intentionally want to call a different URL for a specific endpoint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An improvement that needs to be added rfc Request for Comments: Needs discussion
Projects
None yet
3 participants