Skip to content

Commit cefc2a1

Browse files
jkyberneeesjkyberneees
authored
fix(browser): attribute navigated content to post-redirect URL (L-4)
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.
1 parent f8ecfe4 commit cefc2a1

4 files changed

Lines changed: 74 additions & 1 deletion

File tree

‎AGENTS.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,7 @@ Layered prompt-injection / approval-fatigue defenses. Full reference: [docs/SECU
165165
- **`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.
166166
- **`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.
167167
- **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.
168+
- **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.
168169
- **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.
169170
- **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.
170171
- **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.

‎cmd/odek/browser_tool.go‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -242,7 +242,12 @@ func (t *browserTool) doNavigate(rawURL string) (string, error) {
242242
}
243243

244244
html := string(body)
245-
snap := parseHTML(t.toolCtx(), html, rawURL, resp.StatusCode)
245+
// Use the post-redirect URL for attribution and relative-link resolution.
246+
finalURL := rawURL
247+
if resp.Request != nil && resp.Request.URL != nil {
248+
finalURL = resp.Request.URL.String()
249+
}
250+
snap := parseHTML(t.toolCtx(), html, finalURL, resp.StatusCode)
246251

247252
// Store in state. Keep a persistent copy of the snapshot for current; the
248253
// local variable's address would otherwise escape to the heap implicitly.

‎cmd/odek/browser_tool_test.go‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,69 @@ func TestBrowser_Navigate(t *testing.T) {
5858
}
5959
}
6060

61+
func TestBrowser_Navigate_FollowsRedirect(t *testing.T) {
62+
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
63+
switch r.URL.Path {
64+
case "/redirect":
65+
http.Redirect(w, r, "/target", http.StatusFound)
66+
case "/target":
67+
w.Write([]byte(`<html><head><title>Target</title></head><body>
68+
<h1>Target Page</h1>
69+
<a href="/final">Final</a>
70+
</body></html>`))
71+
case "/final":
72+
w.Write([]byte(`<html><head><title>Final</title></head><body>
73+
<h1>Final Page</h1>
74+
</body></html>`))
75+
default:
76+
w.WriteHeader(http.StatusNotFound)
77+
}
78+
}))
79+
defer ts.Close()
80+
81+
b := newTestBrowserTool()
82+
result := callJSON(t, b, `{"action":"navigate","url":"`+ts.URL+`/redirect"}`)
83+
var r struct {
84+
Title string `json:"title"`
85+
Content string `json:"content"`
86+
URL string `json:"url"`
87+
Elements []struct {
88+
Ref string `json:"ref"`
89+
URL string `json:"url"`
90+
} `json:"elements"`
91+
Error string `json:"error,omitempty"`
92+
}
93+
mustUnmarshal(t, result, &r)
94+
95+
if r.Error != "" {
96+
t.Fatalf("navigate error: %s", r.Error)
97+
}
98+
if r.URL != ts.URL+"/target" {
99+
t.Errorf("url = %q, want %q (post-redirect)", r.URL, ts.URL+"/target")
100+
}
101+
if !strings.Contains(r.Content, "Target Page") {
102+
t.Errorf("content missing target page text: %q", r.Content)
103+
}
104+
105+
// Verify the relative link on the post-redirect page resolves against the
106+
// final URL, not the original redirect URL.
107+
if len(r.Elements) == 0 {
108+
t.Fatal("expected at least one element")
109+
}
110+
clickResult := callJSON(t, b, `{"action":"click","ref":"`+r.Elements[0].Ref+`"}`)
111+
var click struct {
112+
URL string `json:"url"`
113+
Error string `json:"error,omitempty"`
114+
}
115+
mustUnmarshal(t, clickResult, &click)
116+
if click.Error != "" {
117+
t.Fatalf("click error: %s", click.Error)
118+
}
119+
if click.URL != ts.URL+"/final" {
120+
t.Errorf("click resolved to %q, want %q", click.URL, ts.URL+"/final")
121+
}
122+
}
123+
61124
func TestBrowser_Navigate_InvalidURL(t *testing.T) {
62125
b := newTestBrowserTool()
63126
result := callJSON(t, b, `{"action":"navigate","url":"not-a-valid-url"}`)

‎docs/SECURITY.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -536,6 +536,10 @@ In addition, a friction counter tracks approvals per class. After 3 approvals of
536536

537537
`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.
538538

539+
### 36a. Browser post-redirect URL attribution
540+
541+
`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.
542+
539543
### 37. Telegram message length by UTF-16 code units
540544

541545
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`.

0 commit comments

Comments
 (0)