Skip to content
This repository has been archived by the owner on Oct 3, 2022. It is now read-only.

cinema #1113

Closed
wants to merge 8 commits into from
Closed

cinema #1113

wants to merge 8 commits into from

Conversation

hat0r
Copy link
Contributor

@hat0r hat0r commented Sep 23, 2019 •
edited

Feature nobody asked for but everyone will love, or ignore, or whatever.
Allows clients to push videos to feed and watch them together with others.

Features:
- separate feed per thread
- ability to toggle it in board options, defaultly it's off
- time synchronization every 5 seconds
- democratic skip
- status on the top bar
- list with pending videos
- supports youtube, invidious and raw videos but other sources can be fairly easily added
- domains for raw videos must be in whitelist to prevent IP leakage to some shady servers,
whitelist is stored in server configuration and can be updated via fronted
- since commands are extracted from posts there is no additional spam prevention

Youtube support is in separate commit since it requires external script and I don't know if you want that (and that feature at all), it's fetched only when it's needed though.

Demonstration:

@bakape
Copy link
Owner

bakape commented Sep 24, 2019 via email

Chiiruno
Chiiruno suggested changes Sep 25, 2019
View reviewed changes
Copy link
Contributor

@Chiiruno Chiiruno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a fun feature, but there's some issues I see at the surface at least.

Dockerfile Show resolved Hide resolved
websockets/feeds/cinema.go Show resolved Hide resolved
client/util/time.ts Show resolved Hide resolved
db/config.go Outdated Show resolved Hide resolved
docs/installation.md Show resolved Hide resolved
static/src/lang/nl_NL/common.json Show resolved Hide resolved
util/exec_binary.go Show resolved Hide resolved
@hat0r hat0r force-pushed the feature/cinema branch 2 times, most recently from 90c6383 to d8f4d54 Compare September 25, 2019 20:49
@bakape
Copy link
Owner

bakape commented Sep 26, 2019 via email

@bakape
Copy link
Owner

bakape commented Sep 29, 2019

I have a feeling this will be spammed to all hell, because !push is public. What do you think about needing to post with a staff tag (at least janitor) for push to take effect?

@Chiiruno
Copy link
Contributor

Wouldn't that invalidate the whole point if only staff could queue up videos?

@Chiiruno
Copy link
Contributor

https://github.com/bakape/meguca/pull/1113/files#diff-005d84183c08ef0f4deb6eb678de789bR55-R62 does prevent multiple pushes with pushedAlready.
Perhaps locking it behind captcha would be prudent?

@Chiiruno
Copy link
Contributor

Also why !push and !skip instead of #push and #skip?

bakape
bakape suggested changes Sep 29, 2019
View reviewed changes
Copy link
Owner

@bakape bakape left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Server-crashing thread-safety issues and DoS attack vectors.

db/config.go Outdated
@@ -135,7 +135,7 @@ func updateConfigs(_ string) error {
config.Set(conf)
mlog.Update()

return util.Parallel(templates.Recompile, auth.LoadCaptchaServices)
return util.Parallel(templates.Recompile, auth.LoadCaptchaServices, common.ComputeVars)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please maintain an 80 column width.

db/config.go Outdated
c.Created, c.DefaultCSS, c.Title, c.Notice, c.Rules,
pq.StringArray(c.Eightball),
c.Flags, c.NSFW, c.RbText, c.Pyu, c.Created, c.DefaultCSS,
c.Title, c.Notice, c.Rules, c.CinemaEnabled, pq.StringArray(c.Eightball),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please maintain an 80 column width.

common/vars.go Outdated
cinemaSources := []string{}

invidiousUrlRegexpStr := `https?:\/\/(?:www\.)?invidio\.us\/watch(?:.*&|\?)v=(.+)(?:\?.+)*`
InvidiousUrlRegexp = regexp.MustCompile(invidiousUrlRegexpStr)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to recompute this on every function run. Move the initialization to the declaration.

common/vars.go
cinemaSources = append(cinemaSources, invidiousUrlRegexpStr)

youtubeUrlRegexpStr := `https?:\/\/(?:www\.)?youtube\.com\/watch(?:.*&|\?)v=(.+)(?:\?.+)*`
YoutubeUrlRegexp = regexp.MustCompile(youtubeUrlRegexpStr)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to recompute this on every function run. Move the initialization to the declaration.

common/vars.go Outdated
ln := lang.Get()
CinemaPushCommandRegexp = regexp.MustCompile(`^!` + ln.UI["cinemaPush"] +
` (`+ strings.Join(cinemaSources, `|`) +`)$`)
CinemaSkipCommandRegexp = regexp.MustCompile(`^!` + ln.UI["cinemaSkip"] + `$`)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is run on config update and thus CinemaPushCommandRegexp and CinemaSkipCommandRegexp need a sync.RWMutex guard. Since ensuring somebody uses a mutex across packages is not likely (somebody will forget eventually), please create getter functions that maintain the atomicity of grabbing a pointer to the regexp.

websockets/feeds/cinema.go
f.sendToAll(f.envelopSyncTime())
}
case url := <-f.push:
cv, err := parseUrl(url)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will block the entire thread's loop on external I/O - unacceptable, because this function can be triggered by anybody. Easy DoS. Please add a requirement to be the current board's staff to push videos.

websockets/feeds/cinema.go Outdated
// start next
if len(f.playlist) > 0 {
cv := f.playlist[0]
f.videoTimer = time.NewTimer(time.Duration(cv.Duration)*time.Millisecond).C
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stop the previous timer to reduce goroutine usage, if any.

websockets/feeds/feed.go Outdated
@@ -221,10 +227,19 @@ func (f *Feed) sendIPCount() {
}
ips[ip] = struct{}{}
}

