diff --git a/conf/defaults.ini b/conf/defaults.ini index 77bc39c9feb..59e91f483e5 100644 --- a/conf/defaults.ini +++ b/conf/defaults.ini @@ -1747,9 +1747,9 @@ enabled = true #################################### Short Links ############################# [short_links] -# Short links that are never accessed will be deleted as cleanup. Time is set up in days. The default is 7 days. Maximum value is 365. +# Short links that are never accessed will be deleted as cleanup. Time is set up in days. The default is -1 (never expire). Maximum value is 365. # 0 means they will be deleted approximately every 10 minutes. A negative value (such as -1) will disable expiration. -expire_time = 7 +expire_time = -1 #################################### Internal Grafana Metrics ############ # Metrics available at HTTP URL /metrics and /metrics/plugins/:pluginId diff --git a/conf/sample.ini b/conf/sample.ini index eea46ccaacb..8f687aae876 100644 --- a/conf/sample.ini +++ b/conf/sample.ini @@ -1689,8 +1689,8 @@ default_datasource_uid = #################################### Short Links ############################# [short_links] -# Short links which are never accessed will be deleted as cleanup. Time is in days. Default is 7 days. Max is 365. 0 means they will be deleted approximately every 10 minutes. -;expire_time = 7 +# Short links which are never accessed will be deleted as cleanup. Time is in days. Default is -1 (never expire). Max is 365. 0 means they will be deleted approximately every 10 minutes. A negative value (such as -1) will disable expiration. +;expire_time = -1 #################################### Internal Grafana Metrics ########################## # Metrics available at HTTP URL /metrics and /metrics/plugins/:pluginId diff --git a/e2e-playwright/dashboards-suite/dashboard-share-internally.spec.ts b/e2e-playwright/dashboards-suite/dashboard-share-internally.spec.ts index 98d394ccc91..d11a66afa90 100644 --- a/e2e-playwright/dashboards-suite/dashboard-share-internally.spec.ts +++ b/e2e-playwright/dashboards-suite/dashboard-share-internally.spec.ts @@ -118,6 +118,151 @@ test.describe( expect(responseBody.url).toContain('goto'); }); + test('Short URL de-duplication with locked time range', async ({ page, gotoDashboardPage, selectors }) => { + // Navigate to dashboard with specific time range + const dashboardPage = await gotoDashboardPage({ + uid: DASHBOARD_UID, + queryParams: new URLSearchParams({ from: 'now-6h', to: 'now' }), + }); + + // Open share internally drawer + await dashboardPage.getByGrafanaSelector(selectors.pages.Dashboard.DashNav.newShareButton.arrowMenu).click(); + + // Set up response listener BEFORE opening drawer (API call happens when drawer opens with shorten URL enabled) + const createResponse1 = page.waitForResponse( + (response) => response.url().includes('/api/short-urls') && response.request().method() === 'POST' + ); + + await dashboardPage + .getByGrafanaSelector(selectors.pages.Dashboard.DashNav.newShareButton.menu.shareInternally) + .click(); + + await expect(page).toHaveURL(/.*shareView=link.*/); + + // Wait for the first API response + const response1 = await createResponse1; + expect(response1.status()).toBe(200); + const responseBody1 = await response1.json(); + const shortUrl1 = responseBody1.url; + expect(shortUrl1).toContain('goto'); + + // Ensure lock time range is enabled (default) and shorten URL is enabled + const lockTimeRangeSwitch = dashboardPage.getByGrafanaSelector( + selectors.pages.ShareDashboardDrawer.ShareInternally.lockTimeRangeSwitch + ); + const shortenUrlSwitch = dashboardPage.getByGrafanaSelector( + selectors.pages.ShareDashboardDrawer.ShareInternally.shortenUrlSwitch + ); + + // Ensure both are checked + await expect(async () => { + const isLocked = await lockTimeRangeSwitch.isChecked(); + if (!isLocked) { + await lockTimeRangeSwitch.check({ force: true }); + } + const isShortened = await shortenUrlSwitch.isChecked(); + if (!isShortened) { + await shortenUrlSwitch.check({ force: true }); + } + }).toPass(); + + // Wait a moment, then trigger a rebuild to create second short URL + // Toggle a setting off and back on to force URL rebuild + await page.waitForTimeout(1000); + + // Set up response listener before toggling (this will trigger URL rebuild and API call) + const createResponse2 = page.waitForResponse( + (response) => response.url().includes('/api/short-urls') && response.request().method() === 'POST' + ); + + // Toggle lock time range off and back on to force URL rebuild + await lockTimeRangeSwitch.uncheck({ force: true }); + await page.waitForTimeout(500); + await lockTimeRangeSwitch.check({ force: true }); + + const response2 = await createResponse2; + expect(response2.status()).toBe(200); + const responseBody2 = await response2.json(); + const shortUrl2 = responseBody2.url; + expect(shortUrl2).toContain('goto'); + + // Both short URLs should be the same (de-duplication) + expect(shortUrl1).toBe(shortUrl2); + }); + + test('Short URL de-duplication with unlocked time range', async ({ page, gotoDashboardPage, selectors }) => { + // Navigate to dashboard with specific time range + const dashboardPage = await gotoDashboardPage({ + uid: DASHBOARD_UID, + queryParams: new URLSearchParams({ from: 'now-6h', to: 'now' }), + }); + + // Open share internally drawer + await dashboardPage.getByGrafanaSelector(selectors.pages.Dashboard.DashNav.newShareButton.arrowMenu).click(); + + // Disable lock time range first, then set up response listener + const lockTimeRangeSwitch = dashboardPage.getByGrafanaSelector( + selectors.pages.ShareDashboardDrawer.ShareInternally.lockTimeRangeSwitch + ); + + // Set up response listener BEFORE opening drawer (API call happens when drawer opens with shorten URL enabled) + const createResponse1 = page.waitForResponse( + (response) => response.url().includes('/api/short-urls') && response.request().method() === 'POST' + ); + + await dashboardPage + .getByGrafanaSelector(selectors.pages.Dashboard.DashNav.newShareButton.menu.shareInternally) + .click(); + + await expect(page).toHaveURL(/.*shareView=link.*/); + + // Wait for the first API response + const response1 = await createResponse1; + expect(response1.status()).toBe(200); + const responseBody1 = await response1.json(); + const shortUrl1 = responseBody1.url; + expect(shortUrl1).toContain('goto'); + + // Disable lock time range + await expect(lockTimeRangeSwitch).toBeInViewport(); + await expect(async () => { + await lockTimeRangeSwitch.uncheck({ force: true }); + }).toPass(); + + // Ensure shorten URL is enabled + const shortenUrlSwitch = dashboardPage.getByGrafanaSelector( + selectors.pages.ShareDashboardDrawer.ShareInternally.shortenUrlSwitch + ); + await expect(async () => { + const isShortened = await shortenUrlSwitch.isChecked(); + if (!isShortened) { + await shortenUrlSwitch.check({ force: true }); + } + }).toPass(); + + // Wait a moment, then trigger a rebuild to create second short URL + await page.waitForTimeout(1000); + + // Set up response listener before toggling (this will trigger URL rebuild and API call) + const createResponse2 = page.waitForResponse( + (response) => response.url().includes('/api/short-urls') && response.request().method() === 'POST' + ); + + // Toggle lock time range on and back off to force URL rebuild + await lockTimeRangeSwitch.check({ force: true }); + await page.waitForTimeout(500); + await lockTimeRangeSwitch.uncheck({ force: true }); + + const response2 = await createResponse2; + expect(response2.status()).toBe(200); + const responseBody2 = await response2.json(); + const shortUrl2 = responseBody2.url; + expect(shortUrl2).toContain('goto'); + + // Both short URLs should be the same (de-duplication) + expect(shortUrl1).toBe(shortUrl2); + }); + test('Share button gets configured link', async ({ page, gotoDashboardPage, selectors }) => { // Navigate to dashboard with specific time range const dashboardPage = await gotoDashboardPage({ diff --git a/pkg/api/short_url.go b/pkg/api/short_url.go index 7b17fdb01a7..8d9396e3e3b 100644 --- a/pkg/api/short_url.go +++ b/pkg/api/short_url.go @@ -50,14 +50,14 @@ func (hs *HTTPServer) createShortURL(c *contextmodel.ReqContext) response.Respon if err := web.Bind(c.Req, &cmd); err != nil { return response.Err(shorturls.ErrShortURLBadRequest.Errorf("bad request data: %w", err)) } - hs.log.Debug("Received request to create short URL", "path", cmd.Path) + hs.log.Debug("Received request to create short URL", "path", cmd.Path, "uid", cmd.UID) shortURL, err := hs.ShortURLService.CreateShortURL(c.Req.Context(), c.SignedInUser, cmd) if err != nil { return response.Err(err) } shortURLDTO := hs.ShortURLService.ConvertShortURLToDTO(shortURL, hs.Cfg.AppURL) - c.Logger.Debug("Created short URL", "url", shortURLDTO.URL) + c.Logger.Debug("Created short URL", "url", shortURLDTO.URL, "uid", shortURL.Uid, "signature", shortURL.Signature) return response.JSON(http.StatusOK, shortURLDTO) } diff --git a/pkg/services/shorturls/models.go b/pkg/services/shorturls/models.go index 8b284fa13a7..83fc8d4d976 100644 --- a/pkg/services/shorturls/models.go +++ b/pkg/services/shorturls/models.go @@ -20,6 +20,7 @@ type ShortUrl struct { OrgId int64 `json:"-"` Uid string `json:"uid"` Path string `json:"path"` + Signature string `json:"-" xorm:"signature"` CreatedBy int64 `json:"-"` CreatedAt int64 `json:"-"` LastSeenAt int64 `json:"lastSeenAt"` diff --git a/pkg/services/shorturls/shorturlimpl/shorturl.go b/pkg/services/shorturls/shorturlimpl/shorturl.go index a363a4d7627..aa201e43254 100644 --- a/pkg/services/shorturls/shorturlimpl/shorturl.go +++ b/pkg/services/shorturls/shorturlimpl/shorturl.go @@ -2,8 +2,11 @@ package shorturlimpl import ( "context" + "crypto/sha256" "fmt" + "net/url" "path" + "sort" "strings" "time" @@ -16,6 +19,143 @@ import ( var getTime = time.Now +// normalizeTimeParam normalizes absolute timestamps to relative ranges when possible. +// If both from and to are recent absolute timestamps (within last 24h), it normalizes them +// to a relative representation based on their offset from now. +func normalizeTimeParam(paramName, paramValue string, now time.Time) string { + // Try to parse as ISO timestamp + t, err := time.Parse(time.RFC3339, paramValue) + if err != nil { + // Try parsing with nanoseconds + t, err = time.Parse(time.RFC3339Nano, paramValue) + if err != nil { + // Not an ISO timestamp, return as-is (might be relative like "now-6h") + return paramValue + } + } + + // Only normalize timestamps from the last 24 hours + // This prevents normalizing old absolute timestamps that should stay absolute + if t.Before(now.Add(-24 * time.Hour)) { + return paramValue + } + + // Calculate offset from now + offset := now.Sub(t) + + // For "to" parameter, it might be at or slightly after now + // For "from" parameter, it's typically before now + // Handle both cases + if offset >= 0 { + // t is in the past or at now + offsetMinutes := int(offset.Minutes()) + if offsetMinutes == 0 { + return "now" + } + // Convert to relative format + if offsetMinutes < 60 { + return fmt.Sprintf("now-%dm", offsetMinutes) + } else if offsetMinutes < 1440 { // 24 hours + hours := offsetMinutes / 60 + return fmt.Sprintf("now-%dh", hours) + } + // For longer offsets, keep as absolute timestamp but round to minute precision + return t.Truncate(time.Minute).Format(time.RFC3339) + } else { + // t is in the future (shouldn't happen for "from", but might for "to") + // If very close to now (within 1 minute), normalize to "now" + futureOffset := -offset + if futureOffset < time.Minute { + return "now" + } + // Otherwise keep as absolute timestamp + return t.Truncate(time.Minute).Format(time.RFC3339) + } +} + +// normalizePath normalizes a URL path by sorting query parameters alphabetically +// and normalizing recent absolute timestamps to relative ranges. +func normalizePath(pathStr string) string { + // Try to parse as URL to separate path and query + parsedURL, err := url.Parse(pathStr) + if err != nil { + // If parsing fails, return as-is (no query params to normalize) + return pathStr + } + + // If no query parameters, return path as-is + if parsedURL.RawQuery == "" { + return pathStr + } + + // Parse query parameters + values := parsedURL.Query() + + // Normalize time parameters if present + now := getTime() + + // If "to" is present and very close to now (within 5 minutes), use it as the reference point + // This handles the case where "to" represents "now" at the time the URL was created + var referenceTime time.Time = now + if toVal, hasTo := values["to"]; hasTo && len(toVal) > 0 { + if toTime, err := time.Parse(time.RFC3339, toVal[0]); err == nil { + // Try with nanoseconds too + if err != nil { + toTime, err = time.Parse(time.RFC3339Nano, toVal[0]) + } + if err == nil { + // If "to" is within 5 minutes of now (past or future), use it as reference + // This means the URL was created recently and "to" represents "now" + timeDiff := now.Sub(toTime) + if timeDiff < 5*time.Minute && timeDiff > -5*time.Minute { + referenceTime = toTime + } + } + } + } + + if fromVal, hasFrom := values["from"]; hasFrom && len(fromVal) > 0 { + normalized := normalizeTimeParam("from", fromVal[0], referenceTime) + values["from"] = []string{normalized} + } + if toVal, hasTo := values["to"]; hasTo && len(toVal) > 0 { + normalized := normalizeTimeParam("to", toVal[0], referenceTime) + values["to"] = []string{normalized} + } + + // Sort keys for consistent ordering + keys := make([]string, 0, len(values)) + for k := range values { + keys = append(keys, k) + } + sort.Strings(keys) + + // Rebuild query string with sorted keys + var queryParts []string + for _, k := range keys { + // Sort values for each key as well (in case of multiple values) + vals := values[k] + sort.Strings(vals) + for _, v := range vals { + queryParts = append(queryParts, fmt.Sprintf("%s=%s", url.QueryEscape(k), url.QueryEscape(v))) + } + } + + // Reconstruct normalized path + normalizedQuery := strings.Join(queryParts, "&") + parsedURL.RawQuery = normalizedQuery + return parsedURL.String() +} + +// generateSignature creates a SHA256 hash of orgID and normalized path +// to enable de-duplication of short URLs. +func generateSignature(orgID int64, pathStr string) string { + normalizedPath := normalizePath(pathStr) + input := fmt.Sprintf("%d:%s", orgID, normalizedPath) + hash := sha256.Sum256([]byte(input)) + return fmt.Sprintf("%x", hash) +} + type ShortURLService struct { SQLStore store } @@ -50,6 +190,34 @@ func (s ShortURLService) CreateShortURL(ctx context.Context, user identity.Reque return nil, shorturls.ErrShortURLInvalidPath.Errorf("path cannot contain '../': %s", relPath) } + orgID := user.GetOrgID() + + // Generate signature for de-duplication + // If UID is provided, include it in signature to make it unique per UID (bypasses de-duplication) + // If UID is not provided, signature is based on path only (enables de-duplication) + var signature string + if cmd.UID == "" { + signature = generateSignature(orgID, relPath) + + // Check if a short URL with the same signature already exists + existingShortURL, err := s.SQLStore.GetBySignature(ctx, orgID, signature) + if err != nil { + if !shorturls.ErrShortURLNotFound.Is(err) { + return nil, shorturls.ErrShortURLInternal.Errorf("failed to check existing short URL by signature: %w", err) + } + // Not found, continue to create new one + } else if existingShortURL != nil { + // Found existing short URL with same signature, update LastSeenAt and return it + if err := s.SQLStore.Update(ctx, existingShortURL); err != nil { + return nil, shorturls.ErrShortURLInternal.Errorf("failed to update existing short URL: %w", err) + } + return existingShortURL, nil + } + } else { + // Include UID in signature to make it unique per custom UID + signature = generateSignature(orgID, relPath+":"+cmd.UID) + } + uid := cmd.UID if uid == "" { uid = util.GenerateShortUID() @@ -74,14 +242,28 @@ func (s ShortURLService) CreateShortURL(ctx context.Context, user identity.Reque now := time.Now().Unix() shortURL := shorturls.ShortUrl{ - OrgId: user.GetOrgID(), + OrgId: orgID, Uid: uid, Path: relPath, + Signature: signature, CreatedAt: now, } shortURL.CreatedBy, _ = user.GetInternalID() if err := s.SQLStore.Insert(ctx, &shortURL); err != nil { + // Handle potential race condition: if unique constraint violation on (org_id, signature) + // another request may have created the same short URL concurrently + if cmd.UID == "" && signature != "" { + // Check again if it was created by another request + existingShortURL, retryErr := s.SQLStore.GetBySignature(ctx, orgID, signature) + if retryErr == nil && existingShortURL != nil { + // Found it, update LastSeenAt and return it + if updateErr := s.SQLStore.Update(ctx, existingShortURL); updateErr != nil { + return nil, shorturls.ErrShortURLInternal.Errorf("failed to update existing short URL after race condition: %w", updateErr) + } + return existingShortURL, nil + } + } return nil, shorturls.ErrShortURLInternal.Errorf("failed to insert shorturl: %w", err) } diff --git a/pkg/services/shorturls/shorturlimpl/shorturl_test.go b/pkg/services/shorturls/shorturlimpl/shorturl_test.go index 9be82b9e2ef..c89e70b74a1 100644 --- a/pkg/services/shorturls/shorturlimpl/shorturl_test.go +++ b/pkg/services/shorturls/shorturlimpl/shorturl_test.go @@ -2,6 +2,7 @@ package shorturlimpl import ( "context" + "fmt" "testing" "time" @@ -22,7 +23,7 @@ func TestMain(m *testing.M) { func TestIntegrationShortURLService(t *testing.T) { testutil.SkipIntegrationTestInShortMode(t) - user := &user.SignedInUser{UserID: 1} + testUser := &user.SignedInUser{UserID: 1} store := db.InitTestDB(t) t.Run("User can create and read short URLs", func(t *testing.T) { @@ -32,12 +33,12 @@ func TestIntegrationShortURLService(t *testing.T) { service := ShortURLService{SQLStore: &sqlStore{db: store}} - newShortURL, err := service.CreateShortURL(context.Background(), user, cmd) + newShortURL, err := service.CreateShortURL(context.Background(), testUser, cmd) require.NoError(t, err) require.NotNil(t, newShortURL) require.NotEmpty(t, newShortURL.Uid) - existingShortURL, err := service.GetShortURLByUID(context.Background(), user, newShortURL.Uid) + existingShortURL, err := service.GetShortURLByUID(context.Background(), testUser, newShortURL.Uid) require.NoError(t, err) require.NotNil(t, existingShortURL) require.Equal(t, cmd.Path, existingShortURL.Path) @@ -56,13 +57,18 @@ func TestIntegrationShortURLService(t *testing.T) { err := service.UpdateLastSeenAt(context.Background(), existingShortURL) require.NoError(t, err) - updatedShortURL, err := service.GetShortURLByUID(context.Background(), user, existingShortURL.Uid) + updatedShortURL, err := service.GetShortURLByUID(context.Background(), testUser, existingShortURL.Uid) require.NoError(t, err) require.Equal(t, expectedTime.Unix(), updatedShortURL.LastSeenAt) }) t.Run("and stale short urls can be deleted", func(t *testing.T) { - staleShortURL, err := service.CreateShortURL(context.Background(), user, cmd) + // Use a custom UID to bypass de-duplication and create a new stale URL + staleCmd := &dtos.CreateShortURLCmd{ + Path: cmd.Path, + UID: "stale-url-uid", + } + staleShortURL, err := service.CreateShortURL(context.Background(), testUser, staleCmd) require.NoError(t, err) require.NotNil(t, staleShortURL) require.NotEmpty(t, staleShortURL.Uid) @@ -74,7 +80,7 @@ func TestIntegrationShortURLService(t *testing.T) { require.Equal(t, int64(1), cmd.NumDeleted) t.Run("and previously accessed short urls will still exist", func(t *testing.T) { - updatedShortURL, err := service.GetShortURLByUID(context.Background(), user, existingShortURL.Uid) + updatedShortURL, err := service.GetShortURLByUID(context.Background(), testUser, existingShortURL.Uid) require.NoError(t, err) require.NotNil(t, updatedShortURL) }) @@ -92,7 +98,7 @@ func TestIntegrationShortURLService(t *testing.T) { t.Run("User cannot look up nonexistent short URLs", func(t *testing.T) { service := ShortURLService{SQLStore: &sqlStore{db: store}} - shortURL, err := service.GetShortURLByUID(context.Background(), user, "testnotfounduid") + shortURL, err := service.GetShortURLByUID(context.Background(), testUser, "testnotfounduid") require.Error(t, err) require.True(t, shorturls.ErrShortURLNotFound.Is(err)) require.Nil(t, shortURL) @@ -107,26 +113,26 @@ func TestIntegrationShortURLService(t *testing.T) { Path: "/path?test=true", } - newShortURL, err := service.CreateShortURL(ctx, user, cmd) + newShortURL, err := service.CreateShortURL(ctx, testUser, cmd) require.ErrorIs(t, err, shorturls.ErrShortURLAbsolutePath) require.Nil(t, newShortURL) cmd2 := &dtos.CreateShortURLCmd{ Path: "path/../test?test=true", } - newShortURL, err = service.CreateShortURL(ctx, user, cmd2) + newShortURL, err = service.CreateShortURL(ctx, testUser, cmd2) require.ErrorIs(t, err, shorturls.ErrShortURLInvalidPath) require.Nil(t, newShortURL) cmd3 := &dtos.CreateShortURLCmd{ Path: "../path/test?test=true", } - newShortURL, err = service.CreateShortURL(ctx, user, cmd3) + newShortURL, err = service.CreateShortURL(ctx, testUser, cmd3) require.ErrorIs(t, err, shorturls.ErrShortURLInvalidPath) require.Nil(t, newShortURL) }) - t.Run("The same URL will generate different entries", func(t *testing.T) { + t.Run("The same URL will return the same entry (de-duplication)", func(t *testing.T) { service := ShortURLService{SQLStore: &sqlStore{db: store}} ctx := context.Background() @@ -134,20 +140,202 @@ func TestIntegrationShortURLService(t *testing.T) { cmd := &dtos.CreateShortURLCmd{ Path: "mock/path?test=true", } - newShortURL1, err := service.CreateShortURL(ctx, user, cmd) + newShortURL1, err := service.CreateShortURL(ctx, testUser, cmd) + require.NoError(t, err) + require.NotNil(t, newShortURL1) + require.NotEmpty(t, newShortURL1.Uid) + require.NotEmpty(t, newShortURL1.Signature) + + newShortURL2, err := service.CreateShortURL(ctx, testUser, cmd) + require.NoError(t, err) + require.NotNil(t, newShortURL2) + + // Should return the same short URL (de-duplication) + require.Equal(t, newShortURL1.Uid, newShortURL2.Uid) + require.Equal(t, newShortURL1.Path, newShortURL2.Path) + require.Equal(t, newShortURL1.Signature, newShortURL2.Signature) + }) + + t.Run("Path normalization: different query param orders produce same signature", func(t *testing.T) { + service := ShortURLService{SQLStore: &sqlStore{db: store}} + + ctx := context.Background() + + cmd1 := &dtos.CreateShortURLCmd{ + Path: "mock/path?a=1&b=2", + } + newShortURL1, err := service.CreateShortURL(ctx, testUser, cmd1) + require.NoError(t, err) + require.NotNil(t, newShortURL1) + require.NotEmpty(t, newShortURL1.Signature) + + cmd2 := &dtos.CreateShortURLCmd{ + Path: "mock/path?b=2&a=1", + } + newShortURL2, err := service.CreateShortURL(ctx, testUser, cmd2) + require.NoError(t, err) + require.NotNil(t, newShortURL2) + + // Should return the same short URL due to path normalization + require.Equal(t, newShortURL1.Uid, newShortURL2.Uid) + require.Equal(t, newShortURL1.Signature, newShortURL2.Signature) + }) + + t.Run("Time normalization: absolute timestamps with same relative range produce same signature", func(t *testing.T) { + service := ShortURLService{SQLStore: &sqlStore{db: store}} + + ctx := context.Background() + + // Simulate two absolute timestamps that represent the same relative range (now-6h to now) + // These would be created a few seconds apart + now := time.Now() + from1 := now.Add(-6 * time.Hour).Add(-10 * time.Second) + to1 := now.Add(-10 * time.Second) + from2 := now.Add(-6 * time.Hour).Add(-5 * time.Second) + to2 := now.Add(-5 * time.Second) + + cmd1 := &dtos.CreateShortURLCmd{ + Path: fmt.Sprintf("d/test-dashboard?orgId=1&from=%s&to=%s&timezone=browser", + from1.Format(time.RFC3339), to1.Format(time.RFC3339)), + } + newShortURL1, err := service.CreateShortURL(ctx, testUser, cmd1) + require.NoError(t, err) + require.NotNil(t, newShortURL1) + require.NotEmpty(t, newShortURL1.Signature) + + cmd2 := &dtos.CreateShortURLCmd{ + Path: fmt.Sprintf("d/test-dashboard?orgId=1&from=%s&to=%s&timezone=browser", + from2.Format(time.RFC3339), to2.Format(time.RFC3339)), + } + newShortURL2, err := service.CreateShortURL(ctx, testUser, cmd2) + require.NoError(t, err) + require.NotNil(t, newShortURL2) + + // Should return the same short URL due to time normalization + // Both represent "now-6h to now" so they should have the same signature + require.Equal(t, newShortURL1.Uid, newShortURL2.Uid) + require.Equal(t, newShortURL1.Signature, newShortURL2.Signature) + }) + + t.Run("Time normalization: old absolute timestamps are not normalized", func(t *testing.T) { + service := ShortURLService{SQLStore: &sqlStore{db: store}} + + ctx := context.Background() + + // Old timestamps (more than 24 hours ago) should not be normalized + oldTime := time.Now().Add(-48 * time.Hour) + oldFrom := oldTime.Add(-6 * time.Hour) + oldTo := oldTime + + cmd1 := &dtos.CreateShortURLCmd{ + Path: fmt.Sprintf("d/test-dashboard?orgId=1&from=%s&to=%s", + oldFrom.Format(time.RFC3339), oldTo.Format(time.RFC3339)), + } + newShortURL1, err := service.CreateShortURL(ctx, testUser, cmd1) + require.NoError(t, err) + require.NotNil(t, newShortURL1) + require.NotEmpty(t, newShortURL1.Signature) + + // Create another with slightly different old timestamp + oldFrom2 := oldTime.Add(-6 * time.Hour).Add(1 * time.Minute) + oldTo2 := oldTime.Add(1 * time.Minute) + + cmd2 := &dtos.CreateShortURLCmd{ + Path: fmt.Sprintf("d/test-dashboard?orgId=1&from=%s&to=%s", + oldFrom2.Format(time.RFC3339), oldTo2.Format(time.RFC3339)), + } + newShortURL2, err := service.CreateShortURL(ctx, testUser, cmd2) + require.NoError(t, err) + require.NotNil(t, newShortURL2) + + // Old timestamps should NOT be normalized, so they should have different signatures + require.NotEqual(t, newShortURL1.Uid, newShortURL2.Uid) + require.NotEqual(t, newShortURL1.Signature, newShortURL2.Signature) + }) + + t.Run("Time normalization: relative time ranges are preserved", func(t *testing.T) { + service := ShortURLService{SQLStore: &sqlStore{db: store}} + + ctx := context.Background() + + cmd1 := &dtos.CreateShortURLCmd{ + Path: "d/test-dashboard?orgId=1&from=now-6h&to=now&timezone=browser", + } + newShortURL1, err := service.CreateShortURL(ctx, testUser, cmd1) + require.NoError(t, err) + require.NotNil(t, newShortURL1) + require.NotEmpty(t, newShortURL1.Signature) + + cmd2 := &dtos.CreateShortURLCmd{ + Path: "d/test-dashboard?orgId=1&from=now-6h&to=now&timezone=browser", + } + newShortURL2, err := service.CreateShortURL(ctx, testUser, cmd2) + require.NoError(t, err) + require.NotNil(t, newShortURL2) + + // Relative time ranges should produce the same signature + require.Equal(t, newShortURL1.Uid, newShortURL2.Uid) + require.Equal(t, newShortURL1.Signature, newShortURL2.Signature) + }) + + t.Run("Different orgs: same path creates different short URLs", func(t *testing.T) { + service := ShortURLService{SQLStore: &sqlStore{db: store}} + + ctx := context.Background() + + user1 := &user.SignedInUser{UserID: 1, OrgID: 1} + user2 := &user.SignedInUser{UserID: 2, OrgID: 2} + + cmd := &dtos.CreateShortURLCmd{ + Path: "mock/path?test=true", + } + newShortURL1, err := service.CreateShortURL(ctx, user1, cmd) require.NoError(t, err) require.NotNil(t, newShortURL1) require.NotEmpty(t, newShortURL1.Uid) - newShortURL2, err := service.CreateShortURL(ctx, user, cmd) + newShortURL2, err := service.CreateShortURL(ctx, user2, cmd) require.NoError(t, err) require.NotNil(t, newShortURL2) require.NotEmpty(t, newShortURL2.Uid) + // Should create different short URLs for different orgs require.NotEqual(t, newShortURL1.Uid, newShortURL2.Uid) + require.NotEqual(t, newShortURL1.Signature, newShortURL2.Signature) require.Equal(t, newShortURL1.Path, newShortURL2.Path) }) + t.Run("Custom UID bypasses de-duplication", func(t *testing.T) { + service := ShortURLService{SQLStore: &sqlStore{db: store}} + + ctx := context.Background() + + cmd1 := &dtos.CreateShortURLCmd{ + Path: "mock/path?test=true", + UID: "custom-uid-3", + } + newShortURL1, err := service.CreateShortURL(ctx, testUser, cmd1) + require.NoError(t, err) + require.NotNil(t, newShortURL1) + require.Equal(t, "custom-uid-3", newShortURL1.Uid) + require.NotEmpty(t, newShortURL1.Signature) // Custom UID has signature including UID + + cmd2 := &dtos.CreateShortURLCmd{ + Path: "mock/path?test=true", + UID: "custom-uid-4", + } + newShortURL2, err := service.CreateShortURL(ctx, testUser, cmd2) + require.NoError(t, err) + require.NotNil(t, newShortURL2) + require.Equal(t, "custom-uid-4", newShortURL2.Uid) + require.NotEmpty(t, newShortURL2.Signature) // Custom UID has signature including UID + + // Different UIDs, same path - both created because UID was provided + // Signatures should be different because they include the UID + require.NotEqual(t, newShortURL1.Uid, newShortURL2.Uid) + require.NotEqual(t, newShortURL1.Signature, newShortURL2.Signature) + }) + t.Run("Create URL providing the UID", func(t *testing.T) { service := ShortURLService{SQLStore: &sqlStore{db: store}} @@ -157,7 +345,7 @@ func TestIntegrationShortURLService(t *testing.T) { Path: "mock/path?test=true", UID: "custom-uid", } - newShortURL1, err := service.CreateShortURL(ctx, user, cmd) + newShortURL1, err := service.CreateShortURL(ctx, testUser, cmd) require.NoError(t, err) require.NotNil(t, newShortURL1) require.Equal(t, cmd.UID, newShortURL1.Uid) @@ -172,12 +360,12 @@ func TestIntegrationShortURLService(t *testing.T) { Path: "mock/path?test=true", UID: "custom-uid-2", } - newShortURL1, err := service.CreateShortURL(ctx, user, cmd) + newShortURL1, err := service.CreateShortURL(ctx, testUser, cmd) require.NoError(t, err) require.NotNil(t, newShortURL1) require.Equal(t, cmd.UID, newShortURL1.Uid) - newShortURL2, err := service.CreateShortURL(ctx, user, cmd) + newShortURL2, err := service.CreateShortURL(ctx, testUser, cmd) require.ErrorIs(t, err, shorturls.ErrShortURLConflict) require.Nil(t, newShortURL2) }) diff --git a/pkg/services/shorturls/shorturlimpl/store.go b/pkg/services/shorturls/shorturlimpl/store.go index ad74b269d45..aaa38deb6ed 100644 --- a/pkg/services/shorturls/shorturlimpl/store.go +++ b/pkg/services/shorturls/shorturlimpl/store.go @@ -10,6 +10,7 @@ import ( type store interface { Get(ctx context.Context, user identity.Requester, uid string) (*shorturls.ShortUrl, error) + GetBySignature(ctx context.Context, orgID int64, signature string) (*shorturls.ShortUrl, error) Update(ctx context.Context, shortURL *shorturls.ShortUrl) error Insert(ctx context.Context, shortURL *shorturls.ShortUrl) error Delete(ctx context.Context, cmd *shorturls.DeleteShortUrlCommand) error @@ -40,6 +41,26 @@ func (s sqlStore) Get(ctx context.Context, user identity.Requester, uid string) return &shortURL, nil } +func (s sqlStore) GetBySignature(ctx context.Context, orgID int64, signature string) (*shorturls.ShortUrl, error) { + var shortURL shorturls.ShortUrl + err := s.db.WithDbSession(ctx, func(dbSession *db.Session) error { + exists, err := dbSession.Where("org_id=? AND signature=? AND signature IS NOT NULL", orgID, signature).Get(&shortURL) + if err != nil { + return err + } + if !exists { + return shorturls.ErrShortURLNotFound.Errorf("short URL not found by signature") + } + + return nil + }) + if err != nil { + return nil, err + } + + return &shortURL, nil +} + func (s sqlStore) Update(ctx context.Context, shortURL *shorturls.ShortUrl) error { shortURL.LastSeenAt = getTime().Unix() return s.db.WithTransactionalDbSession(ctx, func(dbSession *db.Session) error { diff --git a/pkg/services/sqlstore/migrations/short_url_mig.go b/pkg/services/sqlstore/migrations/short_url_mig.go index 2bb8e229812..8657f3d3341 100644 --- a/pkg/services/sqlstore/migrations/short_url_mig.go +++ b/pkg/services/sqlstore/migrations/short_url_mig.go @@ -28,4 +28,21 @@ func addShortURLMigrations(mg *Migrator) { mg.AddMigration("alter table short_url alter column created_by type to bigint", NewRawSQLMigration(""). Mysql("ALTER TABLE short_url MODIFY created_by BIGINT;"). Postgres("ALTER TABLE short_url ALTER COLUMN created_by TYPE BIGINT;")) + + // Add signature column for de-duplication + shortURLTable := Table{Name: "short_url"} + mg.AddMigration("add signature column to short_url table", NewAddColumnMigration(shortURLTable, &Column{ + Name: "signature", + Type: DB_Varchar, + Length: 64, + Nullable: true, + })) + + // Add unique index on (org_id, signature) with NULL handling + // PostgreSQL needs a partial unique index to handle NULLs correctly + // MySQL/SQLite allow multiple NULLs in unique indexes, which is fine + mg.AddMigration("add unique index short_url.org_id-signature", NewRawSQLMigration(""). + Postgres("CREATE UNIQUE INDEX IF NOT EXISTS UQE_short_url_org_id_signature ON short_url(org_id, signature) WHERE signature IS NOT NULL;"). + Mysql("CREATE UNIQUE INDEX UQE_short_url_org_id_signature ON short_url(org_id, signature);"). + SQLite("CREATE UNIQUE INDEX IF NOT EXISTS UQE_short_url_org_id_signature ON short_url(org_id, signature);")) } diff --git a/pkg/setting/setting.go b/pkg/setting/setting.go index 885ea5703ec..a89343a0e4a 100644 --- a/pkg/setting/setting.go +++ b/pkg/setting/setting.go @@ -1316,12 +1316,13 @@ func (cfg *Cfg) parseINIFile(iniFile *ini.File) error { cfg.QueryHistoryEnabled = queryHistory.Key("enabled").MustBool(true) shortLinks := iniFile.Section("short_links") - cfg.ShortLinkExpiration = shortLinks.Key("expire_time").MustInt(7) + cfg.ShortLinkExpiration = shortLinks.Key("expire_time").MustInt(-1) if cfg.ShortLinkExpiration > 365 { cfg.Logger.Warn("short_links expire_time must be less than 366 days. Setting to 365 days") cfg.ShortLinkExpiration = 365 } + // -1 means never expire, which is allowed panelsSection := iniFile.Section("panels") cfg.DisableSanitizeHtml = panelsSection.Key("disable_sanitize_html").MustBool(false)