Fossil Forum

Forum
Login

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

  1. 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)?

  2. 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?
  3. 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 then f 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 by submit_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) on ticket and/or ticketchng 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.

  4. 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 of tkt_mtime seems very fragile to me (floating point numbers are tested for absolute equality, right?).

  5. 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.

  6. 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 the anonymous. 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.

  7. What are the flaws and potential drawbacks of running a repo without predefined login column (or any of its substitutes) in the ticket and ticketchng 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 the login column of ticketchng table. The last such artifact dates back to 2014-03-04 (7 years old!)
    So what are the opinions on this question?

  8. What about introducing/reserving within the ticket and/or ticketchng 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?

  9. 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.