fix(browser): attribute navigated content to post-redirect URL

jkyberneees · jkyberneees · commit 7164d5f75178 · 2026-07-18T21:32:50.000+02:00
browser_navigate previously used the originally requested URL for the
snapshot URL, untrusted-content source, and relative-link click
resolution. After an HTTP redirect this attributed attacker content to
the pre-redirect domain and broke relative links.

- Use resp.Request.URL (final post-redirect URL) when building the
  snapshot and wrapping content.
- Add TestBrowser_Navigate_FollowsRedirect regression test covering
  URL attribution and relative-link click resolution after a redirect.
- Document the change in SECURITY.md and AGENTS.md.

Closes L-4 in sec_findings.md.
diff --git a/AGENTS.md b/AGENTS.md
@@ -165,6 +165,7 @@ Layered prompt-injection / approval-fatigue defenses. Full reference: [docs/SECU
 - **`parallel_shell` context + process-group kill** (`cmd/odek/perf_tools.go`) — commands now run via `exec.CommandContext` bound to the agent context, in their own process group. Cancellation or timeout kills the whole group (negative PID), so `sh -c 'sleep 3600 &'` cannot leave orphaned children. Per-command timeouts are also capped at 30 minutes.
 - **`batch_patch` trusted-class propagation** (`cmd/odek/perf_tools.go`) — `batch_patch` now passes its cached `trustedClasses` to `CheckOperation`, matching `write_file` and `patch`. A trusted `local_write` class is honored across all patches in the batch instead of re-prompting per patch.
 - **Browser link URL wrapping** (`cmd/odek/browser_tool.go`) — interactive element text was already wrapped as untrusted, but link URLs in `clickableRef.URL` were returned raw. They are now wrapped too, while an unexported `rawURL` is kept for internal click resolution.
+- **Browser post-redirect URL attribution** (`cmd/odek/browser_tool.go`) — `browser_navigate` now uses `resp.Request.URL` (the final post-redirect URL) for the snapshot URL, the untrusted-content source, and relative-link click resolution, instead of attributing content to the original requested URL.
 - **Telegram message length by UTF-16 code units** (`internal/telegram/handler.go`) — `MaxMsgLength` is enforced using UTF-16 code-unit counting, matching Telegram's own limits. Multi-byte UTF-8 characters (e.g. emoji) no longer pass the local check while being rejected by Telegram.
 - **Telegram restart marker permissions** (`cmd/odek/telegram.go`) — `~/.odek/restart.json` is now written with `0600` instead of `0644`, preventing local users from reading the list of active chat IDs.
 - **Telegram singleton flock lock** (`cmd/odek/telegram.go` + `internal/flock`) — the Telegram bot now uses an advisory `flock` on `~/.odek/telegram.lock` instead of a PID file probed with signals. This removes the non-Linux path where a planted PID could cause odek to kill an arbitrary process.
diff --git a/cmd/odek/browser_tool.go b/cmd/odek/browser_tool.go
@@ -242,7 +242,12 @@ func (t *browserTool) doNavigate(rawURL string) (string, error) {
 	}
 
 	html := string(body)
-	snap := parseHTML(t.toolCtx(), html, rawURL, resp.StatusCode)
+	// Use the post-redirect URL for attribution and relative-link resolution.
+	finalURL := rawURL
+	if resp.Request != nil && resp.Request.URL != nil {
+		finalURL = resp.Request.URL.String()
+	}
+	snap := parseHTML(t.toolCtx(), html, finalURL, resp.StatusCode)
 
 	// Store in state. Keep a persistent copy of the snapshot for current; the
 	// local variable's address would otherwise escape to the heap implicitly.
diff --git a/cmd/odek/browser_tool_test.go b/cmd/odek/browser_tool_test.go
@@ -58,6 +58,69 @@ func TestBrowser_Navigate(t *testing.T) {
 	}
 }
 
