fix: auth validation on existing sessions, rely on token only (#1069)

* chore: use http.NoBody

* fix: remove cookie token on logout

* fix: remove token cookie on middleware and redirect

* fix: frontend sets cookie token if authenticated

* refactor: remove session-id, rely on token only

* docs: make swagger

* fix: redirect

* fix: archive route handler

* fix: properly unset cookie

docs/swagger/docs.go

@@ -439,13 +439,8 @@ const docTemplate = `{
             "type": "object",
             "properties": {
                 "expires": {
-                    "description": "Deprecated, used only for legacy APIs",
                     "type": "integer"
                 },
-                "session": {
-                    "description": "Deprecated, used only for legacy APIs",
-                    "type": "string"
-                },
                 "token": {
                     "type": "string"
                 }

docs/swagger/swagger.json

@@ -428,13 +428,8 @@
             "type": "object",
             "properties": {
                 "expires": {
-                    "description": "Deprecated, used only for legacy APIs",
                     "type": "integer"
                 },
-                "session": {
-                    "description": "Deprecated, used only for legacy APIs",
-                    "type": "string"
-                },
                 "token": {
                     "type": "string"
                 }

docs/swagger/swagger.yaml

@@ -27,11 +27,7 @@ definitions:
   api_v1.loginResponseMessage:
     properties:
       expires:
-        description: Deprecated, used only for legacy APIs
         type: integer
-      session:
-        description: Deprecated, used only for legacy APIs
-        type: string
       token:
         type: string
     type: object

internal/http/handlers/api/v1/accounts_test.go

@@ -357,9 +357,7 @@ func TestHandleUpdateAccount(t *testing.T) {
 
 		// Verify we can login with new password
 		loginBody := `{"username": "shiori", "password": "newpass"}`
-		w = testutil.PerformRequest(deps, func(deps model.Dependencies, c model.WebContext) {
+		w = testutil.PerformRequest(deps, HandleLogin, "POST", "/login", testutil.WithBody(loginBody))
-			HandleLogin(deps, c, noopLegacyLoginHandler)
-		}, "POST", "/login", testutil.WithBody(loginBody))
 		require.Equal(t, http.StatusOK, w.Code)
 	})
 
@@ -480,9 +478,7 @@ func TestHandleUpdateAccount(t *testing.T) {
 
 		// Verify password change
 		loginBody := `{"username": "updated", "password": "newpass"}`
-		w = testutil.PerformRequest(deps, func(deps model.Dependencies, c model.WebContext) {
+		w = testutil.PerformRequest(deps, HandleLogin, "POST", "/login", testutil.WithBody(loginBody))
-			HandleLogin(deps, c, noopLegacyLoginHandler)
-		}, "POST", "/login", testutil.WithBody(loginBody))
 		require.Equal(t, http.StatusOK, w.Code)
 	})
 }

internal/http/handlers/api/v1/auth.go

@@ -29,8 +29,7 @@ func (p *loginRequestPayload) IsValid() error {
 
 type loginResponseMessage struct {
 	Token      string `json:"token"`
-	SessionID  string `json:"session"` // Deprecated, used only for legacy APIs
+	Expiration int64  `json:"expires"`
-	Expiration int64  `json:"expires"` // Deprecated, used only for legacy APIs
 }
 
 // @Summary	Login to an account using username and password
@@ -41,7 +40,7 @@ type loginResponseMessage struct {
 // @Success	200		{object}	loginResponseMessage	"Login successful"
 // @Failure	400		{object}	nil						"Invalid login data"
 // @Router		/api/v1/auth/login [post]
-func HandleLogin(deps model.Dependencies, c model.WebContext, legacyLoginHandler model.LegacyLoginHandler) {
+func HandleLogin(deps model.Dependencies, c model.WebContext) {
 	var payload loginRequestPayload
 	if err := json.NewDecoder(c.Request().Body).Decode(&payload); err != nil {
 		response.SendError(c, http.StatusBadRequest, "Invalid JSON payload", nil)
@@ -72,16 +71,8 @@ func HandleLogin(deps model.Dependencies, c model.WebContext, legacyLoginHandler
 		return
 	}
 
-	sessionID, err := legacyLoginHandler(account, expiration)
-	if err != nil {
-		deps.Logger().WithError(err).Error("failed execute legacy login handler")
-		response.SendInternalServerError(c)
-		return
-	}
-
 	response.Send(c, http.StatusOK, loginResponseMessage{
 		Token:      token,
-		SessionID:  sessionID,
 		Expiration: expirationTime.Unix(),
 	})
 }
@@ -215,5 +206,12 @@ func HandleLogout(deps model.Dependencies, c model.WebContext) {
 	if err := middleware.RequireLoggedInUser(deps, c); err != nil {
 		return
 	}
+
+	// Remove token cookie
+	c.Request().AddCookie(&http.Cookie{
+		Name:  "token",
+		Value: "",
+	})
+
 	response.Send(c, http.StatusOK, nil)
 }

internal/http/handlers/api/v1/auth_test.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"net/http"
 	"testing"
-	"time"
 
 	"github.com/go-shiori/shiori/internal/model"
 	"github.com/go-shiori/shiori/internal/testutil"
@@ -12,10 +11,6 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func noopLegacyLoginHandler(_ *model.AccountDTO, _ time.Duration) (string, error) {
-	return "test-session", nil
-}
-
 func TestHandleLogin(t *testing.T) {
 	logger := logrus.New()
 	// _, deps := testutil.GetTestConfigurationAndDependencies(t, context.Background(), logger)
@@ -24,9 +19,7 @@ func TestHandleLogin(t *testing.T) {
 		ctx := context.Background()
 		_, deps := testutil.GetTestConfigurationAndDependencies(t, ctx, logger)
 		body := `{"username":}`
-		w := testutil.PerformRequest(deps, func(deps model.Dependencies, c model.WebContext) {
+		w := testutil.PerformRequest(deps, HandleLogin, "POST", "/login", testutil.WithBody(body))
-			HandleLogin(deps, c, noopLegacyLoginHandler)
-		}, "POST", "/login", testutil.WithBody(body))
 		require.Equal(t, http.StatusBadRequest, w.Code)
 	})
 
@@ -34,9 +27,7 @@ func TestHandleLogin(t *testing.T) {
 		ctx := context.Background()
 		_, deps := testutil.GetTestConfigurationAndDependencies(t, ctx, logger)
 		body := `{"password": "test"}`
-		w := testutil.PerformRequest(deps, func(deps model.Dependencies, c model.WebContext) {
+		w := testutil.PerformRequest(deps, HandleLogin, "POST", "/login", testutil.WithBody(body))
-			HandleLogin(deps, c, noopLegacyLoginHandler)
-		}, "POST", "/login", testutil.WithBody(body))
 		require.Equal(t, http.StatusBadRequest, w.Code)
 	})
 
@@ -44,9 +35,7 @@ func TestHandleLogin(t *testing.T) {
 		ctx := context.Background()
 		_, deps := testutil.GetTestConfigurationAndDependencies(t, ctx, logger)
 		body := `{"username": "test"}`
-		w := testutil.PerformRequest(deps, func(deps model.Dependencies, c model.WebContext) {
+		w := testutil.PerformRequest(deps, HandleLogin, "POST", "/login", testutil.WithBody(body))
-			HandleLogin(deps, c, noopLegacyLoginHandler)
-		}, "POST", "/login", testutil.WithBody(body))
 		require.Equal(t, http.StatusBadRequest, w.Code)
 	})
 
@@ -54,9 +43,7 @@ func TestHandleLogin(t *testing.T) {
 		ctx := context.Background()
 		_, deps := testutil.GetTestConfigurationAndDependencies(t, ctx, logger)
 		body := `{"username": "test", "password": "wrong"}`
-		w := testutil.PerformRequest(deps, func(deps model.Dependencies, c model.WebContext) {
+		w := testutil.PerformRequest(deps, HandleLogin, "POST", "/login", testutil.WithBody(body))
-			HandleLogin(deps, c, noopLegacyLoginHandler)
-		}, "POST", "/login", testutil.WithBody(body))
 		require.Equal(t, http.StatusBadRequest, w.Code)
 	})
 
@@ -74,16 +61,13 @@ func TestHandleLogin(t *testing.T) {
 			"password": "test",
 			"remember_me": true
 		}`
-		w := testutil.PerformRequest(deps, func(deps model.Dependencies, c model.WebContext) {
+		w := testutil.PerformRequest(deps, HandleLogin, "POST", "/login", testutil.WithBody(body))
-			HandleLogin(deps, c, noopLegacyLoginHandler)
-		}, "POST", "/login", testutil.WithBody(body))
 		require.Equal(t, http.StatusOK, w.Code)
 
 		response, err := testutil.NewTestResponseFromReader(w.Body)
 		require.NoError(t, err)
 		response.AssertOk(t)
 		response.AssertMessageContains(t, "token")
-		response.AssertMessageContains(t, "session")
 		response.AssertMessageContains(t, "expires")
 	})
 }

