Skip to content

Ask markup and redirect findings what their values hold and where they lead - #35

Merged
tauanbinato merged 2 commits into
mainfrom
markup-redirect-confirms
Sep 27, 2026
Merged

tauanbinato merged 2 commits into
mainfrom
markup-redirect-confirms

Conversation

@tauanbinato

Copy link
Copy Markdown
Contributor

Follow-up to #34 for three more of the vaultwarden reviews that were wrong, and the same causes elsewhere in the corpus.

The confirm request #34 added for path findings now asks three questions, and a finding reads only the one for its kind:

  • Markup: what do the values hold where they enter the markup: already escaped, percent-encoded or URL-serialized; typed; the program's own; or raw text? vaultwarden's hibp_breach percent-encodes the username with form_urlencoded::byte_serialize before building the link.
  • Redirect: where can the targets lead? A fixed path such as /admin first keeps them on the site; origin + next with no slash between them does not. vaultwarden's admin login redirects to admin_path() followed by the form value, and shiori's to its login page with the path only in the query.

A finding whose one concern is markup or a redirect is asked this, unless it's a consider on parameters, which keeps its existing "values" Choice. Leaning (0.50) toward the harmless options makes it a note, with its own message.

Measured on the 38 corpus projects with path, markup or redirect findings. Only confirm requests were asked, about $0.01.

Result
vaultwarden hibp_breach (markup), post_admin_login (redirect), shiori getBookmark (redirect), all labeled wrong review → note (harmless 0.56, 0.68, 0.58)
express examples/auth login redirect to the Referer consider → note (now labeled debatable; the model misread its shape, but a note fits)
26 markup findings labeled right stay (harmless ≤ 0.22)
6 redirect findings labeled right stay (own site ≤ 0.44)

The first wording of the redirect options ("the program's own origin and a slash") let chatbot-ui's real open redirect (requestUrl.origin + next, so next=@evil.com leaves the site) read as staying on the site at 0.63. The options now write out both forms: that finding answers "anywhere" at 0.93, and none of the right ones move.

With #34, vaultwarden's security reviews go from 16 to 9: the 6 right ones, plus backup_db (its error type's Display, which a macro generates, only writes the program's messages), decode_token_claims, and the debatable email-token log. Those are single cases, left alone rather than tuned to.

Also: the self-check flagged injection_wording once it grew, so the page-script and confirmed-note wordings are now their own functions. INJECTION rule version bumped.

Tests (including one for markup and redirect, checked to fail without the rule), clippy and cargo +1.90.0 check --locked pass; the self-check reports no review or consider.

…ape, split injection wording

The redirect options now name the forms: a fixed path such as /admin
first stays on the site; origin + next with no slash between them does
not. Offered only "origin and a slash", chatbot-ui's origin + next
read as staying on the site at 0.63; with the forms it is 0.93 anywhere.

On the 38 corpus projects with path, markup or redirect findings:
vaultwarden's hibp_breach and admin login redirect and shiori's login
redirect, all labeled wrong, are notes; the 26 markup and 6 redirect
findings labeled right put at most 0.22 and 0.44 on the harmless
options.
@tauanbinato
tauanbinato merged commit 37af140 into main Sep 27, 2026
9 checks passed
@tauanbinato
tauanbinato deleted the markup-redirect-confirms branch September 27, 2026 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant