meteorlxy / vssue

:mailbox: A Vue-powered Issue-based Comment Plugin
https://vssue.js.org
MIT License
777 stars 107 forks source link

[Feature Request] make redirect_uri work until gitlab fix their issue. #99

Closed mohan43u closed 4 years ago

mohan43u commented 4 years ago

What problem does the feature solve?

gitlab oauth should work from any static page. Not just from the document root.

Problem: Currently gitlab oauth workflow is not working when user from a static website try to comment on a particular page other than the root page. This issue was already reported here. But it was closed citing this gitlab issue

Proposed solution

The default redirect_uri= value sent by Vssue to GitLab is window.location.href without [prefix], Instead of sending window.location.href, why not send redirect_uri=window.locaiton.origin with state property containing base64 encoded object with original state and local redirect_uri where this internal redirect_uri should contain window.location.href, so when oath completes, Vssue can check for redirect_uri in state and again redirect to the original url which initiated the oauth request.

cybermoloch commented 4 years ago

This sounds like a good fix since the OAuth standard discourages wildcard matching for the redirect URI and many other OAuth providers do the same as GitLab.

I did try this however and it does seem to break some parts of SPAs. Specifically I was attempting to use Vssue with Vuelog and the URL encoding breaks a few things. The encoding of the URL replacing all special characters gives 404 errors with Vuelog --this is especially an issue as it rewrites the URL bar in the browser which breaks a simple refresh of the page. Many SPAs use an anchor tag (#) in their URL for routing so even the current release of Vssue won't work as you cannot put # in the redirect URI either.

cybermoloch commented 4 years ago

As @meteorlxy mentioned in another issue (https://github.com/meteorlxy/vssue/issues/76), this needs to be applied to all APIs. Passing the current URL as a Base64 string could work for all of them instead of passing the current URL encoded.

I think because we are passing the state into Base64, we may not need to encode it first. (Base64 will accept any single byte characters and any non ASCII characters should already be encoded by the browser as it would be an invalid URL otherwise?) As I previously mentioned, I am using this in an SPA with vue-router in hash mode (Vuelog). Although having vue-router work in history mode might help somewhat, many static hosting will not allow changing parameters for doing a catch-all redirect. (See https://router.vuejs.org/guide/essentials/history-mode.html#example-server-configurations for more info.) Even so, we need to make sure Vssue works in both modes.

Another consideration is that the window.location.origin without the full state loaded may not include the Vssue component so if Vssue is responsible for handling the state and going back to the original page, it may not work as intended. In that case, a new route for should be added and used as the routing for Vssue OAuth requests. An alternative would be to have a Vssue stub component to handle this on another page even if it doesn't allow comments but that seems messier.

meteorlxy commented 4 years ago

Sorry for delay.

I'm planning for Vssue 2.0 with Vue 3.0, which is expected to solve some known issues in a more elegant way.

I'll refer to that PR and our discussions here. Great thanks :smile:

mohan43u commented 4 years ago

@cybermoloch

I think because we are passing the state into Base64, we may not need to encode it first.

This patch not doing any URL encoding. We json stringify one state object then convert it into base64

https://github.com/meteorlxy/vssue/pull/101/files#diff-da508bd9be97a1016faa11587b990b23

No URL encoding involved here. Also, I'm so new to web and not able to understand most of the problems you explained in your comments. I use Vssue with this patch in my personal blog which works as intended. If possible, could you please provide some way to see how this patch not works in SPA?

cybermoloch commented 4 years ago

@mohan43u The encoding I think is BEFORE you start to handle it with your modifications but it is a rabbit hole of functions that do it.

You can do this as a test on your local machine using npm and Vuelog. I have a fork here where I am adding support to Vuelog for Vssue. https://github.com/cybermoloch/Vuelog/tree/feature-vssue

  1. Download the code (git or zip) and run npm install
  2. Run npm run serve
  3. Modify database.js to enable Vssue (in public/userdata) and change the API to GitLab.
  4. Go to a blog post such as showcase -> style examples; the URL is http://localhost:8080/#/blog/showcase/2016/style-examples/ [Normal Vssue]
  5. Replace Vssue with your version and build it. (Download code, run yarn from directory, copy lib files for GitLab API to node_modules\@vssue\api-gitlab-v4\lib in Vuelog directory.)
  6. Page will reload but will quickly replace the URL with http://localhost:8080/#%2Fblog%2Fshowcase%2F2016%2Fstyle-examples%2F=.
  7. Using this URL (refresh or otherwise) will result in the equivalent of a 404 error (Oops! The page you were looking for doesn't exist.) The actual code that does this URL replacement in the address bar is line 79 in the built js file: window.history.replaceState(null, '', replaceURL); but I am assuming it uses this as a base to form the rest of the base 64 operation, state handling operations.
stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

mohan43u commented 4 years ago

@cybermoloch I did some fix to my patch to make sure hash string is not omitted after redirection. Kindly check if it works for you now.