internal/http/handlers/bookmark.go

@@ -5,7 +5,6 @@ import (
 	"html/template"
 	"net/http"
 	"strconv"
-	"strings"
 
 	"github.com/go-shiori/shiori/internal/http/response"
 	"github.com/go-shiori/shiori/internal/model"
@@ -92,8 +91,7 @@ func HandleBookmarkArchiveFile(deps model.Dependencies, c model.WebContext) {
 		return
 	}
 
-	// Get resource path from URL
+	resourcePath := c.Request().PathValue("path")
-	resourcePath := strings.TrimPrefix(c.Request().URL.Path, fmt.Sprintf("/bookmark/%d/archive/file/", bookmark.ID))
 
 	archive, err := deps.Domains().Archiver().GetBookmarkArchive(bookmark)
 	if err != nil {

internal/http/handlers/legacy.go

@@ -50,15 +50,18 @@ func (h *LegacyHandler) HandleLogin(account *model.AccountDTO, expTime time.Dura
 	}
 
 	strSessionID := sessionID.String()
-	h.legacyHandler.SessionCache.Set(strSessionID, account, expTime)
 
 	return strSessionID, nil
 }
 
 // HandleLogout handles the legacy logout endpoint
 func (h *LegacyHandler) HandleLogout(deps model.Dependencies, c model.WebContext) {
-	sessionID := h.legacyHandler.GetSessionID(c.Request())
+	// TODO: Leave cookie handling to API consumer or middleware?
-	h.legacyHandler.SessionCache.Delete(sessionID)
+	// Remove token cookie
+	c.Request().AddCookie(&http.Cookie{
+		Name:  "token",
+		Value: "",
+	})
 }
 
 // HandleGetTags handles GET /api/tags

internal/http/handlers/legacy_test.go

@@ -36,30 +36,6 @@ func TestLegacyHandler(t *testing.T) {
 		sessionID, err := handler.HandleLogin(account, time.Hour)
 		require.NoError(t, err)
 		require.NotEmpty(t, sessionID)
-
-		// Verify session is stored
-		val, found := handler.legacyHandler.SessionCache.Get(sessionID)
-		require.True(t, found)
-		require.Equal(t, account, val)
-	})
-
-	t.Run("HandleLogout", func(t *testing.T) {
-		// Setup session
-		account := &model.AccountDTO{ID: 1}
-		sessionID, _ := handler.HandleLogin(account, time.Hour)
-
-		// Create request with session cookie
-		c, _ := testutil.NewTestWebContext()
-		c.Request().AddCookie(&http.Cookie{
-			Name:  "session-id",
-			Value: sessionID,
-		})
-
-		handler.HandleLogout(deps, c)
-
-		// Verify session is removed
-		_, found := handler.legacyHandler.SessionCache.Get(sessionID)
-		require.False(t, found)
 	})
 
 	t.Run("HandleGetTags", func(t *testing.T) {
@@ -79,7 +55,7 @@ func TestLegacyHandler(t *testing.T) {
 	})
 
 	t.Run("convertParams", func(t *testing.T) {
-		r, _ := http.NewRequest(http.MethodGet, "/api/bookmarks?page=1&tags=test,dev", nil)
+		r, _ := http.NewRequest(http.MethodGet, "/api/bookmarks?page=1&tags=test,dev", http.NoBody)
 		params := handler.convertParams(r)
 
 		require.Len(t, params, 2)

internal/http/middleware/auth.go

@@ -32,8 +32,14 @@ func (m *AuthMiddleware) OnRequest(deps model.Dependencies, c model.WebContext)
 
 	account, err := deps.Domains().Auth().CheckToken(c.Request().Context(), token)
 	if err != nil {
-		deps.Logger().WithError(err).Error("Failed to check token")
+		// If we fail to check token, remove the token cookie and redirect to login
-		return err
+		deps.Logger().WithError(err).WithField("request_id", c.GetRequestID()).Error("Failed to check token")
+		http.SetCookie(c.ResponseWriter(), &http.Cookie{
+			Name:   "token",
+			Value:  "",
+			MaxAge: -1,
+		})
+		return nil
 	}
 
 	c.SetAccount(account)

internal/http/middleware/auth_test.go

@@ -64,6 +64,39 @@ func TestAuthMiddleware(t *testing.T) {
 		require.NoError(t, err)
 		require.NotNil(t, c.GetAccount())
 	})
+
+	t.Run("test invalid token cookie is removed", func(t *testing.T) {
+		// Create an invalid token
+		invalidToken := "invalid-token"
+
+		w := httptest.NewRecorder()
+		r := httptest.NewRequest(http.MethodGet, "/", nil)
+		r.AddCookie(&http.Cookie{
+			Name:   "token",
+			Value:  invalidToken,
+			MaxAge: int(time.Now().Add(time.Minute).Unix()),
+		})
+		c := webcontext.NewWebContext(w, r)
+
+		middleware := NewAuthMiddleware(deps)
+		err := middleware.OnRequest(deps, c)
+		require.NoError(t, err)
+		require.Nil(t, c.GetAccount())
+
+		// Check that the token cookie was removed in the response
+		responseCookies := w.Result().Cookies()
+
+		var tokenCookie *http.Cookie
+		for _, cookie := range responseCookies {
+			if cookie.Name == "token" {
+				tokenCookie = cookie
+				break
+			}
+		}
+
+		require.NotNil(t, tokenCookie, "Token cookie should exist in response")
+		require.Empty(t, tokenCookie.Value, "Token cookie value should be empty")
+	})
 }
 
 func TestRequireLoggedInUser(t *testing.T) {

internal/http/server.go

@@ -49,7 +49,7 @@ func (s *HttpServer) Setup(cfg *config.Config, deps *dependencies.Dependencies)
 	// Bookmark routes
 	s.mux.HandleFunc("GET /bookmark/{id}/content", ToHTTPHandler(deps, handlers.HandleBookmarkContent, globalMiddleware...))
 	s.mux.HandleFunc("GET /bookmark/{id}/archive", ToHTTPHandler(deps, handlers.HandleBookmarkArchive, globalMiddleware...))
-	s.mux.HandleFunc("GET /bookmark/{id}/archive/file/{file}", ToHTTPHandler(deps, handlers.HandleBookmarkArchiveFile, globalMiddleware...))
+	s.mux.HandleFunc("GET /bookmark/{id}/archive/file/{path...}", ToHTTPHandler(deps, handlers.HandleBookmarkArchiveFile, globalMiddleware...))
 	s.mux.HandleFunc("GET /bookmark/{id}/thumb", ToHTTPHandler(deps, handlers.HandleBookmarkThumbnail, globalMiddleware...))
 	s.mux.HandleFunc("GET /bookmark/{id}/ebook", ToHTTPHandler(deps, handlers.HandleBookmarkEbook, globalMiddleware...))
 
@@ -97,10 +97,7 @@ func (s *HttpServer) Setup(cfg *config.Config, deps *dependencies.Dependencies)
 	// API v1 routes
 	// Auth
 	s.mux.HandleFunc("POST /api/v1/auth/login", ToHTTPHandler(deps,
-		func(deps model.Dependencies, c model.WebContext) {
+		api_v1.HandleLogin,
-			// TODO: Remove this once the legacy API is removed
-			api_v1.HandleLogin(deps, c, legacyHandler.HandleLogin)
-		},
 		globalMiddleware...,
 	))
 	s.mux.HandleFunc("POST /api/v1/auth/refresh", ToHTTPHandler(deps,

internal/view/assets/js/component/login.js

@@ -106,7 +106,7 @@ export default {
 			}
 
 			// Remove old cookie
-			document.cookie = `session-id=; Path=${
+			document.cookie = `token=; Path=${
 				new URL(document.baseURI).pathname
 			}; Expires=Thu, 01 Jan 1970 00:00:00 GMT;`;
 
@@ -128,9 +128,6 @@ export default {
 				})
 				.then((json) => {
 					// Save session id
-					document.cookie = `session-id=${json.message.session}; Path=${
-						new URL(document.baseURI).pathname
-					}; Expires=${new Date(json.message.expires * 1000).toUTCString()}`;
 					document.cookie = `token=${json.message.token}; Path=${
 						new URL(document.baseURI).pathname
 					}; Expires=${new Date(json.message.expires * 1000).toUTCString()}`;
@@ -186,9 +183,6 @@ export default {
 		}
 
 		// Clear session data if we reach here
-		document.cookie = `session-id=; Path=${
-			new URL(document.baseURI).pathname
-		}; Expires=Thu, 01 Jan 1970 00:00:00 GMT;`;
 		document.cookie = `token=; Path=${
 			new URL(document.baseURI).pathname
 		}; Expires=Thu, 01 Jan 1970 00:00:00 GMT;`;

internal/view/assets/js/page/base.js

@@ -135,7 +135,7 @@ export default {
 						var loginUrl = new Url("login", document.baseURI);
 						loginUrl.query.dst = window.location.href;
 
-						document.cookie = `session-id=; Path=${
+						document.cookie = `token=; Path=${
 							new URL(document.baseURI).pathname
 						}; Expires=Thu, 01 Jan 1970 00:00:00 GMT;`;
 						location.href = loginUrl.toString();

internal/view/index.html

@@ -95,14 +95,16 @@
 						mainClick: () => {
 							this.dialog.loading = true;
 							fetch(new URL("api/v1/auth/logout", document.baseURI), {
-								method: "post"
+								method: "post",
+								headers: {
+									"Authorization": `Bearer ${localStorage.getItem("shiori-token")}`
+								}
 							}).then(response => {
 								if (!response.ok) throw response;
 								return response;
 							}).then(() => {
 								localStorage.removeItem("shiori-account");
 								localStorage.removeItem("shiori-token");
-								document.cookie = `session-id=; Path=${new URL(document.baseURI).pathname}; Expires=Thu, 01 Jan 1970 00:00:00 GMT;`;
 								document.cookie = `token=; Path=${new URL(document.baseURI).pathname}; Expires=Thu, 01 Jan 1970 00:00:00 GMT;`;
 								this.isLoggedIn = false;
 								this.loginRequired = true;
@@ -189,7 +191,6 @@
 						// Clear invalid session data
 						localStorage.removeItem("shiori-account");
 						localStorage.removeItem("shiori-token");
-						document.cookie = `session-id=; Path=${new URL(document.baseURI).pathname}; Expires=Thu, 01 Jan 1970 00:00:00 GMT;`;
 						document.cookie = `token=; Path=${new URL(document.baseURI).pathname}; Expires=Thu, 01 Jan 1970 00:00:00 GMT;`;
 						return false;
 					}
@@ -202,6 +203,11 @@
 					if (isValid) {
 						this.loadSetting();
 						this.loadAccount();
+
+						// Set the token cookie if empty
+						if (!document.cookie.includes("token")) {
+							document.cookie = `token=${localStorage.getItem("shiori-token")}; Path=${new URL(document.baseURI).pathname}; Expires=${new Date(Date.now() + 1000 * 60 * 60 * 24 * 30).toUTCString()};`;
+						}
 					} else {
 						this.loginRequired = true;
 					}

internal/webserver/handler.go

@@ -16,7 +16,7 @@ type Handler struct {
 	DataDir   string
 	RootPath  string
 	UserCache *cch.Cache
-	SessionCache *cch.Cache
+	// SessionCache *cch.Cache
 	ArchiveCache *cch.Cache
 	Log          bool
 
@@ -24,55 +24,53 @@ type Handler struct {
 }
 
 func (h *Handler) PrepareSessionCache() {
-	h.SessionCache.OnEvicted(func(key string, val interface{}) {
+	// h.SessionCache.OnEvicted(func(key string, val interface{}) {
-		account := val.(*model.AccountDTO)
+	// 	account := val.(*model.AccountDTO)
-		arr, found := h.UserCache.Get(account.Username)
+	// 	arr, found := h.UserCache.Get(account.Username)
-		if !found {
+	// 	if !found {
-			return
+	// 		return
-		}
+	// 	}
 
-		sessionIDs := arr.([]string)
+	// 	sessionIDs := arr.([]string)
-		for i := 0; i < len(sessionIDs); i++ {
+	// 	for i := 0; i < len(sessionIDs); i++ {
-			if sessionIDs[i] == key {
+	// 		if sessionIDs[i] == key {
-				sessionIDs = append(sessionIDs[:i], sessionIDs[i+1:]...)
+	// 			sessionIDs = append(sessionIDs[:i], sessionIDs[i+1:]...)
-				break
+	// 			break
-			}
+	// 		}
-		}
+	// 	}
 
-		h.UserCache.Set(account.Username, sessionIDs, -1)
+	// 	h.UserCache.Set(account.Username, sessionIDs, -1)
-	})
+	// })
-}
-
-func (h *Handler) GetSessionID(r *http.Request) string {
-	// Try to get session ID from the header
-	sessionID := r.Header.Get("X-Session-Id")
-
-	// If not, try it from the cookie
-	if sessionID == "" {
-		cookie, err := r.Cookie("session-id")
-		if err != nil {
-			return ""
-		}
-
-		sessionID = cookie.Value
-	}
-
-	return sessionID
 }
 
 // validateSession checks whether user session is still valid or not
 func (h *Handler) validateSession(r *http.Request) error {
 	authorization := r.Header.Get(model.AuthorizationHeader)
+	if authorization == "" {
+		// Get token from cookie
+		tokenCookie, err := r.Cookie("token")
+		if err != nil {
+			return fmt.Errorf("session is not exist")
+		}
+
+		authorization = tokenCookie.Value
+	}
+
+	var account *model.AccountDTO
+
 	if authorization != "" {
+		var err error
+
 		authParts := strings.SplitN(authorization, " ", 2)
 		if len(authParts) != 2 && authParts[0] != model.AuthorizationTokenType {
 			return fmt.Errorf("session has been expired")
 		}
 
-		account, err := h.dependencies.Domains().Auth().CheckToken(r.Context(), authParts[1])
+		account, err = h.dependencies.Domains().Auth().CheckToken(r.Context(), authParts[1])
 		if err != nil {
 			return fmt.Errorf("session has been expired")
 		}
+	}
 
 	if r.Method != "" && r.Method != "GET" && account.Owner != nil && !*account.Owner {
 		return fmt.Errorf("account level is not sufficient")
@@ -85,25 +83,5 @@ func (h *Handler) validateSession(r *http.Request) error {
 	}).Info("allowing legacy api access using JWT token")
 
 	return nil
-	}
 
-	sessionID := h.GetSessionID(r)
-	if sessionID == "" {
-		return fmt.Errorf("session is not exist")
-	}
-
-	// Make sure session is not expired yet
-	val, found := h.SessionCache.Get(sessionID)
-	if !found {
-		return fmt.Errorf("session has been expired")
-	}
-
-	// If this is not get request, make sure it's owner
-	if r.Method != "" && r.Method != "GET" {
-		if account := val.(*model.AccountDTO); account.Owner != nil && !*account.Owner {
-			return fmt.Errorf("account level is not sufficient")
-		}
-	}
-
-	return nil
 }

internal/webserver/server.go

@@ -23,7 +23,7 @@ func GetLegacyHandler(cfg Config, dependencies model.Dependencies) *Handler {
 		DB:        cfg.DB,
 		DataDir:   cfg.DataDir,
 		UserCache: cch.New(time.Hour, 10*time.Minute),
-		SessionCache: cch.New(time.Hour, 10*time.Minute),
+		// SessionCache: cch.New(time.Hour, 10*time.Minute),
 		ArchiveCache: cch.New(time.Minute, 5*time.Minute),
 		RootPath:     cfg.RootPath,
 		Log:          cfg.Log,