Fossil Forum

Timeline RSS feed broken
Login

Timeline RSS feed broken

Timeline RSS feed broken

(1) By Florian Balmer (florian.balmer) on 2022-06-24 10:41:00 [link] [source]

By check-in [cb651568fb], the timeline RSS feed is broken and only shows entries about wiki pages, regardless of the y= query parameter.

I'm struggling to understand the accompanying changes regarding y= handling, as the check-in comment only mentioned wiki and forum entries, but so far I don't think there was any intent to make the RSS feed more restrictive?

My expectation is that it should be possible to query the RSS feed from scripts, i.e. without any cookie and/or login handling, and the feed should contain whatever is also publicly visible on the main website for a random (non-authenticated) user?

I'm having a short time window to look into this, but as I just said I'm having trouble understanding the permission-related changes in the linked commit, so probably won't be able to come up with anything useful, right now. Any fixes (or explanations) appreciated, thanks!

(2) By Daniel Dumitriu (danield) on 2022-06-24 12:54:56 in reply to 1 [link] [source]

It seems though as timeline.rss currently functions on the Fossil site itself, which uses a later build:

    <item>
      <title>In the Windows application manifest [...]
      <dc:creator>florian</dc:creator> [...]
    </item>

(3) By Stephan Beal (stephan) on 2022-06-24 13:27:38 in reply to 1 [link] [source]

By check-in [cb651568fb], the timeline RSS feed is broken and only shows entries about wiki pages, regardless of the y= query parameter.

Like Daniel, i can't reproduce that on any site i've tested, including this one.

I'm struggling to understand the accompanying changes regarding y= handling, as the check-in comment only mentioned wiki and forum entries, but so far I don't think there was any intent to make the RSS feed more restrictive?

The older code did not account for forum posts and could leak forum posts to users who lack permission to read them. For example, this snippet from the older version:

else if( !g.perm.RdTkt ){
      assert( !g.perm.RdTkt && g.perm.Read && g.perm.RdWiki );
      blob_append(&bSQL, " AND event.type!='t'", -1);
    }

"event.typt!='t'" implicitly includes forum posts but was only intended to show checkins and wiki pages, and it doesn't account for forum permissions.

My expectation is that it should be possible to query the RSS feed from scripts, i.e. without any cookie and/or login handling, and the feed should contain whatever is also publicly visible on the main website for a random (non-authenticated) user?

The new code has the exact same restrictions as before for all types except forum posts, where it now accounts for the possibility that the forum is private. The restructuring simply reformulates the same permission checks in a more readable (to me!) format so that it was easier to add the event.type='f' case.

(4) By Florian Balmer (florian.balmer) on 2022-06-25 04:54:46 in reply to 1 [link] [source]

Thanks Daniel and Stephan for your replies!

I'm sorry! It's y=all (or y=<NULL>) that doesn't work as expected any more:

My script to load the RSS feed auto-appends (or replaces) the y=all query parameter, so I was fooled when trying more specific values for y -- which obviously seem to work.

Yesterday, I burnt all my available time with these two (or, three) lines in the mentioned commit:

  • src/rss.c 50, 170: Why is is possible to declare a variable twice with the same name in the same function, and why do compilers allow this?

  • src/rss.c: 102: It took me some time to understand that this is (philosophically) similar to [ x"" = x"$var" ] once used in shell scripts, because SQLite doesn't accept an empty set for the IN operator, so event.type IN ('x') is used by default to match nothing. (But according to the syntax diagram, empty sets are allowed? The IN flow line is 3 lines up from the bottom.)

With recently touched code, I prefer to bring up (what I think may be) problems to the Forum. But I can look into the y=all and y=<NULL> cases tomorrow, hopefully.

(For the same reason, [b3ed9d37d5] was committed to its own branch, so as not to mess with Richard's recent modifications. But I still think listing the unused --sync option in the online help is a bug. And maybe my comment is a bit snarky, but I still believe determining the correct action for the --sync option will be almost as complex as CSS inheritance, due to the many nested layers of settings coming into play.)

(5) By Stephan Beal (stephan) on 2022-06-25 07:55:02 in reply to 4 [source]

I'm sorry! It's y=all (or y=<NULL>) that doesn't work as expected any more:

Ah, that makes sense. i definitely broke that.

Why is is possible to declare a variable twice with the same name in the same function, and why do compilers allow this?

Deeper scopes always shadow higher ones. However, that particular shadow was unintentional and will be renamed. There are compiler flags to tell them to warn about shadowing, as it's normally unintentional.

(But according to the syntax diagram, empty sets are allowed?)

i honestly didn't think to check and simply assumed (without thinking twice about it) that an empty IN is not permitted. i'll remove that 'x' bit.

Thank you for pointing these out - they'll all be patched within the next hour.

(6) By Stephan Beal (stephan) on 2022-06-25 08:24:12 in reply to 5 [link] [source]

i honestly didn't think to check and simply assumed (without thinking twice about it) that an empty IN is not permitted. i'll remove that 'x' bit.

Actually: the 'x' is there for a completely different reason and either needs to stay or about 4x as much code needs to be added to replace it. Without it, we require more code to know whether a comma is needed after each entry in the list. Having the 'x' there means that all other entries can add a comma without having to know if there was a previous/next entry.

Ah, that makes sense. i definitely broke that.

That y=all breakage was caused by:

    if( g.perm.Read ){
      blob_append(&bSQL, "'ci',", 4);
    }

That 4 should have been a 5. i took this opportunity to replace those appends with blob_append_sql(), in any case, as that's the preferred approach for building SQL-holding blobs.

Fixed in fossil:8903d1ebe079f091.

(7) By Daniel Dumitriu (danield) on 2022-06-25 08:28:32 in reply to 5 [link] [source]

Deeper scopes always shadow higher ones.

Shadowing can be mean :) I think it makes sense to run a build with -Wshadow to check for possible surprises.

(8) By Florian Balmer (florian.balmer) on 2022-06-26 09:04:27 in reply to 6 [link] [source]

Actually: the 'x' is there for a completely different reason ...

Yeah, it's come to my mind after my last post that printing an array-like structure with separators between each of the entries is simpler when appending a dummy element.

So let's leave this nifty puzzle in-place to give the people who might one day implement event.type "x" a little challenge! ;-) It's good you didn't add a comment, or it would be way too easy, otherwise! ;-)

(To be fair, the idea of abusing "x" as an invalid event.type was already there before your current changes.)

... or about 4x as much code ... is needed ...

My gut feeling is that more one-time initialization code to build the SQL query string pays back by fewer parsing work for the SQLite engine and fewer bytecode instructions to execute the query. But it's just a gut feeling, and not considering required developer time ... ;-)

Thanks for the fixes! I just tested them right now, and everything works as expected!