What should the default value for the "vuln-report" setting be?

The vuln-report setting determines what Fossil does when it encounters a TH1 script that violates taint confinement rules. In the current prototype, the default value is "log". That means that existing custom TH1 scripts that happen to have XSS or SQL-injection vulnerabilities will continue to work as before; they will simple cause a new message to be written into the error log (if the error-log is enabled). I chose this default to minimize disruption. But, I wonder if I ought not make the default value "block" to actually try to prevent XSS/SQL-injection vulnerabilities? People running their own Fossil repos with customized TH1 might be inconvenienced by this choice if their custom scripts violate taint confinement. But a simple work-around to get them going again is just:

fossil all set vuln-report log

What do y'all think? What should the default value of vuln-report be in the upcoming 2.26 release?

Maybe for the first run of fossil ui after updating to latest release, show an error in stderr or on the web interface which alerts the user about the potential harm of not setting block if the default remains log.

Maybe for the first run of fossil ui after updating to latest release ...

That would only apply on a per-repo basis, and many repos/clones never interact with "fossil ui". Every user would see this when running "ui" for the first time on every one of their repos, which may cause more end-user disruption than quietly enabling blocking. Only users who have customized skins or tickets are expected to be affected, so other users need not be bothered about it.

What do y'all think? What should the default value of vuln-report be in the upcoming 2.26 release?

It arguably needs to be "block" by default. Those with heavy customizations would benefit from it breaking loudly rather than continuing on quietly if their skin or ticket config trips a new warning. Of course, it's never fun to go back and fix something which "always just worked," but improving security on/integrity of internet-facing sites is a cause worth breaking things for.

We know the XSS threat is real and is easy to unwittingly introduce¹, and it would be a shame to have the containment improvements not be enabled by default. My sites have been running with "block" since the 21st but, lacking any custom skins or tickets, they're comfortably out of any expected "blast radius."

Of course, that's not a technical argument for blocking by default, but there may be no technical argument for or against it.

1. ^{^} For those who didn't catch this a couple of months ago: sqlite.org/wasm, which uses a skin heavily customized by me, was the first such victim we're aware of.

block seems a reasonable default for me.

but not fatal as default, that can be quite "fatal":

If you happen to have a repository with skins created by fossil earlier than 2025-04-19 22:15 (as of now basically everbody I guess), this leads to an infinite loop because of the <img src="$logo_image_url" border="0" alt="$project_name"> in header.

possible TH1 XSS vulnerability due to tainted inline variable: "project_name"
while in /home
...

over and over again, browser never shows anything.

Had to kill the fossil ui process and set vuln-report back to block on the command line

Good catch! Should be fixed now on trunk.

adding the following to a skin header to check which variables are considered taintet in skins

<th1>
foreach v [info vars] {
  if [array exists $v] {} else {
    puts "[string is tainted [set $v]] $v = [set $v]"
    html "<br/>"
  }
}
</th1>

the answer is project_name and project_description - although these are only changable by admin users.

And a somewhat strange side observation: name is always Login, url is always /login, regardless which page one is on. What is the point of this?

these are only changable by admin users.

And yet, there are Admin users who will change these values to include characters like <, >, &, ', \, and so forth. So I made them tainted to ensure their values always get escaped before rendering.

That is a very good point.

For new repositories it should be block, as the default scripts should be fine. For old repositories it is a question if the possible breakage ist wanted. On the other hand, only few will run custom scripts in the first place, so maybe it can be risked.

In reading the new taint confinement rules. I think there's a missing word which makes the sentence the opposite of what's intended.

Note that the tainted/untainted distinction in strings does make it impossible to introduce XSS and SQL-injections vulnerabilities using poorly-written TH1 scripts; it just makes it more difficult and less likely to happen by accident.

$ f diff -c 3
Index: www/th1.md
==================================================================
--- www/th1.md
+++ www/th1.md
@@ -125,7 +125,7 @@
 vulnerabilities are not *accidentally* added to Fossil when
 custom TH1 scripts for headers or footers or tickets are added to a
 repository.  Note that the tainted/untainted distinction in strings does
-make it impossible to introduce XSS and SQL-injections vulnerabilities
+not make it impossible to introduce XSS and SQL-injections vulnerabilities
 using poorly-written TH1 scripts; it just makes it more difficult and
 less likely to happen by accident.  Developers must still consider the
 security implications TH1 customizations they add to Fossil, and take

Also, I too prefer block to be the default.

Isn't TH1 disabled by default at compile-time? So it only impacts those who have it enabled?

TH1 is always on (it's the basis for the skins) but full Tcl support is a compile-time option.