e8d3633 fix: address review feedback for crash-page safety and history batching · dumber

M internal/application/usecase/navigate.go +13 -1


@@ -32,6 +32,10 @@ type historyRecord struct {
32	32		visits int
33	33		}
34	34
	35	+	type historyVisitDeltaIncrementer interface {
	36	+	IncrementVisitCountBy(ctx context.Context, url string, delta int) error
	37	+	}
	38	+
35	39		type paneHistoryState struct {
36	40		lastRawURL string
37	41		lastCanonicalURL string

@@ -258,7 +262,15 @@ func (uc *NavigateUseCase) persistHistory(ctx context.Context, record historyRec
258	262		}
259	263
260	264		if existing != nil {
261		-	for i := 0; i < maxInt(1, record.visits); i++ {
	265	+	delta := maxInt(1, record.visits)
	266	+	if deltaWriter, ok := uc.historyRepo.(historyVisitDeltaIncrementer); ok {
	267	+	if err := deltaWriter.IncrementVisitCountBy(ctx, record.url, delta); err != nil {
	268	+	log.Warn().Err(err).Str("url", record.url).Int("delta", delta).Msg("failed to increment visit count by delta")
	269	+	return
	270	+	}
	271	+	return
	272	+	}
	273	+	for i := 0; i < delta; i++ {
262	274		if err := uc.historyRepo.IncrementVisitCount(ctx, record.url); err != nil {
263	275		log.Warn().Err(err).Str("url", record.url).Msg("failed to increment visit count")
264	276		return

 	visits int
 }
+type historyVisitDeltaIncrementer interface {
+	IncrementVisitCountBy(ctx context.Context, url string, delta int) error
+}
 type paneHistoryState struct {
 	lastRawURL       string
 	lastCanonicalURL string
 	}
 	if existing != nil {
-		for i := 0; i < maxInt(1, record.visits); i++ {
+		delta := maxInt(1, record.visits)
+		if deltaWriter, ok := uc.historyRepo.(historyVisitDeltaIncrementer); ok {
+			if err := deltaWriter.IncrementVisitCountBy(ctx, record.url, delta); err != nil {
+				log.Warn().Err(err).Str("url", record.url).Int("delta", delta).Msg("failed to increment visit count by delta")
+				return
+			}
+			return
+		}
+		for i := 0; i < delta; i++ {
 			if err := uc.historyRepo.IncrementVisitCount(ctx, record.url); err != nil {
 				log.Warn().Err(err).Str("url", record.url).Msg("failed to increment visit count")
 				return

M internal/bootstrap/session.go +1 -1


@@ -98,7 +98,7 @@ func StartBrowserSession(
98	98		persistFn := func(persistCtx context.Context) error {
99	99		persistMu.Do(func() {
100	100		if lockDir != "" {
101		-	abruptSessions, abruptErr := markAbruptExits(lockDir, now.UTC())
	101	+	abruptSessions, abruptErr := markAbruptExits(lockDir, time.Now().UTC())
102	102		if abruptErr != nil {
103	103		logger.Warn().Err(abruptErr).Msg("failed to check abrupt exit markers")
104	104		} else {


@@ -98,7 +98,7 @@ func StartBrowserSession(
98		persistFn := func(persistCtx context.Context) error {	98		persistFn := func(persistCtx context.Context) error {
99		persistMu.Do(func() {	99		persistMu.Do(func() {
100		if lockDir != "" {	100		if lockDir != "" {
101	-	abruptSessions, abruptErr := markAbruptExits(lockDir, now.UTC())	101	+	abruptSessions, abruptErr := markAbruptExits(lockDir, time.Now().UTC())
102		if abruptErr != nil {	102		if abruptErr != nil {
103		logger.Warn().Err(abruptErr).Msg("failed to check abrupt exit markers")	103		logger.Warn().Err(abruptErr).Msg("failed to check abrupt exit markers")
104		} else {	104		} else {

M internal/infrastructure/persistence/sqlite/history_repo.go +14 -0


@@ -311,6 +311,20 @@ func (r *historyRepo) IncrementVisitCount(ctx context.Context, url string) error
311	311		return r.queries.IncrementVisitCount(ctx, url)
312	312		}
313	313
	314	+	func (r *historyRepo) IncrementVisitCountBy(ctx context.Context, url string, delta int) error {
	315	+	// Skip incrementing about:blank - it should always have visit_count = 1
	316	+	if url == aboutBlankURL {
	317	+	return nil
	318	+	}
	319	+	if delta <= 0 {
	320	+	delta = 1
	321	+	}
	322	+	return r.queries.IncrementVisitCountByDelta(ctx, sqlc.IncrementVisitCountByDeltaParams{
	323	+	VisitCount: sql.NullInt64{Int64: int64(delta), Valid: true},
	324	+	Url: url,
	325	+	})
	326	+	}
	327	+
314	328		func (r *historyRepo) Delete(ctx context.Context, id int64) error {
315	329		return r.queries.DeleteHistoryByID(ctx, id)
316	330		}


@@ -311,6 +311,20 @@ func (r *historyRepo) IncrementVisitCount(ctx context.Context, url string) error
311	return r.queries.IncrementVisitCount(ctx, url)	311		return r.queries.IncrementVisitCount(ctx, url)
312	}	312		}
313		313
		314	+	func (r *historyRepo) IncrementVisitCountBy(ctx context.Context, url string, delta int) error {
		315	+	// Skip incrementing about:blank - it should always have visit_count = 1
		316	+	if url == aboutBlankURL {
		317	+	return nil
		318	+	}
		319	+	if delta <= 0 {
		320	+	delta = 1
		321	+	}
		322	+	return r.queries.IncrementVisitCountByDelta(ctx, sqlc.IncrementVisitCountByDeltaParams{
		323	+	VisitCount: sql.NullInt64{Int64: int64(delta), Valid: true},
		324	+	Url: url,
		325	+	})
		326	+	}
		327	+
314	func (r *historyRepo) Delete(ctx context.Context, id int64) error {	328		func (r *historyRepo) Delete(ctx context.Context, id int64) error {
315	return r.queries.DeleteHistoryByID(ctx, id)	329		return r.queries.DeleteHistoryByID(ctx, id)
316	}	330		}

M internal/infrastructure/persistence/sqlite/lazy_repos.go +17 -0


@@ -114,6 +114,23 @@ func (r *LazyHistoryRepository) IncrementVisitCount(ctx context.Context, url str
114	114		return r.repo.IncrementVisitCount(ctx, url)
115	115		}
116	116
	117	+	func (r *LazyHistoryRepository) IncrementVisitCountBy(ctx context.Context, url string, delta int) error {
	118	+	if err := r.init(ctx); err != nil {
	119	+	return err
	120	+	}
	121	+	if deltaRepo, ok := r.repo.(interface {
	122	+	IncrementVisitCountBy(context.Context, string, int) error
	123	+	}); ok {
	124	+	return deltaRepo.IncrementVisitCountBy(ctx, url, delta)
	125	+	}
	126	+	for i := 0; i < delta; i++ {
	127	+	if err := r.repo.IncrementVisitCount(ctx, url); err != nil {
	128	+	return err
	129	+	}
	130	+	}
	131	+	return nil
	132	+	}
	133	+
117	134		func (r *LazyHistoryRepository) Delete(ctx context.Context, id int64) error {
118	135		if err := r.init(ctx); err != nil {
119	136		return err


@@ -114,6 +114,23 @@ func (r *LazyHistoryRepository) IncrementVisitCount(ctx context.Context, url str
114	return r.repo.IncrementVisitCount(ctx, url)	114		return r.repo.IncrementVisitCount(ctx, url)
115	}	115		}
116		116
		117	+	func (r *LazyHistoryRepository) IncrementVisitCountBy(ctx context.Context, url string, delta int) error {
		118	+	if err := r.init(ctx); err != nil {
		119	+	return err
		120	+	}
		121	+	if deltaRepo, ok := r.repo.(interface {
		122	+	IncrementVisitCountBy(context.Context, string, int) error
		123	+	}); ok {
		124	+	return deltaRepo.IncrementVisitCountBy(ctx, url, delta)
		125	+	}
		126	+	for i := 0; i < delta; i++ {
		127	+	if err := r.repo.IncrementVisitCount(ctx, url); err != nil {
		128	+	return err
		129	+	}
		130	+	}
		131	+	return nil
		132	+	}
		133	+
117	func (r *LazyHistoryRepository) Delete(ctx context.Context, id int64) error {	134		func (r *LazyHistoryRepository) Delete(ctx context.Context, id int64) error {
118	if err := r.init(ctx); err != nil {	135		if err := r.init(ctx); err != nil {
119	return err	136		return err

M internal/infrastructure/persistence/sqlite/queries/history.sql +5 -0


@@ -19,6 +19,11 @@ UPDATE history
19	19		SET visit_count = visit_count + 1, last_visited = CURRENT_TIMESTAMP
20	20		WHERE url = ?;
21	21
	22	+	-- name: IncrementVisitCountByDelta :exec
	23	+	UPDATE history
	24	+	SET visit_count = visit_count + ?, last_visited = CURRENT_TIMESTAMP
	25	+	WHERE url = ?;
	26	+
22	27		-- name: SearchHistory :many
23	28		SELECT * FROM history
24	29		WHERE url LIKE '%' \|\| ? \|\| '%' OR title LIKE '%' \|\| ? \|\| '%'


@@ -19,6 +19,11 @@ UPDATE history
19	SET visit_count = visit_count + 1, last_visited = CURRENT_TIMESTAMP	19		SET visit_count = visit_count + 1, last_visited = CURRENT_TIMESTAMP
20	WHERE url = ?;	20		WHERE url = ?;
21		21
		22	+	-- name: IncrementVisitCountByDelta :exec
		23	+	UPDATE history
		24	+	SET visit_count = visit_count + ?, last_visited = CURRENT_TIMESTAMP
		25	+	WHERE url = ?;
		26	+
22	-- name: SearchHistory :many	27		-- name: SearchHistory :many
23	SELECT * FROM history	28		SELECT * FROM history
24	WHERE url LIKE '%' \|\| ? \|\| '%' OR title LIKE '%' \|\| ? \|\| '%'	29		WHERE url LIKE '%' \|\| ? \|\| '%' OR title LIKE '%' \|\| ? \|\| '%'

M internal/infrastructure/persistence/sqlite/sqlc/history.sql.go +16 -0


@@ -410,6 +410,22 @@ func (q *Queries) IncrementVisitCount(ctx context.Context, url string) error {
410	410		return err
411	411		}
412	412
	413	+	const IncrementVisitCountByDelta = `-- name: IncrementVisitCountByDelta :exec
	414	+	UPDATE history
	415	+	SET visit_count = visit_count + ?, last_visited = CURRENT_TIMESTAMP
	416	+	WHERE url = ?
	417	+	`
	418	+
	419	+	type IncrementVisitCountByDeltaParams struct {
	420	+	VisitCount sql.NullInt64 `json:"visit_count"`
	421	+	Url string `json:"url"`
	422	+	}
	423	+
	424	+	func (q *Queries) IncrementVisitCountByDelta(ctx context.Context, arg IncrementVisitCountByDeltaParams) error {
	425	+	_, err := q.db.ExecContext(ctx, IncrementVisitCountByDelta, arg.VisitCount, arg.Url)
	426	+	return err
	427	+	}
	428	+
413	429		const SearchHistory = `-- name: SearchHistory :many
414	430		SELECT id, url, title, favicon_url, visit_count, last_visited, created_at FROM history
415	431		WHERE url LIKE '%' \|\| ? \|\| '%' OR title LIKE '%' \|\| ? \|\| '%'


@@ -410,6 +410,22 @@ func (q *Queries) IncrementVisitCount(ctx context.Context, url string) error {
410	return err	410		return err
411	}	411		}
412		412
		413	+	const IncrementVisitCountByDelta = `-- name: IncrementVisitCountByDelta :exec
		414	+	UPDATE history
		415	+	SET visit_count = visit_count + ?, last_visited = CURRENT_TIMESTAMP
		416	+	WHERE url = ?
		417	+	`
		418	+
		419	+	type IncrementVisitCountByDeltaParams struct {
		420	+	VisitCount sql.NullInt64 `json:"visit_count"`
		421	+	Url string `json:"url"`
		422	+	}
		423	+
		424	+	func (q *Queries) IncrementVisitCountByDelta(ctx context.Context, arg IncrementVisitCountByDeltaParams) error {
		425	+	_, err := q.db.ExecContext(ctx, IncrementVisitCountByDelta, arg.VisitCount, arg.Url)
		426	+	return err
		427	+	}
		428	+
413	const SearchHistory = `-- name: SearchHistory :many	429		const SearchHistory = `-- name: SearchHistory :many
414	SELECT id, url, title, favicon_url, visit_count, last_visited, created_at FROM history	430		SELECT id, url, title, favicon_url, visit_count, last_visited, created_at FROM history
415	WHERE url LIKE '%' \|\| ? \|\| '%' OR title LIKE '%' \|\| ? \|\| '%'	431		WHERE url LIKE '%' \|\| ? \|\| '%' OR title LIKE '%' \|\| ? \|\| '%'

M internal/infrastructure/persistence/sqlite/sqlc/querier.go +1 -0


@@ -64,6 +64,7 @@ type Querier interface {
64	64		GetTotalSessionStatesSize(ctx context.Context) (interface{}, error)
65	65		GetZoomLevel(ctx context.Context, domain string) (ZoomLevel, error)
66	66		IncrementVisitCount(ctx context.Context, url string) error
	67	+	IncrementVisitCountByDelta(ctx context.Context, arg IncrementVisitCountByDeltaParams) error
67	68		InsertSession(ctx context.Context, arg InsertSessionParams) error
68	69		IsWhitelisted(ctx context.Context, domain string) (int64, error)
69	70		ListZoomLevels(ctx context.Context) ([]ZoomLevel, error)


@@ -64,6 +64,7 @@ type Querier interface {
64	GetTotalSessionStatesSize(ctx context.Context) (interface{}, error)	64		GetTotalSessionStatesSize(ctx context.Context) (interface{}, error)
65	GetZoomLevel(ctx context.Context, domain string) (ZoomLevel, error)	65		GetZoomLevel(ctx context.Context, domain string) (ZoomLevel, error)
66	IncrementVisitCount(ctx context.Context, url string) error	66		IncrementVisitCount(ctx context.Context, url string) error
		67	+	IncrementVisitCountByDelta(ctx context.Context, arg IncrementVisitCountByDeltaParams) error
67	InsertSession(ctx context.Context, arg InsertSessionParams) error	68		InsertSession(ctx context.Context, arg InsertSessionParams) error
68	IsWhitelisted(ctx context.Context, domain string) (int64, error)	69		IsWhitelisted(ctx context.Context, domain string) (int64, error)
69	ListZoomLevels(ctx context.Context) ([]ZoomLevel, error)	70		ListZoomLevels(ctx context.Context) ([]ZoomLevel, error)

M internal/infrastructure/webkit/scheme_handler.go +33 -10


@@ -207,12 +207,29 @@ func crashOriginalURI(requestURI string) string {
207	207		return strings.TrimSpace(parsed.Query().Get("url"))
208	208		}
209	209
	210	+	func crashReloadTarget(originalURI string) string {
	211	+	originalURI = strings.TrimSpace(originalURI)
	212	+	if originalURI == "" {
	213	+	return ""
	214	+	}
	215	+	parsed, err := url.Parse(originalURI)
	216	+	if err != nil {
	217	+	return ""
	218	+	}
	219	+	switch strings.ToLower(parsed.Scheme) {
	220	+	case "http", "https":
	221	+	if parsed.Host == "" {
	222	+	return ""
	223	+	}
	224	+	return parsed.String()
	225	+	default:
	226	+	return ""
	227	+	}
	228	+	}
	229	+
210	230		func buildCrashPageHTML(originalURI string) string {
211	231		escapedOriginal := html.EscapeString(originalURI)
212		-	reloadAction := "window.location.reload();"
213		-	if originalURI != "" {
214		-	reloadAction = "window.location.href = targetUrl;"
215		-	}
	232	+	escapedReloadTarget := html.EscapeString(crashReloadTarget(originalURI))
216	233		return fmt.Sprintf(`<!DOCTYPE html>
217	234		<html lang="en">
218	235		<head>

@@ -290,21 +307,27 @@ func buildCrashPageHTML(originalURI string) string {
290	307		<p>The current page was interrupted. You can reload it to continue browsing.</p>
291	308		<div class="url">%s</div>
292	309		<div class="actions">
293		-	<button class="primary" id="reload-btn">Reload page</button>
	310	+	<button class="primary" id="reload-btn" data-target="%s">Reload page</button>
294	311		<button class="secondary" id="stay-btn">Stay on this page</button>
295	312		</div>
296	313		</div>
297	314		<script>
298		-	const targetUrl = %q;
299		-	document.getElementById('reload-btn').addEventListener('click', function() {
300		-	%s
	315	+	const reloadButton = document.getElementById('reload-btn');
	316	+	const targetUrl = (reloadButton.getAttribute('data-target') \|\| '').trim();
	317	+	reloadButton.addEventListener('click', function() {
	318	+	if (targetUrl) {
	319	+	window.location.href = targetUrl;
	320	+	return;
	321	+	}
	322	+	window.location.reload();
301	323		});
302	324		document.getElementById('stay-btn').addEventListener('click', function() {
303		-	window.location.href = 'dumb://home/crash?url=' + encodeURIComponent(targetUrl);
	325	+	const crashURI = targetUrl ? targetUrl : '';
	326	+	window.location.href = 'dumb://home/crash?url=' + encodeURIComponent(crashURI);
304	327		});
305	328		</script>
306	329		</body>
307		-	</html>`, escapedOriginal, originalURI, reloadAction)
	330	+	</html>`, escapedOriginal, escapedReloadTarget)
308	331		}
309	332
310	333		func (h DumbSchemeHandler) buildConfigResponse(cfg config.Config) *SchemeResponse {

 	return strings.TrimSpace(parsed.Query().Get("url"))
 }
+func crashReloadTarget(originalURI string) string {
+	originalURI = strings.TrimSpace(originalURI)
+	if originalURI == "" {
+		return ""
+	}
+	parsed, err := url.Parse(originalURI)
+	if err != nil {
+		return ""
+	}
+	switch strings.ToLower(parsed.Scheme) {
+	case "http", "https":
+		if parsed.Host == "" {
+			return ""
+		}
+		return parsed.String()
+	default:
+		return ""
+	}
+}
 func buildCrashPageHTML(originalURI string) string {
 	escapedOriginal := html.EscapeString(originalURI)
-	reloadAction := "window.location.reload();"
+	escapedReloadTarget := html.EscapeString(crashReloadTarget(originalURI))
-	if originalURI != "" {
-		reloadAction = "window.location.href = targetUrl;"
-	}
 	return fmt.Sprintf(`<!DOCTYPE html>
 <html lang="en">
 <head>
         <p>The current page was interrupted. You can reload it to continue browsing.</p>
         <div class="url">%s</div>
         <div class="actions">
-            <button class="primary" id="reload-btn">Reload page</button>
+            <button class="primary" id="reload-btn" data-target="%s">Reload page</button>
             <button class="secondary" id="stay-btn">Stay on this page</button>
         </div>
     </div>
     <script>
-        const targetUrl = %q;
+        const reloadButton = document.getElementById('reload-btn');
-        document.getElementById('reload-btn').addEventListener('click', function() {
+        const targetUrl = (reloadButton.getAttribute('data-target') || '').trim();
-            %s
+        reloadButton.addEventListener('click', function() {
+            if (targetUrl) {
+                window.location.href = targetUrl;
+                return;
+            }
+            window.location.reload();
         });
         document.getElementById('stay-btn').addEventListener('click', function() {
-            window.location.href = 'dumb://home/crash?url=' + encodeURIComponent(targetUrl);
+            const crashURI = targetUrl ? targetUrl : '';
+            window.location.href = 'dumb://home/crash?url=' + encodeURIComponent(crashURI);
         });
     </script>
 </body>
-</html>`, escapedOriginal, originalURI, reloadAction)
+</html>`, escapedOriginal, escapedReloadTarget)
 }
 func (h *DumbSchemeHandler) buildConfigResponse(cfg *config.Config) *SchemeResponse {

M internal/infrastructure/webkit/scheme_handler_test.go +13 -1


@@ -16,7 +16,7 @@ func TestCrashOriginalURI(t *testing.T) {
16	16
17	17		func TestBuildCrashPageHTMLIncludesReloadTarget(t *testing.T) {
18	18		body := buildCrashPageHTML("https://example.com/foo?a=1&b=2")
19		-	assert.Contains(t, body, "window.location.href = targetUrl;")
	19	+	assert.Contains(t, body, `data-target="https://example.com/foo?a=1&b=2"`)
20	20		assert.Contains(t, body, "https://example.com/foo?a=1&b=2")
21	21		}
22	22

@@ -25,6 +25,18 @@ func TestBuildCrashPageHTMLFallbackReloadWithoutTarget(t *testing.T) {
25	25		assert.Contains(t, body, "window.location.reload();")
26	26		}
27	27
	28	+	func TestBuildCrashPageHTMLRejectsUnsafeReloadScheme(t *testing.T) {
	29	+	body := buildCrashPageHTML("javascript:alert(1)")
	30	+	assert.Contains(t, body, `data-target=""`)
	31	+	}
	32	+
	33	+	func TestBuildCrashPageHTMLEscapesScriptBreakoutPayload(t *testing.T) {
	34	+	payload := `https://example.com/?q=</script><script>alert(1)</script>`
	35	+	body := buildCrashPageHTML(payload)
	36	+	assert.NotContains(t, body, "</script><script>alert(1)</script>")
	37	+	assert.Contains(t, body, "</script><script>alert(1)</script>")
	38	+	}
	39	+
28	40		func TestRegisterDefaultsIncludesCrashHandler(t *testing.T) {
29	41		handler := NewDumbSchemeHandler(context.Background())
30	42		require.NotNil(t, handler)

 func TestBuildCrashPageHTMLIncludesReloadTarget(t *testing.T) {
 	body := buildCrashPageHTML("https://example.com/foo?a=1&b=2")
-	assert.Contains(t, body, "window.location.href = targetUrl;")
+	assert.Contains(t, body, `data-target="https://example.com/foo?a=1&amp;b=2"`)
 	assert.Contains(t, body, "https://example.com/foo?a=1&amp;b=2")
 }
 	assert.Contains(t, body, "window.location.reload();")
 }
+func TestBuildCrashPageHTMLRejectsUnsafeReloadScheme(t *testing.T) {
+	body := buildCrashPageHTML("javascript:alert(1)")
+	assert.Contains(t, body, `data-target=""`)
+}
+func TestBuildCrashPageHTMLEscapesScriptBreakoutPayload(t *testing.T) {
+	payload := `https://example.com/?q=</script><script>alert(1)</script>`
+	body := buildCrashPageHTML(payload)
+	assert.NotContains(t, body, "</script><script>alert(1)</script>")
+	assert.Contains(t, body, "&lt;/script&gt;&lt;script&gt;alert(1)&lt;/script&gt;")
+}
 func TestRegisterDefaultsIncludesCrashHandler(t *testing.T) {
 	handler := NewDumbSchemeHandler(context.Background())
 	require.NotNil(t, handler)