diff --git a/.gitignore b/.gitignore index 840c76a8..afa101cc 100644 --- a/.gitignore +++ b/.gitignore @@ -22,3 +22,4 @@ plikd_*.cfg .vscode plik.iml coverage.out +/plans \ No newline at end of file diff --git a/docs/guide/security.md b/docs/guide/security.md index e8f82c59..c1ee3fc5 100644 --- a/docs/guide/security.md +++ b/docs/guide/security.md @@ -6,6 +6,23 @@ Plik allows users to upload and serve any content as-is. Hosting untrusted conte For security reasons, Plik doesn't trust user-provided MIME types and relies solely on server-side detection. This means some files may not render properly in browsers or embedded viewers that require the correct MIME type. +### Dangerous Content-Type Neutralization + +Plik automatically neutralizes content types that could execute code in the browser: + +| Original type | Served as | Reason | +|---|---|---| +| `text/html`, `*html*` | `application/octet-stream` | Prevents inline script execution | +| `image/svg+xml`, `*svg*` | `application/octet-stream` | SVG can contain `onload` JavaScript handlers | +| `text/xml`, `*xml*` | `application/octet-stream` | XML can be parsed and rendered by browsers | +| `application/javascript`, `*javascript*` | `application/octet-stream` | Prevents script execution | +| `application/x-shockwave-flash` | `application/octet-stream` | Flash content | +| `application/pdf` | `application/octet-stream` | PDF can contain JavaScript | + +::: tip +This protection is always active regardless of `EnhancedWebSecurity`. Use the `?dl=true` query parameter to force a download with `Content-Disposition: attachment`. +::: + ::: warning Office format detection limitation Office formats like `.pptx`, `.docx`, and `.xlsx` are ZIP archives internally, so Go's built-in MIME detector (`http.DetectContentType`) identifies them as `application/zip` instead of their proper types (e.g., `application/vnd.openxmlformats-officedocument.presentationml.presentation`). ::: @@ -18,14 +35,38 @@ When `EnhancedWebSecurity` is enabled in `plikd.cfg`, Plik sets additional HTTP - **X-XSS-Protection**: enabled - **X-Frame-Options**: deny - **Content-Security-Policy**: restrictive policy disabling resource loading, XHR, iframes -- **Secure Cookies**: session cookies only transmitted over HTTPS +- **Strict-Transport-Security**: `max-age=31536000` (1 year) — also set when `SslEnabled` is true +- **Secure Cookies**: session cookies only transmitted over HTTPS — also set when `SslEnabled` is true ::: warning Enhanced security will break audio/video playback, PDF rendering, and other rich content features. Disable it if you need those capabilities. ::: -::: danger Authentication requires HTTPS with EnhancedWebSecurity -When `EnhancedWebSecurity` is enabled, session cookies have the `Secure` flag set and can only be transmitted over HTTPS connections. Authentication will not work over plain HTTP. +::: danger Authentication requires HTTPS with Secure Cookies +When `EnhancedWebSecurity` or `SslEnabled` is enabled, session cookies have the `Secure` flag set and can only be transmitted over HTTPS connections. Authentication will not work over plain HTTP. +::: + +## Upload Password Protection + +When `FeaturePassword` is enabled, uploads can be protected with a login/password pair. Credentials are transmitted via HTTP Basic Authentication. + +Passwords are hashed using **bcrypt(sha256(credentials))** before storage; the plaintext is never persisted. The SHA-256 pre-hash ensures credentials of any length are securely handled within bcrypt's input constraints. + +| Parameter | Limit | +|-----------|-------| +| Login | 128 characters max | +| Password | 128 characters max | + +::: tip +Legacy uploads (created before version 1.4) use MD5 hashing and continue to work until they expire. +::: + +## Removable Uploads + +When `FeatureRemovable` is enabled and an upload is created with `removable: true`, **anyone with the upload URL can delete the upload and its files** — no upload token or authentication is required. This is by design: the `removable` flag is intended for ephemeral, public uploads where ease of cleanup is prioritized over access control. + +::: warning +If you need to control who can delete an upload, do **not** set `removable: true`. Only the upload owner (via upload token, API token, or session) can delete non-removable uploads. ::: ## Download Domain diff --git a/server/common/no_dir_listing.go b/server/common/no_dir_listing.go new file mode 100644 index 00000000..05e0c3a9 --- /dev/null +++ b/server/common/no_dir_listing.go @@ -0,0 +1,20 @@ +package common + +import ( + "net/http" + "strings" +) + +// NoDirListing wraps an http.Handler (typically http.FileServer) to return +// 404 for directory requests instead of generating a directory listing. +// The root path "/" is allowed through so that index.html is served for SPAs. +// All other paths ending in "/" are blocked. +func NoDirListing(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/" && strings.HasSuffix(r.URL.Path, "/") { + http.NotFound(w, r) + return + } + next.ServeHTTP(w, r) + }) +} diff --git a/server/common/no_dir_listing_test.go b/server/common/no_dir_listing_test.go new file mode 100644 index 00000000..3fb52c6f --- /dev/null +++ b/server/common/no_dir_listing_test.go @@ -0,0 +1,60 @@ +package common + +import ( + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestNoDirListing_BlocksSubdirectory(t *testing.T) { + dir := t.TempDir() + subdir := filepath.Join(dir, "subdir") + require.NoError(t, os.Mkdir(subdir, 0755)) + require.NoError(t, os.WriteFile(filepath.Join(subdir, "file.txt"), []byte("hello"), 0644)) + + fs := NoDirListing(http.FileServer(http.Dir(dir))) + + // Subdirectory listing should return 404 + req, err := http.NewRequest("GET", "/subdir/", nil) + require.NoError(t, err) + + rr := httptest.NewRecorder() + fs.ServeHTTP(rr, req) + require.Equal(t, http.StatusNotFound, rr.Code) +} + +func TestNoDirListing_AllowsRootIndex(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "index.html"), []byte("ok"), 0644)) + + fs := NoDirListing(http.FileServer(http.Dir(dir))) + + // Root "/" should pass through (serves index.html) + req, err := http.NewRequest("GET", "/", nil) + require.NoError(t, err) + + rr := httptest.NewRecorder() + fs.ServeHTTP(rr, req) + require.Equal(t, http.StatusOK, rr.Code) + require.Contains(t, rr.Body.String(), "ok") +} + +func TestNoDirListing_AllowsFile(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "test.txt"), []byte("hello"), 0644)) + + fs := NoDirListing(http.FileServer(http.Dir(dir))) + + // File request should succeed + req, err := http.NewRequest("GET", "/test.txt", nil) + require.NoError(t, err) + + rr := httptest.NewRecorder() + fs.ServeHTTP(rr, req) + require.Equal(t, http.StatusOK, rr.Code) + require.Equal(t, "hello", rr.Body.String()) +} diff --git a/server/common/password.go b/server/common/password.go new file mode 100644 index 00000000..d33fc94d --- /dev/null +++ b/server/common/password.go @@ -0,0 +1,28 @@ +package common + +import ( + "crypto/sha256" + "fmt" + + "golang.org/x/crypto/bcrypt" +) + +// HashUploadPassword hashes upload credentials using bcrypt(sha256(base64(login:password))). +// The SHA-256 pre-hash produces a fixed 32-byte (64-hex) digest, removing bcrypt's +// 72-byte input limit and allowing arbitrarily long credentials. +func HashUploadPassword(login string, password string) (string, error) { + b64 := EncodeAuthBasicHeader(login, password) + digest := sha256.Sum256([]byte(b64)) + hash, err := bcrypt.GenerateFromPassword(fmt.Appendf(nil, "%x", digest), bcrypt.DefaultCost) + if err != nil { + return "", err + } + return string(hash), nil +} + +// CheckUploadPassword verifies upload credentials against a stored bcrypt(sha256) hash. +func CheckUploadPassword(b64Creds string, storedHash string) bool { + digest := sha256.Sum256([]byte(b64Creds)) + err := bcrypt.CompareHashAndPassword([]byte(storedHash), fmt.Appendf(nil, "%x", digest)) + return err == nil +} diff --git a/server/common/password_test.go b/server/common/password_test.go new file mode 100644 index 00000000..0fdf6ffd --- /dev/null +++ b/server/common/password_test.go @@ -0,0 +1,41 @@ +package common + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestHashUploadPassword(t *testing.T) { + hash, err := HashUploadPassword("login", "password") + require.NoError(t, err) + require.True(t, len(hash) > 0) + require.Equal(t, "$2", hash[:2], "hash should be bcrypt format") +} + +func TestCheckUploadPassword(t *testing.T) { + hash, err := HashUploadPassword("login", "password") + require.NoError(t, err) + + b64 := EncodeAuthBasicHeader("login", "password") + + // Correct credentials + require.True(t, CheckUploadPassword(b64, hash)) + + // Wrong credentials + wrongB64 := EncodeAuthBasicHeader("login", "wrong") + require.False(t, CheckUploadPassword(wrongB64, hash)) +} + +func TestHashUploadPasswordLongCredentials(t *testing.T) { + // 128-char login and password should work (bcrypt's 72-byte limit is removed by SHA-256 pre-hash) + longLogin := strings.Repeat("a", 128) + longPass := strings.Repeat("b", 128) + + hash, err := HashUploadPassword(longLogin, longPass) + require.NoError(t, err) + + b64 := EncodeAuthBasicHeader(longLogin, longPass) + require.True(t, CheckUploadPassword(b64, hash)) +} diff --git a/server/common/utils.go b/server/common/utils.go index f1c301ca..5af11e43 100644 --- a/server/common/utils.go +++ b/server/common/utils.go @@ -69,15 +69,31 @@ func StripPrefix(prefix string, handler http.Handler) http.Handler { } // SanitizeFilenameForDisposition strips characters that could break a -// Content-Disposition header value: double quotes, CR, LF, and null bytes. +// Content-Disposition header value: double quotes, CR, LF, null bytes, +// and Unicode BiDi override characters that can spoof file extensions. +// It also truncates excessively long names to 1024 characters. func SanitizeFilenameForDisposition(name string) string { r := strings.NewReplacer( `"`, "", "\r", "", "\n", "", "\x00", "", + // Unicode BiDi overrides — can make "evil\u202Efdp.exe" appear as "evil exe.pdf" + "\u202A", "", // LRE (Left-to-Right Embedding) + "\u202B", "", // RLE (Right-to-Left Embedding) + "\u202C", "", // PDF (Pop Directional Formatting) + "\u202D", "", // LRO (Left-to-Right Override) + "\u202E", "", // RLO (Right-to-Left Override) + "\u2066", "", // LRI (Left-to-Right Isolate) + "\u2067", "", // RLI (Right-to-Left Isolate) + "\u2068", "", // FSI (First Strong Isolate) + "\u2069", "", // PDI (Pop Directional Isolate) ) - return r.Replace(name) + name = r.Replace(name) + if len(name) > 1024 { + name = name[:1024] + } + return name } // EncodeAuthBasicHeader return the base64 version of "login:password" diff --git a/server/common/utils_test.go b/server/common/utils_test.go index 3644c85f..bc086627 100644 --- a/server/common/utils_test.go +++ b/server/common/utils_test.go @@ -8,6 +8,7 @@ import ( "io" "net/http" "net/http/httptest" + "strings" "testing" "time" @@ -138,4 +139,18 @@ func TestSanitizeFilenameForDisposition(t *testing.T) { // Multiple dangerous characters at once require.Equal(t, "malicious.txt", SanitizeFilenameForDisposition("mal\"\rici\nous\x00.txt")) + + // Filename truncated at 1024 characters + longName := strings.Repeat("a", 1025) + require.Len(t, SanitizeFilenameForDisposition(longName), 1024) + + // Exactly 1024 passes through + exactName := strings.Repeat("b", 1024) + require.Len(t, SanitizeFilenameForDisposition(exactName), 1024) + + // Unicode BiDi override characters are stripped (RLO can spoof file extensions) + require.Equal(t, "evilfdp.exe", SanitizeFilenameForDisposition("evil\u202Efdp.exe")) + + // Multiple BiDi overrides are stripped + require.Equal(t, "safe.txt", SanitizeFilenameForDisposition("\u202A\u202Bsafe\u2066.\u2069txt\u202C")) } diff --git a/server/context/context.go b/server/context/context.go index 282969c3..ad7b478d 100644 --- a/server/context/context.go +++ b/server/context/context.go @@ -155,6 +155,14 @@ func (ctx *Context) SetAuthenticator(authenticator *common.SessionAuthenticator) ctx.authenticator = authenticator } +// GetAuthenticatorSafe get authenticator from the context (nil-safe, no panic). +func (ctx *Context) GetAuthenticatorSafe() *common.SessionAuthenticator { + ctx.mu.RLock() + defer ctx.mu.RUnlock() + + return ctx.authenticator +} + // GetMetrics get metrics from the context. func (ctx *Context) GetMetrics() *common.PlikMetrics { ctx.mu.RLock() diff --git a/server/context/upload.go b/server/context/upload.go index ff3662a3..9eaa6534 100644 --- a/server/context/upload.go +++ b/server/context/upload.go @@ -7,8 +7,8 @@ import ( "time" "github.com/dustin/go-humanize" + "github.com/root-gg/plik/server/common" - "github.com/root-gg/utils" ) // CreateUpload from params and context (check configuration and default values, generate upload and file IDs, ... ) @@ -38,6 +38,9 @@ func (ctx *Context) CreateUpload(params *common.Upload) (upload *common.Upload, } // Handle Basic Auth parameters + if params.ProtectedByPassword && params.Password == "" { + return nil, fmt.Errorf("upload password is empty") + } err = ctx.setBasicAuth(upload, params.Login, params.Password) if err != nil { return nil, err @@ -178,6 +181,10 @@ func (ctx *Context) setParams(upload *common.Upload, params *common.Upload) (err upload.Comments = params.Comments } + if len(upload.Comments) > 32768 { + return fmt.Errorf("comment is too long (max 32768 bytes)") + } + upload.E2EE = params.E2EE if upload.E2EE != "" && !common.IsValidE2EEScheme(upload.E2EE) { return fmt.Errorf("invalid e2ee scheme %q", upload.E2EE) @@ -250,6 +257,13 @@ func (ctx *Context) setBasicAuth(upload *common.Upload, login string, password s return nil } + if len(login) > 128 { + return fmt.Errorf("login too long (max 128 characters)") + } + if len(password) > 128 { + return fmt.Errorf("password too long (max 128 characters)") + } + if login != "" { upload.Login = login } else { @@ -258,8 +272,9 @@ func (ctx *Context) setBasicAuth(upload *common.Upload, login string, password s upload.ProtectedByPassword = true - // Save only the md5sum of this string to authenticate further requests - upload.Password, err = utils.Md5sum(common.EncodeAuthBasicHeader(upload.Login, password)) + // Hash credentials with bcrypt(sha256(base64(login:password))) + // SHA-256 pre-hash removes bcrypt's 72-byte input limit + upload.Password, err = common.HashUploadPassword(upload.Login, password) if err != nil { return fmt.Errorf("unable to generate password hash : %s", err) } diff --git a/server/context/upload_test.go b/server/context/upload_test.go index 1b7a45d6..5bb4c1fe 100644 --- a/server/context/upload_test.go +++ b/server/context/upload_test.go @@ -2,11 +2,10 @@ package context import ( "net" + "strings" "testing" "time" - "github.com/root-gg/utils" - "github.com/stretchr/testify/require" "github.com/root-gg/plik/server/common" @@ -221,11 +220,8 @@ func TestUpload_PasswordEnabled(t *testing.T) { require.NotNil(t, upload) require.True(t, upload.ProtectedByPassword) - md5sum, err := utils.Md5sum(common.EncodeAuthBasicHeader("login", "password")) - require.NoError(t, err) - + require.True(t, strings.HasPrefix(upload.Password, "$2"), "password should be bcrypt hash") require.Equal(t, "login", upload.Login) - require.Equal(t, md5sum, upload.Password) } func TestUpload_PasswordForced(t *testing.T) { @@ -251,11 +247,48 @@ func TestUpload_PasswordDefaultLogin(t *testing.T) { require.NoError(t, err) require.NotNil(t, upload) require.Equal(t, "plik", upload.Login) - md5sum, err := utils.Md5sum(common.EncodeAuthBasicHeader("plik", "bar")) + require.True(t, strings.HasPrefix(upload.Password, "$2"), "password should be bcrypt hash") + require.True(t, upload.ProtectedByPassword) +} + +func TestUpload_PasswordEmptyWithProtectedFlag(t *testing.T) { + ctx := newTestContext() + ctx.config.FeaturePassword = common.FeatureEnabled + + // Client sends protectedByPassword=true but no password => error + upload, err := ctx.CreateUpload(&common.Upload{ProtectedByPassword: true, Password: ""}) + common.RequireError(t, err, "upload password is empty") + require.Nil(t, upload) + + // Client sends protectedByPassword=true with valid password => ok + upload, err = ctx.CreateUpload(&common.Upload{ProtectedByPassword: true, Password: "password"}) require.NoError(t, err) + require.NotNil(t, upload) + require.True(t, upload.ProtectedByPassword) +} + +func TestUpload_PasswordTooLong(t *testing.T) { + ctx := newTestContext() + ctx.config.FeaturePassword = common.FeatureEnabled - require.Equal(t, md5sum, upload.Password) + // 129-char password exceeds the 128-byte limit + longPassword := strings.Repeat("a", 129) + upload, err := ctx.CreateUpload(&common.Upload{Password: longPassword}) + common.RequireError(t, err, "password too long") + require.Nil(t, upload) + + // 128-char password is accepted + maxPassword := strings.Repeat("a", 128) + upload, err = ctx.CreateUpload(&common.Upload{Password: maxPassword}) + require.NoError(t, err) + require.NotNil(t, upload) require.True(t, upload.ProtectedByPassword) + + // 129-char login exceeds the 128-byte limit + longLogin := strings.Repeat("b", 129) + upload, err = ctx.CreateUpload(&common.Upload{Login: longLogin, Password: "pass"}) + common.RequireError(t, err, "login too long") + require.Nil(t, upload) } func TestUpload_CommentsDisabled(t *testing.T) { @@ -310,6 +343,24 @@ func TestUpload_CommentsForced(t *testing.T) { require.Equal(t, "comments", upload.Comments) } +func TestUpload_CommentsMaxLength(t *testing.T) { + ctx := newTestContext() + ctx.config.FeatureComments = common.FeatureEnabled + + // 32768 bytes should be accepted + longComment := strings.Repeat("a", 32768) + upload, err := ctx.CreateUpload(&common.Upload{Comments: longComment}) + require.NoError(t, err) + require.NotNil(t, upload) + require.Len(t, upload.Comments, 32768) + + // 32769 bytes should be rejected + tooLongComment := strings.Repeat("a", 32769) + upload, err = ctx.CreateUpload(&common.Upload{Comments: tooLongComment}) + common.RequireError(t, err, "comment is too long") + require.Nil(t, upload) +} + func TestUpload_ExtendTTLDisabled(t *testing.T) { ctx := newTestContext() ctx.config.FeatureExtendTTL = common.FeatureDisabled @@ -364,7 +415,7 @@ func TestCreateUpload(t *testing.T) { params.ID = "id" params.UploadToken = "token" params.IsAdmin = true - params.ProtectedByPassword = true + params.RemoteIP = "1.3.3.7" params.TTL = 42 params.ExtendTTL = true diff --git a/server/handlers/get_file.go b/server/handlers/get_file.go index e8df0e7f..337ad404 100644 --- a/server/handlers/get_file.go +++ b/server/handlers/get_file.go @@ -67,14 +67,15 @@ func GetFile(ctx *context.Context, resp http.ResponseWriter, req *http.Request) } } - // Avoid rendering potentially dangerous content types in the browser - // HTML could execute inline scripts, SVG/XML can contain onload handlers (SECU-10) - if strings.Contains(file.Type, "html") || strings.Contains(file.Type, "svg") || file.Type == "text/xml" || file.Type == "text/xml; charset=utf-8" { - file.Type = "application/octet-stream" - } - - // Force the download of the following types as they are blocked by the CSP Header and won't display properly. - if file.Type == "" || strings.Contains(file.Type, "flash") || strings.Contains(file.Type, "pdf") { + // Neutralize content types that could execute code in the browser + // Force download as binary to prevent XSS via inline scripts, SVG onload handlers, etc. + if file.Type == "" || + strings.Contains(file.Type, "html") || + strings.Contains(file.Type, "svg") || + strings.Contains(file.Type, "xml") || + strings.Contains(file.Type, "javascript") || + strings.Contains(file.Type, "flash") || + strings.Contains(file.Type, "pdf") { file.Type = "application/octet-stream" } diff --git a/server/handlers/get_file_test.go b/server/handlers/get_file_test.go index 740abe02..b4e65b70 100644 --- a/server/handlers/get_file_test.go +++ b/server/handlers/get_file_test.go @@ -257,7 +257,7 @@ func TestGetHtmlFile(t *testing.T) { GetFile(ctx, rr, req) context.TestOK(t, rr) - require.Equal(t, "application/octet-stream", rr.Header().Get("Content-Type"), "invalid content type") + require.Equal(t, "application/octet-stream", rr.Header().Get("Content-Type"), "HTML files should be neutralized to octet-stream") } func TestGetSvgFile(t *testing.T) { @@ -312,6 +312,32 @@ func TestGetXmlFile(t *testing.T) { require.Equal(t, "application/octet-stream", rr.Header().Get("Content-Type"), "XML files should be neutralized to octet-stream") } +func TestGetJavascriptFile(t *testing.T) { + config := common.NewConfiguration() + ctx := newTestingContext(config) + + upload := &common.Upload{} + upload.InitializeForTests() + + file := upload.NewFile() + file.Type = "application/javascript" + file.Status = "uploaded" + err := createTestFile(ctx, file, bytes.NewBuffer([]byte("data"))) + require.NoError(t, err, "unable to create test file") + + ctx.SetUpload(upload) + ctx.SetFile(file) + + req, err := http.NewRequest("GET", "/file/", bytes.NewBuffer([]byte{})) + require.NoError(t, err, "unable to create new request") + + rr := ctx.NewRecorder(req) + GetFile(ctx, rr, req) + context.TestOK(t, rr) + + require.Equal(t, "application/octet-stream", rr.Header().Get("Content-Type"), "JS files should be neutralized to octet-stream") +} + func TestGetFileNoType(t *testing.T) { config := common.NewConfiguration() ctx := newTestingContext(config) diff --git a/server/handlers/misc.go b/server/handlers/misc.go index d43bcd0f..30486fe8 100644 --- a/server/handlers/misc.go +++ b/server/handlers/misc.go @@ -30,9 +30,13 @@ func GetConfiguration(ctx *context.Context, resp http.ResponseWriter, req *http. common.WriteJSONResponse(resp, ctx.GetConfig()) } -// Logout return the server configuration +// Logout delete session cookies func Logout(ctx *context.Context, resp http.ResponseWriter, req *http.Request) { - common.Logout(resp, ctx.GetAuthenticator()) + authenticator := ctx.GetAuthenticatorSafe() + if authenticator == nil { + return + } + common.Logout(resp, authenticator) } // GetQrCode return a QRCode for the requested URL diff --git a/server/handlers/misc_test.go b/server/handlers/misc_test.go index 0489a66d..9e6981ad 100644 --- a/server/handlers/misc_test.go +++ b/server/handlers/misc_test.go @@ -182,6 +182,19 @@ func TestLogout(t *testing.T) { context.TestOK(t, rr) } +func TestLogoutNoAuthenticator(t *testing.T) { + ctx := newTestingContext(common.NewConfiguration()) + // Clear the authenticator to simulate auth being disabled + ctx.SetAuthenticator(nil) + + req, err := http.NewRequest("GET", "/logout", bytes.NewBuffer([]byte{})) + require.NoError(t, err, "unable to create new request") + + rr := ctx.NewRecorder(req) + Logout(ctx, rr, req) + context.TestOK(t, rr) +} + func TestGetRedirectionURL(t *testing.T) { ctx := newTestingContext(common.NewConfiguration()) diff --git a/server/handlers/user.go b/server/handlers/user.go index 4b535722..54c1d9f3 100644 --- a/server/handlers/user.go +++ b/server/handlers/user.go @@ -108,7 +108,7 @@ func UpdateUser(ctx *context.Context, resp http.ResponseWriter, req *http.Reques ctx.Forbidden("can't grant yourself admin right, nice try!") return } - if userParams.MaxTTL != user.MaxTTL || userParams.MaxFileSize != user.MaxFileSize { + if userParams.MaxTTL != user.MaxTTL || userParams.MaxFileSize != user.MaxFileSize || userParams.MaxUserSize != user.MaxUserSize { ctx.Forbidden("can't edit your own quota, nice try!") return } diff --git a/server/handlers/user_test.go b/server/handlers/user_test.go index 170d76f6..cab2c71c 100644 --- a/server/handlers/user_test.go +++ b/server/handlers/user_test.go @@ -368,6 +368,65 @@ func TestUpdateUser_FailGrant(t *testing.T) { require.False(t, updatedUser.IsAdmin) } +func TestUpdateUser_FailQuotaMaxUserSize(t *testing.T) { + ctx := newTestingContext(common.NewConfiguration()) + + originalUser := &common.User{ + ID: "local:user", + Provider: "local", + Login: "user", + Password: "password", + Email: "user@root.gg", + Name: "user", + MaxFileSize: 1234, + MaxUserSize: 5678, + MaxTTL: 1234, + IsAdmin: false, + } + ctx.SetUser(originalUser) + + err := ctx.GetMetadataBackend().CreateUser(originalUser) + require.NoError(t, err) + + var updateParams = &struct { + ID string `json:"id,omitempty"` + Provider string `json:"provider"` + Login string `json:"login,omitempty"` + Password string `json:"password"` + Name string `json:"name,omitempty"` + Email string `json:"email,omitempty"` + IsAdmin bool `json:"admin"` + MaxFileSize int64 `json:"maxFileSize"` + MaxUserSize int64 `json:"maxUserSize"` + MaxTTL int `json:"maxTTL"` + }{ + ID: "local:user", + Provider: "local", + Login: "user", + Password: "password", + Email: "user@root.gg", + Name: "user", + MaxFileSize: 1234, + MaxUserSize: -1, // Trying to set unlimited + MaxTTL: 1234, + } + + userJSON, err := utils.ToJsonString(updateParams) + require.NoError(t, err) + + req, err := http.NewRequest("GET", "/", bytes.NewBufferString(userJSON)) + require.NoError(t, err, "unable to create new request") + + rr := ctx.NewRecorder(req) + UpdateUser(ctx, rr, req) + context.TestForbidden(t, rr, "can't edit your own quota") + + // Verify the user was not modified + updatedUser, err := ctx.GetMetadataBackend().GetUser(originalUser.ID) + require.NoError(t, err) + require.Equal(t, int64(5678), updatedUser.MaxUserSize, "MaxUserSize should not have changed") +} + func TestUpdateUser_OKGrant(t *testing.T) { ctx := newTestingContext(common.NewConfiguration()) diff --git a/server/middleware/file.go b/server/middleware/file.go index d22c4ca2..596cdfdd 100644 --- a/server/middleware/file.go +++ b/server/middleware/file.go @@ -45,7 +45,7 @@ func File(ctx *context.Context, next http.Handler) http.Handler { } if file.UploadID != upload.ID { - ctx.InternalServerError("invalid file upload id", nil) + ctx.NotFound("file %s not found", fileID) return } diff --git a/server/middleware/file_test.go b/server/middleware/file_test.go index f68d4c83..9a0a8ae9 100644 --- a/server/middleware/file_test.go +++ b/server/middleware/file_test.go @@ -104,6 +104,42 @@ func TestFileInvalidFileName(t *testing.T) { context.TestInvalidParameter(t, rr, "file name") } +func TestFileCrossUpload(t *testing.T) { + ctx := newTestingContext(common.NewConfiguration()) + + // Create upload A with a file + uploadA := &common.Upload{} + fileA := uploadA.NewFile() + fileA.Name = "filename" + uploadA.InitializeForTests() + err := ctx.GetMetadataBackend().CreateUpload(uploadA) + require.NoError(t, err, "create upload A error") + + // Create upload B (no file) + uploadB := &common.Upload{} + uploadB.InitializeForTests() + err = ctx.GetMetadataBackend().CreateUpload(uploadB) + require.NoError(t, err, "create upload B error") + + // Set context to upload B, but request file from upload A + ctx.SetUpload(uploadB) + + req, err := http.NewRequest("GET", "", &bytes.Buffer{}) + require.NoError(t, err, "unable to create new request") + + vars := map[string]string{ + "fileID": fileA.ID, + "filename": fileA.Name, + } + req = mux.SetURLVars(req, vars) + + rr := ctx.NewRecorder(req) + File(ctx, common.DummyHandler).ServeHTTP(rr, req) + + // Should return 404 Not Found (not 500) to prevent file ID enumeration + context.TestNotFound(t, rr, "file "+fileA.ID+" not found") +} + func TestFile(t *testing.T) { ctx := newTestingContext(common.NewConfiguration()) diff --git a/server/middleware/hsts.go b/server/middleware/hsts.go new file mode 100644 index 00000000..26a26790 --- /dev/null +++ b/server/middleware/hsts.go @@ -0,0 +1,12 @@ +package middleware + +import "net/http" + +// HSTS adds the Strict-Transport-Security header to all responses. +// max-age=31536000 (1 year) tells browsers to always use HTTPS for this domain. +func HSTS(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Strict-Transport-Security", "max-age=31536000") + next.ServeHTTP(w, r) + }) +} diff --git a/server/middleware/hsts_test.go b/server/middleware/hsts_test.go new file mode 100644 index 00000000..10ed6606 --- /dev/null +++ b/server/middleware/hsts_test.go @@ -0,0 +1,42 @@ +package middleware + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestHSTS(t *testing.T) { + dummy := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + + handler := HSTS(dummy) + + req, err := http.NewRequest("GET", "/", nil) + require.NoError(t, err) + + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + + require.Equal(t, http.StatusOK, rr.Code) + require.Equal(t, "max-age=31536000", rr.Header().Get("Strict-Transport-Security")) +} + +func TestHSTS_HeaderAbsentWithout(t *testing.T) { + dummy := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + + // Without HSTS middleware, no header + req, err := http.NewRequest("GET", "/", nil) + require.NoError(t, err) + + rr := httptest.NewRecorder() + dummy.ServeHTTP(rr, req) + + require.Equal(t, http.StatusOK, rr.Code) + require.Empty(t, rr.Header().Get("Strict-Transport-Security")) +} diff --git a/server/middleware/upload.go b/server/middleware/upload.go index ca6fb77d..6de30db2 100644 --- a/server/middleware/upload.go +++ b/server/middleware/upload.go @@ -1,6 +1,7 @@ package middleware import ( + "crypto/subtle" "fmt" "net/http" "strings" @@ -9,6 +10,7 @@ import ( "github.com/root-gg/utils" + "github.com/root-gg/plik/server/common" "github.com/root-gg/plik/server/context" ) @@ -60,13 +62,13 @@ func Upload(ctx *context.Context, next http.Handler) http.Handler { upload.IsAdmin = false uploadToken := req.Header.Get("X-UploadToken") - if uploadToken != "" && uploadToken == upload.UploadToken { + if uploadToken != "" && subtle.ConstantTimeCompare([]byte(uploadToken), []byte(upload.UploadToken)) == 1 { upload.IsAdmin = true } else { token := ctx.GetToken() if token != nil { // A user authenticated with a token can manage uploads created with such token - if upload.Token == token.Token { + if subtle.ConstantTimeCompare([]byte(upload.Token), []byte(token.Token)) == 1 { upload.IsAdmin = true } } else { @@ -111,15 +113,25 @@ func Upload(ctx *context.Context, next http.Handler) http.Handler { forbidden("invalid http authorization scheme") return } - var md5sum string - md5sum, err = utils.Md5sum(auth[1]) - if err != nil { - forbidden("unable to hash credentials") - return - } - if md5sum != upload.Password { - forbidden("invalid credentials") - return + b64Creds := auth[1] + if strings.HasPrefix(upload.Password, "$2") { + // bcrypt(sha256) hash (new uploads) + if !common.CheckUploadPassword(b64Creds, upload.Password) { + forbidden("invalid credentials") + return + } + } else { + // Legacy MD5 hash (old uploads, will expire naturally) + var md5sum string + md5sum, err = utils.Md5sum(b64Creds) + if err != nil { + forbidden("unable to hash credentials") + return + } + if md5sum != upload.Password { + forbidden("invalid credentials") + return + } } } diff --git a/server/middleware/upload_test.go b/server/middleware/upload_test.go index a7bdaee1..7a3100d5 100644 --- a/server/middleware/upload_test.go +++ b/server/middleware/upload_test.go @@ -403,19 +403,54 @@ func TestUploadPassword(t *testing.T) { ctx := newTestingContext(common.NewConfiguration()) ctx.GetConfig().FeatureAuthentication = common.FeatureEnabled + upload := &common.Upload{} + upload.ProtectedByPassword = true + upload.Login = "login" + upload.InitializeForTests() + + // Hash credentials with bcrypt(sha256) (new upload path) + b64str := base64.StdEncoding.EncodeToString([]byte("login:password")) var err error + upload.Password, err = common.HashUploadPassword("login", "password") + require.NoError(t, err, "unable to hash upload credentials") + + err = ctx.GetMetadataBackend().CreateUpload(upload) + require.NoError(t, err, "Unable to create upload") + + req, err := http.NewRequest("GET", "", &bytes.Buffer{}) + require.NoError(t, err, "unable to create new request") + + // Fake gorilla/mux vars + vars := map[string]string{ + "uploadID": upload.ID, + } + req = mux.SetURLVars(req, vars) + + req.Header.Add("Authorization", "Basic "+b64str) + + rr := ctx.NewRecorder(req) + Upload(ctx, common.DummyHandler).ServeHTTP(rr, req) + + require.Equal(t, http.StatusOK, rr.Code, "invalid handler response status code") + require.NotNil(t, ctx.GetUpload(), "missing upload from context") + require.Equal(t, upload.ID, ctx.GetUpload().ID, "invalid upload from context") + require.False(t, upload.IsAdmin, "invalid upload admin status") +} + +func TestUploadPasswordLegacyMD5(t *testing.T) { + ctx := newTestingContext(common.NewConfiguration()) + ctx.GetConfig().FeatureAuthentication = common.FeatureEnabled upload := &common.Upload{} upload.ProtectedByPassword = true upload.Login = "login" - upload.Password = "password" upload.InitializeForTests() - // The Authorization header will contain the base64 version of "login:password" - // Save only the md5sum of this string to authenticate further requests - b64str := base64.StdEncoding.EncodeToString([]byte(upload.Login + ":" + upload.Password)) + // Simulate legacy MD5 hash (old uploads) + b64str := base64.StdEncoding.EncodeToString([]byte("login:password")) + var err error upload.Password, err = utils.Md5sum(b64str) - require.NoError(t, err, "unable to b64encode upload credentials") + require.NoError(t, err, "unable to md5sum upload credentials") err = ctx.GetMetadataBackend().CreateUpload(upload) require.NoError(t, err, "Unable to create upload") @@ -423,7 +458,6 @@ func TestUploadPassword(t *testing.T) { req, err := http.NewRequest("GET", "", &bytes.Buffer{}) require.NoError(t, err, "unable to create new request") - // Fake gorilla/mux vars vars := map[string]string{ "uploadID": upload.ID, } @@ -437,5 +471,4 @@ func TestUploadPassword(t *testing.T) { require.Equal(t, http.StatusOK, rr.Code, "invalid handler response status code") require.NotNil(t, ctx.GetUpload(), "missing upload from context") require.Equal(t, upload.ID, ctx.GetUpload().ID, "invalid upload from context") - require.False(t, upload.IsAdmin, "invalid upload admin status") } diff --git a/server/server/server.go b/server/server/server.go index 05acb496..6d5b26a9 100644 --- a/server/server/server.go +++ b/server/server/server.go @@ -426,12 +426,18 @@ func (ps *PlikServer) getHTTPHandler() (handler http.Handler) { ps.config.NewLogger().Warningf("Webapp directory %s not found, consider setting config.NoWebInterface to true", ps.config.WebappDirectory) } - router.PathPrefix("/clients/").Handler(http.StripPrefix("/clients/", http.FileServer(http.Dir(ps.config.ClientsDirectory)))) - router.PathPrefix("/changelog/").Handler(http.StripPrefix("/changelog/", http.FileServer(http.Dir(ps.config.ChangelogDirectory)))) - router.PathPrefix("/").Handler(http.FileServer(http.Dir(ps.config.WebappDirectory))) + router.PathPrefix("/clients/").Handler(http.StripPrefix("/clients/", common.NoDirListing(http.FileServer(http.Dir(ps.config.ClientsDirectory))))) + router.PathPrefix("/changelog/").Handler(http.StripPrefix("/changelog/", common.NoDirListing(http.FileServer(http.Dir(ps.config.ChangelogDirectory))))) + router.PathPrefix("/").Handler(common.NoDirListing(http.FileServer(http.Dir(ps.config.WebappDirectory)))) } handler = common.StripPrefix(ps.config.Path, router) + + // Add HSTS header when TLS is configured + if ps.config.SslEnabled || ps.config.EnhancedWebSecurity { + handler = middleware.HSTS(handler) + } + return handler } @@ -560,7 +566,7 @@ func (ps *PlikServer) initializeAuthenticator() (err error) { ps.authenticator = &common.SessionAuthenticator{ SignatureKey: setting.Value, - SecureCookies: ps.config.EnhancedWebSecurity, + SecureCookies: ps.config.EnhancedWebSecurity || ps.config.SslEnabled, SessionTimeout: ps.config.GetSessionTimeout(), Path: ps.config.GetPath(), } diff --git a/webapp/src/components/FileRow.vue b/webapp/src/components/FileRow.vue index 062349b6..55a265c6 100644 --- a/webapp/src/components/FileRow.vue +++ b/webapp/src/components/FileRow.vue @@ -129,7 +129,8 @@ function fileUrl() { + target="_blank" + rel="noopener noreferrer"> {{ file.fileName }}