+func TestBrowser_Navigate_FollowsRedirect(t *testing.T) {
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch r.URL.Path {
+		case "/redirect":
+			http.Redirect(w, r, "/target", http.StatusFound)
+		case "/target":
+			w.Write([]byte(`<html><head><title>Target</title></head><body>
+				<h1>Target Page</h1>
+				<a href="/final">Final</a>
+			</body></html>`))
+		case "/final":
+			w.Write([]byte(`<html><head><title>Final</title></head><body>
+				<h1>Final Page</h1>
+			</body></html>`))
+		default:
+			w.WriteHeader(http.StatusNotFound)
+		}
+	}))
+	defer ts.Close()
+
+	b := newTestBrowserTool()
+	result := callJSON(t, b, `{"action":"navigate","url":"`+ts.URL+`/redirect"}`)
+	var r struct {
+		Title    string `json:"title"`
+		Content  string `json:"content"`
+		URL      string `json:"url"`
+		Elements []struct {
+			Ref string `json:"ref"`
+			URL string `json:"url"`
+		} `json:"elements"`
+		Error string `json:"error,omitempty"`
+	}
+	mustUnmarshal(t, result, &r)
+
+	if r.Error != "" {
+		t.Fatalf("navigate error: %s", r.Error)
+	}
+	if r.URL != ts.URL+"/target" {
+		t.Errorf("url = %q, want %q (post-redirect)", r.URL, ts.URL+"/target")
+	}
+	if !strings.Contains(r.Content, "Target Page") {
+		t.Errorf("content missing target page text: %q", r.Content)
+	}
+
+	// Verify the relative link on the post-redirect page resolves against the
+	// final URL, not the original redirect URL.
+	if len(r.Elements) == 0 {
+		t.Fatal("expected at least one element")
+	}
+	clickResult := callJSON(t, b, `{"action":"click","ref":"`+r.Elements[0].Ref+`"}`)
+	var click struct {
+		URL   string `json:"url"`
+		Error string `json:"error,omitempty"`
+	}
+	mustUnmarshal(t, clickResult, &click)
+	if click.Error != "" {
+		t.Fatalf("click error: %s", click.Error)
+	}
+	if click.URL != ts.URL+"/final" {
+		t.Errorf("click resolved to %q, want %q", click.URL, ts.URL+"/final")
+	}
+}
+
 func TestBrowser_Navigate_InvalidURL(t *testing.T) {
 	b := newTestBrowserTool()
 	result := callJSON(t, b, `{"action":"navigate","url":"not-a-valid-url"}`)
diff --git a/docs/SECURITY.md b/docs/SECURITY.md
@@ -536,6 +536,10 @@ In addition, a friction counter tracks approvals per class. After 3 approvals of
 
 `browser` already wrapped page title, content, and interactive-element text as untrusted, but the `URL` field of each `clickableRef` was emitted as a raw JSON string. A hostile page could set `href` to a `javascript:`, `data:`, or attacker-controlled URL containing instruction-like text. The `URL` field is now wrapped as untrusted before serialization. An unexported `rawURL` preserves the original value so internal click resolution continues to work.
 
+### 36a. Browser post-redirect URL attribution
+
+`browser_navigate` previously attributed the fetched content to the URL originally requested, even when the HTTP client followed redirects. An attacker could point a reputable-looking URL at a redirector and have the resulting page content labeled with the reputable domain, and relative links on the landing page resolved against the wrong origin. `browser_navigate` now uses `resp.Request.URL` (the final post-redirect URL) for the snapshot URL, the untrusted-content wrapper source, and click resolution.
+
 ### 37. Telegram message length by UTF-16 code units
 
 Telegram's message and caption limits are defined in UTF-16 code units, but `internal/telegram/handler.go` was using `len(msg.Text)` and `len(msg.Caption)`, which count UTF-8 bytes. Emoji and other supplementary-plane characters consume 4 UTF-8 bytes but 2 UTF-16 code units, so emoji-heavy messages could pass the local check and then be rejected by Telegram. The handler now counts UTF-16 code units via `utf16Len`.
