Skip to content

Commit

Permalink
fix: login redirect not working properly after component refactor (#1044
Browse files Browse the repository at this point in the history
)

* feat: Add secure destination URL handling for login redirect

* fix: rootpath on redirect to login

* fixed tests
  • Loading branch information
fmartingr authored Dec 31, 2024
1 parent 0b745c1 commit e1e5828
Show file tree
Hide file tree
Showing 5 changed files with 53 additions and 9 deletions.
12 changes: 10 additions & 2 deletions internal/http/response/shortcuts.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package response

import (
"net/http"
"net/url"

"github.com/gin-gonic/gin"
)
Expand Down Expand Up @@ -35,10 +36,17 @@ func SendInternalServerError(ctx *gin.Context) {
}

// SendNotFound directly sends a not found response
func RedirectToLogin(ctx *gin.Context, dst string) {
ctx.Redirect(http.StatusFound, "/login?dst="+dst)
func RedirectToLogin(ctx *gin.Context, webroot, dst string) {
url := url.URL{
Path: webroot,
RawQuery: url.Values{
"dst": []string{dst},
}.Encode(),
}
ctx.Redirect(http.StatusFound, url.String())
}

// NotFound directly sends a not found response
func NotFound(ctx *gin.Context) {
ctx.AbortWithStatus(http.StatusNotFound)
}
2 changes: 1 addition & 1 deletion internal/http/routes/bookmark.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ func (r *BookmarkRoutes) getBookmark(c *context.Context) (*model.BookmarkDTO, er
}

if bookmark.Public != 1 && !c.UserIsLogged() {
response.RedirectToLogin(c.Context, c.Request.URL.String())
response.RedirectToLogin(c.Context, r.deps.Config.Http.RootPath, c.Request.URL.String())
return nil, model.ErrUnauthorized
}

Expand Down
3 changes: 2 additions & 1 deletion internal/http/routes/bookmark_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"net/http"
"net/http/httptest"
"net/url"
"strconv"
"testing"

Expand Down Expand Up @@ -122,7 +123,7 @@ func TestBookmarkContentHandler(t *testing.T) {
req, _ := http.NewRequest("GET", path, nil)
g.ServeHTTP(w, req)
require.Equal(t, http.StatusFound, w.Code)
require.Equal(t, "/login?dst="+path, w.Header().Get("Location"))
require.Equal(t, "/?dst="+url.QueryEscape(path), w.Header().Get("Location"))
})

t.Run("get existing bookmark content", func(t *testing.T) {
Expand Down
33 changes: 33 additions & 0 deletions internal/view/assets/js/component/login.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,35 @@ export default {
username: "",
password: "",
remember: false,
destination: "/", // Default destination
};
},
emits: ["login-success"],
methods: {
sanitizeDestination(dst) {
try {
// Remove any leading/trailing whitespace
dst = dst.trim();

// Decode the URL to handle any encoded characters
dst = decodeURIComponent(dst);

// Create a URL object to parse the destination
const url = new URL(dst, window.location.origin);

// Only allow paths from the same origin
if (url.origin !== window.location.origin) {
return "/";
}

// Only return the pathname and search params
return url.pathname + url.search + url.hash;
} catch (e) {
// If any error occurs during parsing, return root
return "/";
}
},

async getErrorMessage(err) {
switch (err.constructor) {
case Error:
Expand Down Expand Up @@ -119,6 +144,9 @@ export default {

this.visible = false;
this.$emit("login-success");

// Redirect to sanitized destination
if (this.destination !== "/") window.location.href = this.destination;
})
.catch((err) => {
this.loading = false;
Expand All @@ -129,6 +157,11 @@ export default {
},
},
async mounted() {
// Get and sanitize destination from URL parameters
const urlParams = new URLSearchParams(window.location.search);
const dst = urlParams.get("dst");
this.destination = dst ? this.sanitizeDestination(dst) : "/";

// Check if there's a valid session first
const token = localStorage.getItem("shiori-token");
if (token) {
Expand Down
12 changes: 7 additions & 5 deletions internal/view/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,8 @@
document.cookie = `token=; Path=${new URL(document.baseURI).pathname}; Expires=Thu, 01 Jan 1970 00:00:00 GMT;`;
this.isLoggedIn = false;
this.loginRequired = true;
this.dialog.loading = false;
this.dialog.visible = false;
}).catch(err => {
this.dialog.loading = false;
this.getErrorMessage(err).then(msg => {
Expand Down Expand Up @@ -155,7 +157,7 @@
owner: owner,
};
},

onLoginSuccess() {
this.loadSetting();
this.loadAccount();
Expand All @@ -165,7 +167,7 @@
async validateSession() {
const token = localStorage.getItem("shiori-token");
const account = localStorage.getItem("shiori-account");

if (!(token && account)) {
return false;
}
Expand All @@ -176,11 +178,11 @@
"Authorization": `Bearer ${token}`
}
});

if (!response.ok) {
throw new Error('Invalid session');
}

return true;
} catch (err) {
// Clear invalid session data
Expand All @@ -195,7 +197,7 @@
async checkLoginStatus() {
const isValid = await this.validateSession();
this.isLoggedIn = isValid;

if (isValid) {
this.loadSetting();
this.loadAccount();
Expand Down

0 comments on commit e1e5828

Please sign in to comment.