cf, ok := feeds.cinemaFeeds[f.id]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No mutex guard.

websockets/feeds/feeds.go Show resolved Hide resolved
websockets/feeds/feed.go Outdated
@@ -65,6 +67,8 @@ type Feed struct {
moderatePost chan moderationMessage
// Let sent sync counter
lastSyncCount syncCount
// trigger sending IP count/cinema status, fired from cinema feed loop
sendIPCountChan chan int
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A Feed and and a cinemaFeed don't have linked lifetimes, thus it is possible for this channel to cause blocked and thus leaked goroutines. Please use the global SendTo function instead and delete this channel.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what is your solution here. SendTo buffers a message to all clients, but in order to make it I need information from main feed (IP, IP active count), which can only be obtained from its goroutine.

Maybe guard those two values with mutex in a separate struct as I've done in cinema feed and make sendIPCount thread safe. Another option is to send the same message from cinema feed with values from main feed set to null, then client won't update them or entirely move cinema status to a different websocket message.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right - SendTo() is not what is needed. A better solution would be adding a reasonable buffer to sendIPCountChan, say 16, and then using a non-blocking select send in the cinema feed. In case it's unclear, I mean this:

select {
case f.sendIPCountChan <- count:
default: 
// Log error
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, sounds good, if I understand correctly that additional select is to catch error when the buffer is full?

@bakape
Copy link
Owner

bakape commented Sep 29, 2019

@Chiiruno See the comments. Making !push available to non-staff is a DoS vulnerability, as it blocks the central cinema loop. If you want to watch cinema, be staff or make your own board for it. I'm open to other suggestions, that solve the problems.

@Chiiruno
Copy link
Contributor

@bakape I did, is guarding it behind captchouli not enough?
Perhaps captchouli per individual push and a set limit for pushes per IP per hour/day would be even better?

@bakape
Copy link
Owner

bakape commented Sep 29, 2019

@Chiiruno The Captchouli integration currently does not tie a solved captcha to an explicit action permission. It only checks, if a captcha had been solved recently enough. That is fine for stuff like server-local resource usage. but not fine for external I/O. You could just solve one captcha and then spam pushes to DoS the cinema.

@Chiiruno
Copy link
Contributor

@bakape Well, from what I read, you can't do multiple pushes in one post, so even with that current blockage, it's only 3 posts (and thus 3 pushes) that you can DoS with until the next captchouli.
That said, my idea was to have the server check the post, if it contains 1 !push, then ask for another captcha from client to submit, and if it contains more than one, disregard all !push in the post and submit rest of post, or disregard the entire post.

@Chiiruno
Copy link
Contributor

Could also have !push automatically increase spam score to require another captcha from IP.

@bakape
Copy link
Owner

bakape commented Sep 30, 2019 via email

@hat0r
Copy link
Contributor Author

hat0r commented Sep 30, 2019

Regarding that DoS on cinema push I thought about per goroutine push cooldown, e.g. 30 seconds and limit on playlist length, but captcha is more elegant. Playlist length limit would still be reasonable thing to do though.

Also why !push and !skip instead of #push and #skip?

I wanted to distinguish that it's not a word command but line command. First sign could go along with the command word to the language config, since they are already there

@Chiiruno
Copy link
Contributor

but captcha is more elegant

All you should need to do is increase spam score to the captcha limit for that IP every !push and it should be good.

Playlist length limit would still be reasonable thing to do though

Playlist should definitely have a length limit, but it should only really be for extreme numbers like above... maybe 2048? To make sure we don't clog up things in the server with too much playlist data over time.

I wanted to distinguish that it's not a word command but line command. First sign could go along with the command word to the language config, since they are already there

That makes sense, although !skip doesn't really apply there, since it's a word command, but I also don't think it should be a # command either considering it's tied to the cinema, which uses !.
I'm guessing lat doesn't really mind so it's all good.

@hat0r
Copy link
Contributor Author

hat0r commented Sep 30, 2019

So: incrementing captcha score by spamDetectionThreshold after push and a playlist limit, what do you think @bakape?

@hat0r
Copy link
Contributor Author

hat0r commented Sep 30, 2019 •
edited

!skip doesn't really apply there, since it's a word command

It is in that sense that there can't be anything else in its line.

@bakape
Copy link
Owner

bakape commented Sep 30, 2019

So: incrementing captcha score by spamDetectionThreshold after push and a playlist limit, what do you think @bakape?

2 * spamDetectionThreshold, because the threshold is a moving target and we need something that would not decay as readily. A Post with nothing but a push should trigger a captcha on any subsequent post made within a few minutes.

I'm fine with the ! syntax.

@bakape
Copy link
Owner

bakape commented Oct 1, 2019 via email

@hat0r
Copy link
Contributor Author

hat0r commented Oct 20, 2019

above issues should be solved now

@Chiiruno
Copy link
Contributor

Chiiruno commented Oct 20, 2019 •
edited

git pull --rebase upstream master

@hat0r
Copy link
Contributor Author

hat0r commented Oct 20, 2019

that's right

@bakape
Copy link
Owner

bakape commented Oct 28, 2019 via email

@hat0r
Copy link
Contributor Author

hat0r commented Oct 28, 2019

sure

@Chiiruno
Copy link
Contributor

Chiiruno commented Nov 7, 2019

Will readdress once new version is up and working.
Or will implement as we go.

@Chiiruno Chiiruno closed this Nov 7, 2019
@Chiiruno Chiiruno mentioned this pull request Nov 7, 2019
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bakape bakape bakape requested changes

@Chiiruno Chiiruno Chiiruno requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@hat0r @bakape @Chiiruno @Madouura