A deeper look into tweaking of ticketing subsystem (and some issues discovered)
(1) By george on 2021-04-04 15:39:31 [link] [source]
I would like to bring up a couple of issues (and questions) related to the customization of the ticketing subsystem. Below is a modeling story backed by exemplary public repositories. These two repositories are just clones of the Wapp repository which I tweaked a little-bit for this demonstration. Wapp's repo was chosen because it is a well-formed repository that is relatively small and that already contains some tickets (that are dispersed in time).
Lets imagine that a developer has created a custom VIEW within the ticket schema definition of a public repo:
CREATE VIEW fx_tktlastucard AS
SELECT DISTINCT
ticket.tkt_id AS tkt_id,
event.user AS lastucard
FROM
ticket JOIN ticketchng USING( tkt_id, tkt_mtime )
JOIN event ON tkt_rid = event.objid ;
The purpose of this view is to help extract U-cards from the ticket change artifacts.
Lets assume that on Monday's morning a user Joe (and maybe also dozens of other users) cloned this repository to a local machine:
f clone https://testbed.sha3.link/fossil/tickets-monday/tkt
cd tkt
The URL above is real and the command should work
(in April 2021, most of the time, fingers crossed).
For brevity it is assumed that the commands bellow are run from
this just created directory tkt
(that holds a checkout).
By running f ui
Joe will immediately notice that a handy todo
link within main-menu ("▶" - a right-pointing triangle)
does not yield the expected result. Instead it shows a report that
does not exist at the developer's (upstream) repository.
It turns out that upon cloning Fossil creates a report "All Tickets" unless the so-named report already exists upstream. Fossil puts this report in the beginning of the reports list. This shifts numeration of all the other reports, which breaks custom skins that rely on a certain reports numbering.
There is another 🐞
Lets imagine that (unlike this example) UserIDs within
the repository need to be fairly long
(e.g. of the form firstname.familyname yielding 10-30 characters)
while the number of users is tractable (say below 30).
In this case, for the "handy todo" report it might be enough to
just show a couple of letters from the UserID. Lets assume that on
Monday's evening a developer implements this by adding the third
column "lu
" into the SELECT
clause of the fx_tktlastucard
view:
CASE max(instr(user,'.'),instr(user,'_'))
WHEN 0 THEN substr(user,1,2)
ELSE
substr(user,1,1) ||
substr(user,max(instr(user,'.'),instr(user,'_'))+1,1)
END AS lu
Afterwards updates the report's SQL-query accordingly, deploys all these, and at the midnight publishs a Technote urging users to pull the updated configuration for the tickets-tracker.
The Tuesday's arrival to Joe can be modeled by the command:
f remote https://testbed.sha3.link/fossil/tickets-tuesday
Now, if (say on Tuesday's morning) Joe executes:
f configuration pull ticket --overwrite
then the following would be shown:
Database error: table fx_tktlastucard already exists
Outside of the IT-community typically just a few users are willing to
fiddle with the database in the terminal. Therefore "the party comes
to the support team". 💃
Perhaps it comes quite mercilessly because on the users' machines now
not just a single report, but the whole repositories are kinda broken
(due to missing ticket
and ticketchng
tables):
f ui --page timeline?y=a
warning: SQLITE_ERROR(1): no such table: ticket in
"SELECT status='Closed' ....
😱
f rebuild
0.0% complete...
SQLITE_ERROR(1): table fx_tktlastucard already exists in ....
😱😱
To actually fix the broken state of the repository (its ticket subsystem) the user should execute:
f sql "DROP VIEW fx_tktlastucard"
f configuration pull ticket --overwrite
The resolution of this issue is relatively easy, but is barely
discoverable and may put headache and frustration on inexperienced
users. It is particularly unobvious that the --overwrite
option is
actually required both to fix the reports' numbering and to pull
updated configuration.
Questions and Suggestions
The policy around the "All Tickets" report should be clarified. I'm curious if others treat the existing behavior as an issue? And if so, then what they expect to be the correct behavior? Maybe make cloning a bit smarter (i.e. don't create this report unconditionally)?
I think that the ticket schema restrictions should be relaxed. It should be possible to DROP an object (a table/view/index/trigger) that could have been created earlier within the same restrictions. For now Fossil allows to CREATE "fx_*" tables, views and indices. I think Fossil should also allow to DROP them. Please consider the patch. I've already suggested this before. In fact I usually deploy a version of Fossil that is patched accordingly. Also Stephan wrote a nice comment on the topic.
An alternative solution I can think of is to find and DROP all objects that match
ticket*
pattern before the executing of a (user-provided) ticket schema (its SQL-definition).Some SQLite-related questions remain:
- Is INDEX dropped automatically when (any of) the underline table is dropped?
- Should we also allow to create (and drop) TRIGGERs (see the next item for rationale)?
- Are TRIGGERs dropped automatically?
The
fx_tktlastucard
VIEW described above may be appropriate (?) for this particular case, but what if the task is more complicated? It looks like shooting in the foot to include (disk|mem|cpu)-expensive SQL-query into a very frequently requested page (reads prevail over updates). There is a need for some other approach (other than VIEWS). It must be robust but should not put any burden on the shoulders of plain users who may be not at all technical people; say those who (if equipped with a manual) can not do much above installing fossil and thenf clone
,f ui
,f sync
.
For the time being Fossil have /tktsetup_change and /xfersetup_ticket. If I understood correctly (?) the former is invoked only bysubmit_ticket
Th1 command, while the later - on any ticket change (pages, commands, xfers) but requires th1-hooks to be enabled.
Another option is to put TRIGGER(s) onticket
and/orticketchng
table. Ticket schema seems like the most appropriate place to define them, provided that the ticket schema authorizer were adjusted to permit them. This option seems more lightweight and "integrated". It would be nice to know if somebody have already tried the "triggers route" and what was the experience.I would be thankful for thoughts and advice on this.
Is the above definition of
fx_tktlastucard
actually correct? Does it really deliver U-card from the latest ticket change artifact? These lines suggest so, right? Also the comparison oftkt_mtime
seems very fragile to me (floating point numbers are tested for absolute equality, right?).Can
fx_tktlastucard
view be rewritten in a smarter way that runs faster? Maybe using correlated sub-queries? And also deliver the earliest U-card, not just the latest.A curious person can not learn SQL-query of the report that consumes
fx_tktlastucard
view through the repository's public WebUI, because TktFmt capability is not provided for theanonymous
. But this is sort of confusion, because he/she can clone this repo and learn it afterwards. I kindly invite comments and feedback into a dedicated thread.What are the flaws and potential drawbacks of running a repo without predefined
login
column (or any of its substitutes) in theticket
andticketchng
tables? IMHO it brings a fair amount of confusion into/tkthistory
(why two equal values? are they always equal? if so, why this redundancy?).Actually I think that this field should be deprecated (and removed) from the default ticket configuration. Tuesday's repo runs without it. And because of the login J-cards it produces "
Untracked field login
" at/tkthistory
. A quick view through the repositories of Fossil, SQLite and Tcl did not find much deviations of U-card from thelogin
column ofticketchng
table. The last such artifact dates back to 2014-03-04 (7 years old!)
So what are the opinions on this question?What about introducing/reserving within the
ticket
and/orticketchng
tables a few documented optional columns which (if present) would be recognized by Fossil and populated automatically? I'm primarily thinking about U-cards (because these are unconditionally required) but perhaps someone else can point another good candidates?If the holly
anonymous
is allowed to alter tickets via the WWW then is it a good idea to put into U-cards something other than "anonymous" (e.g. "anonymous@example.com")?
(2) By Stephan Beal (stephan) on 2021-04-05 03:24:27 in reply to 1 [link] [source]
(HUGE HUGE SNIP)
It turns out that upon cloning Fossil creates a report "All Tickets" unless the so-named report already exists upstream. Fossil puts this report in the beginning of the reports list. This shifts numeration of all the other reports, which breaks custom skins that rely on a certain reports numbering.
Ticket reports should "probably" get a new field in which a random, stable name gets stored (say, 8 hash digits, or a hash of the creation time, or similar). Exposing unstable/mutable IDs in the UI should really be avoided (in the same way that we never expose blob.rid
values via links).
If the holly anonymous is allowed to alter tickets via the WWW then is it a good idea to put into U-cards something other than "anonymous" (e.g. "anonymous@example.com")?
Why? Both forms are equally anonymous to a person reading the ticket but they are two distinctly different names for fossil's purposes. The name "anonymous" is "magical" for fossil and is compared as a literal string in many places in the code. e.g. we cannot query the timeline or the /reports
for changes made by an anonymous user edits if the name is not exactly anonymous
.
As to the rest of it: i don't use the ticketing system enough to have been bothered by any of the points you bring up. The ticketing system was one of fossil's first major features and could certainly use some modernization. It would surprise me greatly if there were any objections to you applying some love and attention to it, provided you don't break sqlite's tickets db along the way.
(3) By Richard Hipp (drh) on 2021-06-12 22:51:04 in reply to 1 [link] [source]
The patch is a security problem. It cannot be merge to trunk.
With this patch in place a malicious actor could trick a victim into syncing their ticket setup, which would then cause SQLite to delete irreplaceable data, such as the BLOB table or the USER table.
(4.1) By george on 2021-06-13 11:31:08 edited from 4.0 in reply to 3 [link] [source]
delete irreplaceable data, such as the BLOB table or the USER table
I understand the concern.
However I don't understand how that is possible.
Could you please elaborate the attack scenario a little bit.
I think that lines 420-424 and 434-438
clearly disallow DROP unless a table's name starts
either with "ticket" or "fx_".
"ticket" and "ticketchng" tables are repopulated when repository is rebuilt.
"fx_" tables can indeed be deleted. But those tables aren't synced
from the upstream, these are created and populated locally.
Users who customized their repositories that far might be pretty
knowledgeable and "hard-to-trick".
Am I wrong?
UPDATE:
If the possibility of DROPs in the "fx_" namespace seems to be
completely unacceptable then what about adding another namespace,
say "ftx_" (for Fossil's Tickets eXtensions)?
Tables/views/indicies in the "ftx_" namespace would enjoy all the access
rights relevant for the "fx_" namespace, and also
may be DROPed from within a ticket's schema definition.